Bug 40517 - Allow grouping existing holds
Summary: Allow grouping existing holds
Status: Pushed to main
Alias: None
Product: Koha
Classification: Unclassified
Component: Hold requests (show other bugs)
Version: Main
Hardware: All All
: P5 - low enhancement
Assignee: Pedro Amorim
QA Contact: Andrew Fuerste-Henry
URL: https://github.com/openfifth/koha/com...
Keywords:
Depends on: 15516 40395
Blocks: 40529 40552
  Show dependency treegraph
 
Reported: 2025-07-25 14:23 UTC by Pedro Amorim
Modified: 2025-10-29 17:53 UTC (History)
11 users (show)

See Also:
GIT URL:
Initiative type: ---
Sponsorship status: ---
Crowdfunding goal: 0
Patch complexity: ---
Documentation contact:
Documentation submission:
Text to go in the release notes:
Staff can now group existing patron holds together directly from the patron holds table, in addition to creating hold groups when placing new holds. Previously, hold groups could only be created when placing new holds (if the DisplayAddHoldGroups system preference was enabled). With this enhancement, staff can now select multiple existing holds from a patron's holds list and group them together using a new "Group selected" button. **Key features:** - **Group selected button**: Select two or more holds from the patron holds table and click "Group selected" to create a new hold group - **Visual group IDs**: Hold groups are displayed with easy-to-understand sequential numbers (1, 2, 3, etc.) rather than internal database IDs - **Automatic cleanup**: If a hold group is left with only one hold (or none) after cancellations or fulfilment, the group is automatically deleted - **Smart restrictions**: You cannot group holds that have already been found (waiting for pickup) - **Multiple groups**: Create as many different hold groups as needed for a patron **Using hold groups:** 1. Navigate to a patron's record and click the "Holds" tab 2. Select two or more holds using the checkboxes 3. Click the "Group selected" button and confirm 4. The holds are now grouped together with a visual group ID displayed in the holds table 5. Click on the group number to view or manage the hold group **For administrators:** This feature requires the DisplayAddHoldGroups system preference to be enabled. The new "Group selected" button appears alongside other hold management buttons on patron holds pages. **REST API:** A new API endpoint `/api/v1/patrons/{patron_id}/hold_groups` has been added to support listing and creating hold groups programmatically.
Version(s) released in:
25.11.00
Circulation function:


