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. |