Bug 40529 - Update how hold groups work
Summary: Update how hold groups work
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: rel_25_11_candidate, release-notes-needed
Depends on: 40517
Blocks: 40552
  Show dependency treegraph
 
Reported: 2025-07-28 13:20 UTC by Pedro Amorim
Modified: 2025-11-03 21:16 UTC (History)
10 users (show)

See Also:
GIT URL:
Initiative type: ---
Sponsorship status: ---
Comma delimited list of Sponsors:
Crowdfunding goal: 0
Patch complexity: ---
Documentation contact:
Documentation submission:
Text to go in the release notes:
This patchset changes how hold groups work: Previously, only Waiting holds triggered group-based actions by cancelling other holds in the group. Now, when a hold in a group becomes In transit or Waiting, it is designated as the group's target hold. Subsequent check-ins skip other holds in the same group unless the target is cancelled, allowing a new target to be set. This improves hold fulfillment logic and supports dynamic reassignment of target holds. The behavior applies to check-ins, holds to pull and the holds queue.
Version(s) released in:
25.11.00
Circulation function:


Attachments
Bug 40529: Add hold group targets logic (11.49 KB, patch)
2025-07-30 11:58 UTC, Pedro Amorim
Details | Diff | Splinter Review
Bug 40529: Behavior change: Main patch (9.94 KB, patch)
2025-07-30 11:58 UTC, Pedro Amorim
Details | Diff | Splinter Review
Bug 40529: Behavior change: Add target info on hold group modal (1.51 KB, patch)
2025-07-30 11:58 UTC, Pedro Amorim
Details | Diff | Splinter Review
Bug 40529: Behavior change: Update old tests (1.26 KB, patch)
2025-07-30 11:58 UTC, Pedro Amorim
Details | Diff | Splinter Review
Bug 40529: Add unit tests (17.18 KB, patch)
2025-07-30 11:58 UTC, Pedro Amorim
Details | Diff | Splinter Review
Bug 40529: Add hold group targets logic (11.49 KB, patch)
2025-08-06 09:14 UTC, Pedro Amorim
Details | Diff | Splinter Review
Bug 40529: Behavior change: Main patch (9.94 KB, patch)
2025-08-06 09:14 UTC, Pedro Amorim
Details | Diff | Splinter Review
Bug 40529: Behavior change: Add target info on hold group modal (1.51 KB, patch)
2025-08-06 09:15 UTC, Pedro Amorim
Details | Diff | Splinter Review
Bug 40529: Behavior change: Update old tests (1.08 KB, patch)
2025-08-06 09:15 UTC, Pedro Amorim
Details | Diff | Splinter Review
Bug 40529: Add unit tests (17.36 KB, patch)
2025-08-06 09:15 UTC, Pedro Amorim
Details | Diff | Splinter Review
Bug 40529: Add hold group targets logic (11.49 KB, patch)
2025-08-07 10:44 UTC, Pedro Amorim
Details | Diff | Splinter Review
Bug 40529: Behavior change: Main patch (9.94 KB, patch)
2025-08-07 10:44 UTC, Pedro Amorim
Details | Diff | Splinter Review
Bug 40529: Behavior change: Add target info on hold group modal (1.51 KB, patch)
2025-08-07 10:44 UTC, Pedro Amorim
Details | Diff | Splinter Review
Bug 40529: Add unit tests (17.36 KB, patch)
2025-08-07 10:44 UTC, Pedro Amorim
Details | Diff | Splinter Review
Bug 40529: (QA follow-up): Include target holds (4.02 KB, patch)
2025-08-07 10:44 UTC, Pedro Amorim
Details | Diff | Splinter Review
Bug 40529: (QA follow-up): Remove hold as hold group target when reverted (7.31 KB, patch)
2025-08-07 10:44 UTC, Pedro Amorim
Details | Diff | Splinter Review
Bug 40529: Behavior change: Main patch (10.08 KB, patch)
2025-08-11 09:24 UTC, OpenFifth Sandboxes
Details | Diff | Splinter Review
Bug 40529: Behavior change: Add target info on hold group modal (1.66 KB, patch)
2025-08-11 09:24 UTC, OpenFifth Sandboxes
Details | Diff | Splinter Review
Bug 40529: Add unit tests (17.51 KB, patch)
2025-08-11 09:24 UTC, OpenFifth Sandboxes
Details | Diff | Splinter Review
Bug 40529: (QA follow-up): Include target holds (4.17 KB, patch)
2025-08-11 09:24 UTC, OpenFifth Sandboxes
Details | Diff | Splinter Review
Bug 40529: (QA follow-up): Remove hold as hold group target when reverted (7.45 KB, patch)
2025-08-11 09:24 UTC, OpenFifth Sandboxes
Details | Diff | Splinter Review
Bug 40529: Add hold group targets logic (11.49 KB, patch)
2025-09-23 12:25 UTC, Pedro Amorim
Details | Diff | Splinter Review
Bug 40529: Behavior change: Main patch (10.09 KB, patch)
2025-09-23 12:25 UTC, Pedro Amorim
Details | Diff | Splinter Review
Bug 40529: Behavior change: Add target info on hold group modal (1.66 KB, patch)
2025-09-23 12:25 UTC, Pedro Amorim
Details | Diff | Splinter Review
Bug 40529: Add unit tests (17.54 KB, patch)
2025-09-23 12:25 UTC, Pedro Amorim
Details | Diff | Splinter Review
Bug 40529: (QA follow-up): Include target holds (4.17 KB, patch)
2025-09-23 12:25 UTC, Pedro Amorim
Details | Diff | Splinter Review
Bug 40529: (QA follow-up): Remove hold as hold group target when reverted (7.33 KB, patch)
2025-09-23 12:25 UTC, Pedro Amorim
Details | Diff | Splinter Review
Bug 40529: (QA follow-up): Add skip logic to GetPendingHoldRequestsForBib (1.98 KB, patch)
2025-09-23 13:50 UTC, Pedro Amorim
Details | Diff | Splinter Review
Bug 40529: (QA follow-up) Rebase and fix tests (1.93 KB, patch)
2025-09-23 13:50 UTC, Pedro Amorim
Details | Diff | Splinter Review
Bug 40529: Add hold group targets logic (11.56 KB, patch)
2025-09-23 15:51 UTC, Andrew Fuerste-Henry
Details | Diff | Splinter Review
Bug 40529: Behavior change: Main patch (10.16 KB, patch)
2025-09-23 15:51 UTC, Andrew Fuerste-Henry
Details | Diff | Splinter Review
Bug 40529: Behavior change: Add target info on hold group modal (1.73 KB, patch)
2025-09-23 15:51 UTC, Andrew Fuerste-Henry
Details | Diff | Splinter Review
Bug 40529: Add unit tests (17.61 KB, patch)
2025-09-23 15:51 UTC, Andrew Fuerste-Henry
Details | Diff | Splinter Review
Bug 40529: (QA follow-up): Include target holds (4.23 KB, patch)
2025-09-23 15:51 UTC, Andrew Fuerste-Henry
Details | Diff | Splinter Review
Bug 40529: (QA follow-up): Remove hold as hold group target when reverted (7.40 KB, patch)
2025-09-23 15:51 UTC, Andrew Fuerste-Henry
Details | Diff | Splinter Review
Bug 40529: (QA follow-up): Add skip logic to GetPendingHoldRequestsForBib (2.05 KB, patch)
2025-09-23 15:51 UTC, Andrew Fuerste-Henry
Details | Diff | Splinter Review
Bug 40529: (QA follow-up) Rebase and fix tests (1.99 KB, patch)
2025-09-23 15:51 UTC, Andrew Fuerste-Henry
Details | Diff | Splinter Review
Bug 40529: Add hold group targets logic (6.91 KB, patch)
2025-10-23 13:03 UTC, Pedro Amorim
Details | Diff | Splinter Review
Bug 40529: Behavior change: Main patch (10.16 KB, patch)
2025-10-23 13:03 UTC, Pedro Amorim
Details | Diff | Splinter Review
Bug 40529: Behavior change: Add target info on hold group modal (1.73 KB, patch)
2025-10-23 13:03 UTC, Pedro Amorim
Details | Diff | Splinter Review
Bug 40529: Add unit tests (17.61 KB, patch)
2025-10-23 13:03 UTC, Pedro Amorim
Details | Diff | Splinter Review
Bug 40529: (QA follow-up): Include target holds (4.23 KB, patch)
2025-10-23 13:03 UTC, Pedro Amorim
Details | Diff | Splinter Review
Bug 40529: (QA follow-up): Remove hold as hold group target when reverted (5.73 KB, patch)
2025-10-23 13:03 UTC, Pedro Amorim
Details | Diff | Splinter Review
Bug 40529: (QA follow-up): Add skip logic to GetPendingHoldRequestsForBib (2.05 KB, patch)
2025-10-23 13:03 UTC, Pedro Amorim
Details | Diff | Splinter Review
Bug 40529: (QA follow-up) Rebase and fix tests (1.99 KB, patch)
2025-10-23 13:03 UTC, Pedro Amorim
Details | Diff | Splinter Review
Bug 40529: [DONT PUSH] dbic (5.04 KB, patch)
2025-10-23 13:03 UTC, Pedro Amorim
Details | Diff | Splinter Review
Bug 40529: (QA follow-up): Add UNIQUE KEY entries to table definition (4.32 KB, patch)
2025-10-29 19:26 UTC, Pedro Amorim
Details | Diff | Splinter Review
Bug 40529: [DONT PUSH] Newly generated dbic (1.81 KB, patch)
2025-10-29 19:26 UTC, Pedro Amorim
Details | Diff | Splinter Review
Bug 40529: (QA follow-up): Fix DB changes (8.60 KB, patch)
2025-11-03 14:12 UTC, Pedro Amorim
Details | Diff | Splinter Review
Bug 40529: [DONT PUSH] Newly generated dbic (5.04 KB, patch)
2025-11-03 14:12 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-28 13:20:08 UTC

    
Comment 1 Pedro Amorim 2025-07-30 11:58:08 UTC Comment hidden (obsolete)
Comment 2 Pedro Amorim 2025-07-30 11:58:11 UTC Comment hidden (obsolete)
Comment 3 Pedro Amorim 2025-07-30 11:58:13 UTC Comment hidden (obsolete)
Comment 4 Pedro Amorim 2025-07-30 11:58:15 UTC Comment hidden (obsolete)
Comment 5 Pedro Amorim 2025-07-30 11:58:18 UTC Comment hidden (obsolete)
Comment 6 Pedro Amorim 2025-08-06 09:14:57 UTC Comment hidden (obsolete)
Comment 7 Pedro Amorim 2025-08-06 09:14:59 UTC Comment hidden (obsolete)
Comment 8 Pedro Amorim 2025-08-06 09:15:02 UTC Comment hidden (obsolete)
Comment 9 Pedro Amorim 2025-08-06 09:15:04 UTC Comment hidden (obsolete)
Comment 10 Pedro Amorim 2025-08-06 09:15:07 UTC Comment hidden (obsolete)
Comment 11 Pedro Amorim 2025-08-07 10:44:36 UTC
Created attachment 185210 [details] [review]
Bug 40529: Add hold group targets logic

