RM-2562: Deleting record from versions produces inconsistent behaviour

* Version history shows destroyed version despite meta-data shell of version record still being present (RM-2839)
 * Integration tests (RM-2842)

+review RM-223



git-svn-id: https://svn.alfresco.com/repos/alfresco-enterprise/modules/recordsmanagement/BRANCHES/V2.3@119506 c4b6b30b-aa2e-2d43-bbcb-ca4b014f7261

rm-server/config/alfresco/module/org_alfresco_module_rm/rm-action-context.xml

@@ -258,6 +258,7 @@
    <bean id="destroy" class="org.alfresco.module.org_alfresco_module_rm.action.impl.DestroyAction" parent="rmAction"
          depends-on="rmDestroyRecordsScheduledForDestructionCapability">
       <property name="capabilityService" ref="CapabilityService" />
+      <property name="recordableVersionService" ref="RecordableVersionService" />
       <property name="eagerContentStoreCleaner" ref="eagerContentStoreCleaner"/>
       <property name="ghostingEnabled">
          <value>${rm.ghosting.enabled}</value>

rm-server/config/alfresco/module/org_alfresco_module_rm/rm-version-context.xml

@@ -14,7 +14,6 @@
    
    <!-- Recordable version model behaviours -->
    <bean id="rmv.versionRecord" class="org.alfresco.module.org_alfresco_module_rm.model.rma.aspect.VersionRecordAspect" parent="rm.baseBehaviour">
-        <property name="versionService" ref="VersionService" />
         <property name="recordableVersionService" ref="RecordableVersionService" />
         <property name="relationshipService" ref="RelationshipService" />
    </bean>
@@ -71,6 +70,7 @@
             org.alfresco.module.org_alfresco_module_rm.version.RecordableVersionService.isCurrentVersionRecorded=RM_ALLOW
             org.alfresco.module.org_alfresco_module_rm.version.RecordableVersionService.isRecordedVersion=RM_ALLOW
             org.alfresco.module.org_alfresco_module_rm.version.RecordableVersionService.getVersionRecord=RM_ALLOW
+            org.alfresco.module.org_alfresco_module_rm.version.RecordableVersionService.getRecordedVersion=RM_ALLOW
             org.alfresco.module.org_alfresco_module_rm.version.RecordableVersionService.createRecordFromLatestVersion=RM_ALLOW
             org.alfresco.module.org_alfresco_module_rm.version.RecordableVersionService.isRecordedVersionDestroyed=RM_ALLOW
             org.alfresco.module.org_alfresco_module_rm.version.RecordableVersionService.destroyRecordedVersion=RM_ALLOW

rm-server/source/java/org/alfresco/module/org_alfresco_module_rm/action/impl/DestroyAction.java

@@ -29,6 +29,7 @@ import org.alfresco.module.org_alfresco_module_rm.action.RMDispositionActionExec
 import org.alfresco.module.org_alfresco_module_rm.capability.CapabilityService;
 import org.alfresco.module.org_alfresco_module_rm.disposition.DispositionActionDefinition;
 import org.alfresco.module.org_alfresco_module_rm.disposition.DispositionSchedule;
+import org.alfresco.module.org_alfresco_module_rm.version.RecordableVersionService;
 import org.alfresco.repo.content.cleanup.EagerContentStoreCleaner;
 import org.alfresco.service.cmr.action.Action;
 import org.alfresco.service.cmr.dictionary.DataTypeDefinition;
@@ -36,6 +37,7 @@ import org.alfresco.service.cmr.repository.ChildAssociationRef;
 import org.alfresco.service.cmr.repository.ContentData;
 import org.alfresco.service.cmr.repository.NodeRef;
 import org.alfresco.service.cmr.security.AccessStatus;
+import org.alfresco.service.cmr.version.Version;
 import org.alfresco.service.namespace.QName;
 import org.apache.commons.lang.StringUtils;
 
