ACS-3220 Folder rules delete rule v1 API (#1232)

* ACS-3220: Delete single folder rule.

* ACS-3220: Delete single folder rule - adding more negative unit tests.

* ACS-3220: Delete single folder rule - fixing review comments.

remote-api/src/main/java/org/alfresco/rest/api/Rules.java

@@ -73,4 +73,13 @@ public interface Rules
      * @throws RuleServiceException If the folder is already linked to another rule set.
      */
     List<Rule> createRules(String folderNodeId, String ruleSetId, List<Rule> rule);
+
+    /**
+     * Delete rule for rule's ID and check associations with folder node and rule set node
+     *
+     * @param folderNodeId - folder node ID
+     * @param ruleSetId - rule set ID
+     * @param ruleId - rule ID     *
+     */
+    void deleteRuleById(String folderNodeId, String ruleSetId, String ruleId);
 }

remote-api/src/main/java/org/alfresco/rest/api/impl/RulesImpl.java

@@ -95,6 +95,16 @@ public class RulesImpl implements Rules
                     .collect(Collectors.toList());
     }
 
+    @Override
+    public void deleteRuleById(String folderNodeId, String ruleSetId, String ruleId)
+    {
+        final NodeRef folderNodeRef = validateFolderNode(folderNodeId);
+        final NodeRef ruleSetNodeRef = validateRuleSetNode(ruleSetId, folderNodeRef);
+        final NodeRef ruleNodeRef = validateRuleNode(ruleId, ruleSetNodeRef);
+        final org.alfresco.service.cmr.rule.Rule rule = ruleService.getRule(ruleNodeRef);
+        ruleService.removeRule(folderNodeRef, rule);
+    }
+
     public void setNodes(Nodes nodes)
     {
         this.nodes = nodes;

remote-api/src/main/java/org/alfresco/rest/api/nodes/NodeRulesRelation.java

@@ -47,8 +47,8 @@ import java.util.List;
  */
 @Experimental
 @RelationshipResource(name = "rules", entityResource = NodeRuleSetsRelation.class, title = "Folder node rules")
-public class NodeRulesRelation implements RelationshipResourceAction.Read<Rule>, RelationshipResourceAction.ReadById<Rule>, RelationshipResourceAction.Create<Rule>,
+public class NodeRulesRelation implements RelationshipResourceAction.Read<Rule>, RelationshipResourceAction.ReadById<Rule>,
-    InitializingBean
+        RelationshipResourceAction.Create<Rule>, RelationshipResourceAction.Delete, InitializingBean
 {
 
     private Rules rules;
@@ -130,4 +130,26 @@ public class NodeRulesRelation implements RelationshipResourceAction.Read<Rule>,
     {
         this.rules = rules;
     }
+
+    /**
+     * Delete single folder rule for given node's, rule set's and rule's IDs.
+     *
+     * - DELETE /nodes/{folderNodeId}/rule-sets/{ruleSetId}/rules/{ruleId}
+     *
+     * @param folderNodeId - entity resource context for this relationship
+     * @param ruleSetId - rule set node ID (associated with folder node)
+     * @param parameters - Should not be null. Should contain at least ruleId (relationship2Id)
+     * @throws RelationshipResourceNotFoundException in case resource was not found
+     */
+    @WebApiDescription(
+            title="Delete folder node rule",
+            description = "Deletes a single rule definition for given node's, rule set's and rule's IDs",
+            successStatus = HttpServletResponse.SC_NO_CONTENT
+    )
+    @Override
+    public void delete(String folderNodeId, String ruleSetId, Parameters parameters)
+    {
+        final String ruleId = parameters.getRelationship2Id();
+        rules.deleteRuleById(folderNodeId, ruleSetId, ruleId);
+    }
 }

remote-api/src/test/java/org/alfresco/rest/api/impl/RulesImplTest.java

@@ -45,6 +45,7 @@ import java.util.List;
 import junit.framework.TestCase;
 import org.alfresco.rest.api.Nodes;
 import org.alfresco.rest.api.model.rules.Rule;
+import org.alfresco.rest.framework.core.exceptions.EntityNotFoundException;
 import org.alfresco.rest.framework.core.exceptions.InvalidArgumentException;
 import org.alfresco.rest.framework.core.exceptions.PermissionDeniedException;
 import org.alfresco.rest.framework.resource.parameters.CollectionWithPagingInfo;
@@ -95,7 +96,7 @@ public class RulesImplTest extends TestCase
         MockitoAnnotations.openMocks(this);
 
         given(nodesMock.validateOrLookupNode(eq(FOLDER_NODE_ID), any())).willReturn(folderNodeRef);
-        given(nodesMock.validateNode(eq(RULE_SET_ID))).willReturn(ruleSetNodeRef);
+        given(nodesMock.validateNode(RULE_SET_ID)).willReturn(ruleSetNodeRef);
         given(nodesMock.nodeMatches(any(), any(), any())).willReturn(true);
         given(permissionServiceMock.hasReadPermission(any())).willReturn(AccessStatus.ALLOWED);
     }
