Skip to content
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.

Sign up for GitHub

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

Jump to bottom

feat(core) Add update and delete comment Java/Rest APIs #4565

Merged
merged 16 commits into from
Nov 29, 2024
Merged

feat(core) Add update and delete comment Java/Rest APIs #4565

merged 16 commits into from
Nov 29, 2024

Conversation

yanavasileva
Copy link
Member

@yanavasileva yanavasileva commented Aug 28, 2024 •
edited
Loading

related to: #2551

@yanavasileva yanavasileva added the ci:no-build Prevents any CI stage from running. label Aug 28, 2024
@yanavasileva yanavasileva self-assigned this Aug 28, 2024
@yanavasileva yanavasileva added ci:default-build Runs the builds that have no explicit trigger (e.g. different history levels). ci:migration Runs the process instance migration builds. ci:rest-api Runs extra builds for the REST API (currently only WLS compatibility builds). ci:rolling-update Runs the rolling update builds. and removed ci:no-build Prevents any CI stage from running. labels Aug 29, 2024
@yanavasileva yanavasileva mentioned this pull request Aug 30, 2024
Closed
@yanavasileva yanavasileva added the ci:all-db Runs the builds for all databases. label Sep 20, 2024
@prajwolbhandari1 @yanavasileva
feat(engine) Add update and delete Java/Rest APIs of task and
b07e214 
processInstance

DELETE /task/{taskId}/comment/{commentId}
  - deletes a comment of a given taskId and commentId
DELETE /task/{taskId}/comment
  - deletes all comments of a given taskId
PUT /task/comment
  - updates a comment
DELETE /process-instance/{processInstanceId}/comment/{commentId}
 - deletes a comment of a given processInstanceId and commentId
DELETE /process-instance/{processInstanceId}/comment.
 - deletes all comments of a given processInstanceId
PUT /process-instance/comment
 - updates a comment

related to: #2551

feat(engine-rest-core) Add update and delete Java/Rest APIs of task and
processInstance

feat(engine-rest-openapi) Add update and delete Java/Rest APIs of task
and processInstance

feat(engine)
- add revision to comments
- change authorization to task_assign from task_update
- refactor code
- add more java docs

feat(engine) update task_assign authorization to task_work for
delete/update comments

Feedback Adjustments
- fix verbiage in ftl files
- add standalone tests for delete comment(s) by process instance id and
task id
- fix/rewrite sql statement
- write additional process instance tests
- few minor changes

Fix engine-rest tests
@yanavasileva yanavasileva added ci:all-as Runs the builds for all application servers. ci:webapp-integration Runs the webapp integration builds. labels Nov 15, 2024
@yanavasileva yanavasileva force-pushed the fidelity-contributions-finalRepoCommentsApis branch from 0878e62 to b07e214 Compare November 15, 2024 08:47
@yanavasileva yanavasileva added ci:h2 Runs the builds for the H2 database. ci:e2e Runs the frontend end-to-end tests. ci:no-build Prevents any CI stage from running. labels Nov 16, 2024
@yanavasileva yanavasileva removed the ci:no-build Prevents any CI stage from running. label Nov 18, 2024
@yanavasileva yanavasileva marked this pull request as ready for review November 18, 2024 08:38
@yanavasileva
Copy link
Member Author

To reviewer:
I have reviewed the first commit. Please check only the commits that I am a single author.

@yanavasileva yanavasileva changed the title Fidelity contributions final repo comments apis feat(core) Add update and delete command Java/Rest APIs Nov 18, 2024
@yanavasileva yanavasileva mentioned this pull request Nov 18, 2024
Closed
yanavasileva
yanavasileva commented Nov 18, 2024
View reviewed changes
engine/src/main/resources/org/camunda/bpm/engine/db/create/activiti.db2.create.history.sql
@@ -187,6 +187,7 @@ create table ACT_HI_COMMENT (
FULL_MSG_ BLOB,
TENANT_ID_ varchar(64),
REMOVAL_TIME_ timestamp,
REV_ integer not null default 1,
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why? - https://confluence.camunda.com/display/AP/Maintaining+Backwards+Compatibility#:~:text=Add%20new%20column%20(not%20null%20but%20with%20default%20value)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's not that we could not add REV_ without default value because it would be nullable and null by default, which is backward compatible. But nullable does not make sense for REV_ because we want all Comment entities to have a revision index.

@yanavasileva, did I capture your intention correctly?

Copy link
Member

@mboskamp mboskamp Nov 18, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Several other REV_ columns in the same file are nullable, though, and don't have a default value.
❓ Why is this change necessary?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sharing discussed for transparency.

-- According to the confluence page the column either needs to be nullable or not null with a default value.
As we need to add default value either way, I decided to make the column not nullable.

-- do you know if we ever had this situation in the past?

-- Yes, 7.1 to 7.2 and there three queries there doing the same.

mboskamp
mboskamp reviewed Nov 18, 2024
View reviewed changes
Copy link
Member

@mboskamp mboskamp left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 Looks great in general.
❓ I am not sure if I understood why the REV_ column needs to be non-nullable and have a default value. Several other REV_ columns in other tables are nullable. Details in this thread.

engine-rest/engine-rest-openapi/src/main/templates/paths/process-instance/{id}/comment/put.ftl Outdated Show resolved Hide resolved
...e-rest-openapi/src/main/templates/paths/process-instance/{id}/comment/{commentId}/delete.ftl Outdated Show resolved Hide resolved
@yanavasileva
Copy link
Member Author

I will ignore the small suggestions for now so I don't trigger the CI again.

mboskamp
mboskamp approved these changes Nov 22, 2024
View reviewed changes
Copy link
Member

@mboskamp mboskamp left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

@yanavasileva
Copy link
Member Author

I am postponing the merge for now as I was not able to test the user operations yet.

@yanavasileva yanavasileva added ci:no-build Prevents any CI stage from running. and removed ci:h2 Runs the builds for the H2 database. ci:all-as Runs the builds for all application servers. ci:all-db Runs the builds for all databases. ci:migration Runs the process instance migration builds. ci:rolling-update Runs the rolling update builds. ci:webapp-integration Runs the webapp integration builds. labels Nov 29, 2024
@yanavasileva @mboskamp
Apply suggestions from code review
213260b 
Co-authored-by: Miklas Boskamp <20189772+mboskamp@users.noreply.github.com>
@yanavasileva yanavasileva removed the ci:no-build Prevents any CI stage from running. label Nov 29, 2024
@yanavasileva
Copy link
Member Author

@mboskamp, I made a small adjustment, can you review it: b00569b

mboskamp
mboskamp approved these changes Nov 29, 2024
View reviewed changes
Copy link
Member

@mboskamp mboskamp left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

@yanavasileva yanavasileva merged commit 536442d into master Nov 29, 2024
5 of 6 checks passed
@yanavasileva yanavasileva deleted the fidelity-contributions-finalRepoCommentsApis branch November 29, 2024 14:52
@yanavasileva yanavasileva changed the title feat(core) Add update and delete command Java/Rest APIs feat(core) Add update and delete comment Java/Rest APIs Nov 29, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mboskamp mboskamp mboskamp approved these changes

Assignees

@yanavasileva yanavasileva

Labels
ci:default-build Runs the builds that have no explicit trigger (e.g. different history levels). ci:e2e Runs the frontend end-to-end tests. ci:rest-api Runs extra builds for the REST API (currently only WLS compatibility builds).
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@yanavasileva @mboskamp @prajwolbhandari1