Hold group target hold:
The hold that is set as the hold group's target. If a hold group has a
hold target, subsequent check-ins that would 'trap' items to the
remaining holds in the group are instead skipped.
Comment 12 Pedro Amorim 2025-08-07 10:44:39 UTC
Created attachment 185211 [details] [review]
Bug 40529: Behavior change: Main patch

When a hold that belongs to a hold group becomes 'In transit' or 'Waiting',
it now becomes that hold group's target hold.
Subsequent check-ins that would satisfy other holds in the same hold
group are skipped, as they are part of a group that already has a
target.

The behavior change introduced here that differs from the groundwork
from bug 15516 are:
1) Before, only 'Waiting' holds would consider an action based on hold
   groups i.e. cancel the other holds in the group. Now 'In transit'
   also does this.
2) This hold groups related action used to be to cancel other holds in
   the group. This is no longer the case. Now, the respective hold is
   set as the hold group's target. This causes the holds in the same group that
   are not the target to be skipped when a check-in that would otherwise
   satisfy one of those holds happens. This behavior change allows the following
   scenario which wasn't possible before:

   A hold is marked 'Waiting'. It becomes its group target. For any reason,
   that hold is then cancelled. Any subsequent check-in of an item of a
   hold in this group may now becomes that group's new target.
   Similarly, the same use case above is also true for 'In transit' holds.

----

   The 'skip non-target holds' behavior has been added to check-in logic
(Reserves::_Findgroupreserve), 'holds to pull' logic
(pendingreserves.pl) and holds queue (HoldsQueue.pm).
Comment 13 Pedro Amorim 2025-08-07 10:44:41 UTC
Created attachment 185212 [details] [review]
Bug 40529: Behavior change: Add target info on hold group modal
Comment 14 Pedro Amorim 2025-08-07 10:44:44 UTC
Created attachment 185213 [details] [review]
Bug 40529: Add unit tests

