https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=40529 Pedro Amorim <pedro.amorim@openfifth.co.uk> changed: What |Removed |Added ---------------------------------------------------------------------------- URL| |https://github.com/openfift | |h/koha/commits/b_40529/ -- You are receiving this mail because: You are the assignee for the bug. You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=40529 Pedro Amorim <pedro.amorim@openfifth.co.uk> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|NEW |Needs Signoff -- You are receiving this mail because: You are watching all bug changes. You are the assignee for the bug.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=40529 --- Comment #2 from Pedro Amorim <pedro.amorim@openfifth.co.uk> --- Created attachment 184869 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=184869&action=edit 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). -- You are receiving this mail because: You are the assignee for the bug. You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=40529 --- Comment #4 from Pedro Amorim <pedro.amorim@openfifth.co.uk> --- Created attachment 184871 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=184871&action=edit Bug 40529: Behavior change: Update old tests This is needed or the Reserves are created with a hold_group_id, which will create a related hold_group and hold_group_target_holds. As DisplayAddHoldGroups is considered to be enabled, it would otherwise skip non-target holds and cause these tests to fail. -- You are receiving this mail because: You are watching all bug changes. You are the assignee for the bug.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=40529 Katie Bliss <kebliss@dmpl.org> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |kebliss@dmpl.org -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=40529 Pedro Amorim <pedro.amorim@openfifth.co.uk> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Needs Signoff |Patch doesn't apply -- You are receiving this mail because: You are watching all bug changes. You are the assignee for the bug.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=40529 Pedro Amorim <pedro.amorim@openfifth.co.uk> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #184868|0 |1 is obsolete| | --- Comment #6 from Pedro Amorim <pedro.amorim@openfifth.co.uk> --- Created attachment 185137 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=185137&action=edit 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. -- You are receiving this mail because: You are watching all bug changes. You are the assignee for the bug.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=40529 Pedro Amorim <pedro.amorim@openfifth.co.uk> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #184870|0 |1 is obsolete| | --- Comment #8 from Pedro Amorim <pedro.amorim@openfifth.co.uk> --- Created attachment 185139 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=185139&action=edit Bug 40529: Behavior change: Add target info on hold group modal -- You are receiving this mail because: You are watching all bug changes. You are the assignee for the bug.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=40529 Pedro Amorim <pedro.amorim@openfifth.co.uk> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #184872|0 |1 is obsolete| | --- Comment #10 from Pedro Amorim <pedro.amorim@openfifth.co.uk> --- Created attachment 185141 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=185141&action=edit 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* -- You are receiving this mail because: You are watching all bug changes. You are the assignee for the bug.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=40529 Pedro Amorim <pedro.amorim@openfifth.co.uk> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #185138|0 |1 is obsolete| | --- Comment #12 from Pedro Amorim <pedro.amorim@openfifth.co.uk> --- Created attachment 185211 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=185211&action=edit 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). -- You are receiving this mail because: You are watching all bug changes. You are the assignee for the bug.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=40529 Pedro Amorim <pedro.amorim@openfifth.co.uk> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #185141|0 |1 is obsolete| | --- Comment #14 from Pedro Amorim <pedro.amorim@openfifth.co.uk> --- Created attachment 185213 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=185213&action=edit 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* -- You are receiving this mail because: You are watching all bug changes. You are the assignee for the bug.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=40529 --- Comment #16 from Pedro Amorim <pedro.amorim@openfifth.co.uk> --- Created attachment 185215 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=185215&action=edit 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 -- You are receiving this mail because: You are watching all bug changes. You are the assignee for the bug.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=40529 OpenFifth Sandboxes <sandboxes@openfifth.co.uk> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #185211|0 |1 is obsolete| | --- Comment #17 from OpenFifth Sandboxes <sandboxes@openfifth.co.uk> --- Created attachment 185303 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=185303&action=edit 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> -- You are receiving this mail because: You are the assignee for the bug. You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=40529 OpenFifth Sandboxes <sandboxes@openfifth.co.uk> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #185213|0 |1 is obsolete| | --- Comment #19 from OpenFifth Sandboxes <sandboxes@openfifth.co.uk> --- Created attachment 185305 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=185305&action=edit 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> -- You are receiving this mail because: You are watching all bug changes. You are the assignee for the bug.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=40529 OpenFifth Sandboxes <sandboxes@openfifth.co.uk> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #185215|0 |1 is obsolete| | --- Comment #21 from OpenFifth Sandboxes <sandboxes@openfifth.co.uk> --- Created attachment 185307 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=185307&action=edit 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> -- You are receiving this mail because: You are watching all bug changes. You are the assignee for the bug.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=40529 Andrew Fuerste-Henry <andrew@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Signed Off |Failed QA --- Comment #23 from Andrew Fuerste-Henry <andrew@bywatersolutions.com> --- 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. -- You are receiving this mail because: You are watching all bug changes. You are the assignee for the bug.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=40529 Pedro Amorim <pedro.amorim@openfifth.co.uk> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Failed QA |Patch doesn't apply --- Comment #24 from Pedro Amorim <pedro.amorim@openfifth.co.uk> --- (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. -- You are receiving this mail because: You are the assignee for the bug. You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=40529 Pedro Amorim <pedro.amorim@openfifth.co.uk> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #185210|0 |1 is obsolete| | --- Comment #25 from Pedro Amorim <pedro.amorim@openfifth.co.uk> --- Created attachment 186793 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=186793&action=edit 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. -- You are receiving this mail because: You are the assignee for the bug. You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=40529 Pedro Amorim <pedro.amorim@openfifth.co.uk> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #185304|0 |1 is obsolete| | --- Comment #27 from Pedro Amorim <pedro.amorim@openfifth.co.uk> --- Created attachment 186795 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=186795&action=edit Bug 40529: Behavior change: Add target info on hold group modal Signed-off-by: Anneli Österman <anneli.osterman@koha-suomi.fi> -- You are receiving this mail because: You are the assignee for the bug. You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=40529 Pedro Amorim <pedro.amorim@openfifth.co.uk> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #185306|0 |1 is obsolete| | --- Comment #29 from Pedro Amorim <pedro.amorim@openfifth.co.uk> --- Created attachment 186797 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=186797&action=edit 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> -- You are receiving this mail because: You are watching all bug changes. You are the assignee for the bug.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=40529 Pedro Amorim <pedro.amorim@openfifth.co.uk> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Needs Signoff |Failed QA -- You are receiving this mail because: You are watching all bug changes. You are the assignee for the bug.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=40529 Pedro Amorim <pedro.amorim@openfifth.co.uk> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Failed QA |Signed Off -- You are receiving this mail because: You are the assignee for the bug. You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=40529 --- Comment #33 from Pedro Amorim <pedro.amorim@openfifth.co.uk> --- Created attachment 186804 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=186804&action=edit 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 -- You are receiving this mail because: You are watching all bug changes. You are the assignee for the bug.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=40529 Andrew Fuerste-Henry <andrew@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Signed Off |Passed QA -- You are receiving this mail because: You are the assignee for the bug. You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=40529 Andrew Fuerste-Henry <andrew@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #186794|0 |1 is obsolete| | --- Comment #36 from Andrew Fuerste-Henry <andrew@bywatersolutions.com> --- Created attachment 186826 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=186826&action=edit 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> -- You are receiving this mail because: You are the assignee for the bug. You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=40529 Andrew Fuerste-Henry <andrew@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #186796|0 |1 is obsolete| | --- Comment #38 from Andrew Fuerste-Henry <andrew@bywatersolutions.com> --- Created attachment 186828 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=186828&action=edit 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> -- You are receiving this mail because: You are watching all bug changes. You are the assignee for the bug.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=40529 Andrew Fuerste-Henry <andrew@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #186798|0 |1 is obsolete| | --- Comment #40 from Andrew Fuerste-Henry <andrew@bywatersolutions.com> --- Created attachment 186830 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=186830&action=edit 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> -- You are receiving this mail because: You are watching all bug changes. You are the assignee for the bug.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=40529 Andrew Fuerste-Henry <andrew@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #186804|0 |1 is obsolete| | --- Comment #42 from Andrew Fuerste-Henry <andrew@bywatersolutions.com> --- Created attachment 186832 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=186832&action=edit 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> -- You are receiving this mail because: You are watching all bug changes. You are the assignee for the bug.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=40529 Lucas Gass (lukeg) <lucas@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Passed QA |BLOCKED -- You are receiving this mail because: You are the assignee for the bug. You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=40529 Pedro Amorim <pedro.amorim@openfifth.co.uk> changed: What |Removed |Added ---------------------------------------------------------------------------- Keywords| |rel_25_11_candidate -- You are receiving this mail because: You are watching all bug changes. You are the assignee for the bug.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=40529 Pedro Amorim <pedro.amorim@openfifth.co.uk> changed: What |Removed |Added ---------------------------------------------------------------------------- Assignee|koha-bugs@lists.koha-commun |pedro.amorim@openfifth.co.u |ity.org |k -- You are receiving this mail because: You are the assignee for the bug. You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=40529 Pedro Amorim <pedro.amorim@openfifth.co.uk> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #186826|0 |1 is obsolete| | --- Comment #44 from Pedro Amorim <pedro.amorim@openfifth.co.uk> --- Created attachment 188349 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=188349&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=40529 Pedro Amorim <pedro.amorim@openfifth.co.uk> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #186828|0 |1 is obsolete| | --- Comment #46 from Pedro Amorim <pedro.amorim@openfifth.co.uk> --- Created attachment 188351 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=188351&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=40529 Pedro Amorim <pedro.amorim@openfifth.co.uk> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #186830|0 |1 is obsolete| | --- Comment #48 from Pedro Amorim <pedro.amorim@openfifth.co.uk> --- Created attachment 188353 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=188353&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=40529 Pedro Amorim <pedro.amorim@openfifth.co.uk> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #186832|0 |1 is obsolete| | --- Comment #50 from Pedro Amorim <pedro.amorim@openfifth.co.uk> --- Created attachment 188355 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=188355&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=40529 --- Comment #52 from Pedro Amorim <pedro.amorim@openfifth.co.uk> --- 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. -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=40529 --- Comment #54 from Pedro Amorim <pedro.amorim@openfifth.co.uk> --- Created attachment 188594 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=188594&action=edit 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. -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=40529 Pedro Amorim <pedro.amorim@openfifth.co.uk> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |jonathan.druart@gmail.com --- Comment #56 from Pedro Amorim <pedro.amorim@openfifth.co.uk> --- 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. -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=40529 --- Comment #58 from Jonathan Druart <jonathan.druart@gmail.com> --- Note that I have skipped the 2 DBIC patches and regenerated the schema files. -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=40529 Pedro Amorim <pedro.amorim@openfifth.co.uk> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #188595|0 |1 is obsolete| | -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=40529 --- Comment #59 from Pedro Amorim <pedro.amorim@openfifth.co.uk> --- Created attachment 188933 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=188933&action=edit 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* -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=40529 --- Comment #61 from Pedro Amorim <pedro.amorim@openfifth.co.uk> --- (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! -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=40529 --- Comment #62 from Lucas Gass (lukeg) <lucas@bywatersolutions.com> --- Nice work everyone! Pushed to main for 25.11 -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=40529 Lucas Gass (lukeg) <lucas@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Keywords|rel_25_11_candidate, | |release-notes-needed | -- You are receiving this mail because: You are watching all bug changes.