@@ -109,15 +110,15 @@ public class RulesImplTest extends TestCase
         // when
         final CollectionWithPagingInfo<Rule> rulesPage = rules.getRules(FOLDER_NODE_ID, RULE_SET_ID, paging);
 
-        then(nodesMock).should().validateOrLookupNode(eq(FOLDER_NODE_ID), isNull());
+        then(nodesMock).should().validateOrLookupNode(FOLDER_NODE_ID, null);
-        then(nodesMock).should().validateNode(eq(RULE_SET_ID));
+        then(nodesMock).should().validateNode(RULE_SET_ID);
         then(nodesMock).should().nodeMatches(eq(folderNodeRef), any(), isNull());
         then(nodesMock).should().nodeMatches(eq(ruleSetNodeRef), any(), isNull());
         then(nodesMock).shouldHaveNoMoreInteractions();
-        then(permissionServiceMock).should().hasReadPermission(eq(folderNodeRef));
+        then(permissionServiceMock).should().hasReadPermission(folderNodeRef);
         then(permissionServiceMock).shouldHaveNoMoreInteractions();
-        then(ruleServiceMock).should().isRuleSetAssociatedWithFolder(eq(ruleSetNodeRef), eq(folderNodeRef));
+        then(ruleServiceMock).should().isRuleSetAssociatedWithFolder(ruleSetNodeRef, folderNodeRef);
-        then(ruleServiceMock).should().getRules(eq(folderNodeRef));
+        then(ruleServiceMock).should().getRules(folderNodeRef);
         then(ruleServiceMock).shouldHaveNoMoreInteractions();
         assertThat(rulesPage)
                 .isNotNull()
@@ -139,13 +140,13 @@ public class RulesImplTest extends TestCase
         // when
         final CollectionWithPagingInfo<Rule> rulesPage = rules.getRules(FOLDER_NODE_ID, DEFAULT_ID, paging);
 
-        then(nodesMock).should().validateOrLookupNode(eq(FOLDER_NODE_ID), isNull());
+        then(nodesMock).should().validateOrLookupNode(FOLDER_NODE_ID, null);
         then(nodesMock).should().nodeMatches(eq(folderNodeRef), any(), isNull());
         then(nodesMock).shouldHaveNoMoreInteractions();
-        then(permissionServiceMock).should().hasReadPermission(eq(folderNodeRef));
+        then(permissionServiceMock).should().hasReadPermission(folderNodeRef);
         then(permissionServiceMock).shouldHaveNoMoreInteractions();
-        then(ruleServiceMock).should().getRuleSetNode(eq(folderNodeRef));
+        then(ruleServiceMock).should().getRuleSetNode(folderNodeRef);
-        then(ruleServiceMock).should().getRules(eq(folderNodeRef));
+        then(ruleServiceMock).should().getRules(folderNodeRef);
         then(ruleServiceMock).shouldHaveNoMoreInteractions();
         assertThat(rulesPage)
                 .isNotNull()