Test plan, k-t-d, patches applied:
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) Notice there are 3 holds all part of hold group '1'
8) Check-in one of the items in this hold group: 39999000005578. Confirm hold.
9) Repeat 6). Notice 'Continuous delivery' is now the hold group 'target'.
10) Place a new hold for one of the other 2 'non-target' holds for a different patron. Visit:
<staff_url>/cgi-bin/koha/reserve/request.pl?biblionumber=29
11) Enter 'henry' on the patron input. Pick 'Henry Acevedo (23529000035676)'. Click 'Place hold'. Notice Henry's hold is priority 2. Let's check-in the item for this record: 39999000001310
12) Notice the eligible hold is Henry's, not the koha user's hold. This is because the hold that relates to koha's user is part of a hold group that already has a target so it was skipped. You may confirm the hold.
13) Now let's cancel 'target' hold, visit:
<staff_url>/cgi-bin/koha/members/moremember.pl?borrowernumber=51
14) Click the 'Holds' tab and the red trash icon next to 'Continuous delivery'. Confirm cancellation.
15) Check the holds again, now there's only 2 holds in the existing group.
16) Let's repeat the exercise as before and place a hold for 'Henry' for 'Clean code', visit:
<staff_url>/cgi-bin/koha/reserve/request.pl?biblionumber=114
17) Enter 'henry' and place a hold as before.
18) The 'Clean code' record now has 2 holds: first for 'koha' user and the second for 'Henry'.
19) Now check-in the item for that record. Notice it's the 'koha' user's hold that comes up. This is because it's first in priority and the hold is part of a hold group that does not yet have a target. Confirm hold.
20) Go back to 'Henry' and cancel his holds. Visit:
<staff_url>/cgi-bin/koha/members/moremember.pl?borrowernumber=19
21) Select 'both', click 'Cancel selected'. Confirm cancellation.
22) Visit 'holds to pull':
<staff_url>/cgi-bin/koha/circ/pendingreserves.pl
23) Notice no holds show, even though 'koha' user has a hold that does not yet have an item assigned to it. This hold was skipped because it's part of a hold group that already has a target. To confirm this, disable DisplayAddHoldGroups and refresh 'Holds to pull'. Confirm it shows.
24) The same happens for the 'holds queue' i.e. 'non-target' holds are skipped. To test, with the sys pref still disabled, run the build_holds_queue.pl script:
$ perl misc/cronjobs/holds/build_holds_queue.pl
25) Notice the 'non-target' hold shows, and that is expected if DisplayAddHoldGroups is disabled. Enable DisplayAddHoldGroups and repeat 24. Confirm the hold is gone.

The behavior above works as described for holds that become 'In transit' or 'Waiting'.

Run the updated test files:
prove ./t/db_dependent/Koha/Hold.t
prove ./t/db_dependent/Reserves/HoldGroup.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*
Comment 15 Pedro Amorim 2025-08-07 10:44:47 UTC
Created attachment 185214 [details] [review]
Bug 40529: (QA follow-up): Include target holds

The skipping of non target holds was skipping all holds that are part of a hold group which has a target, including the target hold itself. This was wrong. Target holds need to be included as they are considered to be a regular 'found' hold.

Tests that prove the bug and its fix added to:
prove ./t/db_dependent/Koha/Hold.t
Comment 16 Pedro Amorim 2025-08-07 10:44:49 UTC
Created attachment 185215 [details] [review]
Bug 40529: (QA follow-up): Remove hold as hold group target when reverted

If a hold that is a hold group target, i.e. it's 'waiting' or 'in transit', is reverted it needs to remove itself as its own hold group target

Tests added here as well:
prove t/db_dependent/Koha/Hold.t
Comment 17 OpenFifth Sandboxes 2025-08-11 09:24:48 UTC
Created attachment 185303 [details] [review]
Bug 40529: Behavior change: Main patch

When a hold that belongs to a hold group becomes 'In transit' or 'Waiting',
it now becomes that hold group's target hold.
Subsequent check-ins that would satisfy other holds in the same hold
group are skipped, as they are part of a group that already has a
target.

The behavior change introduced here that differs from the groundwork
from bug 15516 are:
1) Before, only 'Waiting' holds would consider an action based on hold
   groups i.e. cancel the other holds in the group. Now 'In transit'
   also does this.
2) This hold groups related action used to be to cancel other holds in
   the group. This is no longer the case. Now, the respective hold is
   set as the hold group's target. This causes the holds in the same group that
   are not the target to be skipped when a check-in that would otherwise
   satisfy one of those holds happens. This behavior change allows the following
   scenario which wasn't possible before:

   A hold is marked 'Waiting'. It becomes its group target. For any reason,
   that hold is then cancelled. Any subsequent check-in of an item of a
   hold in this group may now becomes that group's new target.
   Similarly, the same use case above is also true for 'In transit' holds.

----

   The 'skip non-target holds' behavior has been added to check-in logic
(Reserves::_Findgroupreserve), 'holds to pull' logic
(pendingreserves.pl) and holds queue (HoldsQueue.pm).

Signed-off-by: Anneli Österman <anneli.osterman@koha-suomi.fi>
Comment 18 OpenFifth Sandboxes 2025-08-11 09:24:51 UTC
Created attachment 185304 [details] [review]
Bug 40529: Behavior change: Add target info on hold group modal

Signed-off-by: Anneli Österman <anneli.osterman@koha-suomi.fi>
Comment 19 OpenFifth Sandboxes 2025-08-11 09:24:53 UTC
Created attachment 185305 [details] [review]
Bug 40529: Add unit tests

Test plan, k-t-d, patches applied:
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) Notice there are 3 holds all part of hold group '1'
8) Check-in one of the items in this hold group: 39999000005578. Confirm hold.
9) Repeat 6). Notice 'Continuous delivery' is now the hold group 'target'.
10) Place a new hold for one of the other 2 'non-target' holds for a different patron. Visit:
<staff_url>/cgi-bin/koha/reserve/request.pl?biblionumber=29
11) Enter 'henry' on the patron input. Pick 'Henry Acevedo (23529000035676)'. Click 'Place hold'. Notice Henry's hold is priority 2. Let's check-in the item for this record: 39999000001310
12) Notice the eligible hold is Henry's, not the koha user's hold. This is because the hold that relates to koha's user is part of a hold group that already has a target so it was skipped. You may confirm the hold.
13) Now let's cancel 'target' hold, visit:
<staff_url>/cgi-bin/koha/members/moremember.pl?borrowernumber=51
14) Click the 'Holds' tab and the red trash icon next to 'Continuous delivery'. Confirm cancellation.
15) Check the holds again, now there's only 2 holds in the existing group.
16) Let's repeat the exercise as before and place a hold for 'Henry' for 'Clean code', visit:
<staff_url>/cgi-bin/koha/reserve/request.pl?biblionumber=114
17) Enter 'henry' and place a hold as before.
18) The 'Clean code' record now has 2 holds: first for 'koha' user and the second for 'Henry'.
19) Now check-in the item for that record. Notice it's the 'koha' user's hold that comes up. This is because it's first in priority and the hold is part of a hold group that does not yet have a target. Confirm hold.
20) Go back to 'Henry' and cancel his holds. Visit:
<staff_url>/cgi-bin/koha/members/moremember.pl?borrowernumber=19
21) Select 'both', click 'Cancel selected'. Confirm cancellation.
22) Visit 'holds to pull':
<staff_url>/cgi-bin/koha/circ/pendingreserves.pl
23) Notice no holds show, even though 'koha' user has a hold that does not yet have an item assigned to it. This hold was skipped because it's part of a hold group that already has a target. To confirm this, disable DisplayAddHoldGroups and refresh 'Holds to pull'. Confirm it shows.
24) The same happens for the 'holds queue' i.e. 'non-target' holds are skipped. To test, with the sys pref still disabled, run the build_holds_queue.pl script:
$ perl misc/cronjobs/holds/build_holds_queue.pl
25) Notice the 'non-target' hold shows, and that is expected if DisplayAddHoldGroups is disabled. Enable DisplayAddHoldGroups and repeat 24. Confirm the hold is gone.

The behavior above works as described for holds that become 'In transit' or 'Waiting'.

Run the updated test files:
prove ./t/db_dependent/Koha/Hold.t
prove ./t/db_dependent/Reserves/HoldGroup.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*

