camunda-bpm-platform icon indicating copy to clipboard operation
camunda-bpm-platform copied to clipboard

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

Open ThorbenLindhauer opened this issue 4 years ago • 12 comments

This issue was imported from JIRA:

Field Value
JIRA Link CAM-12761
Reporter NYBzR2x
What is this name? This pseudonym name was generated based on the user name in JIRA to protect the personal data of our JIRA users. You can use this identifier to search for issues by the same reporter.
Has restricted visibility comments false

User story

When using ACTHICOMMENT to save user comments for tasks and process instances, I want to be able to update and delete comments.

Background

Currently, the rest API supports the creation of comments for tasks - to complete the functionality and user journey around comments, the ability to updated and delete comments via REST API is requested (also for process instances).

Acceptance criteria

  • I can use the Rest API to update and delete comments added to tasks 
  • All Comment APIs are covered by authorization checks
  • cover OpenAPI documentation

Links:

  • is caused by https://jira.camunda.com/browse/SUPPORT-9095
  • is related to https://jira.camunda.com/browse/CAMPLAN-2
### Pull requests
- [ ] https://github.com/camunda/camunda-bpm-platform/pull/4333

ThorbenLindhauer avatar Nov 13 '20 08:11 ThorbenLindhauer

Let me work on it

prajwolbhandari1 avatar Apr 03 '24 18:04 prajwolbhandari1

@ThorbenLindhauer do we need to change UI as well or just the API?

prajwolbhandari1 avatar Apr 04 '24 18:04 prajwolbhandari1

@ThorbenLindhauer @yanavasileva

I am thinking about these APIs:

DELETE - comment by ID, task ID or ProcessId

What do you guys think?

prajwolbhandari1 avatar Apr 09 '24 17:04 prajwolbhandari1

Hi @prajwolbhandari1,

It's great to see you are onto your next contribution.

do we need to change UI as well or just the API?

The scope of the ticket is only REST and Java API, no need for changes in the UI.

DELETE - comment by ID, task ID or ProcessId

That's a good start. My tip is to have a look at delete attachment (Java/REST) API, it might be similar implementation.

I see the ticket scope targets an update comment API as well. If you consider covering that in your PR as well, you will need something like TaskService#saveComment(comment) and then PUT request similar to the updateTask REST API (link).

I hope that helps to get you out of the door.

Best, Yana

yanavasileva avatar Apr 10 '24 06:04 yanavasileva

Thanks for your reply Yana.

Let me work on these two endpoints:

  1. Delete by commentID endpoint just what you have suggested above that is similar to attachment.
  2. Update a comment endpoint as you have stated above.

Do we need to have endpoints that deletes all comments associated to taskId/processInstanceId or implementing above two endpoints will work for now?

Thank you again, Yana.

prajwolbhandari1 avatar Apr 10 '24 20:04 prajwolbhandari1

Looking into further, looks like deleting comments associated by taskId/processInstanceId makes sense. Let me implement those as well.

prajwolbhandari1 avatar Apr 10 '24 21:04 prajwolbhandari1

@yanavasileva story says it needs to do this:

All Comment APIs are covered by authorization checks

Can you point me out an example where it does it checks authorization checks?

prajwolbhandari1 avatar Apr 17 '24 15:04 prajwolbhandari1

Here: https://github.com/camunda/camunda-bpm-platform/blob/master/engine/src/main/java/org/camunda/bpm/engine/impl/GetDeployedTaskFormCmd.java#L49-L51 More info: https://docs.camunda.org/manual/7.21/user-guide/process-engine/authorization-service/#permissions-by-resource I would expect for both delete and update comments

  • by task id to check for update permission on task resource
  • by process instance id to check for update permission on process instance resource

You might notice that authorization checks are not performed for the existing comment (and attachment) APIs. I think we should still add them for newly introduced APIs. Also I don't think we need to introduce a new resources (similar to what's suggested here: https://github.com/camunda/camunda-bpm-platform/issues/2090#issuecomment-1275801172)

yanavasileva avatar Apr 17 '24 16:04 yanavasileva

Here: https://github.com/camunda/camunda-bpm-platform/blob/master/engine/src/main/java/org/camunda/bpm/engine/impl/GetDeployedTaskFormCmd.java#L49-L51 More info: https://docs.camunda.org/manual/7.21/user-guide/process-engine/authorization-service/#permissions-by-resource I would expect for both delete and update comments

  • by task id to check for update permission on task resource
  • by process instance id to check for update permission on process instance resource

You might notice that authorization checks are not performed for the existing comment (and attachment) APIs. I think we should still add them for newly introduced APIs. Also I don't think we need to introduce a new resources (similar to what's suggested here: #2090 (comment)) -- You mean we do or don't need to introduce a new resource? Just wanted to make sure :)

prajwolbhandari1 avatar Apr 18 '24 00:04 prajwolbhandari1

-- You mean we do or don't need to introduce a new resource? Just wanted to make sure :)

We don't need to introduce a new resource. My bad for the wording.

yanavasileva avatar Apr 18 '24 06:04 yanavasileva

@yanavasileva opened a PR. Can you please take a look at it when you get a chance?

https://github.com/camunda/camunda-bpm-platform/pull/4333

prajwolbhandari1 avatar May 13 '24 16:05 prajwolbhandari1

@yanavasileva Do you mind taking a look at this change?

prajwolbhandari1 avatar May 17 '24 04:05 prajwolbhandari1

Requested changes from the contributor last week.

yanavasileva avatar Sep 16 '24 10:09 yanavasileva