Bug 35560 - Use the REST API for holds history
Summary: Use the REST API for holds history
Status: Signed Off
Alias: None
Product: Koha
Classification: Unclassified
Component: Hold requests (show other bugs)
Version: Main
Hardware: All All
: P5 - low enhancement
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: 2025-01-27 21:03 UTC (History)
9 users (show)

See Also:
Change sponsored?: ---
Patch complexity: ---
Documentation contact:
Documentation submission:
Text to go in the release notes:
This enhancement to a patron's hold history section in the staff interface: - Uses the REST API to generate the holds history page. - Separates the holds history into two tables: "Current holds" and "Past holds". - Adds filters for each table, such as Show all, Pending, Waiting, Fulfilled.
Version(s) released in:
Circulation function:


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 (lukeg)
Details | Diff | Splinter Review
Bug 35560: Add filter on status (7.34 KB, patch)
2024-04-19 22:31 UTC, Lucas Gass (lukeg)
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 (lukeg)
Details | Diff | Splinter Review
Bug 35560: Use the REST API for holds (15.18 KB, patch)
2025-01-27 09:01 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 35560: Add filter on status (7.42 KB, patch)
2025-01-27 09:01 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 35560: (follow-up) Add in-page links to jump between tables (1.84 KB, patch)
2025-01-27 09:01 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 35560: Use the REST API for holds (15.23 KB, patch)
2025-01-27 20:28 UTC, David Nind
Details | Diff | Splinter Review
Bug 35560: Add filter on status (7.46 KB, patch)
2025-01-27 20:28 UTC, David Nind
Details | Diff | Splinter Review
Bug 35560: (follow-up) Add in-page links to jump between tables (1.88 KB, patch)
2025-01-27 20:29 UTC, David Nind
Details | Diff | Splinter Review
Screenshots - Patron holds table before and after the patches (168.12 KB, application/pdf)
2025-01-27 20:58 UTC, David Nind
Details

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 (tcohen) 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 (ashimema) 2024-01-26 09:54:47 UTC
*** Bug 35353 has been marked as a duplicate of this bug. ***
Comment 11 Martin Renvoize (ashimema) 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 (khall) 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 (khall) 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 (tcohen) 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 (lukeg) 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 (lukeg) 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 (lukeg) 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 (lukeg) 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?
Comment 28 David Nind 2025-01-26 20:33:23 UTC
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
Comment 29 Jonathan Druart 2025-01-27 09:01:32 UTC
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>
Comment 30 Jonathan Druart 2025-01-27 09:01:35 UTC
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>
Comment 31 Jonathan Druart 2025-01-27 09:01:39 UTC
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>
Comment 32 David Nind 2025-01-27 20:28:54 UTC
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>
Comment 33 David Nind 2025-01-27 20:28:57 UTC
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>
Comment 34 David Nind 2025-01-27 20:29:00 UTC
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>
Comment 35 David Nind 2025-01-27 20:58:38 UTC
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.
Comment 36 David Nind 2025-01-27 21:03:37 UTC
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.