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.

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

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

Open
wants to merge 8 commits into
base: master
Choose a base branch
from
Original file line number Diff line number Diff line change
Expand Up @@ -657,5 +657,12 @@
<@lib.parameter name = "parentTaskId"
location = "query"
type = "string"
desc = "Restrict query to all tasks that are sub tasks of the given task. Takes a task id." />

<@lib.parameter name = "withCommentAttachmentInfo"
location = "query"
type = "boolean"
defaultValue = "false"
last = last
desc = "Restrict query to all tasks that are sub tasks of the given task. Takes a task id." />
desc = "Check if task has attachments and/or comments. Value may only be `true`, as
Copy link
Member

Choose a reason for hiding this comment

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

❌ Please add performance considerations to the description. 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.

`false` is the default behavior." />
Copy link
Member

Choose a reason for hiding this comment

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

❌ 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.

Copy link
Author

Choose a reason for hiding this comment

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

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

Copy link
Author

Choose a reason for hiding this comment

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

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?

Copy link
Member

@yanavasileva yanavasileva Jun 12, 2024

Choose a reason for hiding this comment

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

@sumankumarpani, the ticket was initially reported for GET /task REST API but covering GET /task/{id} will add consistency to the endpoints. The TaskDto is used as a return type for all of those endpoints so the made changes so far make sense.

Copy link
Author

Choose a reason for hiding this comment

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

Got it, Thanks.

Copy link
Member

Choose a reason for hiding this comment

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

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

Original file line number Diff line number Diff line change
Expand Up @@ -129,8 +129,18 @@
<@lib.property
name = "tenantId"
type = "string"
last = true
desc = "If not `null`, the tenant id of the task." />

<@lib.property
name = "attachment"
type = "boolean"
desc = "Specifies if an attachment exists for the task." />

<@lib.property
name = "comment"
type = "boolean"
last = true
desc = "Specifies if an comment exists for the task." />

</@lib.dto>
</#macro>
Original file line number Diff line number Diff line change
Expand Up @@ -68,7 +68,9 @@
"binding": "version",
"version": 2
},
"tenantId": "aTenantId"
"tenantId": "aTenantId",
"attachments":false,
"comments":false
}
]
}'] />
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -119,7 +119,9 @@
"binding": "version",
"version": 2
},
"tenantId":"aTenantId"
"tenantId":"aTenantId",
"attachments":false,
"comments":false
}
]
}',
Expand Down Expand Up @@ -148,7 +150,9 @@
"caseDefinitionId": null,
"suspended": false,
"formKey": "embedded:app:develop/invoice-forms/approve-invoice.html",
"tenantId": null
"tenantId": null,
"attachments":false,
"comments":false
}
]
}'
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -53,7 +53,9 @@
"binding": "version",
"version": 2
},
"tenantId":"aTenantId"
"tenantId":"aTenantId",
"attachments":false,
"comments":false
}
}'] />