Signed-off-by: Anneli Österman <anneli.osterman@koha-suomi.fi>
Comment 20 OpenFifth Sandboxes 2025-08-11 09:24:56 UTC
Created attachment 185306 [details] [review]
Bug 40529: (QA follow-up): Include target holds

The skipping of non target holds was skipping all holds that are part of a hold group which has a target, including the target hold itself. This was wrong. Target holds need to be included as they are considered to be a regular 'found' hold.

Tests that prove the bug and its fix added to:
prove ./t/db_dependent/Koha/Hold.t

Signed-off-by: Anneli Österman <anneli.osterman@koha-suomi.fi>
Comment 21 OpenFifth Sandboxes 2025-08-11 09:24:58 UTC
Created attachment 185307 [details] [review]
Bug 40529: (QA follow-up): Remove hold as hold group target when reverted

If a hold that is a hold group target, i.e. it's 'waiting' or 'in transit', is reverted it needs to remove itself as its own hold group target

Tests added here as well:
prove t/db_dependent/Koha/Hold.t

Signed-off-by: Anneli Österman <anneli.osterman@koha-suomi.fi>
Comment 22 Anneli Österman 2025-08-11 09:27:57 UTC
I could not test numbers 24-25 in the test plan or run the tests. Otherwise worked as described.
Comment 23 Andrew Fuerste-Henry 2025-09-22 20:45:06 UTC
I hit one issue in testing here in steps 24/25, when I build the holds queue while my test patron (Patron A) has grouped hold for which one hold is Waiting and therefore the target of the group. 

With DisaplyAddHoldGroups enabled, the other holds in that group correctly do not show in the Queue. However, if one of the bibs that is not the target of the hold group has a second hold on it for another patron (Patron B), that title shows in the holds queue as needing to be pulled for Patron A.

Holds to Pull correctly shows that second title as needing to be pulled for Patron B.
At checkin, the item correctly allocates to Patron B.
But the queue is incorrectly grabbing the patron from the first hold, even though that hold should be fully skipped because it's not the target for its group.
Comment 24 Pedro Amorim 2025-09-23 12:03:25 UTC
(In reply to Andrew Fuerste-Henry from comment #23)
> I hit one issue in testing here in steps 24/25, when I build the holds queue
> while my test patron (Patron A) has grouped hold for which one hold is
> Waiting and therefore the target of the group. 
> 
> With DisaplyAddHoldGroups enabled, the other holds in that group correctly
> do not show in the Queue. However, if one of the bibs that is not the target
> of the hold group has a second hold on it for another patron (Patron B),
> that title shows in the holds queue as needing to be pulled for Patron A.
> 
> Holds to Pull correctly shows that second title as needing to be pulled for
> Patron B.
> At checkin, the item correctly allocates to Patron B.
> But the queue is incorrectly grabbing the patron from the first hold, even
> though that hold should be fully skipped because it's not the target for its
> group.

Thank you Andrew, not applying for me currently. Taking a look at a rebase and then your comments.
Comment 25 Pedro Amorim 2025-09-23 12:25:34 UTC
Created attachment 186793 [details] [review]
Bug 40529: Add hold group targets logic

Hold group target hold:
The hold that is set as the hold group's target. If a hold group has a
hold target, subsequent check-ins that would 'trap' items to the
remaining holds in the group are instead skipped.
Comment 26 Pedro Amorim 2025-09-23 12:25:36 UTC
Created attachment 186794 [details] [review]
Bug 40529: Behavior change: Main patch

When a hold that belongs to a hold group becomes 'In transit' or 'Waiting',
it now becomes that hold group's target hold.
Subsequent check-ins that would satisfy other holds in the same hold
group are skipped, as they are part of a group that already has a
target.

The behavior change introduced here that differs from the groundwork
from bug 15516 are:
1) Before, only 'Waiting' holds would consider an action based on hold
   groups i.e. cancel the other holds in the group. Now 'In transit'
   also does this.
2) This hold groups related action used to be to cancel other holds in
   the group. This is no longer the case. Now, the respective hold is
   set as the hold group's target. This causes the holds in the same group that
   are not the target to be skipped when a check-in that would otherwise
   satisfy one of those holds happens. This behavior change allows the following
   scenario which wasn't possible before:

   A hold is marked 'Waiting'. It becomes its group target. For any reason,
   that hold is then cancelled. Any subsequent check-in of an item of a
   hold in this group may now becomes that group's new target.
   Similarly, the same use case above is also true for 'In transit' holds.

----

   The 'skip non-target holds' behavior has been added to check-in logic
(Reserves::_Findgroupreserve), 'holds to pull' logic
(pendingreserves.pl) and holds queue (HoldsQueue.pm).

Signed-off-by: Anneli Österman <anneli.osterman@koha-suomi.fi>
Comment 27 Pedro Amorim 2025-09-23 12:25:39 UTC
Created attachment 186795 [details] [review]
Bug 40529: Behavior change: Add target info on hold group modal

Signed-off-by: Anneli Österman <anneli.osterman@koha-suomi.fi>
Comment 28 Pedro Amorim 2025-09-23 12:25:41 UTC
Created attachment 186796 [details] [review]
Bug 40529: Add unit tests

Test plan, k-t-d, patches applied:
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) Notice there are 3 holds all part of hold group '1'
8) Check-in one of the items in this hold group: 39999000005578. Confirm hold.
9) Repeat 6). Notice 'Continuous delivery' is now the hold group 'target'.
10) Place a new hold for one of the other 2 'non-target' holds for a different patron. Visit:
<staff_url>/cgi-bin/koha/reserve/request.pl?biblionumber=29
11) Enter 'henry' on the patron input. Pick 'Henry Acevedo (23529000035676)'. Click 'Place hold'. Notice Henry's hold is priority 2. Let's check-in the item for this record: 39999000001310
12) Notice the eligible hold is Henry's, not the koha user's hold. This is because the hold that relates to koha's user is part of a hold group that already has a target so it was skipped. You may confirm the hold.
13) Now let's cancel 'target' hold, visit:
<staff_url>/cgi-bin/koha/members/moremember.pl?borrowernumber=51
14) Click the 'Holds' tab and the red trash icon next to 'Continuous delivery'. Confirm cancellation.
15) Check the holds again, now there's only 2 holds in the existing group.
16) Let's repeat the exercise as before and place a hold for 'Henry' for 'Clean code', visit:
<staff_url>/cgi-bin/koha/reserve/request.pl?biblionumber=114
17) Enter 'henry' and place a hold as before.
18) The 'Clean code' record now has 2 holds: first for 'koha' user and the second for 'Henry'.
19) Now check-in the item for that record. Notice it's the 'koha' user's hold that comes up. This is because it's first in priority and the hold is part of a hold group that does not yet have a target. Confirm hold.
20) Go back to 'Henry' and cancel his holds. Visit:
<staff_url>/cgi-bin/koha/members/moremember.pl?borrowernumber=19
21) Select 'both', click 'Cancel selected'. Confirm cancellation.
22) Visit 'holds to pull':
<staff_url>/cgi-bin/koha/circ/pendingreserves.pl
23) Notice no holds show, even though 'koha' user has a hold that does not yet have an item assigned to it. This hold was skipped because it's part of a hold group that already has a target. To confirm this, disable DisplayAddHoldGroups and refresh 'Holds to pull'. Confirm it shows.
24) The same happens for the 'holds queue' i.e. 'non-target' holds are skipped. To test, with the sys pref still disabled, run the build_holds_queue.pl script:
$ perl misc/cronjobs/holds/build_holds_queue.pl
25) Notice the 'non-target' hold shows, and that is expected if DisplayAddHoldGroups is disabled. Enable DisplayAddHoldGroups and repeat 24. Confirm the hold is gone.

