SAK-49493 Gradebook add support for gradebooks by group #12843
Conversation
bgarciaentornos
changed the title
bgarciaentornos
force-pushed
the
SAK-49493
branch
2 times, most recently
from
41ff009
to
68be104
Compare
bgarciaentornos
force-pushed
the
SAK-49493
branch
3 times, most recently
from
9cd10f4
to
2535d47
Compare
bgarciaentornos
force-pushed
the
SAK-49493
branch
2 times, most recently
from
40e08d3
to
3d8de3a
Compare
| // get all assignments in Gradebook | ||
| List gradebookAssignments = gradingService.getAssignments(contextString, contextString, SortType.SORT_BY_NONE); | ||
|
|
||
| context.put("gradebookAssignmentsSelectedDisabled", gradebookAssignmentsSelectedDisabled); | ||
| context.put("gradebookAssignmentsLabel", gradebookAssignmentsLabel); | ||
| // filtering out those from Samigo | ||
| for (Iterator i = gradebookAssignments.iterator(); i.hasNext(); ) { | ||
| org.sakaiproject.grading.api.Assignment gAssignment = (org.sakaiproject.grading.api.Assignment) i.next(); | ||
| if (!gAssignment.getExternallyMaintained() || gAssignment.getExternallyMaintained() && gAssignment.getExternalAppName().equals(assignmentService.getToolId())) { |
There was a problem hiding this comment.
Probably should simply ask the gardingService for only assignments with a specifc tool id vs getting all assignments and then attempting to filter them, this should be something the gradingService is responsible for
There was a problem hiding this comment.
This is actually the existing "site instance" code, we just left it as it is (and I think that's what should be done). It appears as modified because it's now inside an if block that checks if the feature is enabled, so we added some tabs.
assignment/tool/src/java/org/sakaiproject/assignment/tool/AssignmentAction.java
Outdated
Show resolved
Hide resolved
| // What this part does is search in one gradebook or several (if isGradebookGroupEnabled is active) | ||
| // for the category that contains the assignment. When it finds it, it retrieves the points for the | ||
| // category (category.getPointsForCategory()) and stores them in a HashMap that keeps track | ||
| // of the gradebook + category score. In the end, instead of using the old Double, | ||
| // we will iterate through the HashMap, using the Double from each entry, and perform | ||
| // the same check as before. | ||
| private void buildGradebookPointsMap(String gbUid, String siteId, String assignmentRef, |
There was a problem hiding this comment.
this should likely be something the gradingService should do, remember this feature is about enabling group gradebooks and then ensuring tools integrate with that.
Any time your having to get data from the GradingService and then filter or manipulate it in a tool then you should likely should consider moving the method to the GradingService and have it do that. Remember the tools are users of the GradingService.
assignment/tool/src/java/org/sakaiproject/assignment/tool/AssignmentToolUtils.java
Outdated
Show resolved
Hide resolved
ern
changed the title
config/configuration/bundles/src/bundle/org/sakaiproject/config/bundle/default.sakai.properties
Show resolved
Hide resolved
| @@ -140,6 +140,9 @@ public class Assignment implements Serializable, Comparable<Assignment> { | |||
| @Getter | |||
| @Setter | |||
| private String gradebookId; | |||
| @Getter | |||
| @Setter | |||
| private String gradebookUid; | |||
There was a problem hiding this comment.
so the gradebookId is the same as the site which is by default a UUID, how does gradebookUid differ?
There was a problem hiding this comment.
No, gradebookId is a existing database sequence. It's used some times for direct access, this is just a setter/getter that was missing for some specific use but we're just setting a different value, not adding anything.
|
|
||
| import java.text.MessageFormat; | ||
| import java.util.Locale; | ||
| import java.util.MissingResourceException; | ||
| import java.util.ResourceBundle; | ||
|
|
||
| import org.sakaiproject.util.ResourceLoader; | ||
| //import org.sakaiproject.util.ResourceLoader; |
| } | ||
|
|
||
| /* |
| @SpringBean(name = "org.sakaiproject.user.api.UserDirectoryService") | ||
| protected UserDirectoryService userDirectoryService; | ||
|
|
||
| public static final String GB_PREF_KEY = "GBNG-"; |
There was a problem hiding this comment.
| public static final String GB_PREF_KEY = "GBNG-"; | |
| public static final String GB_PREF_KEY = "GB-"; |
There was a problem hiding this comment.
See next comment
| PreferencesEdit prefsEdit = null; | ||
| try { | ||
| prefsEdit = preferencesService.edit(currentUserId); | ||
| } | ||
| catch (IdUnusedException e) { | ||
| try { | ||
| prefsEdit = preferencesService.add(currentUserId); | ||
| } catch (PermissionException e1) { | ||
| log.warn("setUserGbPreference PermissionException attempting to add prefs for user {}, prefName={}", currentUserId, prefName); | ||
| } catch (IdUsedException e1) { | ||
| log.warn("setUserGbPreference IdUsedException attempting to add prefs for user {}, prefName={}", currentUserId, prefName); | ||
| } | ||
| } catch (PermissionException e) { | ||
| log.warn("setUserGbPreference PermissionException attempting to edit prefs for user {}, prefName={}", currentUserId, prefName); | ||
| } catch (InUseException e) { | ||
| log.warn("setUserGbPreference InUseException attempting to edit prefs for user {}, prefName={}", currentUserId, prefName); | ||
| } | ||
| ResourcePropertiesEdit props = prefsEdit.getPropertiesEdit(GB_PREF_KEY + siteId); | ||
| props.addProperty(prefName, prefValue); | ||
| preferencesService.commit(prefsEdit); |
There was a problem hiding this comment.
check elsewhere in sakai for a better way to make edits to a users preferences
There was a problem hiding this comment.
This was moved from the GradebookNgBusinessService class, as we tried to avoid to get current context or user on that class. But we didn't add or modify that part.
| <!-- Allow multiple instances of this tool within one site --> | ||
| <configuration name="allowMultipleInstances" value="true" /> |
There was a problem hiding this comment.
this seems to contrast with the sakai property?
There was a problem hiding this comment.
Setting false the property will override this.
gradebookng/api/src/main/java/org/sakaiproject/grading/api/MessageHelper.java
Outdated
Show resolved
Hide resolved
|
Thanks a lot for the review @ern , all the comments have been applied or answered |
| @@ -129,12 +136,12 @@ public String archive(final String siteId, final Document doc, final Stack<Eleme | |||
| // <GradebookConfig> | |||
| Element gradebookConfigEl = doc.createElement("GradebookConfig"); | |||
|
|
|||
| Gradebook gradebook = this.gradingService.getGradebook(siteId); | |||
| Gradebook gradebook = this.gradingService.getGradebook(siteId, siteId); | |||
There was a problem hiding this comment.
This is strange to me
Gradebook gradebook = this.gradingService.getGradebook(siteId, siteId);
There was a problem hiding this comment.
This is just for retrocompatibility and an exception case. It is called when archiving the tool and that functionality won't be covered for groups. I guess we could add a one parameter method just for that case but it feels redundant and we preferred it like this.
|
We're reviewing the cypress tests EDIT: @ottenhoff first look and it doesn't really look related to this, rubrics and resubmission stuff but we don't touch that. Just saw this, what previous assignments change is that modification related to?
|
I think it was just getting the tests working again. Maybe you just need to rebase from upstream/master |
bgarciaentornos
force-pushed
the
SAK-49493
branch
from
058f254
to
1ebdf52
Compare
|
I
I rebased it and it seems they're still failing, not sure. We'll take a look but if you find out anything please let us know, thanks |
Co-authored-by: jumarub <[email protected]> Co-authored-by: JuanDavid102 <[email protected]>
bgarciaentornos
force-pushed
the
SAK-49493
branch
from
1ebdf52
to
0d6afdb
Compare
|
closing as there is a new PR #13005 |
Following up the work done last year in the S2U-26 jira under the Unidigital project, this PR overpasses #12338 , which can be closed.
As previously mentioned, this feature provides the option to generate different gradebook instances for selected groups of a site and all this is achieved with no database changes, no gradebook ui modifications and can be disabled via property. The development is based upon lots of methods being overridden with a new parameter to separate the siteId=gradebookUid simplification that was in the previous situation.
In the near future there are some parts that will probably have to be adapted with other announced changes that are coming into the gradebook integration with tools like LTI and assignments. But with such a big piece of work it's necessary to have a first version we can include before the 25 freeze date while we keep working on updating, improving the feature and building upon it.
And also, as @ern mentioned, once everything is settled, a big round of QA can be done to test this and other features modifying the same bits of code.
I also want to thank to all the people of the EDF team who have worked on this development, the S2U group for making it happen, plus the biggest gratitude to @jesusmmp for all the help and his university (Murcia) for sponsoring the second phase.