Task api enhancement to include attachment and comments fields. #4306

sumankumarpani · 2024-04-29T05:42:29Z

Feat(engine) : User task api provide comment and attachment properties

Enhance the get tasks api to include "attachments" and "comments" boolean field which return existence of associated attachment and comments with the task.
Include boolean filter 'withCommentAttachmentInfo' and evaluate existence of attachment and comments when true.
Enhance Swagger specs

related to GIT Issue : #2404

…related boolean fields.

yanavasileva · 2024-05-13T06:23:50Z

Hi @sumankumarpani,

Thank you for raising this.
We will have a look and get back to you.

Best,
Yana

sumankumarpani · 2024-05-16T04:56:30Z

Thanks @yanavasileva , let me know what are your thoughts/ suggestions.

yanavasileva

@sumankumarpani, I haven't finish completely the review yet but I want to share early feedback and store my notes.
👍 Overall expression is that your approach is good, I just want to have a second look.

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

engine/src/main/java/org/camunda/bpm/engine/impl/TaskQueryImpl.java

engine/src/main/java/org/camunda/bpm/engine/task/Task.java

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

engine/src/main/java/org/camunda/bpm/engine/task/TaskQuery.java

engine-rest/engine-rest/src/main/java/org/camunda/bpm/engine/rest/dto/task/TaskDto.java

…nhancements' into feature/ENTDAPCOE-126-task-api-enhancements

…/github.com/fidelity-contributions/camunda-camunda-bpm-platform into feature/ENTDAPCOE-126-task-api-enhancements

CLAassistant · 2024-05-24T19:48:37Z

Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you all sign our Contributor License Agreement before we can accept your contribution.
1 out of 2 committers have signed the CLA.

✅ sumankumarpani
❌ brianwarner
_{You have signed the CLA already but the status is still pending? Let us recheck it.}

yanavasileva

Thank you for considering my feedback so far, I managed to complete the review now.
Please see the comments below.

yanavasileva · 2024-05-28T07:39:14Z

engine-rest/engine-rest/src/main/java/org/camunda/bpm/engine/rest/dto/task/TaskDto.java