The behavior above works as described for holds that become 'In transit' or 'Waiting'.

Run the updated test files:
prove ./t/db_dependent/Koha/Hold.t
prove ./t/db_dependent/Reserves/HoldGroup.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*

Signed-off-by: Anneli Österman <anneli.osterman@koha-suomi.fi>
Comment 29 Pedro Amorim 2025-09-23 12:25:44 UTC
Created attachment 186797 [details] [review]
Bug 40529: (QA follow-up): Include target holds

The skipping of non target holds was skipping all holds that are part of a hold group which has a target, including the target hold itself. This was wrong. Target holds need to be included as they are considered to be a regular 'found' hold.

Tests that prove the bug and its fix added to:
prove ./t/db_dependent/Koha/Hold.t

Signed-off-by: Anneli Österman <anneli.osterman@koha-suomi.fi>
Comment 30 Pedro Amorim 2025-09-23 12:25:46 UTC
Created attachment 186798 [details] [review]
Bug 40529: (QA follow-up): Remove hold as hold group target when reverted

If a hold that is a hold group target, i.e. it's 'waiting' or 'in transit', is reverted it needs to remove itself as its own hold group target

Tests added here as well:
prove t/db_dependent/Koha/Hold.t

Signed-off-by: Anneli Österman <anneli.osterman@koha-suomi.fi>
Comment 31 Pedro Amorim 2025-09-23 12:26:12 UTC
This has now been rebased on top of 40671 and 40672
Comment 32 Pedro Amorim 2025-09-23 13:50:42 UTC
Created attachment 186803 [details] [review]
Bug 40529: (QA follow-up): Add skip logic to GetPendingHoldRequestsForBib

This fixes the issue reported by Andrew on comment 23.
Following the scenario he describes, the hold should now still appear, but attached to the correct patron (Patron B).
This was an oversight as I wasn't aware of GetPendingHoldRequestsForBib and had only added the skip logic to GetBibsWithPendingHoldRequests.
Comment 33 Pedro Amorim 2025-09-23 13:50:45 UTC
Created attachment 186804 [details] [review]
Bug 40529: (QA follow-up) Rebase and fix tests

This further rebases on top of 40058 which removed RevertWaitingStatus.
It also fixes a warning where 'eq was being utilized on initialized' coming from sub remove_as_hold_group_target
Comment 34 Pedro Amorim 2025-09-23 13:52:34 UTC
(In reply to Pedro Amorim from comment #24)
> (In reply to Andrew Fuerste-Henry from comment #23)
> > But the queue is incorrectly grabbing the patron from the first hold, even
> > though that hold should be fully skipped because it's not the target for its
> > group.
> 
> Thank you Andrew, not applying for me currently. Taking a look at a rebase
> and then your comments.

Hi Andrew, thank you. I believe this is now fixed. Can you please test again? Thank you.
Comment 35 Andrew Fuerste-Henry 2025-09-23 15:51:23 UTC
Created attachment 186825 [details] [review]
Bug 40529: Add hold group targets logic

Hold group target hold:
The hold that is set as the hold group's target. If a hold group has a
hold target, subsequent check-ins that would 'trap' items to the
remaining holds in the group are instead skipped.

Signed-off-by: Andrew Fuerste Henry <andrew@bywatersolutions.com>
Comment 36 Andrew Fuerste-Henry 2025-09-23 15:51:26 UTC
Created attachment 186826 [details] [review]
Bug 40529: Behavior change: Main patch

When a hold that belongs to a hold group becomes 'In transit' or 'Waiting',
it now becomes that hold group's target hold.
Subsequent check-ins that would satisfy other holds in the same hold
group are skipped, as they are part of a group that already has a
target.

The behavior change introduced here that differs from the groundwork
from bug 15516 are:
1) Before, only 'Waiting' holds would consider an action based on hold
   groups i.e. cancel the other holds in the group. Now 'In transit'
   also does this.
2) This hold groups related action used to be to cancel other holds in
   the group. This is no longer the case. Now, the respective hold is
   set as the hold group's target. This causes the holds in the same group that
   are not the target to be skipped when a check-in that would otherwise
   satisfy one of those holds happens. This behavior change allows the following
   scenario which wasn't possible before:

   A hold is marked 'Waiting'. It becomes its group target. For any reason,
   that hold is then cancelled. Any subsequent check-in of an item of a
   hold in this group may now becomes that group's new target.
   Similarly, the same use case above is also true for 'In transit' holds.

----

   The 'skip non-target holds' behavior has been added to check-in logic
(Reserves::_Findgroupreserve), 'holds to pull' logic
(pendingreserves.pl) and holds queue (HoldsQueue.pm).

Signed-off-by: Anneli Österman <anneli.osterman@koha-suomi.fi>
Signed-off-by: Andrew Fuerste Henry <andrew@bywatersolutions.com>
Comment 37 Andrew Fuerste-Henry 2025-09-23 15:51:29 UTC
Created attachment 186827 [details] [review]
Bug 40529: Behavior change: Add target info on hold group modal

Signed-off-by: Anneli Österman <anneli.osterman@koha-suomi.fi>
Signed-off-by: Andrew Fuerste Henry <andrew@bywatersolutions.com>
Comment 38 Andrew Fuerste-Henry 2025-09-23 15:51:32 UTC
Created attachment 186828 [details] [review]
Bug 40529: Add unit tests

Test plan, k-t-d, patches applied:
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) Notice there are 3 holds all part of hold group '1'
8) Check-in one of the items in this hold group: 39999000005578. Confirm hold.
9) Repeat 6). Notice 'Continuous delivery' is now the hold group 'target'.
10) Place a new hold for one of the other 2 'non-target' holds for a different patron. Visit:
<staff_url>/cgi-bin/koha/reserve/request.pl?biblionumber=29
11) Enter 'henry' on the patron input. Pick 'Henry Acevedo (23529000035676)'. Click 'Place hold'. Notice Henry's hold is priority 2. Let's check-in the item for this record: 39999000001310
12) Notice the eligible hold is Henry's, not the koha user's hold. This is because the hold that relates to koha's user is part of a hold group that already has a target so it was skipped. You may confirm the hold.
13) Now let's cancel 'target' hold, visit:
<staff_url>/cgi-bin/koha/members/moremember.pl?borrowernumber=51
14) Click the 'Holds' tab and the red trash icon next to 'Continuous delivery'. Confirm cancellation.
15) Check the holds again, now there's only 2 holds in the existing group.
16) Let's repeat the exercise as before and place a hold for 'Henry' for 'Clean code', visit:
<staff_url>/cgi-bin/koha/reserve/request.pl?biblionumber=114
17) Enter 'henry' and place a hold as before.
18) The 'Clean code' record now has 2 holds: first for 'koha' user and the second for 'Henry'.
19) Now check-in the item for that record. Notice it's the 'koha' user's hold that comes up. This is because it's first in priority and the hold is part of a hold group that does not yet have a target. Confirm hold.
20) Go back to 'Henry' and cancel his holds. Visit:
<staff_url>/cgi-bin/koha/members/moremember.pl?borrowernumber=19
21) Select 'both', click 'Cancel selected'. Confirm cancellation.
22) Visit 'holds to pull':
<staff_url>/cgi-bin/koha/circ/pendingreserves.pl
23) Notice no holds show, even though 'koha' user has a hold that does not yet have an item assigned to it. This hold was skipped because it's part of a hold group that already has a target. To confirm this, disable DisplayAddHoldGroups and refresh 'Holds to pull'. Confirm it shows.
24) The same happens for the 'holds queue' i.e. 'non-target' holds are skipped. To test, with the sys pref still disabled, run the build_holds_queue.pl script:
$ perl misc/cronjobs/holds/build_holds_queue.pl
25) Notice the 'non-target' hold shows, and that is expected if DisplayAddHoldGroups is disabled. Enable DisplayAddHoldGroups and repeat 24. Confirm the hold is gone.