Expand Down
yanavasileva marked this conversation as resolved.
Show resolved Hide resolved
Original file line number Diff line number Diff line change
Expand Up @@ -49,7 +49,8 @@ public class TaskDto {
private String formKey;
private CamundaFormRef camundaFormRef;
private String tenantId;

private boolean attachments;
private boolean comments;
public String getId() {
return id;
}
Expand Down Expand Up @@ -194,6 +195,21 @@ public void setTenantId(String tenantId) {
this.tenantId = tenantId;
}

public boolean getAttachments() {
return attachments;
}
public void setAttachments(boolean attachments) {
this.attachments = attachments;
}

public boolean isComments() {
return comments;
}

public void setComments(boolean comments) {
this.comments = comments;
}

public static TaskDto fromEntity(Task task) {
TaskDto dto = new TaskDto();
dto.id = task.getId();
Expand Down Expand Up @@ -221,6 +237,8 @@ public static TaskDto fromEntity(Task task) {
dto.caseInstanceId = task.getCaseInstanceId();
dto.suspended = task.isSuspended();
dto.tenantId = task.getTenantId();
dto.attachments = task.getAttachments();
dto.comments = task.getComments();

try {
dto.formKey = task.getFormKey();
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -212,6 +212,8 @@ public class TaskQueryDto extends AbstractQueryDto<TaskQuery> {

private List<TaskQueryDto> orQueries;

private Boolean withCommentAttachmentInfo;

public TaskQueryDto() {

}
Expand Down Expand Up @@ -708,6 +710,11 @@ public void setVariableValuesIgnoreCase(Boolean variableValuesCaseInsensitive) {
this.variableValuesIgnoreCase = variableValuesCaseInsensitive;
}

@CamundaQueryParam(value = "withCommentAttachmentInfo", converter = BooleanConverter.class)
public void setWithCommentAttachmentInfo(Boolean withCommentAttachmentInfo) {
this.withCommentAttachmentInfo = withCommentAttachmentInfo;
}

@Override
protected boolean isValidSortByValue(String value) {
return VALID_SORT_BY_VALUES.contains(value);
Expand Down Expand Up @@ -1078,6 +1085,8 @@ public Boolean isVariableValuesIgnoreCase() {
return variableValuesIgnoreCase;
}

public Boolean getWithCommentAttachmentInfo() { return withCommentAttachmentInfo;}

@Override
protected void applyFilters(TaskQuery query) {
if (orQueries != null) {
Expand Down Expand Up @@ -1442,6 +1451,9 @@ protected void applyFilters(TaskQuery query) {
}
}
}
if (withCommentAttachmentInfo != null && withCommentAttachmentInfo) {
query.withCommentAttachmentInfo();
}
}

@Override
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -313,6 +313,8 @@ public void testGetSingleTask() {
.body("caseDefinitionId", equalTo(MockProvider.EXAMPLE_CASE_DEFINITION_ID))
.body("tenantId", equalTo(MockProvider.EXAMPLE_TENANT_ID))
.body("lastUpdated", equalTo(MockProvider.EXAMPLE_TASK_LAST_UPDATED))
.body("attachments", equalTo(MockProvider.EXAMPLE_TASK_ATTACHMENT_STATE))
.body("comments", equalTo(MockProvider.EXAMPLE_TASK_COMMENT_STATE))
.when().get(SINGLE_TASK_URL);
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -187,6 +187,8 @@ public void testSimpleTaskQuery() {
boolean returnedSuspensionState = from(content).getBoolean("[0].suspended");
String returnedFormKey = from(content).getString("[0].formKey");
String returnedTenantId = from(content).getString("[0].tenantId");
boolean returnedAttachmentsInfo = from(content).getBoolean("[0].attachments");
boolean returnedCommentsInfo = from(content).getBoolean("[0].comments");

assertThat(MockProvider.EXAMPLE_TASK_NAME).isEqualTo(returnedTaskName);
assertThat(MockProvider.EXAMPLE_TASK_ID).isEqualTo(returnedId);
Expand All @@ -210,6 +212,8 @@ public void testSimpleTaskQuery() {
assertThat(MockProvider.EXAMPLE_TASK_SUSPENSION_STATE).isEqualTo(returnedSuspensionState);
assertThat(MockProvider.EXAMPLE_FORM_KEY).isEqualTo(returnedFormKey);
assertThat(MockProvider.EXAMPLE_TENANT_ID).isEqualTo(returnedTenantId);
assertThat(MockProvider.EXAMPLE_TASK_ATTACHMENT_STATE).isEqualTo(returnedAttachmentsInfo);
assertThat(MockProvider.EXAMPLE_TASK_COMMENT_STATE).isEqualTo(returnedCommentsInfo);

}

Expand Down Expand Up @@ -521,6 +525,7 @@ private Map<String, Boolean> getCompleteBooleanQueryParameters() {
parameters.put("withCandidateUsers", true);
parameters.put("withoutCandidateUsers", true);
parameters.put("withoutDueDate", true);
parameters.put("withCommentAttachmentInfo", true);

return parameters;
}
Expand Down Expand Up @@ -586,6 +591,7 @@ private void verifyBooleanParameterQueryInvocation() {
verify(mockQuery).withCandidateUsers();
verify(mockQuery).withoutCandidateUsers();
verify(mockQuery).withoutDueDate();
verify(mockQuery).withCommentAttachmentInfo();
}

@Test
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -195,6 +195,10 @@ public abstract class MockProvider {
public static final String EXAMPLE_TASK_DEFINITION_KEY = "aTaskDefinitionKey";
public static final boolean EXAMPLE_TASK_SUSPENSION_STATE = false;

public static final boolean EXAMPLE_TASK_ATTACHMENT_STATE = false;

public static final boolean EXAMPLE_TASK_COMMENT_STATE = false;

// task comment
public static final String EXAMPLE_TASK_COMMENT_ID = "aTaskCommentId";
public static final String EXAMPLE_TASK_COMMENT_FULL_MESSAGE = "aTaskCommentFullMessage";
Expand Down Expand Up @@ -1055,7 +1059,9 @@ public static MockTaskBuilder mockTask() {
.caseExecutionId(EXAMPLE_CASE_EXECUTION_ID)
.formKey(EXAMPLE_FORM_KEY)
.camundaFormRef(EXAMPLE_FORM_KEY, EXAMPLE_FORM_REF_BINDING, EXAMPLE_FORM_REF_VERSION)
.tenantId(EXAMPLE_TENANT_ID);
.tenantId(EXAMPLE_TENANT_ID)
.attachments(EXAMPLE_TASK_ATTACHMENT_STATE)
.comments(EXAMPLE_TASK_COMMENT_STATE);
}

public static List<Task> createMockTasks() {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -50,6 +50,10 @@ public class MockTaskBuilder {
private String formKey;
private CamundaFormRef camundaFormRef;
private String tenantId;
private boolean attachments;
private boolean comments;



public MockTaskBuilder id(String id) {
this.id = id;
Expand Down Expand Up @@ -163,6 +167,16 @@ public MockTaskBuilder tenantId(String tenantId) {
return this;
}

public MockTaskBuilder attachments(boolean hasAttachments) {
this.attachments = hasAttachments;
return this;
}

public MockTaskBuilder comments(boolean hasComments) {
this.comments = hasComments;
return this;
}

public Task build() {
Task mockTask = mock(Task.class);
when(mockTask.getId()).thenReturn(id);
Expand All @@ -187,6 +201,8 @@ public Task build() {
when(mockTask.getFormKey()).thenReturn(formKey);
when(mockTask.getCamundaFormRef()).thenReturn(camundaFormRef);
when(mockTask.getTenantId()).thenReturn(tenantId);
when(mockTask.getAttachments()).thenReturn(attachments);
when(mockTask.getComments()).thenReturn(comments);
return mockTask;
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -44,6 +44,7 @@
import org.camunda.bpm.engine.task.Task;
import org.camunda.bpm.engine.task.TaskQuery;
import org.camunda.bpm.engine.variable.type.ValueType;
import org.camunda.bpm.engine.impl.history.HistoryLevel;

/**
* @author Joram Barrez
Expand Down Expand Up @@ -175,6 +176,7 @@ public class TaskQueryImpl extends AbstractQuery<TaskQuery, Task> implements Tas
// or query /////////////////////////////
protected List<TaskQueryImpl> queries = new ArrayList<>(Arrays.asList(this));
protected boolean isOrQueryActive = false;
protected boolean withCommentAttachmentInfo = false;

public TaskQueryImpl() {
}
Expand Down Expand Up @@ -1088,6 +1090,12 @@ protected boolean hasExcludingConditions() {
|| CompareUtil.elementIsNotContainedInArray(processInstanceBusinessKey, processInstanceBusinessKeys);
}

@Override
public TaskQuery withCommentAttachmentInfo() {
this.withCommentAttachmentInfo = true;
return this;
}

public List<String> getCandidateGroups() {
if (cachedCandidateGroups != null) {
return cachedCandidateGroups;
Expand Down Expand Up @@ -1441,6 +1449,13 @@ public List<Task> executeList(CommandContext commandContext, Page page) {
}
}

if (withCommentAttachmentInfo && Context.getProcessEngineConfiguration().getHistoryLevel()!= HistoryLevel.HISTORY_LEVEL_NONE) {
yanavasileva marked this conversation as resolved.
Show resolved Hide resolved
Copy link
Member

Choose a reason for hiding this comment

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

❌ 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)) {

for (Task task : taskList) {
// verify attachment and comments exists for the task
((TaskEntity) task).initializeAttachmentAndComments();
}
}

return taskList;
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -158,6 +158,8 @@ public class TaskEntity extends AbstractVariableScope implements Task, DelegateT
protected boolean isFormKeyInitialized = false;
protected String formKey;
protected CamundaFormRef camundaFormRef;
protected boolean attachmentExists;
protected boolean commentExists;

@SuppressWarnings({ "unchecked" })
protected transient VariableStore<VariableInstanceEntity> variableStore
Expand Down Expand Up @@ -1451,6 +1453,10 @@ public void initializeFormKey() {
}
}
}
public void initializeAttachmentAndComments(){
this.attachmentExists = !getProcessEngine().getTaskService().getTaskAttachments(id).isEmpty();
this.commentExists = !getProcessEngine().getTaskService().getTaskComments(id).isEmpty();
Comment on lines +1457 to +1458
Copy link
Member

@yanavasileva yanavasileva May 28, 2024

Choose a reason for hiding this comment

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

❌ 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();

}
yanavasileva marked this conversation as resolved.
Show resolved Hide resolved

@Override
public String getFormKey() {
Expand Down Expand Up @@ -1758,7 +1764,14 @@ public void bpmnError(String errorCode, String errorMessage, Map<String, Object>
throw ProcessEngineLogger.CMD_LOGGER.exceptionBpmnErrorPropagationFailed(errorCode, ex);
}
}

@Override
public boolean getAttachments() {
return attachmentExists;
}
@Override
public boolean getComments() {
return commentExists;
}
yanavasileva marked this conversation as resolved.
Show resolved Hide resolved
public void escalation(String escalationCode, Map<String, Object> variables) {
ensureTaskActive();
ActivityExecution activityExecution = getExecution();
Expand Down
5 changes: 5 additions & 0 deletions engine/src/main/java/org/camunda/bpm/engine/task/Task.java
Original file line number Diff line number Diff line change
Expand Up @@ -189,4 +189,9 @@ public interface Task {
*/
void setTenantId(String tenantId);

/** Returns if an attachment exists for the task */
boolean getAttachments();
/** Signifies if a comment exists for the task */
boolean getComments();
yanavasileva marked this conversation as resolved.
Show resolved Hide resolved

}
yanavasileva marked this conversation as resolved.
Show resolved Hide resolved
Original file line number Diff line number Diff line change
Expand Up @@ -1104,4 +1104,9 @@ public interface TaskQuery extends Query<TaskQuery, Task> {
* this exception, {@link #or()} must be invoked first.
*/
TaskQuery endOr();

/**
* Evaluates existence of attachment and comments associated with the task, defaults to false.
*/
Comment on lines +1108 to +1110
Copy link
Member

Choose a reason for hiding this comment

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

❌ 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.

TaskQuery withCommentAttachmentInfo();
yanavasileva marked this conversation as resolved.
Show resolved Hide resolved
}