@@ -49,7 +50,9 @@ public class TaskDto {
  private String formKey;
  private CamundaFormRef camundaFormRef;
  private String tenantId;
-
+  @JsonProperty("attachment")


❌ For consistency, let's remove the annotation:

Suggested change

@JsonProperty("attachment")

I was reluctant to add this, I am facing an weird issue where hasAttachment is translated to attachments instead of attachment in the json response, hence decided to add. Let me know your thoughts.

I saw this. I will try to see from where it's coming.

You can also try to debug it.

I figured it , jackson considers the setter method to decide the boolean var name.

yanavasileva · 2024-05-28T07:52:44Z

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

+    this.attachmentExists = !getProcessEngine().getTaskService().getTaskAttachments(id).isEmpty();
+    this.commentExists = !getProcessEngine().getTaskService().getTaskComments(id).isEmpty();


❌ Please re-use the command context instead of executing command (get attachment/comments) in the command (task query):

Suggested change

this.attachmentExists = !getProcessEngine().getTaskService().getTaskAttachments(id).isEmpty();

this.commentExists = !getProcessEngine().getTaskService().getTaskComments(id).isEmpty();

this.attachmentExists = !Context.getCommandContext().getAttachmentManager().findAttachmentsByTaskId(id).isEmpty();

this.commentExists = !Context.getCommandContext().getCommentManager().findCommentsByTaskId(id).isEmpty();

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

yanavasileva · 2024-05-28T07:58:10Z

engine/src/main/java/org/camunda/bpm/engine/task/TaskQuery.java

+  /**
+   * Evaluates existence of attachment and comments associated with the task, defaults to false.
+   */


❌ Please add performance considerations to the javadoc. Adding the filter will do additional attachment and comments queries to the database, that might slow down the query in case of big size of the tables.

yanavasileva · 2024-05-28T07:59:24Z

engine/src/test/java/org/camunda/bpm/engine/test/api/task/TaskQueryTest.java

+  @Test
+  @Deployment(resources = "org/camunda/bpm/engine/test/api/oneTaskProcess.bpmn20.xml")


❌ Add required history level (don't forget to add the imports):

Suggested change

@Test

@Deployment(resources = "org/camunda/bpm/engine/test/api/oneTaskProcess.bpmn20.xml")

@Test

@RequiredHistoryLevel(ProcessEngineConfiguration.HISTORY_ACTIVITY)

@Deployment(resources = "org/camunda/bpm/engine/test/api/oneTaskProcess.bpmn20.xml")

yanavasileva · 2024-05-28T09:44:20Z

engine/src/main/java/org/camunda/bpm/engine/impl/TaskQueryImpl.java

@@ -1441,6 +1449,13 @@ public List<Task> executeList(CommandContext commandContext, Page page) {
      }
    }

+    if (withCommentAttachmentInfo && Context.getProcessEngineConfiguration().getHistoryLevel()!= HistoryLevel.HISTORY_LEVEL_NONE) {


❌ Use equals to compare:

Suggested change

if (withCommentAttachmentInfo && Context.getProcessEngineConfiguration().getHistoryLevel()!= HistoryLevel.HISTORY_LEVEL_NONE) {

if (withCommentAttachmentInfo && !Context.getProcessEngineConfiguration().getHistoryLevel().equals(HistoryLevel.HISTORY_LEVEL_NONE)) {

yanavasileva · 2024-05-28T09:47:18Z

engine/src/test/java/org/camunda/bpm/engine/test/api/task/TaskQueryTest.java

+import static org.junit.Assert.*;
+import static org.junit.Assert.assertFalse;


🔧 Following code styles guidelines:

Suggested change

import static org.junit.Assert.*;

import static org.junit.Assert.assertFalse;

import static org.junit.Assert.assertEquals;

import static org.junit.Assert.assertFalse;

import static org.junit.Assert.assertNotNull;

import static org.junit.Assert.assertNull;

import static org.junit.Assert.assertTrue;

import static org.junit.Assert.fail;

yanavasileva · 2024-05-28T09:48:27Z

engine/src/test/java/org/camunda/bpm/engine/test/api/task/TaskQueryTest.java

+  }
+  @Test
+  @Deployment(resources = "org/camunda/bpm/engine/test/api/oneTaskProcess.bpmn20.xml")


❌ Add required history level:

Suggested change

}

@Test

@Deployment(resources = "org/camunda/bpm/engine/test/api/oneTaskProcess.bpmn20.xml")

}

@Test

@RequiredHistoryLevel(ProcessEngineConfiguration.HISTORY_ACTIVITY)

@Deployment(resources = "org/camunda/bpm/engine/test/api/oneTaskProcess.bpmn20.xml")

yanavasileva · 2024-05-28T10:12:45Z

...t-openapi/src/main/templates/models/org/camunda/bpm/engine/rest/dto/runtime/task/TaskDto.ftl

❌ I don't see an action on this comment (in OpenAPI, REST API, Java API):
#2404 (comment)
One option is the described here:
#2404 (comment)
Other option will be to add comment to the APIs that can be true only if withCommentAttachmentInfo filter is passed. That might create confusion in users.

@yanavasileva , I am exploring option 1 of extending the dto class and adding additional variables.

I got a doubt @yanavasileva , our changes should be applicable to GET & POST /task apis only ,and not /task/
{id} , let me know if my understanding is correct?

yanavasileva · 2024-05-28T10:14:03Z

...t-openapi/src/main/templates/models/org/camunda/bpm/engine/rest/dto/runtime/task/TaskDto.ftl

❌ In all OpenAPI files, hasAttachment/hasComment changes are not reflected.

Task api enhancements to include attachment and comments association …

d744faa

…related boolean fields.

sumankumarpani mentioned this pull request Apr 29, 2024

Get user task query provides comment and attachment properties #2404

Open

danielkelemen requested a review from yanavasileva May 3, 2024 12:48

danielkelemen assigned sumankumarpani May 3, 2024

comment qa

e9bd295

yanavasileva reviewed May 22, 2024

View reviewed changes

sumankumarpani and others added 6 commits May 24, 2024 15:14

addressed review comments

7f9ba41

addressed review comments

998f8a2

Merge remote-tracking branch 'origin/feature/ENTDAPCOE-126-task-api-e…

5fd4d3c

…nhancements' into feature/ENTDAPCOE-126-task-api-enhancements

cleanup

d3b33d7

Merge branch 'master' into feature/ENTDAPCOE-126-task-api-enhancements

76a428d

Merge branch 'feature/ENTDAPCOE-126-task-api-enhancements' of https:/…

3ea772d

…/github.com/fidelity-contributions/camunda-camunda-bpm-platform into feature/ENTDAPCOE-126-task-api-enhancements

yanavasileva requested changes May 28, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Task api enhancement to include attachment and comments fields. #4306

Task api enhancement to include attachment and comments fields. #4306

sumankumarpani commented Apr 29, 2024 •

edited

yanavasileva commented May 13, 2024

sumankumarpani commented May 16, 2024

yanavasileva left a comment •

edited

CLAassistant commented May 24, 2024

yanavasileva left a comment

yanavasileva May 28, 2024

sumankumarpani May 28, 2024 •

edited

yanavasileva May 28, 2024

yanavasileva May 28, 2024

sumankumarpani May 29, 2024

yanavasileva May 28, 2024 •

edited

yanavasileva May 28, 2024

yanavasileva May 28, 2024

yanavasileva May 28, 2024

yanavasileva May 28, 2024

yanavasileva May 28, 2024

yanavasileva May 28, 2024

sumankumarpani May 29, 2024

sumankumarpani May 31, 2024

yanavasileva May 28, 2024

		this.attachmentExists = !getProcessEngine().getTaskService().getTaskAttachments(id).isEmpty();
		this.commentExists = !getProcessEngine().getTaskService().getTaskComments(id).isEmpty();

		@Test
		@Deployment(resources = "org/camunda/bpm/engine/test/api/oneTaskProcess.bpmn20.xml")

	if (withCommentAttachmentInfo && Context.getProcessEngineConfiguration().getHistoryLevel()!= HistoryLevel.HISTORY_LEVEL_NONE) {
	if (withCommentAttachmentInfo && !Context.getProcessEngineConfiguration().getHistoryLevel().equals(HistoryLevel.HISTORY_LEVEL_NONE)) {

		import static org.junit.Assert.*;
		import static org.junit.Assert.assertFalse;

-import static org.junit.Assert.*;
-import static org.junit.Assert.assertFalse;
+import static org.junit.Assert.assertEquals;
+import static org.junit.Assert.assertFalse;
+import static org.junit.Assert.assertNotNull;
+import static org.junit.Assert.assertNull;
+import static org.junit.Assert.assertTrue;
+import static org.junit.Assert.fail;

Task api enhancement to include attachment and comments fields. #4306

Are you sure you want to change the base?

Task api enhancement to include attachment and comments fields. #4306

Conversation

sumankumarpani commented Apr 29, 2024 • edited

yanavasileva commented May 13, 2024

sumankumarpani commented May 16, 2024

yanavasileva left a comment • edited

Choose a reason for hiding this comment

CLAassistant commented May 24, 2024

yanavasileva left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sumankumarpani May 28, 2024 • edited

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 May 28, 2024 • edited

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

sumankumarpani commented Apr 29, 2024 •

edited

yanavasileva left a comment •

edited

sumankumarpani May 28, 2024 •

edited

yanavasileva May 28, 2024 •

edited