feat(core) Add update and delete command Java/Rest APIs #4333

prajwolbhandari1 · 2024-05-13T16:03:11Z

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

…Instance 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: camunda#2551

…nd 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: camunda#2551

…k and 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: camunda#2551

yanavasileva · 2024-05-17T06:14:27Z

Hi @prajwolbhandari1,

Thank you for raising this.
We will have a look and get back to you. As the changes are a lot it might take some time to cover everything.

Best,
Yana

yanavasileva · 2024-05-17T08:22:44Z

Note to myself:

docs for user operation logs and authorizations

prajwol123 · 2024-06-12T12:49:16Z

@yanavasileva any update on this?

yanavasileva · 2024-06-13T14:20:01Z

Thank you for your patience with this.
I didn't have the time to start with the review yet. I will get to it next week.

yanavasileva

👍 The changes look good as a start.
Unfortunately, more aspects need to be covered, I left comments below. One more general note:
❌ There's a lot of code duplication in the code of the commands. Please merge the commands (for PI and task) similarly to DeleteAttachmentCmd. That way, the maintenance of the code will be easier.

yanavasileva · 2024-06-14T07:33:17Z

...ne/src/main/java/org/camunda/bpm/engine/impl/persistence/entity/UserOperationLogManager.java

@@ -493,6 +493,34 @@ public void logAttachmentOperation(String operation, ExecutionEntity processInst
    }
  }

