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
JBPM-6670 - Allow to search tasks assigned by potential owner through #1084
Conversation
Can one of the admins verify this PR? Comment with 'ok to test' to start the build. |
3 similar comments
Can one of the admins verify this PR? Comment with 'ok to test' to start the build. |
Can one of the admins verify this PR? Comment with 'ok to test' to start the build. |
Can one of the admins verify this PR? Comment with 'ok to test' to start the build. |
Can one of the admins verify this PR? Comment with 'ok to test' to start the build. |
1 similar comment
Can one of the admins verify this PR? Comment with 'ok to test' to start the build. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
few comments left to discuss/address
potOwnersList.add(potOwner); | ||
|
||
if(userGroupCallback.getGroupsForUser(potOwner) != null) { | ||
potOwnersList.addAll(userGroupCallback.getGroupsForUser(potOwner)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd move this to separate line
List<String> groups = userGroupCallback.getGroupsForUser(potOwner);
otherwise it might be rather heavy in some cases to call it twice
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
|
||
@Override | ||
public QueryResultMapper<List<UserTaskInstanceWithPotOwnerDesc>> forColumnMapping(Map<String, String> columnMapping) { | ||
return new UserTaskInstanceWithModifVarsQueryMapper(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is it intended to ignore columnMapping?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, why?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the method has argument columnMapping but it is never used like it most likely should be added as constructor of return new UserTaskInstanceWithModifVarsQueryMapper()
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry Maciej, I didn't understand you well. I think is good ignoring the argument columnMapping for this mapper. Similar to: https://github.com/kiegroup/jbpm/blob/master/jbpm-services/jbpm-kie-services/src/main/java/org/jbpm/kie/services/impl/query/mapper/UserTaskInstanceWithVarsQueryMapper.java
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
that's why I asked if that was intended or not. If you believe there will not be a need to read up custom variables then it's ok with me.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, it's ok
|
||
@Override | ||
public QueryResultMapper<List<UserTaskInstanceWithPotOwnerDesc>> forColumnMapping(Map<String, String> columnMapping) { | ||
return new UserTaskInstanceWithPotOwnerQueryMapper(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same here, is it intended to ignore columnMapping?
95c58b9
to
44d68f7
Compare
ok to test |
44d68f7
to
90c29ff
Compare
90c29ff
to
4e9ea14
Compare
Allow to search tasks assigned by potential owner through