The behavior above works as described for holds that become 'In transit' or 'Waiting'.

Run the updated test files:
prove ./t/db_dependent/Koha/Hold.t
prove ./t/db_dependent/Reserves/HoldGroup.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*

Signed-off-by: Anneli Österman <anneli.osterman@koha-suomi.fi>
Signed-off-by: Andrew Fuerste Henry <andrew@bywatersolutions.com>
Comment 39 Andrew Fuerste-Henry 2025-09-23 15:51:35 UTC
Created attachment 186829 [details] [review]
Bug 40529: (QA follow-up): Include target holds

The skipping of non target holds was skipping all holds that are part of a hold group which has a target, including the target hold itself. This was wrong. Target holds need to be included as they are considered to be a regular 'found' hold.

Tests that prove the bug and its fix added to:
prove ./t/db_dependent/Koha/Hold.t

Signed-off-by: Anneli Österman <anneli.osterman@koha-suomi.fi>
Signed-off-by: Andrew Fuerste Henry <andrew@bywatersolutions.com>
Comment 40 Andrew Fuerste-Henry 2025-09-23 15:51:38 UTC
Created attachment 186830 [details] [review]
Bug 40529: (QA follow-up): Remove hold as hold group target when reverted

If a hold that is a hold group target, i.e. it's 'waiting' or 'in transit', is reverted it needs to remove itself as its own hold group target

Tests added here as well:
prove t/db_dependent/Koha/Hold.t

Signed-off-by: Anneli Österman <anneli.osterman@koha-suomi.fi>
Signed-off-by: Andrew Fuerste Henry <andrew@bywatersolutions.com>
Comment 41 Andrew Fuerste-Henry 2025-09-23 15:51:41 UTC
Created attachment 186831 [details] [review]
Bug 40529: (QA follow-up): Add skip logic to GetPendingHoldRequestsForBib

This fixes the issue reported by Andrew on comment 23.
Following the scenario he describes, the hold should now still appear, but attached to the correct patron (Patron B).
This was an oversight as I wasn't aware of GetPendingHoldRequestsForBib and had only added the skip logic to GetBibsWithPendingHoldRequests.

Signed-off-by: Andrew Fuerste Henry <andrew@bywatersolutions.com>
Comment 42 Andrew Fuerste-Henry 2025-09-23 15:51:43 UTC
Created attachment 186832 [details] [review]
Bug 40529: (QA follow-up) Rebase and fix tests

This further rebases on top of 40058 which removed RevertWaitingStatus.
It also fixes a warning where 'eq was being utilized on initialized' coming from sub remove_as_hold_group_target

Signed-off-by: Andrew Fuerste Henry <andrew@bywatersolutions.com>
Comment 43 Pedro Amorim 2025-10-23 13:03:24 UTC
Created attachment 188348 [details] [review]
Bug 40529: Add hold group targets logic

Hold group target hold:
The hold that is set as the hold group's target. If a hold group has a
hold target, subsequent check-ins that would 'trap' items to the
remaining holds in the group are instead skipped.

Signed-off-by: Andrew Fuerste Henry <andrew@bywatersolutions.com>
Comment 44 Pedro Amorim 2025-10-23 13:03:26 UTC
Created attachment 188349 [details] [review]
Bug 40529: Behavior change: Main patch

When a hold that belongs to a hold group becomes 'In transit' or 'Waiting',
it now becomes that hold group's target hold.
Subsequent check-ins that would satisfy other holds in the same hold
group are skipped, as they are part of a group that already has a
target.

The behavior change introduced here that differs from the groundwork
from bug 15516 are:
1) Before, only 'Waiting' holds would consider an action based on hold
   groups i.e. cancel the other holds in the group. Now 'In transit'
   also does this.
2) This hold groups related action used to be to cancel other holds in
   the group. This is no longer the case. Now, the respective hold is
   set as the hold group's target. This causes the holds in the same group that
   are not the target to be skipped when a check-in that would otherwise
   satisfy one of those holds happens. This behavior change allows the following
   scenario which wasn't possible before:

   A hold is marked 'Waiting'. It becomes its group target. For any reason,
   that hold is then cancelled. Any subsequent check-in of an item of a
   hold in this group may now becomes that group's new target.
   Similarly, the same use case above is also true for 'In transit' holds.

----

   The 'skip non-target holds' behavior has been added to check-in logic
(Reserves::_Findgroupreserve), 'holds to pull' logic
(pendingreserves.pl) and holds queue (HoldsQueue.pm).

Signed-off-by: Anneli Österman <anneli.osterman@koha-suomi.fi>
Signed-off-by: Andrew Fuerste Henry <andrew@bywatersolutions.com>
Comment 45 Pedro Amorim 2025-10-23 13:03:29 UTC
Created attachment 188350 [details] [review]
Bug 40529: Behavior change: Add target info on hold group modal

Signed-off-by: Anneli Österman <anneli.osterman@koha-suomi.fi>
Signed-off-by: Andrew Fuerste Henry <andrew@bywatersolutions.com>
Comment 46 Pedro Amorim 2025-10-23 13:03:32 UTC
Created attachment 188351 [details] [review]
Bug 40529: Add unit tests

