Fix for CLOUD-1824 (partial).

This fix addresses the creation of 'ghost tasks' - or more correctly, the failure to properly clean up ghost tasks.
There was already code to automatically delete invitation tasks when a site was deleted, but it was no longer working and had no test coverage.

This checkin adds a test case for this issue and also fixes it.
I also added & improved logging in various places.
The bug was in ActionExecuterAbstractBase, where an action run on a non-existent, but non-null, NodeRef was considered to be run on a locked node and hence not run.



git-svn-id: https://svn.alfresco.com/repos/alfresco-enterprise/alfresco/HEAD/root@52158 c4b6b30b-aa2e-2d43-bbcb-ca4b014f7261

source/java/org/alfresco/repo/action/executer/ActionExecuterAbstractBase.java

@@ -241,8 +241,18 @@ public abstract class ActionExecuterAbstractBase extends ParameterizedItemAbstra
         
         // Only execute the action if this action is read only or the actioned upon node reference doesn't
         // have a lock applied for this user.
-        if ((baseNodeService == null || actionedUponNodeRef == null || mlAwareNodeService.exists(actionedUponNodeRef)) && // Not all actions are node based
+        boolean nodeIsLockedForThisUser = false;
-            (ignoreLock || !LockUtils.isLockedAndReadOnly(actionedUponNodeRef, lockService)))
+        
+        // null nodeRefs can't be locked and some actions can be run against 'null' nodes.
+        // non-existent nodes can't be locked.
+        if (!ignoreLock &&
+            actionedUponNodeRef != null &&
+            mlAwareNodeService.exists(actionedUponNodeRef))
+        {
+            nodeIsLockedForThisUser = LockUtils.isLockedAndReadOnly(actionedUponNodeRef, lockService);
+        }
+        
+        if ( !nodeIsLockedForThisUser)
         {
             // Execute the implementation
             executeImpl(action, actionedUponNodeRef);

source/java/org/alfresco/repo/invitation/InvitationCleanupTest.java

@@ -0,0 +1,182 @@
+/*
+ * Copyright (C) 2005-2013 Alfresco Software Limited.
+ *
+ * This file is part of Alfresco
+ *
+ * Alfresco is free software: you can redistribute it and/or modify
+ * it under the terms of the GNU Lesser General Public License as published by
+ * the Free Software Foundation, either version 3 of the License, or
+ * (at your option) any later version.
+ *
+ * Alfresco is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU Lesser General Public License for more details.
+ *
+ * You should have received a copy of the GNU Lesser General Public License
+ * along with Alfresco. If not, see <http://www.gnu.org/licenses/>.
+ */
+
+package org.alfresco.repo.invitation;
+
+import static org.junit.Assert.assertEquals;
+
+import java.util.ArrayList;
+import java.util.List;
+
+import org.alfresco.repo.security.authentication.AuthenticationUtil;
+import org.alfresco.repo.security.authentication.AuthenticationUtil.RunAsWork;
+import org.alfresco.repo.site.SiteModel;
+import org.alfresco.repo.transaction.RetryingTransactionHelper;
+import org.alfresco.repo.transaction.RetryingTransactionHelper.RetryingTransactionCallback;
+import org.alfresco.service.cmr.invitation.Invitation.ResourceType;
+import org.alfresco.service.cmr.invitation.InvitationService;
+import org.alfresco.service.cmr.site.SiteService;
+import org.alfresco.service.cmr.site.SiteVisibility;
+import org.alfresco.service.cmr.workflow.WorkflowService;
+import org.alfresco.service.cmr.workflow.WorkflowTask;
+import org.alfresco.service.cmr.workflow.WorkflowTaskState;
+import org.alfresco.util.test.junitrules.AlfrescoPerson;
+import org.alfresco.util.test.junitrules.ApplicationContextInit;
+import org.alfresco.util.test.junitrules.TemporarySites;
+import org.junit.BeforeClass;
+import org.junit.ClassRule;
+import org.junit.Test;
+import org.junit.rules.RuleChain;
+
+/**
+ * Test class which ensures that workflows created by {@link InvitationService} are correctly cleaned up when no longer needed.
+ * 
+ * @author Neil Mc Erlean
+ * @since 4.2
+ */
+public class InvitationCleanupTest
+{
+    // Rule to initialise the default Alfresco spring configuration
+    public static ApplicationContextInit APP_CONTEXT_INIT = new ApplicationContextInit();
+    
+    // Rules to create 2 test users.
+    public static AlfrescoPerson TEST_USER1 = new AlfrescoPerson(APP_CONTEXT_INIT);
+    public static AlfrescoPerson TEST_USER2 = new AlfrescoPerson(APP_CONTEXT_INIT);
+    
+    // Rule to manage test sites
+    public static TemporarySites TEST_SITES = new TemporarySites(APP_CONTEXT_INIT);
+    
+    // Tie them together in a static Rule Chain
+    @ClassRule public static RuleChain ruleChain = RuleChain.outerRule(APP_CONTEXT_INIT)
+                                                            .around(TEST_USER1)
+                                                            .around(TEST_USER2)
+                                                            .around(TEST_SITES);
+    
+    // Various services
+    private static InvitationService         INVITATION_SERVICE;
+    private static SiteService               SITE_SERVICE;
+    private static RetryingTransactionHelper TRANSACTION_HELPER;
+    private static WorkflowService           WORKFLOW_SERVICE;
+    
+    @BeforeClass public static void initStaticData() throws Exception
+    {
+        INVITATION_SERVICE = APP_CONTEXT_INIT.getApplicationContext().getBean("InvitationService", InvitationService.class);
+        SITE_SERVICE       = APP_CONTEXT_INIT.getApplicationContext().getBean("SiteService", SiteService.class);
+        TRANSACTION_HELPER = APP_CONTEXT_INIT.getApplicationContext().getBean("retryingTransactionHelper", RetryingTransactionHelper.class);
+        WORKFLOW_SERVICE   = APP_CONTEXT_INIT.getApplicationContext().getBean("WorkflowService", WorkflowService.class);
+    }
+    
+    /** See CLOUD-1824 for details on bug &amp; ALF-11872 for details on a related, older bug. */
+    @Test public void pendingJoinRequestsToModeratedSitesShouldDisappearOnSiteDeletion() throws Exception
+    {
+        // First, UserOne creates a site - we'll name it after this test method.
+        final String siteShortName = InvitationCleanupTest.class.getSimpleName() + "-test-site";
+        
+        TEST_SITES.createSite("sitePreset", siteShortName, "title", "description", SiteVisibility.MODERATED, TEST_USER1.getUsername());
+        
+        // Now UserTwo makes a request to join the site
+        AuthenticationUtil.runAs(new RunAsWork<Void>()
+        {
+            @Override public Void doWork() throws Exception
+            {
+                return TRANSACTION_HELPER.doInTransaction(new RetryingTransactionCallback<Void>()
+                {
+                    public Void execute() throws Throwable
+                    {
+                        INVITATION_SERVICE.inviteModerated("Let me in!",
+                                                           TEST_USER2.getUsername(),
+                                                           ResourceType.WEB_SITE,
+                                                           siteShortName,
+                                                           SiteModel.SITE_CONTRIBUTOR);
+                        return null;
+                    }
+                });
+            }
+        }, TEST_USER2.getUsername());
+        
+        AuthenticationUtil.runAs(new RunAsWork<Void>()
+        {
+            @Override public Void doWork() throws Exception
+            {
+                TRANSACTION_HELPER.doInTransaction(new RetryingTransactionCallback<Void>()
+                {
+                    public Void execute() throws Throwable
+                    {
+                        // Sanity check: UserOne should now have a task assigned to them
+                        assertUserHasTasks(TEST_USER1.getUsername(), 1);
+                        
+                        return null;
+                    }
+                });
+                
+                TRANSACTION_HELPER.doInTransaction(new RetryingTransactionCallback<Void>()
+                {
+                    public Void execute() throws Throwable
+                    {
+                        // UserOne ignores the task and instead deletes the site.
+                        SITE_SERVICE.deleteSite(siteShortName);
+                        
+                        return null;
+                    }
+                });
+                
+                // The pending inviations are deleted asynchronously and so we must wait for that deletion to complete.
+                // TODO Obviously Thread.sleep is not the best way to do this.
+                Thread.sleep(1000);
+                
+                TRANSACTION_HELPER.doInTransaction(new RetryingTransactionCallback<Void>()
+                {
+                    public Void execute() throws Throwable
+                    {
+                        // UserOne now creates a new site WITH THE SAME NAME.
+                        TEST_SITES.createSite("sitePreset", siteShortName, "title", "description", SiteVisibility.MODERATED, TEST_USER1.getUsername());
+                        
+                        return null;
+                    }
+                });
+                
+                TRANSACTION_HELPER.doInTransaction(new RetryingTransactionCallback<Void>()
+                {
+                    public Void execute() throws Throwable
+                    {
+                        // Now the task should remain unseen/deleted.
+                        assertUserHasTasks(TEST_USER1.getUsername(), 0);
+                        
+                        return null;
+                    }
+                });
+                
+                return null;
+            }
+        }, TEST_USER1.getUsername());
+    }
+    
+    private List<WorkflowTask> assertUserHasTasks(final String username, final int number)
+    {
+        List<WorkflowTask> tasks = WORKFLOW_SERVICE.getAssignedTasks(username, WorkflowTaskState.IN_PROGRESS);
+        
+        // Need mutable collection.
+        List<WorkflowTask> allTasks = new ArrayList<WorkflowTask>();
+        allTasks.addAll(tasks);
+        allTasks.addAll(WORKFLOW_SERVICE.getPooledTasks(username));
+        
+        assertEquals("Wrong number of tasks assigned to user", number, allTasks.size());
+        return tasks;
+    }
+}

source/java/org/alfresco/repo/invitation/InvitationServiceImpl.java

@@ -808,6 +808,7 @@ public class InvitationServiceImpl implements InvitationService, NodeServicePoli
                 }
             } 
         }
