-
Notifications
You must be signed in to change notification settings - Fork 1.6k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Git issue 4046 : Persist taskState/lifeCycleState & return in get task and get history task api #4218
Git issue 4046 : Persist taskState/lifeCycleState & return in get task and get history task api #4218
Conversation
Signed-off-by: Sahu, Jyoti <Jyoti.Sahu@fmr.com>
…-enhancement Signed-off-by: Sahu, Jyoti <Jyoti.Sahu@fmr.com>
Hi @jyotisahu9 , Thank you for raising this. Best, |
Hi @yanavasileva , Did you get a chance to review the code changes. Thanks, |
Hi @jyotisahu9, I will need more time to review the PR as the change is not trivial. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍 Overall you covered a lot of the changes in the first run, that's great! Nice work.
🔧 Could you please remove the GIT Issue
line from the comments and the javadoc. With the modern IDE and github features, tracking the commit, the PR, and the issue is easier so I think it's redundant.
I left comments for what's missing or can be improved. Please have a look.
Thank you for your patience for the review.
engine/src/main/resources/org/camunda/bpm/engine/db/upgrade/cockroachdb_engine_7.20_to_7.21.sql
Outdated
Show resolved
Hide resolved
...sources/org/camunda/bpm/engine/db/liquibase/baseline/liquibase.cockroachdb.create.engine.sql
Outdated
Show resolved
Hide resolved
public static final String TASK_STATE_INIT = "Init"; | ||
public static final String TASK_STATE_CREATED = "Created"; | ||
public static final String TASK_STATE_COMPLETED = "Completed"; | ||
public static final String TASK_STATE_DELETED = "Deleted"; | ||
public static final String TASK_STATE_UPDATED = "Updated"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
❌ Let's rename the existing TaskState
enum to LifecycleState
to be more explicit what is it about and create a new TaskState enum to contain those states.
❓ Is it possible to extend and reuse the existing TaskState
enum? Will require avoiding handling of the new "Updated" state.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have updated the logic to use existing taskState enum, additionally did some changes in enum itself to retain required taskstate text in DB. I have removed extra code related to previous implemented taskState like constants and new method.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍 It's great that you managed to reuse the lifecycle state.
It's a bit pity that we can't simply extend it and persist it for your purposes. 🤔 Maybe you can give it another thought if that's possible without breaking the existing functionality. As the only difference is the updated state between lifecycle and newly introduced task state, right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I tried to persist lifecycle state instead of taskState but then it saves values like STATE_CREATED, STATE_COMPLETED etc. in db. I can't change it existing value as its been used extensively in current logic, so I am trying to persist taskState but its using existing enum.
engine/src/test/java/org/camunda/bpm/engine/test/history/HistoricTaskInstanceTest.java
Show resolved
Hide resolved
engine/src/main/java/org/camunda/bpm/engine/impl/persistence/entity/TaskEntity.java
Outdated
Show resolved
Hide resolved
engine-rest/engine-rest-openapi/src/main/templates/paths/task/{id}/get.ftl
Outdated
Show resolved
Hide resolved
engine/src/main/java/org/camunda/bpm/engine/impl/persistence/entity/TaskEntity.java
Show resolved
Hide resolved
.../test/java/org/camunda/bpm/engine/rest/history/HistoricTaskInstanceRestServiceQueryTest.java
Show resolved
Hide resolved
Thanks @yanavasileva for your review comments. I am going through them and would do the needful. |
* set false as default value for initializeTelemetry and switch to primitive * engine tests: test default value * LoginIT: remove telemetry close modal * initializeTelemetryProperty: add null check related to camunda#4167
…amunda#4230) camunda/team-automation-platform#126
…a#4228) in installation path camunda#4227
Related to camunda#3848 Co-authored-by: Daniel Kelemen <danielkelemen@users.noreply.github.com>
related to camunda#4239
2d38833
to
c7f927c
Compare
Hi @yanavasileva, Latest code changes for review comments have been pushed. I have resolved few of the review conversation, for few I have added my comments and it can be closed once you are good with them. Please review the code whenever you get a chance and let me know in case of any issues. Thanks, |
@jyotisahu9, thank you for considering my input. I will have a look at made changes and get back to you next week. |
👍 Overall the changes look good, I want to have a look again at the tests, then I will merge the PR. |
...n/resources/org/camunda/bpm/engine/db/liquibase/baseline/liquibase.mariadb.create.engine.sql
Outdated
Show resolved
Hide resolved
engine/src/main/resources/org/camunda/bpm/engine/db/upgrade/oracle_engine_7.21_to_7.22.sql
Outdated
Show resolved
Hide resolved
engine/src/main/resources/org/camunda/bpm/engine/db/upgrade/mssql_engine_7.21_to_7.22.sql
Outdated
Show resolved
Hide resolved
Hi @yanavasileva, Sql script corrections are banked. Request you to review them and merge the PR. Thanks, |
engine/src/test/java/org/camunda/bpm/engine/test/history/HistoricTaskInstanceTest.java
Outdated
Show resolved
Hide resolved
engine/src/test/java/org/camunda/bpm/engine/test/history/HistoricTaskInstanceTest.java
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The tests need to be modified.
Please note, that I am not able to commit to your branch/fork, so I can't do the changes on my own.
Hi @yanavasileva , I went through the review comment. Do you want me to just remove the assertion from HistoricTaskInstanceTest file ? Thanks, |
Hi Jyoti,
No. Beside the suggested removal of the assertion, I expect to add similar test assertion for runtime tasks where the state is created and updated. I suggest to add the assertions to Best, |
Hi @yanavasileva , Pushed the test case review comments fixes. Thanks |
Are the changes good to be merged? Thanks, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@jyotisahu9, the changes look good. I am executing some tests at the moment to verify there are no regressions then I will proceed with the merge.
I want to bring a detail to your attention and write it down for future reference. Lines 360 to 366 in e543c70
So once a task is completed the historic task entity will look like as follows: {
"id": "8c6c12d3-288c-11ef-9ca5-00ff18b69f72",
"processDefinitionKey": "invoice",
"processDefinitionId": "invoice:2:8a96b6d5-288c-11ef-9ca5-00ff18b69f72",
"processInstanceId": "8c684224-288c-11ef-9ca5-00ff18b69f72",
"executionId": "8c684224-288c-11ef-9ca5-00ff18b69f72",
"caseDefinitionKey": null,
"caseDefinitionId": null,
"caseInstanceId": null,
"caseExecutionId": null,
"activityInstanceId": "approveInvoice:8c6c12d2-288c-11ef-9ca5-00ff18b69f72",
"name": "Approve Invoice",
"description": "Approve the invoice (or not).",
"deleteReason": "completed",
"owner": null,
"assignee": null,
"startTime": "2024-06-07T09:22:38.792+0200",
"endTime": "2024-06-12T09:22:38.792+0200",
"duration": 432000000,
"taskDefinitionKey": "approveInvoice",
"priority": 50,
"due": "2024-06-14T09:22:38.792+0200",
"parentTaskId": null,
"followUp": null,
"tenantId": null,
"removalTime": null,
"rootProcessInstanceId": "8c684224-288c-11ef-9ca5-00ff18b69f72",
"taskState": "Completed"
} "Deleted" state will be assigned only if a task delete is executed. I don't suggest any changes on this, I just wanted to highlight the behaviour in case it's important for you. |
Thanks @yanavasileva for merging the pull request. |
Git issue : #4046
Camunda forum issue : https://forum.camunda.io/t/task-state-in-camunda-7-vs-camunda-8/48746/3
Persist task state & update Get task & Get Tasks (Historic) api's to return task state.