Multi page selection in favorite list

[ThoWagen]

Currently, only the records of one page can be selected in the favorite list.

In this PR, multi page selection is introduced. This means that one can select records from multiple pages in the favorite list or even all.

This could also be implemented for the result list in the future but it would be more complicated to store the selection for different searches and the option to select all results wouldn't make sense for thousands of results.

[demiankatz]

Thanks, @ThoWagen, this looks like a useful feature. I've scheduled this PR for 10.0 to be sure it gets more attention after 9.1 is completed. My only thought in the meantime is that in some situations, the limitation of only selecting one page at a time may have benefits, since it imposes limits on functionality that may have performance or capacity caps. As you say, search results are challenging because they can be very large... but it is also theoretically possible for a favorite list to have hundreds or thousands of records as well. This is very likely to exceed the capacity of the book bag, so "select all" and "add to cart" is likely to be problematic here. And other things like exports may not scale up so well. Have you done any testing around the edges of this implementation? Is it possible that we need to set some kind of configurable upper limit on how many things can be selected and/or make this whole feature configurable so it can be disabled in situations where it will cause problems?

[ThoWagen]

You have a good point. We use this in our system for more than a year now. Once we had a user with over a thousand of records in his favorite list. He wanted to print them all at once and this resulted in running out of memory for loading all of the records. It is very rare that users have that many favorites but we still should address this. I think there are multiple possible solutions for this:

1. One could restrict the number of favorites a user can have. This is probably the laziest solution and I don't think we should do this. But I just wanted to mention the option.
2. Restricting the number of ids that are provided by VuFind/Search/Favorites/Results and informing the user if the limit is exceeded. Here informing the user might be a tricky part but we wouldn't have to change any other parts.
3. Restricting the the number of records that can be used by the different bulk actions. If the limit is exceeded don't perform the action and inform the users. The limits could be individually configured for the different actions and are also used for any other ways when the actions are used.

I think 3. would be the best solution.

Maybe we could provide both select_all and select_all_on_page and make them configurable.
What do you think?

[demiankatz]

Thanks, @ThoWagen, I agree that option 3 seems like the best solution. I also like the idea of offering the option of both types of "select all" buttons. Perhaps it would make sense to tackle the bulk action limits in a separate PR to prevent the work here from getting too complicated. I could imagine that some are more complicated than others -- e.g. for "export," different methods might potentially have different limitations, so we might need to add some more complex configuration there.

[ThoWagen]

Now, multi page selection and the select-all checkboxes are configurable. Supporting all the different options was trickier than expected. But I created Mink tests for them and now it should work.

[demiankatz]

@ThoWagen, as discussed on today's Community Call, I think the next step before we can move this forward is to begin implementing limits on bulk actions. We should figure out which actions need limits, where to configure them, etc. Do you have any thoughts on this, or would you like me to try to find time to prototype something? (I'm willing, but it may be a couple of months before I get to it at the current rate of project activity).

[ThoWagen]

Maybe we can create a BulkAction section in the config.ini and set the limits there. Then we can experiment with the different actions and find out the best values. But I don't know yet when I will be able to get into this.

[demiankatz]

Maybe we can create a BulkAction section in the config.ini and set the limits there. Then we can experiment with the different actions and find out the best values. But I don't know yet when I will be able to get into this.

Yes, that makes sense to me. Such a section might also offer the opportunity to individually disable unwanted bulk options (by setting limit to 0, for example). Since neither of us knows when we will have time for this yet, whoever gets there first can comment here when beginning work, and then we will ensure that we don't both do it at the same time. :-)

[ThoWagen]

@demiankatz I will have a look into the limits now.

[demiankatz]

Thanks, @ThoWagen! Sorry I didn't manage to get there myself. Also, it looks like there's a small conflict that needs to be resolved here when you have a moment.

[demiankatz]

Thanks for the progress here, @ThoWagen -- see below for one minor suggestion related to the test suite.

Beyond that, the only remaining question is whether anything should be adjusted to be more future-proof in case we decide to add this functionality to search results as well. As you noted in the description here, there are some obvious challenges to implementing that, and it makes sense to get this functionality merged before attempting to add that. I'm just wondering if we should make any changes to the configuration setting names so that if we add more options in the future, they do not become ambiguous or confusing. Would it make sense to use multi_page_favorites_selection and checkbox_select_all_favorites_type (or something like that) to further make explicit the limited scope of this functionality? I think if we added similar functionality for other bulk actions, we would need to configure it independently, and if we had the same setting names in multiple sections of config.ini, that might make certain types of errors more common. What do you think?