Test plan, k-t-d, patches applied:
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) Notice there are 3 holds all part of hold group '1'
8) Check-in one of the items in this hold group: 39999000005578. Confirm hold.
9) Repeat 6). Notice 'Continuous delivery' is now the hold group 'target'.
10) Place a new hold for one of the other 2 'non-target' holds for a different patron. Visit:
<staff_url>/cgi-bin/koha/reserve/request.pl?biblionumber=29
11) Enter 'henry' on the patron input. Pick 'Henry Acevedo (23529000035676)'. Click 'Place hold'. Notice Henry's hold is priority 2. Let's check-in the item for this record: 39999000001310
12) Notice the eligible hold is Henry's, not the koha user's hold. This is because the hold that relates to koha's user is part of a hold group that already has a target so it was skipped. You may confirm the hold.
13) Now let's cancel 'target' hold, visit:
<staff_url>/cgi-bin/koha/members/moremember.pl?borrowernumber=51
14) Click the 'Holds' tab and the red trash icon next to 'Continuous delivery'. Confirm cancellation.
15) Check the holds again, now there's only 2 holds in the existing group.
16) Let's repeat the exercise as before and place a hold for 'Henry' for 'Clean code', visit:
<staff_url>/cgi-bin/koha/reserve/request.pl?biblionumber=114
17) Enter 'henry' and place a hold as before.
18) The 'Clean code' record now has 2 holds: first for 'koha' user and the second for 'Henry'.
19) Now check-in the item for that record. Notice it's the 'koha' user's hold that comes up. This is because it's first in priority and the hold is part of a hold group that does not yet have a target. Confirm hold.
20) Go back to 'Henry' and cancel his holds. Visit:
<staff_url>/cgi-bin/koha/members/moremember.pl?borrowernumber=19
21) Select 'both', click 'Cancel selected'. Confirm cancellation.
22) Visit 'holds to pull':
<staff_url>/cgi-bin/koha/circ/pendingreserves.pl
23) Notice no holds show, even though 'koha' user has a hold that does not yet have an item assigned to it. This hold was skipped because it's part of a hold group that already has a target. To confirm this, disable DisplayAddHoldGroups and refresh 'Holds to pull'. Confirm it shows.
24) The same happens for the 'holds queue' i.e. 'non-target' holds are skipped. To test, with the sys pref still disabled, run the build_holds_queue.pl script:
$ perl misc/cronjobs/holds/build_holds_queue.pl
25) Notice the 'non-target' hold shows, and that is expected if DisplayAddHoldGroups is disabled. Enable DisplayAddHoldGroups and repeat 24. Confirm the hold is gone.

The behavior above works as described for holds that become 'In transit' or 'Waiting'.

Run the updated test files:
prove ./t/db_dependent/Koha/Hold.t
prove ./t/db_dependent/Reserves/HoldGroup.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*

Signed-off-by: Anneli Österman <anneli.osterman@koha-suomi.fi>
Signed-off-by: Andrew Fuerste Henry <andrew@bywatersolutions.com>
Comment 47 Pedro Amorim 2025-10-23 13:03:35 UTC
Created attachment 188352 [details] [review]
Bug 40529: (QA follow-up): Include target holds

The skipping of non target holds was skipping all holds that are part of a hold group which has a target, including the target hold itself. This was wrong. Target holds need to be included as they are considered to be a regular 'found' hold.

Tests that prove the bug and its fix added to:
prove ./t/db_dependent/Koha/Hold.t

Signed-off-by: Anneli Österman <anneli.osterman@koha-suomi.fi>
Signed-off-by: Andrew Fuerste Henry <andrew@bywatersolutions.com>
Comment 48 Pedro Amorim 2025-10-23 13:03:38 UTC
Created attachment 188353 [details] [review]
Bug 40529: (QA follow-up): Remove hold as hold group target when reverted

If a hold that is a hold group target, i.e. it's 'waiting' or 'in transit', is reverted it needs to remove itself as its own hold group target

Tests added here as well:
prove t/db_dependent/Koha/Hold.t

Signed-off-by: Anneli Österman <anneli.osterman@koha-suomi.fi>
Signed-off-by: Andrew Fuerste Henry <andrew@bywatersolutions.com>
Comment 49 Pedro Amorim 2025-10-23 13:03:40 UTC
Created attachment 188354 [details] [review]
Bug 40529: (QA follow-up): Add skip logic to GetPendingHoldRequestsForBib

This fixes the issue reported by Andrew on comment 23.
Following the scenario he describes, the hold should now still appear, but attached to the correct patron (Patron B).
This was an oversight as I wasn't aware of GetPendingHoldRequestsForBib and had only added the skip logic to GetBibsWithPendingHoldRequests.

Signed-off-by: Andrew Fuerste Henry <andrew@bywatersolutions.com>
Comment 50 Pedro Amorim 2025-10-23 13:03:43 UTC
Created attachment 188355 [details] [review]
Bug 40529: (QA follow-up) Rebase and fix tests

This further rebases on top of 40058 which removed RevertWaitingStatus.
It also fixes a warning where 'eq was being utilized on initialized' coming from sub remove_as_hold_group_target

