Description
Jonathan Druart
2023-12-13 14:58:32 UTC
Would it be conceivable to have 2 (UI) tables, one for current holds and one for old ones? Katrin, Nick, do you have opinions on this? I like this one, we currently have a need for a deleted holds api (so outside systems can send emails related to cancellations) and this maes sense once we have that I think separating into current and old makes sense from a usability point of view. Created attachment 160100 [details] [review] Bug 35560: [POC] Make the holds list use the REST API Created attachment 160101 [details] [review] Bug 35560: DBIC specific I am willing to continue the work here (during 24.05 cycle) if there is commitment from one tester and one QA member. Known problems: 0. Fix bug 35353 first 1. Both tables are using the same table settings (could be fixed on a separate bug, not considering blocker) 2. Missing tests 3. Not filtering on status 4. UI needs improvement (at least header missing on top of each table) I'm in. I'm in *** Bug 35353 has been marked as a duplicate of this bug. *** I do prefer these API's being added in their own bug personally.. that way they get clear visability in the release notes and are simple to QA directly (In reply to Tomás Cohen Arazi from comment #8) > I'm in. And my axe! (In reply to Jonathan Druart from comment #1) > Would it be conceivable to have 2 (UI) tables, one for current holds and one > for old ones? That seems reasonable to me. Just have a couple tables "Current holds" and "Historical holds" or some such wording. (In reply to Jonathan Druart from comment #1) > Would it be conceivable to have 2 (UI) tables, one for current holds and one > for old ones? Go ahead! I meant to do that exact same thing many times but failed to have the time! Created attachment 163831 [details] [review] Bug 35560: Use the REST API for holds This patch uses the new REST API endpoint to retrieve the old holds. We can now have 2 tables, one for the current holds and one for the old ones. Test plan: Have several holds with several statuses. Notice that you see the holds in the respective tables. Test the usual sorting filtering DT feature and confirm that everything works as expected. Created attachment 163832 [details] [review] Bug 35560: Add filter on status (In reply to Jonathan Druart from comment #7) > I am willing to continue the work here (during 24.05 cycle) if there is > commitment from one tester and one QA member. > > > Known problems: > 0. Fix bug 35353 first Done. > 1. Both tables are using the same table settings (could be fixed on a > separate bug, not considering blocker) Decided to have this on its own bug. > 2. Missing tests Done. > 3. Not filtering on status Done. > 4. UI needs improvement (at least header missing on top of each table) Looks good enough for now for me. Let me know if you have suggestions. Ready for testing. Created attachment 164093 [details] [review] Bug 35560: (follow-up) Add in-page links to jump between tables Also a terminology change: I think "Past holds" reads better than "Historical holds." This works well for me except with title sorting. The "anti-the" sorting type doesn't seem to be taken into account even though the markup looks correct. (In reply to Owen Leonard from comment #19) > This works well for me except with title sorting. The "anti-the" sorting > type doesn't seem to be taken into account even though the markup looks > correct. That's is something global, we need to fix it at the REST API level (no idea how!). It should be fixed separately. Opened bug 36633. (In reply to Jonathan Druart from comment #20) > That's is something global Does that mean we can't use any custom sorting plugins? Created attachment 165231 [details] [review] Bug 35560: Use the REST API for holds This patch uses the new REST API endpoint to retrieve the old holds. We can now have 2 tables, one for the current holds and one for the old ones. Test plan: Have several holds with several statuses. Notice that you see the holds in the respective tables. Test the usual sorting filtering DT feature and confirm that everything works as expected. Signed-off-by: Lucas Gass <lucas@bywatersolutions.com> Created attachment 165232 [details] [review] Bug 35560: Add filter on status Signed-off-by: Lucas Gass <lucas@bywatersolutions.com> Created attachment 165233 [details] [review] Bug 35560: (follow-up) Add in-page links to jump between tables Also a terminology change: I think "Past holds" reads better than "Historical holds." Signed-off-by: Lucas Gass <lucas@bywatersolutions.com> Works well and looks good, besides the already noted title sorting. I am adding my sign-off here but leaving at NSO for more testers. (In reply to Owen Leonard from comment #22) > (In reply to Jonathan Druart from comment #20) > > > That's is something global > > Does that mean we can't use any custom sorting plugins? string-sort should not be needed dates are sorted correctly already______ nsb/nse: I don't know how behaves the dbms callnumber: should work without additional trick, but needs to be confirmed Am I missing other ones? Not unexpectedly, the patches no longer apply... git bz apply 35560 Bug 35560 - Use the REST API for holds history 165231 - Bug 35560: Use the REST API for holds 165232 - Bug 35560: Add filter on status 165233 - Bug 35560: (follow-up) Add in-page links to jump between tables Apply? [(y)es, (n)o, (i)nteractive] y Applying: Bug 35560: Use the REST API for holds Using index info to reconstruct a base tree... M koha-tmpl/intranet-tmpl/prog/en/modules/members/holdshistory.tt Falling back to patching base and 3-way merge... Auto-merging koha-tmpl/intranet-tmpl/prog/en/modules/members/holdshistory.tt CONFLICT (content): Merge conflict in koha-tmpl/intranet-tmpl/prog/en/modules/members/holdshistory.tt error: Failed to merge in the changes. Patch failed at 0001 Bug 35560: Use the REST API for holds Created attachment 177161 [details] [review] Bug 35560: Use the REST API for holds This patch uses the new REST API endpoint to retrieve the old holds. We can now have 2 tables, one for the current holds and one for the old ones. Test plan: Have several holds with several statuses. Notice that you see the holds in the respective tables. Test the usual sorting filtering DT feature and confirm that everything works as expected. Signed-off-by: Lucas Gass <lucas@bywatersolutions.com> Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org> Created attachment 177162 [details] [review] Bug 35560: Add filter on status Signed-off-by: Lucas Gass <lucas@bywatersolutions.com> Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org> Created attachment 177163 [details] [review] Bug 35560: (follow-up) Add in-page links to jump between tables Also a terminology change: I think "Past holds" reads better than "Historical holds." Signed-off-by: Lucas Gass <lucas@bywatersolutions.com> Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org> Created attachment 177200 [details] [review] Bug 35560: Use the REST API for holds This patch uses the new REST API endpoint to retrieve the old holds. We can now have 2 tables, one for the current holds and one for the old ones. Test plan: Have several holds with several statuses. Notice that you see the holds in the respective tables. Test the usual sorting filtering DT feature and confirm that everything works as expected. Signed-off-by: Lucas Gass <lucas@bywatersolutions.com> Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org> Signed-off-by: David Nind <david@davidnind.com> Created attachment 177201 [details] [review] Bug 35560: Add filter on status Signed-off-by: Lucas Gass <lucas@bywatersolutions.com> Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org> Signed-off-by: David Nind <david@davidnind.com> Created attachment 177202 [details] [review] Bug 35560: (follow-up) Add in-page links to jump between tables Also a terminology change: I think "Past holds" reads better than "Historical holds." Signed-off-by: Lucas Gass <lucas@bywatersolutions.com> Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org> Signed-off-by: David Nind <david@davidnind.com> Created attachment 177203 [details]
Screenshots - Patron holds table before and after the patches
I've signed off, however I find the page much more "busy" now.
I've attached a PDF with before and after screenshots.
I don't have any suggestions right at the moment, but maybe we could ask Owen to have a look.
Adding Owen, in case he would like to provide some feedback on the change to the patron holds history page in the staff interface. I've attached a PDF with before and after screenshots. In my view it is now a bit more "busy", but I don't have any ideas right now on how to improve it. Created attachment 177792 [details] [review] Bug 35560: Use the REST API for holds This patch uses the new REST API endpoint to retrieve the old holds. We can now have 2 tables, one for the current holds and one for the old ones. Test plan: Have several holds with several statuses. Notice that you see the holds in the respective tables. Test the usual sorting filtering DT feature and confirm that everything works as expected. Signed-off-by: Lucas Gass <lucas@bywatersolutions.com> Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org> Signed-off-by: David Nind <david@davidnind.com> Created attachment 177793 [details] [review] Bug 35560: Add filter on status Signed-off-by: Lucas Gass <lucas@bywatersolutions.com> Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org> Signed-off-by: David Nind <david@davidnind.com> Created attachment 177794 [details] [review] Bug 35560: (follow-up) Add in-page links to jump between tables Also a terminology change: I think "Past holds" reads better than "Historical holds." Signed-off-by: Lucas Gass <lucas@bywatersolutions.com> Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org> Signed-off-by: David Nind <david@davidnind.com> Created attachment 178897 [details] [review] Bug 35560: Use the REST API for holds This patch uses the new REST API endpoint to retrieve the old holds. We can now have 2 tables, one for the current holds and one for the old ones. Test plan: Have several holds with several statuses. Notice that you see the holds in the respective tables. Test the usual sorting filtering DT feature and confirm that everything works as expected. Signed-off-by: Lucas Gass <lucas@bywatersolutions.com> Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org> Signed-off-by: David Nind <david@davidnind.com> Created attachment 178898 [details] [review] Bug 35560: Add filter on status Signed-off-by: Lucas Gass <lucas@bywatersolutions.com> Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org> Signed-off-by: David Nind <david@davidnind.com> Created attachment 178899 [details] [review] Bug 35560: (follow-up) Add in-page links to jump between tables Also a terminology change: I think "Past holds" reads better than "Historical holds." Signed-off-by: Lucas Gass <lucas@bywatersolutions.com> Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org> Signed-off-by: David Nind <david@davidnind.com> Created attachment 178910 [details] [review] Bug 35560: Use the REST API for holds This patch uses the new REST API endpoint to retrieve the old holds. We can now have 2 tables, one for the current holds and one for the old ones. Test plan: Have several holds with several statuses. Notice that you see the holds in the respective tables. Test the usual sorting filtering DT feature and confirm that everything works as expected. Signed-off-by: Lucas Gass <lucas@bywatersolutions.com> Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org> Signed-off-by: David Nind <david@davidnind.com> Signed-off-by: Owen Leonard <oleonard@myacpl.org> Created attachment 178911 [details] [review] Bug 35560: Add filter on status Signed-off-by: Lucas Gass <lucas@bywatersolutions.com> Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org> Signed-off-by: David Nind <david@davidnind.com> Signed-off-by: Owen Leonard <oleonard@myacpl.org> Created attachment 178912 [details] [review] Bug 35560: (follow-up) Add in-page links to jump between tables Also a terminology change: I think "Past holds" reads better than "Historical holds." Signed-off-by: Lucas Gass <lucas@bywatersolutions.com> Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org> Signed-off-by: David Nind <david@davidnind.com> Signed-off-by: Owen Leonard <oleonard@myacpl.org> Created attachment 178913 [details] [review] Bug 35560: (follow-up) Changes to classes and ids; BS5 follow-ups This follow-up changes the "selected" class to "select_hold_status" to avoid possible future collisions with other CSS. IDs are added to the checkbox containers because I can already tell that some libraries will want to hide the "Processing" checkbox which is only relevant to some setups. The patch also makes a couple corrections to "alerts" to update them with BS5 classes. Since we now split in two tables, does it still make sense to fetch and display empty cancellation date in hold history ? Also if we have change to "past holds", we also should change "This patron has no historical holds." (In reply to Baptiste Wojtkowski (bwoj) from comment #47) > Since we now split in two tables, does it still make sense to fetch and > display empty cancellation date in hold history ? Maybe, but it would be nice to have it pushed as it, then we iterate later. It has been waiting for a long time already. Now the 2 tables are identical, and so it's not trivial (neither impossible) to fix that. What do you think? I can work on that as soon as it's pushed. > Also if we have change to "past holds", we also should change "This patron > has no historical holds." Done. Created attachment 179552 [details] [review] Bug 35560: historical => past Created attachment 179573 [details] [review] Bug 35560: Use the REST API for holds This patch uses the new REST API endpoint to retrieve the old holds. We can now have 2 tables, one for the current holds and one for the old ones. Test plan: Have several holds with several statuses. Notice that you see the holds in the respective tables. Test the usual sorting filtering DT feature and confirm that everything works as expected. Signed-off-by: Lucas Gass <lucas@bywatersolutions.com> Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org> Signed-off-by: David Nind <david@davidnind.com> Signed-off-by: Owen Leonard <oleonard@myacpl.org> Signed-off-by: Baptiste Wojtkowski <baptiste.wojtkowski@biblibre.com> Created attachment 179574 [details] [review] Bug 35560: Add filter on status Signed-off-by: Lucas Gass <lucas@bywatersolutions.com> Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org> Signed-off-by: David Nind <david@davidnind.com> Signed-off-by: Owen Leonard <oleonard@myacpl.org> Signed-off-by: Baptiste Wojtkowski <baptiste.wojtkowski@biblibre.com> Created attachment 179575 [details] [review] Bug 35560: (follow-up) Add in-page links to jump between tables Also a terminology change: I think "Past holds" reads better than "Historical holds." Signed-off-by: Lucas Gass <lucas@bywatersolutions.com> Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org> Signed-off-by: David Nind <david@davidnind.com> Signed-off-by: Owen Leonard <oleonard@myacpl.org> Signed-off-by: Baptiste Wojtkowski <baptiste.wojtkowski@biblibre.com> Created attachment 179576 [details] [review] Bug 35560: (follow-up) Changes to classes and ids; BS5 follow-ups This follow-up changes the "selected" class to "select_hold_status" to avoid possible future collisions with other CSS. IDs are added to the checkbox containers because I can already tell that some libraries will want to hide the "Processing" checkbox which is only relevant to some setups. The patch also makes a couple corrections to "alerts" to update them with BS5 classes. Signed-off-by: Baptiste Wojtkowski <baptiste.wojtkowski@biblibre.com> Created attachment 179577 [details] [review] Bug 35560: historical => past Signed-off-by: Baptiste Wojtkowski <baptiste.wojtkowski@biblibre.com> I would like to see 2 tabs instead of tables on top of each other, as that might mean a lot of scrolling and a "messier" page. Tabs would also follow an existing interface pattern. What do you think? (I can file a bug later) Pushed for 25.05! Well done everyone, thank you! (In reply to Katrin Fischer from comment #55) > I would like to see 2 tabs instead of tables on top of each other, as that > might mean a lot of scrolling and a "messier" page. Tabs would also follow > an existing interface pattern. What do you think? (I can file a bug later) I personally don't like tab, but I guess we need to wait for feedback from librarians? (In reply to Jonathan Druart from comment #57) > (In reply to Katrin Fischer from comment #55) > > I would like to see 2 tabs instead of tables on top of each other, as that > > might mean a lot of scrolling and a "messier" page. Tabs would also follow > > an existing interface pattern. What do you think? (I can file a bug later) > > I personally don't like tab, but I guess we need to wait for feedback from > librarians? Yes, I was leaving the comment in hope to get some opinions :) Nice work everyone! Pushed to 24.11.x for 24.11.05 |