TSK-1143 Update Task PLANNED - Holger's comments

lib/taskana-core/src/main/java/pro/taskana/common/internal/util/DaysToWorkingDaysConverter.java

@@ -3,6 +3,7 @@ package pro.taskana.common.internal.util;
 import static java.time.temporal.ChronoUnit.DAYS;
 
 import java.time.DayOfWeek;
+import java.time.Duration;
 import java.time.Instant;
 import java.time.LocalDate;
 import java.time.LocalDateTime;
@@ -82,6 +83,16 @@ public final class DaysToWorkingDaysConverter {
         .orElse(0);
   }
 
+  public Instant addWorkingDaysToInstant(Instant instant, Duration workingDays) {
+    long days = convertWorkingDaysToDays(instant, workingDays.toDays());
+    return instant.plus(Duration.ofDays(days));
+  }
+
+  public Instant subtractWorkingDaysFromInstant(Instant instant, Duration workingDays) {
+    long days = convertWorkingDaysToDays(instant, -workingDays.toDays());
+    return instant.plus(Duration.ofDays(days));
+  }
+
   public boolean isWorkingDay(long day, Instant referenceDate) {
     LocalDateTime dateToCheck =
         LocalDateTime.ofInstant(referenceDate, ZoneId.systemDefault()).plusDays(day);

lib/taskana-core/src/main/java/pro/taskana/task/internal/AttachmentHandler.java

@@ -79,7 +79,7 @@ public class AttachmentHandler {
   }
 
   void insertAndDeleteAttachmentsOnTaskUpdate(TaskImpl newTaskImpl, TaskImpl oldTaskImpl)
-      throws AttachmentPersistenceException {
+      throws InvalidArgumentException, AttachmentPersistenceException {
     if (LOGGER.isDebugEnabled()) {
       LOGGER.debug(
           "entry to insertAndDeleteAttachmentsOnTaskUpdate(oldTaskImpl = {}, newTaskImpl = {})",
@@ -122,40 +122,44 @@ public class AttachmentHandler {
   }
 
   void insertNewAttachmentsOnTaskUpdate(TaskImpl newTaskImpl, TaskImpl oldTaskImpl)
-      throws AttachmentPersistenceException {
+      throws InvalidArgumentException, AttachmentPersistenceException {
     List<String> oldAttachmentIds =
         oldTaskImpl.getAttachments().stream()
             .map(AttachmentSummary::getId)
             .collect(Collectors.toList());
-    List<AttachmentPersistenceException> exceptions = new ArrayList<>();
+    List<InvalidArgumentException> invalidArgumentExceptions = new ArrayList<>();
+    List<AttachmentPersistenceException> attachmentPersistenceExceptions = new ArrayList<>();
     newTaskImpl
         .getAttachments()
         .forEach(
             a -> {
               if (!oldAttachmentIds.contains(a.getId())) {
                 try {
-                  insertNewAttachmentOnTaskUpdate(newTaskImpl, a);
+                  initializeAndInsertAttachment(newTaskImpl, (AttachmentImpl) a);
+                } catch (InvalidArgumentException excpt) {
+                  invalidArgumentExceptions.add(excpt);
+                  LOGGER.warn("attempted to insert attachment {} and caught exception", a, excpt);
                 } catch (AttachmentPersistenceException excpt) {
-                  exceptions.add(excpt);
+                  attachmentPersistenceExceptions.add(excpt);
                   LOGGER.warn("attempted to insert attachment {} and caught exception", a, excpt);
                 }
               }
             });
-    if (!exceptions.isEmpty()) {
+    if (!invalidArgumentExceptions.isEmpty()) {
-      throw exceptions.get(0);
+      throw invalidArgumentExceptions.get(0);
+    }
+    if (!attachmentPersistenceExceptions.isEmpty()) {
+      throw attachmentPersistenceExceptions.get(0);
     }
   }
 
   void insertNewAttachmentsOnTaskCreation(TaskImpl task)
-      throws InvalidArgumentException {
+      throws InvalidArgumentException, AttachmentPersistenceException {
     List<Attachment> attachments = task.getAttachments();
     if (attachments != null) {
       for (Attachment attachment : attachments) {
         AttachmentImpl attachmentImpl = (AttachmentImpl) attachment;
-        initAttachment(attachmentImpl, task);
+        initializeAndInsertAttachment(task, attachmentImpl);
-        ObjectReference objRef = attachmentImpl.getObjectReference();
-        ObjectReference.validate(objRef, "ObjectReference", "Attachment");
-        attachmentMapper.insert(attachmentImpl);
       }
     }
   }
@@ -191,28 +195,6 @@ public class AttachmentHandler {
     LOGGER.debug("exit from deleteRemovedAttachmentsOnTaskUpdate()");
   }
 
-  void insertNewAttachmentOnTaskUpdate(TaskImpl newTaskImpl, Attachment attachment)
-      throws AttachmentPersistenceException {
-    LOGGER.debug("entry to insertNewAttachmentOnTaskUpdate()");
-    AttachmentImpl attachmentImpl = (AttachmentImpl) attachment;
-    initAttachment(attachmentImpl, newTaskImpl);
-
-    try {
-      attachmentMapper.insert(attachmentImpl);
-      LOGGER.debug(
-          "TaskService.updateTask() for TaskId={} INSERTED an Attachment={}.",
-          newTaskImpl.getId(),
-          attachmentImpl);
-    } catch (PersistenceException e) {
-      throw new AttachmentPersistenceException(
-          String.format(
-              "Cannot insert the Attachement %s for Task %s  because it already exists.",
-              attachmentImpl.getId(), newTaskImpl.getId()),
-          e.getCause());
-    }
-    LOGGER.debug("exit from insertNewAttachmentOnTaskUpdate(), returning");
-  }
-
   void initAttachment(AttachmentImpl attachment, Task newTask) {
     LOGGER.debug("entry to initAttachment()");
     if (attachment.getId() == null) {
@@ -230,4 +212,25 @@ public class AttachmentHandler {
     LOGGER.debug("exit from initAttachment()");
   }
 
+  private void initializeAndInsertAttachment(TaskImpl task, AttachmentImpl attachmentImpl)
+      throws AttachmentPersistenceException, InvalidArgumentException {
+    LOGGER.debug("entry to initializeAndInsertAttachment()");
+    initAttachment(attachmentImpl, task);
+    ObjectReference objRef = attachmentImpl.getObjectReference();
+    ObjectReference.validate(objRef, "ObjectReference", "Attachment");
+    try {
+      attachmentMapper.insert(attachmentImpl);
+      LOGGER.debug(
+          "TaskService.updateTask() for TaskId={} INSERTED an Attachment={}.",
+          task.getId(),
+          attachmentImpl);
+    } catch (PersistenceException e) {
+      throw new AttachmentPersistenceException(
+          String.format(
+              "Cannot insert the Attachement %s for Task %s  because it already exists.",
+              attachmentImpl.getId(), task.getId()),
+          e.getCause());
+    }
+    LOGGER.debug("exit from initializeAndInsertAttachment()");
+  }
 }

lib/taskana-core/src/main/java/pro/taskana/task/internal/ServiceLevelHandler.java

@@ -68,7 +68,7 @@ class ServiceLevelHandler {
   // - For each task iterate through all referenced classifications and find minimum ServiceLevel
   // - collect the results into a map Duration -> List of tasks
   // - for each duration in this map update due date of all associated tasks
-  public BulkLog setPlannedPropertyOfTasksImpl(Instant planned, List<String> argTaskIds) {
+  BulkLog setPlannedPropertyOfTasksImpl(Instant planned, List<String> argTaskIds) {
     BulkLog bulkLog = new BulkLog();
     if (argTaskIds == null || argTaskIds.isEmpty()) {
       return bulkLog;
@@ -131,7 +131,8 @@ class ServiceLevelHandler {
     }
     // classification update
     if (forRefreshOnClassificationUpdate) {
-      newTaskImpl.setDue(newPlannedDueInstant(newTaskImpl, durationPrioHolder.getDuration(), true));
+      newTaskImpl.setDue(converter.addWorkingDaysToInstant(newTaskImpl.getPlanned(),
+          durationPrioHolder.getDuration()));
       return newTaskImpl;
     }
     // creation of new task
@@ -142,16 +143,6 @@ class ServiceLevelHandler {
     }
   }
 
-  Instant newPlannedDueInstant(TaskImpl task, Duration duration, boolean fromPlannedToDue) {
-    if (fromPlannedToDue) {
-      long days = converter.convertWorkingDaysToDays(task.getPlanned(), duration.toDays());
-      return task.getPlanned().plus(Duration.ofDays(days));
-    } else {
-      long days = converter.convertWorkingDaysToDays(task.getDue(), -duration.toDays());
-      return task.getDue().plus(Duration.ofDays(days));
-    }
-  }
-
   DurationPrioHolder determineTaskPrioDuration(TaskImpl newTaskImpl, boolean onlyPriority) {
     Set<ClassificationSummary> classificationsInvolved = getInvolvedClassifications(newTaskImpl);
 
@@ -220,18 +211,21 @@ class ServiceLevelHandler {
     // case 1: no change of planned / due, but potentially change of an attachment or classification
     if (oldTaskImpl.getDue().equals(newTaskImpl.getDue())
         && oldTaskImpl.getPlanned().equals(newTaskImpl.getPlanned())) {
-      newTaskImpl.setDue(newPlannedDueInstant(newTaskImpl, durationPrioHolder.getDuration(), true));
+      newTaskImpl.setDue(converter.addWorkingDaysToInstant(newTaskImpl.getPlanned(),
+          durationPrioHolder.getDuration()));
     } else if (oldTaskImpl.getDue().equals(newTaskImpl.getDue())
         && newTaskImpl.getPlanned() != null) {
       // case 2: planned was changed
-      newTaskImpl.setDue(newPlannedDueInstant(newTaskImpl, durationPrioHolder.getDuration(), true));
+      newTaskImpl.setDue(converter.addWorkingDaysToInstant(newTaskImpl.getPlanned(),
+          durationPrioHolder.getDuration()));
     } else { // case 3: due was changed
       if (newTaskImpl.getDue() == null) {
-        newTaskImpl.setDue(
+        newTaskImpl.setDue(converter.addWorkingDaysToInstant(newTaskImpl.getPlanned(),
-            newPlannedDueInstant(newTaskImpl, durationPrioHolder.getDuration(), true));
+            durationPrioHolder.getDuration()));
       } else {
         Instant planned =
-            newPlannedDueInstant(newTaskImpl, durationPrioHolder.getDuration(), false);
+            (converter.subtractWorkingDaysFromInstant(newTaskImpl.getDue(),
+                durationPrioHolder.getDuration()));
         if (newTaskImpl.getPlanned() != null && !planned.equals(newTaskImpl.getPlanned())) {
           throw new InvalidArgumentException(
               "Cannot update a task with given planned "
@@ -246,7 +240,8 @@ class ServiceLevelHandler {
   private TaskImpl updatePlannedDueOnCreationOfNewTask(
       TaskImpl newTaskImpl, DurationPrioHolder durationPrioHolder) throws InvalidArgumentException {
     if (newTaskImpl.getDue() != null) { // due is specified: calculate back and check correctnes
-      Instant planned = newPlannedDueInstant(newTaskImpl, durationPrioHolder.getDuration(), false);
+      Instant planned = (converter.subtractWorkingDaysFromInstant(newTaskImpl.getDue(),
+          durationPrioHolder.getDuration()));
       if (newTaskImpl.getPlanned() != null && !planned.equals(newTaskImpl.getPlanned())) {
         throw new InvalidArgumentException(
             "Cannot create a task with given planned "
@@ -254,7 +249,8 @@ class ServiceLevelHandler {
       }
       newTaskImpl.setPlanned(planned);
     } else { // task.due is null: calculate forward from planned
-      newTaskImpl.setDue(newPlannedDueInstant(newTaskImpl, durationPrioHolder.getDuration(), true));
+      newTaskImpl.setDue(converter.addWorkingDaysToInstant(newTaskImpl.getPlanned(),
+          durationPrioHolder.getDuration()));
     }
     return newTaskImpl;
   }

lib/taskana-core/src/main/java/pro/taskana/task/internal/TaskServiceImpl.java

@@ -413,6 +413,8 @@ public class TaskServiceImpl implements TaskService {
       newTaskImpl = checkConcurrencyAndSetModified(newTaskImpl, oldTaskImpl);
 
       attachmentHandler.insertAndDeleteAttachmentsOnTaskUpdate(newTaskImpl, oldTaskImpl);
+      ObjectReference.validate(newTaskImpl.getPrimaryObjRef(), "primary ObjectReference", TASK);
+
       standardUpdateActions(oldTaskImpl, newTaskImpl);
 
       taskMapper.update(newTaskImpl);
@@ -1191,7 +1193,12 @@ public class TaskServiceImpl implements TaskService {
     if (task.getDescription() == null && classification != null) {
       task.setDescription(classification.getDescription());
     }
-    attachmentHandler.insertNewAttachmentsOnTaskCreation(task);
+    try {
+      attachmentHandler.insertNewAttachmentsOnTaskCreation(task);
+    } catch (AttachmentPersistenceException e) {
+      throw new SystemException(
+          "Internal error when trying to insert new Attachments on Task Creation.", e);
+    }
     LOGGER.debug("exit from standardSettings()");
   }
 
@@ -1565,8 +1572,6 @@ public class TaskServiceImpl implements TaskService {
 
   private void standardUpdateActions(TaskImpl oldTaskImpl, TaskImpl newTaskImpl)
       throws InvalidArgumentException, InvalidStateException, ClassificationNotFoundException {
-    ObjectReference.validate(newTaskImpl.getPrimaryObjRef(), "primary ObjectReference", TASK);
-
     if (oldTaskImpl.getExternalId() == null
         || !(oldTaskImpl.getExternalId().equals(newTaskImpl.getExternalId()))) {
       throw new InvalidArgumentException(

lib/taskana-core/src/test/java/acceptance/task/ServiceLevelPriorityAccTest.java

@@ -50,10 +50,7 @@ public class ServiceLevelPriorityAccTest extends AbstractAccTest {
       userName = "user_3_2",
       groupNames = {"group_2"})
   @Test
-  void testSetPlannedWithDuplicatesSucceeds()
+  void testSetPlannedWithDuplicatesSucceeds() throws NotAuthorizedException, TaskNotFoundException {
-      throws NotAuthorizedException, TaskNotFoundException, ClassificationNotFoundException,
-          InvalidArgumentException, InvalidStateException, ConcurrencyException,
-          AttachmentPersistenceException {
 
     // This test works with the following tasks (w/o attachments) and classifications
     //
@@ -124,7 +121,7 @@ public class ServiceLevelPriorityAccTest extends AbstractAccTest {
   void testSetPlannedForTasksWithAttachmentsSucceeds()
       throws NotAuthorizedException, TaskNotFoundException, ClassificationNotFoundException,
           InvalidArgumentException, InvalidStateException, ConcurrencyException,
-          AttachmentPersistenceException, SQLException {
+          AttachmentPersistenceException {
 
     // This test works with the following tasks, attachments and classifications
     //
@@ -179,7 +176,6 @@ public class ServiceLevelPriorityAccTest extends AbstractAccTest {
     assertThat(dueBulk0).isEqualTo(planned.plus(1, ChronoUnit.DAYS));
     assertThat(dueBulk1).isEqualTo(planned.plus(2, ChronoUnit.DAYS));
     assertThat(dueBulk2).isEqualTo(planned.plus(1, ChronoUnit.DAYS));
-    long delta = Duration.between(planned, due1).toDays();
 
     assertThat(results.containsErrors()).isFalse();
     assertThat(dueBulk0).isEqualTo(due0);
@@ -253,8 +249,7 @@ public class ServiceLevelPriorityAccTest extends AbstractAccTest {
       userName = "admin",
       groupNames = {"group_2"})
   @Test
-  void testSetPlannedPropertyOnEmptyTasksList()
+  void testSetPlannedPropertyOnEmptyTasksList() {
-      throws NotAuthorizedException, TaskNotFoundException {
     Instant planned = getInstant("2020-05-03T07:00:00");
     BulkOperationResults<String, TaskanaException> results =
         taskanaEngine.getTaskService().setPlannedPropertyOfTasks(planned, new ArrayList<>());
@@ -274,9 +269,7 @@ public class ServiceLevelPriorityAccTest extends AbstractAccTest {
       groupNames = {"group_2"})
   @Test
   void testSetPlannedPropertyOnSingleTaskWithBulkUpdate()
-      throws NotAuthorizedException, TaskNotFoundException, InvalidArgumentException,
+      throws NotAuthorizedException, TaskNotFoundException, InvalidArgumentException {
-          ConcurrencyException, InvalidStateException, ClassificationNotFoundException,
-          AttachmentPersistenceException {
     String taskId = "TKI:000000000000000000000000000000000002";
     Instant planned = getInstant("2020-05-03T07:00:00");
     // test bulk operation setPlanned...
@@ -293,12 +286,11 @@ public class ServiceLevelPriorityAccTest extends AbstractAccTest {
       userName = "admin",
       groupNames = {"group_2"})
   @Test
-  void testSetPlannedPropertyOnSingleTaskWitHTaskUpdate()
+  void testSetPlannedPropertyOnSingleTaskWithTaskUpdate()
       throws NotAuthorizedException, TaskNotFoundException, InvalidArgumentException,
           ConcurrencyException, InvalidStateException, ClassificationNotFoundException,
           AttachmentPersistenceException {
     String taskId = "TKI:000000000000000000000000000000000002";
-    Instant planned = getInstant("2020-05-03T07:00:00");
     DaysToWorkingDaysConverter converter = DaysToWorkingDaysConverter.initialize();
     Task task = taskService.getTask(taskId);
     // test update of planned date via updateTask()
@@ -324,10 +316,9 @@ public class ServiceLevelPriorityAccTest extends AbstractAccTest {
     task.setDue(planned.plus(Duration.ofDays(8)));
     Task finalTask = task;
     assertThatThrownBy(
-        () -> {
+      () -> {
-            taskService.updateTask(finalTask);
+        taskService.updateTask(finalTask);
-        })
+      }).isInstanceOf(InvalidArgumentException.class);
-        .isInstanceOf(InvalidArgumentException.class);
 
     // update due and planned as expected.
     task = taskService.getTask(taskId);
@@ -359,7 +350,7 @@ public class ServiceLevelPriorityAccTest extends AbstractAccTest {
     String taskId = "TKI:000000000000000000000000000000000002";
     final Instant planned = getInstant("2020-05-03T07:00:00");
     Task task = taskService.getTask(taskId);
-    
+
     task.setPlanned(null);
     task = taskService.updateTask(task);
     DaysToWorkingDaysConverter converter = DaysToWorkingDaysConverter.initialize();
@@ -389,8 +380,7 @@ public class ServiceLevelPriorityAccTest extends AbstractAccTest {
       userName = "admin",
       groupNames = {"group_2"})
   @Test
-  void testSetPlannedPropertyOnAllTasks()
+  void testSetPlannedPropertyOnAllTasks() throws SQLException {
-      throws NotAuthorizedException, TaskNotFoundException, SQLException {
     Instant planned = getInstant("2020-05-03T07:00:00");
     List<TaskSummary> allTasks = taskService.createTaskQuery().list();
     // Now update each task with updateTask() and new planned
@@ -410,6 +400,19 @@ public class ServiceLevelPriorityAccTest extends AbstractAccTest {
         t -> assertThat(t.getDue().equals(bulkUpdatedTaskMap.get(t.getId()))));
   }
 
+  @WithAccessId(
+      userName = "user_1_2",
+      groupNames = {"group_1"})
+  @Test
+  void testUpdatePlannedAndDue() throws NotAuthorizedException, TaskNotFoundException {
+    TaskService taskService = taskanaEngine.getTaskService();
+    Task task = taskService.getTask("TKI:000000000000000000000000000000000030");
+    task.setPlanned(getInstant("2020-04-21T07:00:00"));
+    task.setDue(getInstant("2020-04-21T10:00:00"));
+    assertThatThrownBy(() -> taskService.updateTask(task))
+        .isInstanceOf(InvalidArgumentException.class);
+  }
+
   private TaskSummary getUpdatedTaskSummary(TaskSummary t, Instant planned) {
     try {
       Task task = taskService.getTask(t.getId());

lib/taskana-core/src/test/java/acceptance/task/UpdateTaskAccTest.java

@@ -74,8 +74,7 @@ class UpdateTaskAccTest extends AbstractAccTest {
       groupNames = {"group_1"})
   @Test
   void testThrowsExceptionIfMandatoryPrimaryObjectReferenceIsNotSetOrIncomplete()
-      throws NotAuthorizedException, ClassificationNotFoundException, TaskNotFoundException,
+      throws NotAuthorizedException, TaskNotFoundException {
-          ConcurrencyException, AttachmentPersistenceException {
 
     TaskService taskService = taskanaEngine.getTaskService();
     Task task = taskService.getTask("TKI:000000000000000000000000000000000000");
@@ -343,19 +342,4 @@ class UpdateTaskAccTest extends AbstractAccTest {
     assertThat(retrievedUpdatedTask.getCallbackInfo()).isEqualTo(callbackInfo);
   }
 
-  @WithAccessId(
-      userName = "user_1_2",
-      groupNames = {"group_1"})
-  @Test
-  void testUpdatePlannedAndDue()
-      throws NotAuthorizedException, TaskNotFoundException, ClassificationNotFoundException,
-          InvalidArgumentException, InvalidStateException, ConcurrencyException,
-          AttachmentPersistenceException {
-    TaskService taskService = taskanaEngine.getTaskService();
-    Task task = taskService.getTask("TKI:000000000000000000000000000000000030");
-    task.setPlanned(getInstant("2020-04-21T07:00:00"));
-    task.setDue(getInstant("2020-04-21T10:00:00"));
-    assertThatThrownBy(() -> taskService.updateTask(task))
-        .isInstanceOf(InvalidArgumentException.class);
-  }
 }