Bug 40517

Summary: Allow grouping existing holds
Product: Koha Reporter: Pedro Amorim <pedro.amorim>
Component: Hold requestsAssignee: Pedro Amorim <pedro.amorim>
Status: Signed Off --- QA Contact: Testopia <testopia>
Severity: enhancement    
Priority: P5 - low CC: andrew, anneli.osterman, chloe.zermatten, dcook, gmcharlt, kebliss, lisette, martin.renvoize, pedro.amorim, tomascohen
Version: Main   
Hardware: All   
OS: All   
URL: https://github.com/openfifth/koha/commits/group_existing_holds/
GIT URL: Change sponsored?: ---
Patch complexity: --- Documentation contact:
Documentation submission: Text to go in the release notes:
Version(s) released in:
Circulation function:
Bug Depends on: 15516, 40395    
Bug Blocks: 40529, 40552    
Attachments: Bug 40517: Add hold groups relationship to patron
Bug 40517: Move creation of hold group into Patron class. Add exceptions
Bug 40517: REST API: Hold groups endpoint
Bug 40517: Add visual_group_id to hold_groups
Bug 40517: Patron holds table: Add 'group selected holds' functionality
Bug 40517: Patron page holds table buttons rearrangement
Bug 40517: Add unit tests
Bug 40517: (QA follow-up): Cleanup obsolete code
Bug 40517: Add hold groups relationship to patron
Bug 40517: Move creation of hold group into Patron class. Add exceptions
Bug 40517: REST API: Hold groups endpoint
Bug 40517: Add visual_group_id to hold_groups
Bug 40517: Patron holds table: Add 'group selected holds' functionality
Bug 40517: Patron page holds table buttons rearrangement
Bug 40517: Add unit tests
Bug 40517: (QA follow-up): Cleanup obsolete code
Bug 40517: (QA follow-up): Fix 'group selected' button active with just 1 hold selected

Description Pedro Amorim 2025-07-25 14:23:24 UTC

    
Comment 1 Pedro Amorim 2025-07-25 14:38:28 UTC
Created attachment 184640 [details] [review]
Bug 40517: Add hold groups relationship to patron
Comment 2 Pedro Amorim 2025-07-25 14:38:31 UTC
Created attachment 184641 [details] [review]
Bug 40517: Move creation of hold group into Patron class. Add exceptions

Refactor: Update controller logic to make use of patron->create_hold_group

This ensures consistency when creating a new hold group for a patron,
regardless if on the Staff UI or REST API
Comment 3 Pedro Amorim 2025-07-25 14:38:34 UTC
Created attachment 184642 [details] [review]
Bug 40517: REST API: Hold groups endpoint

list and add actions:
/patrons/{patron_id}/hold_groups

New test file:
prove t/db_dependent/api/v1/patrons_hold_groups.t
Comment 4 Pedro Amorim 2025-07-25 14:38:36 UTC
Created attachment 184643 [details] [review]
Bug 40517: Add visual_group_id to hold_groups
Comment 5 Pedro Amorim 2025-07-25 14:38:40 UTC
Created attachment 184644 [details] [review]
Bug 40517: Patron holds table: Add 'group selected holds' functionality

This adds the UI elements required to group selected holds from the
patron pages as well as 2 new concepts:

Visual group ID:
The visual group ID is designed to be easy to understand from a UI/UX
perspective and not show the user an indefinitely incremented value.
When holds are cancelled, filled or moved to a different group, if the
old group is left empty or with a single hold, the hold group is deleted.
Comment 6 Pedro Amorim 2025-07-25 14:38:43 UTC
Created attachment 184645 [details] [review]
Bug 40517: Patron page holds table buttons rearrangement

This table now has 4 total buttons, 3 of which relate to a selection of
holds. Rearranging the buttons is now necessary to present them nicely.
Comment 7 Pedro Amorim 2025-07-25 14:38:45 UTC
Created attachment 184646 [details] [review]
Bug 40517: Add unit tests

Run the updated test file:
prove ./t/db_dependent/Koha/Patron.t

