Closes #2364 - Add Rerouting of Tasks in JAVA-API #2373

jamesrdi · 2023-09-06T08:57:27Z

Added rerouting functionality, and fixed code smells in TaskServiceImpl and TaskQueryFilterParameter
https://sonarcloud.io/summary/new_code?id=jamesrdi_taskana&branch=TSK-2364

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

arolfes · 2023-11-17T09:20:43Z

Hello @jamesrdi , please sync fork with current master and rebase this PR.

jamesrdi · 2023-11-17T09:49:17Z

Hello @jamesrdi , please sync fork with current master and rebase this PR.

I have just rebased it

ryzheboka · 2024-01-30T10:21:43Z

...ry-provider/src/test/java/acceptance/events/task/CreateHistoryEventOnTaskRerouteAccTest.java

+class CreateHistoryEventOnTaskRerouteAccTest {
+  @TaskanaInject TaskanaEngine taskanaEngine;
+  @TaskanaInject TaskService taskService;
+  @TaskanaInject WorkbasketService workbasketService;


Please add a newline after @TaskanaInject attributes

Ok is added

ryzheboka · 2024-01-30T10:23:01Z

...ry-provider/src/test/java/acceptance/events/task/CreateHistoryEventOnTaskRerouteAccTest.java

+  Task task2;
+  Task task3;
+  Task task4;
+  private SimpleHistoryServiceImpl historyService = new SimpleHistoryServiceImpl();


Why is this private? Why do you initialize it here instead of the setup?

I agree, I have made it not private

ryzheboka · 2024-01-30T10:23:37Z

...ry-provider/src/test/java/acceptance/events/task/CreateHistoryEventOnTaskRerouteAccTest.java

+
+  @WithAccessId(user = "admin")
+  @BeforeAll
+  void setUp() throws Exception {


We use lowercase "setup" in all other tests. So please change it :)

Ok I have changed it

ryzheboka · 2024-01-30T10:24:13Z

...ry-provider/src/test/java/acceptance/events/task/CreateHistoryEventOnTaskRerouteAccTest.java

+  @WithAccessId(user = "admin")
+  @BeforeAll
+  void setUp() throws Exception {
+    taskanaHistoryEngine = TaskanaHistoryEngineImpl.createTaskanaEngine(taskanaEngine);


Why do you create it here instead of using @TaskanaInject ?

TaskanaHistoryEngineImpl is not injectible. Here is the error thrown when I try to inject it:

org.junit.platform.commons.JUnitException: Cannot inject field 'taskanaHistoryEngine'. Type 'class pro.taskana.simplehistory.impl.TaskanaHistoryEngineImpl' is not an injectable TASKANA type

ryzheboka · 2024-01-30T10:26:36Z

...ry-provider/src/test/java/acceptance/events/task/CreateHistoryEventOnTaskRerouteAccTest.java

+    List<TaskHistoryEvent> events =
+        taskHistoryQueryMapper.queryHistoryEvents(
+            (TaskHistoryQueryImpl) historyService.createTaskHistoryQuery().taskIdIn(task4.getId()));
+    assertThat(events).isEmpty();


Why do you need this assert? You already deleted all task4 related history events

Ok I agree, I have deleted the assert

ryzheboka · 2024-01-30T10:29:15Z

...ry-provider/src/test/java/acceptance/events/task/CreateHistoryEventOnTaskRerouteAccTest.java

+        taskHistoryQueryMapper.queryHistoryEvents(
+            (TaskHistoryQueryImpl)
+                historyService.createTaskHistoryQuery().taskIdIn(taskIds.toArray(new String[0])));
+    assertThat(events).isEmpty();


Why do you need this assert?

I agree, I have deleted this assert

ryzheboka · 2024-01-30T10:32:47Z

...ry-provider/src/test/java/acceptance/events/task/CreateHistoryEventOnTaskRerouteAccTest.java

+            (TaskHistoryQueryImpl)
+                historyService.createTaskHistoryQuery().taskIdIn(taskIds.toArray(new String[0])));
+
+    assertThat(events)


Could you rewrite the assertion part? It would be more readable without the if/else if/else block?

How do you suggest I rewrite it?

ryzheboka · 2024-01-30T10:33:28Z

...ry-provider/src/test/java/acceptance/events/task/CreateHistoryEventOnTaskRerouteAccTest.java

+    }
+  }
+
+  TaskHistoryQueryMapper getHistoryQueryMapper()


