SAK-50685 Rubrics concurrent grading duplicate new evaluations #14307
Conversation
WalkthroughsaveEvaluation now fetches-or-creates an Evaluation (by ID or by associationId/evaluatedItemId/owner) and merges incoming CriterionOutcomes into the existing Evaluation (adding, updating, removing outcomes as needed). A new test verifies repeated saves produce a single persisted Evaluation. Changes
Suggested reviewers
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (2)
rubrics/impl/src/test/org/sakaiproject/rubrics/impl/test/RubricsServiceTests.java (2)
1043-1064: Test should validate concurrent save scenarios to match the PR objective.The test saves the same evaluation twice sequentially, but the PR title indicates this fix addresses "concurrent grading duplicate new evaluations." The test should actually simulate concurrent save attempts to verify the fix prevents race conditions.
Given the concurrency-related imports at lines 37-41, consider implementing a concurrent test:
Concurrent test implementation
@Test public void saveDuplicateNewEvaluations() throws Exception { switchToInstructor(); RubricTransferBean rubric = rubricsService.createDefaultRubric(siteId); String toolId = "sakai.assignment"; String toolItemId = "item-concurrent"; Map<String, String> rbcsParams = new HashMap<>(); rbcsParams.put(RubricsConstants.RBCS_ASSOCIATE, "1"); rbcsParams.put(RubricsConstants.RBCS_LIST, rubric.getId().toString()); ToolItemRubricAssociation association = rubricsService.saveRubricAssociation(toolId, toolItemId, rbcsParams) .orElseThrow(() -> new IllegalStateException("Association not created")); EvaluationTransferBean evaluation = buildEvaluation(association.getId(), rubric, toolItemId); - EvaluationTransferBean eval1 = rubricsService.saveEvaluation(evaluation, siteId); - EvaluationTransferBean eval2 = rubricsService.saveEvaluation(evaluation, siteId); + + // Test concurrent saves + ExecutorService executor = Executors.newFixedThreadPool(5); + CountDownLatch latch = new CountDownLatch(1); + List<Future<EvaluationTransferBean>> futures = new ArrayList<>(); + + for (int i = 0; i < 5; i++) { + futures.add(executor.submit(() -> { + latch.await(); + return rubricsService.saveEvaluation(evaluation, siteId); + })); + } + + latch.countDown(); // Release all threads simultaneously + + List<EvaluationTransferBean> results = new ArrayList<>(); + for (Future<EvaluationTransferBean> future : futures) { + results.add(future.get()); + } + executor.shutdown(); - assertNotNull(eval1); - assertNotNull(eval2); + results.forEach(result -> assertNotNull(result)); List<Evaluation> evaluations = evaluationRepository.findByAssociationId(association.getId()); assertEquals(1, evaluations.size()); assertTrue(evaluationRepository.findByAssociationIdAndEvaluatedItemIdAndOwner(association.getId(), toolItemId, user2).isPresent()); }
37-41: Unused concurrency imports.The concurrency-related imports added at lines 37-41 are not currently used in any test. These were likely added with the intention to implement concurrent testing but weren't utilized in the final test implementation.
If the concurrent test suggested in the previous comment is implemented, these imports will be needed. Otherwise, consider removing them to keep the imports clean.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
rubrics/impl/src/main/java/org/sakaiproject/rubrics/impl/RubricsServiceImpl.javarubrics/impl/src/test/org/sakaiproject/rubrics/impl/test/RubricsServiceTests.java
🧰 Additional context used
📓 Path-based instructions (1)
**/*.java
📄 CodeRabbit inference engine (.cursor/rules/logging-rule.mdc)
**/*.java: Use SLF4J parameterized logging (logger.info("Value is: {}", value)) instead of string concatenation (logger.info("Value is: " + value))
Log messages and code comments should be in English. Log messages should never be translated.
**/*.java: Java: Never use local variable type inference (var). Always declare explicit types. Yes:Map<String, Integer> counts = new HashMap<>();No:var counts = new HashMap<String, Integer>();
When proposing Java code, spell out full types in local variable declarations,forloops, and try-with-resources
When editing Java, prefer clarity over brevity; avoid introducing language features that aren't widely used in the repo
Treat any PR or suggestion containing Javavaras non-compliant. Recommend replacing with explicit types before merge
**/*.java: Use Java 17 for trunk development (Java 11 was used for Sakai 22 and Sakai 23)
Do not use local variable type inference (var) in Java code. Always declare explicit types (e.g.,List<String> names = new ArrayList<>();notvar names = new ArrayList<String>();). Enforced by Checkstyle rule duringmvn validate
Files:
rubrics/impl/src/main/java/org/sakaiproject/rubrics/impl/RubricsServiceImpl.javarubrics/impl/src/test/org/sakaiproject/rubrics/impl/test/RubricsServiceTests.java
🧠 Learnings (1)
📓 Common learnings
Learnt from: ottenhoff
Repo: sakaiproject/sakai PR: 0
File: :0-0
Timestamp: 2025-10-07T15:11:27.298Z
Learning: In samigo’s Total Scores view (samigo/samigo-app/src/webapp/jsf/evaluation/totalScores.jsp), mailto links were hidden after commit dee05746 (PR #12312, SAK-49674) added a render check requiring email.fromEmailAddress to be non-empty; PR #14154 (SAK-52058) restores visibility by checking only description.email.
🧬 Code graph analysis (1)
rubrics/impl/src/main/java/org/sakaiproject/rubrics/impl/RubricsServiceImpl.java (1)
webcomponents/tool/src/main/frontend/packages/sakai-rubrics/test/data.js (2)
evaluation(259-264)evaluation(259-264)
⏰ Context from checks skipped due to timeout of 900000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: maven-build
- GitHub Check: sakai-deploy
- GitHub Check: maven-build
rubrics/impl/src/main/java/org/sakaiproject/rubrics/impl/RubricsServiceImpl.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
rubrics/impl/src/main/java/org/sakaiproject/rubrics/impl/RubricsServiceImpl.java (1)
906-916: Logic error: setting null criterionId on new outcome.At line 908, the code checks
if (beanCriterionId == null), and then proceeds to create a newCriterionOutcomeand set itscriterionIdtooutcomeBean.getCriterionId()(line 911), which is null. This will create an outcome with a null criterion ID, which is likely invalid.This branch appears to be unreachable in normal operation since criterion outcomes from the UI should always have a criterion ID. Consider either:
- Removing this dead code branch, or
- Adding a log statement and skipping the outcome if criterionId is null
🔎 Suggested fix
for (CriterionOutcomeTransferBean outcomeBean : evaluationBean.getCriterionOutcomes()) { Long beanCriterionId = outcomeBean.getCriterionId(); - if (beanCriterionId == null) { - // add - CriterionOutcome outcome = new CriterionOutcome(); - outcome.setCriterionId(outcomeBean.getCriterionId()); - outcome.setPoints(outcomeBean.getPoints()); - outcome.setComments(outcomeBean.getComments()); - outcome.setPointsAdjusted(outcomeBean.getPointsAdjusted()); - outcome.setSelectedRatingId(outcomeBean.getSelectedRatingId()); - outcomes.add(outcome); - } else if (outcomeIds.contains(beanCriterionId)) { + if (beanCriterionId == null) { + log.warn("Skipping criterion outcome with null criterionId"); + continue; + } + if (outcomeIds.contains(beanCriterionId)) {
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
rubrics/impl/src/main/java/org/sakaiproject/rubrics/impl/RubricsServiceImpl.javarubrics/impl/src/test/org/sakaiproject/rubrics/impl/test/RubricsServiceTests.java
🧰 Additional context used
📓 Path-based instructions (1)
**/*.java
📄 CodeRabbit inference engine (.cursor/rules/logging-rule.mdc)
**/*.java: Use SLF4J parameterized logging (logger.info("Value is: {}", value)) instead of string concatenation (logger.info("Value is: " + value))
Log messages and code comments should be in English. Log messages should never be translated.
**/*.java: Java: Never use local variable type inference (var). Always declare explicit types. Yes:Map<String, Integer> counts = new HashMap<>();No:var counts = new HashMap<String, Integer>();
When proposing Java code, spell out full types in local variable declarations,forloops, and try-with-resources
When editing Java, prefer clarity over brevity; avoid introducing language features that aren't widely used in the repo
Treat any PR or suggestion containing Javavaras non-compliant. Recommend replacing with explicit types before merge
**/*.java: Use Java 17 for trunk development (Java 11 was used for Sakai 22 and Sakai 23)
Do not use local variable type inference (var) in Java code. Always declare explicit types (e.g.,List<String> names = new ArrayList<>();notvar names = new ArrayList<String>();). Enforced by Checkstyle rule duringmvn validate
Files:
rubrics/impl/src/main/java/org/sakaiproject/rubrics/impl/RubricsServiceImpl.javarubrics/impl/src/test/org/sakaiproject/rubrics/impl/test/RubricsServiceTests.java
🧠 Learnings (1)
📓 Common learnings
Learnt from: ottenhoff
Repo: sakaiproject/sakai PR: 0
File: :0-0
Timestamp: 2025-10-07T15:11:27.298Z
Learning: In samigo’s Total Scores view (samigo/samigo-app/src/webapp/jsf/evaluation/totalScores.jsp), mailto links were hidden after commit dee05746 (PR #12312, SAK-49674) added a render check requiring email.fromEmailAddress to be non-empty; PR #14154 (SAK-52058) restores visibility by checking only description.email.
🧬 Code graph analysis (1)
rubrics/impl/src/main/java/org/sakaiproject/rubrics/impl/RubricsServiceImpl.java (2)
webcomponents/tool/src/main/frontend/packages/sakai-rubrics/test/data.js (2)
evaluation(259-264)evaluation(259-264)lessonbuilder/tool/src/webapp/js/show-page.js (1)
re(58-58)
⏰ Context from checks skipped due to timeout of 900000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: sakai-deploy
- GitHub Check: maven-build
- GitHub Check: maven-build
🔇 Additional comments (3)
rubrics/impl/src/main/java/org/sakaiproject/rubrics/impl/RubricsServiceImpl.java (2)
875-899: Fetch-or-create logic correctly implements deduplication for concurrent saves.The implementation properly handles the case where multiple saves occur for the same evaluation by checking for an existing evaluation using
associationId,evaluatedItemId, andevaluatedItemOwnerId. When found, it reuses the existing evaluation; otherwise, it creates a new one with criterion outcomes populated from the bean.Note: The use of
.toList()at line 896 is compatible with Java 17 (used for trunk development per coding guidelines).
961-996: Variable rename improves clarity.The rename from
outcomestooutcomesByIdat line 967 better reflects that the map is indexed by criterion ID. The backup logic correctly updates the returned evaluation with the latest criterion outcomes and handles newly added outcomes for dynamic rubrics.rubrics/impl/src/test/org/sakaiproject/rubrics/impl/test/RubricsServiceTests.java (1)
1033-1064: Well-structured test validates fetch-or-create deduplication behavior.The test correctly verifies that:
- Multiple saves of the same evaluation data result in a single persisted evaluation (lines 1057-1059)
- The evaluation ID remains consistent across saves (lines 1054-1055)
- The last save wins for mutable fields like
evaluatorId(line 1060)- The evaluation can be found using both query methods (lines 1062-1063)
This effectively tests the concurrent grading deduplication fix described in the PR objectives.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
rubrics/impl/src/main/java/org/sakaiproject/rubrics/impl/RubricsServiceImpl.java (3)
885-898: Consider initializing key fields immediately when creating a new Evaluation.The new
Evaluationinstance created here doesn't setassociationId,evaluatedItemId, orevaluatedItemOwnerIduntil lines 948-949 and 952-953. While functionally correct, initializing these fields immediately in theorElseGetblock would make the code clearer and reduce the distance between creation and initialization.🔎 Proposed refactor
evaluation = evaluationRepository.findByAssociationIdAndEvaluatedItemIdAndOwner( evaluationBean.getAssociationId(), evaluationBean.getEvaluatedItemId(), evaluationBean.getEvaluatedItemOwnerId()) .orElseGet(() -> { // Create a new evaluation with outcomes from the bean Evaluation e = new Evaluation(); + e.setAssociationId(evaluationBean.getAssociationId()); + e.setEvaluatedItemId(evaluationBean.getEvaluatedItemId()); + e.setEvaluatedItemOwnerId(evaluationBean.getEvaluatedItemOwnerId()); e.getCriterionOutcomes().addAll(evaluationBean.getCriterionOutcomes().stream().map(o -> { CriterionOutcome outcome = new CriterionOutcome(); outcome.setCriterionId(o.getCriterionId()); outcome.setPoints(o.getPoints()); outcome.setComments(o.getComments()); outcome.setPointsAdjusted(o.getPointsAdjusted()); outcome.setSelectedRatingId(o.getSelectedRatingId()); return outcome; }).toList()); return e; });
906-916: Clarify the purpose of the null criterionId check.The condition at line 908 checks if
beanCriterionId == null, but this would only occur if the UI sends malformed data (a criterion outcome without a criterionId). Based on the test code,buildEvaluationalways sets criterionIds from the rubric, so this branch appears to be dead code or defensive programming for invalid input.If this is intentional defensive coding, consider adding a comment explaining it handles malformed input. Otherwise, this branch could be removed, or the condition should check for something else to identify new outcomes (though lines 927-938 already handle that case).
967-971: Variable nameoutcomesByIdis misleading.The map created at line 967 is keyed by
CriterionOutcome::getCriterionId, not by the outcome's ID. The nameoutcomesByIdsuggests it's keyed by the outcome's ID field, which could confuse future maintainers. Consider renaming tooutcomesByCriterionIdfor accuracy.🔎 Proposed refactor
- Map<Long, CriterionOutcome> outcomesById = savedEvaluation.getCriterionOutcomes().stream() + Map<Long, CriterionOutcome> outcomesByCriterionId = savedEvaluation.getCriterionOutcomes().stream() .collect(Collectors.toMap(CriterionOutcome::getCriterionId, co -> co)); - re.getCriterionOutcomes().removeIf(o -> outcomesById.get(o.getCriterionId()) == null); + re.getCriterionOutcomes().removeIf(o -> outcomesByCriterionId.get(o.getCriterionId()) == null); re.getCriterionOutcomes().forEach(rco -> { - CriterionOutcome o = outcomesById.get(rco.getCriterionId()); + CriterionOutcome o = outcomesByCriterionId.get(rco.getCriterionId()); rco.setSelectedRatingId(o.getSelectedRatingId());
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
rubrics/impl/src/main/java/org/sakaiproject/rubrics/impl/RubricsServiceImpl.javarubrics/impl/src/test/org/sakaiproject/rubrics/impl/test/RubricsServiceTests.java
🧰 Additional context used
📓 Path-based instructions (1)
**/*.java
📄 CodeRabbit inference engine (.cursor/rules/logging-rule.mdc)
**/*.java: Use SLF4J parameterized logging (logger.info("Value is: {}", value)) instead of string concatenation (logger.info("Value is: " + value))
Log messages and code comments should be in English. Log messages should never be translated.
**/*.java: Java: Never use local variable type inference (var). Always declare explicit types. Yes:Map<String, Integer> counts = new HashMap<>();No:var counts = new HashMap<String, Integer>();
When proposing Java code, spell out full types in local variable declarations,forloops, and try-with-resources
When editing Java, prefer clarity over brevity; avoid introducing language features that aren't widely used in the repo
Treat any PR or suggestion containing Javavaras non-compliant. Recommend replacing with explicit types before merge
**/*.java: Use Java 17 for trunk development (Java 11 was used for Sakai 22 and Sakai 23)
Do not use local variable type inference (var) in Java code. Always declare explicit types (e.g.,List<String> names = new ArrayList<>();notvar names = new ArrayList<String>();). Enforced by Checkstyle rule duringmvn validate
Files:
rubrics/impl/src/main/java/org/sakaiproject/rubrics/impl/RubricsServiceImpl.javarubrics/impl/src/test/org/sakaiproject/rubrics/impl/test/RubricsServiceTests.java
🧠 Learnings (1)
📓 Common learnings
Learnt from: ottenhoff
Repo: sakaiproject/sakai PR: 0
File: :0-0
Timestamp: 2025-10-07T15:11:27.298Z
Learning: In samigo’s Total Scores view (samigo/samigo-app/src/webapp/jsf/evaluation/totalScores.jsp), mailto links were hidden after commit dee05746 (PR #12312, SAK-49674) added a render check requiring email.fromEmailAddress to be non-empty; PR #14154 (SAK-52058) restores visibility by checking only description.email.
⏰ Context from checks skipped due to timeout of 900000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: sakai-deploy
- GitHub Check: maven-build
- GitHub Check: maven-build
🔇 Additional comments (1)
rubrics/impl/src/test/org/sakaiproject/rubrics/impl/test/RubricsServiceTests.java (1)
1033-1064: LGTM! Well-structured test for duplicate evaluation handling.The test effectively validates the new fetch-or-create behavior:
- Multiple saves with the same (association, item, owner) reuse a single evaluation
- Different evaluators can update the same evaluation
- The last evaluator is persisted
This aligns with the PR objective to prevent duplicate evaluations during concurrent grading.
1 participant
https://sakaiproject.atlassian.net/browse/SAK-50685
Summary by CodeRabbit
Bug Fixes
Tests
✏️ Tip: You can customize this high-level summary in your review settings.