JBPM-5841: Round Robin Assignment Strategy

* add more unit tests
* fix ignored excluded users

jbpm-human-task/jbpm-human-task-core/src/main/java/org/jbpm/services/task/assignment/impl/strategy/RoundRobinAssignmentStrategy.java

@@ -15,6 +15,7 @@
 
 package org.jbpm.services.task.assignment.impl.strategy;
 
+import java.util.ArrayList;
 import java.util.Iterator;
 import java.util.List;
 import java.util.Map;
@@ -36,12 +37,14 @@
 import org.slf4j.LoggerFactory;
 
 public class RoundRobinAssignmentStrategy implements AssignmentStrategy {
+
     private static final Logger logger = LoggerFactory.getLogger(RoundRobinAssignmentStrategy.class);
     private static final String IDENTIFIER = "RoundRobin";
 
-    private Map<String,CircularQueue<OrganizationalEntity>> circularQueueMap = new ConcurrentHashMap();
+    private Map<String, CircularQueue<OrganizationalEntity>> circularQueueMap = new ConcurrentHashMap<>();
 
     private class CircularQueue<T> extends LinkedBlockingQueue<T> {
+
         @Override
         public synchronized T take() {
             T headValue = null;
@@ -50,7 +53,7 @@ public synchronized T take() {
                 super.offer(headValue);
             } catch (InterruptedException e) {
                 logger.error("Thread interrupted during the 'take' from a circular queue in the " +
-                                     "RoundRobinAssignmentStrategy",e);
+                        "RoundRobinAssignmentStrategy", e);
             }
             return headValue;
         }
@@ -62,9 +65,9 @@ public String getIdentifier() {
     }
 
     @Override
-    public Assignment apply(Task task, TaskContext taskContext, String s) {
-        List<OrganizationalEntity> excluded = ((InternalPeopleAssignments)task.getPeopleAssignments()).getExcludedOwners();
-        UserInfo userInfo = (UserInfo) ((org.jbpm.services.task.commands.TaskContext)taskContext).get(EnvironmentName.TASK_USER_INFO);
+    public Assignment apply(Task task, TaskContext taskContext, String excludedUser) {
+        UserInfo userInfo = (UserInfo) ((org.jbpm.services.task.commands.TaskContext) taskContext).get(EnvironmentName.TASK_USER_INFO);
+        List<OrganizationalEntity> excluded = getExcludedEntities(task, userInfo);
 
         // Get the the users from the task's the potential owners
         List<OrganizationalEntity> potentialOwners = task.getPeopleAssignments().getPotentialOwners().stream()
@@ -74,7 +77,7 @@ public Assignment apply(Task task, TaskContext taskContext, String s) {
         // Get the users belonging to groups that are potential owners
         task.getPeopleAssignments().getPotentialOwners().stream().filter(oe -> oe instanceof Group)
                 .forEach(oe -> {
-                    Iterator<OrganizationalEntity> groupUsers = userInfo.getMembersForGroup((Group)oe);
+                    Iterator<OrganizationalEntity> groupUsers = userInfo.getMembersForGroup((Group) oe);
                     if (groupUsers != null) {
                         groupUsers.forEachRemaining(user -> {
                             if (user != null && !excluded.contains(user) && !potentialOwners.contains(user)) {
@@ -83,8 +86,14 @@ public Assignment apply(Task task, TaskContext taskContext, String s) {
                         });
                     }
                 });
+
+        if (excludedUser != null) {
+            logger.debug("Removing excluded user {} from the list of eligible users", excludedUser);
+            potentialOwners.removeIf(entity -> entity.getId().equals(excludedUser));
+        }
+
         String queueName = getQueueName(task);
-        CircularQueue<OrganizationalEntity> mappedQueue = synchronizedQueue(queueName,potentialOwners);
+        CircularQueue<OrganizationalEntity> mappedQueue = synchronizedQueue(queueName, potentialOwners);
         OrganizationalEntity owner = mappedQueue.take();
         return new Assignment(owner.getId());
     }
@@ -97,24 +106,22 @@ public Assignment apply(Task task, TaskContext taskContext, String s) {
      * @return The CircularQueue that contains all potential owners
      */
     private synchronized CircularQueue<OrganizationalEntity> synchronizedQueue(String queueName,
-                                                                              List<OrganizationalEntity> potentialOwners) {
-    	CircularQueue<OrganizationalEntity> existingQueue = (queueName == null || queueName.trim().length() == 0) ? null:circularQueueMap.get(queueName);
-    	//
-    	// If the queue does not exist then a new CircularQueue should be created
-    	//
-        final CircularQueue<OrganizationalEntity> workingQueue = existingQueue != null ? existingQueue:new CircularQueue();
+                                                                               List<OrganizationalEntity> potentialOwners) {
+        CircularQueue<OrganizationalEntity> existingQueue = (queueName == null || queueName.trim().length() == 0) ? null : circularQueueMap.get(queueName);
+        // If the queue does not exist then a new CircularQueue should be created
+        final CircularQueue<OrganizationalEntity> workingQueue = existingQueue != null ? existingQueue : new CircularQueue();
         potentialOwners.forEach(po -> {
-            if (!queueContainsUser(workingQueue,po)) {
+            if (!queueContainsUser(workingQueue, po)) {
                 workingQueue.add(po);
             }
         });
         workingQueue.removeIf(oe -> !potentialOwners.contains(oe));
         circularQueueMap.put(queueName, workingQueue);
         return workingQueue;
     }
-    
+
     protected boolean queueContainsUser(CircularQueue<OrganizationalEntity> queue, OrganizationalEntity oe) {
-    	return queue.contains(oe);
+        return queue.contains(oe);
     }
 
     /**
@@ -124,6 +131,20 @@ protected boolean queueContainsUser(CircularQueue<OrganizationalEntity> queue, O
      * @return The generated queue name
      */
     protected String getQueueName(Task task) {
-        return task.getTaskData().getProcessId()+"_"+task.getTaskData().getDeploymentId()+"_"+task.getName();
+        return task.getTaskData().getProcessId() + "_" + task.getTaskData().getDeploymentId() + "_" + task.getName();
+    }
+
+    private static List<OrganizationalEntity> getExcludedEntities(Task task, UserInfo userInfo) {
+        List<OrganizationalEntity> excluded = ((InternalPeopleAssignments) task.getPeopleAssignments()).getExcludedOwners();
+
+        List<OrganizationalEntity> excludedUsers = new ArrayList<>();
+        for (OrganizationalEntity entity : excluded) {
+            if (entity instanceof Group) {
+                userInfo.getMembersForGroup((Group) entity).forEachRemaining(excludedUsers::add);
+            }
+        }
+        excluded.addAll(excludedUsers);
+
+        return excluded;
     }
 }

jbpm-human-task/jbpm-human-task-core/src/test/java/org/jbpm/services/task/assignment/AbstractAssignmentTest.java

@@ -0,0 +1,67 @@
+/*
+ * Copyright 2017 Red Hat, Inc. and/or its affiliates.
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * 
+ *      http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+*/
+
+package org.jbpm.services.task.assignment;
+
+import java.io.StringReader;
+import java.util.Collections;
+import java.util.List;
+
+import org.jbpm.services.task.HumanTaskServicesBaseTest;
+import org.jbpm.services.task.impl.factories.TaskFactory;
+import org.kie.api.task.model.OrganizationalEntity;
+import org.kie.api.task.model.Task;
+import org.kie.api.task.model.User;
+
+import static org.assertj.core.api.Assertions.*;
+
+public class AbstractAssignmentTest extends HumanTaskServicesBaseTest {
+
+    protected void assertPotentialOwners(Task task, int expectedSize, String... expectedNames) {
+        List<OrganizationalEntity> potentialOwners = task.getPeopleAssignments().getPotentialOwners();
+
+        assertThat(potentialOwners).hasSize(expectedSize);
+
+        if (expectedNames.length > 0) {
+            assertThat(potentialOwners).as("No match for expected potential owner name")
+                    .extracting(OrganizationalEntity::getId)
+                    .contains(expectedNames);
+        }
+    }
+
+    protected void assertActualOwner(Task task, String expectedActualOwner) {
+        User actualOwner = task.getTaskData().getActualOwner();
+        assertThat(actualOwner).as("No actual owner when expected").isNotNull();
+        assertThat(actualOwner.getId()).as("Not matching actual owner").isEqualTo(expectedActualOwner);
+    }
+
+    protected void assertNoActualOwner(Task task) {
+        assertThat(task.getTaskData().getActualOwner()).as("Actual owner present when not expected").isNull();
+    }
+
+    protected long createAndAssertTask(String taskExpression, String actualOwner, int expectedPotentialOwners,
+                                       String... expectedPotentialOwnerNames) {
+        Task task = TaskFactory.evalTask(new StringReader(taskExpression));
+        assertPotentialOwners(task, expectedPotentialOwners);
+
+        long taskId = taskService.addTask(task, Collections.emptyMap());
+
+        task = taskService.getTaskById(taskId);
+        assertPotentialOwners(task, expectedPotentialOwners, expectedPotentialOwnerNames);
+        assertActualOwner(task, actualOwner);
+
+        return taskId;
+    }
+}

jbpm-human-task/jbpm-human-task-core/src/test/java/org/jbpm/services/task/assignment/BusinessRuleAssignmentTest.java

@@ -47,7 +47,7 @@
 
 
 
-public class BusinessRuleAssignmentTest extends AbstractAssignmentTests {
+public class BusinessRuleAssignmentTest extends AbstractAssignmentTest {
 
     private static final Logger logger = LoggerFactory.getLogger(BusinessRuleAssignmentTest.class); 
 

jbpm-human-task/jbpm-human-task-core/src/test/java/org/jbpm/services/task/assignment/PotentialOwnerBusynessAssignmentTest.java

@@ -38,7 +38,7 @@
 
 
 
-public class PotentialOwnerBusynessAssignmentTest extends AbstractAssignmentTests {
+public class PotentialOwnerBusynessAssignmentTest extends AbstractAssignmentTest {
 
 	private PoolingDataSource pds;
 	private EntityManagerFactory emf;