SAK-50620 rubrics prevent deletion of evaluated rubrics (#13101)

rubrics/api/src/main/java/org/sakaiproject/rubrics/api/RubricsService.java

@@ -58,7 +58,7 @@ public interface RubricsService {
 
     RubricTransferBean saveRubric(RubricTransferBean rubricBean);
 
-    void deleteRubric(Long rubricId);
+    boolean deleteRubric(Long rubricId);
 
     Optional<CriterionTransferBean> createDefaultCriterion(String siteId, Long rubricId);
 

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

@@ -157,7 +157,7 @@ public class RubricsServiceImpl implements RubricsService, EntityProducer, Entit
 
     public void init() {
 
-        canEdit = tira -> isEditor(tira.getRubric().getOwnerId());
+        canEdit = tira -> isCurrentUserEditor(tira.getRubric().getOwnerId());
         canEvaluate = tira -> isEvaluator(tira.getRubric().getOwnerId());
         isCreator = tira -> tira.getCreatorId().equalsIgnoreCase(sessionManager.getCurrentSessionUserId());
 
@@ -176,7 +176,7 @@ public RubricTransferBean createDefaultRubric(String siteId) {
 
         String currentUserId = sessionManager.getCurrentSessionUserId();
 
-        if (StringUtils.isBlank(currentUserId) || !isEditor(siteId)) {
+        if (StringUtils.isBlank(currentUserId) || !isCurrentUserEditor(siteId)) {
             throw new SecurityException("You must be a rubrics editor to create/edit rubrics");
         }
 
@@ -259,7 +259,7 @@ public RubricTransferBean createDefaultRubric(String siteId) {
 
     public RubricTransferBean copyRubricToSite(Long rubricId, String toSiteId) {
 
-        if (!isEditor(toSiteId)) {
+        if (!isCurrentUserEditor(toSiteId)) {
             throw new SecurityException("You need to be a rubrics editor to get a site's rubrics");
         }
 
@@ -278,7 +278,7 @@ public RubricTransferBean copyRubricToSite(Long rubricId, String toSiteId) {
     @Transactional(readOnly = true)
     public List<RubricTransferBean> getRubricsForSite(String siteId) {
 
-        if (!isEditor(siteId)) {
+        if (!isCurrentUserEditor(siteId)) {
             throw new SecurityException("You need to be an editor to get a site's rubrics");
         }
 
@@ -294,12 +294,32 @@ public List<RubricTransferBean> getSharedRubrics() {
             .map(r -> decorateRubricBean(new RubricTransferBean(r))).collect(Collectors.toList());
     }
 
-    public void deleteRubric(Long rubricId) {
+    public boolean deleteRubric(Long rubricId) {
+
+        Optional<Rubric> optRubric = rubricRepository.findById(rubricId);
+
+        if (!optRubric.isPresent()) {
+            return false;
+        }
+
+        Rubric rubric = optRubric.get();
+
+        if (!isCurrentUserEditor(rubric.getOwnerId())) {
+            log.warn("The current user {} needs to be an editor in site {} to get the site's rubrics", sessionManager.getCurrentSessionUserId(), rubric.getOwnerId());
+            return false;
+        }
+
+        if (associationRepository.findByRubricId(rubricId).stream()
+                .anyMatch(ass -> !evaluationRepository.findByAssociationId(ass.getId()).isEmpty())) {
+            log.warn("Rubric {} cannot be deleted. It has evaluations against it", rubricId);
+            return false;
+        }
 
         // SAK-42944 removing the soft-deleted associations
-        associationRepository.findByRubricId(rubricId).forEach(ass -> evaluationRepository.deleteByToolItemRubricAssociation_Id(ass.getId()));
+        rubric.getAssociations().forEach(ass -> evaluationRepository.deleteByToolItemRubricAssociation_Id(ass.getId()));
 
-        rubricRepository.deleteById(rubricId);
+        rubricRepository.delete(rubric);
+        return true;
     }
 
     private RubricTransferBean decorateRubricBean(RubricTransferBean bean) {
@@ -382,7 +402,7 @@ public Optional<CriterionTransferBean> createDefaultCriterion(String siteId, Lon
 
         String currentUserId = sessionManager.getCurrentSessionUserId();
 
-        if (StringUtils.isBlank(currentUserId) || !isEditor(siteId)) {
+        if (StringUtils.isBlank(currentUserId) || !isCurrentUserEditor(siteId)) {
             throw new SecurityException("You must be a rubrics editor to create/edit criteria");
         }
 
@@ -432,7 +452,7 @@ public Optional<CriterionTransferBean> createDefaultEmptyCriterion(String siteId
 
         String currentUserId = sessionManager.getCurrentSessionUserId();
 
-        if (StringUtils.isBlank(currentUserId) || !isEditor(siteId)) {
+        if (StringUtils.isBlank(currentUserId) || !isCurrentUserEditor(siteId)) {
             throw new SecurityException("You must be a rubrics editor to create/edit criteria");
         }
 
@@ -460,7 +480,7 @@ public Optional<RatingTransferBean> createDefaultRating(String siteId, Long rubr
 
         String currentUserId = sessionManager.getCurrentSessionUserId();
 
-        if (StringUtils.isBlank(currentUserId) || !isEditor(siteId)) {
+        if (StringUtils.isBlank(currentUserId) || !isCurrentUserEditor(siteId)) {
             throw new SecurityException("You must be a rubrics editor to create/edit ratings");
         }
 
@@ -490,7 +510,7 @@ public RubricTransferBean saveRubric(RubricTransferBean bean) {
 
         String currentUserId = sessionManager.getCurrentSessionUserId();
 
-        if (StringUtils.isBlank(currentUserId) || !isEditor(bean.getOwnerId())) {
+        if (StringUtils.isBlank(currentUserId) || !isCurrentUserEditor(bean.getOwnerId())) {
             throw new SecurityException("You must be a rubrics editor to create/edit rubrics");
         }
 
@@ -588,7 +608,7 @@ public CriterionTransferBean updateCriterion(CriterionTransferBean bean, String
 
         String currentUserId = sessionManager.getCurrentSessionUserId();
 
-        if (StringUtils.isBlank(currentUserId) || !isEditor(siteId)) {
+        if (StringUtils.isBlank(currentUserId) || !isCurrentUserEditor(siteId)) {
             throw new SecurityException("You must be a rubrics editor to create/edit criteria");
         }
 
@@ -618,7 +638,7 @@ public CriterionTransferBean updateCriterion(CriterionTransferBean bean, String
 
     public void deleteCriterion(Long rubricId, Long criterionId, String siteId) {
 
-        if (!isEditor(siteId)) {
+        if (!isCurrentUserEditor(siteId)) {
             throw new SecurityException("You must be a rubrics editor to delete criteria");
         }
 
@@ -634,7 +654,7 @@ public RatingTransferBean updateRating(RatingTransferBean bean, String siteId) {
 
         String currentUserId = sessionManager.getCurrentSessionUserId();
 
-        if (StringUtils.isBlank(currentUserId) || !isEditor(siteId)) {
+        if (StringUtils.isBlank(currentUserId) || !isCurrentUserEditor(siteId)) {
             throw new SecurityException("You must be a rubrics editor to create/edit ratings");
         }
 
@@ -657,7 +677,7 @@ public RatingTransferBean updateRating(RatingTransferBean bean, String siteId) {
 
     public CriterionTransferBean deleteRating(Long ratingId, Long criterionId, String siteId, Long rubricId) {
 
-        if (!isEditor(siteId)) {
+        if (!isCurrentUserEditor(siteId)) {
             throw new SecurityException("You must be a rubrics editor to create/edit ratings");
         }
 
@@ -687,7 +707,7 @@ public Optional<RubricTransferBean> getRubric(Long rubricId) {
 
             String currentUserId = userDirectoryService.getCurrentUser().getId();
             if (rubric.getShared()
-                || isEditor(rubric.getOwnerId())
+                || isCurrentUserEditor(rubric.getOwnerId())
                 || isEvaluee(rubric.getOwnerId())
                 || rubric.getCreatorId().equalsIgnoreCase(currentUserId)) {
                 return decorateRubricBean(new RubricTransferBean(rubric));
@@ -700,7 +720,7 @@ public Optional<RubricTransferBean> getRubric(Long rubricId) {
     @Transactional(readOnly = true)
     public Optional<CriterionTransferBean> getCriterion(Long criterionId, String siteId) {
 
-        if (!isEditor(siteId)) {
+        if (!isCurrentUserEditor(siteId)) {
             throw new SecurityException("You must be a rubrics editor to get criteria");
         }
 
@@ -1609,7 +1629,7 @@ public boolean parseEntityReference(String reference, Reference ref) {
 
     public void deleteSiteRubrics(String siteId) {
 
-        if (!isEditor(siteId)) {
+        if (!isCurrentUserEditor(siteId)) {
             throw new SecurityException("You must be a rubrics editor to delete a site's rubrics");
         }
 
@@ -1635,7 +1655,7 @@ private Optional<Double> getCriterionPoints(Criterion cri, Evaluation evaluation
         return Optional.of(points);
     }
 
-    private boolean isEditor(String siteId) {
+    private boolean isCurrentUserEditor(String siteId) {
 
         return securityService.unlock(RubricsConstants.RBCS_PERMISSIONS_EDITOR, siteService.siteReference(siteId));
     }

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

@@ -316,21 +316,27 @@ public void deleteRubric() {
         List<Evaluation> evaluations = evaluationRepository.findAll();
         assertEquals(2, evaluations.size());
 
-        rubricsService.deleteRubric(rubricBean.getId());
+        // This should fail as we have evaluations
+        boolean deletedOk = rubricsService.deleteRubric(rubricBean.getId());
 
-        assertFalse(rubricsService.getRubric(rubricBean.getId()).isPresent());
+        assertFalse(deletedOk);
+
+        associations = associationRepository.findAll();
+        assertEquals(2, associations.size());
+
+        evaluationRepository.findAll().forEach(ev -> evaluationRepository.delete(ev));
 
-        evaluations = evaluationRepository.findAll();
-        assertEquals(0, evaluations.size());
+        deletedOk = rubricsService.deleteRubric(rubricBean.getId());
+
+        assertTrue(deletedOk);
+
+        assertFalse(rubricsService.getRubric(rubricBean.getId()).isPresent());
 
         List<Criterion> criteria = criterionRepository.findAll();
         assertEquals(0, criteria.size());
 
         associations = associationRepository.findAll();
         assertEquals(0, associations.size());
-
-        evaluations = evaluationRepository.findAll();
-        assertEquals(0, evaluations.size());
     }
 
     @Test

webapi/src/main/java/org/sakaiproject/webapi/controllers/RubricsRestController.java

@@ -269,8 +269,11 @@ public ResponseEntity deleteRubric(@PathVariable Long rubricId) {
 
         checkSakaiSession();
 
-        rubricsService.deleteRubric(rubricId);
-        return ResponseEntity.ok().build();
+        if (rubricsService.deleteRubric(rubricId)) {
+            return ResponseEntity.ok().build();
+        }
+
+        return ResponseEntity.status(HttpStatus.FORBIDDEN).build();
 	}
 
     @GetMapping(value = "/sites/{siteId}/rubrics/{rubricId}", produces = MediaType.APPLICATION_JSON_VALUE)