Bug 35560

Summary: Use the REST API for holds history
Product: Koha Reporter: Jonathan Druart <jonathan.druart>
Component: Hold requestsAssignee: Jonathan Druart <jonathan.druart>
Status: Needs Signoff --- QA Contact: Testopia <testopia>
Severity: enhancement    
Priority: P5 - low CC: didier.gautheron, gmcharlt, kyle, lucas, martin.renvoize, nick, tomascohen
Version: Main   
Hardware: All   
OS: All   
See Also: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=36633
Change sponsored?: --- Patch complexity: ---
Documentation contact: Documentation submission:
Text to go in the release notes:
Version(s) released in:
Circulation function:
Bug Depends on: 35353    
Bug Blocks: 34832    
Attachments: Bug 35560: [POC] Make the holds list use the REST API
Bug 35560: DBIC specific
Bug 35560: Use the REST API for holds
Bug 35560: Add filter on status
Bug 35560: (follow-up) Add in-page links to jump between tables
Bug 35560: Use the REST API for holds
Bug 35560: Add filter on status
Bug 35560: (follow-up) Add in-page links to jump between tables

Description Jonathan Druart 2023-12-13 14:58:32 UTC
On members/holdshistory.pl
Comment 1 Jonathan Druart 2023-12-13 14:59:19 UTC
Would it be conceivable to have 2 (UI) tables, one for current holds and one for old ones?
Comment 2 Jonathan Druart 2023-12-15 08:14:56 UTC
Katrin, Nick, do you have opinions on this?
Comment 3 Nick Clemens (kidclamp) 2023-12-15 18:47:30 UTC
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
Comment 4 Katrin Fischer 2023-12-16 13:37:17 UTC
I think separating into current and old makes sense from a usability point of view.
Comment 5 Jonathan Druart 2023-12-20 10:27:32 UTC
Created attachment 160100 [details] [review]
Bug 35560: [POC] Make the holds list use the REST API
Comment 6 Jonathan Druart 2023-12-20 10:27:34 UTC
Created attachment 160101 [details] [review]
Bug 35560: DBIC specific
Comment 7 Jonathan Druart 2023-12-20 10:28:32 UTC
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)
Comment 8 Tomás Cohen Arazi 2023-12-20 12:46:32 UTC
I'm in.
Comment 9 Didier Gautheron 2023-12-20 13:27:36 UTC
I'm in
Comment 10 Martin Renvoize 2024-01-26 09:54:47 UTC
*** Bug 35353 has been marked as a duplicate of this bug. ***
Comment 11 Martin Renvoize 2024-01-26 09:58:15 UTC
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
Comment 12 Kyle M Hall 2024-01-26 14:21:27 UTC
(In reply to Tomás Cohen Arazi from comment #8)
> I'm in.

And my axe!
Comment 13 Kyle M Hall 2024-01-26 14:31:07 UTC
(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.
Comment 14 Tomás Cohen Arazi 2024-01-26 19:06:52 UTC
(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!
Comment 15 Jonathan Druart 2024-03-25 16:26:52 UTC
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.
Comment 16 Jonathan Druart 2024-03-25 16:26:55 UTC
Created attachment 163832 [details] [review]
Bug 35560: Add filter on status
Comment 17 Jonathan Druart 2024-03-25 16:28:02 UTC
(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.
Comment 18 Owen Leonard 2024-03-28 17:22:51 UTC
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."
Comment 19 Owen Leonard 2024-03-28 17:24:13 UTC
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.
Comment 20 Jonathan Druart 2024-04-18 06:39:48 UTC
(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.
Comment 21 Jonathan Druart 2024-04-18 06:44:30 UTC
Opened bug 36633.
Comment 22 Owen Leonard 2024-04-18 12:28:54 UTC
(In reply to Jonathan Druart from comment #20)

> That's is something global

Does that mean we can't use any custom sorting plugins?
Comment 23 Lucas Gass 2024-04-19 22:31:24 UTC
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>
Comment 24 Lucas Gass 2024-04-19 22:31:26 UTC
Created attachment 165232 [details] [review]
Bug 35560: Add filter on status

Signed-off-by: Lucas Gass <lucas@bywatersolutions.com>
Comment 25 Lucas Gass 2024-04-19 22:31:29 UTC
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>
Comment 26 Lucas Gass 2024-04-19 22:33:08 UTC
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.
Comment 27 Jonathan Druart 2024-04-22 13:17:53 UTC
(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?