+        
         return invitationIds;
 
     }
@@ -887,15 +888,19 @@ public class InvitationServiceImpl implements InvitationService, NodeServicePoli
             if(jbpmTasks !=null)
             {
                 results.addAll(jbpmTasks);
-            }
+                
+                if (logger.isTraceEnabled()) { logger.trace("Found " + jbpmTasks.size() + " jBPM moderated invitation tasks."); }
+           }
         }
         if(workflowAdminService.isEngineEnabled(ActivitiConstants.ENGINE_ID))
         {
             query.setTaskName(WorkflowModelModeratedInvitation.WF_ACTIVITI_REVIEW_TASK);
-            List<WorkflowTask> jbpmTasks = this.workflowService.queryTasks(query, true);
+            List<WorkflowTask> activitiTasks = this.workflowService.queryTasks(query, true);
-            if(jbpmTasks !=null)
+            if(activitiTasks !=null)
             {
-                results.addAll(jbpmTasks);
+                results.addAll(activitiTasks);
+                
+                if (logger.isTraceEnabled()) { logger.trace("Found " + activitiTasks.size() + " Activiti moderated invitation tasks."); }
             }
         }
         if (logger.isDebugEnabled())
@@ -948,15 +953,19 @@ public class InvitationServiceImpl implements InvitationService, NodeServicePoli
             if(jbpmTasks !=null)
             {
                 results.addAll(jbpmTasks);
+                
+                if (logger.isTraceEnabled()) { logger.trace("Found " + jbpmTasks.size() + " jBPM nominated invitation tasks."); }
             }
         }
         if(workflowAdminService.isEngineEnabled(ActivitiConstants.ENGINE_ID))
         {
             query.setTaskName(WorkflowModelNominatedInvitation.WF_TASK_ACTIVIT_INVITE_PENDING);
-            List<WorkflowTask> jbpmTasks = this.workflowService.queryTasks(query, true);
+            List<WorkflowTask> activitiTasks = this.workflowService.queryTasks(query, true);
-            if(jbpmTasks !=null)
+            if(activitiTasks !=null)
             {
-                results.addAll(jbpmTasks);
+                results.addAll(activitiTasks);
+                
+                if (logger.isTraceEnabled()) { logger.trace("Found " + activitiTasks.size() + " Activiti nominated invitation tasks."); }
             }
         }
         if (logger.isDebugEnabled())
