Feature/git issue #4487 fetch task variables feature#4754
Feature/git issue #4487 fetch task variables feature#4754prakashpalanisamy wants to merge 11 commits intocamunda:masterfrom
Conversation
|
@yanavasileva : This PR has code changes for all the APIs and test cases. Open API spec is not yet done from my end. Will be updating in few days time. Meanwhile I need input on the current commit/checkpoint. Thank you!! |
|
Hi @prakashpalanisamy, Thanks |
|
@venetrius I have updated the PR with few commits few days back. Please have a look when you get time. Please let me know of your comments/inputs. Thank you!! |
|
Hi @prakashpalanisamy, |
There was a problem hiding this comment.
Hi .prajwolbhandari1. @prakashpalanisamy
I have added a few questions / suggestions
| .camundaFormRef(EXAMPLE_FORM_KEY, EXAMPLE_FORM_REF_BINDING, EXAMPLE_FORM_REF_VERSION) | ||
| .tenantId(EXAMPLE_TENANT_ID) | ||
| .taskState(EXAMPLE_HISTORIC_TASK_STATE) | ||
| .tenantId(EXAMPLE_TENANT_ID) |
There was a problem hiding this comment.
the tenantId is duplicate and hence was removed. This was used in some of my related tests, So I removed this line item. Hope thats okay.
| List<TaskDto> tasks = new ArrayList<TaskDto>(); | ||
| for (Task task : matchingTasks) { | ||
| VariableMap taskVariables; | ||
| if (withTaskVariablesInReturn) { |
There was a problem hiding this comment.
This loop can introduce 2 extra DB query per task instance. One to get Variables and one for the Comments.
Did you consider if you could do it with fewer DB calls?
There was a problem hiding this comment.
Hi @venetrius , Here one call is being made for each task in the loop - either taskVars or taskLocalVars. The comments and attachment info is already obtained and just used to populate the DTO accordingly. Please let me know your thoughts.
There was a problem hiding this comment.
@venetrius , I understand that there is a DB call made to get the comment and attachment info additional to getAllTask, while getting the list of tasks (matchingTasks). I understand in the above loop, one additional call is being made to get either taskVariables or taskLocalVariables to be added to the response. OpenAPI documentation is updated with exercising caution while using the comment/attachment request. Do you think its apt to add another cautionary note in the documentation for the variablesInReturn query as well? Please provide your thoughts and direction on how to proceed further.
| List<Task> matchingTasks = executeTaskQuery(firstResult, maxResults, query); | ||
|
|
||
| List<TaskDto> tasks = new ArrayList<TaskDto>(); | ||
|
|
There was a problem hiding this comment.
Line 104 - 115 seems unececcerly complicated. I think this would mean the same:
boolean withTaskVars = Boolean.TRUE.equals(queryDto.getWithTaskVariablesInReturn());
boolean withTaskLocalVars = Boolean.TRUE.equals(queryDto.getWithTaskLocalVariablesInReturn());
boolean withCommentInfo = Boolean.TRUE.equals(queryDto.getWithCommentAttachmentInfo());
if (withTaskVars || withTaskLocalVars){
return getVariablesForTasks(engine, matchingTasks, withTaskVars, withCommentInfo);
}
There was a problem hiding this comment.
@venetrius I agree with your comments. Ill push the updates shortly.
There was a problem hiding this comment.
I have pushed the changes. Can you please take a look?
@venetrius , I believe it was directed to me. I will take a look at your comments. Thanks! |
Yes you are correct, sorry. |
|
@venetrius Whenever you get time, can you please take a look at my response and provide your inputs/directions. Thanks! |
|
Hi @prakashpalanisamy |
|
Hi @prakashpalanisamy |
|
No problem @venetrius . Please take your time 🙂 |
venetrius
left a comment
venetrius
left a comment
There was a problem hiding this comment.
👍 Looks good to me! Thanks for considering my recommendations.
Could you rebase from master afterwards I will run the CI again and it should be ready to go.
|
@prakashpalanisamy |
|
Thank you @venetrius |
2 participants
This PR has code changes for all the APIs and test cases. Open API spec is not yet done from my end. Will be updating in few days time. Meanwhile I need input on the current commit/checkpoint. Thank you!!
Related to: #4487