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

Function "Select all rows" is not applied to KitodoScripts #5267

Closed
andre-hohmann opened this issue Jul 27, 2022 · 8 comments · Fixed by #5511
Closed

Function "Select all rows" is not applied to KitodoScripts #5267

andre-hohmann opened this issue Jul 27, 2022 · 8 comments · Fixed by #5511
Assignees
Labels

Comments

@andre-hohmann
Copy link
Collaborator

andre-hohmann commented Jul 27, 2022

Describe the bug
If a list of results with several pages is shown in "processes" and all processes are selected, the following occurs:

  • The KitodoScpripts are only applied to the processes of the result page, which is shown to the user.

To Reproduce
Steps to reproduce the behavior:

  1. Search enough processes to generate a result list of several result pages.
  2. Use function "select all rows".
  3. Click on the icon for the next result page to see that all rows are selected.
  4. Go back to the first result page
  5. Click on "Actions"
  6. Click on KitodoScript "addData" and use key and value according to the applied ruleset and execute the KitodoScript
  7. Wait until the KitodoScript is finished.
  8. Open a process of the first result page and check, if the metadata has been added
  9. Click on the icon for the second result page to see if all rows are selected
  10. Open a process of the second result page and check, if the metadata has been added

Expected behavior
If all rows of a result list are selected, they all should be regarded, if a KitodoScript is applied.

Screenshots
function "select all rows"
Trefferliste01
function "Actions"
Trefferliste02

Release
Kitodo.Production version 3.4.4

Desktop (please complete the following information):

  • OS: Windows 10
  • Chrome 103.0.5060.134 (Offizieller Build) (64-Bit)
@matthias-ronge
Copy link
Collaborator

Even more: If you select all processes on all pages, and there are 15 pages, the KitodoScript is executed 15 times for each process of page 1.

Correct behaviour should be: The script should be executed once for each process of each page.

@BartChris
Copy link
Collaborator

BartChris commented Aug 12, 2022

probably related to/duplicate of: #4948

@andre-hohmann
Copy link
Collaborator Author

Thanks a lot for your addition. I would not close the issue as duplicate, because in the other issue, only the generation of images is described.
It is important to me to point out, that other functions are affected from this behavior, too.

@BartChris
Copy link
Collaborator

BartChris commented Oct 28, 2022

I just looked into it and it seems that the problem does not appear if i select all processes by hand, only if the functionality "Select all rows" is used. In this case the process list is already incorrect before the data is passed to the actual processing. So this probably has something to do with the setup and configuration of the Datatable.

The processes on the first page are already repeated/duplicated in the list selectedProcessesOrProcessDTOs when "Select all rows" is selected and the setter method is called.

public void setSelectedProcessesOrProcessDTOs(List<? extends Object> selectedProcessesOrProcessDTOs) {
this.selectedProcessesOrProcessDTOs = selectedProcessesOrProcessDTOs;

PS:
The feature of selecting all rows seems to be quite problematic (primefaces/primefaces#6730 , primefaces/primefaces#8110)
This leads me to the general question if processing a lot of processes is really best served by the datatable interface or if the batch interface (with extended functionality) is better suited for that. But this is probably out of the scope of this issue.

@andre-hohmann
Copy link
Collaborator Author

@BartChris : Thanks a lot for the contribution. I hope that this supports the solution of the issue.

You are right, regarding the possible relocation of the function into the batch interface or another interface.
That would be more appropriate in Discussion - only after all aspects are analyzed and discussed, an issue with a precise demand/proposal should be created.
I guess, that it will be a major feature/improvement.

For me, it is important that the current implementation works correctly - although it might not be the best implementation.

@BartChris
Copy link
Collaborator

BartChris commented Nov 7, 2022

It seems as if the issue can be fixed, when the "select all" functionality is not delegated to the Primefaces mechanism but is implemented by hand.

I added the following method to ProcessForm.java (ProcessBaseListView.java)

 public void selectAll() throws DataException {
          ProcessService processService = ServiceManager.getProcessService();
          this.selectedProcessesOrProcessDTOs = processService.findByQuery(processService.getQueryForFilter(
            this.isShowClosedProcesses(),  isShowInactiveProjects(),getFilter()), false);
    }

and added a button to call that method. This queries the search index with the current filter settings and sets the currently selected records to all elements in the result.

I have the impression that implementing that mechanism as a custom method is the only way to determine what data is actually selected, when the user chooses "Select all".
Another advantage is that the user can also unselect rows without losing the other selections. This also fixes the issue #5266.

A video demonstrating the behaviour after applying the fix:

kitodo_selection_fix_neu.mp4

@solth
Copy link
Member

solth commented Nov 7, 2022

@BartChris thanks for this proposed fix. I wonder, though, if it is a viable solution to load all elements in a real production system with millions of processes instead of just 13 like in your example. I think that is the main reason why the "Select all" button hasn't been implemented in this most straight-forward way until now. I guess we need to find a solution where the selectedProcessesOrProcessDTOs variable does not really have to hold all those processes at once but instead fake the selection and somehow apply the "lazy loading" principle here as well.

@BartChris
Copy link
Collaborator

BartChris commented Nov 7, 2022

@solth You are correct i suppose. Holding a lot of processes in memory is probably not a good idea and it would probably be better if triggering the button just sets a variable allProcessesSelected to true and then those processes can be retrieved when the list of all processes is actually needed. -> when "getSelectedProcesses" is called.

On the other hand: Is the current implementation (in master) not doing exactly that (building a huge list)? Using "Select all", results in "setSelectedProcessesOrProcessDTOs" to be called with a potentially large list of Processes, with the difference that processes from page one are repeated over and over.

PS: Doing it in the prefered (memory lightweight) way would probably still have the problem that after clicking only on a single row, all other selections are lost. So that it is impossible to unselect processes even if they should not be included in the batch process. There might of course be scenarios where somebody wants to select millions of records at once, but there are probably a lot of cases where it would be more useful to select a subset and allow that processes can be deselected by hand, witthout losing the selection. I suppose there is not an optimal solution. This is the main reason why i think the datatable is a suboptimal batch interface.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
No open projects
Development

Successfully merging a pull request may close this issue.

6 participants