SAK-49894 WC SakaiGrader distorts group assignment scores with overrides #12469
Conversation
| @@ -679,10 +682,6 @@ private Map<String, Object> submissionToMap(Set<String> activeSubmitters, Assign | |||
| submission.put("grade", assignmentService.getGradeDisplay(as.getGrade(), assignment.getTypeOfGrade(), assignment.getScaleFactor())); | |||
| } | |||
|
|
|||
| if (StringUtils.isNotBlank(as.getGrade())) { | |||
There was a problem hiding this comment.
If mergeOverride is false, will this removal mean that the overall grade for a submission will never be sent?
There was a problem hiding this comment.
The conditional expressed in line 678 matches what I'm proposing be removed starting with line 682. The code in red, if left as-is will present a score of 99.00 to the user as 9900. Unless there's a case here I'm not picking up on, it seems like 682-685 should be removed.
| String grade = assignmentService.getGradeForSubmitter(as, as.getSubmitters().isEmpty() ? null : as.getSubmitters().stream().findAny().get().getSubmitter()); | ||
| if (StringUtils.isNotBlank(grade)) submission.put("grade", grade); | ||
| if (mergeOverride) { | ||
| String grade = assignmentService.getGradeForSubmitter(as, as.getSubmitters().isEmpty() ? null : as.getSubmitters().stream().findAny().get().getSubmitter()); |
There was a problem hiding this comment.
I know this was my code, but this needs a check on findAny. It may return an empty optional. Probably very little chance of that, but it should still be handled.
There was a problem hiding this comment.
I believe line 901 is where the student override score is harnessed to then be erroneously placed in the 'Grade' field (for the overarching score for the group). The following line seals the deal.
Are you contending that as.getSubmitters().stream().findAny().get().getSubmitter() is actually affecting the data that would be passed back via JSON? At a cursory level, the (sole?) function of that chain of calls seems to be to identify a submitter (to then get that submitter's grade, etc.)
There was a problem hiding this comment.
No, what I mean is that findAny can return an empty optional and get() will throw an exception in that case. I should have handled it when I added it.
There was a problem hiding this comment.
Assuming that the only effect (and perhaps intent) of lines 901-902 is to assert as the group's grade a student override, then it would seem that lines 900-903 can be removed along with the removal of other references to 'mergeOverride'. Do you concur? If not, can you clarify what would need to be addressed in a revised commit?
| @@ -11,7 +11,7 @@ export const gradableDataMixin = Base => class extends Base { | |||
| return new Promise(resolve => { | |||
|
|
|||
| // Then, request the full set of data | |||
| const url = `/direct/assignment/gradable.json?gradableId=${gradableId}${submissionId ? `&submissionId=${submissionId}` : ""}`; | |||
| const url = `/direct/assignment/gradable.json?gradableId=${gradableId}${submissionId ? `&submissionId=${submissionId}` : ""}&mergeOverride=false`; | |||
There was a problem hiding this comment.
gradable.json is only ever called from the grader code and you are always setting mergeOverride to false. So, can't you just update the entity provider logic to just not override with the individual student grade?
There was a problem hiding this comment.
Sure, assuming that Grader is the sole client of the getGradableForSite method of AssignmentEntityProvider. Once that's confirmed, then I can refactor the code to remove this extra parameter.
There was a problem hiding this comment.
It definitely is. I wrote the endpoint just for the grader
ern
changed the title
Jira: https://sakaiproject.atlassian.net/browse/SAK-49894
The intent of this PR is to share how to fix the specific bug described in the test plan of SAK-49894, the core problem being in AssignmentEntityProvider. The approach I took was to add a new parameter “mergeOverride” that the Grader code could explicitly invoke to solve this issue without breaking some other scenario (which I don’t know if such exists, but erring on the side of caution I thought it might).
Furthermore, the introduction of ‘mergeOverride’ exposed another bug where the “grade” value that would be returned is not formatted for displaying to users. I remove this logic (lines 682-685), given that it seems redundant based on the if-else conditional immediately preceding it (unless there’s something subtle here that I’m missing).
Given my aforementioned caution regarding scenarios to preserve (i.e., where ‘mergeOverride’ should indeed remain ‘true’), I assume that adding a unit test regarding this and other cases would be warranted. I did not attempt that here but others are more than welcome to push a commit here that would take care of that.
Finally, I made two fixes here to Grader’s text field. The “id” attribution should be conditional, displaying only for the Grade field, and not also for the grade override fields. Also, I repaired the “class” attribute such that the value of “points-input” will actually render in the DOM for the Grade field. This is important for decreasing that field’s width; otherwise, for peer evaluations, the info popover icon is forced to a new line/row.