Bug 35560 - Use the REST API for holds history
Summary: Use the REST API for holds history
Status: Needs Signoff
Alias: None
Product: Koha
Classification: Unclassified
Component: Hold requests (show other bugs)
Version: Main
Hardware: All All
: P5 - low enhancement (vote)
Assignee: Jonathan Druart
QA Contact: Testopia
URL:
Keywords:
Depends on: 35353
Blocks: 34832
  Show dependency treegraph
 
Reported: 2023-12-13 14:58 UTC by Jonathan Druart
Modified: 2024-04-22 13:17 UTC (History)
8 users (show)

See Also:
Change sponsored?: ---
Patch complexity: ---
Documentation contact:
Documentation submission:
Text to go in the release notes:
Version(s) released in:


Attachments
Bug 35560: [POC] Make the holds list use the REST API (18.93 KB, patch)
2023-12-20 10:27 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 35560: DBIC specific (1.46 KB, patch)
2023-12-20 10:27 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 35560: Use the REST API for holds (15.04 KB, patch)
2024-03-25 16:26 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 35560: Add filter on status (7.29 KB, patch)
2024-03-25 16:26 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 35560: (follow-up) Add in-page links to jump between tables (1.71 KB, patch)
2024-03-28 17:22 UTC, Owen Leonard
Details | Diff | Splinter Review
Bug 35560: Use the REST API for holds (15.09 KB, patch)
2024-04-19 22:31 UTC, Lucas Gass
Details | Diff | Splinter Review
Bug 35560: Add filter on status (7.34 KB, patch)
2024-04-19 22:31 UTC, Lucas Gass
Details | Diff | Splinter Review
Bug 35560: (follow-up) Add in-page links to jump between tables (1.76 KB, patch)
2024-04-19 22:31 UTC, Lucas Gass
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
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?