Closes #2269 - Implement READTASKS Permission #2285

jamesrdi · 2023-06-02T11:13:54Z

https://sonarcloud.io/summary/new_code?id=jamesrdi_taskana&branch=TSK-2269

Release Notes:

For the submitter:

I updated the documentation and will supply links to the specific files
I did not update the documentation
I included a link to the SonarCloud branch analysis
After integration of the PR, I added a description of changes on the current release notes
I did not update the current release notes
I put the ticket in review
After integration of the pull request, I verified our bluemix test environment is not broken

Verified by the reviewer:

Commit message format → (Closes) #: Your commit message. i.e. Closes Commit Hooks won't work without JIRA Tickets #2271: Your commit message.
Submitter's update to documentation is sufficient
SonarCloud analysis meets our standards
Update of the current release notes reflects changes
PR fulfills the ticket
Edge cases and unwanted side effects are tested
Readability

ryzheboka

Please add tests for WorkbasketService and WorkbasketQuery.

lib/taskana-core-test/src/test/java/acceptance/task/get/GetTaskAccTest.java

lib/taskana-core-test/src/test/java/acceptance/task/query/TaskQueryImplAccTest.java

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

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

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

lib/taskana-core-test/src/test/java/acceptance/task/query/TaskQueryImplAccTest.java

ryzheboka · 2023-06-09T13:09:53Z

lib/taskana-core-test/src/test/java/acceptance/task/query/TaskQueryImplAccTest.java

+
+    @WithAccessId(user = "user-1-1")
+    @Test
+    void should_ThrowException_When_WorkbasketOfTaskHasReadTasksButNoReadPerm() {


This test doesn't throw an exception, please rename it

lib/taskana-core-test/src/test/java/acceptance/task/query/TaskQueryImplAccTest.java

...a-core/src/test/java/acceptance/workbasket/update/UpdateWorkbasketAuthorizationsAccTest.java

jamesrdi · 2023-06-13T13:26:10Z

...a-core/src/test/java/acceptance/workbasket/update/UpdateWorkbasketAuthorizationsAccTest.java

+    theAccessItem.setPermission(WorkbasketPermission.READTASKS, false);
+    workbasketService.updateWorkbasketAccessItem(theAccessItem);
+
+    List<WorkbasketAccessItem> accessItems2 = workbasketService.getWorkbasketAccessItems(wbId);


@gitgoodjhe after updating Permission Workbasket Access Item, should the Access Item be queried again to check if Permission is updated? In other tests, access items are queried again, which actually is not needed.

holgerhagen · 2023-06-15T08:41:58Z

lib/taskana-core-test/src/test/java/acceptance/task/query/TaskQueryImplAccTest.java

+
+    @WithAccessId(user = "user-1-1")
+    @Test
+    void should_QueryByTaskId_When_WorkbasketHasReadAndReadTasksButNoOpenPerm() {


Maybe another test with a query for task7 and task8 makes sense, making sure that only task8 is returned?

holgerhagen · 2023-06-15T08:50:53Z

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

@@ -117,7 +117,8 @@ public static String queryTaskSummariesDb2() {
        + "s.ACCESS_ID IN "
        + "(<foreach item='item' collection='accessIdIn' separator=',' >#{item}</foreach>) "
        + "and "
-        + "s.WORKBASKET_ID = X.WORKBASKET_ID AND s.perm_read = 1 fetch first 1 rows only"
+        + "s.WORKBASKET_ID = X.WORKBASKET_ID AND s.perm_read = 1 AND s.perm_readtasks = 1"


For me it would have been sufficient to only check the readtasks permission, to keep the complexity small. But in this case it does not increase the complexity too much, or what do you think?

holgerhagen · 2023-06-15T08:53:24Z

...skana-core/src/test/java/acceptance/workbasket/query/QueryWorkbasketByPermissionAccTest.java

@@ -97,6 +97,20 @@ void should_GetAllTransferTargetsForUserAndGroup_When_QueryingForMultiplePermiss
    assertThat(results).hasSize(4);
  }

+  @WithAccessId(user = "businessadmin")
+  @Test
+  void should_GetAllTransferTargetsForUserAndGroup_When_QueryingForReadTasksPermissions()


The name is wrong, isn't it? Why "TransferTargets"?

holgerhagen · 2023-06-15T08:53:58Z

...skana-core/src/test/java/acceptance/workbasket/query/QueryWorkbasketByPermissionAccTest.java

@@ -160,6 +174,18 @@ void should_GetAllTransferTargetsForSubjectUser_When_QueryingForSinglePermission
    assertThat(results).hasSize(1);
  }

+  @WithAccessId(user = "user-1-1")
+  @Test
+  void should_GetAllTransferTargetsForSubjectUser_When_QueryingForReadTasksPermission() {


gitgoodjhe · 2023-06-15T11:55:38Z

Great work, well done! @jamesrdi

jamesrdi force-pushed the TSK-2269 branch 2 times, most recently from ddf27ea to d815270 Compare June 6, 2023 09:17

ryzheboka force-pushed the new-permissions branch from 7e218fb to 696d2de Compare June 6, 2023 09:24

jamesrdi force-pushed the TSK-2269 branch 3 times, most recently from 4421a22 to 57e3441 Compare June 6, 2023 11:03

ryzheboka requested changes Jun 6, 2023

View reviewed changes

jamesrdi force-pushed the TSK-2269 branch 2 times, most recently from f40cfef to f1bf816 Compare June 9, 2023 08:05