Relevant tests worth running to ensure no regressions are added to existing covered functionality:
prove ./t/db_dependent/Circulation*
prove ./t/db_dependent/Hold*
prove ./t/db_dependent/Reserves*

Test plan, k-t-d:
1) Apply patches, enable DisplayAddHoldGroups system preference
2) Do a search for 'test':
<staff_url>/cgi-bin/koha/catalogue/search.pl?q=test
3) Click the 'Select all' link at the top of the search results, click the 'Place hold' button.
4) Enter the 'koha' user
5) Pick a pickup location for each of the holds and click 'Place holds'.
6) Visit the 'koha' patron:
<staff_url>/cgi-bin/koha/members/moremember.pl?borrowernumber=51
7) Click the 'holds' tab. Notice a new 'Group selected' button now exists
8) Pick any number of holds (more than 2) and click the 'Group selected' button. Confirm
9) Notice the hold group shows '1'. If you click on it you're presented the hold group modal.
10) Group multiple holds multiple times.
11) Notice you can't group a hold that has already been found.
12) Notice you can't click the 'Group selected' button unless at least 2 holds are selected.
13) Notice a hold group is deleted if left with 0 or 1 hold e.g. make a group of 2 holds, cancel 1, notice the remaining hold is left without a group.

prove t/db_dependent/api/v1/patrons_hold_groups.t
prove t/db_dependent/Koha/Patron.t
Comment 8 David Cook 2025-07-28 06:51:43 UTC
Comment on attachment 184641 [details] [review]
Bug 40517: Move creation of hold group into Patron class. Add exceptions

Review of attachment 184641 [details] [review]:
-----------------------------------------------------------------

::: reserve/placerequest.pl
@@ +140,1 @@
>                          hold_group_id    => $hold_group ? $hold_group->id : undef,

I haven't tried yet but guessing this line needs to be removed?

@@ +165,1 @@
>                              hold_group_id    => $hold_group ? $hold_group->id : undef,