@@ -193,7 +194,7 @@ public class RulesImplTest extends TestCase
         assertThatExceptionOfType(InvalidArgumentException.class).isThrownBy(
             () -> rules.getRules(FOLDER_NODE_ID, RULE_SET_ID, paging));
 
-        then(ruleServiceMock).should().isRuleSetAssociatedWithFolder(eq(ruleSetNodeRef), eq(folderNodeRef));
+        then(ruleServiceMock).should().isRuleSetAssociatedWithFolder(ruleSetNodeRef, folderNodeRef);
         then(ruleServiceMock).shouldHaveNoMoreInteractions();
     }
 
@@ -212,7 +213,7 @@ public class RulesImplTest extends TestCase
     @Test
     public void testGetRuleById()
     {
-        given(nodesMock.validateNode(eq(RULE_ID))).willReturn(ruleNodeRef);
+        given(nodesMock.validateNode(RULE_ID)).willReturn(ruleNodeRef);
         given(ruleServiceMock.isRuleSetAssociatedWithFolder(any(), any())).willReturn(true);
         given(ruleServiceMock.isRuleAssociatedWithRuleSet(any(), any())).willReturn(true);
         given(ruleServiceMock.getRule(any())).willReturn(createRule(RULE_ID));
@@ -220,18 +221,18 @@ public class RulesImplTest extends TestCase
         // when
         final Rule rule = rules.getRuleById(FOLDER_NODE_ID, RULE_SET_ID, RULE_ID);
 
-        then(nodesMock).should().validateOrLookupNode(eq(FOLDER_NODE_ID), isNull());
+        then(nodesMock).should().validateOrLookupNode(FOLDER_NODE_ID, null);
-        then(nodesMock).should().validateNode(eq(RULE_SET_ID));
+        then(nodesMock).should().validateNode(RULE_SET_ID);
-        then(nodesMock).should().validateNode(eq(RULE_ID));
+        then(nodesMock).should().validateNode(RULE_ID);
         then(nodesMock).should().nodeMatches(eq(folderNodeRef), any(), isNull());
         then(nodesMock).should().nodeMatches(eq(ruleSetNodeRef), any(), isNull());
         then(nodesMock).should().nodeMatches(eq(ruleNodeRef), any(), isNull());
         then(nodesMock).shouldHaveNoMoreInteractions();
-        then(permissionServiceMock).should().hasReadPermission(eq(folderNodeRef));
+        then(permissionServiceMock).should().hasReadPermission(folderNodeRef);
         then(permissionServiceMock).shouldHaveNoMoreInteractions();
-        then(ruleServiceMock).should().isRuleSetAssociatedWithFolder(eq(ruleSetNodeRef), eq(folderNodeRef));
+        then(ruleServiceMock).should().isRuleSetAssociatedWithFolder(ruleSetNodeRef, folderNodeRef);
-        then(ruleServiceMock).should().isRuleAssociatedWithRuleSet(eq(ruleNodeRef), eq(ruleSetNodeRef));
+        then(ruleServiceMock).should().isRuleAssociatedWithRuleSet(ruleNodeRef, ruleSetNodeRef);
-        then(ruleServiceMock).should().getRule(eq(ruleNodeRef));
+        then(ruleServiceMock).should().getRule(ruleNodeRef);
         then(ruleServiceMock).shouldHaveNoMoreInteractions();
         assertThat(rule)
                 .isNotNull()
@@ -243,7 +244,7 @@ public class RulesImplTest extends TestCase
     public void testGetRuleByIdForDefaultRuleSet()
     {
         final String defaultRuleSetId = "-default-";
-        given(nodesMock.validateNode(eq(RULE_ID))).willReturn(ruleNodeRef);
+        given(nodesMock.validateNode(RULE_ID)).willReturn(ruleNodeRef);
         given(ruleServiceMock.getRuleSetNode(any())).willReturn(ruleSetNodeRef);
         given(ruleServiceMock.isRuleAssociatedWithRuleSet(any(), any())).willReturn(true);
         given(ruleServiceMock.getRule(any())).willReturn(createRule(RULE_ID));
@@ -251,15 +252,15 @@ public class RulesImplTest extends TestCase
         // when
         final Rule rule = rules.getRuleById(FOLDER_NODE_ID, defaultRuleSetId, RULE_ID);
 
-        then(nodesMock).should().validateOrLookupNode(eq(FOLDER_NODE_ID), isNull());
+        then(nodesMock).should().validateOrLookupNode(FOLDER_NODE_ID, null);
-        then(nodesMock).should().validateNode(eq(RULE_ID));
+        then(nodesMock).should().validateNode(RULE_ID);
         then(nodesMock).should().nodeMatches(eq(folderNodeRef), any(), isNull());
         then(nodesMock).should().nodeMatches(eq(ruleNodeRef), any(), isNull());
         then(nodesMock).shouldHaveNoMoreInteractions();
-        then(permissionServiceMock).should().hasReadPermission(eq(folderNodeRef));
+        then(permissionServiceMock).should().hasReadPermission(folderNodeRef);
         then(permissionServiceMock).shouldHaveNoMoreInteractions();
-        then(ruleServiceMock).should().getRuleSetNode(eq(folderNodeRef));
+        then(ruleServiceMock).should().getRuleSetNode(folderNodeRef);
-        then(ruleServiceMock).should().isRuleAssociatedWithRuleSet(eq(ruleNodeRef), eq(ruleSetNodeRef));
+        then(ruleServiceMock).should().isRuleAssociatedWithRuleSet(ruleNodeRef, ruleSetNodeRef);
         then(ruleServiceMock).should().getRule(eq(ruleNodeRef));
         then(ruleServiceMock).shouldHaveNoMoreInteractions();
         assertThat(rule)
@@ -271,8 +272,8 @@ public class RulesImplTest extends TestCase
     @Test
     public void testGetRuleByIdForNotAssociatedRuleToRuleSet()
     {
-        given(nodesMock.validateNode(eq(RULE_SET_ID))).willReturn(ruleSetNodeRef);
+        given(nodesMock.validateNode(RULE_SET_ID)).willReturn(ruleSetNodeRef);
-        given(nodesMock.validateNode(eq(RULE_ID))).willReturn(ruleNodeRef);
+        given(nodesMock.validateNode(RULE_ID)).willReturn(ruleNodeRef);
         given(ruleServiceMock.isRuleSetAssociatedWithFolder(any(), any())).willReturn(true);
         given(ruleServiceMock.isRuleAssociatedWithRuleSet(any(), any())).willReturn(false);
 
@@ -280,8 +281,8 @@ public class RulesImplTest extends TestCase
         assertThatExceptionOfType(InvalidArgumentException.class).isThrownBy(
             () -> rules.getRuleById(FOLDER_NODE_ID, RULE_SET_ID, RULE_ID));
 
-        then(ruleServiceMock).should().isRuleSetAssociatedWithFolder(eq(ruleSetNodeRef), eq(folderNodeRef));
+        then(ruleServiceMock).should().isRuleSetAssociatedWithFolder(ruleSetNodeRef, folderNodeRef);
-        then(ruleServiceMock).should().isRuleAssociatedWithRuleSet(eq(ruleNodeRef), eq(ruleSetNodeRef));
+        then(ruleServiceMock).should().isRuleAssociatedWithRuleSet(ruleNodeRef, ruleSetNodeRef);
         then(ruleServiceMock).shouldHaveNoMoreInteractions();
     }
 
@@ -394,10 +395,136 @@ public class RulesImplTest extends TestCase
         assertThat(actual).isEqualTo(expected);
     }
 
+    @Test
+    public void testDeleteRuleById() {
+        given(nodesMock.validateNode(RULE_SET_ID)).willReturn(ruleSetNodeRef);
+        given(nodesMock.validateNode(RULE_ID)).willReturn(ruleNodeRef);
+        given(ruleServiceMock.isRuleAssociatedWithRuleSet(any(), any())).willReturn(true);
+        given(ruleServiceMock.isRuleSetAssociatedWithFolder(any(), any())).willReturn(true);
+        org.alfresco.service.cmr.rule.Rule rule = createRule(RULE_ID);
+        given(ruleServiceMock.getRule(any())).willReturn(rule);
+
+        //when
+        rules.deleteRuleById(FOLDER_NODE_ID, RULE_SET_ID, RULE_ID);
+
+        then(nodesMock).should().validateOrLookupNode(FOLDER_NODE_ID, null);
+        then(nodesMock).should().validateNode(RULE_SET_ID);
+        then(nodesMock).should().validateNode(RULE_ID);
+        then(nodesMock).should().nodeMatches(eq(folderNodeRef), any(), isNull());
+        then(nodesMock).should().nodeMatches(eq(ruleSetNodeRef), any(), isNull());
+        then(nodesMock).should().nodeMatches(eq(ruleNodeRef), any(), isNull());
+        then(nodesMock).shouldHaveNoMoreInteractions();
+        then(permissionServiceMock).should().hasReadPermission(folderNodeRef);
+        then(permissionServiceMock).shouldHaveNoMoreInteractions();
+        then(ruleServiceMock).should().isRuleSetAssociatedWithFolder(ruleSetNodeRef, folderNodeRef);
+        then(ruleServiceMock).should().isRuleAssociatedWithRuleSet(ruleNodeRef, ruleSetNodeRef);
+        then(ruleServiceMock).should().getRule(ruleNodeRef);
+        then(ruleServiceMock).should().removeRule(folderNodeRef, rule);
+        then(ruleServiceMock).shouldHaveNoMoreInteractions();
+    }
+
+    @Test
+    public void testDeleteRuleById_NonExistingRuleId() {
+        given(nodesMock.validateNode(RULE_SET_ID)).willReturn(ruleSetNodeRef);
+        given(nodesMock.validateNode(RULE_ID)).willThrow(new EntityNotFoundException(RULE_ID));
+        given(ruleServiceMock.isRuleSetAssociatedWithFolder(any(), any())).willReturn(true);
+
+        //when
+        assertThatExceptionOfType(EntityNotFoundException.class).isThrownBy(
+                () -> rules.deleteRuleById(FOLDER_NODE_ID, RULE_SET_ID, RULE_ID));
+
+        then(nodesMock).should().validateOrLookupNode(FOLDER_NODE_ID, null);
+        then(nodesMock).should().validateNode(RULE_SET_ID);
+        then(nodesMock).should().validateNode(RULE_ID);
+        then(nodesMock).should().nodeMatches(eq(folderNodeRef), any(), isNull());
+        then(nodesMock).should().nodeMatches(eq(ruleSetNodeRef), any(), isNull());
+        then(nodesMock).shouldHaveNoMoreInteractions();
+        then(permissionServiceMock).should().hasReadPermission(folderNodeRef);
+        then(permissionServiceMock).shouldHaveNoMoreInteractions();
+        then(ruleServiceMock).should().isRuleSetAssociatedWithFolder(ruleSetNodeRef, folderNodeRef);
+        then(ruleServiceMock).shouldHaveNoMoreInteractions();
+    }
+
+    @Test
+    public void testDeleteRuleById_RuleIdNotInRuleSet() {
+        given(nodesMock.validateNode(RULE_SET_ID)).willReturn(ruleSetNodeRef);
+        given(nodesMock.validateNode(RULE_ID)).willReturn(ruleNodeRef);
+        given(ruleServiceMock.isRuleSetAssociatedWithFolder(any(), any())).willReturn(true);
+        given(ruleServiceMock.isRuleAssociatedWithRuleSet(any(), any())).willReturn(false);
+
+        //when
+        assertThatExceptionOfType(InvalidArgumentException.class).isThrownBy(
+                () -> rules.deleteRuleById(FOLDER_NODE_ID, RULE_SET_ID, RULE_ID));
+
+        then(nodesMock).should().validateOrLookupNode(FOLDER_NODE_ID, null);
+        then(nodesMock).should().validateNode(RULE_SET_ID);
+        then(nodesMock).should().validateNode(RULE_ID);
+        then(nodesMock).should().nodeMatches(eq(folderNodeRef), any(), isNull());
+        then(nodesMock).should().nodeMatches(eq(ruleSetNodeRef), any(), isNull());
+        then(nodesMock).should().nodeMatches(eq(ruleNodeRef), any(), isNull());
+        then(nodesMock).shouldHaveNoMoreInteractions();
+        then(permissionServiceMock).should().hasReadPermission(folderNodeRef);
+        then(permissionServiceMock).shouldHaveNoMoreInteractions();
+        then(ruleServiceMock).should().isRuleSetAssociatedWithFolder(ruleSetNodeRef, folderNodeRef);
+        then(ruleServiceMock).should().isRuleAssociatedWithRuleSet(ruleNodeRef, ruleSetNodeRef);
+        then(ruleServiceMock).shouldHaveNoMoreInteractions();
+    }
+
+    @Test
+    public void testDeleteRuleById_NonExistingRuleSetId() {
+        given(nodesMock.validateNode(RULE_SET_ID)).willThrow(new EntityNotFoundException(RULE_SET_ID));
+
+        //when
+        assertThatExceptionOfType(EntityNotFoundException.class).isThrownBy(
+                () -> rules.deleteRuleById(FOLDER_NODE_ID, RULE_SET_ID, RULE_ID));
+
+        then(nodesMock).should().validateOrLookupNode(FOLDER_NODE_ID, null);
+        then(nodesMock).should().validateNode(RULE_SET_ID);
+        then(nodesMock).should().nodeMatches(eq(folderNodeRef), any(), isNull());
+        then(nodesMock).shouldHaveNoMoreInteractions();
+        then(permissionServiceMock).should().hasReadPermission(folderNodeRef);
+        then(permissionServiceMock).shouldHaveNoMoreInteractions();
+        then(ruleServiceMock).shouldHaveNoMoreInteractions();
+    }
+
+    @Test
+    public void testDeleteRuleById_RuleSetNotInFolder() {
+        given(nodesMock.validateNode(RULE_SET_ID)).willReturn(ruleSetNodeRef);
+        given(ruleServiceMock.isRuleSetAssociatedWithFolder(any(), any())).willReturn(false);
+
+        //when
+        assertThatExceptionOfType(InvalidArgumentException.class).isThrownBy(
+                () -> rules.deleteRuleById(FOLDER_NODE_ID, RULE_SET_ID, RULE_ID));
+
+        then(nodesMock).should().validateOrLookupNode(FOLDER_NODE_ID, null);
+        then(nodesMock).should().validateNode(RULE_SET_ID);
+        then(nodesMock).should().nodeMatches(eq(folderNodeRef), any(), isNull());
+        then(nodesMock).should().nodeMatches(eq(ruleSetNodeRef), any(), isNull());
+        then(nodesMock).shouldHaveNoMoreInteractions();
+        then(permissionServiceMock).should().hasReadPermission(folderNodeRef);
+        then(permissionServiceMock).shouldHaveNoMoreInteractions();
+        then(ruleServiceMock).should().isRuleSetAssociatedWithFolder(ruleSetNodeRef, folderNodeRef);
+        then(ruleServiceMock).shouldHaveNoMoreInteractions();
+    }
+
+    @Test
+    public void testDeleteRuleById_NonExistingFolderId() {
+        given(nodesMock.validateOrLookupNode(FOLDER_NODE_ID, null)).willThrow(new EntityNotFoundException(RULE_ID));
+
+        //when
+        assertThatExceptionOfType(EntityNotFoundException.class).isThrownBy(
+                () -> rules.deleteRuleById(FOLDER_NODE_ID, RULE_SET_ID, RULE_ID));
+
+        then(nodesMock).should().validateOrLookupNode(FOLDER_NODE_ID, null);
+        then(nodesMock).shouldHaveNoMoreInteractions();
+        then(permissionServiceMock).shouldHaveNoMoreInteractions();
+        then(ruleServiceMock).shouldHaveNoMoreInteractions();
+    }
+
     private static org.alfresco.service.cmr.rule.Rule createRule(final String id) {
         final org.alfresco.service.cmr.rule.Rule rule = new org.alfresco.service.cmr.rule.Rule();
         rule.setNodeRef(new NodeRef(StoreRef.STORE_REF_WORKSPACE_SPACESSTORE, id));
 
         return rule;
     }
-}
+}