Attachments
Bug 40517: Add hold groups relationship to patron (6.49 KB, patch)
2025-07-25 14:38 UTC, Pedro Amorim
Details | Diff | Splinter Review
Bug 40517: Move creation of hold group into Patron class. Add exceptions (10.23 KB, patch)
2025-07-25 14:38 UTC, Pedro Amorim
Details | Diff | Splinter Review
Bug 40517: REST API: Hold groups endpoint (18.90 KB, patch)
2025-07-25 14:38 UTC, Pedro Amorim
Details | Diff | Splinter Review
Bug 40517: Add visual_group_id to hold_groups (9.14 KB, patch)
2025-07-25 14:38 UTC, Pedro Amorim
Details | Diff | Splinter Review
Bug 40517: Patron holds table: Add 'group selected holds' functionality (16.88 KB, patch)
2025-07-25 14:38 UTC, Pedro Amorim
Details | Diff | Splinter Review
Bug 40517: Patron page holds table buttons rearrangement (8.12 KB, patch)
2025-07-25 14:38 UTC, Pedro Amorim
Details | Diff | Splinter Review
Bug 40517: Add unit tests (5.64 KB, patch)
2025-07-25 14:38 UTC, Pedro Amorim
Details | Diff | Splinter Review
Bug 40517: (QA follow-up): Cleanup obsolete code (3.00 KB, patch)
2025-07-28 11:13 UTC, Pedro Amorim
Details | Diff | Splinter Review
Bug 40517: Add hold groups relationship to patron (6.54 KB, patch)
2025-07-28 11:54 UTC, Biblibre Sandboxes
Details | Diff | Splinter Review
Bug 40517: Move creation of hold group into Patron class. Add exceptions (10.28 KB, patch)
2025-07-28 11:54 UTC, Biblibre Sandboxes
Details | Diff | Splinter Review
Bug 40517: REST API: Hold groups endpoint (18.94 KB, patch)
2025-07-28 11:54 UTC, Biblibre Sandboxes
Details | Diff | Splinter Review
Bug 40517: Add visual_group_id to hold_groups (9.17 KB, patch)
2025-07-28 11:54 UTC, Biblibre Sandboxes
Details | Diff | Splinter Review
Bug 40517: Patron holds table: Add 'group selected holds' functionality (16.92 KB, patch)
2025-07-28 11:54 UTC, Biblibre Sandboxes
Details | Diff | Splinter Review
Bug 40517: Patron page holds table buttons rearrangement (8.17 KB, patch)
2025-07-28 11:54 UTC, Biblibre Sandboxes
Details | Diff | Splinter Review
Bug 40517: Add unit tests (5.70 KB, patch)
2025-07-28 11:54 UTC, Biblibre Sandboxes
Details | Diff | Splinter Review
Bug 40517: (QA follow-up): Cleanup obsolete code (3.05 KB, patch)
2025-07-28 11:54 UTC, Biblibre Sandboxes
Details | Diff | Splinter Review
Bug 40517: (QA follow-up): Fix 'group selected' button active with just 1 hold selected (2.16 KB, patch)
2025-08-07 10:56 UTC, Pedro Amorim
Details | Diff | Splinter Review
Bug 40517: Add hold groups relationship to patron (6.54 KB, patch)
2025-09-22 10:29 UTC, Pedro Amorim
Details | Diff | Splinter Review
Bug 40517: Move creation of hold group into Patron class. Add exceptions (10.29 KB, patch)
2025-09-22 10:29 UTC, Pedro Amorim
Details | Diff | Splinter Review
Bug 40517: REST API: Hold groups endpoint (18.95 KB, patch)
2025-09-22 10:30 UTC, Pedro Amorim
Details | Diff | Splinter Review
Bug 40517: Add visual_group_id to hold_groups (7.21 KB, patch)
2025-09-22 10:30 UTC, Pedro Amorim
Details | Diff | Splinter Review
Bug 40517: Patron holds table: Add 'group selected holds' functionality (16.92 KB, patch)
2025-09-22 10:30 UTC, Pedro Amorim
Details | Diff | Splinter Review
Bug 40517: Patron page holds table buttons rearrangement (8.30 KB, patch)
2025-09-22 10:30 UTC, Pedro Amorim
Details | Diff | Splinter Review
Bug 40517: Add unit tests (5.70 KB, patch)
2025-09-22 10:30 UTC, Pedro Amorim
Details | Diff | Splinter Review
Bug 40517: (QA follow-up): Cleanup obsolete code (3.06 KB, patch)
2025-09-22 10:30 UTC, Pedro Amorim
Details | Diff | Splinter Review
Bug 40517: (QA follow-up): Fix 'group selected' button active with just 1 hold selected (2.16 KB, patch)
2025-09-22 10:30 UTC, Pedro Amorim
Details | Diff | Splinter Review
Bug 40517: Add hold groups relationship to patron (6.61 KB, patch)
2025-09-22 20:05 UTC, Andrew Fuerste-Henry
Details | Diff | Splinter Review
Bug 40517: Move creation of hold group into Patron class. Add exceptions (10.35 KB, patch)
2025-09-22 20:05 UTC, Andrew Fuerste-Henry
Details | Diff | Splinter Review
Bug 40517: REST API: Hold groups endpoint (19.02 KB, patch)
2025-09-22 20:05 UTC, Andrew Fuerste-Henry
Details | Diff | Splinter Review
Bug 40517: Add visual_group_id to hold_groups (7.28 KB, patch)
2025-09-22 20:05 UTC, Andrew Fuerste-Henry
Details | Diff | Splinter Review
Bug 40517: Patron holds table: Add 'group selected holds' functionality (16.99 KB, patch)
2025-09-22 20:05 UTC, Andrew Fuerste-Henry
Details | Diff | Splinter Review
Bug 40517: Patron page holds table buttons rearrangement (8.37 KB, patch)
2025-09-22 20:05 UTC, Andrew Fuerste-Henry
Details | Diff | Splinter Review
Bug 40517: Add unit tests (5.76 KB, patch)
2025-09-22 20:05 UTC, Andrew Fuerste-Henry
Details | Diff | Splinter Review
Bug 40517: (QA follow-up): Cleanup obsolete code (3.12 KB, patch)
2025-09-22 20:05 UTC, Andrew Fuerste-Henry
Details | Diff | Splinter Review
Bug 40517: (QA follow-up): Fix 'group selected' button active with just 1 hold selected (2.22 KB, patch)
2025-09-22 20:05 UTC, Andrew Fuerste-Henry
Details | Diff | Splinter Review
Bug 40517: Add hold groups relationship to patron (6.61 KB, patch)
2025-10-13 08:34 UTC, Pedro Amorim
Details | Diff | Splinter Review
Bug 40517: Move creation of hold group into Patron class. Add exceptions (10.35 KB, patch)
2025-10-13 08:34 UTC, Pedro Amorim
Details | Diff | Splinter Review
Bug 40517: REST API: Hold groups endpoint (19.02 KB, patch)
2025-10-13 08:34 UTC, Pedro Amorim
Details | Diff | Splinter Review
Bug 40517: Add visual_group_id to hold_groups (7.11 KB, patch)
2025-10-13 08:34 UTC, Pedro Amorim
Details | Diff | Splinter Review
Bug 40517: Patron holds table: Add 'group selected holds' functionality (16.99 KB, patch)
2025-10-13 08:34 UTC, Pedro Amorim
Details | Diff | Splinter Review
Bug 40517: Patron page holds table buttons rearrangement (8.34 KB, patch)
2025-10-13 08:34 UTC, Pedro Amorim
Details | Diff | Splinter Review
Bug 40517: Add unit tests (5.76 KB, patch)
2025-10-13 08:34 UTC, Pedro Amorim
Details | Diff | Splinter Review
Bug 40517: (QA follow-up): Cleanup obsolete code (3.20 KB, patch)
2025-10-13 08:34 UTC, Pedro Amorim
Details | Diff | Splinter Review
Bug 40517: (QA follow-up): Fix 'group selected' button active with just 1 hold selected (2.22 KB, patch)
2025-10-13 08:34 UTC, Pedro Amorim
Details | Diff | Splinter Review
Bug 40517: dbic (3.05 KB, patch)
2025-10-13 08:43 UTC, Pedro Amorim
Details | Diff | Splinter Review
Bug 40517: DB changes (1.13 KB, patch)
2025-10-13 08:44 UTC, Pedro Amorim
Details | Diff | Splinter Review
Bug 40517: atomicupdate (1.86 KB, patch)
2025-10-13 08:44 UTC, Pedro Amorim
Details | Diff | Splinter Review
Bug 40517: Add hold groups relationship to patron (1.13 KB, patch)
2025-10-13 08:44 UTC, Pedro Amorim
Details | Diff | Splinter Review
Bug 40517: Move creation of hold group into Patron class. Add exceptions (10.35 KB, patch)
2025-10-13 08:44 UTC, Pedro Amorim
Details | Diff | Splinter Review
Bug 40517: REST API: Hold groups endpoint (19.02 KB, patch)
2025-10-13 08:44 UTC, Pedro Amorim
Details | Diff | Splinter Review
Bug 40517: Add visual_group_id to hold_groups (7.11 KB, patch)
2025-10-13 08:44 UTC, Pedro Amorim
Details | Diff | Splinter Review
Bug 40517: Patron holds table: Add 'group selected holds' functionality (16.99 KB, patch)
2025-10-13 08:44 UTC, Pedro Amorim
Details | Diff | Splinter Review
Bug 40517: Patron page holds table buttons rearrangement (8.34 KB, patch)
2025-10-13 08:44 UTC, Pedro Amorim
Details | Diff | Splinter Review
Bug 40517: Add unit tests (5.76 KB, patch)
2025-10-13 08:44 UTC, Pedro Amorim
Details | Diff | Splinter Review
Bug 40517: (QA follow-up): Cleanup obsolete code (3.20 KB, patch)
2025-10-13 08:44 UTC, Pedro Amorim
Details | Diff | Splinter Review
Bug 40517: (QA follow-up): Fix 'group selected' button active with just 1 hold selected (2.22 KB, patch)
2025-10-13 08:44 UTC, Pedro Amorim
Details | Diff | Splinter Review
Bug 40517: dbic (3.27 KB, patch)
2025-10-16 08:41 UTC, Pedro Amorim
Details | Diff | Splinter Review
Bug 40517: DB changes (1.13 KB, patch)
2025-10-16 08:41 UTC, Pedro Amorim
Details | Diff | Splinter Review
Bug 40517: atomicupdate (1.86 KB, patch)
2025-10-16 08:41 UTC, Pedro Amorim
Details | Diff | Splinter Review
Bug 40517: Add hold groups relationship to patron (1.13 KB, patch)
2025-10-16 08:41 UTC, Pedro Amorim
Details | Diff | Splinter Review
Bug 40517: Move creation of hold group into Patron class. Add exceptions (10.35 KB, patch)
2025-10-16 08:41 UTC, Pedro Amorim
Details | Diff | Splinter Review
Bug 40517: REST API: Hold groups endpoint (19.02 KB, patch)
2025-10-16 08:41 UTC, Pedro Amorim
Details | Diff | Splinter Review
Bug 40517: Add visual_group_id to hold_groups (4.68 KB, patch)
2025-10-16 08:41 UTC, Pedro Amorim
Details | Diff | Splinter Review
Bug 40517: Patron holds table: Add 'group selected holds' functionality (16.99 KB, patch)
2025-10-16 08:41 UTC, Pedro Amorim
Details | Diff | Splinter Review
Bug 40517: Patron page holds table buttons rearrangement (8.34 KB, patch)
2025-10-16 08:41 UTC, Pedro Amorim
Details | Diff | Splinter Review
Bug 40517: Add unit tests (5.77 KB, patch)
2025-10-16 08:41 UTC, Pedro Amorim
Details | Diff | Splinter Review
Bug 40517: (QA follow-up): Cleanup obsolete code (3.20 KB, patch)
2025-10-16 08:41 UTC, Pedro Amorim
Details | Diff | Splinter Review
Bug 40517: (QA follow-up): Fix 'group selected' button active with just 1 hold selected (2.22 KB, patch)
2025-10-16 08:41 UTC, Pedro Amorim
Details | Diff | Splinter Review
Bug 40517: (follow-up) Add a new line to EOF (798 bytes, patch)
2025-10-22 12:31 UTC, Lucas Gass (lukeg)
Details | Diff | Splinter Review
Bug 40517: (follow-up) Fix GNU license links (1.96 KB, patch)
2025-10-22 13:06 UTC, Lucas Gass (lukeg)
Details | Diff | Splinter Review
Bug 40517: (QA follow-up): Fix xt/api.t (1.41 KB, patch)
2025-10-23 11:43 UTC, Pedro Amorim
Details | Diff | Splinter Review
Bug 40517: (QA follow-up): Fix kohstructure.sql (1.42 KB, patch)
2025-10-24 17:06 UTC, Pedro Amorim
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
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)
Comment 21 Andrew Fuerste-Henry 2025-09-19 16:53:25 UTC
Pedro, can you please give this a rebase? I started QA but hit more merge errors than I was entirely comfortable sorting out on my own.
Comment 22 Pedro Amorim 2025-09-22 10:29:56 UTC
Created attachment 186672 [details] [review]
Bug 40517: Add hold groups relationship to patron