@@ -1547,7 +1556,7 @@ public class InvitationServiceImpl implements InvitationService, NodeServicePoli
      */
     public void beforeDeleteNode(NodeRef nodeRef)
     {
-        logger.debug("beforeDeleteNode");
+        if (logger.isDebugEnabled()) { logger.debug("beforeDeleteNode"); }
 
         final NodeRef siteRef = nodeRef;
 
@@ -1567,7 +1576,7 @@ public class InvitationServiceImpl implements InvitationService, NodeServicePoli
                         long start =0;
                         if (logger.isDebugEnabled())
                         {
-                            logger.debug("Invitation service delete node fired " + type + ", " + siteName);
+                            logger.debug("Invitation service beforeDeleteNode fired " + type + ", " + siteName);
                             start = System.currentTimeMillis();
                         }
                         InvitationSearchCriteriaImpl criteria =
@@ -1585,13 +1594,13 @@ public class InvitationServiceImpl implements InvitationService, NodeServicePoli
                         Action action = actionService.createAction(CancelWorkflowActionExecuter.NAME);
                         action.setParameterValue(CancelWorkflowActionExecuter.PARAM_WORKFLOW_ID_LIST, (Serializable)invitationIds);
                                                 
-                        // Cancel the workflows asynchronously
+                        // Cancel the workflows asynchronously - see ALF-11872 (svn rev 32936 for details on why this is asynchronous).
-                        actionService.executeAction(action, siteRef, false, true);        
+                        actionService.executeAction(action, siteRef, false, true); // FIXME Here's the fix - make the steRef null
                         
                         if (logger.isDebugEnabled())
                         {
                             long end = System.currentTimeMillis();
-                            logger.debug("Invitations cancelled: " + invitationIds.size() + " in "+ (end-start) + " ms");
+                            logger.debug("Invitation cancellations requested: " + invitationIds.size() + " in "+ (end-start) + " ms");
                         }
                     }
                 }

