SAK-50685 Rubrics concurrent grading duplicate new evaluations (#14307)

Author: ern

rubrics/impl/src/main/java/org/sakaiproject/rubrics/impl/RubricsServiceImpl.java

@@ -872,9 +872,34 @@ public EvaluationTransferBean saveEvaluation(EvaluationTransferBean evaluationBe
 
         Evaluation evaluation;
         List<Long> newOutcomesCriterionIds = new ArrayList<>();
+
         if (evaluationBean.getId() != null) {
+            // UI knows the evaluation ID, fetch it directly
             evaluation = evaluationRepository.getById(evaluationBean.getId());
+        } else {
+            // UI thinks this is new, but check if an evaluation exists for this association and item
+            evaluation = evaluationRepository.findByAssociationIdAndEvaluatedItemIdAndOwner(
+                    evaluationBean.getAssociationId(),
+                    evaluationBean.getEvaluatedItemId(),
+                    evaluationBean.getEvaluatedItemOwnerId())
+                .orElseGet(() -> {
+                    // Create a new evaluation with outcomes from the bean
+                    Evaluation e = new Evaluation();
+                    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;
+                });
+        }
 
+        // If evaluation already exists (has an ID), merge the criterion outcomes
+        if (evaluation.getId() != null) {
             List<CriterionOutcome> outcomes = evaluation.getCriterionOutcomes();
             List<Long> outcomeIds = outcomes.stream().map(CriterionOutcome::getCriterionId).collect(Collectors.toList());
 
@@ -914,17 +939,6 @@ public EvaluationTransferBean saveEvaluation(EvaluationTransferBean evaluationBe
             }
             // outcomeIds should be empty, if not the db contained outcomes not reported in the ui so remove them
             outcomes.removeIf(o -> outcomeIds.contains(o.getCriterionId()));
-        } else {
-            evaluation = new Evaluation();
-            evaluation.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;
-            }).collect(Collectors.toList()));
         }
 
         // only set these once
@@ -950,11 +964,11 @@ public EvaluationTransferBean saveEvaluation(EvaluationTransferBean evaluationBe
             ReturnedEvaluation returnedEvaluation = returnedEvaluationRepository.findByOriginalEvaluationId(evaluation.getId())
                 .map(re -> {
                     re.setOverallComment(savedEvaluation.getOverallComment());
-                    Map<Long, CriterionOutcome> outcomes = savedEvaluation.getCriterionOutcomes().stream()
+                    Map<Long, CriterionOutcome> outcomesById = savedEvaluation.getCriterionOutcomes().stream()
                             .collect(Collectors.toMap(CriterionOutcome::getCriterionId, co -> co));
-                    re.getCriterionOutcomes().removeIf(o -> outcomes.get(o.getCriterionId()) == null);
+                    re.getCriterionOutcomes().removeIf(o -> outcomesById.get(o.getCriterionId()) == null);
                     re.getCriterionOutcomes().forEach(rco -> {
-                        CriterionOutcome o = outcomes.get(rco.getCriterionId());
+                        CriterionOutcome o = outcomesById.get(rco.getCriterionId());
                         rco.setSelectedRatingId(o.getSelectedRatingId());
                         rco.setPointsAdjusted(o.getPointsAdjusted());
                         rco.setPoints(o.getPoints());

rubrics/impl/src/test/org/sakaiproject/rubrics/impl/test/RubricsServiceTests.java

@@ -81,14 +81,9 @@
 import org.springframework.test.context.junit4.SpringJUnit4ClassRunner;
 import org.springframework.test.util.AopTestUtils;
 
-import javax.xml.parsers.DocumentBuilder;
-import javax.xml.parsers.DocumentBuilderFactory;
-import javax.xml.parsers.ParserConfigurationException;
-
 import org.w3c.dom.CDATASection;
 import org.w3c.dom.Document;
 import org.w3c.dom.Element;
-import org.w3c.dom.Node;
 import org.w3c.dom.NodeList;
 
 import lombok.extern.slf4j.Slf4j;
@@ -1035,6 +1030,39 @@ public void merge() {
         assertTrue(newTitles.contains(extraTitle));
     }
 
+    @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); // duplicate save from the same instructor
+        evaluation.setEvaluatorId(user3);
+        EvaluationTransferBean eval3 = rubricsService.saveEvaluation(evaluation, siteId); // duplicate save from different instructor
+
+        assertNotNull(eval1);
+        assertNotNull(eval2);
+        assertNotNull(eval3);
+        assertEquals(eval1.getId(), eval2.getId());
+        assertEquals(eval1.getId(), eval3.getId());
+
+        List<Evaluation> evaluations = evaluationRepository.findByAssociationId(association.getId());
+        assertEquals(1, evaluations.size());
+        assertEquals(eval1.getId(), evaluations.get(0).getId());
+        assertEquals(user3, evaluations.get(0).getEvaluatorId());
+
+        assertTrue(evaluationRepository.findByAssociationIdAndEvaluatedItemId(association.getId(), toolItemId).isPresent());
+        assertTrue(evaluationRepository.findByAssociationIdAndEvaluatedItemIdAndOwner(association.getId(), toolItemId, user2).isPresent());
+    }
+
     private EvaluationTransferBean buildEvaluation(Long associationId, RubricTransferBean rubricBean, String toolItemId) {
 
         EvaluationTransferBean etb = new EvaluationTransferBean();