remote-api/src/test/java/org/alfresco/rest/api/nodes/NodeRulesRelationTest.java

@@ -89,4 +89,14 @@ public class NodeRulesRelationTest extends TestCase
         then(rulesMock).should().getRuleById(eq(FOLDER_NODE_ID), eq(RULE_SET_ID), eq(RULE_ID));
         then(rulesMock).shouldHaveNoMoreInteractions();
     }
-}
+
+    @Test
+    public void testDeleteById() {
+        final Parameters parameters = ParamsExtender.valueOf(null, FOLDER_NODE_ID, RULE_SET_ID, RULE_ID);
+        // when
+        nodeRulesRelation.delete(FOLDER_NODE_ID, RULE_SET_ID, parameters);
+
+        then(rulesMock).should().deleteRuleById(eq(FOLDER_NODE_ID), eq(RULE_SET_ID), eq(RULE_ID));
+        then(rulesMock).shouldHaveNoMoreInteractions();
+    }
+}

repository/src/main/java/org/alfresco/repo/rule/RuleServiceImpl.java

@@ -900,10 +900,9 @@ public class RuleServiceImpl
     {
         checkForLinkedRules(nodeRef);
         
-        if (this.permissionService.hasPermission(nodeRef, PermissionService.CHANGE_PERMISSIONS) == AccessStatus.ALLOWED)
+        if (permissionService.hasPermission(nodeRef, PermissionService.CHANGE_PERMISSIONS) == AccessStatus.ALLOWED)
         {
-            if (this.nodeService.exists(nodeRef) == true &&
+            if (nodeService.exists(nodeRef) && nodeService.hasAspect(nodeRef, RuleModel.ASPECT_RULES))
-                this.nodeService.hasAspect(nodeRef, RuleModel.ASPECT_RULES) == true)
             {
                 disableRules(nodeRef);
                 try
@@ -911,7 +910,7 @@ public class RuleServiceImpl
                     NodeRef ruleNodeRef = rule.getNodeRef();
                     if (ruleNodeRef != null)
                     {
-                        this.nodeService.removeChild(getSavedRuleFolderRef(nodeRef), ruleNodeRef);
+                        nodeService.removeChild(getSavedRuleFolderRef(nodeRef), ruleNodeRef);
                     }
                 }
                 finally
@@ -934,7 +933,7 @@ public class RuleServiceImpl
                         }
                     }
                     
-                    this.nodeService.removeAspect(nodeRef, RuleModel.ASPECT_RULES);
+                    nodeService.removeAspect(nodeRef, RuleModel.ASPECT_RULES);
                 }
             }
             // Drop the rules from the cache
@@ -944,8 +943,8 @@ public class RuleServiceImpl
         {
             throw new RuleServiceException("Insufficient permissions to remove a rule.");
         }
-    }    
+    }
-    
+
     /**
      * Checks if rules are linked and throws an exception if they are.
      *