TSK-1468: Added validation of name and type of workbasket when updating Workbasket (#1366)

* TSK-1468: Added validation of name and type of workbasket when updating Workbasket

* TSK-1468: Improvements after review

lib/taskana-core/src/main/java/pro/taskana/workbasket/api/WorkbasketService.java

@@ -65,13 +65,15 @@ public interface WorkbasketService {
    *
    * @param workbasket The Workbasket to update
    * @return the updated Workbasket
+   * @throws InvalidWorkbasketException if workbasket name or type is invalid
    * @throws NotAuthorizedException if the current user is not authorized to update the work basket
    * @throws WorkbasketNotFoundException if the workbasket cannot be found.
    * @throws ConcurrencyException if an attempt is made to update the workbasket and another user
    *     updated it already
    */
   Workbasket updateWorkbasket(Workbasket workbasket)
-      throws NotAuthorizedException, WorkbasketNotFoundException, ConcurrencyException;
+      throws InvalidWorkbasketException, NotAuthorizedException, WorkbasketNotFoundException,
+          ConcurrencyException;
 
   /**
    * Returns a new WorkbasketAccessItem which is not persisted.

lib/taskana-core/src/main/java/pro/taskana/workbasket/internal/WorkbasketServiceImpl.java

@@ -173,38 +173,30 @@ public class WorkbasketServiceImpl implements WorkbasketService {
 
   @Override
   public Workbasket updateWorkbasket(Workbasket workbasketToUpdate)
-      throws NotAuthorizedException, WorkbasketNotFoundException, ConcurrencyException {
+      throws NotAuthorizedException, WorkbasketNotFoundException, ConcurrencyException,
+          InvalidWorkbasketException {
 
     LOGGER.debug("entry to updateWorkbasket(Workbasket = {})", workbasketToUpdate);
 
     taskanaEngine.getEngine().checkRoleMembership(TaskanaRole.BUSINESS_ADMIN, TaskanaRole.ADMIN);
-
+    WorkbasketImpl workbasketImplToUpdate = (WorkbasketImpl) workbasketToUpdate;
-    WorkbasketImpl workbasketImplToUpdate = null;
+    validateNameAndType(workbasketToUpdate);
 
     try {
-
       taskanaEngine.openConnection();
 
-      workbasketImplToUpdate = (WorkbasketImpl) workbasketToUpdate;
-
       Workbasket oldWorkbasket =
           this.getWorkbasket(workbasketImplToUpdate.getKey(), workbasketImplToUpdate.getDomain());
-
       checkModifiedHasNotChanged(oldWorkbasket, workbasketImplToUpdate);
-
       workbasketImplToUpdate.setModified(Instant.now());
 
       if (workbasketImplToUpdate.getId() == null || workbasketImplToUpdate.getId().isEmpty()) {
-
         workbasketMapper.updateByKeyAndDomain(workbasketImplToUpdate);
-
       } else {
-
         workbasketMapper.update(workbasketImplToUpdate);
       }
 
       if (HistoryEventManager.isHistoryEnabled()) {
-
         String details =
             ObjectAttributeChangeDetector.determineChangesInAttributes(
                 oldWorkbasket, workbasketToUpdate);
@@ -216,15 +208,13 @@ public class WorkbasketServiceImpl implements WorkbasketService {
                 taskanaEngine.getEngine().getCurrentUserContext().getUserid(),
                 details));
       }
+
       LOGGER.debug(
           "Method updateWorkbasket() updated workbasket '{}'", workbasketImplToUpdate.getId());
 
       return workbasketImplToUpdate;
-
     } finally {
-
       taskanaEngine.returnConnection();
-
       LOGGER.debug("exit from updateWorkbasket(). Returning result {} ", workbasketImplToUpdate);
     }
   }
@@ -841,7 +831,7 @@ public class WorkbasketServiceImpl implements WorkbasketService {
     LOGGER.debug("entry to deleteWorkbasket(workbasketId = {})", workbasketId);
     taskanaEngine.getEngine().checkRoleMembership(TaskanaRole.BUSINESS_ADMIN, TaskanaRole.ADMIN);
 
-    validateWorkbasketId(workbasketId);
+    validateId(workbasketId);
 
     try {
       taskanaEngine.openConnection();
@@ -1104,16 +1094,6 @@ public class WorkbasketServiceImpl implements WorkbasketService {
     return accessItems;
   }
 
-  private void validateWorkbasketId(String workbasketId) throws InvalidArgumentException {
-    if (workbasketId == null) {
-      throw new InvalidArgumentException("The WorkbasketId can´t be NULL");
-    }
-
-    if (workbasketId.isEmpty()) {
-      throw new InvalidArgumentException("The WorkbasketId can´t be EMPTY for deleteWorkbasket()");
-    }
-  }
-
   private long getCountTasksByWorkbasketId(String workbasketId) {
     return taskanaEngine
         .getEngine()
@@ -1158,20 +1138,17 @@ public class WorkbasketServiceImpl implements WorkbasketService {
   private void validateWorkbasket(Workbasket workbasket)
       throws InvalidWorkbasketException, DomainNotFoundException {
     // check that required properties (database not null) are set
+    validateNameAndType(workbasket);
+
     if (workbasket.getId() == null || workbasket.getId().length() == 0) {
       throw new InvalidWorkbasketException("Id must not be null for " + workbasket);
-    } else if (workbasket.getKey() == null || workbasket.getKey().length() == 0) {
-      throw new InvalidWorkbasketException("Key must not be null for " + workbasket);
     }
-    if (workbasket.getName() == null || workbasket.getName().length() == 0) {
+    if (workbasket.getKey() == null || workbasket.getKey().length() == 0) {
-      throw new InvalidWorkbasketException("Name must not be null for " + workbasket);
+      throw new InvalidWorkbasketException("Key must not be null for " + workbasket);
     }
     if (workbasket.getDomain() == null) {
       throw new InvalidWorkbasketException("Domain must not be null for " + workbasket);
     }
-    if (workbasket.getType() == null) {
-      throw new InvalidWorkbasketException("Type must not be null for " + workbasket);
-    }
     if (!taskanaEngine.domainExists(workbasket.getDomain())) {
       throw new DomainNotFoundException(
           workbasket.getDomain(),
@@ -1179,6 +1156,27 @@ public class WorkbasketServiceImpl implements WorkbasketService {
     }
   }
 
+  private void validateId(String workbasketId) throws InvalidArgumentException {
+    if (workbasketId == null) {
+      throw new InvalidArgumentException("The WorkbasketId can´t be NULL");
+    }
+    if (workbasketId.isEmpty()) {
+      throw new InvalidArgumentException("The WorkbasketId can´t be EMPTY for deleteWorkbasket()");
+    }
+  }
+
+  private void validateNameAndType(Workbasket workbasket) throws InvalidWorkbasketException {
+    if (workbasket.getName() == null) {
+      throw new InvalidWorkbasketException("Name must not be NULL for " + workbasket);
+    }
+    if (workbasket.getName().length() == 0) {
+      throw new InvalidWorkbasketException("Name must not be EMPTY for " + workbasket);
+    }
+    if (workbasket.getType() == null) {
+      throw new InvalidWorkbasketException("Type must not be NULL for " + workbasket);
+    }
+  }
+
   private List<WorkbasketPermission> getPermissionsFromWorkbasketAccessItem(
       WorkbasketAccessItem workbasketAccessItem) {
     List<WorkbasketPermission> permissions = new ArrayList<>();
@@ -1199,7 +1197,7 @@ public class WorkbasketServiceImpl implements WorkbasketService {
     taskanaEngine.getEngine().checkRoleMembership(TaskanaRole.BUSINESS_ADMIN, TaskanaRole.ADMIN);
     try {
       taskanaEngine.openConnection();
-      validateWorkbasketId(workbasketId);
+      validateId(workbasketId);
       WorkbasketImpl workbasket = workbasketMapper.findById(workbasketId);
       workbasket.setMarkedForDeletion(true);
       workbasketMapper.update(workbasket);

lib/taskana-core/src/test/java/acceptance/workbasket/UpdateWorkbasketAccTest.java

@@ -18,6 +18,7 @@ import pro.taskana.common.test.security.WithAccessId;
 import pro.taskana.workbasket.api.WorkbasketCustomField;
 import pro.taskana.workbasket.api.WorkbasketService;
 import pro.taskana.workbasket.api.WorkbasketType;
+import pro.taskana.workbasket.api.exceptions.InvalidWorkbasketException;
 import pro.taskana.workbasket.api.exceptions.WorkbasketNotFoundException;
 import pro.taskana.workbasket.api.models.Workbasket;
 import pro.taskana.workbasket.internal.models.WorkbasketImpl;
@@ -57,6 +58,35 @@ class UpdateWorkbasketAccTest extends AbstractAccTest {
     assertThat(updatedWorkbasket.getModified()).isNotEqualTo(modified);
   }
 
+  @WithAccessId(user = "businessadmin")
+  @Test
+  void should_ThrowException_When_UpdatingWorkbasketWithInvalidName() throws Exception {
+    WorkbasketService workbasketService = taskanaEngine.getWorkbasketService();
+
+    WorkbasketImpl workbasket =
+        (WorkbasketImpl) workbasketService.getWorkbasket("GPK_KSC", "DOMAIN_A");
+    workbasket.setName(null);
+    assertThatThrownBy(() -> workbasketService.updateWorkbasket(workbasket))
+        .isInstanceOf(InvalidWorkbasketException.class);
+
+    workbasket.setName("");
+    assertThatThrownBy(() -> workbasketService.updateWorkbasket(workbasket))
+        .isInstanceOf(InvalidWorkbasketException.class);
+  }
+
+  @WithAccessId(user = "businessadmin")
+  @Test
+  void should_ThrowException_When_UpdatingWorkbasketWithTypeNull() throws Exception {
+    WorkbasketService workbasketService = taskanaEngine.getWorkbasketService();
+
+    WorkbasketImpl workbasket =
+        (WorkbasketImpl) workbasketService.getWorkbasket("GPK_KSC", "DOMAIN_A");
+    workbasket.setType(null);
+
+    assertThatThrownBy(() -> workbasketService.updateWorkbasket(workbasket))
+        .isInstanceOf(InvalidWorkbasketException.class);
+  }
+
   @WithAccessId(user = "businessadmin")
   @Test
   void testUpdateWorkbasketWithConcurrentModificationShouldThrowException() throws Exception {