-
-
Notifications
You must be signed in to change notification settings - Fork 956
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
SAK-51005 Assignments invalid user can cause an NPE in peer review logic #13314
Conversation
@@ -165,7 +161,7 @@ public void execute(String opaqueContext) { | |||
AssignmentSubmission s = submissionIdMap.get(p.getId().getSubmissionId()); | |||
Optional<AssignmentSubmissionSubmitter> ass = assignmentService.getSubmissionSubmittee(s);//Next, increment the count for studentAssessorsMap | |||
String submitterId = assignmentService.getSubmitterIdForAssignment(assignment, ass.get().getSubmitter()); | |||
Integer count = ass.isPresent() ? studentAssessorsMap.get(submitterId) : 0; | |||
Integer count = studentAssessorsMap.get(submitterId); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are you sure this doesn't introduce a potential NPE?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The check is redundant because calling ass.get().getSubmitter()
earlier already guarantees that the Optional is present—otherwise, an exception would have been thrown.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
improved the logic and the use of Optionals that were not used correctly
@ern if you want any of this, please create a Jira.
The validUsers stuff seems fairly important. i believe event handling in assignments can throw an Exception causing important stuff (after saving an assignment) to fail ....
the PeerReview stuff is just optimization i think.