Bug 29196 - Follow-up to Bug 27068 - Remove unnecessary check
Summary: Follow-up to Bug 27068 - Remove unnecessary check
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Hold requests (show other bugs)
Version: master
Hardware: All All
: P5 - low major (vote)
Assignee: Nick Clemens
QA Contact: Kyle M Hall
URL:
Keywords:
: 29219 (view as bug list)
Depends on:
Blocks:
 
Reported: 2021-10-08 13:39 UTC by Nick Clemens
Modified: 2023-06-08 22:28 UTC (History)
5 users (show)

See Also:
Change sponsored?: ---
Patch complexity: ---
Documentation contact:
Documentation submission:
Text to go in the release notes:
Version(s) released in:
22.11.00


Attachments
Bug 29196: Bug 27068 follow-up (2.17 KB, patch)
2021-10-08 13:51 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 29196: Follow-up to Bug 27068 - Remove unnecessary check (1.31 KB, patch)
2021-10-13 16:24 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 29196: Follow-up to Bug 27068 - Remove unnecessary check (1.35 KB, patch)
2021-10-22 20:45 UTC, David Nind
Details | Diff | Splinter Review
Bug 29196: Follow-up to Bug 27068 - Remove unnecessary check (2.37 KB, patch)
2022-09-21 19:50 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 29196: (Bug 27068 follow-up) - Remove unnecessary check (2.36 KB, patch)
2022-09-30 12:36 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 29196: (Bug 27068 follow-up) - Remove unnecessary check (2.42 KB, patch)
2022-09-30 13:46 UTC, Kyle M Hall
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Nick Clemens 2021-10-08 13:39:34 UTC
726                     # Don't fill item level holds that contravene the hold pickup policy at this time
727                     next unless $item->{hold_fulfillment_policy} eq 'any'
728                         || $request->{branchcode} eq $item->{ $item->{hold_fulfillment_policy} };


Should be:
# Don't fill item level holds that contravene the hold pickup policy at this time
next unless _checkHoldPolicy($item, $request);
Comment 1 Nick Clemens 2021-10-08 13:51:32 UTC
Created attachment 125963 [details] [review]
Bug 29196: Bug 27068 follow-up

Test plan stolen/updated from 27068

To test:
 1) In library groups add a root group and check it as hold group.
 2) Add two libraries to the group
 3) In circulation and fines rules, in 'Default checkout, hold and return
 policy', in Hold pickup library match change the value to 'From patron's hold group'
 4) Place a hold from a patron whose home library is from the group
 4.5) perl misc/cronjobs/holds/build_holdsqueue.pl
 5) Go to /cgi-bin/koha/circ/view_holdsqueue.pl
 6) Select the holding branch of the item with a hold
 7) observe no results
 8) Apply Patch
 9) Repeat 5-6
10) The item should come up on the holds queue results
11) Place a hold on an item where 1 record has 2 copies, 1 in the hold group, 1 not.
12) Run the HoldQueue for the library not in the group and make sure the hold isn't showing.
13) Turn on transportation cost matrix and set costs for the libraries within the group.
14) Place a hold for a patron where multiple copies are on the bib.
15) Check both branch's hold queue for the item, it should only show on the lower cost branch's list if both copies are available.
Comment 2 Kyle M Hall 2021-10-13 15:28:38 UTC
Nick and I talked about this. We are quite sure that line of code is superfluous and can be safely removed.
Comment 3 Kyle M Hall 2021-10-13 15:29:10 UTC
*** Bug 29219 has been marked as a duplicate of this bug. ***
Comment 4 Kyle M Hall 2021-10-13 16:24:39 UTC
Created attachment 126226 [details] [review]
Bug 29196: Follow-up to Bug 27068 - Remove unnecessary check

The introduction of _checkHoldPolicy has made this check superfluous.

Test Plan:
1) Apply this patch
2) prove t/db_dependent/HoldsQueue.t
Comment 5 David Nind 2021-10-22 20:45:03 UTC
Created attachment 126788 [details] [review]
Bug 29196: Follow-up to Bug 27068 - Remove unnecessary check

The introduction of _checkHoldPolicy has made this check superfluous.

Test Plan:
1) Apply this patch
2) prove t/db_dependent/HoldsQueue.t