I haven't tried yet but guessing this line needs to be removed?
Comment 9 David Cook 2025-07-28 06:53:22 UTC
Apologies for my delay on this one. I've run out of time today for a meaningful review, but I will take a look at this one and bug 40395.
Comment 10 Pedro Amorim 2025-07-28 08:20:10 UTC
(In reply to David Cook from comment #8)
> Comment on attachment 184641 [details] [review] [review]
> Bug 40517: Move creation of hold group into Patron class. Add exceptions
> 
> Review of attachment 184641 [details] [review] [review]:
> -----------------------------------------------------------------
> 
> ::: reserve/placerequest.pl
> @@ +140,1 @@
> >                          hold_group_id    => $hold_group ? $hold_group->id : undef,
> 
> I haven't tried yet but guessing this line needs to be removed?
> 
> @@ +165,1 @@
> >                              hold_group_id    => $hold_group ? $hold_group->id : undef,
> 
> I haven't tried yet but guessing this line needs to be removed?

Good spot, yes most definitely an oversight, these should be removed. I'll correct.
Comment 11 Pedro Amorim 2025-07-28 11:13:07 UTC
Created attachment 184729 [details] [review]
Bug 40517: (QA follow-up): Cleanup obsolete code

The patch 'Move creation of hold group into Patron class' is
moving the creation of the hold group logic to the Patron.pm class file
but it didn't remove all the obsolete code.
This is fixed here.
Comment 12 Biblibre Sandboxes 2025-07-28 11:54:37 UTC
Created attachment 184732 [details] [review]
Bug 40517: Add hold groups relationship to patron

Signed-off-by: Anneli <anneli.osterman@koha-suomi.fi>
Comment 13 Biblibre Sandboxes 2025-07-28 11:54:39 UTC
Created attachment 184733 [details] [review]
Bug 40517: Move creation of hold group into Patron class. Add exceptions

Refactor: Update controller logic to make use of patron->create_hold_group

This ensures consistency when creating a new hold group for a patron,
regardless if on the Staff UI or REST API

Signed-off-by: Anneli <anneli.osterman@koha-suomi.fi>
Comment 14 Biblibre Sandboxes 2025-07-28 11:54:41 UTC
Created attachment 184734 [details] [review]
Bug 40517: REST API: Hold groups endpoint

list and add actions:
/patrons/{patron_id}/hold_groups

New test file:
prove t/db_dependent/api/v1/patrons_hold_groups.t

Signed-off-by: Anneli <anneli.osterman@koha-suomi.fi>
Comment 15 Biblibre Sandboxes 2025-07-28 11:54:44 UTC
Created attachment 184735 [details] [review]
Bug 40517: Add visual_group_id to hold_groups

Signed-off-by: Anneli <anneli.osterman@koha-suomi.fi>
Comment 16 Biblibre Sandboxes 2025-07-28 11:54:46 UTC
Created attachment 184736 [details] [review]
Bug 40517: Patron holds table: Add 'group selected holds' functionality

This adds the UI elements required to group selected holds from the
patron pages as well as 2 new concepts:

Visual group ID:
The visual group ID is designed to be easy to understand from a UI/UX
perspective and not show the user an indefinitely incremented value.
When holds are cancelled, filled or moved to a different group, if the
old group is left empty or with a single hold, the hold group is deleted.

Signed-off-by: Anneli <anneli.osterman@koha-suomi.fi>
Comment 17 Biblibre Sandboxes 2025-07-28 11:54:48 UTC
Created attachment 184737 [details] [review]
Bug 40517: Patron page holds table buttons rearrangement

This table now has 4 total buttons, 3 of which relate to a selection of
holds. Rearranging the buttons is now necessary to present them nicely.

Signed-off-by: Anneli <anneli.osterman@koha-suomi.fi>
Comment 18 Biblibre Sandboxes 2025-07-28 11:54:50 UTC
Created attachment 184738 [details] [review]
Bug 40517: Add unit tests

Run the updated test file:
prove ./t/db_dependent/Koha/Patron.t

Relevant tests worth running to ensure no regressions are added to existing covered functionality:
prove ./t/db_dependent/Circulation*
prove ./t/db_dependent/Hold*
prove ./t/db_dependent/Reserves*

Test plan, k-t-d:
1) Apply patches, enable DisplayAddHoldGroups system preference
2) Do a search for 'test':
<staff_url>/cgi-bin/koha/catalogue/search.pl?q=test
3) Click the 'Select all' link at the top of the search results, click the 'Place hold' button.
4) Enter the 'koha' user
5) Pick a pickup location for each of the holds and click 'Place holds'.
6) Visit the 'koha' patron:
<staff_url>/cgi-bin/koha/members/moremember.pl?borrowernumber=51
7) Click the 'holds' tab. Notice a new 'Group selected' button now exists
8) Pick any number of holds (more than 2) and click the 'Group selected' button. Confirm
9) Notice the hold group shows '1'. If you click on it you're presented the hold group modal.
10) Group multiple holds multiple times.
11) Notice you can't group a hold that has already been found.
12) Notice you can't click the 'Group selected' button unless at least 2 holds are selected.
13) Notice a hold group is deleted if left with 0 or 1 hold e.g. make a group of 2 holds, cancel 1, notice the remaining hold is left without a group.

prove t/db_dependent/api/v1/patrons_hold_groups.t
prove t/db_dependent/Koha/Patron.t

Signed-off-by: Anneli <anneli.osterman@koha-suomi.fi>
Comment 19 Biblibre Sandboxes 2025-07-28 11:54:53 UTC
Created attachment 184739 [details] [review]
Bug 40517: (QA follow-up): Cleanup obsolete code

The patch 'Move creation of hold group into Patron class' is
moving the creation of the hold group logic to the Patron.pm class file
but it didn't remove all the obsolete code.
This is fixed here.

Signed-off-by: Anneli <anneli.osterman@koha-suomi.fi>
Comment 20 Pedro Amorim 2025-08-07 10:56:12 UTC
Created attachment 185217 [details] [review]
Bug 40517: (QA follow-up): Fix 'group selected' button active with just 1 hold selected

This fixes the following bug:
1) Select 1 hold, notice the 'group selected' button is still disabled
2) Select a 2nd hold, notice the 'group selected' button becomes enabled
3) Unselect one of the holds, notice the 'group select' is kept enabled (this is wrong, it should become disabled)