Signed-off-by: Andrew Fuerste Henry <andrew@bywatersolutions.com>
Comment 51 Pedro Amorim 2025-10-23 13:03:46 UTC
Created attachment 188356 [details] [review]
Bug 40529: [DONT PUSH] dbic
Comment 52 Pedro Amorim 2025-10-23 13:12:51 UTC
Assuming this lands in 25.11, the release notes here may confuse people (and/or the bug title itself):
This is changing behavior that was introduced in bug 15516, which itself was only pushed to 25.11, i.e. for anyone using this feature from 25.11 onwards, there's no actual "change", it's how the functionality is presented from the start.
Not a huge deal, I don't think, but wanted to mention.
Comment 53 Lucas Gass (lukeg) 2025-10-29 17:09:26 UTC
Again we now have structural diffs that need to be reconciled:

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; }]...
3527a3528,3545
> -- Table structure for table `hold_group_target_holds`
> --
> 
> DROP TABLE IF EXISTS `hold_group_target_holds`;
> /*!40101 SET @saved_cs_client     = @@character_set_client */;
> /*!40101 SET character_set_client = utf8mb4 */;
> CREATE TABLE `hold_group_target_holds` (
>   `hold_group_id` int(10) unsigned NOT NULL COMMENT 'foreign key, linking this to the hold_groups table',
>   `reserve_id` int(11) NOT NULL COMMENT 'foreign key, linking this to the reserves table',
>   PRIMARY KEY (`hold_group_id`,`reserve_id`),
>   UNIQUE KEY `hold_group_id` (`hold_group_id`),
>   UNIQUE KEY `reserve_id` (`reserve_id`),
>   CONSTRAINT `hold_group_target_holds_ibfk_1` FOREIGN KEY (`hold_group_id`) REFERENCES `hold_groups` (`hold_group_id`) ON DELETE CASCADE,
>   CONSTRAINT `hold_group_target_holds_ibfk_2` FOREIGN KEY (`reserve_id`) REFERENCES `reserves` (`reserve_id`) ON DELETE CASCADE
> ) ENGINE=InnoDB DEFAULT CHARSET=utf8mb4 COLLATE=utf8mb4_unicode_ci;
> /*!40101 SET character_set_client = @saved_cs_client */;
> 
> --
5094,5108d5111
< ) ENGINE=InnoDB DEFAULT CHARSET=utf8mb4 COLLATE=utf8mb4_unicode_ci;
< /*!40101 SET character_set_client = @saved_cs_client */;
< 
< --
< -- Table structure for table `hold_group_target_holds`
< --
< DROP TABLE IF EXISTS `hold_group_target_holds`;
< /*!40101 SET @saved_cs_client     = @@character_set_client */;
< /*!40101 SET character_set_client = utf8 */;
< CREATE TABLE hold_group_target_holds (
<     `hold_group_id` int unsigned NOT NULL UNIQUE COMMENT 'foreign key, linking this to the hold_groups table',
<     `reserve_id` int(11) DEFAULT NULL UNIQUE COMMENT 'foreign key, linking this to the reserves table',
<     PRIMARY KEY (`hold_group_id`, `reserve_id`),
<     CONSTRAINT `hold_group_target_holds_ibfk_1` FOREIGN KEY (`hold_group_id`) REFERENCES `hold_groups` (`hold_group_id`) ON DELETE CASCADE,
<     CONSTRAINT `hold_group_target_holds_ibfk_2` FOREIGN KEY (`reserve_id`) REFERENCES `reserves` (`reserve_id`) ON DELETE CASCADE
ERROR - DB structures are not identical
Comment 54 Pedro Amorim 2025-10-29 19:26:23 UTC
Created attachment 188594 [details] [review]
Bug 40529: (QA follow-up): Add UNIQUE KEY entries to table definition

The UNIQUE KEY are required as a hold group 'might have' a target hold, not 'has many' target holds.
This update and the other things this patch updates in atomicupdate + kohastructure should  fix the error thrown by command:
perl /kohadevbox/misc4dev/run_tests.pl --instance=kohadev --db-password=password --run-db-compare-only --compare-with=HEAD

But they don't, the error still complains:
ERROR - DB structures are not identical

Even thought the produced table from atomicupdate and table on kohastructure are exactly the same, line for line.
Comment 55 Pedro Amorim 2025-10-29 19:26:26 UTC
Created attachment 188595 [details] [review]
Bug 40529: [DONT PUSH] Newly generated dbic
Comment 56 Pedro Amorim 2025-10-29 19:28:38 UTC
Joubu, can you please help? I must be missing something here but can't find it, this also shows ERROR - DB structures are not identical:

perl /kohadevbox/misc4dev/run_tests.pl --instance=kohadev --db-password=password --run-db-compare-only --compare-with=HEAD

See the message on last commit please.
Comment 57 Jonathan Druart 2025-11-03 11:16:50 UTC
(In reply to Pedro Amorim from comment #56)
> Joubu, can you please help? I must be missing something here but can't find
> it, this also shows ERROR - DB structures are not identical:
> 
> perl /kohadevbox/misc4dev/run_tests.pl --instance=kohadev
> --db-password=password --run-db-compare-only --compare-with=HEAD
> 
> See the message on last commit please.

> -- Table structure for table `hold_group_target_holds`
> --
> 
> DROP TABLE IF EXISTS `hold_group_target_holds`;
> /*!40101 SET @saved_cs_client     = @@character_set_client */;
> /*!40101 SET character_set_client = utf8mb4 */;
> CREATE TABLE `hold_group_target_holds` (
>   `hold_group_id` int(10) unsigned NOT NULL COMMENT 'foreign key, linking this to the hold_groups table',
>   `reserve_id` int(11) NOT NULL COMMENT 'foreign key, linking this to the reserves table',
>   PRIMARY KEY (`hold_group_id`,`reserve_id`),
>   UNIQUE KEY `uq_hold_group_target_holds_hold_group_id` (`hold_group_id`),
>   UNIQUE KEY `uq_hold_group_target_holds_reserve_id` (`reserve_id`),
>   CONSTRAINT `hold_group_target_holds_ibfk_1` FOREIGN KEY (`hold_group_id`) REFERENCES `hold_groups` (`hold_group_id`) ON DELETE CASCADE,
>   CONSTRAINT `hold_group_target_holds_ibfk_2` FOREIGN KEY (`reserve_id`) REFERENCES `reserves` (`reserve_id`) ON DELETE CASCADE
> ) ENGINE=InnoDB DEFAULT CHARSET=utf8mb4 COLLATE=utf8mb4_unicode_ci;
> /*!40101 SET character_set_client = @saved_cs_client */;
> 
> --
5094,5110d5111
< ) ENGINE=InnoDB DEFAULT CHARSET=utf8mb4 COLLATE=utf8mb4_unicode_ci;
< /*!40101 SET character_set_client = @saved_cs_client */;
< 
< --
< -- Table structure for table `hold_group_target_holds`
< --
< DROP TABLE IF EXISTS `hold_group_target_holds`;
< /*!40101 SET @saved_cs_client     = @@character_set_client */;
< /*!40101 SET character_set_client = utf8mb4 */;
< CREATE TABLE `hold_group_target_holds` (
<     `hold_group_id` int(10) unsigned NOT NULL COMMENT 'foreign key, linking this to the hold_groups table',
<     `reserve_id` int(11) NOT NULL COMMENT 'foreign key, linking this to the reserves table',
<     PRIMARY KEY (`hold_group_id`,`reserve_id`),
<     UNIQUE KEY `uq_hold_group_target_holds_hold_group_id` (`hold_group_id`),
<     UNIQUE KEY `uq_hold_group_target_holds_reserve_id` (`reserve_id`),
<     CONSTRAINT `hold_group_target_holds_ibfk_1` FOREIGN KEY (`hold_group_id`) REFERENCES `hold_groups` (`hold_group_id`) ON DELETE CASCADE,
<     CONSTRAINT `hold_group_target_holds_ibfk_2` FOREIGN KEY (`reserve_id`) REFERENCES `reserves` (`reserve_id`) ON DELETE CASCADE
ERROR - DB structures are not identical

This is the diff I get.

The table def is not at the correct place in kohastructure.sql, they are sorted alphabetically.
Comment 58 Jonathan Druart 2025-11-03 11:17:44 UTC
Note that I have skipped the 2 DBIC patches and regenerated the schema files.
Comment 59 Pedro Amorim 2025-11-03 14:12:05 UTC
Created attachment 188933 [details] [review]
Bug 40529: (QA follow-up): Fix DB changes

Add UNIQUE KEY entries to table definition
The UNIQUE KEY are required as a hold group 'might have' a target hold, not 'has many' target holds.
This update and the other things this patch updates in atomicupdate + kohastructure should  fix the error thrown by command:
perl /kohadevbox/misc4dev/run_tests.pl --instance=kohadev --db-password=password --run-db-compare-only --compare-with=HEAD

This command no longer produces any errors.

Also in this patch:
Renamed table from 'hold_group_target_holds' to 'hold_groups_target_holds' and move it alphabetically to be after 'hold_groups' in kohastructure.sql
This is cleaner, as it matches the 'hold_groups' related table, and allows for alphabetical sorting on kohastructure.sql that makes sense i.e. hold_groups is created first, then hold_group_target_holds is created second with an FK to the former.

All of this passes:
prove ./t/db_dependent/Koha/Hold.t
prove ./t/db_dependent/Reserves/HoldGroup.t
prove ./t/db_dependent/Circulation*
prove ./t/db_dependent/Hold*
prove ./t/db_dependent/Reserves*
Comment 60 Pedro Amorim 2025-11-03 14:12:08 UTC
Created attachment 188934 [details] [review]
Bug 40529: [DONT PUSH] Newly generated dbic
Comment 61 Pedro Amorim 2025-11-03 14:13:11 UTC
(In reply to Jonathan Druart from comment #57)
> 
> The table def is not at the correct place in kohastructure.sql, they are
> sorted alphabetically.

Thank you Joubu, I tunnel visioned and never spotted/considered this. I believe my last updated patch now fixes this, thanks to your comment!
Comment 62 Lucas Gass (lukeg) 2025-11-03 21:16:15 UTC
Nice work everyone!

Pushed to main for 25.11