Once we have settled these finishing touches, I will ask @sturkel89 to do some hands-on testing and see if she can find any remaining problems with the functionality (though I'm optimistic that it is already quite solid, given the thoroughness of the test suite).

[ThoWagen]

Unfortunately, there are still some bugs in this PR. I will fix them when I have the time for it. But in this state it is not ready to be reviewed further.

[demiankatz]

Unfortunately, there are still some bugs in this PR. I will fix them when I have the time for it. But in this state it is not ready to be reviewed further.

Thanks, @ThoWagen! Let me know if I can be of any help along the way.

[ThoWagen]

The bugs are fixed after significant refactoring in list_item_selection.js. Also implemented the suggested changes of the config names.

Now, it should not be hard to add multi page selection for searches as long as we don't also add a "select all global" button. But we would need a "clear selection" button instead. And it could also be implemented for checked out items and holds. Actually in all places where we have select checkboxes on multiple pages. But this is something for followup PRs of course.

I think we could support some bulk actions for much larger numbers of records if we implement this with some kind of async background jobs and present the users a loading bar or something similar. But since this is not a huge priority for us I don't this I will look into this anytime soon.

[ThoWagen]

@sturkel89 Thanks for the hint. The checkbox was supposed to be hidden. It is fixed now.

[sturkel89]

The orphan checkbox is now hidden, and all of the multi-page selection features in Favorites are still working properly. Ready to merge!

[sturkel89]

I spoke too soon! I notice that the label for selecting all on page is now showing a variable name instead of the appropriate public-facing label. Can you fix this before the branch is merged? THANKS!

[ThoWagen]

@sturkel89 select_all is still being translated. Is it possible that you tested other branches in the meantime and the languages cache got replaced? That is usually the reason why translations don't work properly when switching between different branches.

[sturkel89]

@sturkel89 select_all is still being translated. Is it possible that you tested other branches in the meantime and the languages cache got replaced? That is usually the reason why translations don't work properly when switching between different branches.

Yes, that is definitely a possibility! I went into another branch and came back out, and also cleared some caches along the way to repair another problem. I only noticed this issue when I went back into your branch; I hadn't noticed it when I went in the first time today, so it probably wasn't a problem then.

Sorry for the mix-up!

[demiankatz]

Thanks for the progress here, @ThoWagen and @sturkel89. I did a bit more thorough reviewing and testing myself and had a few comments (see below).

Also, a general user interface question: might there be value in having some indicator of how many items are selected somewhere? I realize that space is at a premium, so maybe it's not worth it, but it's possible to get into a situation where many checkboxes are selected, but there is no visible indication that this is the case -- e.g. click "select all entries" and then click "select all entries on page". If there's more than one page of results, this gets you to a state where nothing is checked on the current page, but lots of values are selected. If a user doesn't entirely understand the user interface, this could lead them to unexpected results.

[ThoWagen]

@demiankatz Concerning your question about the selection count indicator I am working on adding a "clear selection" button that also shows the current selection count.

[demiankatz]

Thanks for addressing all of my review comments, @ThoWagen -- I think we are nearly done.

I have one more minor comment below, plus I'm curious if you saw my question about the UI design in my previous review. Now that I look at how the checkboxes and buttons are wrapped:

It seems to me that it wouldn't be too hard to add a [x entries selected] indicator to the right of the checkboxes without impacting the overall screen real estate. I think this might be helpful to further highlight the effect of clicking some of the checkboxes, and to alert the user of off-screen checked boxes. If you dislike the idea, I won't insist on it, but I think it might be useful if it's not hard to implement.

[demiankatz]

Thanks, @ThoWagen, regarding the clear button: I see you must have answered that part of my question while I was in the process of reviewing your other changes. Sorry for missing your earlier comment!

[ThoWagen]

I added a button that clears the complete selection and is shown if multi page selection is enabled. It also has an indicator for the selection count. It should be useful especially if multi page selection is enabled but the select all global checkbox is not.

I also removed the flex design that ensures that the "select all" checkbox is shown in the same row as the action buttons since that does not work with multiple checkboxes and the new button. If you still want that behavior in the case that multi page selection is disabled I can look into it. But it might make things more complicated so that it is not worth the effort.

If you approve the current solution I will also add a Mink test for the clear selection button.

[demiankatz]

Thanks, @ThoWagen, I think that functionally speaking, this is very helpful and works the way I would expect. Just two thoughts:

1.) I wonder if it would be more flexible from an i18n perspective if the translation string had a placeholder for the count in it (e.g. Clear Selection (%%count%%)), instead of using a separate HTML element to hold the count. That would make it easier to customize with something like Clear all %%count%% selected checkboxes if somebody wanted to do something like that. I realize that supporting this would require leveraging our Javascript-side translation system, which adds a bit of overhead to everything (passing the translation string to the client through the jsTranslations view helper, and using VuFind.translate to generate the new output). Maybe it's not worth the effort, but I thought it might be worth considering at least.

2.) I think the aesthetics of the button look a little bit off. In some themes, the button font doesn't match other fonts on the page. In some places the button text is bold while other text is not. In every situation, the text seems out of alignment with the checkboxes, and the height of the clear button is different from the buttons on the line above (which isn't necessarily a bad choice but might look a little distracting). Here's what I'm seeing, at least:

bootstrap3:

bootprint3:

sandal:

Do you think there might be some simple improvements we could make to the styles?

[ThoWagen]

@demiankatz
About 1.): I think thats a good idea. I changed it.

About 2.): I revised the design. Now the text in the button should be aligned with the checkbox labels and also the font style should match. I intended to make have a lower height for the button. I think it would be to large if it has the same size as the buttons above. But I can see that the design may not yet be optimal. However, I am not sure how to improve it. Do you have any idea? Or maybe some one else could share their opinion.

[demiankatz]

Thanks, @ThoWagen, I think this is much better now. I agree that increasing the button height would likely make things worse rather than better. I'll ask @sturkel89 to give this one more look in case she has any further opinions, and if it looks good to her, I think we can merge it.

[sturkel89]

I think the Clear Selection button and the selected number indicator are a great improvement to this feature!

I reviewed the improved versions in all three branches. The font size and styling are consistent within each branch between the relevant elements, and the button height seems to be just right. Functionality is perfect, as far as I can see (including sorting within Favorites lists!).

I think we're ready to merge! Thanks for all of your good work on this, Thomas!

[demiankatz]

Thanks, @sturkel89 -- merging now!