Why isn't this private?

I have made it private

ryzheboka · 2024-01-30T10:36:49Z

...ry-provider/src/test/java/acceptance/events/task/CreateHistoryEventOnTaskRerouteAccTest.java

Could you also test rerouteTasks(...) for its history events?

The second test should_CreateRerouteHistoryEvent_When_MultipleTasksAreRerouted() tests the history events for rerouteTasks()

ryzheboka · 2024-01-30T10:38:58Z

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

@@ -776,6 +778,78 @@ public void setCustomInt8(Integer customInt8) {
    this.customInt8 = customInt8;
  }

+  @Override
+  public Task asTask() {


Why do we need this?

It is used to convert tasksummaries obrained from the query to tasks, which will be passed to the function taskanaEngine.getTaskRoutingManager().determineWorkbasketId(task). determineWorkbasketId() only accept task as parameter, not tasksummary instances.

Casting cannot be used here, this is the error thrown when trying to cast:
Caused by: java.lang.ClassCastException: Cannot cast pro.taskana.task.internal.models.TaskSummaryImpl to pro.taskana.task.api.models.Task

jamesrdi force-pushed the TSK-2364 branch 7 times, most recently from 5c22b0c to f359273 Compare September 7, 2023 07:28

jamesrdi force-pushed the TSK-2364 branch 2 times, most recently from a267bf8 to cac955b Compare September 15, 2023 11:36

jamesrdi mentioned this pull request Sep 19, 2023

Closes #2378 - Add DeleteHistoryEvent for Task #2380

Merged

14 tasks

jamesrdi force-pushed the TSK-2364 branch 2 times, most recently from 1efe994 to 247e04f Compare November 17, 2023 09:32

ryzheboka requested changes Jan 30, 2024

View reviewed changes

jamesrdi force-pushed the TSK-2364 branch 3 times, most recently from f13a55d to 421a607 Compare January 30, 2024 14:01

Closes Taskana#2364 - Add Rerouting of Tasks in JAVA-API

421a607

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Closes #2364 - Add Rerouting of Tasks in JAVA-API #2373

Closes #2364 - Add Rerouting of Tasks in JAVA-API #2373

jamesrdi commented Sep 6, 2023 •

edited

Loading

arolfes commented Nov 17, 2023

jamesrdi commented Nov 17, 2023

ryzheboka Jan 30, 2024

jamesrdi Jan 30, 2024

ryzheboka Jan 30, 2024

jamesrdi Jan 30, 2024

ryzheboka Jan 30, 2024

jamesrdi Jan 30, 2024

ryzheboka Jan 30, 2024

jamesrdi Jan 30, 2024

ryzheboka Jan 30, 2024

jamesrdi Jan 30, 2024

ryzheboka Jan 30, 2024

jamesrdi Jan 30, 2024

ryzheboka Jan 30, 2024

jamesrdi Jan 30, 2024

ryzheboka Jan 30, 2024

jamesrdi Jan 30, 2024

ryzheboka Jan 30, 2024

jamesrdi Jan 30, 2024

ryzheboka Jan 30, 2024

jamesrdi Jan 30, 2024

Closes #2364 - Add Rerouting of Tasks in JAVA-API #2373

Are you sure you want to change the base?

Closes #2364 - Add Rerouting of Tasks in JAVA-API #2373

Conversation

jamesrdi commented Sep 6, 2023 • edited Loading

For the submitter:

Verified by the reviewer:

arolfes commented Nov 17, 2023

jamesrdi commented Nov 17, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jamesrdi commented Sep 6, 2023 •

edited

Loading