+  public void logCommentOperation(String operation, TaskEntity task, PropertyChange propertyChange) {


Note to myself:
❓ What about tenant checks?

yanavasileva · 2024-06-14T07:36:23Z

engine/src/main/java/org/camunda/bpm/engine/history/UserOperationLogEntry.java

✍️ The new user operation logs must be documented here: https://docs.camunda.org/manual/latest/user-guide/process-engine/history/user-operation-log/#glossary-of-operations-logged-in-the-user-operation-log
Could you please raise a PR in the docs repo? (page)

yanavasileva · 2024-06-14T08:06:00Z

engine/src/main/java/org/camunda/bpm/engine/impl/persistence/entity/CommentEntity.java

❌ ✍️ Along with the delete API, we need to add a revision field to the entity. Documented here:
https://github.com/camunda/camunda-bpm-platform/blob/master/engine/src/main/java/org/camunda/bpm/engine/impl/persistence/entity/CommentEntity.java#L44
That will require changes in the ACT_HI_COMMENT table and mybatis scripts.

yanavasileva · 2024-06-14T08:12:30Z

engine/src/main/java/org/camunda/bpm/engine/TaskService.java

🔧 Consider adding @throws AuthorizationException note to the javadoc where applicable for the new APIs.

yanavasileva · 2024-06-14T08:20:51Z

engine/src/main/java/org/camunda/bpm/engine/impl/cfg/CommandChecker.java

+  /**
+   * Check if it is allowed to update a task
+   */
+  void checkUpdateTask(TaskEntity task);
+


❌ After reading the docs, I think ~~#checkTaskAssign()~~ #checkTaskWork() check should be used to perform the command checks for the new commands.
Reference: https://docs.camunda.org/manual/latest/user-guide/process-engine/authorization-service/#additional-task-permissions (✍️ the table should be updated with the actions as well)

Suggested change

/**

* Check if it is allowed to update a task

*/

void checkUpdateTask(TaskEntity task);

That makes sense for actions for comments using taskId.

How about update/delete comments using processInstanceId ?

Can we use this?

camunda-bpm-platform/engine/src/main/java/org/camunda/bpm/engine/impl/cfg/auth/AuthorizationCommandChecker.java

Line 256 in c307a42

public void checkUpdateProcessInstanceById(String processInstanceId) {

That makes sense for actions for comments using taskId.

How about update/delete comments using processInstanceId ?

Can we use this?

camunda-bpm-platform/engine/src/main/java/org/camunda/bpm/engine/impl/cfg/auth/AuthorizationCommandChecker.java

Line 256 in c307a42

public void checkUpdateProcessInstanceById(String processInstanceId) {

Digging more into it, for me using checkTaskAssign makes more sense on this on as well. Let me know if you think otherwise.

@prajwolbhandari1, by bad for the wrong reference. #checkTaskWork() should be used, it covers both API using task and process checks.
https://github.com/camunda/camunda-bpm-platform/blob/master/engine/src/main/java/org/camunda/bpm/engine/impl/cfg/auth/AuthorizationCommandChecker.java#L676C15-L676C28

@prajwolbhandari1, by bad for the wrong reference. #checkTaskWork() should be used, it covers both API using task and process checks. https://github.com/camunda/camunda-bpm-platform/blob/master/engine/src/main/java/org/camunda/bpm/engine/impl/cfg/auth/AuthorizationCommandChecker.java#L676C15-L676C28

No worries Yana. I will make these changes. Thank you.

yanavasileva · 2024-06-14T08:21:23Z

engine/src/main/java/org/camunda/bpm/engine/impl/cfg/auth/AuthorizationCommandChecker.java

+  public void checkUpdateTask(TaskEntity task) {
+    getAuthorizationManager().checkAuthorization(UPDATE, TASK, task.getId());
+  }
+


❌ See above comment, reuse #checkTaskAssign().

Suggested change

public void checkUpdateTask(TaskEntity task) {

getAuthorizationManager().checkAuthorization(UPDATE, TASK, task.getId());

}

yanavasileva · 2024-06-14T08:21:30Z

engine/src/main/java/org/camunda/bpm/engine/impl/cfg/multitenancy/TenantCommandChecker.java

+  public void checkUpdateTask(TaskEntity task) {
+    if (task != null && !getTenantManager().isAuthenticatedTenant(task.getTenantId())) {
+      throw LOG.exceptionCommandWithUnauthorizedTenant("update the task '" + task.getId() + "'");
+    }
+  }
+


❌ See above comment, reuse #checkTaskAssign().

Suggested change

public void checkUpdateTask(TaskEntity task) {

if (task != null && !getTenantManager().isAuthenticatedTenant(task.getTenantId())) {

throw LOG.exceptionCommandWithUnauthorizedTenant("update the task '" + task.getId() + "'");

}

}

yanavasileva · 2024-06-14T08:23:24Z

engine/src/main/java/org/camunda/bpm/engine/impl/cmd/DeleteProcessInstanceCommentCmd.java

+import org.camunda.bpm.engine.impl.persistence.entity.TaskEntity;
+
+/**
+ * see https://github.com/camunda/camunda-bpm-platform/issues/2551


❌ Please remove all of the git ticket references from the code. The modern IDE and GitHub offer enough support to track the origins to ticket and pull request.

Suggested change

* see https://github.com/camunda/camunda-bpm-platform/issues/2551

yanavasileva · 2024-06-14T08:28:28Z

engine/src/main/resources/org/camunda/bpm/engine/impl/mapping/entity/Comment.xml

    where TASK_ID_ = #{taskId,jdbcType=VARCHAR}
      and ID_ =  #{id,jdbcType=VARCHAR}
  </select>

+  <select id="selectCommentByProcessInstanceIdAndCommentId" parameterType="map" resultMap="commentResultMap">


💭 I see a different result map for above query. Maybe we need to consider that for this query as well? The type of FULL_MSG_ is different for postgresql.

prajwolbhandari1 · 2024-06-19T20:11:00Z

thank you @yanavasileva. I will work on these changes as soon as possible.

github-actions · 2024-07-12T04:05:13Z

Closing due to lack of requested feedback. If you would like us to look at this, please provide the requested information to re-open the PR.

prajwol123 · 2024-07-12T05:46:44Z

I am done with all changes. I will raise PR as soon as possible. Sent from my iPhoneOn Jul 11, 2024, at 10:05 PM, github-actions[bot] ***@***.***> wrote: Closed #4333. —Reply to this email directly, view it on GitHub, or unsubscribe.You are receiving this because you commented.Message ID: ***@***.***>

yanavasileva · 2024-07-12T06:29:20Z

@prajwolbhandari1, you can continue the work in this one, I reopened it for you.

prajwolbhandari1 added 4 commits May 2, 2024 08:46

chore(engine) added git issue number in newly created files.

c307a42

prajwolbhandari1 mentioned this pull request May 13, 2024

Java and Rest API supports update and delete of task and process instance comments #2551

Open

yanavasileva assigned prajwolbhandari1 May 17, 2024

yanavasileva self-requested a review May 17, 2024 07:37

yanavasileva requested changes Jun 14, 2024

View reviewed changes

yanavasileva added the group:requested-info DRI: Yana label Jun 14, 2024

github-actions bot added the group:stale DRI: Yana label Jun 15, 2024

github-actions bot added group:stale DRI: Yana and removed group:stale DRI: Yana labels Jun 20, 2024

github-actions bot removed the group:stale DRI: Yana label Jun 27, 2024

yanavasileva changed the title ~~feat(engine, engine-rest-core, engine-rest-openapi) Add update and delete Java/Rest APIs of task and processInstance~~ feat(core) Add update and delete command Java/Rest APIs Jun 27, 2024

github-actions bot added group:stale DRI: Yana and removed group:stale DRI: Yana labels Jun 28, 2024

github-actions bot added the group:closed-as-inactive DRI: Yana label Jul 12, 2024

github-actions bot closed this Jul 12, 2024

yanavasileva reopened this Jul 12, 2024

yanavasileva removed the group:requested-info DRI: Yana label Jul 12, 2024

yanavasileva removed group:closed-as-inactive DRI: Yana group:stale DRI: Yana labels Jul 12, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(core) Add update and delete command Java/Rest APIs #4333

feat(core) Add update and delete command Java/Rest APIs #4333

prajwolbhandari1 commented May 13, 2024

yanavasileva commented May 17, 2024

yanavasileva commented May 17, 2024

prajwol123 commented Jun 12, 2024

yanavasileva commented Jun 13, 2024

yanavasileva left a comment

yanavasileva Jun 14, 2024

yanavasileva Jun 14, 2024

yanavasileva Jun 14, 2024

yanavasileva Jun 14, 2024

yanavasileva Jun 14, 2024 •

edited

Loading

prajwolbhandari1 Jun 24, 2024

prajwolbhandari1 Jun 26, 2024

yanavasileva Jun 27, 2024 •

edited

Loading

prajwolbhandari1 Jul 2, 2024

yanavasileva Jun 14, 2024

yanavasileva Jun 14, 2024

yanavasileva Jun 14, 2024

yanavasileva Jun 14, 2024

prajwolbhandari1 commented Jun 19, 2024

github-actions bot commented Jul 12, 2024

prajwol123 commented Jul 12, 2024 via email

yanavasileva commented Jul 12, 2024

	public void checkUpdateTask(TaskEntity task) {
	getAuthorizationManager().checkAuthorization(UPDATE, TASK, task.getId());
	}

feat(core) Add update and delete command Java/Rest APIs #4333

Are you sure you want to change the base?

feat(core) Add update and delete command Java/Rest APIs #4333

Conversation

prajwolbhandari1 commented May 13, 2024

yanavasileva commented May 17, 2024

yanavasileva commented May 17, 2024

prajwol123 commented Jun 12, 2024

yanavasileva commented Jun 13, 2024

yanavasileva left a 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

yanavasileva Jun 14, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yanavasileva Jun 27, 2024 • edited Loading

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

prajwolbhandari1 commented Jun 19, 2024

github-actions bot commented Jul 12, 2024

prajwol123 commented Jul 12, 2024 via email

yanavasileva commented Jul 12, 2024

yanavasileva Jun 14, 2024 •

edited

Loading

yanavasileva Jun 27, 2024 •

edited

Loading