Signed-off-by: Anneli <anneli.osterman@koha-suomi.fi>
Comment 23 Pedro Amorim 2025-09-22 10:29:58 UTC
Created attachment 186673 [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 24 Pedro Amorim 2025-09-22 10:30:01 UTC
Created attachment 186674 [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 25 Pedro Amorim 2025-09-22 10:30:04 UTC
Created attachment 186675 [details] [review]
Bug 40517: Add visual_group_id to hold_groups

Signed-off-by: Anneli <anneli.osterman@koha-suomi.fi>
Comment 26 Pedro Amorim 2025-09-22 10:30:07 UTC
Created attachment 186676 [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 27 Pedro Amorim 2025-09-22 10:30:09 UTC
Created attachment 186677 [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 28 Pedro Amorim 2025-09-22 10:30:12 UTC
Created attachment 186678 [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 29 Pedro Amorim 2025-09-22 10:30:15 UTC
Created attachment 186679 [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 30 Pedro Amorim 2025-09-22 10:30:17 UTC
Created attachment 186680 [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)
Comment 31 Andrew Fuerste-Henry 2025-09-22 20:05:30 UTC
Created attachment 186745 [details] [review]
Bug 40517: Add hold groups relationship to patron

Signed-off-by: Anneli <anneli.osterman@koha-suomi.fi>
Signed-off-by: Andrew Fuerste Henry <andrew@bywatersolutions.com>
Comment 32 Andrew Fuerste-Henry 2025-09-22 20:05:33 UTC
Created attachment 186746 [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>
Signed-off-by: Andrew Fuerste Henry <andrew@bywatersolutions.com>
Comment 33 Andrew Fuerste-Henry 2025-09-22 20:05:36 UTC
Created attachment 186747 [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>
Signed-off-by: Andrew Fuerste Henry <andrew@bywatersolutions.com>
Comment 34 Andrew Fuerste-Henry 2025-09-22 20:05:39 UTC
Created attachment 186748 [details] [review]
Bug 40517: Add visual_group_id to hold_groups

Signed-off-by: Anneli <anneli.osterman@koha-suomi.fi>
Signed-off-by: Andrew Fuerste Henry <andrew@bywatersolutions.com>
Comment 35 Andrew Fuerste-Henry 2025-09-22 20:05:42 UTC
Created attachment 186749 [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>
Signed-off-by: Andrew Fuerste Henry <andrew@bywatersolutions.com>
Comment 36 Andrew Fuerste-Henry 2025-09-22 20:05:45 UTC
Created attachment 186750 [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>
Signed-off-by: Andrew Fuerste Henry <andrew@bywatersolutions.com>
Comment 37 Andrew Fuerste-Henry 2025-09-22 20:05:48 UTC
Created attachment 186751 [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>
Signed-off-by: Andrew Fuerste Henry <andrew@bywatersolutions.com>
Comment 38 Andrew Fuerste-Henry 2025-09-22 20:05:51 UTC
Created attachment 186752 [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>
Signed-off-by: Andrew Fuerste Henry <andrew@bywatersolutions.com>
Comment 39 Andrew Fuerste-Henry 2025-09-22 20:05:54 UTC
Created attachment 186753 [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)

Signed-off-by: Andrew Fuerste Henry <andrew@bywatersolutions.com>
Comment 40 Lucas Gass (lukeg) 2025-10-10 18:22:06 UTC
Can you separate the schema changes from the other database updates?
Comment 41 Pedro Amorim 2025-10-13 08:34:31 UTC
Created attachment 187789 [details] [review]
Bug 40517: Add hold groups relationship to patron

Signed-off-by: Anneli <anneli.osterman@koha-suomi.fi>
Signed-off-by: Andrew Fuerste Henry <andrew@bywatersolutions.com>
Comment 42 Pedro Amorim 2025-10-13 08:34:33 UTC
Created attachment 187790 [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>
Signed-off-by: Andrew Fuerste Henry <andrew@bywatersolutions.com>
Comment 43 Pedro Amorim 2025-10-13 08:34:36 UTC
Created attachment 187791 [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>
Signed-off-by: Andrew Fuerste Henry <andrew@bywatersolutions.com>
Comment 44 Pedro Amorim 2025-10-13 08:34:39 UTC
Created attachment 187792 [details] [review]
Bug 40517: Add visual_group_id to hold_groups

Signed-off-by: Anneli <anneli.osterman@koha-suomi.fi>
Signed-off-by: Andrew Fuerste Henry <andrew@bywatersolutions.com>
Comment 45 Pedro Amorim 2025-10-13 08:34:42 UTC
Created attachment 187793 [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>
Signed-off-by: Andrew Fuerste Henry <andrew@bywatersolutions.com>
Comment 46 Pedro Amorim 2025-10-13 08:34:44 UTC
Created attachment 187794 [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>
Signed-off-by: Andrew Fuerste Henry <andrew@bywatersolutions.com>
Comment 47 Pedro Amorim 2025-10-13 08:34:47 UTC
Created attachment 187795 [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>
Signed-off-by: Andrew Fuerste Henry <andrew@bywatersolutions.com>
Comment 48 Pedro Amorim 2025-10-13 08:34:50 UTC
Created attachment 187796 [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>
Signed-off-by: Andrew Fuerste Henry <andrew@bywatersolutions.com>
Comment 49 Pedro Amorim 2025-10-13 08:34:53 UTC
Created attachment 187797 [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)

Signed-off-by: Andrew Fuerste Henry <andrew@bywatersolutions.com>
Comment 50 Pedro Amorim 2025-10-13 08:43:57 UTC
Created attachment 187801 [details] [review]
Bug 40517: dbic
Comment 51 Pedro Amorim 2025-10-13 08:44:01 UTC
Created attachment 187802 [details] [review]
Bug 40517: DB changes
Comment 52 Pedro Amorim 2025-10-13 08:44:05 UTC
Created attachment 187803 [details] [review]
Bug 40517: atomicupdate
Comment 53 Pedro Amorim 2025-10-13 08:44:08 UTC
Created attachment 187804 [details] [review]
Bug 40517: Add hold groups relationship to patron
Comment 54 Pedro Amorim 2025-10-13 08:44:12 UTC
Created attachment 187805 [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>
Signed-off-by: Andrew Fuerste Henry <andrew@bywatersolutions.com>
Comment 55 Pedro Amorim 2025-10-13 08:44:15 UTC
Created attachment 187806 [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>
Signed-off-by: Andrew Fuerste Henry <andrew@bywatersolutions.com>
Comment 56 Pedro Amorim 2025-10-13 08:44:19 UTC
Created attachment 187807 [details] [review]
Bug 40517: Add visual_group_id to hold_groups

Signed-off-by: Anneli <anneli.osterman@koha-suomi.fi>
Signed-off-by: Andrew Fuerste Henry <andrew@bywatersolutions.com>
Comment 57 Pedro Amorim 2025-10-13 08:44:23 UTC
Created attachment 187808 [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>
Signed-off-by: Andrew Fuerste Henry <andrew@bywatersolutions.com>
Comment 58 Pedro Amorim 2025-10-13 08:44:26 UTC
Created attachment 187809 [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>
Signed-off-by: Andrew Fuerste Henry <andrew@bywatersolutions.com>
Comment 59 Pedro Amorim 2025-10-13 08:44:29 UTC
Created attachment 187810 [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>
Signed-off-by: Andrew Fuerste Henry <andrew@bywatersolutions.com>
Comment 60 Pedro Amorim 2025-10-13 08:44:33 UTC
Created attachment 187811 [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>
Signed-off-by: Andrew Fuerste Henry <andrew@bywatersolutions.com>
Comment 61 Pedro Amorim 2025-10-13 08:44:36 UTC
Created attachment 187812 [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)

Signed-off-by: Andrew Fuerste Henry <andrew@bywatersolutions.com>
Comment 62 Pedro Amorim 2025-10-16 08:41:25 UTC
Created attachment 187965 [details] [review]
Bug 40517: dbic
Comment 63 Pedro Amorim 2025-10-16 08:41:28 UTC
Created attachment 187966 [details] [review]
Bug 40517: DB changes
Comment 64 Pedro Amorim 2025-10-16 08:41:31 UTC
Created attachment 187967 [details] [review]
Bug 40517: atomicupdate
Comment 65 Pedro Amorim 2025-10-16 08:41:34 UTC
Created attachment 187968 [details] [review]
Bug 40517: Add hold groups relationship to patron
Comment 66 Pedro Amorim 2025-10-16 08:41:37 UTC
Created attachment 187969 [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>
Signed-off-by: Andrew Fuerste Henry <andrew@bywatersolutions.com>
Comment 67 Pedro Amorim 2025-10-16 08:41:40 UTC
Created attachment 187970 [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>
Signed-off-by: Andrew Fuerste Henry <andrew@bywatersolutions.com>
Comment 68 Pedro Amorim 2025-10-16 08:41:43 UTC
Created attachment 187971 [details] [review]
Bug 40517: Add visual_group_id to hold_groups

Signed-off-by: Anneli <anneli.osterman@koha-suomi.fi>
Signed-off-by: Andrew Fuerste Henry <andrew@bywatersolutions.com>
Comment 69 Pedro Amorim 2025-10-16 08:41:46 UTC
Created attachment 187972 [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>
Signed-off-by: Andrew Fuerste Henry <andrew@bywatersolutions.com>
Comment 70 Pedro Amorim 2025-10-16 08:41:49 UTC
Created attachment 187973 [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>
Signed-off-by: Andrew Fuerste Henry <andrew@bywatersolutions.com>
Comment 71 Pedro Amorim 2025-10-16 08:41:52 UTC
Created attachment 187974 [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>
Signed-off-by: Andrew Fuerste Henry <andrew@bywatersolutions.com>
Comment 72 Pedro Amorim 2025-10-16 08:41:55 UTC
Created attachment 187975 [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>
Signed-off-by: Andrew Fuerste Henry <andrew@bywatersolutions.com>
Comment 73 Pedro Amorim 2025-10-16 08:41:58 UTC
Created attachment 187976 [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)

Signed-off-by: Andrew Fuerste Henry <andrew@bywatersolutions.com>
Comment 74 Lucas Gass (lukeg) 2025-10-22 12:31:52 UTC
Created attachment 188293 [details] [review]
Bug 40517: (follow-up) Add a new line to EOF
Comment 75 Lucas Gass (lukeg) 2025-10-22 13:06:56 UTC
Created attachment 188294 [details] [review]
Bug 40517: (follow-up) Fix GNU license links
Comment 76 Lucas Gass (lukeg) 2025-10-22 13:22:14 UTC
With this patchset applied xt/api.t fails:

kohadev-koha@kohadevbox:koha(main)$ prove -v xt/api.t 
xt/api.t .. 
1..8
ok 1 - GET /api/v1/
ok 2
# Subtest: The spec passes the swagger-cli validation
    1..1
    not ok 1 - Validation exit code is 0

    #   Failed test 'Validation exit code is 0'
    #   at xt/api.t line 103.
    #          got: '256'
    #     expected: '0'
    # Validation failed. /paths/patrons/{patron_id}/hold_groups/post is missing path parameter(s) for {patron_id}
    # Looks like you failed 1 test of 1.
not ok 3 - The spec passes the swagger-cli validation

#   Failed test 'The spec passes the swagger-cli validation'
#   at xt/api.t line 108.
# Subtest: tags tests
    1..1
    not ok 1 - No tag errors in the spec

    #   Failed test 'No tag errors in the spec'
    #   at xt/api.t line 130.
    #     Structures begin differing at:
    #          $got->[0] = 'get /patrons/{patron_id}/hold_groups -> uses tag 'hold_groups' not present in top level list'
    #     $expected->[0] = Does not exist
get /patrons/{patron_id}/hold_groups -> uses tag 'hold_groups' not present in top level list
post /patrons/{patron_id}/hold_groups -> uses tag 'hold_groups' not present in top level list
    # Looks like you failed 1 test of 1.
not ok 4 - tags tests
# Subtest: 400 response tests

#   Failed test 'tags tests'
#   at xt/api.t line 135.
    1..1
    ok 1 - No errors in 400 definitions in the spec
ok 5 - 400 response tests
# Subtest: POST (201) have location header
Comment 77 Pedro Amorim 2025-10-23 11:43:35 UTC
Created attachment 188343 [details] [review]
Bug 40517: (QA follow-up): Fix xt/api.t

prove -v xt/api.t
Comment 78 Lucas Gass (lukeg) 2025-10-24 16:29:33 UTC
If I push this as is we will break Koha_Main_MariaDB_update. kohastructure.sql must match the DB structure:


Schema dump completed.
Running [sudo koha-shell kohadev -p -c 'git diff && git diff --quiet || { echo "ERROR - There is a diff in DBIC schema files" && exit 1; }']...
Running [sudo koha-dump --schema-only kohadev]...
Dumping Koha site kohadev:
* schema to /var/spool/koha/kohadev/kohadev-schema-2025-10-24.sql
Running [sed -n '/-- Table structure/,/-- Dump completed/{//!p;}' /var/spool/koha/kohadev/kohadev-schema-$(date '+%Y-%m-%d').sql > /tmp/upgraded_db.sql]...
Running [sed -n '/-- Table structure/,/-- Dump completed/{//!p;}' installer/data/mysql/kohastructure.sql > /tmp/current_db.sql]...
Running [diff /tmp/current_db.sql /tmp/upgraded_db.sql && diff -q /tmp/current_db.sql /tmp/upgraded_db.sql || { echo "ERROR - DB structures are not identical" && exit 1; }]...
3537a3538
>   PRIMARY KEY (`hold_group_id`),
3539,3540c3540
<   CONSTRAINT `hold_groups_ibfk_1` FOREIGN KEY (`borrowernumber`) REFERENCES `borrowers` (`borrowernumber`) ON DELETE CASCADE,
<   PRIMARY KEY (`hold_group_id`)
---
>   CONSTRAINT `hold_groups_ibfk_1` FOREIGN KEY (`borrowernumber`) REFERENCES `borrowers` (`borrowernumber`) ON DELETE CASCADE
ERROR - DB structures are not identical


Can you look Pedro?
Comment 79 Pedro Amorim 2025-10-24 17:06:12 UTC
Created attachment 188422 [details] [review]
Bug 40517: (QA follow-up): Fix kohstructure.sql

The order was wrong, PRIMARY KEY must come first.
Comment 80 Pedro Amorim 2025-10-24 17:10:10 UTC
Follow-up pushed. Sorry for the mishap Lucas and thank you for your patience!
Comment 81 Lucas Gass (lukeg) 2025-10-29 17:53:14 UTC
Pushed to main for 25.11.00