source/java/org/alfresco/repo/workflow/CancelWorkflowActionExecuter.java

@@ -27,12 +27,16 @@ import org.alfresco.service.cmr.action.ParameterDefinition;
 import org.alfresco.service.cmr.dictionary.DataTypeDefinition;
 import org.alfresco.service.cmr.repository.NodeRef;
 import org.alfresco.service.cmr.workflow.WorkflowService;
+import org.apache.commons.logging.Log;
+import org.apache.commons.logging.LogFactory;
 
 /**
  * @author dward
  */
 public class CancelWorkflowActionExecuter extends ActionExecuterAbstractBase
 {
+    protected static Log log = LogFactory.getLog(CancelWorkflowActionExecuter.class);
+
     public static String NAME = "cancel-workflow";
     
     public static final String PARAM_WORKFLOW_ID_LIST = "workflow-id-list";   // list of workflow IDs
@@ -55,6 +59,9 @@ public class CancelWorkflowActionExecuter extends ActionExecuterAbstractBase
     {
         @SuppressWarnings("unchecked")
         List<String> workflowIds = (List<String>) action.getParameterValue(PARAM_WORKFLOW_ID_LIST);
+        
+        if (log.isTraceEnabled()) { log.trace("Cancelling " + (workflowIds == null ? 0 : workflowIds.size()) + " workflows by ID."); }
+        
         if (workflowIds != null && !workflowIds.isEmpty())
         {
             this.workflowService.cancelWorkflows(workflowIds);

source/java/org/alfresco/repo/workflow/WorkflowServiceImpl.java

@@ -570,6 +570,8 @@ public class WorkflowServiceImpl implements WorkflowService
      */
     public List<WorkflowInstance> cancelWorkflows(List<String> workflowIds)
     {
+        if (logger.isTraceEnabled()) { logger.trace("Cancelling " + (workflowIds == null ? 0 : workflowIds.size()) + " workflowIds..."); }
+        
         List<WorkflowInstance> result = new ArrayList<WorkflowInstance>(workflowIds.size());
         
         // Batch the workflow IDs by engine ID