Bug 40529

Summary: Update how hold groups work
Product: Koha Reporter: Pedro Amorim <pedro.amorim>
Component: Hold requestsAssignee: Bugs List <koha-bugs>
Status: Signed Off --- QA Contact: Testopia <testopia>
Severity: enhancement    
Priority: P5 - low CC: andrew, anneli.osterman, gmcharlt, kebliss, lisette, martin.renvoize, pedro.amorim, tomascohen
Version: Main   
Hardware: All   
OS: All   
URL: https://github.com/openfifth/koha/commits/b_40529/
GIT URL: Change sponsored?: ---
Patch complexity: --- Documentation contact:
Documentation submission: Text to go in the release notes:
Version(s) released in:
Circulation function:
Bug Depends on: 40517    
Bug Blocks: 40552    
Attachments: Bug 40529: Add hold group targets logic
Bug 40529: Behavior change: Main patch
Bug 40529: Behavior change: Add target info on hold group modal
Bug 40529: Behavior change: Update old tests
Bug 40529: Add unit tests
Bug 40529: Add hold group targets logic
Bug 40529: Behavior change: Main patch
Bug 40529: Behavior change: Add target info on hold group modal
Bug 40529: Behavior change: Update old tests
Bug 40529: Add unit tests
Bug 40529: Add hold group targets logic
Bug 40529: Behavior change: Main patch
Bug 40529: Behavior change: Add target info on hold group modal
Bug 40529: Add unit tests
Bug 40529: (QA follow-up): Include target holds
Bug 40529: (QA follow-up): Remove hold as hold group target when reverted
Bug 40529: Behavior change: Main patch
Bug 40529: Behavior change: Add target info on hold group modal
Bug 40529: Add unit tests
Bug 40529: (QA follow-up): Include target holds
Bug 40529: (QA follow-up): Remove hold as hold group target when reverted

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.