Signed-off-by: David Nind <david@davidnind.com>
Comment 6 Joonas Kylmälä 2021-10-30 19:59:00 UTC
Kyle, please provide unit tests for the 'holdgroup' and 'patrongroup' cases to highlight this bug and fix the commit message to describe the bug (so basically copy&paste the commit message from Nick's patch)
Comment 7 Nick Clemens 2022-09-21 19:50:53 UTC
Created attachment 140832 [details] [review]
Bug 29196: Follow-up to Bug 27068 - Remove unnecessary check

The introduction of _checkHoldPolicy has made this check superfluous.
Test plan stolen/updated from 27068

To test:
 1) In library groups add a root group and check it as hold group.
 2) Add two libraries to the group
 3) In circulation and fines rules, in 'Default checkout, hold and return
 policy', in Hold pickup library match change the value to 'From patron's hold group'
 4) Place a hold from a patron whose home library is from the group
 4.5) perl misc/cronjobs/holds/build_holdsqueue.pl
 5) Go to /cgi-bin/koha/circ/view_holdsqueue.pl
 6) Select the holding branch of the item with a hold
 7) observe no results
 8) Apply Patch
 9) Repeat 5-6
10) The item should come up on the holds queue results
11) Place a hold on an item where 1 record has 2 copies, 1 in the hold group, 1 not.
12) Run the HoldQueue for the library not in the group and make sure the hold isn't showing.
13) Turn on transportation cost matrix and set costs for the libraries within the group.
14) Place a hold for a patron where multiple copies are on the bib.
15) Check both branch's hold queue for the item, it should only show on the lower cost branch's list if both copies are available.

Signed-off-by: David Nind <david@davidnind.com>
Comment 8 Nick Clemens 2022-09-21 19:51:24 UTC
(In reply to Joonas Kylmälä from comment #6)
> Kyle, please provide unit tests for the 'holdgroup' and 'patrongroup' cases
> to highlight this bug and fix the commit message to describe the bug (so
> basically copy&paste the commit message from Nick's patch)

Are tests from 27068 not sufficient?
Comment 9 Marcel de Rooy 2022-09-30 09:52:56 UTC
        * Commit title does not contain 'follow-up' correctly spelt - c61176ac2a
Comment 10 Nick Clemens 2022-09-30 12:36:28 UTC
Created attachment 141131 [details] [review]
Bug 29196: (Bug 27068 follow-up) - Remove unnecessary check

The introduction of _checkHoldPolicy has made this check superfluous.
Test plan stolen/updated from 27068

To test:
 1) In library groups add a root group and check it as hold group.
 2) Add two libraries to the group
 3) In circulation and fines rules, in 'Default checkout, hold and return
 policy', in Hold pickup library match change the value to 'From patron's hold group'
 4) Place a hold from a patron whose home library is from the group
 4.5) perl misc/cronjobs/holds/build_holdsqueue.pl
 5) Go to /cgi-bin/koha/circ/view_holdsqueue.pl
 6) Select the holding branch of the item with a hold
 7) observe no results
 8) Apply Patch
 9) Repeat 5-6
10) The item should come up on the holds queue results
11) Place a hold on an item where 1 record has 2 copies, 1 in the hold group, 1 not.
12) Run the HoldQueue for the library not in the group and make sure the hold isn't showing.
13) Turn on transportation cost matrix and set costs for the libraries within the group.
14) Place a hold for a patron where multiple copies are on the bib.
15) Check both branch's hold queue for the item, it should only show on the lower cost branch's list if both copies are available.

Signed-off-by: David Nind <david@davidnind.com>
Comment 11 Kyle M Hall 2022-09-30 13:46:00 UTC
Created attachment 141155 [details] [review]
Bug 29196: (Bug 27068 follow-up) - Remove unnecessary check

The introduction of _checkHoldPolicy has made this check superfluous.
Test plan stolen/updated from 27068

To test:
 1) In library groups add a root group and check it as hold group.
 2) Add two libraries to the group
 3) In circulation and fines rules, in 'Default checkout, hold and return
 policy', in Hold pickup library match change the value to 'From patron's hold group'
 4) Place a hold from a patron whose home library is from the group
 4.5) perl misc/cronjobs/holds/build_holdsqueue.pl
 5) Go to /cgi-bin/koha/circ/view_holdsqueue.pl
 6) Select the holding branch of the item with a hold
 7) observe no results
 8) Apply Patch
 9) Repeat 5-6
10) The item should come up on the holds queue results
11) Place a hold on an item where 1 record has 2 copies, 1 in the hold group, 1 not.
12) Run the HoldQueue for the library not in the group and make sure the hold isn't showing.
13) Turn on transportation cost matrix and set costs for the libraries within the group.
14) Place a hold for a patron where multiple copies are on the bib.
15) Check both branch's hold queue for the item, it should only show on the lower cost branch's list if both copies are available.

Signed-off-by: David Nind <david@davidnind.com>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 12 Tomás Cohen Arazi 2022-10-03 16:53:08 UTC
Pushed to master for 22.11.

Nice work everyone, thanks!