@@ -55,6 +57,9 @@ public class DestroyAction extends RMDispositionActionExecuterAbstractBase
     /** Capability service */
     private CapabilityService capabilityService;
     
+    /** Recordable version service */
+    private RecordableVersionService recordableVersionService;
+
     /** Indicates if ghosting is enabled or not */
     private boolean ghostingEnabled = true;
 
@@ -74,6 +79,14 @@ public class DestroyAction extends RMDispositionActionExecuterAbstractBase
         this.capabilityService = capabilityService;
     }
     
+    /**
+     * @param recordableVersionService  recordable version service
+     */
+    public void setRecordableVersionService(RecordableVersionService recordableVersionService)
+    {
+        this.recordableVersionService = recordableVersionService;
+    }
+
     /**
      * @param ghostingEnabled   true if ghosting is enabled, false otherwise
      */
@@ -150,6 +163,13 @@ public class DestroyAction extends RMDispositionActionExecuterAbstractBase
 
         if (isGhostOnDestroySetForAction(action, record))
         {
+            // mark version as destroyed
+            Version version = recordableVersionService.getRecordedVersion(record);
+            if (version != null)
+            {
+                recordableVersionService.destroyRecordedVersion(version);
+            }
+            
             // Add the ghosted aspect
             getNodeService().addAspect(record, ASPECT_GHOSTED, null);
         }

rm-server/source/java/org/alfresco/module/org_alfresco_module_rm/model/rma/aspect/VersionRecordAspect.java

@@ -18,9 +18,6 @@
  */
 package org.alfresco.module.org_alfresco_module_rm.model.rma.aspect;
 
-import static org.alfresco.module.org_alfresco_module_rm.version.RecordableVersionModel.PROP_VERSIONED_NODEREF;
-import static org.alfresco.module.org_alfresco_module_rm.version.RecordableVersionModel.PROP_VERSION_LABEL;
-
 import java.util.Set;
 
 import org.alfresco.module.org_alfresco_module_rm.model.BaseBehaviourBean;
@@ -34,9 +31,6 @@ import org.alfresco.repo.policy.annotation.BehaviourKind;
 import org.alfresco.repo.security.authentication.AuthenticationUtil.RunAsWork;
 import org.alfresco.service.cmr.repository.NodeRef;
 import org.alfresco.service.cmr.version.Version;
-import org.alfresco.service.cmr.version.VersionHistory;
-import org.alfresco.service.cmr.version.VersionService;
-import org.apache.commons.lang.StringUtils;
 
 /**
  * rmv:versionRecord behaviour bean
@@ -51,23 +45,12 @@ import org.apache.commons.lang.StringUtils;
 public class VersionRecordAspect extends    BaseBehaviourBean
                                  implements NodeServicePolicies.BeforeDeleteNodePolicy
 {
-    /** version service */
-    private VersionService versionService;
-    
     /** recordable version service */
     private RecordableVersionService recordableVersionService;
     
     /** relationship service */
     private RelationshipService relationshipService;
     
-    /**
-     * @param versionService    version service
-     */
-    public void setVersionService(VersionService versionService)
-    {
-        this.versionService = versionService;
-    }
-    
     /**
      * @param recordableVersionService  recordable version service
      */
@@ -93,16 +76,7 @@ public class VersionRecordAspect extends    BaseBehaviourBean
     @Behaviour (kind = BehaviourKind.CLASS)
     public void beforeDeleteNode(final NodeRef nodeRef)
     {
-        final NodeRef versionedNodeRef = (NodeRef) nodeService.getProperty(nodeRef, PROP_VERSIONED_NODEREF);
+        final Version version = recordableVersionService.getRecordedVersion(nodeRef);
-        if (versionedNodeRef != null)
-        {
-            String versionLabel = (String) nodeService.getProperty(nodeRef, PROP_VERSION_LABEL);
-            if (StringUtils.isNotBlank(versionLabel))
-            {
-                final VersionHistory versionHistory = versionService.getVersionHistory(versionedNodeRef);
-                if (versionHistory != null)
-                {
-                    final Version version = versionHistory.getVersion(versionLabel);
         if (version != null)
         {
             authenticationUtil.runAsSystem(new RunAsWork<Void>()
@@ -146,7 +120,4 @@ public class VersionRecordAspect extends    BaseBehaviourBean
             });
         }         
     }
-            }
-        }
-    }
 }

