-
Notifications
You must be signed in to change notification settings - Fork 68
Remove reliance on bidirectional association in task-user relation #6802
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
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -304,20 +304,15 @@ private BeanQuery formBeanQuery(Map<?, String> filters, boolean onlyOwnTasks, bo | |
| */ | ||
| public void replaceProcessingUser(Task task, User user) { | ||
| User currentProcessingUser = task.getProcessingUser(); | ||
|
|
||
| if (Objects.isNull(user) && Objects.isNull(currentProcessingUser)) { | ||
| logger.info("do nothing - there is neither a new nor an old user"); | ||
| } else if (Objects.isNull(user)) { | ||
| currentProcessingUser.getProcessingTasks().remove(task); | ||
| task.setProcessingUser(null); | ||
| } else if (Objects.isNull(currentProcessingUser)) { | ||
| user.getProcessingTasks().add(task); | ||
| task.setProcessingUser(user); | ||
| } else if (Objects.equals(currentProcessingUser.getId(), user.getId())) { | ||
| logger.info("do nothing - both are the same"); | ||
| } else { | ||
| currentProcessingUser.getProcessingTasks().remove(task); | ||
| user.getProcessingTasks().add(task); | ||
| task.setProcessingUser(user); | ||
| } | ||
| } | ||
|
|
@@ -805,6 +800,41 @@ public static List<Task> getTasksInWorkByOtherUsers(List<Task> tasks) { | |
| .collect(Collectors.toList()); | ||
| } | ||
|
|
||
| /** | ||
| * Retrieve and return all tasks assigned to the given user | ||
| * that are currently in progress and linked to a process. | ||
| * | ||
| * @param user the processing user | ||
| * @return list of tasks in progress for the given user | ||
| */ | ||
| public List<Task> getTasksInProgress(User user) { | ||
| String hql = "FROM Task t WHERE t.processingUser = :user " | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Limiting / restrict the task list to tasks where the user is set as
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Good point, i have to think about that.
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I just noticed that this method is mostly used for emptiness checks; we can probably optimize: If only existence is checked, we do not need to retrieve all the tasks and should do an existence check.
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
return user.getProcessingTasks().stream()I think establishing the stream in that way also limited the tasks down to those the user is actually assigned to before. |
||
| + "AND t.processingStatus = :status " | ||
| + "AND t.process IS NOT NULL"; | ||
| Map<String, Object> params = Map.of( | ||
| "user", user, | ||
| "status", TaskStatus.INWORK | ||
| ); | ||
| return dao.getByQuery(hql, params); | ||
| } | ||
|
|
||
| /** | ||
| * Returns whether the user has at least one task in progress. | ||
| * @param user the processing user | ||
| * @return true if the user has tasks in progress, otherwise false | ||
| */ | ||
| public boolean hasTasksInProgress(User user) throws DAOException { | ||
| String hql = "FROM Task t " | ||
| + "WHERE t.processingUser = :user " | ||
| + "AND t.processingStatus = :status " | ||
| + "AND t.process IS NOT NULL"; | ||
|
|
||
| return dao.has(hql, Map.of( | ||
| "user", user, | ||
| "status", TaskStatus.INWORK | ||
| )); | ||
| } | ||
|
|
||
| /** | ||
| * Compute and return list of tasks that are eligible as 'currentTask' for a new correction comment. | ||
| * | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -138,13 +138,13 @@ | |
|
|
||
| <h:panelGroup styleClass="action" | ||
| rendered="#{SecurityAccessController.hasAuthorityToUnassignTasks()}" | ||
| title="#{empty UserForm.getTasksInProgress(item) ? msgs.userWithoutTasks : msgs.unassignTasks}"> | ||
| title="#{UserForm.hasTasksInProgress(item) ? msgs.unassignTasks : msgs.userWithoutTasks}"> | ||
|
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It could be that the user list also benefits from the more efficient checks. |
||
| <p:commandLink id="unassignTasks" | ||
| action="#{UserForm.resetTasksToOpen(item)}" | ||
| styleClass="#{empty UserForm.getTasksInProgress(item) ? 'ui-state-disabled' : ''}" | ||
| disabled="#{empty UserForm.getTasksInProgress(item)}" | ||
| styleClass="#{UserForm.hasTasksInProgress(item) ? '' : 'ui-state-disabled'}" | ||
| disabled="#{not UserForm.hasTasksInProgress(item)}" | ||
| update="@this"> | ||
| <h:outputText><i class="fa fa-user-times"/></h:outputText> | ||
| <h:outputText><i class="fa fa-user-times"/></h:outputText> | ||
| <p:confirm message=" #{msgs.confirmUnassignTasks}" header="#{msgs.confirmRelease}"/> | ||
| </p:commandLink> | ||
| </h:panelGroup> | ||
|
|
||
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.
Just a question: Would an additional restriction to the used client of the current user not even more helpful? If an user is assigned to more clients than this list here would include all the tasks over all clients and not only the tasks of the current used client in the UI.
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.
Yes, might be useful.
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.
We have not filtered by client before, maybe this is not necessary, as this method is only called in special places.