rm-server/source/java/org/alfresco/module/org_alfresco_module_rm/version/RecordableVersionService.java

@@ -56,6 +56,14 @@ public interface RecordableVersionService
      */
     NodeRef getVersionRecord(Version version);
     
+    /**
+     * Gets the version that relates to the version record
+     * 
+     * @param versionRecord version record node reference
+     * @return Version  version or null if not found
+     */
+    Version getRecordedVersion(NodeRef record);
+    
     /**
      * Creates a record from the latest version, marking it as recorded.
      * <p>

rm-server/source/java/org/alfresco/module/org_alfresco_module_rm/version/RecordableVersionServiceImpl.java

@@ -55,6 +55,7 @@ import org.alfresco.service.cmr.version.VersionType;
 import org.alfresco.service.namespace.QName;
 import org.alfresco.util.ParameterCheck;
 import org.alfresco.util.PropertyMap;
+import org.apache.commons.lang.StringUtils;
 import org.apache.commons.logging.Log;
 import org.apache.commons.logging.LogFactory;
 
@@ -504,64 +505,27 @@ public class RecordableVersionServiceImpl extends    Version2ServiceImpl
         return versionRecord;
     }
     
+    /**
+     * @see org.alfresco.module.org_alfresco_module_rm.version.RecordableVersionService#getRecordedVersion(org.alfresco.service.cmr.repository.NodeRef)
+     */
     @Override
-    protected VersionHistory buildVersionHistory(NodeRef versionHistoryRef, NodeRef nodeRef)
+    public Version getRecordedVersion(NodeRef versionRecord)
     {
-        VersionHistory versionHistory = super.buildVersionHistory(versionHistoryRef, nodeRef);
+        Version version = null;
-        
+        NodeRef versionedNodeRef = (NodeRef) nodeService.getProperty(versionRecord, RecordableVersionModel.PROP_VERSIONED_NODEREF);
-        // create an empty version history if appropriate
+        if (versionedNodeRef != null)
-        if (versionHistoryRef != null && 
-            nodeRef != null &&
-            versionHistory == null &&
-            getAllVersions(versionHistoryRef).isEmpty() == true)
         {
-            versionHistory = new EmptyVersionHistory();
+            String versionLabel = (String) nodeService.getProperty(versionRecord, RecordableVersionModel.PROP_VERSION_LABEL);
+            if (StringUtils.isNotBlank(versionLabel))
+            {
+                VersionHistory versionHistory = getVersionHistory(versionedNodeRef);
+                if (versionHistory != null)
+                {
+                    version = versionHistory.getVersion(versionLabel);
                 }
-        
-        return versionHistory;
             }
-    
-    public class EmptyVersionHistory implements VersionHistory
-    {
-        private static final long serialVersionUID = 3449832161314670033L;
-
-        @Override
-        public Version getRootVersion()
-        {
-            return null;
-        }
-
-        @Override
-        public Version getHeadVersion()
-        {
-            return null;
-        }
-
-        @SuppressWarnings("unchecked")
-        @Override
-        public Collection<Version> getAllVersions()
-        {
-            return (Collection<Version>)Collections.EMPTY_LIST;
-        }
-
-        @Override
-        public Version getPredecessor(Version version)
-        {
-            return null;
-        }
-
-        @SuppressWarnings("unchecked")
-        @Override
-        public Collection<Version> getSuccessors(Version version)
-        {
-            return (Collection<Version>)Collections.EMPTY_LIST;
-        }
-
-        @Override
-        public Version getVersion(String versionLabel)
-        {
-            return null;
         }
+        return version;
     }
 
     /**

rm-server/test/java/org/alfresco/module/org_alfresco_module_rm/test/integration/version/DeleteRecordVersionTest.java

@@ -19,13 +19,18 @@
 package org.alfresco.module.org_alfresco_module_rm.test.integration.version;
 
 import java.io.Serializable;
+import java.util.Collection;
 import java.util.HashMap;
 import java.util.Map;
 import java.util.Set;
 
 import org.alfresco.model.ContentModel;
+import org.alfresco.module.org_alfresco_module_rm.action.impl.CompleteEventAction;
+import org.alfresco.module.org_alfresco_module_rm.action.impl.CutOffAction;
+import org.alfresco.module.org_alfresco_module_rm.action.impl.DestroyAction;
 import org.alfresco.module.org_alfresco_module_rm.relationship.Relationship;
 import org.alfresco.module.org_alfresco_module_rm.relationship.RelationshipService;
+import org.alfresco.module.org_alfresco_module_rm.test.util.CommonRMTestUtils;
 import org.alfresco.module.org_alfresco_module_rm.version.RecordableVersionModel;
 import org.alfresco.module.org_alfresco_module_rm.version.RecordableVersionPolicy;
 import org.alfresco.repo.content.MimetypeMap;
@@ -397,4 +402,105 @@ public class DeleteRecordVersionTest extends RecordableVersionsBaseTest
             }
         });  
     }
+    
+    /**
+     * Given that a version record
+     * When the version record is destroyed whilst retaining the meta data
+     * Then the version is marked as destroyed in the collab version history
+     */
+    public void testDetroyVerionRecordWithMetadata()
+    {
+        final NodeRef myDocument = createDocumentWithRecordVersions();
+        
+        doBehaviourDrivenTest(new BehaviourDrivenTest()
+        {
+            private VersionHistory versionHistory;
+            private NodeRef recordVersion11;
+            
+            public void given() throws Exception
+            {
+                // create file plan structure
+                NodeRef myCategory = filePlanService.createRecordCategory(filePlan, GUID.generate());
+                utils.createBasicDispositionSchedule(myCategory, GUID.generate(), GUID.generate(), true, true);
+                
+                NodeRef myRecordFolder = recordFolderService.createRecordFolder(myCategory, GUID.generate());
+                
+                // get version history
+                versionHistory = versionService.getVersionHistory(myDocument);
+                
+                // file and complete all the version records into my record folder
+                for (Version version : versionHistory.getAllVersions())
+                {
+                    NodeRef record = recordableVersionService.getVersionRecord(version);
+                    fileFolderService.move(record, myRecordFolder, null);
+                    utils.completeRecord(record);
+                }
+            }
+            
+            public void when()
+            {   
+                Version version11 = versionHistory.getVersion("1.1");
+                recordVersion11 = recordableVersionService.getVersionRecord(version11);
+                
+                Map<String, Serializable> params = new HashMap<String, Serializable>(1);
+                params.put(CompleteEventAction.PARAM_EVENT_NAME, CommonRMTestUtils.DEFAULT_EVENT_NAME);
+                rmActionService.executeRecordsManagementAction(recordVersion11, CompleteEventAction.NAME, params);  
+                
+                rmActionService.executeRecordsManagementAction(recordVersion11, CutOffAction.NAME);
+                
+                rmActionService.executeRecordsManagementAction(recordVersion11, DestroyAction.NAME);
+            }                       
+            
+            public void then()
+            {
+                // verify that the version history looks as expected
+                VersionHistory versionHistory = versionService.getVersionHistory(myDocument);
+                assertNotNull(versionHistory);                
+                Collection<Version> versions = versionHistory.getAllVersions();
+                assertEquals(3, versions.size());
+                
+                // verify 1.2 setup as expected
+                Version version12 = versionHistory.getHeadVersion();
+                assertEquals("1.2", version12.getVersionLabel());
+                assertFalse(recordableVersionService.isRecordedVersionDestroyed(version12));
+                NodeRef recordVersion12 = recordableVersionService.getVersionRecord(version12);
+                assertNotNull(recordVersion12);
+                assertFalse(recordService.isMetadataStub(recordVersion12));
+                
+                assertTrue(relationshipService.getRelationshipsTo(recordVersion12, "versions").isEmpty());
+                
+                Set<Relationship> from12 = relationshipService.getRelationshipsFrom(recordVersion12, "versions");
+                assertEquals(1, from12.size());
+                
+                // verify 1.1 setup as expected
+                Version version11 = versionHistory.getPredecessor(version12);
+                assertEquals("1.1", version11.getVersionLabel());
+                assertTrue(recordableVersionService.isRecordedVersionDestroyed(version11));
+                assertNotNull(recordVersion11);
+                assertTrue(recordService.isMetadataStub(recordVersion11));
+                
+                Set<Relationship> to11 = relationshipService.getRelationshipsTo(recordVersion11, "versions");
+                assertEquals(1, to11.size());
+                assertEquals(recordVersion12, to11.iterator().next().getSource());
+                
+                Set<Relationship> from11 = relationshipService.getRelationshipsFrom(recordVersion11, "versions");
+                assertEquals(1, from11.size());
+                
+                // verify 1.0 setup as expected
+                Version version10 = versionHistory.getPredecessor(version11);
+                assertEquals("1.0", version10.getVersionLabel());
+                assertFalse(recordableVersionService.isRecordedVersionDestroyed(version10));
+                NodeRef recordVersion10 = recordableVersionService.getVersionRecord(version10);
+                assertNotNull(recordVersion10);
+                assertFalse(recordService.isMetadataStub(recordVersion10));
+                
+                Set<Relationship> to10 = relationshipService.getRelationshipsTo(recordVersion10, "versions");
+                assertEquals(1, to10.size());
+                assertEquals(recordVersion11, to10.iterator().next().getSource());
+                
+                assertTrue(relationshipService.getRelationshipsFrom(recordVersion10, "versions").isEmpty());
+
+            }
+        }); 
+    }
 }

rm-server/unit-test/java/org/alfresco/module/org_alfresco_module_rm/model/rma/aspect/VersionRecordAspectUnitTest.java

@@ -18,7 +18,6 @@
  */
 package org.alfresco.module.org_alfresco_module_rm.model.rma.aspect;
 
-import static org.alfresco.module.org_alfresco_module_rm.version.RecordableVersionModel.PROP_VERSIONED_NODEREF;
 import static org.mockito.Matchers.any;
 import static org.mockito.Mockito.never;
 import static org.mockito.Mockito.verify;
@@ -53,7 +52,7 @@ public class VersionRecordAspectUnitTest extends BaseUnitTest
     private @InjectMocks VersionRecordAspect versionRecordAspect;
     
     /**
-     * given that version node ref is null
+     * given that there is no recorded version
      * before delete of record
      * then nothing happens
      */
@@ -62,7 +61,7 @@ public class VersionRecordAspectUnitTest extends BaseUnitTest
     {
         NodeRef nodeRef = generateNodeRef();
         
-        when(mockedNodeService.getProperty(nodeRef, PROP_VERSIONED_NODEREF))
+        when(mockedRecordableVersionService.getRecordedVersion(nodeRef))
             .thenReturn(null);
         
         versionRecordAspect.beforeDeleteNode(nodeRef);
@@ -72,87 +71,7 @@ public class VersionRecordAspectUnitTest extends BaseUnitTest
     }
      
     /**
-     * given that version node ref is not null
+     * given that there is a recorded version
-     * and version label is null
-     * before delete of record
-     * then nothing happens
-     */
-    @Test
-    public void beforeDeleteNoVersionLabel()
-    {
-        NodeRef nodeRef = generateNodeRef();
-        NodeRef versionedNodeRef = generateNodeRef();
-        
-        when(mockedNodeService.getProperty(nodeRef, PROP_VERSIONED_NODEREF))
-            .thenReturn(versionedNodeRef);
-        when(mockedNodeService.getProperty(nodeRef, RecordableVersionModel.PROP_VERSION_LABEL))
-            .thenReturn(null);
-        
-        versionRecordAspect.beforeDeleteNode(nodeRef);
-        
-        verify(mockedVersionService, never()).getVersionHistory(versionedNodeRef);      
-        verify(mockedRecordableVersionService, never()).destroyRecordedVersion(any(Version.class)); 
-    }
-    
-    /**
-     * given that version node ref is not null
-     * and version label is not null
-     * and version history is null
-     * before delete of record
-     * then nothing happens
-     */
-    @Test
-    public void beforeDeleteNoVersionHistory()
-    {
-        NodeRef nodeRef = generateNodeRef();
-        NodeRef versionedNodeRef = generateNodeRef();
-        
-        when(mockedNodeService.getProperty(nodeRef, PROP_VERSIONED_NODEREF))
-            .thenReturn(versionedNodeRef);
-        when(mockedNodeService.getProperty(nodeRef, RecordableVersionModel.PROP_VERSION_LABEL))
-            .thenReturn(generateText());
-        when(mockedVersionService.getVersionHistory(versionedNodeRef))
-            .thenReturn(null);
-        
-        versionRecordAspect.beforeDeleteNode(nodeRef);
-           
-        verify(mockedRecordableVersionService, never()).destroyRecordedVersion(any(Version.class)); 
-    }
-    
-    /**
-     * given that version node ref is not null
-     * and version label is not null
-     * and version history is not null
-     * and the version relating to the version label is null
-     * before delete of record
-     * then nothing happens
-     */
-    @Test
-    public void beforeDeleteNoVersionForLabel()
-    {
-        NodeRef nodeRef = generateNodeRef();
-        NodeRef versionedNodeRef = generateNodeRef();
-        String versionLabel = generateText();
-        
-        when(mockedNodeService.getProperty(nodeRef, PROP_VERSIONED_NODEREF))
-            .thenReturn(versionedNodeRef);
-        when(mockedNodeService.getProperty(nodeRef, RecordableVersionModel.PROP_VERSION_LABEL))
-            .thenReturn(versionLabel);
-        when(mockedVersionService.getVersionHistory(versionedNodeRef))
-            .thenReturn(mockedVersionHistory);        
-        when(mockedVersionHistory.getVersion(versionLabel))
-            .thenReturn(null);
-        
-        versionRecordAspect.beforeDeleteNode(nodeRef);
-           
-        verify(mockedRecordableVersionService, never()).destroyRecordedVersion(any(Version.class)); 
-    }
-    
-    /**
-     * given that version node ref is not null
-     * and version label is not null
-     * and version history is not null
-     * and the version relating to the version label is not null
      * before delete of record
      * then the version is marked as destroyed
      */
@@ -160,16 +79,8 @@ public class VersionRecordAspectUnitTest extends BaseUnitTest
     public void beforeDeleteMarkVersionDestroyed()
     {
         NodeRef nodeRef = generateNodeRef();
-        NodeRef versionedNodeRef = generateNodeRef();
-        String versionLabel = generateText();
         
-        when(mockedNodeService.getProperty(nodeRef, PROP_VERSIONED_NODEREF))
+        when(mockedRecordableVersionService.getRecordedVersion(nodeRef))
-            .thenReturn(versionedNodeRef);
-        when(mockedNodeService.getProperty(nodeRef, RecordableVersionModel.PROP_VERSION_LABEL))
-            .thenReturn(versionLabel);
-        when(mockedVersionService.getVersionHistory(versionedNodeRef))
-            .thenReturn(mockedVersionHistory);        
-        when(mockedVersionHistory.getVersion(versionLabel))
             .thenReturn(mockedVersion);
         
         versionRecordAspect.beforeDeleteNode(nodeRef);