Description
Kyle M Hall (khall)
2019-02-07 08:00:43 UTC
I know you probably just wanted the bug number, but to avoid duplicates and because I am curious... can we get a bit more description? :) This development will add pickup location groups. 1. Add a new feature checkbox to groups feature to define 'local hold groups' 2. Hold policy in circ rules - new option 'from local hold group' [effectively act as homelibrary + libraries in groups you are in] 2a. This will apply per item (C4::Reserves::CanItemBeReserved) 2b. possibly add a system preference to set home or holding as control 2c. Will check patron's homelibrary to see if possible 3. On checkin, hold policy will be checked to see if hold can be filled according to policy in 2a (dropping through all holds to find highest priority that can be satisfied) Thx, Tomas! *** Bug 20747 has been marked as a duplicate of this bug. *** Created attachment 88138 [details] [review] Bug 22284: New ft_local_hold_group column, and 'holgroup' enum option In order to add groups for holds, this patch provides a new column in library_groups to identify them. Also provides new enum option for hold_fulfillment_policy column of default_branch_circ_rules, default_circ_rules, default_branch_item_rules and branch_item_rules. To test: 1) check that there is no ft_local_hold_group column in library_groups and that there are no 'holdgroup' enum option in hold_fulfillment_policy in default_branch_circ_rules, default_circ_rules, default_branch_item_rules and branch_item_rules 2) apply this patch 3) run perl installer/data/mysql/updatedatabase.pl SUCCESS => !(step 1) ;-D 4) Sign off Sponsored-by: VOKAL Created attachment 88139 [details] [review] Bug 22284: (follow-up) Add schema modifications Sponsored-by: VOKAL Created attachment 88140 [details] [review] Bug 22284: Add new checkbox in Library Groups This patch adds a new checkbox option 'Is local hold group' when you create a new root group. To test: 1) In intranet go to Library Groups in admin module 2) Add new root group 3) Check that there is no 'Is local hold group' checkbox 4) Apply this patch 5) Add new root group SUCCESS => a new 'Is local hold group' checkbox appears 6) Select 'Is local hold group' and save 7) Click edit button for root group SUCCESS => 'Is local hold group' is checked 8) Sign off Sponsored-by: VOKAL Created attachment 88141 [details] [review] Bug 22284: New methods in Koha::Library::Groups and Koha::Library This patch adds new methods in Koha::Library::Groups and Koha::Library. 1) For Koha::Library::Groups adds get_root_ancestor that returns all root groups for a given search parameters, for example Koha::Library::Groups->get_root_ancestor( { id => $group_id } ) 2) For Koha::Library adds 2.1) get_hold_libraries: returns all libraries (including self) that belongs to the same holdgroups. If $self belongs to several holdgroups it will return a distinct list of all libraries belonging to them. 2.2) validate_hold_sibling: Returns 1 if the given parameters matches any of the libraries that belong to any of the holdgroups this library belongs. For example $library->validate_hold_sibling( { branchcode => $branchcode } ) To test: 1) apply this patch 2) prove t/db_dependent/Koha/Libraries.t t/db_dependent/LibraryGroups.t SUCCESS => green letters :-D 3) Sign off Sponsored-by: VOKAL Created attachment 88142 [details] [review] Bug 22284: New hold group options in smart-rules This patch adds new options for 'Default checkout, hold and return policy' and 'Default holds policy by item type', both in 'Hold policy' and 'Hold pickup library match' columns To test: 1) Go to 'Circulation and fines rules' in admin 2) Check that there are no options for hold group in 'Hold policy' and 'Hold pickup library match' columns 3) Apply this patch SUCCESS => in 'Hold policy' you should see 'From local hold group' option, and in 'Hold pickup library match' you should see "Item's hold group" 4) Select any of those options and save SUCCESS => options should be selected when page reloads 5) Sign off Sponsored-by: VOKAL Created attachment 88143 [details] [review] Bug 22284: Control hold groups in C4::Reserves This patch modifies C4::Reserves to control when hold group options where selected in smart rules. In CanItemBeReserved adds 2 new error status messages 1) branchNotInHoldGroup: when a patron's homebranch is not in item's hold group 2) pickupNotInHoldGroup: when a selected pickup location is not in item's hold group Also CheckReserves is modified when item's priority is defined, to control by hold group when required. Finally, IsAvailableForItemLevelRequest was also modified to control by hold group when required. To test: 1) Apply this patch 2) prove t/db_dependent/Holds.t SUCCESS => Result: PASS 3) Sign off Sponsored-by: VOKAL Created attachment 88144 [details] [review] Bug 22284: New messages in reserve/request.tt This patch adds new messages to 'Hold' column in 'Place a hold on a specific item' table Those messages are 1) Cannot place hold from patrons's library: this message appears when patron's homebranch is not in item's hold group 2) Pickup library is not in hold group: self explanatory To test: 1) In library groups add a root group and check it as hold group. 2) Add a library to the group 3) In circulation and fines rules, in 'Default checkout, hold and return policy', in Hold policy change the value to 'From local hold group' 4) Search a patron from a different library than step 2, select one and click 'search to hold' 5) Search by location for items in the library of step 2 6) On any item, clic on 'Place hold for ...' SUCCESS => when the page is loaded, in the 'Place a hold on a specific item', you should see the message "Cannot place hold from patrons's library" in 'Hold' column 7) In circulation and fines rules, in 'Default checkout, hold and return policy', in 'Hold policy' change the value again to 'From any library' and change 'Hold pickup library match' to "Item's hold group" 8) Repeat steps 4 to 6 SUCCESS => when the page is loaded, in the 'Place a hold on a specific item', you should see the message "Pickup library is not in hold group" in 'Hold' column 9) Sign off Sponsored-by: VOKAL Created attachment 88145 [details] [review] Bug 22284: Opac pickup_locations This patch modifies Koha::Libraries->pickup_location and moves most of the logic to Koha::Item and Koha::Biblio in preparation for api endpoints in the future. There where 2 methods added 1) Koha::Item->pickup_locations that given a patron, returns all pickup locations of this item, considering hold fulfillment rules, and hold allowed rules. 2) Koha::Biblio->pickup_locations that given a patron, returns a distinct list of libraries returned by each of this biblio items pickup location. Koha::Libraries->pickup_location analyzes input param and calls Koha::Item->pickup_locations or Koha::Biblio->pickup_locations as needed. Also in opac-reserve.tt the way options where obtained to fill the pickup location select was modified to pass the patron as a parameter. To test: 1) opac: try to place hold on a item and check that all libraries are shown in the pickup location select. 2) intranet: in Library groups, add 2 root groups marked as local hold group and add different libraries to each. 3) opac: login as a user of a library belonging to one hold group, and search try to place a hold on an item belongin to the other hold group. 4) intranet: in Circulation and fines rules, play with 'Hold policy' and 'Hold pickup library match' rules. 5) opac: On each modification of the rules reload the page. SUCCESS => Every time you reload the page, the number of pickup locations showed in select varies. 6) prove t/db_dependent/Koha/Biblios.t t/db_dependent/Koha/Items.t SUCCESS => Result: PASS 7) Sign off Sponsored-by: VOKAL Comment on attachment 88141 [details] [review] Bug 22284: New methods in Koha::Library::Groups and Koha::Library Review of attachment 88141 [details] [review]: ----------------------------------------------------------------- ::: Koha/Library.pm @@ +26,5 @@ > use Koha::Database; > use Koha::StockRotationStages; > +use Koha::Libraries; > + > +use Data::Dumper qw(Dumper); I think this needs to be removed. @@ +115,5 @@ > + my @hold_libraries = $self->get_hold_libraries; > + > + foreach (@hold_libraries) { > + my $hold_library = $_; > + my $cond = 1; Variable name isn't very self explanatory. What condition? ::: t/db_dependent/LibraryGroups.t @@ +9,5 @@ > use t::lib::TestBuilder; > use Koha::Database; > > +use Data::Dumper; > + I think this also needs to be removed. Created attachment 88369 [details] [review] Bug 22284: New methods in Koha::Library::Groups and Koha::Library This patch adds new methods in Koha::Library::Groups and Koha::Library. 1) For Koha::Library::Groups adds get_root_ancestor that returns all root groups for a given search parameters, for example Koha::Library::Groups->get_root_ancestor( { id => $group_id } ) 2) For Koha::Library adds 2.1) get_hold_libraries: returns all libraries (including self) that belongs to the same holdgroups. If $self belongs to several holdgroups it will return a distinct list of all libraries belonging to them. 2.2) validate_hold_sibling: Returns 1 if the given parameters matches any of the libraries that belong to any of the holdgroups this library belongs. For example $library->validate_hold_sibling( { branchcode => $branchcode } ) To test: 1) apply this patch 2) prove t/db_dependent/Koha/Libraries.t t/db_dependent/LibraryGroups.t SUCCESS => green letters :-D 3) Sign off Sponsored-by: VOKAL Hello Kyle, Unused "use" sentences removed and clarified the variable. Sorry about that. Thanks. Hi Augustín, I started to testing, overall it is looking good, I will continue on that, but just found some issues, which should be addressed: prove t/db_dependent/Reserves.t # Failed test 'CheckReserves Test 1' # at t/db_dependent/Reserves.t line 132. # got: '' # expected: 'Reserved' # Failed test 'CheckReserves() include reserve_id in its response' # at t/db_dependent/Reserves.t line 134. # Failed test 'CheckReserves Test 2' # at t/db_dependent/Reserves.t line 137. # got: '' # expected: 'Reserved' # Failed test 'CheckReserves Test 3' # at t/db_dependent/Reserves.t line 140. # got: '' # expected: 'Reserved' t/db_dependent/Reserves.t .................... 59/60 # Looks like you failed 4 tests of 60. t/db_dependent/Reserves.t .................... Dubious, test returned 4 (wstat 1024, 0x400) Failed 4/60 subtests prove t/db_dependent/Holds.t # Failed test '$branch_1 patron allowed to reserve $branch_2 item with IndependentBranches OFF (bug 2394)' # at t/db_dependent/Holds.t line 270. # Failed test '$branch_1 patron NOT allowed to reserve $branch_2 item with IndependentBranches ON ... (bug 2394)' # at t/db_dependent/Holds.t line 278. # Failed test '... unless canreservefromotherbranches is ON (bug 2394)' # at t/db_dependent/Holds.t line 285. t/db_dependent/Holds.t .. 41/60 # Failed test 'Hold can be trapped for damaged item with AllowHoldsOnDamagedItems enabled' # at t/db_dependent/Holds.t line 309. t/db_dependent/Holds.t .. 53/60 # Failed test 'Patron can reserve item with hold limit of 1, no holds placed' # at t/db_dependent/Holds.t line 594. # Structures begin differing at: # $got->{status} = 'branchNotInHoldGroup' # $expected->{status} = 'OK' # Failed test 'Patron can reserve item with 2 reserves daily cap' # at t/db_dependent/Holds.t line 611. # Structures begin differing at: # $got->{status} = 'branchNotInHoldGroup' # $expected->{status} = 'OK' # Failed test 'Patron can reserve item with 2 bib level hold placed on different days, 2 reserves daily cap' # at t/db_dependent/Holds.t line 630. # Structures begin differing at: # $got->{status} = 'branchNotInHoldGroup' # $expected->{status} = 'OK' # Failed test 'Patron can reserve if holds_per_day is undef (i.e. undef is unlimited daily cap)' # at t/db_dependent/Holds.t line 649. # Structures begin differing at: # $got->{status} = 'branchNotInHoldGroup' # $expected->{status} = 'OK' # Failed test 'Patron can reserve if holds_per_day is undef (i.e. undef is unlimited daily cap)' # at t/db_dependent/Holds.t line 656. # Structures begin differing at: # $got->{status} = 'branchNotInHoldGroup' # $expected->{status} = 'OK' # Looks like you failed 5 tests of 10. t/db_dependent/Holds.t .. 58/60 # Failed test 'CanItemBeReserved / holds_per_day tests' # at t/db_dependent/Holds.t line 676. t/db_dependent/Holds.t .. 60/60 # Looks like you failed 5 tests of 60. t/db_dependent/Holds.t .. Dubious, test returned 5 (wstat 1280, 0x500) Failed 5/60 subtests The tests needs to reflect changes in C4::Reserves. The Comment 13 is not reflected yet. Comment on attachment 88145 [details] [review] Bug 22284: Opac pickup_locations Review of attachment 88145 [details] [review]: ----------------------------------------------------------------- ::: Koha/Item.pm @@ +297,5 @@ > + > + my $branch_control = C4::Context->preference('HomeOrHoldingBranch'); > + my $library = $branch_control eq 'holdingbranch' ? $self->holding_branch : $self->home_branch; > + > + #warn $branch_control.' '.$branchitemrule->{holdallowed}.' '.$library->branchcode.' '.$patron->branchcode; This line should be removed Created attachment 88561 [details] [review] Bug 22284: (follow-up) Remove commented warn and address test failures (In reply to Josef Moravec from comment #17) > Comment on attachment 88145 [details] [review] [review] > Bug 22284: Opac pickup_locations > > Review of attachment 88145 [details] [review] [review]: > ----------------------------------------------------------------- > > ::: Koha/Item.pm > @@ +297,5 @@ > > + > > + my $branch_control = C4::Context->preference('HomeOrHoldingBranch'); > > + my $library = $branch_control eq 'holdingbranch' ? $self->holding_branch : $self->home_branch; > > + > > + #warn $branch_control.' '.$branchitemrule->{holdallowed}.' '.$library->branchcode.' '.$patron->branchcode; > > This line should be removed Hello Josef, Thanks for taking the Time to test. Issues where addressed in follow-up. Regarding comment #13, it was resolved in attachment 88369 [details] [review] Created attachment 88793 [details] [review] Bug 22284: New ft_local_hold_group column, and 'holgroup' enum option In order to add groups for holds, this patch provides a new column in library_groups to identify them. Also provides new enum option for hold_fulfillment_policy column of default_branch_circ_rules, default_circ_rules, default_branch_item_rules and branch_item_rules. To test: 1) check that there is no ft_local_hold_group column in library_groups and that there are no 'holdgroup' enum option in hold_fulfillment_policy in default_branch_circ_rules, default_circ_rules, default_branch_item_rules and branch_item_rules 2) apply this patch 3) run perl installer/data/mysql/updatedatabase.pl SUCCESS => !(step 1) ;-D 4) Sign off Sponsored-by: VOKAL Created attachment 88794 [details] [review] Bug 22284: (follow-up) Add schema modifications Sponsored-by: VOKAL Created attachment 88795 [details] [review] Bug 22284: Add new checkbox in Library Groups This patch adds a new checkbox option 'Is local hold group' when you create a new root group. To test: 1) In intranet go to Library Groups in admin module 2) Add new root group 3) Check that there is no 'Is local hold group' checkbox 4) Apply this patch 5) Add new root group SUCCESS => a new 'Is local hold group' checkbox appears 6) Select 'Is local hold group' and save 7) Click edit button for root group SUCCESS => 'Is local hold group' is checked 8) Sign off Sponsored-by: VOKAL Created attachment 88796 [details] [review] Bug 22284: New hold group options in smart-rules This patch adds new options for 'Default checkout, hold and return policy' and 'Default holds policy by item type', both in 'Hold policy' and 'Hold pickup library match' columns To test: 1) Go to 'Circulation and fines rules' in admin 2) Check that there are no options for hold group in 'Hold policy' and 'Hold pickup library match' columns 3) Apply this patch SUCCESS => in 'Hold policy' you should see 'From local hold group' option, and in 'Hold pickup library match' you should see "Item's hold group" 4) Select any of those options and save SUCCESS => options should be selected when page reloads 5) Sign off Sponsored-by: VOKAL Created attachment 88797 [details] [review] Bug 22284: Control hold groups in C4::Reserves This patch modifies C4::Reserves to control when hold group options where selected in smart rules. In CanItemBeReserved adds 2 new error status messages 1) branchNotInHoldGroup: when a patron's homebranch is not in item's hold group 2) pickupNotInHoldGroup: when a selected pickup location is not in item's hold group Also CheckReserves is modified when item's priority is defined, to control by hold group when required. Finally, IsAvailableForItemLevelRequest was also modified to control by hold group when required. To test: 1) Apply this patch 2) prove t/db_dependent/Holds.t SUCCESS => Result: PASS 3) Sign off Sponsored-by: VOKAL Created attachment 88798 [details] [review] Bug 22284: New messages in reserve/request.tt This patch adds new messages to 'Hold' column in 'Place a hold on a specific item' table Those messages are 1) Cannot place hold from patrons's library: this message appears when patron's homebranch is not in item's hold group 2) Pickup library is not in hold group: self explanatory To test: 1) In library groups add a root group and check it as hold group. 2) Add a library to the group 3) In circulation and fines rules, in 'Default checkout, hold and return policy', in Hold policy change the value to 'From local hold group' 4) Search a patron from a different library than step 2, select one and click 'search to hold' 5) Search by location for items in the library of step 2 6) On any item, clic on 'Place hold for ...' SUCCESS => when the page is loaded, in the 'Place a hold on a specific item', you should see the message "Cannot place hold from patrons's library" in 'Hold' column 7) In circulation and fines rules, in 'Default checkout, hold and return policy', in 'Hold policy' change the value again to 'From any library' and change 'Hold pickup library match' to "Item's hold group" 8) Repeat steps 4 to 6 SUCCESS => when the page is loaded, in the 'Place a hold on a specific item', you should see the message "Pickup library is not in hold group" in 'Hold' column 9) Sign off Sponsored-by: VOKAL Created attachment 88799 [details] [review] Bug 22284: Opac pickup_locations This patch modifies Koha::Libraries->pickup_location and moves most of the logic to Koha::Item and Koha::Biblio in preparation for api endpoints in the future. There where 2 methods added 1) Koha::Item->pickup_locations that given a patron, returns all pickup locations of this item, considering hold fulfillment rules, and hold allowed rules. 2) Koha::Biblio->pickup_locations that given a patron, returns a distinct list of libraries returned by each of this biblio items pickup location. Koha::Libraries->pickup_location analyzes input param and calls Koha::Item->pickup_locations or Koha::Biblio->pickup_locations as needed. Also in opac-reserve.tt the way options where obtained to fill the pickup location select was modified to pass the patron as a parameter. To test: 1) opac: try to place hold on a item and check that all libraries are shown in the pickup location select. 2) intranet: in Library groups, add 2 root groups marked as local hold group and add different libraries to each. 3) opac: login as a user of a library belonging to one hold group, and search try to place a hold on an item belongin to the other hold group. 4) intranet: in Circulation and fines rules, play with 'Hold policy' and 'Hold pickup library match' rules. 5) opac: On each modification of the rules reload the page. SUCCESS => Every time you reload the page, the number of pickup locations showed in select varies. 6) prove t/db_dependent/Koha/Biblios.t t/db_dependent/Koha/Items.t SUCCESS => Result: PASS 7) Sign off Sponsored-by: VOKAL Created attachment 88800 [details] [review] Bug 22284: New methods in Koha::Library::Groups and Koha::Library This patch adds new methods in Koha::Library::Groups and Koha::Library. 1) For Koha::Library::Groups adds get_root_ancestor that returns all root groups for a given search parameters, for example Koha::Library::Groups->get_root_ancestor( { id => $group_id } ) 2) For Koha::Library adds 2.1) get_hold_libraries: returns all libraries (including self) that belongs to the same holdgroups. If $self belongs to several holdgroups it will return a distinct list of all libraries belonging to them. 2.2) validate_hold_sibling: Returns 1 if the given parameters matches any of the libraries that belong to any of the holdgroups this library belongs. For example $library->validate_hold_sibling( { branchcode => $branchcode } ) To test: 1) apply this patch 2) prove t/db_dependent/Koha/Libraries.t t/db_dependent/LibraryGroups.t SUCCESS => green letters :-D 3) Sign off Sponsored-by: VOKAL Created attachment 88801 [details] [review] Bug 22284: (follow-up) Remove commented warn and address test failures Created attachment 88802 [details] [review] Bug 22284: (follow-up) fix test count after merge (In reply to Agustín Moyano from comment #29) > Created attachment 88802 [details] [review] [review] > Bug 22284: (follow-up) fix test count after merge Bad bug title.. wasn't merge.. was rebase Sorry. Hi Agustin, I seem to be getting test failures like these on these current patches, have a look please? Functionally, the dev seems to be working up to this point of testing, so don't stress too much about that. :) Cheers, Liz (In reply to Josef Moravec from comment #16) > Hi Augustín, > I started to testing, overall it is looking good, I will continue on that, > but just found some issues, which should be addressed: > > prove t/db_dependent/Reserves.t > > # Failed test 'CheckReserves Test 1' > # at t/db_dependent/Reserves.t line 132. > # got: '' > # expected: 'Reserved' > > # Failed test 'CheckReserves() include reserve_id in its response' > # at t/db_dependent/Reserves.t line 134. > > # Failed test 'CheckReserves Test 2' > # at t/db_dependent/Reserves.t line 137. > # got: '' > # expected: 'Reserved' > > # Failed test 'CheckReserves Test 3' > # at t/db_dependent/Reserves.t line 140. > # got: '' > # expected: 'Reserved' > t/db_dependent/Reserves.t .................... 59/60 # Looks like you failed > 4 tests of 60. > t/db_dependent/Reserves.t .................... Dubious, test returned 4 > (wstat 1024, 0x400) > Failed 4/60 subtests > > > prove t/db_dependent/Holds.t > # Failed test '$branch_1 patron allowed to reserve $branch_2 item with > IndependentBranches OFF (bug 2394)' > # at t/db_dependent/Holds.t line 270. > > # Failed test '$branch_1 patron NOT allowed to reserve $branch_2 item with > IndependentBranches ON ... (bug 2394)' > # at t/db_dependent/Holds.t line 278. > > # Failed test '... unless canreservefromotherbranches is ON (bug 2394)' > # at t/db_dependent/Holds.t line 285. > t/db_dependent/Holds.t .. 41/60 > # Failed test 'Hold can be trapped for damaged item with > AllowHoldsOnDamagedItems enabled' > # at t/db_dependent/Holds.t line 309. > t/db_dependent/Holds.t .. 53/60 > # Failed test 'Patron can reserve item with hold limit of 1, no holds > placed' > # at t/db_dependent/Holds.t line 594. > # Structures begin differing at: > # $got->{status} = 'branchNotInHoldGroup' > # $expected->{status} = 'OK' > > # Failed test 'Patron can reserve item with 2 reserves daily cap' > # at t/db_dependent/Holds.t line 611. > # Structures begin differing at: > # $got->{status} = 'branchNotInHoldGroup' > # $expected->{status} = 'OK' > > # Failed test 'Patron can reserve item with 2 bib level hold placed on > different days, 2 reserves daily cap' > # at t/db_dependent/Holds.t line 630. > # Structures begin differing at: > # $got->{status} = 'branchNotInHoldGroup' > # $expected->{status} = 'OK' > > # Failed test 'Patron can reserve if holds_per_day is undef (i.e. > undef is unlimited daily cap)' > # at t/db_dependent/Holds.t line 649. > # Structures begin differing at: > # $got->{status} = 'branchNotInHoldGroup' > # $expected->{status} = 'OK' > > # Failed test 'Patron can reserve if holds_per_day is undef (i.e. > undef is unlimited daily cap)' > # at t/db_dependent/Holds.t line 656. > # Structures begin differing at: > # $got->{status} = 'branchNotInHoldGroup' > # $expected->{status} = 'OK' > # Looks like you failed 5 tests of 10. > t/db_dependent/Holds.t .. 58/60 > # Failed test 'CanItemBeReserved / holds_per_day tests' > # at t/db_dependent/Holds.t line 676. > t/db_dependent/Holds.t .. 60/60 # Looks like you failed 5 tests of 60. > t/db_dependent/Holds.t .. Dubious, test returned 5 (wstat 1280, 0x500) > Failed 5/60 subtests > > The tests needs to reflect changes in C4::Reserves. > > The Comment 13 is not reflected yet. (In reply to Liz Rea from comment #31) > Hi Agustin, > > I seem to be getting test failures like these on these current patches, have > a look please? > > Functionally, the dev seems to be working up to this point of testing, so > don't stress too much about that. :) > > Cheers, > Liz > > (In reply to Josef Moravec from comment #16) > > Hi Augustín, > > I started to testing, overall it is looking good, I will continue on that, > > but just found some issues, which should be addressed: > > > > prove t/db_dependent/Reserves.t > > > > # Failed test 'CheckReserves Test 1' > > # at t/db_dependent/Reserves.t line 132. > > # got: '' > > # expected: 'Reserved' > > > > # Failed test 'CheckReserves() include reserve_id in its response' > > # at t/db_dependent/Reserves.t line 134. > > > > # Failed test 'CheckReserves Test 2' > > # at t/db_dependent/Reserves.t line 137. > > # got: '' > > # expected: 'Reserved' > > > > # Failed test 'CheckReserves Test 3' > > # at t/db_dependent/Reserves.t line 140. > > # got: '' > > # expected: 'Reserved' > > t/db_dependent/Reserves.t .................... 59/60 # Looks like you failed > > 4 tests of 60. > > t/db_dependent/Reserves.t .................... Dubious, test returned 4 > > (wstat 1024, 0x400) > > Failed 4/60 subtests > > > > > > prove t/db_dependent/Holds.t > > # Failed test '$branch_1 patron allowed to reserve $branch_2 item with > > IndependentBranches OFF (bug 2394)' > > # at t/db_dependent/Holds.t line 270. > > > > # Failed test '$branch_1 patron NOT allowed to reserve $branch_2 item with > > IndependentBranches ON ... (bug 2394)' > > # at t/db_dependent/Holds.t line 278. > > > > # Failed test '... unless canreservefromotherbranches is ON (bug 2394)' > > # at t/db_dependent/Holds.t line 285. > > t/db_dependent/Holds.t .. 41/60 > > # Failed test 'Hold can be trapped for damaged item with > > AllowHoldsOnDamagedItems enabled' > > # at t/db_dependent/Holds.t line 309. > > t/db_dependent/Holds.t .. 53/60 > > # Failed test 'Patron can reserve item with hold limit of 1, no holds > > placed' > > # at t/db_dependent/Holds.t line 594. > > # Structures begin differing at: > > # $got->{status} = 'branchNotInHoldGroup' > > # $expected->{status} = 'OK' > > > > # Failed test 'Patron can reserve item with 2 reserves daily cap' > > # at t/db_dependent/Holds.t line 611. > > # Structures begin differing at: > > # $got->{status} = 'branchNotInHoldGroup' > > # $expected->{status} = 'OK' > > > > # Failed test 'Patron can reserve item with 2 bib level hold placed on > > different days, 2 reserves daily cap' > > # at t/db_dependent/Holds.t line 630. > > # Structures begin differing at: > > # $got->{status} = 'branchNotInHoldGroup' > > # $expected->{status} = 'OK' > > > > # Failed test 'Patron can reserve if holds_per_day is undef (i.e. > > undef is unlimited daily cap)' > > # at t/db_dependent/Holds.t line 649. > > # Structures begin differing at: > > # $got->{status} = 'branchNotInHoldGroup' > > # $expected->{status} = 'OK' > > > > # Failed test 'Patron can reserve if holds_per_day is undef (i.e. > > undef is unlimited daily cap)' > > # at t/db_dependent/Holds.t line 656. > > # Structures begin differing at: > > # $got->{status} = 'branchNotInHoldGroup' > > # $expected->{status} = 'OK' > > # Looks like you failed 5 tests of 10. > > t/db_dependent/Holds.t .. 58/60 > > # Failed test 'CanItemBeReserved / holds_per_day tests' > > # at t/db_dependent/Holds.t line 676. > > t/db_dependent/Holds.t .. 60/60 # Looks like you failed 5 tests of 60. > > t/db_dependent/Holds.t .. Dubious, test returned 5 (wstat 1280, 0x500) > > Failed 5/60 subtests > > > > The tests needs to reflect changes in C4::Reserves. > > > > The Comment 13 is not reflected yet. Hello Liz, thanks for testing.. It's weird.. I've tested it and it passes. I changed chekcout rules before testing, and get no errors (that's the only thing I can think of that may affect those tests).. could you please tell me what was your rules configuration before testing? Thanks kohadev-koha@kohadevbox:/home/vagrant/kohaclone$ prove t/db_dependent/Reserves.t t/db_dependent/Reserves.t .. ok All tests successful. Files=1, Tests=60, 9 wallclock secs ( 0.02 usr 0.01 sys + 2.81 cusr 0.39 csys = 3.23 CPU) Result: PASS kohadev-koha@kohadevbox:/home/vagrant/kohaclone$ prove t/db_dependent/Holds.t t/db_dependent/Holds.t .. ok All tests successful. Files=1, Tests=61, 11 wallclock secs ( 0.02 usr 0.01 sys + 3.09 cusr 0.68 csys = 3.80 CPU) Result: PASS I think this test failure may be related to bug 22453 - I've got a correct date/tz devbox this morning and it's all ok - I'm sorry I didn't catch that yesterday. Onward with testing! This morning I've got something different: I have a test failing in Libraries.t - I've updated all the things(dbic and restart_all) and I feel like it should be good to go, but it's not >.< 1..2 # Subtest: UseBranchTransferLimits = OFF 1..5 # No tests run! not ok 1 - No tests run for subtest "UseBranchTransferLimits = OFF" # Failed test 'No tests run for subtest "UseBranchTransferLimits = OFF"' # at t/db_dependent/Koha/Libraries.t line 152. # Looks like you planned 2 tests but ran 1. # Looks like you failed 1 test of 1 run. not ok 6 - pickup_locations# Failed test 'pickup_locations' # at t/db_dependent/Koha/Libraries.t line 409. The method Koha::Items->pickup_locations is not covered by tests! I know you all want to get this feature in before slush, sorry to keep bouncing it back to you. Liz Created attachment 88939 [details] [review] Bug 22284: New ft_local_hold_group column, and 'holgroup' enum option In order to add groups for holds, this patch provides a new column in library_groups to identify them. Also provides new enum option for hold_fulfillment_policy column of default_branch_circ_rules, default_circ_rules, default_branch_item_rules and branch_item_rules. To test: 1) check that there is no ft_local_hold_group column in library_groups and that there are no 'holdgroup' enum option in hold_fulfillment_policy in default_branch_circ_rules, default_circ_rules, default_branch_item_rules and branch_item_rules 2) apply this patch 3) run perl installer/data/mysql/updatedatabase.pl SUCCESS => !(step 1) ;-D 4) Sign off Sponsored-by: VOKAL Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Created attachment 88940 [details] [review] Bug 22284: (follow-up) Add schema modifications Sponsored-by: VOKAL Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Created attachment 88941 [details] [review] Bug 22284: Add new checkbox in Library Groups This patch adds a new checkbox option 'Is local hold group' when you create a new root group. To test: 1) In intranet go to Library Groups in admin module 2) Add new root group 3) Check that there is no 'Is local hold group' checkbox 4) Apply this patch 5) Add new root group SUCCESS => a new 'Is local hold group' checkbox appears 6) Select 'Is local hold group' and save 7) Click edit button for root group SUCCESS => 'Is local hold group' is checked 8) Sign off Sponsored-by: VOKAL Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Created attachment 88942 [details] [review] Bug 22284: New hold group options in smart-rules This patch adds new options for 'Default checkout, hold and return policy' and 'Default holds policy by item type', both in 'Hold policy' and 'Hold pickup library match' columns To test: 1) Go to 'Circulation and fines rules' in admin 2) Check that there are no options for hold group in 'Hold policy' and 'Hold pickup library match' columns 3) Apply this patch SUCCESS => in 'Hold policy' you should see 'From local hold group' option, and in 'Hold pickup library match' you should see "Item's hold group" 4) Select any of those options and save SUCCESS => options should be selected when page reloads 5) Sign off Sponsored-by: VOKAL Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Created attachment 88943 [details] [review] Bug 22284: Control hold groups in C4::Reserves This patch modifies C4::Reserves to control when hold group options where selected in smart rules. In CanItemBeReserved adds 2 new error status messages 1) branchNotInHoldGroup: when a patron's homebranch is not in item's hold group 2) pickupNotInHoldGroup: when a selected pickup location is not in item's hold group Also CheckReserves is modified when item's priority is defined, to control by hold group when required. Finally, IsAvailableForItemLevelRequest was also modified to control by hold group when required. To test: 1) Apply this patch 2) prove t/db_dependent/Holds.t SUCCESS => Result: PASS 3) Sign off Sponsored-by: VOKAL Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Created attachment 88944 [details] [review] Bug 22284: New messages in reserve/request.tt This patch adds new messages to 'Hold' column in 'Place a hold on a specific item' table Those messages are 1) Cannot place hold from patrons's library: this message appears when patron's homebranch is not in item's hold group 2) Pickup library is not in hold group: self explanatory To test: 1) In library groups add a root group and check it as hold group. 2) Add a library to the group 3) In circulation and fines rules, in 'Default checkout, hold and return policy', in Hold policy change the value to 'From local hold group' 4) Search a patron from a different library than step 2, select one and click 'search to hold' 5) Search by location for items in the library of step 2 6) On any item, clic on 'Place hold for ...' SUCCESS => when the page is loaded, in the 'Place a hold on a specific item', you should see the message "Cannot place hold from patrons's library" in 'Hold' column 7) In circulation and fines rules, in 'Default checkout, hold and return policy', in 'Hold policy' change the value again to 'From any library' and change 'Hold pickup library match' to "Item's hold group" 8) Repeat steps 4 to 6 SUCCESS => when the page is loaded, in the 'Place a hold on a specific item', you should see the message "Pickup library is not in hold group" in 'Hold' column 9) Sign off Sponsored-by: VOKAL Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Created attachment 88945 [details] [review] Bug 22284: Opac pickup_locations This patch modifies Koha::Libraries->pickup_location and moves most of the logic to Koha::Item and Koha::Biblio in preparation for api endpoints in the future. There where 2 methods added 1) Koha::Item->pickup_locations that given a patron, returns all pickup locations of this item, considering hold fulfillment rules, and hold allowed rules. 2) Koha::Biblio->pickup_locations that given a patron, returns a distinct list of libraries returned by each of this biblio items pickup location. Koha::Libraries->pickup_location analyzes input param and calls Koha::Item->pickup_locations or Koha::Biblio->pickup_locations as needed. Also in opac-reserve.tt the way options where obtained to fill the pickup location select was modified to pass the patron as a parameter. To test: 1) opac: try to place hold on a item and check that all libraries are shown in the pickup location select. 2) intranet: in Library groups, add 2 root groups marked as local hold group and add different libraries to each. 3) opac: login as a user of a library belonging to one hold group, and search try to place a hold on an item belongin to the other hold group. 4) intranet: in Circulation and fines rules, play with 'Hold policy' and 'Hold pickup library match' rules. 5) opac: On each modification of the rules reload the page. SUCCESS => Every time you reload the page, the number of pickup locations showed in select varies. 6) prove t/db_dependent/Koha/Biblios.t t/db_dependent/Koha/Items.t SUCCESS => Result: PASS 7) Sign off Sponsored-by: VOKAL Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Created attachment 88946 [details] [review] Bug 22284: New methods in Koha::Library::Groups and Koha::Library This patch adds new methods in Koha::Library::Groups and Koha::Library. 1) For Koha::Library::Groups adds get_root_ancestor that returns all root groups for a given search parameters, for example Koha::Library::Groups->get_root_ancestor( { id => $group_id } ) 2) For Koha::Library adds 2.1) get_hold_libraries: returns all libraries (including self) that belongs to the same holdgroups. If $self belongs to several holdgroups it will return a distinct list of all libraries belonging to them. 2.2) validate_hold_sibling: Returns 1 if the given parameters matches any of the libraries that belong to any of the holdgroups this library belongs. For example $library->validate_hold_sibling( { branchcode => $branchcode } ) To test: 1) apply this patch 2) prove t/db_dependent/Koha/Libraries.t t/db_dependent/LibraryGroups.t SUCCESS => green letters :-D 3) Sign off Sponsored-by: VOKAL Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Created attachment 88947 [details] [review] Bug 22284: (follow-up) Remove commented warn and address test failures Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Created attachment 88948 [details] [review] Bug 22284: (follow-up) fix test count after merge Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> New dependency on bug 15496 only needs to apply last patch 88938 - Bug 15496: (QA follow-up) Fix remaining cases Created attachment 88949 [details] [review] Bug 22284: (follow-up) fixes after 15496 Created attachment 88955 [details] [review] Bug 22284: New ft_local_hold_group column, and 'holgroup' enum option In order to add groups for holds, this patch provides a new column in library_groups to identify them. Also provides new enum option for hold_fulfillment_policy column of default_branch_circ_rules, default_circ_rules, default_branch_item_rules and branch_item_rules. To test: 1) check that there is no ft_local_hold_group column in library_groups and that there are no 'holdgroup' enum option in hold_fulfillment_policy in default_branch_circ_rules, default_circ_rules, default_branch_item_rules and branch_item_rules 2) apply this patch 3) run perl installer/data/mysql/updatedatabase.pl SUCCESS => !(step 1) ;-D 4) Sign off Sponsored-by: VOKAL Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Created attachment 88956 [details] [review] Bug 22284: (follow-up) Add schema modifications Sponsored-by: VOKAL Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Created attachment 88957 [details] [review] Bug 22284: Add new checkbox in Library Groups This patch adds a new checkbox option 'Is local hold group' when you create a new root group. To test: 1) In intranet go to Library Groups in admin module 2) Add new root group 3) Check that there is no 'Is local hold group' checkbox 4) Apply this patch 5) Add new root group SUCCESS => a new 'Is local hold group' checkbox appears 6) Select 'Is local hold group' and save 7) Click edit button for root group SUCCESS => 'Is local hold group' is checked 8) Sign off Sponsored-by: VOKAL Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Created attachment 88958 [details] [review] Bug 22284: New hold group options in smart-rules This patch adds new options for 'Default checkout, hold and return policy' and 'Default holds policy by item type', both in 'Hold policy' and 'Hold pickup library match' columns To test: 1) Go to 'Circulation and fines rules' in admin 2) Check that there are no options for hold group in 'Hold policy' and 'Hold pickup library match' columns 3) Apply this patch SUCCESS => in 'Hold policy' you should see 'From local hold group' option, and in 'Hold pickup library match' you should see "Item's hold group" 4) Select any of those options and save SUCCESS => options should be selected when page reloads 5) Sign off Sponsored-by: VOKAL Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Created attachment 88959 [details] [review] Bug 22284: Control hold groups in C4::Reserves This patch modifies C4::Reserves to control when hold group options where selected in smart rules. In CanItemBeReserved adds 2 new error status messages 1) branchNotInHoldGroup: when a patron's homebranch is not in item's hold group 2) pickupNotInHoldGroup: when a selected pickup location is not in item's hold group Also CheckReserves is modified when item's priority is defined, to control by hold group when required. Finally, IsAvailableForItemLevelRequest was also modified to control by hold group when required. To test: 1) Apply this patch 2) prove t/db_dependent/Holds.t SUCCESS => Result: PASS 3) Sign off Sponsored-by: VOKAL Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Created attachment 88960 [details] [review] Bug 22284: New messages in reserve/request.tt This patch adds new messages to 'Hold' column in 'Place a hold on a specific item' table Those messages are 1) Cannot place hold from patrons's library: this message appears when patron's homebranch is not in item's hold group 2) Pickup library is not in hold group: self explanatory To test: 1) In library groups add a root group and check it as hold group. 2) Add a library to the group 3) In circulation and fines rules, in 'Default checkout, hold and return policy', in Hold policy change the value to 'From local hold group' 4) Search a patron from a different library than step 2, select one and click 'search to hold' 5) Search by location for items in the library of step 2 6) On any item, clic on 'Place hold for ...' SUCCESS => when the page is loaded, in the 'Place a hold on a specific item', you should see the message "Cannot place hold from patrons's library" in 'Hold' column 7) In circulation and fines rules, in 'Default checkout, hold and return policy', in 'Hold policy' change the value again to 'From any library' and change 'Hold pickup library match' to "Item's hold group" 8) Repeat steps 4 to 5 9) Find a biblio with items in 2 different libraries SUCCESS => when the page is loaded, in the 'Place a hold on a specific item', you should see the message "Pickup library is not in hold group" in 'Hold' column for the item that has no hold group 10) Sign off Sponsored-by: VOKAL Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Created attachment 88961 [details] [review] Bug 22284: Opac pickup_locations This patch modifies Koha::Libraries->pickup_location and moves most of the logic to Koha::Item and Koha::Biblio in preparation for api endpoints in the future. There where 2 methods added 1) Koha::Item->pickup_locations that given a patron, returns all pickup locations of this item, considering hold fulfillment rules, and hold allowed rules. 2) Koha::Biblio->pickup_locations that given a patron, returns a distinct list of libraries returned by each of this biblio items pickup location. Koha::Libraries->pickup_location analyzes input param and calls Koha::Item->pickup_locations or Koha::Biblio->pickup_locations as needed. Also in opac-reserve.tt the way options where obtained to fill the pickup location select was modified to pass the patron as a parameter. To test: 1) opac: try to place hold on a item and check that all libraries are shown in the pickup location select. 2) intranet: in Library groups, add 2 root groups marked as local hold group and add different libraries to each. 3) opac: login as a user of a library belonging to one hold group, and search try to place a hold on an item belongin to the other hold group. 4) intranet: in Circulation and fines rules, play with 'Hold policy' and 'Hold pickup library match' rules. 5) opac: On each modification of the rules reload the page. SUCCESS => Every time you reload the page, the number of pickup locations showed in select varies. 6) prove t/db_dependent/Koha/Biblios.t t/db_dependent/Koha/Items.t SUCCESS => Result: PASS 7) Sign off Sponsored-by: VOKAL Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Created attachment 88962 [details] [review] Bug 22284: New methods in Koha::Library::Groups and Koha::Library This patch adds new methods in Koha::Library::Groups and Koha::Library. 1) For Koha::Library::Groups adds get_root_ancestor that returns all root groups for a given search parameters, for example Koha::Library::Groups->get_root_ancestor( { id => $group_id } ) 2) For Koha::Library adds 2.1) get_hold_libraries: returns all libraries (including self) that belongs to the same holdgroups. If $self belongs to several holdgroups it will return a distinct list of all libraries belonging to them. 2.2) validate_hold_sibling: Returns 1 if the given parameters matches any of the libraries that belong to any of the holdgroups this library belongs. For example $library->validate_hold_sibling( { branchcode => $branchcode } ) To test: 1) apply this patch 2) prove t/db_dependent/Koha/Libraries.t t/db_dependent/LibraryGroups.t SUCCESS => green letters :-D 3) Sign off Sponsored-by: VOKAL Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Created attachment 88963 [details] [review] Bug 22284: (follow-up) Remove commented warn and address test failures Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Created attachment 88964 [details] [review] Bug 22284: (follow-up) fix test count after merge Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Created attachment 88965 [details] [review] Bug 22284: (follow-up) fixes after 15496 At first glance I'd say that this patchset is going to conflict a lot with bug 18936 (and deps), that is going to remove the circ tables you are modifying. (In reply to Jonathan Druart from comment #58) > At first glance I'd say that this patchset is going to conflict a lot with > bug 18936 (and deps), that is going to remove the circ tables you are > modifying. Hello Jonathan, for what I see, mostly test are going to conflict with those changes.. the modifications I did to circ tables where to add a new value to enum columns.. and with this new schema I don't think it will be a problem. I'll make this patch depend on 18936 and see what happens. Thanks for the warning. Created attachment 89060 [details] [review] Bug 22284: New ft_local_hold_group column, and 'holgroup' enum option In order to add groups for holds, this patch provides a new column in library_groups to identify them. Also provides new enum option for hold_fulfillment_policy column of default_branch_circ_rules, default_circ_rules, default_branch_item_rules and branch_item_rules. To test: 1) check that there is no ft_local_hold_group column in library_groups and that there are no 'holdgroup' enum option in hold_fulfillment_policy in default_branch_circ_rules, default_circ_rules, default_branch_item_rules and branch_item_rules 2) apply this patch 3) run perl installer/data/mysql/updatedatabase.pl SUCCESS => !(step 1) ;-D 4) Sign off Sponsored-by: VOKAL Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Created attachment 89061 [details] [review] Bug 22284: (follow-up) Add schema modifications Sponsored-by: VOKAL Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Created attachment 89062 [details] [review] Bug 22284: Add new checkbox in Library Groups This patch adds a new checkbox option 'Is local hold group' when you create a new root group. To test: 1) In intranet go to Library Groups in admin module 2) Add new root group 3) Check that there is no 'Is local hold group' checkbox 4) Apply this patch 5) Add new root group SUCCESS => a new 'Is local hold group' checkbox appears 6) Select 'Is local hold group' and save 7) Click edit button for root group SUCCESS => 'Is local hold group' is checked 8) Sign off Sponsored-by: VOKAL Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Created attachment 89063 [details] [review] Bug 22284: New hold group options in smart-rules This patch adds new options for 'Default checkout, hold and return policy' and 'Default holds policy by item type', both in 'Hold policy' and 'Hold pickup library match' columns To test: 1) Go to 'Circulation and fines rules' in admin 2) Check that there are no options for hold group in 'Hold policy' and 'Hold pickup library match' columns 3) Apply this patch SUCCESS => in 'Hold policy' you should see 'From local hold group' option, and in 'Hold pickup library match' you should see "Item's hold group" 4) Select any of those options and save SUCCESS => options should be selected when page reloads 5) Sign off Sponsored-by: VOKAL Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Created attachment 89064 [details] [review] Bug 22284: Control hold groups in C4::Reserves This patch modifies C4::Reserves to control when hold group options where selected in smart rules. In CanItemBeReserved adds 2 new error status messages 1) branchNotInHoldGroup: when a patron's homebranch is not in item's hold group 2) pickupNotInHoldGroup: when a selected pickup location is not in item's hold group Also CheckReserves is modified when item's priority is defined, to control by hold group when required. Finally, IsAvailableForItemLevelRequest was also modified to control by hold group when required. To test: 1) Apply this patch 2) prove t/db_dependent/Holds.t SUCCESS => Result: PASS 3) Sign off Sponsored-by: VOKAL Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Created attachment 89065 [details] [review] Bug 22284: New methods in Koha::Library::Groups and Koha::Library This patch adds new methods in Koha::Library::Groups and Koha::Library. 1) For Koha::Library::Groups adds get_root_ancestor that returns all root groups for a given search parameters, for example Koha::Library::Groups->get_root_ancestor( { id => $group_id } ) 2) For Koha::Library adds 2.1) get_hold_libraries: returns all libraries (including self) that belongs to the same holdgroups. If $self belongs to several holdgroups it will return a distinct list of all libraries belonging to them. 2.2) validate_hold_sibling: Returns 1 if the given parameters matches any of the libraries that belong to any of the holdgroups this library belongs. For example $library->validate_hold_sibling( { branchcode => $branchcode } ) To test: 1) apply this patch 2) prove t/db_dependent/Koha/Libraries.t t/db_dependent/LibraryGroups.t SUCCESS => green letters :-D 3) Sign off Sponsored-by: VOKAL Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Created attachment 89066 [details] [review] Bug 22284: Opac pickup_locations This patch modifies Koha::Libraries->pickup_location and moves most of the logic to Koha::Item and Koha::Biblio in preparation for api endpoints in the future. There where 2 methods added 1) Koha::Item->pickup_locations that given a patron, returns all pickup locations of this item, considering hold fulfillment rules, and hold allowed rules. 2) Koha::Biblio->pickup_locations that given a patron, returns a distinct list of libraries returned by each of this biblio items pickup location. Koha::Libraries->pickup_location analyzes input param and calls Koha::Item->pickup_locations or Koha::Biblio->pickup_locations as needed. Also in opac-reserve.tt the way options where obtained to fill the pickup location select was modified to pass the patron as a parameter. To test: 1) opac: try to place hold on a item and check that all libraries are shown in the pickup location select. 2) intranet: in Library groups, add 2 root groups marked as local hold group and add different libraries to each. 3) opac: login as a user of a library belonging to one hold group, and search try to place a hold on an item belongin to the other hold group. 4) intranet: in Circulation and fines rules, play with 'Hold policy' and 'Hold pickup library match' rules. 5) opac: On each modification of the rules reload the page. SUCCESS => Every time you reload the page, the number of pickup locations showed in select varies. 6) prove t/db_dependent/Koha/Biblios.t t/db_dependent/Koha/Items.t SUCCESS => Result: PASS 7) Sign off Sponsored-by: VOKAL Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Created attachment 89067 [details] [review] Bug 22284: (follow-up) Default to circulation control branch when no hold libraries are found Created attachment 89068 [details] [review] Bug 22284: New message, new column and filter pickup locations in reserve/request.tt This patch adds a new message to 'Hold' column in 'Place a hold on a specific item' table. The message is "Cannot place hold from patrons's library". It appears when patron's homebranch is not in item's hold group, and hold_fulfillment_policy is set to 'holdgroup'. This patch also adds a new column "Allowed pickup locations" that lists allowed pickup locations per item. Finally, the select that displays pickup locations is filtered by allowed pickup locations, when multi_hold is not enabled To test: 1) Apply this patch 2) In library groups add a root group and check it as hold group. 3) Add two libraries to the group 4) In circulation and fines rules, in 'Default checkout, hold and return policy', in Hold policy change the value to 'From local hold group' 5) Search a patron from a different library than step 3, select one and click 'search to hold' 6) Search by location for items in any library of step 3 7) On any item, clic on 'Place hold for ...' SUCCESS => when the page is loaded, in the 'Place a hold on a specific item', you should see the message "Cannot place hold from patrons's library" in 'Hold' column => You should see a new column called "Allowed pickup locations" and the message is "Any library" 8) In circulation and fines rules, in 'Default checkout, hold and return policy', in 'Hold policy' change the value again to 'From any library' and change 'Hold pickup library match' to "Item's hold group" 8) Repeat steps 5 to 7 SUCCESS => when the page is loaded, you should see the "Pickup at" select filtered by libraries in hold group => You should see in "Allowed pickup locations" a coma separated list of the libraries in item's hold group => If biblio has an item whose control branch is not in a hold group, you should see the control branch name in "Allowed pickup locations" 9) Sign off Sponsored-by: VOKAL Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Created attachment 89069 [details] [review] Bug 22284: (follow-up) Remove commented warn and address test failures Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Created attachment 89070 [details] [review] Bug 22284: (follow-up) fix test count after merge Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Created attachment 89071 [details] [review] Bug 22284: (follow-up) fixes after 15496 Created attachment 89072 [details] [review] Bug 22284: (follow-up) fixes after 18936 Created attachment 89073 [details] [review] Bug 22284: New ft_local_hold_group column, and 'holgroup' enum option In order to add groups for holds, this patch provides a new column in library_groups to identify them. Also provides new enum option for hold_fulfillment_policy column of default_branch_circ_rules, default_circ_rules, default_branch_item_rules and branch_item_rules. To test: 1) check that there is no ft_local_hold_group column in library_groups and that there are no 'holdgroup' enum option in hold_fulfillment_policy in default_branch_circ_rules, default_circ_rules, default_branch_item_rules and branch_item_rules 2) apply this patch 3) perl installer/data/mysql/updatedatabase.pl && dbic SUCCESS => !(step 1) ;-D 4) Sign off Sponsored-by: VOKAL Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Created attachment 89074 [details] [review] Bug 22284: (follow-up) Add schema modifications Sponsored-by: VOKAL Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Created attachment 89075 [details] [review] Bug 22284: Add new checkbox in Library Groups This patch adds a new checkbox option 'Is local hold group' when you create a new root group. To test: 1) In intranet go to Library Groups in admin module 2) Add new root group 3) Check that there is no 'Is local hold group' checkbox 4) Apply this patch 5) Add new root group SUCCESS => a new 'Is local hold group' checkbox appears 6) Select 'Is local hold group' and save 7) Click edit button for root group SUCCESS => 'Is local hold group' is checked 8) Sign off Sponsored-by: VOKAL Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Created attachment 89076 [details] [review] Bug 22284: New hold group options in smart-rules This patch adds new options for 'Default checkout, hold and return policy' and 'Default holds policy by item type', both in 'Hold policy' and 'Hold pickup library match' columns To test: 1) Go to 'Circulation and fines rules' in admin 2) Check that there are no options for hold group in 'Hold policy' and 'Hold pickup library match' columns 3) Apply this patch SUCCESS => in 'Hold policy' you should see 'From local hold group' option, and in 'Hold pickup library match' you should see "Item's hold group" 4) Select any of those options and save SUCCESS => options should be selected when page reloads 5) Sign off Sponsored-by: VOKAL Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Created attachment 89077 [details] [review] Bug 22284: Control hold groups in C4::Reserves This patch modifies C4::Reserves to control when hold group options where selected in smart rules. In CanItemBeReserved adds 2 new error status messages 1) branchNotInHoldGroup: when a patron's homebranch is not in item's hold group 2) pickupNotInHoldGroup: when a selected pickup location is not in item's hold group Also CheckReserves is modified when item's priority is defined, to control by hold group when required. Finally, IsAvailableForItemLevelRequest was also modified to control by hold group when required. To test: 1) Apply this patch 2) prove t/db_dependent/Holds.t SUCCESS => Result: PASS 3) Sign off Sponsored-by: VOKAL Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Created attachment 89078 [details] [review] Bug 22284: New methods in Koha::Library::Groups and Koha::Library This patch adds new methods in Koha::Library::Groups and Koha::Library. 1) For Koha::Library::Groups adds get_root_ancestor that returns all root groups for a given search parameters, for example Koha::Library::Groups->get_root_ancestor( { id => $group_id } ) 2) For Koha::Library adds 2.1) get_hold_libraries: returns all libraries (including self) that belongs to the same holdgroups. If $self belongs to several holdgroups it will return a distinct list of all libraries belonging to them. 2.2) validate_hold_sibling: Returns 1 if the given parameters matches any of the libraries that belong to any of the holdgroups this library belongs. For example $library->validate_hold_sibling( { branchcode => $branchcode } ) To test: 1) apply this patch 2) prove t/db_dependent/Koha/Libraries.t t/db_dependent/LibraryGroups.t SUCCESS => green letters :-D 3) Sign off Sponsored-by: VOKAL Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Created attachment 89079 [details] [review] Bug 22284: Opac pickup_locations This patch modifies Koha::Libraries->pickup_location and moves most of the logic to Koha::Item and Koha::Biblio in preparation for api endpoints in the future. There where 2 methods added 1) Koha::Item->pickup_locations that given a patron, returns all pickup locations of this item, considering hold fulfillment rules, and hold allowed rules. 2) Koha::Biblio->pickup_locations that given a patron, returns a distinct list of libraries returned by each of this biblio items pickup location. Koha::Libraries->pickup_location analyzes input param and calls Koha::Item->pickup_locations or Koha::Biblio->pickup_locations as needed. Also in opac-reserve.tt the way options where obtained to fill the pickup location select was modified to pass the patron as a parameter. To test: 1) opac: try to place hold on a item and check that all libraries are shown in the pickup location select. 2) intranet: in Library groups, add 2 root groups marked as local hold group and add different libraries to each. 3) opac: login as a user of a library belonging to one hold group, and search try to place a hold on an item belongin to the other hold group. 4) intranet: in Circulation and fines rules, play with 'Hold policy' and 'Hold pickup library match' rules. 5) opac: On each modification of the rules reload the page. SUCCESS => Every time you reload the page, the number of pickup locations showed in select varies. 6) prove t/db_dependent/Koha/Biblios.t t/db_dependent/Koha/Items.t SUCCESS => Result: PASS 7) Sign off Sponsored-by: VOKAL Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Created attachment 89080 [details] [review] Bug 22284: (follow-up) Default to circulation control branch when no hold libraries are found Created attachment 89081 [details] [review] Bug 22284: New message, new column and filter pickup locations in reserve/request.tt This patch adds a new message to 'Hold' column in 'Place a hold on a specific item' table. The message is "Cannot place hold from patrons's library". It appears when patron's homebranch is not in item's hold group, and hold_fulfillment_policy is set to 'holdgroup'. This patch also adds a new column "Allowed pickup locations" that lists allowed pickup locations per item. Finally, the select that displays pickup locations is filtered by allowed pickup locations, when multi_hold is not enabled To test: 1) Apply this patch 2) In library groups add a root group and check it as hold group. 3) Add two libraries to the group 4) In circulation and fines rules, in 'Default checkout, hold and return policy', in Hold policy change the value to 'From local hold group' 5) Search a patron from a different library than step 3, select one and click 'search to hold' 6) Search by location for items in any library of step 3 7) On any item, clic on 'Place hold for ...' SUCCESS => when the page is loaded, in the 'Place a hold on a specific item', you should see the message "Cannot place hold from patrons's library" in 'Hold' column => You should see a new column called "Allowed pickup locations" and the message is "Any library" 8) In circulation and fines rules, in 'Default checkout, hold and return policy', in 'Hold policy' change the value again to 'From any library' and change 'Hold pickup library match' to "Item's hold group" 8) Repeat steps 5 to 7 SUCCESS => when the page is loaded, you should see the "Pickup at" select filtered by libraries in hold group => You should see in "Allowed pickup locations" a coma separated list of the libraries in item's hold group => If biblio has an item whose control branch is not in a hold group, you should see the control branch name in "Allowed pickup locations" 9) Sign off Sponsored-by: VOKAL Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Created attachment 89082 [details] [review] Bug 22284: (follow-up) Remove commented warn and address test failures Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Created attachment 89083 [details] [review] Bug 22284: (follow-up) fix test count after merge Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Created attachment 89084 [details] [review] Bug 22284: (follow-up) fixes after 15496 Created attachment 89085 [details] [review] Bug 22284: (follow-up) fixes after 18936 Created attachment 89122 [details] [review] Bug 22284: New ft_local_hold_group column, and 'holgroup' enum option In order to add groups for holds, this patch provides a new column in library_groups to identify them. Also provides new enum option for hold_fulfillment_policy column of default_branch_circ_rules, default_circ_rules, default_branch_item_rules and branch_item_rules. To test: 1) check that there is no ft_local_hold_group column in library_groups and that there are no 'holdgroup' enum option in hold_fulfillment_policy in default_branch_circ_rules, default_circ_rules, default_branch_item_rules and branch_item_rules 2) apply this patch 3) perl installer/data/mysql/updatedatabase.pl && dbic SUCCESS => !(step 1) ;-D 4) Sign off Sponsored-by: VOKAL Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Created attachment 89123 [details] [review] Bug 22284: (follow-up) Add schema modifications Sponsored-by: VOKAL Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Created attachment 89124 [details] [review] Bug 22284: Add new checkbox in Library Groups This patch adds a new checkbox option 'Is local hold group' when you create a new root group. To test: 1) In intranet go to Library Groups in admin module 2) Add new root group 3) Check that there is no 'Is local hold group' checkbox 4) Apply this patch 5) Add new root group SUCCESS => a new 'Is local hold group' checkbox appears 6) Select 'Is local hold group' and save 7) Click edit button for root group SUCCESS => 'Is local hold group' is checked 8) Sign off Sponsored-by: VOKAL Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Created attachment 89125 [details] [review] Bug 22284: New hold group options in smart-rules This patch adds new options for 'Default checkout, hold and return policy' and 'Default holds policy by item type', both in 'Hold policy' and 'Hold pickup library match' columns To test: 1) Go to 'Circulation and fines rules' in admin 2) Check that there are no options for hold group in 'Hold policy' and 'Hold pickup library match' columns 3) Apply this patch SUCCESS => in 'Hold policy' you should see 'From local hold group' option, and in 'Hold pickup library match' you should see "Item's hold group" 4) Select any of those options and save SUCCESS => options should be selected when page reloads 5) Sign off Sponsored-by: VOKAL Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Created attachment 89126 [details] [review] Bug 22284: Control hold groups in C4::Reserves This patch modifies C4::Reserves to control when hold group options where selected in smart rules. In CanItemBeReserved adds 2 new error status messages 1) branchNotInHoldGroup: when a patron's homebranch is not in item's hold group 2) pickupNotInHoldGroup: when a selected pickup location is not in item's hold group Also CheckReserves is modified when item's priority is defined, to control by hold group when required. Finally, IsAvailableForItemLevelRequest was also modified to control by hold group when required. To test: 1) Apply this patch 2) prove t/db_dependent/Holds.t SUCCESS => Result: PASS 3) Sign off Sponsored-by: VOKAL Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Created attachment 89127 [details] [review] Bug 22284: New methods in Koha::Library::Groups and Koha::Library This patch adds new methods in Koha::Library::Groups and Koha::Library. 1) For Koha::Library::Groups adds get_root_ancestor that returns all root groups for a given search parameters, for example Koha::Library::Groups->get_root_ancestor( { id => $group_id } ) 2) For Koha::Library adds 2.1) get_hold_libraries: returns all libraries (including self) that belongs to the same holdgroups. If $self belongs to several holdgroups it will return a distinct list of all libraries belonging to them. 2.2) validate_hold_sibling: Returns 1 if the given parameters matches any of the libraries that belong to any of the holdgroups this library belongs. For example $library->validate_hold_sibling( { branchcode => $branchcode } ) To test: 1) apply this patch 2) prove t/db_dependent/Koha/Libraries.t t/db_dependent/LibraryGroups.t SUCCESS => green letters :-D 3) Sign off Sponsored-by: VOKAL Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Created attachment 89128 [details] [review] Bug 22284: Opac pickup_locations This patch modifies Koha::Libraries->pickup_location and moves most of the logic to Koha::Item and Koha::Biblio in preparation for api endpoints in the future. There where 2 methods added 1) Koha::Item->pickup_locations that given a patron, returns all pickup locations of this item, considering hold fulfillment rules, and hold allowed rules. 2) Koha::Biblio->pickup_locations that given a patron, returns a distinct list of libraries returned by each of this biblio items pickup location. Koha::Libraries->pickup_location analyzes input param and calls Koha::Item->pickup_locations or Koha::Biblio->pickup_locations as needed. Also in opac-reserve.tt the way options where obtained to fill the pickup location select was modified to pass the patron as a parameter. To test: 1) opac: try to place hold on a item and check that all libraries are shown in the pickup location select. 2) intranet: in Library groups, add 2 root groups marked as local hold group and add different libraries to each. 3) opac: login as a user of a library belonging to one hold group, and search try to place a hold on an item belongin to the other hold group. 4) intranet: in Circulation and fines rules, play with 'Hold policy' and 'Hold pickup library match' rules. 5) opac: On each modification of the rules reload the page. SUCCESS => Every time you reload the page, the number of pickup locations showed in select varies. 6) prove t/db_dependent/Koha/Biblios.t t/db_dependent/Koha/Items.t SUCCESS => Result: PASS 7) Sign off Sponsored-by: VOKAL Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Created attachment 89129 [details] [review] Bug 22284: (follow-up) Default to circulation control branch when no hold libraries are found Created attachment 89130 [details] [review] Bug 22284: New message, new column and filter pickup locations in reserve/request.tt This patch adds a new message to 'Hold' column in 'Place a hold on a specific item' table. The message is "Cannot place hold from patrons's library". It appears when patron's homebranch is not in item's hold group, and hold_fulfillment_policy is set to 'holdgroup'. This patch also adds a new column "Allowed pickup locations" that lists allowed pickup locations per item. Finally, the select that displays pickup locations is filtered by allowed pickup locations, when multi_hold is not enabled To test: 1) Apply this patch 2) In library groups add a root group and check it as hold group. 3) Add two libraries to the group 4) In circulation and fines rules, in 'Default checkout, hold and return policy', in Hold policy change the value to 'From local hold group' 5) Search a patron from a different library than step 3, select one and click 'search to hold' 6) Search by location for items in any library of step 3 7) On any item, clic on 'Place hold for ...' SUCCESS => when the page is loaded, in the 'Place a hold on a specific item', you should see the message "Cannot place hold from patrons's library" in 'Hold' column => You should see a new column called "Allowed pickup locations" and the message is "Any library" 8) In circulation and fines rules, in 'Default checkout, hold and return policy', in 'Hold policy' change the value again to 'From any library' and change 'Hold pickup library match' to "Item's hold group" 8) Repeat steps 5 to 7 SUCCESS => when the page is loaded, you should see the "Pickup at" select filtered by libraries in hold group => You should see in "Allowed pickup locations" a coma separated list of the libraries in item's hold group => If biblio has an item whose control branch is not in a hold group, you should see the control branch name in "Allowed pickup locations" 9) Sign off Sponsored-by: VOKAL Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Created attachment 89131 [details] [review] Bug 22284: Filter item's pickup locations in holds_table.inc This patch filters pickup locations per item in holds_table.inc To test: 1) Go to Circulation and fines rules and change max holds allowed to > 0, and change 'Hold pickup library match' to homebranch 2) Search for a patron 3) Click on "Search to hold" 4) Search for any bilio 5) Place hold on a specific item CHECK => Item's pickup location is a select with all libraries 6) Apply this patch 7) Repeat steps 2 to 5 SUCCESS => Item's pickup location is filtered by allowed pickup locations 8) Sign off Sponsored-by: VOKAL Created attachment 89132 [details] [review] Bug 22284: (follow-up) Remove commented warn and address test failures Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Created attachment 89133 [details] [review] Bug 22284: (follow-up) fix test count after merge Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Created attachment 89134 [details] [review] Bug 22284: (follow-up) fixes after 15496 Created attachment 89135 [details] [review] Bug 22284: (follow-up) fixes after 18936 (In reply to Agustín Moyano from comment #86) > Created attachment 89122 [details] [review] [review] > Bug 22284: New ft_local_hold_group column, and 'holgroup' enum option > > In order to add groups for holds, this patch provides a new column in > library_groups to identify them. Also provides new enum option for > hold_fulfillment_policy column of default_branch_circ_rules, > default_circ_rules, default_branch_item_rules and branch_item_rules. > > To test: > 1) check that there is no ft_local_hold_group column in library_groups > and that there are no 'holdgroup' enum option in hold_fulfillment_policy > in default_branch_circ_rules, default_circ_rules, default_branch_item_rules > and branch_item_rules > 2) apply this patch > 3) perl installer/data/mysql/updatedatabase.pl && dbic > SUCCESS => !(step 1) ;-D > 4) Sign off > > Sponsored-by: VOKAL > > Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> After bug 18936, omit from this test plan the 'holdgroup' enum columns i'll retest this tomorrow I think I've tried to test this a couple of times now but I just can't seem to get the patches for 18936 to apply to even get these ones on. Hit me up in IRC if you'd like to help me get all the patches applied so I can test - this is good functionality and I hate to see it sit. Liz Alright, thanks Agustin for helping me get these patches to apply! Have gotten through a bit of testing and I have some questions about the feature generally. A note: I've attached the set of MARC records that I usually supplement my devbox with, just so we are looking at the same things. Import them using the Stage MARC for import tool. :) my groups: North: Centerville, Fairfield, Franklin South: Springfield, Troy, Union The other libraries are not in a group, they stand alone. Test, opac: Biblio: De Crasi Aristophea. Dissertatio - 3 items, Centerville, Fairview, Midway Patron: Nicole Schneider, Franklin Pickup locations seen: Fairview, Centerville, Fairfield, Franklin Seems OK? Except maybe Fairview, as it isn't in the patron's group. Biblio: The Vixens, A novel. 1 item, Midway Patron: Nicole Schneider, Franklin Pickup locations seen: Midway Hm. Why not the libraries in Nicole's group (Centerville, Fairfield, Franklin) Biblio: Gairm, 1 item, Centerville Patron: Nicole Schneider, Franklin Pickup locations seen: Centerville, Fairfield, Franklin This seems right - pick up locations in the patron's group, but it is also the item's group. Biblio: De Crasi Aristophea. Dissertatio - 3 items, Centerville, Fairview, Midway Patron: Henry Acevedo, Midway Pickup locations seen: Midway, Fairview, Centerville, Fairfield, Franklin Logic (I think? can you confirm?): Midway because it is the patron's home library. Fairview because there is an item from Fairview? Centerville, Fairfield, Franklin, in a hold group, however our patron is not in this hold group? Is this right? Biblio: The Vixens, A novel. 1 item, Midway Patron: Henry Acevedo, Midway Pickup locations seen: Midway Thoughts: Midway is not in a group. It's like an individual branch is a group unto itself? Is that correct behaviour? Biblio: Gairm, 1 item, Centerville Patron: Henry Acevedo, Midway Pickup locations seen: Centerville, Fairfield, Franklin Thoughts: The patron is not in this group, I don't understand why we are using the item's group and not the patron's group? The main question I have is, which is the location that defines which group the reserve can be picked up in, the patron's branch (which is what I believe the intent of the feature request was) or the item's homebranch? If someone from VOKAL could comment on what they are expecting in the above scenarios, I think that would be really helpful. I'm going to stop here for now, as I think this question of which location is canonical is quite core to this dev. Liz Created attachment 89523 [details]
MARC records for testing, compatible with default branches
Hello Liz Thanks again for testing! By your results of your tests, I infer that Hold policy was set to "From any library" and Hold pickup library match was set to "Item's hold group" Maybe I got it wrong, but I interpreted that: 1) Hold policy determines who can place a hold. 2) Hold pickup library match determines where an item could be picked up. It wasn't clear in this bug description, but I based pickup library match to item's homebranch/holdingbranch (depending on 'HomeOrHoldingBranch' preference) So now I'll reply test by test > my groups: > North: Centerville, Fairfield, Franklin > South: Springfield, Troy, Union > > The other libraries are not in a group, they stand alone. > > Test, opac: > > Biblio: De Crasi Aristophea. Dissertatio - 3 items, Centerville, Fairview, > Midway > Patron: Nicole Schneider, Franklin > Pickup locations seen: Fairview, Centerville, Fairfield, Franklin > Seems OK? Except maybe Fairview, as it isn't in the patron's group. In the test I did, I had Fairview, Centerville, Fairfield, Franklin and Midway, and that's consistent with having North holdgroup plus Franklin and Midway that have no holgroup. > > Biblio: The Vixens, A novel. 1 item, Midway > Patron: Nicole Schneider, Franklin > Pickup locations seen: Midway > Hm. Why not the libraries in Nicole's group (Centerville, Fairfield, > Franklin) As I based holdgroup on item's homebranch/holdgingbranch, and because Midway is not in a holgroup, you only see Midway > > Biblio: Gairm, 1 item, Centerville > Patron: Nicole Schneider, Franklin > Pickup locations seen: Centerville, Fairfield, Franklin > This seems right - pick up locations in the patron's group, but it is also > the item's group. > > Biblio: De Crasi Aristophea. Dissertatio - 3 items, Centerville, Fairview, > Midway > Patron: Henry Acevedo, Midway > Pickup locations seen: Midway, Fairview, Centerville, Fairfield, Franklin > Logic (I think? can you confirm?): > Midway because it is the patron's home library. No, it's because there is an item from Midway. > Fairview because there is an item from Fairview? right > Centerville, Fairfield, Franklin, in a hold group, however our patron is not > in this hold group? Is this right? If Hold policy was set to 'any', means any patron can place a hold (as I may have misinterpreted), so those 3 libraries are because there is an item in Centerville, that belongs to North holdgroup. This result should be exactly as your first test. > Biblio: The Vixens, A novel. 1 item, Midway > Patron: Henry Acevedo, Midway > Pickup locations seen: Midway > Thoughts: Midway is not in a group. It's like an individual branch is a > group unto itself? Is that correct behaviour? That's correct.. if a library isn't in a holdgroup, then it's taken as an individual holdgroup. > > Biblio: Gairm, 1 item, Centerville > Patron: Henry Acevedo, Midway > Pickup locations seen: Centerville, Fairfield, Franklin > Thoughts: The patron is not in this group, I don't understand why we are > using the item's group and not the patron's group? > If you want the truth.. the plain, raw, ugly truth is that I didn't know which one to pick, based on this bug description.. so I saw that in 'Hold pickup library match', options where based on item, and I went with the flow.. :P Maybe I can add another option.. something like "Patron's hold group" in pickup library match. Agustín I think we'll just want to know which way the library that requested it wants to go. I'm told there's some kind of meeting today that might shed some light. :) All of your notes make sense - I think it's working as you coded it, and it's TBD whether that's how the requester wants it to work. Hopefully we find out today! Liz (In reply to Liz Rea from comment #106) > I think we'll just want to know which way the library that requested it > wants to go. I'm told there's some kind of meeting today that might shed > some light. :) > > All of your notes make sense - I think it's working as you coded it, and > it's TBD whether that's how the requester wants it to work. Hopefully we > find out today! > > Liz Maybe if VOKAL want's Patron's hold group, or if it just makes sense to have it, I can add that option to Hold Pickup Library Match in a separate bug that depends on this one. Agustín (In reply to Agustín Moyano from comment #107) > (In reply to Liz Rea from comment #106) > > I think we'll just want to know which way the library that requested it > > wants to go. I'm told there's some kind of meeting today that might shed > > some light. :) > > > > All of your notes make sense - I think it's working as you coded it, and > > it's TBD whether that's how the requester wants it to work. Hopefully we > > find out today! > > > > Liz > > Maybe if VOKAL want's Patron's hold group, or if it just makes sense to have > it, I can add that option to Hold Pickup Library Match in a separate bug > that depends on this one. > > Agustín Keep in mind there's this sysprefs already: - canreservefromotherbranches - ReservesControlBranch both should affect this behaviour. That's true, I'll retest with those set away from the defaults, maybe it will make it work the way I think they probably want it. Liz (In reply to Liz Rea from comment #103) > The main question I have is, which is the location that defines which group > the reserve can be picked up in, the patron's branch (which is what I > believe the intent of the feature request was) or the item's homebranch? I think the weirdness here is that the current implementation is not considering ReservesControlBranch and instead it is using HomeOrHoldingBranch, which is not intended for holds. I have reviewed both sysprefs history, and noticed this. So, the solution is to actually allow the library to choose the desired behaviour through this syspref. The options it provides are: - Check the patron's home library to see if the patron can place a hold on the item. - Check the item's home library to see if the patron can place a hold on the item. So I think we just need to stick with that, and both possible answers to the question will be covered. It also looks like it would follow Koha's current approach. Created attachment 89836 [details] [review] Bug 22284: New ft_local_hold_group column, and 'holgroup' enum option In order to add groups for holds, this patch provides a new column in library_groups to identify them. Also provides new enum option for hold_fulfillment_policy column of default_branch_circ_rules, default_circ_rules, default_branch_item_rules and branch_item_rules. To test: 1) check that there is no ft_local_hold_group column in library_groups and that there are no 'holdgroup' enum option in hold_fulfillment_policy in default_branch_circ_rules, default_circ_rules, default_branch_item_rules and branch_item_rules 2) apply this patch 3) perl installer/data/mysql/updatedatabase.pl && dbic SUCCESS => !(step 1) ;-D 4) Sign off Sponsored-by: VOKAL Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Created attachment 89837 [details] [review] Bug 22284: (follow-up) Add schema modifications Sponsored-by: VOKAL Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Created attachment 89838 [details] [review] Bug 22284: Add new checkbox in Library Groups This patch adds a new checkbox option 'Is local hold group' when you create a new root group. To test: 1) In intranet go to Library Groups in admin module 2) Add new root group 3) Check that there is no 'Is local hold group' checkbox 4) Apply this patch 5) Add new root group SUCCESS => a new 'Is local hold group' checkbox appears 6) Select 'Is local hold group' and save 7) Click edit button for root group SUCCESS => 'Is local hold group' is checked 8) Sign off Sponsored-by: VOKAL Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Created attachment 89839 [details] [review] Bug 22284: New hold group options in smart-rules This patch adds new options for 'Default checkout, hold and return policy' and 'Default holds policy by item type', both in 'Hold policy' and 'Hold pickup library match' columns To test: 1) Go to 'Circulation and fines rules' in admin 2) Check that there are no options for hold group in 'Hold policy' and 'Hold pickup library match' columns 3) Apply this patch SUCCESS => in 'Hold policy' you should see 'From local hold group' option, and in 'Hold pickup library match' you should see "Item's hold group" 4) Select any of those options and save SUCCESS => options should be selected when page reloads 5) Sign off Sponsored-by: VOKAL Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Created attachment 89840 [details] [review] Bug 22284: Control hold groups in C4::Reserves This patch modifies C4::Reserves to control when hold group options where selected in smart rules. In CanItemBeReserved adds 2 new error status messages 1) branchNotInHoldGroup: when a patron's homebranch is not in item's hold group 2) pickupNotInHoldGroup: when a selected pickup location is not in item's hold group Also CheckReserves is modified when item's priority is defined, to control by hold group when required. Finally, IsAvailableForItemLevelRequest was also modified to control by hold group when required. To test: 1) Apply this patch 2) prove t/db_dependent/Holds.t SUCCESS => Result: PASS 3) Sign off Sponsored-by: VOKAL Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Created attachment 89841 [details] [review] Bug 22284: New methods in Koha::Library::Groups and Koha::Library This patch adds new methods in Koha::Library::Groups and Koha::Library. 1) For Koha::Library::Groups adds get_root_ancestor that returns all root groups for a given search parameters, for example Koha::Library::Groups->get_root_ancestor( { id => $group_id } ) 2) For Koha::Library adds 2.1) get_hold_libraries: returns all libraries (including self) that belongs to the same holdgroups. If $self belongs to several holdgroups it will return a distinct list of all libraries belonging to them. 2.2) validate_hold_sibling: Returns 1 if the given parameters matches any of the libraries that belong to any of the holdgroups this library belongs. For example $library->validate_hold_sibling( { branchcode => $branchcode } ) To test: 1) apply this patch 2) prove t/db_dependent/Koha/Libraries.t t/db_dependent/LibraryGroups.t SUCCESS => green letters :-D 3) Sign off Sponsored-by: VOKAL Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Created attachment 89842 [details] [review] Bug 22284: Opac pickup_locations This patch modifies Koha::Libraries->pickup_location and moves most of the logic to Koha::Item and Koha::Biblio in preparation for api endpoints in the future. There where 2 methods added 1) Koha::Item->pickup_locations that given a patron, returns all pickup locations of this item, considering hold fulfillment rules, and hold allowed rules. 2) Koha::Biblio->pickup_locations that given a patron, returns a distinct list of libraries returned by each of this biblio items pickup location. Koha::Libraries->pickup_location analyzes input param and calls Koha::Item->pickup_locations or Koha::Biblio->pickup_locations as needed. Also in opac-reserve.tt the way options where obtained to fill the pickup location select was modified to pass the patron as a parameter. To test: 1) opac: try to place hold on a item and check that all libraries are shown in the pickup location select. 2) intranet: in Library groups, add 2 root groups marked as local hold group and add different libraries to each. 3) opac: login as a user of a library belonging to one hold group, and search try to place a hold on an item belongin to the other hold group. 4) intranet: in Circulation and fines rules, play with 'Hold policy' and 'Hold pickup library match' rules. 5) opac: On each modification of the rules reload the page. SUCCESS => Every time you reload the page, the number of pickup locations showed in select varies. 6) prove t/db_dependent/Koha/Biblios.t t/db_dependent/Koha/Items.t SUCCESS => Result: PASS 7) Sign off Sponsored-by: VOKAL Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Created attachment 89843 [details] [review] Bug 22284: (follow-up) Default to circulation control branch when no hold libraries are found Created attachment 89844 [details] [review] Bug 22284: New message, new column and filter pickup locations in reserve/request.tt This patch adds a new message to 'Hold' column in 'Place a hold on a specific item' table. The message is "Cannot place hold from patrons's library". It appears when patron's homebranch is not in item's hold group, and hold_fulfillment_policy is set to 'holdgroup'. This patch also adds a new column "Allowed pickup locations" that lists allowed pickup locations per item. Finally, the select that displays pickup locations is filtered by allowed pickup locations, when multi_hold is not enabled To test: 1) Apply this patch 2) In library groups add a root group and check it as hold group. 3) Add two libraries to the group 4) In circulation and fines rules, in 'Default checkout, hold and return policy', in Hold policy change the value to 'From local hold group' 5) Search a patron from a different library than step 3, select one and click 'search to hold' 6) Search by location for items in any library of step 3 7) On any item, clic on 'Place hold for ...' SUCCESS => when the page is loaded, in the 'Place a hold on a specific item', you should see the message "Cannot place hold from patrons's library" in 'Hold' column => You should see a new column called "Allowed pickup locations" and the message is "Any library" 8) In circulation and fines rules, in 'Default checkout, hold and return policy', in 'Hold policy' change the value again to 'From any library' and change 'Hold pickup library match' to "Item's hold group" 8) Repeat steps 5 to 7 SUCCESS => when the page is loaded, you should see the "Pickup at" select filtered by libraries in hold group => You should see in "Allowed pickup locations" a coma separated list of the libraries in item's hold group => If biblio has an item whose control branch is not in a hold group, you should see the control branch name in "Allowed pickup locations" 9) Sign off Sponsored-by: VOKAL Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Created attachment 89845 [details] [review] Bug 22284: Filter item's pickup locations in holds_table.inc This patch filters pickup locations per item in holds_table.inc To test: 1) Go to Circulation and fines rules and change max holds allowed to > 0, and change 'Hold pickup library match' to homebranch 2) Search for a patron 3) Click on "Search to hold" 4) Search for any bilio 5) Place hold on a specific item CHECK => Item's pickup location is a select with all libraries 6) Apply this patch 7) Repeat steps 2 to 5 SUCCESS => Item's pickup location is filtered by allowed pickup locations 8) Sign off Sponsored-by: VOKAL Created attachment 89846 [details] [review] Bug 22284: (follow-up) Remove commented warn and address test failures Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Created attachment 89847 [details] [review] Bug 22284: (follow-up) fix test count after merge Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Created attachment 89848 [details] [review] Bug 22284: (follow-up) fixes after 15496 Created attachment 89849 [details] [review] Bug 22284: (follow-up) fixes after 18936 Hi Agustin, Thanks for updating your patches, I've had a look over them and the behaviour seems better to me. I'm willing to sign this off, but I'd really like for VOKAL to test it with their real-life requirements and scenarios in mind - I'm not with that institution and I can't purport to speak to their functional requirements. I've approached Joy at ByWater to see if she can arrange a testing setup for them with this patchset, hopefully we'll hear something soon about that - I know how frustrating it is to endlessly rebase. Note to QA: I haven't attached signed off patches, pending VOKAL's testing/comments. Let me know if you'd like me to do something differently. The dev itself looks solid, passes the QA scripts and all of your new tests are passing. The functionality seems good so far as I can see, thank you for all of that and nice job! Liz (In reply to Liz Rea from comment #125) > Hi Agustin, > > Thanks for updating your patches, I've had a look over them and the > behaviour seems better to me. I'm willing to sign this off, but I'd really > like for VOKAL to test it with their real-life requirements and scenarios in > mind - I'm not with that institution and I can't purport to speak to their > functional requirements. > > I've approached Joy at ByWater to see if she can arrange a testing setup for > them with this patchset, hopefully we'll hear something soon about that - I > know how frustrating it is to endlessly rebase. > > Note to QA: I haven't attached signed off patches, pending VOKAL's > testing/comments. Let me know if you'd like me to do something differently. > > The dev itself looks solid, passes the QA scripts and all of your new tests > are passing. The functionality seems good so far as I can see, thank you for > all of that and nice job! > > Liz Hello Liz, thanks for letting me know. I won't touch this bug untill we've got feedback from VOKAL, but I can't help thinking on how ReserveControlBranch and HomeOrHolding preferences interact with each other. I think maybe (please correct me if I'm wrong) they work in the following way ReserveControlBranch tell's you which circulation rules should be applied.. for example: We got two libraries: Franklin and Centerville We got a patron from Franklin that want's to place a hold on an item from Centerville, so if ReserveControlBranch is set to patron's home library, we should use Franklin's rules.. else we should use Centerville's rules.. So, in this example: 1. If ReserveControlBranch is set to "patron's home library", and Franklin has a rule of "home library" the hold wouldn't be allowed. 2. If ReserveControlBranch is set to "item's home library" and Centerville has a rule of "any library" the hold would be allowed. Now for HomeOrHolding, I think this tell's you to which library it should be compared when checking the item.. so let's suppose in this same example that the item has as home library Centerville, but as holding library Franklin.. If ReserveControlBranch is set to "patron's home library" (and Franklin still has a rule of "home library") 1. If HomeOrHolding is set to "home", the hold would not be allowed 2. If HomeOrHolding is set to "holding", in this case hold would be allowed, because we would be comparing patron's home library with items holding library. What makes some noise to me, is that the rule name says "From home library", so maybe HomeOrHolding really doesn't apply at all, and we should always check for home library. Other thing I'm not sure is, let's suppose ReserveControBranch is set to "item's home library", should HomeOrHolding affect also which rule should we use?.. I mean, using the same example, if HomeOrHolding is set to "home", we should use Centerville's rules, else we shoul use Franklin's rules?... of course, if HomeOrHolding doesn't apply at all, this question has no sense also. Sorry for spitting all this out.. Agustín It seems to me that ReserveControlBranch and HomeOrHolding are doing the same thing, but one for holds and the other one for checkout-ability. I've seen this pattern in Koha a lot, in which rules are duplicated to give more flexibility, and it makes sense. Even though it leads to impossible situations (i.e. you can place a hold on an item you cannot check out). Created attachment 90133 [details] [review] Bug 22284: (follow-up) Remove HomeOrHolding from reserves Created attachment 90134 [details] [review] Bug 22284: Add "patron's hold group" as new hold_fulfillment_policy option This patch adds "patron's hold group" as a new option to Hold pickup library match To test: 1. Set ReservesControlBranch preference to item. 2. Create a hold group 3. Go to circulation and fines rules SUCCESS => in 'Hold pickup library match' there is a new option called "patron's hold group" 4. In a library not in hold group set 'Hold policy' to "any" and 'Hold pickup library match' to "patron's hold group" 5. Search for a user in the hold group 6. 'Search to hold' for items of the library of step 4 7. Select an item and 'Place hold for [user]' SUCCESS => in 'Pickup at' you should see patron's hold group as options 8. In OPAC sign in as the same user of step 5 9. Search for the item in step 7 SUCCESS => in 'Pick up locations' you should see patron's hold group as options 10. Sign off Test will be fixed in next follow-up Created attachment 90197 [details] [review] Bug 22284: (follow-up) Use GetReserveControlBranch in Koha::Item->pickup_locations Created attachment 90198 [details] [review] Bug 22284: (follow-up) Fix tests Created attachment 90199 [details] [review] Bug 22284: Add "patron's hold group" as new hold_fulfillment_policy option This patch adds "patron's hold group" as a new option to Hold pickup library match To test: 1. Set ReservesControlBranch preference to item. 2. Create a hold group 3. Go to circulation and fines rules SUCCESS => in 'Hold pickup library match' there is a new option called "patron's hold group" 4. In a library not in hold group set 'Hold policy' to "any" and 'Hold pickup library match' to "patron's hold group" 5. Search for a user in the hold group 6. 'Search to hold' for items of the library of step 4 7. Select an item and 'Place hold for [user]' SUCCESS => in 'Pickup at' you should see patron's hold group as options 8. In OPAC sign in as the same user of step 5 9. Search for the item in step 7 SUCCESS => in 'Pick up locations' you should see patron's hold group as options 10. Sign off Created attachment 90200 [details] [review] Bug 22284: (follow-up) Use GetReserveControlBranch in Koha::Item->pickup_locations Created attachment 90201 [details] [review] Bug 22284: (follow-up) Fix tests Ok, I can see this bug just became a big mess (this comment is going to be number 137!).. To spare those of you, brave enough to test this, I'll try to make a unified test plan To test: 1) Apply all patches (including dependencies) 2) perl installer/data/mysql/updatedatabase.pl 3) dbic ## Library groups ## # new checkbox option 'Is local hold group' on root groups 4) In intranet go to Library groups in admin module 5) Add new root group SUCCESS => a new 'Is local hold group' checkbox appears 6) Select 'Is local hold group' and save 7) Click edit button for root group SUCCESS => 'Is local hold group' is checked ## smart-rules ## # new options for 'Hold policy' and 'Hold fulfillment policy' 8) Go to 'Circulation and fines rules' in admin SUCCESS => in 'Hold policy' you should see 'From local hold group' option, and in 'Hold pickup library match' you should see "Item's hold group" and "Patron's hold group" new options. 9) Select any of those options and save SUCCESS => options should be selected when page reloads ## pickup_locations ## # modified algorithm for obtaining pickup_locations 10) intranet: in Library groups, add 2 root groups marked as local hold group and add different libraries to each. 11) opac: login as a user of a library belonging to one hold group, and search try to place a hold on an item belongin to the other hold group. 12) intranet: in Circulation and fines rules, play with 'Hold policy' and 'Hold pickup library match' rules. 13) opac: On each modification of the rules reload the page. SUCCESS => Every time you reload the page, the number of pickup locations showed in select varies. ## reserve/request.tt ## # new message and column in 'Place a hold on a specific item' table and filtered pickup select 14) In 'Circulation and fines rules', in 'Default checkout, hold and return policy', in 'Hold policy' change the value to 'From local hold group' and 'Hold pickup library match' to 'any' 15) Search a patron from a library not belonging to any holdgroup of step 10, select one and click 'search to hold' 16) Search by location for items in any library of step 10 17) On any item, clic on 'Place hold for ...' SUCCESS => when the page is loaded, in the 'Place a hold on a specific item' table, you should see the message "Cannot place hold from patrons's library" in 'Hold' column => You should see a new column called "Allowed pickup locations" and the message is "Any library" => Item's pickup locations select should have all libraries 18) In 'Circulation and fines rules', in 'Default checkout, hold and return policy', in 'Hold policy' change the value again to 'From any library' and change 'Hold pickup library match' to "Item's hold group" 19) Repeat steps 15 to 17 SUCCESS => when the page is loaded, you should see the "Pickup at" select filtered by libraries in hold group => You should see in "Allowed pickup locations" a coma separated list of the libraries in item's hold group => If biblio has an item whose control branch is not in a hold group, you should see the control branch name in "Allowed pickup locations" => Item's pickup location select is filtered by allowed pickup locations ## Tests ## 20) prove t/db_dependent/Holds.t 21) prove t/db_dependent/Koha/Biblios.t 22) prove t/db_dependent/Koha/Items.t 23) prove t/db_dependent/Koha/Libraries.t 24) prove t/db_dependent/LibraryGroups.t ## Finally (\o/ hurray!!) 25) Sing off patches! Hi Agustin, This is looking pretty good, a few things to note: - there's a typo in the Hold column "Cannot place hold from patrons's library" should be "Cannot place hold from patron's library" When an item is selected in the intranet for an item level hold, the location select shows all of the libraries from every group represented on the page (i.e. if you have group 1 CPL, MPL and group 2 UNI, IPL, the list will show CPL, MPL, UNI, IPL in the pickup locations. There are test failures: Holds.t # Failed test 'can request item if policy that matches on item type allows it' # 48 I can add a follow up for the typo - and I will, if you can have a look at the other stuff! This is a really good feature I think - libraries will really like having the option to do these things. Cheers, Liz Created attachment 90469 [details] [review] Bug 22284: (follow-up) Fix typo in request.tt Created attachment 90525 [details] [review] Bug 22284: (follow-up) Filter pickup on specific item click Hi Liz > - there's a typo in the Hold column "Cannot place hold from patrons's > library" should be "Cannot place hold from patron's library" Fixed! > When an item is selected in the intranet for an item level hold, the > location select shows all of the libraries from every group represented on > the page (i.e. if you have group 1 CPL, MPL and group 2 UNI, IPL, the list > will show CPL, MPL, UNI, IPL in the pickup locations. Done! > > There are test failures: > > Holds.t > # Failed test 'can request item if policy that matches on item type allows > it' > # 48 Sorry, I cannot reproduce test failure $ prove t/db_dependent/Holds.t t/db_dependent/Holds.t .. ok All tests successful. Files=1, Tests=61, 6 wallclock secs ( 0.03 usr 0.02 sys + 3.77 cusr 0.57 csys = 4.39 CPU) Result: PASS Thanks, Agustín Created attachment 91261 [details] [review] Bug 22284: New ft_local_hold_group column, and 'holgroup' enum option In order to add groups for holds, this patch provides a new column in library_groups to identify them. Also provides new enum option for hold_fulfillment_policy column of default_branch_circ_rules, default_circ_rules, default_branch_item_rules and branch_item_rules. To test: 1) check that there is no ft_local_hold_group column in library_groups and that there are no 'holdgroup' enum option in hold_fulfillment_policy in default_branch_circ_rules, default_circ_rules, default_branch_item_rules and branch_item_rules 2) apply this patch 3) perl installer/data/mysql/updatedatabase.pl && dbic SUCCESS => !(step 1) ;-D 4) Sign off Sponsored-by: VOKAL Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: Josef Moravec <josef.moravec@gmail.com> Created attachment 91262 [details] [review] Bug 22284: Add new checkbox in Library Groups This patch adds a new checkbox option 'Is local hold group' when you create a new root group. To test: 1) In intranet go to Library Groups in admin module 2) Add new root group 3) Check that there is no 'Is local hold group' checkbox 4) Apply this patch 5) Add new root group SUCCESS => a new 'Is local hold group' checkbox appears 6) Select 'Is local hold group' and save 7) Click edit button for root group SUCCESS => 'Is local hold group' is checked 8) Sign off Sponsored-by: VOKAL Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: Josef Moravec <josef.moravec@gmail.com> Created attachment 91263 [details] [review] Bug 22284: New hold group options in smart-rules This patch adds new options for 'Default checkout, hold and return policy' and 'Default holds policy by item type', both in 'Hold policy' and 'Hold pickup library match' columns To test: 1) Go to 'Circulation and fines rules' in admin 2) Check that there are no options for hold group in 'Hold policy' and 'Hold pickup library match' columns 3) Apply this patch SUCCESS => in 'Hold policy' you should see 'From local hold group' option, and in 'Hold pickup library match' you should see "Item's hold group" 4) Select any of those options and save SUCCESS => options should be selected when page reloads 5) Sign off Sponsored-by: VOKAL Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: Josef Moravec <josef.moravec@gmail.com> Created attachment 91264 [details] [review] Bug 22284: Control hold groups in C4::Reserves This patch modifies C4::Reserves to control when hold group options where selected in smart rules. In CanItemBeReserved adds 2 new error status messages 1) branchNotInHoldGroup: when a patron's homebranch is not in item's hold group 2) pickupNotInHoldGroup: when a selected pickup location is not in item's hold group Also CheckReserves is modified when item's priority is defined, to control by hold group when required. Finally, IsAvailableForItemLevelRequest was also modified to control by hold group when required. To test: 1) Apply this patch 2) prove t/db_dependent/Holds.t SUCCESS => Result: PASS 3) Sign off Sponsored-by: VOKAL Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: Josef Moravec <josef.moravec@gmail.com> Created attachment 91265 [details] [review] Bug 22284: New methods in Koha::Library::Groups and Koha::Library This patch adds new methods in Koha::Library::Groups and Koha::Library. 1) For Koha::Library::Groups adds get_root_ancestor that returns all root groups for a given search parameters, for example Koha::Library::Groups->get_root_ancestor( { id => $group_id } ) 2) For Koha::Library adds 2.1) get_hold_libraries: returns all libraries (including self) that belongs to the same holdgroups. If $self belongs to several holdgroups it will return a distinct list of all libraries belonging to them. 2.2) validate_hold_sibling: Returns 1 if the given parameters matches any of the libraries that belong to any of the holdgroups this library belongs. For example $library->validate_hold_sibling( { branchcode => $branchcode } ) To test: 1) apply this patch 2) prove t/db_dependent/Koha/Libraries.t t/db_dependent/LibraryGroups.t SUCCESS => green letters :-D 3) Sign off Sponsored-by: VOKAL Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: Josef Moravec <josef.moravec@gmail.com> Created attachment 91266 [details] [review] Bug 22284: Opac pickup_locations This patch modifies Koha::Libraries->pickup_location and moves most of the logic to Koha::Item and Koha::Biblio in preparation for api endpoints in the future. There where 2 methods added 1) Koha::Item->pickup_locations that given a patron, returns all pickup locations of this item, considering hold fulfillment rules, and hold allowed rules. 2) Koha::Biblio->pickup_locations that given a patron, returns a distinct list of libraries returned by each of this biblio items pickup location. Koha::Libraries->pickup_location analyzes input param and calls Koha::Item->pickup_locations or Koha::Biblio->pickup_locations as needed. Also in opac-reserve.tt the way options where obtained to fill the pickup location select was modified to pass the patron as a parameter. To test: 1) opac: try to place hold on a item and check that all libraries are shown in the pickup location select. 2) intranet: in Library groups, add 2 root groups marked as local hold group and add different libraries to each. 3) opac: login as a user of a library belonging to one hold group, and search try to place a hold on an item belongin to the other hold group. 4) intranet: in Circulation and fines rules, play with 'Hold policy' and 'Hold pickup library match' rules. 5) opac: On each modification of the rules reload the page. SUCCESS => Every time you reload the page, the number of pickup locations showed in select varies. 6) prove t/db_dependent/Koha/Biblios.t t/db_dependent/Koha/Items.t SUCCESS => Result: PASS 7) Sign off Sponsored-by: VOKAL Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: Josef Moravec <josef.moravec@gmail.com> Created attachment 91267 [details] [review] Bug 22284: (follow-up) Default to circulation control branch when no hold libraries are found Signed-off-by: Josef Moravec <josef.moravec@gmail.com> Created attachment 91268 [details] [review] Bug 22284: New message, new column and filter pickup locations in reserve/request.tt This patch adds a new message to 'Hold' column in 'Place a hold on a specific item' table. The message is "Cannot place hold from patrons's library". It appears when patron's homebranch is not in item's hold group, and hold_fulfillment_policy is set to 'holdgroup'. This patch also adds a new column "Allowed pickup locations" that lists allowed pickup locations per item. Finally, the select that displays pickup locations is filtered by allowed pickup locations, when multi_hold is not enabled To test: 1) Apply this patch 2) In library groups add a root group and check it as hold group. 3) Add two libraries to the group 4) In circulation and fines rules, in 'Default checkout, hold and return policy', in Hold policy change the value to 'From local hold group' 5) Search a patron from a different library than step 3, select one and click 'search to hold' 6) Search by location for items in any library of step 3 7) On any item, clic on 'Place hold for ...' SUCCESS => when the page is loaded, in the 'Place a hold on a specific item', you should see the message "Cannot place hold from patrons's library" in 'Hold' column => You should see a new column called "Allowed pickup locations" and the message is "Any library" 8) In circulation and fines rules, in 'Default checkout, hold and return policy', in 'Hold policy' change the value again to 'From any library' and change 'Hold pickup library match' to "Item's hold group" 8) Repeat steps 5 to 7 SUCCESS => when the page is loaded, you should see the "Pickup at" select filtered by libraries in hold group => You should see in "Allowed pickup locations" a coma separated list of the libraries in item's hold group => If biblio has an item whose control branch is not in a hold group, you should see the control branch name in "Allowed pickup locations" 9) Sign off Sponsored-by: VOKAL Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: Josef Moravec <josef.moravec@gmail.com> Created attachment 91269 [details] [review] Bug 22284: Filter item's pickup locations in holds_table.inc This patch filters pickup locations per item in holds_table.inc To test: 1) Go to Circulation and fines rules and change max holds allowed to > 0, and change 'Hold pickup library match' to homebranch 2) Search for a patron 3) Click on "Search to hold" 4) Search for any bilio 5) Place hold on a specific item CHECK => Item's pickup location is a select with all libraries 6) Apply this patch 7) Repeat steps 2 to 5 SUCCESS => Item's pickup location is filtered by allowed pickup locations 8) Sign off Sponsored-by: VOKAL Signed-off-by: Josef Moravec <josef.moravec@gmail.com> Created attachment 91270 [details] [review] Bug 22284: (follow-up) Remove commented warn and address test failures Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: Josef Moravec <josef.moravec@gmail.com> Created attachment 91271 [details] [review] Bug 22284: (follow-up) fix test count after merge Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: Josef Moravec <josef.moravec@gmail.com> Created attachment 91272 [details] [review] Bug 22284: (follow-up) fixes after 15496 Signed-off-by: Josef Moravec <josef.moravec@gmail.com> Created attachment 91273 [details] [review] Bug 22284: (follow-up) fixes after 18936 Signed-off-by: Josef Moravec <josef.moravec@gmail.com> Created attachment 91274 [details] [review] Bug 22284: (follow-up) Remove HomeOrHolding from reserves Signed-off-by: Josef Moravec <josef.moravec@gmail.com> Created attachment 91275 [details] [review] Bug 22284: Add "patron's hold group" as new hold_fulfillment_policy option This patch adds "patron's hold group" as a new option to Hold pickup library match To test: 1. Set ReservesControlBranch preference to item. 2. Create a hold group 3. Go to circulation and fines rules SUCCESS => in 'Hold pickup library match' there is a new option called "patron's hold group" 4. In a library not in hold group set 'Hold policy' to "any" and 'Hold pickup library match' to "patron's hold group" 5. Search for a user in the hold group 6. 'Search to hold' for items of the library of step 4 7. Select an item and 'Place hold for [user]' SUCCESS => in 'Pickup at' you should see patron's hold group as options 8. In OPAC sign in as the same user of step 5 9. Search for the item in step 7 SUCCESS => in 'Pick up locations' you should see patron's hold group as options 10. Sign off Signed-off-by: Josef Moravec <josef.moravec@gmail.com> Created attachment 91276 [details] [review] Bug 22284: (follow-up) Use GetReserveControlBranch in Koha::Item->pickup_locations Signed-off-by: Josef Moravec <josef.moravec@gmail.com> Created attachment 91277 [details] [review] Bug 22284: (follow-up) Fix tests Signed-off-by: Josef Moravec <josef.moravec@gmail.com> Created attachment 91278 [details] [review] Bug 22284: (follow-up) Fix typo in request.tt Signed-off-by: Josef Moravec <josef.moravec@gmail.com> Created attachment 91279 [details] [review] Bug 22284: (follow-up) Filter pickup on specific item click Signed-off-by: Josef Moravec <josef.moravec@gmail.com> Created attachment 91280 [details] [review] Bug 22284: (Do not push) Schema update Signed-off-by: Josef Moravec <josef.moravec@gmail.com> Created attachment 91282 [details] [review] Bug 22284: (follow-up) Fix typos transfered -> transferred Signed-off-by: Josef Moravec <josef.moravec@gmail.com> Hi, This isn't currently applying I'm very sorry: Applying: Bug 22284: Opac pickup_locations Using index info to reconstruct a base tree... M koha-tmpl/opac-tmpl/bootstrap/en/modules/opac-reserve.tt Falling back to patching base and 3-way merge... Auto-merging koha-tmpl/opac-tmpl/bootstrap/en/modules/opac-reserve.tt CONFLICT (content): Merge conflict in koha-tmpl/opac-tmpl/bootstrap/en/modules/opac-reserve.tt Cheers, Liz Created attachment 91701 [details] [review] Bug 22284: New ft_local_hold_group column, and 'holgroup' enum option In order to add groups for holds, this patch provides a new column in library_groups to identify them. Also provides new enum option for hold_fulfillment_policy column of default_branch_circ_rules, default_circ_rules, default_branch_item_rules and branch_item_rules. To test: 1) check that there is no ft_local_hold_group column in library_groups and that there are no 'holdgroup' enum option in hold_fulfillment_policy in default_branch_circ_rules, default_circ_rules, default_branch_item_rules and branch_item_rules 2) apply this patch 3) perl installer/data/mysql/updatedatabase.pl && dbic SUCCESS => !(step 1) ;-D 4) Sign off Sponsored-by: VOKAL Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: Josef Moravec <josef.moravec@gmail.com> Created attachment 91702 [details] [review] Bug 22284: Add new checkbox in Library Groups This patch adds a new checkbox option 'Is local hold group' when you create a new root group. To test: 1) In intranet go to Library Groups in admin module 2) Add new root group 3) Check that there is no 'Is local hold group' checkbox 4) Apply this patch 5) Add new root group SUCCESS => a new 'Is local hold group' checkbox appears 6) Select 'Is local hold group' and save 7) Click edit button for root group SUCCESS => 'Is local hold group' is checked 8) Sign off Sponsored-by: VOKAL Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: Josef Moravec <josef.moravec@gmail.com> Created attachment 91703 [details] [review] Bug 22284: New hold group options in smart-rules This patch adds new options for 'Default checkout, hold and return policy' and 'Default holds policy by item type', both in 'Hold policy' and 'Hold pickup library match' columns To test: 1) Go to 'Circulation and fines rules' in admin 2) Check that there are no options for hold group in 'Hold policy' and 'Hold pickup library match' columns 3) Apply this patch SUCCESS => in 'Hold policy' you should see 'From local hold group' option, and in 'Hold pickup library match' you should see "Item's hold group" 4) Select any of those options and save SUCCESS => options should be selected when page reloads 5) Sign off Sponsored-by: VOKAL Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: Josef Moravec <josef.moravec@gmail.com> Created attachment 91704 [details] [review] Bug 22284: Control hold groups in C4::Reserves This patch modifies C4::Reserves to control when hold group options where selected in smart rules. In CanItemBeReserved adds 2 new error status messages 1) branchNotInHoldGroup: when a patron's homebranch is not in item's hold group 2) pickupNotInHoldGroup: when a selected pickup location is not in item's hold group Also CheckReserves is modified when item's priority is defined, to control by hold group when required. Finally, IsAvailableForItemLevelRequest was also modified to control by hold group when required. To test: 1) Apply this patch 2) prove t/db_dependent/Holds.t SUCCESS => Result: PASS 3) Sign off Sponsored-by: VOKAL Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: Josef Moravec <josef.moravec@gmail.com> Created attachment 91705 [details] [review] Bug 22284: New methods in Koha::Library::Groups and Koha::Library This patch adds new methods in Koha::Library::Groups and Koha::Library. 1) For Koha::Library::Groups adds get_root_ancestor that returns all root groups for a given search parameters, for example Koha::Library::Groups->get_root_ancestor( { id => $group_id } ) 2) For Koha::Library adds 2.1) get_hold_libraries: returns all libraries (including self) that belongs to the same holdgroups. If $self belongs to several holdgroups it will return a distinct list of all libraries belonging to them. 2.2) validate_hold_sibling: Returns 1 if the given parameters matches any of the libraries that belong to any of the holdgroups this library belongs. For example $library->validate_hold_sibling( { branchcode => $branchcode } ) To test: 1) apply this patch 2) prove t/db_dependent/Koha/Libraries.t t/db_dependent/LibraryGroups.t SUCCESS => green letters :-D 3) Sign off Sponsored-by: VOKAL Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: Josef Moravec <josef.moravec@gmail.com> Created attachment 91706 [details] [review] Bug 22284: Opac pickup_locations This patch modifies Koha::Libraries->pickup_location and moves most of the logic to Koha::Item and Koha::Biblio in preparation for api endpoints in the future. There where 2 methods added 1) Koha::Item->pickup_locations that given a patron, returns all pickup locations of this item, considering hold fulfillment rules, and hold allowed rules. 2) Koha::Biblio->pickup_locations that given a patron, returns a distinct list of libraries returned by each of this biblio items pickup location. Koha::Libraries->pickup_location analyzes input param and calls Koha::Item->pickup_locations or Koha::Biblio->pickup_locations as needed. Also in opac-reserve.tt the way options where obtained to fill the pickup location select was modified to pass the patron as a parameter. To test: 1) opac: try to place hold on a item and check that all libraries are shown in the pickup location select. 2) intranet: in Library groups, add 2 root groups marked as local hold group and add different libraries to each. 3) opac: login as a user of a library belonging to one hold group, and search try to place a hold on an item belongin to the other hold group. 4) intranet: in Circulation and fines rules, play with 'Hold policy' and 'Hold pickup library match' rules. 5) opac: On each modification of the rules reload the page. SUCCESS => Every time you reload the page, the number of pickup locations showed in select varies. 6) prove t/db_dependent/Koha/Biblios.t t/db_dependent/Koha/Items.t SUCCESS => Result: PASS 7) Sign off Sponsored-by: VOKAL Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: Josef Moravec <josef.moravec@gmail.com> Created attachment 91707 [details] [review] Bug 22284: (follow-up) Default to circulation control branch when no hold libraries are found Signed-off-by: Josef Moravec <josef.moravec@gmail.com> Created attachment 91708 [details] [review] Bug 22284: New message, new column and filter pickup locations in reserve/request.tt This patch adds a new message to 'Hold' column in 'Place a hold on a specific item' table. The message is "Cannot place hold from patrons's library". It appears when patron's homebranch is not in item's hold group, and hold_fulfillment_policy is set to 'holdgroup'. This patch also adds a new column "Allowed pickup locations" that lists allowed pickup locations per item. Finally, the select that displays pickup locations is filtered by allowed pickup locations, when multi_hold is not enabled To test: 1) Apply this patch 2) In library groups add a root group and check it as hold group. 3) Add two libraries to the group 4) In circulation and fines rules, in 'Default checkout, hold and return policy', in Hold policy change the value to 'From local hold group' 5) Search a patron from a different library than step 3, select one and click 'search to hold' 6) Search by location for items in any library of step 3 7) On any item, clic on 'Place hold for ...' SUCCESS => when the page is loaded, in the 'Place a hold on a specific item', you should see the message "Cannot place hold from patrons's library" in 'Hold' column => You should see a new column called "Allowed pickup locations" and the message is "Any library" 8) In circulation and fines rules, in 'Default checkout, hold and return policy', in 'Hold policy' change the value again to 'From any library' and change 'Hold pickup library match' to "Item's hold group" 8) Repeat steps 5 to 7 SUCCESS => when the page is loaded, you should see the "Pickup at" select filtered by libraries in hold group => You should see in "Allowed pickup locations" a coma separated list of the libraries in item's hold group => If biblio has an item whose control branch is not in a hold group, you should see the control branch name in "Allowed pickup locations" 9) Sign off Sponsored-by: VOKAL Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: Josef Moravec <josef.moravec@gmail.com> Created attachment 91709 [details] [review] Bug 22284: Filter item's pickup locations in holds_table.inc This patch filters pickup locations per item in holds_table.inc To test: 1) Go to Circulation and fines rules and change max holds allowed to > 0, and change 'Hold pickup library match' to homebranch 2) Search for a patron 3) Click on "Search to hold" 4) Search for any bilio 5) Place hold on a specific item CHECK => Item's pickup location is a select with all libraries 6) Apply this patch 7) Repeat steps 2 to 5 SUCCESS => Item's pickup location is filtered by allowed pickup locations 8) Sign off Sponsored-by: VOKAL Signed-off-by: Josef Moravec <josef.moravec@gmail.com> Created attachment 91710 [details] [review] Bug 22284: (follow-up) Remove commented warn and address test failures Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: Josef Moravec <josef.moravec@gmail.com> Created attachment 91711 [details] [review] Bug 22284: (follow-up) fix test count after merge Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: Josef Moravec <josef.moravec@gmail.com> Created attachment 91712 [details] [review] Bug 22284: (follow-up) fixes after 15496 Signed-off-by: Josef Moravec <josef.moravec@gmail.com> Created attachment 91713 [details] [review] Bug 22284: (follow-up) fixes after 18936 Signed-off-by: Josef Moravec <josef.moravec@gmail.com> Created attachment 91714 [details] [review] Bug 22284: (follow-up) Remove HomeOrHolding from reserves Signed-off-by: Josef Moravec <josef.moravec@gmail.com> Created attachment 91715 [details] [review] Bug 22284: Add "patron's hold group" as new hold_fulfillment_policy option This patch adds "patron's hold group" as a new option to Hold pickup library match To test: 1. Set ReservesControlBranch preference to item. 2. Create a hold group 3. Go to circulation and fines rules SUCCESS => in 'Hold pickup library match' there is a new option called "patron's hold group" 4. In a library not in hold group set 'Hold policy' to "any" and 'Hold pickup library match' to "patron's hold group" 5. Search for a user in the hold group 6. 'Search to hold' for items of the library of step 4 7. Select an item and 'Place hold for [user]' SUCCESS => in 'Pickup at' you should see patron's hold group as options 8. In OPAC sign in as the same user of step 5 9. Search for the item in step 7 SUCCESS => in 'Pick up locations' you should see patron's hold group as options 10. Sign off Signed-off-by: Josef Moravec <josef.moravec@gmail.com> Created attachment 91716 [details] [review] Bug 22284: (follow-up) Use GetReserveControlBranch in Koha::Item->pickup_locations Signed-off-by: Josef Moravec <josef.moravec@gmail.com> Created attachment 91717 [details] [review] Bug 22284: (follow-up) Fix tests Signed-off-by: Josef Moravec <josef.moravec@gmail.com> Created attachment 91718 [details] [review] Bug 22284: (follow-up) Fix typo in request.tt Signed-off-by: Josef Moravec <josef.moravec@gmail.com> Created attachment 91719 [details] [review] Bug 22284: (follow-up) Filter pickup on specific item click Signed-off-by: Josef Moravec <josef.moravec@gmail.com> Created attachment 91720 [details] [review] Bug 22284: (Do not push) Schema update Signed-off-by: Josef Moravec <josef.moravec@gmail.com> Created attachment 91721 [details] [review] Bug 22284: (follow-up) Fix typos transfered -> transferred Signed-off-by: Josef Moravec <josef.moravec@gmail.com> Created attachment 91750 [details] [review] Bug 22284: (follow-up) fixes after 18936 Signed-off-by: Josef Moravec <josef.moravec@gmail.com> Created attachment 91751 [details] [review] Bug 22284: (follow-up) Remove HomeOrHolding from reserves Signed-off-by: Josef Moravec <josef.moravec@gmail.com> Created attachment 91752 [details] [review] Bug 22284: Add "patron's hold group" as new hold_fulfillment_policy option This patch adds "patron's hold group" as a new option to Hold pickup library match To test: 1. Set ReservesControlBranch preference to item. 2. Create a hold group 3. Go to circulation and fines rules SUCCESS => in 'Hold pickup library match' there is a new option called "patron's hold group" 4. In a library not in hold group set 'Hold policy' to "any" and 'Hold pickup library match' to "patron's hold group" 5. Search for a user in the hold group 6. 'Search to hold' for items of the library of step 4 7. Select an item and 'Place hold for [user]' SUCCESS => in 'Pickup at' you should see patron's hold group as options 8. In OPAC sign in as the same user of step 5 9. Search for the item in step 7 SUCCESS => in 'Pick up locations' you should see patron's hold group as options 10. Sign off Signed-off-by: Josef Moravec <josef.moravec@gmail.com> Created attachment 91753 [details] [review] Bug 22284: (follow-up) Use GetReserveControlBranch in Koha::Item->pickup_locations Signed-off-by: Josef Moravec <josef.moravec@gmail.com> Created attachment 91754 [details] [review] Bug 22284: (follow-up) Fix tests Signed-off-by: Josef Moravec <josef.moravec@gmail.com> Created attachment 91755 [details] [review] Bug 22284: (follow-up) Fix typo in request.tt Signed-off-by: Josef Moravec <josef.moravec@gmail.com> Created attachment 91756 [details] [review] Bug 22284: (follow-up) Filter pickup on specific item click Signed-off-by: Josef Moravec <josef.moravec@gmail.com> Created attachment 91757 [details] [review] Bug 22284: (Do not push) Schema update Signed-off-by: Josef Moravec <josef.moravec@gmail.com> Created attachment 91758 [details] [review] Bug 22284: (follow-up) Fix typos transfered -> transferred Signed-off-by: Josef Moravec <josef.moravec@gmail.com> Hi, Applied this today with an eye to sign it off, and got a merge conflict :( Applying: Bug 22284: New message, new column and filter pickup locations in reserve/request.tt Using index info to reconstruct a base tree... M koha-tmpl/intranet-tmpl/prog/en/modules/reserve/request.tt M reserve/request.pl Falling back to patching base and 3-way merge... Auto-merging reserve/request.pl Auto-merging koha-tmpl/intranet-tmpl/prog/en/modules/reserve/request.tt CONFLICT (content): Merge conflict in koha-tmpl/intranet-tmpl/prog/en/modules/reserve/request.tt error: Failed to merge in the changes. Patch failed at 0001 Bug 22284: New message, new column and filter pickup locations in reserve/request.tt Created attachment 92554 [details] [review] Bug 22284: New ft_local_hold_group column, and 'holgroup' enum option In order to add groups for holds, this patch provides a new column in library_groups to identify them. Also provides new enum option for hold_fulfillment_policy column of default_branch_circ_rules, default_circ_rules, default_branch_item_rules and branch_item_rules. To test: 1) check that there is no ft_local_hold_group column in library_groups and that there are no 'holdgroup' enum option in hold_fulfillment_policy in default_branch_circ_rules, default_circ_rules, default_branch_item_rules and branch_item_rules 2) apply this patch 3) perl installer/data/mysql/updatedatabase.pl && dbic SUCCESS => !(step 1) ;-D 4) Sign off Sponsored-by: VOKAL Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: Josef Moravec <josef.moravec@gmail.com> Created attachment 92555 [details] [review] Bug 22284: Add new checkbox in Library Groups This patch adds a new checkbox option 'Is local hold group' when you create a new root group. To test: 1) In intranet go to Library Groups in admin module 2) Add new root group 3) Check that there is no 'Is local hold group' checkbox 4) Apply this patch 5) Add new root group SUCCESS => a new 'Is local hold group' checkbox appears 6) Select 'Is local hold group' and save 7) Click edit button for root group SUCCESS => 'Is local hold group' is checked 8) Sign off Sponsored-by: VOKAL Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: Josef Moravec <josef.moravec@gmail.com> Created attachment 92556 [details] [review] Bug 22284: New hold group options in smart-rules This patch adds new options for 'Default checkout, hold and return policy' and 'Default holds policy by item type', both in 'Hold policy' and 'Hold pickup library match' columns To test: 1) Go to 'Circulation and fines rules' in admin 2) Check that there are no options for hold group in 'Hold policy' and 'Hold pickup library match' columns 3) Apply this patch SUCCESS => in 'Hold policy' you should see 'From local hold group' option, and in 'Hold pickup library match' you should see "Item's hold group" 4) Select any of those options and save SUCCESS => options should be selected when page reloads 5) Sign off Sponsored-by: VOKAL Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: Josef Moravec <josef.moravec@gmail.com> Created attachment 92557 [details] [review] Bug 22284: Control hold groups in C4::Reserves This patch modifies C4::Reserves to control when hold group options where selected in smart rules. In CanItemBeReserved adds 2 new error status messages 1) branchNotInHoldGroup: when a patron's homebranch is not in item's hold group 2) pickupNotInHoldGroup: when a selected pickup location is not in item's hold group Also CheckReserves is modified when item's priority is defined, to control by hold group when required. Finally, IsAvailableForItemLevelRequest was also modified to control by hold group when required. To test: 1) Apply this patch 2) prove t/db_dependent/Holds.t SUCCESS => Result: PASS 3) Sign off Sponsored-by: VOKAL Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: Josef Moravec <josef.moravec@gmail.com> Created attachment 92558 [details] [review] Bug 22284: New methods in Koha::Library::Groups and Koha::Library This patch adds new methods in Koha::Library::Groups and Koha::Library. 1) For Koha::Library::Groups adds get_root_ancestor that returns all root groups for a given search parameters, for example Koha::Library::Groups->get_root_ancestor( { id => $group_id } ) 2) For Koha::Library adds 2.1) get_hold_libraries: returns all libraries (including self) that belongs to the same holdgroups. If $self belongs to several holdgroups it will return a distinct list of all libraries belonging to them. 2.2) validate_hold_sibling: Returns 1 if the given parameters matches any of the libraries that belong to any of the holdgroups this library belongs. For example $library->validate_hold_sibling( { branchcode => $branchcode } ) To test: 1) apply this patch 2) prove t/db_dependent/Koha/Libraries.t t/db_dependent/LibraryGroups.t SUCCESS => green letters :-D 3) Sign off Sponsored-by: VOKAL Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: Josef Moravec <josef.moravec@gmail.com> Created attachment 92559 [details] [review] Bug 22284: Opac pickup_locations This patch modifies Koha::Libraries->pickup_location and moves most of the logic to Koha::Item and Koha::Biblio in preparation for api endpoints in the future. There where 2 methods added 1) Koha::Item->pickup_locations that given a patron, returns all pickup locations of this item, considering hold fulfillment rules, and hold allowed rules. 2) Koha::Biblio->pickup_locations that given a patron, returns a distinct list of libraries returned by each of this biblio items pickup location. Koha::Libraries->pickup_location analyzes input param and calls Koha::Item->pickup_locations or Koha::Biblio->pickup_locations as needed. Also in opac-reserve.tt the way options where obtained to fill the pickup location select was modified to pass the patron as a parameter. To test: 1) opac: try to place hold on a item and check that all libraries are shown in the pickup location select. 2) intranet: in Library groups, add 2 root groups marked as local hold group and add different libraries to each. 3) opac: login as a user of a library belonging to one hold group, and search try to place a hold on an item belongin to the other hold group. 4) intranet: in Circulation and fines rules, play with 'Hold policy' and 'Hold pickup library match' rules. 5) opac: On each modification of the rules reload the page. SUCCESS => Every time you reload the page, the number of pickup locations showed in select varies. 6) prove t/db_dependent/Koha/Biblios.t t/db_dependent/Koha/Items.t SUCCESS => Result: PASS 7) Sign off Sponsored-by: VOKAL Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: Josef Moravec <josef.moravec@gmail.com> Created attachment 92560 [details] [review] Bug 22284: (follow-up) Default to circulation control branch when no hold libraries are found Signed-off-by: Josef Moravec <josef.moravec@gmail.com> Created attachment 92561 [details] [review] Bug 22284: New message, new column and filter pickup locations in reserve/request.tt This patch adds a new message to 'Hold' column in 'Place a hold on a specific item' table. The message is "Cannot place hold from patrons's library". It appears when patron's homebranch is not in item's hold group, and hold_fulfillment_policy is set to 'holdgroup'. This patch also adds a new column "Allowed pickup locations" that lists allowed pickup locations per item. Finally, the select that displays pickup locations is filtered by allowed pickup locations, when multi_hold is not enabled To test: 1) Apply this patch 2) In library groups add a root group and check it as hold group. 3) Add two libraries to the group 4) In circulation and fines rules, in 'Default checkout, hold and return policy', in Hold policy change the value to 'From local hold group' 5) Search a patron from a different library than step 3, select one and click 'search to hold' 6) Search by location for items in any library of step 3 7) On any item, clic on 'Place hold for ...' SUCCESS => when the page is loaded, in the 'Place a hold on a specific item', you should see the message "Cannot place hold from patrons's library" in 'Hold' column => You should see a new column called "Allowed pickup locations" and the message is "Any library" 8) In circulation and fines rules, in 'Default checkout, hold and return policy', in 'Hold policy' change the value again to 'From any library' and change 'Hold pickup library match' to "Item's hold group" 8) Repeat steps 5 to 7 SUCCESS => when the page is loaded, you should see the "Pickup at" select filtered by libraries in hold group => You should see in "Allowed pickup locations" a coma separated list of the libraries in item's hold group => If biblio has an item whose control branch is not in a hold group, you should see the control branch name in "Allowed pickup locations" 9) Sign off Sponsored-by: VOKAL Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: Josef Moravec <josef.moravec@gmail.com> Created attachment 92562 [details] [review] Bug 22284: Filter item's pickup locations in holds_table.inc This patch filters pickup locations per item in holds_table.inc To test: 1) Go to Circulation and fines rules and change max holds allowed to > 0, and change 'Hold pickup library match' to homebranch 2) Search for a patron 3) Click on "Search to hold" 4) Search for any bilio 5) Place hold on a specific item CHECK => Item's pickup location is a select with all libraries 6) Apply this patch 7) Repeat steps 2 to 5 SUCCESS => Item's pickup location is filtered by allowed pickup locations 8) Sign off Sponsored-by: VOKAL Signed-off-by: Josef Moravec <josef.moravec@gmail.com> Created attachment 92563 [details] [review] Bug 22284: (follow-up) Remove commented warn and address test failures Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: Josef Moravec <josef.moravec@gmail.com> Created attachment 92564 [details] [review] Bug 22284: (follow-up) fix test count after merge Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: Josef Moravec <josef.moravec@gmail.com> Created attachment 92565 [details] [review] Bug 22284: (follow-up) fixes after 15496 Signed-off-by: Josef Moravec <josef.moravec@gmail.com> Created attachment 92566 [details] [review] Bug 22284: (follow-up) fixes after 18936 Signed-off-by: Josef Moravec <josef.moravec@gmail.com> Created attachment 92567 [details] [review] Bug 22284: (follow-up) Remove HomeOrHolding from reserves Signed-off-by: Josef Moravec <josef.moravec@gmail.com> Created attachment 92568 [details] [review] Bug 22284: Add "patron's hold group" as new hold_fulfillment_policy option This patch adds "patron's hold group" as a new option to Hold pickup library match To test: 1. Set ReservesControlBranch preference to item. 2. Create a hold group 3. Go to circulation and fines rules SUCCESS => in 'Hold pickup library match' there is a new option called "patron's hold group" 4. In a library not in hold group set 'Hold policy' to "any" and 'Hold pickup library match' to "patron's hold group" 5. Search for a user in the hold group 6. 'Search to hold' for items of the library of step 4 7. Select an item and 'Place hold for [user]' SUCCESS => in 'Pickup at' you should see patron's hold group as options 8. In OPAC sign in as the same user of step 5 9. Search for the item in step 7 SUCCESS => in 'Pick up locations' you should see patron's hold group as options 10. Sign off Signed-off-by: Josef Moravec <josef.moravec@gmail.com> Created attachment 92569 [details] [review] Bug 22284: (follow-up) Use GetReserveControlBranch in Koha::Item->pickup_locations Signed-off-by: Josef Moravec <josef.moravec@gmail.com> Created attachment 92570 [details] [review] Bug 22284: (follow-up) Fix tests Signed-off-by: Josef Moravec <josef.moravec@gmail.com> Created attachment 92571 [details] [review] Bug 22284: (follow-up) Fix typo in request.tt Signed-off-by: Josef Moravec <josef.moravec@gmail.com> Created attachment 92572 [details] [review] Bug 22284: (follow-up) Filter pickup on specific item click Signed-off-by: Josef Moravec <josef.moravec@gmail.com> Created attachment 92573 [details] [review] Bug 22284: (Do not push) Schema update Signed-off-by: Josef Moravec <josef.moravec@gmail.com> Created attachment 92574 [details] [review] Bug 22284: (follow-up) Fix typos transfered -> transferred Signed-off-by: Josef Moravec <josef.moravec@gmail.com> Sponsored-by line is 'VOKAL'.. do they have an expanded name or is that it? (In reply to Martin Renvoize from comment #215) > Sponsored-by line is 'VOKAL'.. do they have an expanded name or is that it? `Vermont Organization of Koha Automated Libraries` perhaps? Created attachment 92752 [details] [review] Bug 22284: New ft_local_hold_group column, and 'holgroup' enum option In order to add groups for holds, this patch provides a new column in library_groups to identify them. Also provides new enum option for hold_fulfillment_policy column of default_branch_circ_rules, default_circ_rules, default_branch_item_rules and branch_item_rules. To test: 1) check that there is no ft_local_hold_group column in library_groups and that there are no 'holdgroup' enum option in hold_fulfillment_policy in default_branch_circ_rules, default_circ_rules, default_branch_item_rules and branch_item_rules 2) apply this patch 3) perl installer/data/mysql/updatedatabase.pl && dbic SUCCESS => !(step 1) ;-D 4) Sign off Sponsored-by: VOKAL Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: Josef Moravec <josef.moravec@gmail.com> Created attachment 92753 [details] [review] Bug 22284: Add new checkbox in Library Groups This patch adds a new checkbox option 'Is local hold group' when you create a new root group. To test: 1) In intranet go to Library Groups in admin module 2) Add new root group 3) Check that there is no 'Is local hold group' checkbox 4) Apply this patch 5) Add new root group SUCCESS => a new 'Is local hold group' checkbox appears 6) Select 'Is local hold group' and save 7) Click edit button for root group SUCCESS => 'Is local hold group' is checked 8) Sign off Sponsored-by: VOKAL Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: Josef Moravec <josef.moravec@gmail.com> Created attachment 92754 [details] [review] Bug 22284: New hold group options in smart-rules This patch adds new options for 'Default checkout, hold and return policy' and 'Default holds policy by item type', both in 'Hold policy' and 'Hold pickup library match' columns To test: 1) Go to 'Circulation and fines rules' in admin 2) Check that there are no options for hold group in 'Hold policy' and 'Hold pickup library match' columns 3) Apply this patch SUCCESS => in 'Hold policy' you should see 'From local hold group' option, and in 'Hold pickup library match' you should see "Item's hold group" 4) Select any of those options and save SUCCESS => options should be selected when page reloads 5) Sign off Sponsored-by: VOKAL Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: Josef Moravec <josef.moravec@gmail.com> Created attachment 92755 [details] [review] Bug 22284: Control hold groups in C4::Reserves This patch modifies C4::Reserves to control when hold group options where selected in smart rules. In CanItemBeReserved adds 2 new error status messages 1) branchNotInHoldGroup: when a patron's homebranch is not in item's hold group 2) pickupNotInHoldGroup: when a selected pickup location is not in item's hold group Also CheckReserves is modified when item's priority is defined, to control by hold group when required. Finally, IsAvailableForItemLevelRequest was also modified to control by hold group when required. To test: 1) Apply this patch 2) prove t/db_dependent/Holds.t SUCCESS => Result: PASS 3) Sign off Sponsored-by: VOKAL Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: Josef Moravec <josef.moravec@gmail.com> Created attachment 92756 [details] [review] Bug 22284: New methods in Koha::Library::Groups and Koha::Library This patch adds new methods in Koha::Library::Groups and Koha::Library. 1) For Koha::Library::Groups adds get_root_ancestor that returns all root groups for a given search parameters, for example Koha::Library::Groups->get_root_ancestor( { id => $group_id } ) 2) For Koha::Library adds 2.1) get_hold_libraries: returns all libraries (including self) that belongs to the same holdgroups. If $self belongs to several holdgroups it will return a distinct list of all libraries belonging to them. 2.2) validate_hold_sibling: Returns 1 if the given parameters matches any of the libraries that belong to any of the holdgroups this library belongs. For example $library->validate_hold_sibling( { branchcode => $branchcode } ) To test: 1) apply this patch 2) prove t/db_dependent/Koha/Libraries.t t/db_dependent/LibraryGroups.t SUCCESS => green letters :-D 3) Sign off Sponsored-by: VOKAL Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: Josef Moravec <josef.moravec@gmail.com> Created attachment 92757 [details] [review] Bug 22284: Opac pickup_locations This patch modifies Koha::Libraries->pickup_location and moves most of the logic to Koha::Item and Koha::Biblio in preparation for api endpoints in the future. There where 2 methods added 1) Koha::Item->pickup_locations that given a patron, returns all pickup locations of this item, considering hold fulfillment rules, and hold allowed rules. 2) Koha::Biblio->pickup_locations that given a patron, returns a distinct list of libraries returned by each of this biblio items pickup location. Koha::Libraries->pickup_location analyzes input param and calls Koha::Item->pickup_locations or Koha::Biblio->pickup_locations as needed. Also in opac-reserve.tt the way options where obtained to fill the pickup location select was modified to pass the patron as a parameter. To test: 1) opac: try to place hold on a item and check that all libraries are shown in the pickup location select. 2) intranet: in Library groups, add 2 root groups marked as local hold group and add different libraries to each. 3) opac: login as a user of a library belonging to one hold group, and search try to place a hold on an item belongin to the other hold group. 4) intranet: in Circulation and fines rules, play with 'Hold policy' and 'Hold pickup library match' rules. 5) opac: On each modification of the rules reload the page. SUCCESS => Every time you reload the page, the number of pickup locations showed in select varies. 6) prove t/db_dependent/Koha/Biblios.t t/db_dependent/Koha/Items.t SUCCESS => Result: PASS 7) Sign off Sponsored-by: VOKAL Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: Josef Moravec <josef.moravec@gmail.com> Created attachment 92758 [details] [review] Bug 22284: (follow-up) Default to circulation control branch when no hold libraries are found Signed-off-by: Josef Moravec <josef.moravec@gmail.com> Created attachment 92759 [details] [review] Bug 22284: New message, new column and filter pickup locations in reserve/request.tt This patch adds a new message to 'Hold' column in 'Place a hold on a specific item' table. The message is "Cannot place hold from patrons's library". It appears when patron's homebranch is not in item's hold group, and hold_fulfillment_policy is set to 'holdgroup'. This patch also adds a new column "Allowed pickup locations" that lists allowed pickup locations per item. Finally, the select that displays pickup locations is filtered by allowed pickup locations, when multi_hold is not enabled To test: 1) Apply this patch 2) In library groups add a root group and check it as hold group. 3) Add two libraries to the group 4) In circulation and fines rules, in 'Default checkout, hold and return policy', in Hold policy change the value to 'From local hold group' 5) Search a patron from a different library than step 3, select one and click 'search to hold' 6) Search by location for items in any library of step 3 7) On any item, clic on 'Place hold for ...' SUCCESS => when the page is loaded, in the 'Place a hold on a specific item', you should see the message "Cannot place hold from patrons's library" in 'Hold' column => You should see a new column called "Allowed pickup locations" and the message is "Any library" 8) In circulation and fines rules, in 'Default checkout, hold and return policy', in 'Hold policy' change the value again to 'From any library' and change 'Hold pickup library match' to "Item's hold group" 8) Repeat steps 5 to 7 SUCCESS => when the page is loaded, you should see the "Pickup at" select filtered by libraries in hold group => You should see in "Allowed pickup locations" a coma separated list of the libraries in item's hold group => If biblio has an item whose control branch is not in a hold group, you should see the control branch name in "Allowed pickup locations" 9) Sign off Sponsored-by: VOKAL Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: Josef Moravec <josef.moravec@gmail.com> Created attachment 92760 [details] [review] Bug 22284: Filter item's pickup locations in holds_table.inc This patch filters pickup locations per item in holds_table.inc To test: 1) Go to Circulation and fines rules and change max holds allowed to > 0, and change 'Hold pickup library match' to homebranch 2) Search for a patron 3) Click on "Search to hold" 4) Search for any bilio 5) Place hold on a specific item CHECK => Item's pickup location is a select with all libraries 6) Apply this patch 7) Repeat steps 2 to 5 SUCCESS => Item's pickup location is filtered by allowed pickup locations 8) Sign off Sponsored-by: VOKAL Signed-off-by: Josef Moravec <josef.moravec@gmail.com> Created attachment 92761 [details] [review] Bug 22284: (follow-up) Squash multiple follow-ups * Bug 22284: (follow-up) Remove commented warn and address test failures Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: Josef Moravec <josef.moravec@gmail.com> * Bug 22284: (follow-up) fix test count after merge Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: Josef Moravec <josef.moravec@gmail.com> * Bug 22284: (follow-up) fixes after 15496 Signed-off-by: Josef Moravec <josef.moravec@gmail.com> * Bug 22284: (follow-up) fixes after 18936 Signed-off-by: Josef Moravec <josef.moravec@gmail.com> * Bug 22284: (follow-up) Remove HomeOrHolding from reserves Signed-off-by: Josef Moravec <josef.moravec@gmail.com> Created attachment 92762 [details] [review] Bug 22284: Add "patron's hold group" as new hold_fulfillment_policy option This patch adds "patron's hold group" as a new option to Hold pickup library match To test: 1. Set ReservesControlBranch preference to item. 2. Create a hold group 3. Go to circulation and fines rules SUCCESS => in 'Hold pickup library match' there is a new option called "patron's hold group" 4. In a library not in hold group set 'Hold policy' to "any" and 'Hold pickup library match' to "patron's hold group" 5. Search for a user in the hold group 6. 'Search to hold' for items of the library of step 4 7. Select an item and 'Place hold for [user]' SUCCESS => in 'Pickup at' you should see patron's hold group as options 8. In OPAC sign in as the same user of step 5 9. Search for the item in step 7 SUCCESS => in 'Pick up locations' you should see patron's hold group as options 10. Sign off Signed-off-by: Josef Moravec <josef.moravec@gmail.com> Created attachment 92763 [details] [review] Bug 22284: (follow-up) Squash multiple follow-ups * Bug 22284: (follow-up) Use GetReserveControlBranch in Koha::Item->pickup_locations Signed-off-by: Josef Moravec <josef.moravec@gmail.com> * Bug 22284: (follow-up) Fix tests Signed-off-by: Josef Moravec <josef.moravec@gmail.com> * Bug 22284: (follow-up) Fix typo in request.tt Signed-off-by: Josef Moravec <josef.moravec@gmail.com> * Bug 22284: (follow-up) Filter pickup on specific item click Signed-off-by: Josef Moravec <josef.moravec@gmail.com> * Bug 22284: (follow-up) Fix typos transfered -> transferred Signed-off-by: Josef Moravec <josef.moravec@gmail.com> Created attachment 92764 [details] [review] Bug 22284: (Do not push) Schema update Signed-off-by: Josef Moravec <josef.moravec@gmail.com> Created attachment 92849 [details] [review] Bug 22284: New ft_local_hold_group column, and 'holgroup' enum option In order to add groups for holds, this patch provides a new column in library_groups to identify them. Also provides new enum option for hold_fulfillment_policy column of default_branch_circ_rules, default_circ_rules, default_branch_item_rules and branch_item_rules. To test: 1) check that there is no ft_local_hold_group column in library_groups and that there are no 'holdgroup' enum option in hold_fulfillment_policy in default_branch_circ_rules, default_circ_rules, default_branch_item_rules and branch_item_rules 2) apply this patch 3) perl installer/data/mysql/updatedatabase.pl && dbic SUCCESS => !(step 1) ;-D 4) Sign off Sponsored-by: VOKAL Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: Josef Moravec <josef.moravec@gmail.com> Signed-off-by: Liz Rea <wizzyrea@gmail.com> Created attachment 92850 [details] [review] Bug 22284: Add new checkbox in Library Groups This patch adds a new checkbox option 'Is local hold group' when you create a new root group. To test: 1) In intranet go to Library Groups in admin module 2) Add new root group 3) Check that there is no 'Is local hold group' checkbox 4) Apply this patch 5) Add new root group SUCCESS => a new 'Is local hold group' checkbox appears 6) Select 'Is local hold group' and save 7) Click edit button for root group SUCCESS => 'Is local hold group' is checked 8) Sign off Sponsored-by: VOKAL Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: Josef Moravec <josef.moravec@gmail.com> Signed-off-by: Liz Rea <wizzyrea@gmail.com> Created attachment 92851 [details] [review] Bug 22284: New hold group options in smart-rules This patch adds new options for 'Default checkout, hold and return policy' and 'Default holds policy by item type', both in 'Hold policy' and 'Hold pickup library match' columns To test: 1) Go to 'Circulation and fines rules' in admin 2) Check that there are no options for hold group in 'Hold policy' and 'Hold pickup library match' columns 3) Apply this patch SUCCESS => in 'Hold policy' you should see 'From local hold group' option, and in 'Hold pickup library match' you should see "Item's hold group" 4) Select any of those options and save SUCCESS => options should be selected when page reloads 5) Sign off Sponsored-by: VOKAL Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: Josef Moravec <josef.moravec@gmail.com> Signed-off-by: Liz Rea <wizzyrea@gmail.com> Created attachment 92852 [details] [review] Bug 22284: Control hold groups in C4::Reserves This patch modifies C4::Reserves to control when hold group options where selected in smart rules. In CanItemBeReserved adds 2 new error status messages 1) branchNotInHoldGroup: when a patron's homebranch is not in item's hold group 2) pickupNotInHoldGroup: when a selected pickup location is not in item's hold group Also CheckReserves is modified when item's priority is defined, to control by hold group when required. Finally, IsAvailableForItemLevelRequest was also modified to control by hold group when required. To test: 1) Apply this patch 2) prove t/db_dependent/Holds.t SUCCESS => Result: PASS 3) Sign off Sponsored-by: VOKAL Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: Josef Moravec <josef.moravec@gmail.com> Signed-off-by: Liz Rea <wizzyrea@gmail.com> Created attachment 92853 [details] [review] Bug 22284: New methods in Koha::Library::Groups and Koha::Library This patch adds new methods in Koha::Library::Groups and Koha::Library. 1) For Koha::Library::Groups adds get_root_ancestor that returns all root groups for a given search parameters, for example Koha::Library::Groups->get_root_ancestor( { id => $group_id } ) 2) For Koha::Library adds 2.1) get_hold_libraries: returns all libraries (including self) that belongs to the same holdgroups. If $self belongs to several holdgroups it will return a distinct list of all libraries belonging to them. 2.2) validate_hold_sibling: Returns 1 if the given parameters matches any of the libraries that belong to any of the holdgroups this library belongs. For example $library->validate_hold_sibling( { branchcode => $branchcode } ) To test: 1) apply this patch 2) prove t/db_dependent/Koha/Libraries.t t/db_dependent/LibraryGroups.t SUCCESS => green letters :-D 3) Sign off Sponsored-by: VOKAL Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: Josef Moravec <josef.moravec@gmail.com> Signed-off-by: Liz Rea <wizzyrea@gmail.com> Created attachment 92854 [details] [review] Bug 22284: Opac pickup_locations This patch modifies Koha::Libraries->pickup_location and moves most of the logic to Koha::Item and Koha::Biblio in preparation for api endpoints in the future. There where 2 methods added 1) Koha::Item->pickup_locations that given a patron, returns all pickup locations of this item, considering hold fulfillment rules, and hold allowed rules. 2) Koha::Biblio->pickup_locations that given a patron, returns a distinct list of libraries returned by each of this biblio items pickup location. Koha::Libraries->pickup_location analyzes input param and calls Koha::Item->pickup_locations or Koha::Biblio->pickup_locations as needed. Also in opac-reserve.tt the way options where obtained to fill the pickup location select was modified to pass the patron as a parameter. To test: 1) opac: try to place hold on a item and check that all libraries are shown in the pickup location select. 2) intranet: in Library groups, add 2 root groups marked as local hold group and add different libraries to each. 3) opac: login as a user of a library belonging to one hold group, and search try to place a hold on an item belongin to the other hold group. 4) intranet: in Circulation and fines rules, play with 'Hold policy' and 'Hold pickup library match' rules. 5) opac: On each modification of the rules reload the page. SUCCESS => Every time you reload the page, the number of pickup locations showed in select varies. 6) prove t/db_dependent/Koha/Biblios.t t/db_dependent/Koha/Items.t SUCCESS => Result: PASS 7) Sign off Sponsored-by: VOKAL Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: Josef Moravec <josef.moravec@gmail.com> Signed-off-by: Liz Rea <wizzyrea@gmail.com> Created attachment 92855 [details] [review] Bug 22284: (follow-up) Default to circulation control branch when no hold libraries are found Signed-off-by: Josef Moravec <josef.moravec@gmail.com> Signed-off-by: Liz Rea <wizzyrea@gmail.com> Created attachment 92856 [details] [review] Bug 22284: New message, new column and filter pickup locations in reserve/request.tt This patch adds a new message to 'Hold' column in 'Place a hold on a specific item' table. The message is "Cannot place hold from patrons's library". It appears when patron's homebranch is not in item's hold group, and hold_fulfillment_policy is set to 'holdgroup'. This patch also adds a new column "Allowed pickup locations" that lists allowed pickup locations per item. Finally, the select that displays pickup locations is filtered by allowed pickup locations, when multi_hold is not enabled To test: 1) Apply this patch 2) In library groups add a root group and check it as hold group. 3) Add two libraries to the group 4) In circulation and fines rules, in 'Default checkout, hold and return policy', in Hold policy change the value to 'From local hold group' 5) Search a patron from a different library than step 3, select one and click 'search to hold' 6) Search by location for items in any library of step 3 7) On any item, clic on 'Place hold for ...' SUCCESS => when the page is loaded, in the 'Place a hold on a specific item', you should see the message "Cannot place hold from patrons's library" in 'Hold' column => You should see a new column called "Allowed pickup locations" and the message is "Any library" 8) In circulation and fines rules, in 'Default checkout, hold and return policy', in 'Hold policy' change the value again to 'From any library' and change 'Hold pickup library match' to "Item's hold group" 8) Repeat steps 5 to 7 SUCCESS => when the page is loaded, you should see the "Pickup at" select filtered by libraries in hold group => You should see in "Allowed pickup locations" a coma separated list of the libraries in item's hold group => If biblio has an item whose control branch is not in a hold group, you should see the control branch name in "Allowed pickup locations" 9) Sign off Sponsored-by: VOKAL Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: Josef Moravec <josef.moravec@gmail.com> Signed-off-by: Liz Rea <wizzyrea@gmail.com> Created attachment 92857 [details] [review] Bug 22284: Filter item's pickup locations in holds_table.inc This patch filters pickup locations per item in holds_table.inc To test: 1) Go to Circulation and fines rules and change max holds allowed to > 0, and change 'Hold pickup library match' to homebranch 2) Search for a patron 3) Click on "Search to hold" 4) Search for any bilio 5) Place hold on a specific item CHECK => Item's pickup location is a select with all libraries 6) Apply this patch 7) Repeat steps 2 to 5 SUCCESS => Item's pickup location is filtered by allowed pickup locations 8) Sign off Sponsored-by: VOKAL Signed-off-by: Josef Moravec <josef.moravec@gmail.com> Signed-off-by: Liz Rea <wizzyrea@gmail.com> Created attachment 92858 [details] [review] Bug 22284: (follow-up) Squash multiple follow-ups * Bug 22284: (follow-up) Remove commented warn and address test failures Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: Josef Moravec <josef.moravec@gmail.com> * Bug 22284: (follow-up) fix test count after merge Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: Josef Moravec <josef.moravec@gmail.com> * Bug 22284: (follow-up) fixes after 15496 Signed-off-by: Josef Moravec <josef.moravec@gmail.com> * Bug 22284: (follow-up) fixes after 18936 Signed-off-by: Josef Moravec <josef.moravec@gmail.com> * Bug 22284: (follow-up) Remove HomeOrHolding from reserves Signed-off-by: Josef Moravec <josef.moravec@gmail.com> Signed-off-by: Liz Rea <wizzyrea@gmail.com> Created attachment 92859 [details] [review] Bug 22284: Add "patron's hold group" as new hold_fulfillment_policy option This patch adds "patron's hold group" as a new option to Hold pickup library match To test: 1. Set ReservesControlBranch preference to item. 2. Create a hold group 3. Go to circulation and fines rules SUCCESS => in 'Hold pickup library match' there is a new option called "patron's hold group" 4. In a library not in hold group set 'Hold policy' to "any" and 'Hold pickup library match' to "patron's hold group" 5. Search for a user in the hold group 6. 'Search to hold' for items of the library of step 4 7. Select an item and 'Place hold for [user]' SUCCESS => in 'Pickup at' you should see patron's hold group as options 8. In OPAC sign in as the same user of step 5 9. Search for the item in step 7 SUCCESS => in 'Pick up locations' you should see patron's hold group as options 10. Sign off Signed-off-by: Josef Moravec <josef.moravec@gmail.com> Signed-off-by: Liz Rea <wizzyrea@gmail.com> Created attachment 92860 [details] [review] Bug 22284: (follow-up) Squash multiple follow-ups * Bug 22284: (follow-up) Use GetReserveControlBranch in Koha::Item->pickup_locations Signed-off-by: Josef Moravec <josef.moravec@gmail.com> * Bug 22284: (follow-up) Fix tests Signed-off-by: Josef Moravec <josef.moravec@gmail.com> * Bug 22284: (follow-up) Fix typo in request.tt Signed-off-by: Josef Moravec <josef.moravec@gmail.com> * Bug 22284: (follow-up) Filter pickup on specific item click Signed-off-by: Josef Moravec <josef.moravec@gmail.com> * Bug 22284: (follow-up) Fix typos transfered -> transferred Signed-off-by: Josef Moravec <josef.moravec@gmail.com> Signed-off-by: Liz Rea <wizzyrea@gmail.com> Created attachment 92861 [details] [review] Bug 22284: (Do not push) Schema update Signed-off-by: Josef Moravec <josef.moravec@gmail.com> Signed-off-by: Liz Rea <wizzyrea@gmail.com> Patch doesn't apply, can you please rebase? Apply? [(y)es, (n)o, (i)nteractive] y Applying: Bug 22284: New ft_local_hold_group column, and 'holgroup' enum option Applying: Bug 22284: Add new checkbox in Library Groups Applying: Bug 22284: New hold group options in smart-rules Applying: Bug 22284: Control hold groups in C4::Reserves Applying: Bug 22284: New methods in Koha::Library::Groups and Koha::Library Using index info to reconstruct a base tree... M Koha/Library.pm M t/db_dependent/Koha/Libraries.t Falling back to patching base and 3-way merge... Auto-merging t/db_dependent/Koha/Libraries.t CONFLICT (content): Merge conflict in t/db_dependent/Koha/Libraries.t Auto-merging Koha/Library.pm CONFLICT (content): Merge conflict in Koha/Library.pm error: Failed to merge in the changes. Patch failed at 0001 Bug 22284: New methods in Koha::Library::Groups and Koha::Library The copy of the patch that failed is found in: .git/rebase-apply/patch When you have resolved this problem run "git bz apply --continue". If you would prefer to skip this patch, instead run "git bz apply --skip". To restore the original branch and stop patching run "git bz apply --abort". Patch left in /tmp/Bug-22284-New-methods-in-KohaLibraryGroups-and-Koh-yTz2MT.patch Created attachment 94483 [details] [review] Bug 22284: New ft_local_hold_group column, and 'holgroup' enum option In order to add groups for holds, this patch provides a new column in library_groups to identify them. Also provides new enum option for hold_fulfillment_policy column of default_branch_circ_rules, default_circ_rules, default_branch_item_rules and branch_item_rules. To test: 1) check that there is no ft_local_hold_group column in library_groups and that there are no 'holdgroup' enum option in hold_fulfillment_policy in default_branch_circ_rules, default_circ_rules, default_branch_item_rules and branch_item_rules 2) apply this patch 3) perl installer/data/mysql/updatedatabase.pl && dbic SUCCESS => !(step 1) ;-D 4) Sign off Sponsored-by: VOKAL Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: Josef Moravec <josef.moravec@gmail.com> Signed-off-by: Liz Rea <wizzyrea@gmail.com> Created attachment 94484 [details] [review] Bug 22284: Add new checkbox in Library Groups This patch adds a new checkbox option 'Is local hold group' when you create a new root group. To test: 1) In intranet go to Library Groups in admin module 2) Add new root group 3) Check that there is no 'Is local hold group' checkbox 4) Apply this patch 5) Add new root group SUCCESS => a new 'Is local hold group' checkbox appears 6) Select 'Is local hold group' and save 7) Click edit button for root group SUCCESS => 'Is local hold group' is checked 8) Sign off Sponsored-by: VOKAL Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: Josef Moravec <josef.moravec@gmail.com> Signed-off-by: Liz Rea <wizzyrea@gmail.com> Created attachment 94485 [details] [review] Bug 22284: New hold group options in smart-rules This patch adds new options for 'Default checkout, hold and return policy' and 'Default holds policy by item type', both in 'Hold policy' and 'Hold pickup library match' columns To test: 1) Go to 'Circulation and fines rules' in admin 2) Check that there are no options for hold group in 'Hold policy' and 'Hold pickup library match' columns 3) Apply this patch SUCCESS => in 'Hold policy' you should see 'From local hold group' option, and in 'Hold pickup library match' you should see "Item's hold group" 4) Select any of those options and save SUCCESS => options should be selected when page reloads 5) Sign off Sponsored-by: VOKAL Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: Josef Moravec <josef.moravec@gmail.com> Signed-off-by: Liz Rea <wizzyrea@gmail.com> Created attachment 94486 [details] [review] Bug 22284: Control hold groups in C4::Reserves This patch modifies C4::Reserves to control when hold group options where selected in smart rules. In CanItemBeReserved adds 2 new error status messages 1) branchNotInHoldGroup: when a patron's homebranch is not in item's hold group 2) pickupNotInHoldGroup: when a selected pickup location is not in item's hold group Also CheckReserves is modified when item's priority is defined, to control by hold group when required. Finally, IsAvailableForItemLevelRequest was also modified to control by hold group when required. To test: 1) Apply this patch 2) prove t/db_dependent/Holds.t SUCCESS => Result: PASS 3) Sign off Sponsored-by: VOKAL Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: Josef Moravec <josef.moravec@gmail.com> Signed-off-by: Liz Rea <wizzyrea@gmail.com> Created attachment 94487 [details] [review] Bug 22284: New methods in Koha::Library::Groups and Koha::Library This patch adds new methods in Koha::Library::Groups and Koha::Library. 1) For Koha::Library::Groups adds get_root_ancestor that returns all root groups for a given search parameters, for example Koha::Library::Groups->get_root_ancestor( { id => $group_id } ) 2) For Koha::Library adds 2.1) get_hold_libraries: returns all libraries (including self) that belongs to the same holdgroups. If $self belongs to several holdgroups it will return a distinct list of all libraries belonging to them. 2.2) validate_hold_sibling: Returns 1 if the given parameters matches any of the libraries that belong to any of the holdgroups this library belongs. For example $library->validate_hold_sibling( { branchcode => $branchcode } ) To test: 1) apply this patch 2) prove t/db_dependent/Koha/Libraries.t t/db_dependent/LibraryGroups.t SUCCESS => green letters :-D 3) Sign off Sponsored-by: VOKAL Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: Josef Moravec <josef.moravec@gmail.com> Signed-off-by: Liz Rea <wizzyrea@gmail.com> Created attachment 94488 [details] [review] Bug 22284: Opac pickup_locations This patch modifies Koha::Libraries->pickup_location and moves most of the logic to Koha::Item and Koha::Biblio in preparation for api endpoints in the future. There where 2 methods added 1) Koha::Item->pickup_locations that given a patron, returns all pickup locations of this item, considering hold fulfillment rules, and hold allowed rules. 2) Koha::Biblio->pickup_locations that given a patron, returns a distinct list of libraries returned by each of this biblio items pickup location. Koha::Libraries->pickup_location analyzes input param and calls Koha::Item->pickup_locations or Koha::Biblio->pickup_locations as needed. Also in opac-reserve.tt the way options where obtained to fill the pickup location select was modified to pass the patron as a parameter. To test: 1) opac: try to place hold on a item and check that all libraries are shown in the pickup location select. 2) intranet: in Library groups, add 2 root groups marked as local hold group and add different libraries to each. 3) opac: login as a user of a library belonging to one hold group, and search try to place a hold on an item belongin to the other hold group. 4) intranet: in Circulation and fines rules, play with 'Hold policy' and 'Hold pickup library match' rules. 5) opac: On each modification of the rules reload the page. SUCCESS => Every time you reload the page, the number of pickup locations showed in select varies. 6) prove t/db_dependent/Koha/Biblios.t t/db_dependent/Koha/Items.t SUCCESS => Result: PASS 7) Sign off Sponsored-by: VOKAL Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: Josef Moravec <josef.moravec@gmail.com> Signed-off-by: Liz Rea <wizzyrea@gmail.com> Created attachment 94489 [details] [review] Bug 22284: (follow-up) Default to circulation control branch when no hold libraries are found Signed-off-by: Josef Moravec <josef.moravec@gmail.com> Signed-off-by: Liz Rea <wizzyrea@gmail.com> Created attachment 94490 [details] [review] Bug 22284: New message, new column and filter pickup locations in reserve/request.tt This patch adds a new message to 'Hold' column in 'Place a hold on a specific item' table. The message is "Cannot place hold from patrons's library". It appears when patron's homebranch is not in item's hold group, and hold_fulfillment_policy is set to 'holdgroup'. This patch also adds a new column "Allowed pickup locations" that lists allowed pickup locations per item. Finally, the select that displays pickup locations is filtered by allowed pickup locations, when multi_hold is not enabled To test: 1) Apply this patch 2) In library groups add a root group and check it as hold group. 3) Add two libraries to the group 4) In circulation and fines rules, in 'Default checkout, hold and return policy', in Hold policy change the value to 'From local hold group' 5) Search a patron from a different library than step 3, select one and click 'search to hold' 6) Search by location for items in any library of step 3 7) On any item, clic on 'Place hold for ...' SUCCESS => when the page is loaded, in the 'Place a hold on a specific item', you should see the message "Cannot place hold from patrons's library" in 'Hold' column => You should see a new column called "Allowed pickup locations" and the message is "Any library" 8) In circulation and fines rules, in 'Default checkout, hold and return policy', in 'Hold policy' change the value again to 'From any library' and change 'Hold pickup library match' to "Item's hold group" 8) Repeat steps 5 to 7 SUCCESS => when the page is loaded, you should see the "Pickup at" select filtered by libraries in hold group => You should see in "Allowed pickup locations" a coma separated list of the libraries in item's hold group => If biblio has an item whose control branch is not in a hold group, you should see the control branch name in "Allowed pickup locations" 9) Sign off Sponsored-by: VOKAL Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: Josef Moravec <josef.moravec@gmail.com> Signed-off-by: Liz Rea <wizzyrea@gmail.com> Created attachment 94491 [details] [review] Bug 22284: Filter item's pickup locations in holds_table.inc This patch filters pickup locations per item in holds_table.inc To test: 1) Go to Circulation and fines rules and change max holds allowed to > 0, and change 'Hold pickup library match' to homebranch 2) Search for a patron 3) Click on "Search to hold" 4) Search for any bilio 5) Place hold on a specific item CHECK => Item's pickup location is a select with all libraries 6) Apply this patch 7) Repeat steps 2 to 5 SUCCESS => Item's pickup location is filtered by allowed pickup locations 8) Sign off Sponsored-by: VOKAL Signed-off-by: Josef Moravec <josef.moravec@gmail.com> Signed-off-by: Liz Rea <wizzyrea@gmail.com> Created attachment 94492 [details] [review] Bug 22284: (follow-up) Squash multiple follow-ups * Bug 22284: (follow-up) Remove commented warn and address test failures Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: Josef Moravec <josef.moravec@gmail.com> * Bug 22284: (follow-up) fix test count after merge Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: Josef Moravec <josef.moravec@gmail.com> * Bug 22284: (follow-up) fixes after 15496 Signed-off-by: Josef Moravec <josef.moravec@gmail.com> * Bug 22284: (follow-up) fixes after 18936 Signed-off-by: Josef Moravec <josef.moravec@gmail.com> * Bug 22284: (follow-up) Remove HomeOrHolding from reserves Signed-off-by: Josef Moravec <josef.moravec@gmail.com> Signed-off-by: Liz Rea <wizzyrea@gmail.com> Created attachment 94493 [details] [review] Bug 22284: Add "patron's hold group" as new hold_fulfillment_policy option This patch adds "patron's hold group" as a new option to Hold pickup library match To test: 1. Set ReservesControlBranch preference to item. 2. Create a hold group 3. Go to circulation and fines rules SUCCESS => in 'Hold pickup library match' there is a new option called "patron's hold group" 4. In a library not in hold group set 'Hold policy' to "any" and 'Hold pickup library match' to "patron's hold group" 5. Search for a user in the hold group 6. 'Search to hold' for items of the library of step 4 7. Select an item and 'Place hold for [user]' SUCCESS => in 'Pickup at' you should see patron's hold group as options 8. In OPAC sign in as the same user of step 5 9. Search for the item in step 7 SUCCESS => in 'Pick up locations' you should see patron's hold group as options 10. Sign off Signed-off-by: Josef Moravec <josef.moravec@gmail.com> Signed-off-by: Liz Rea <wizzyrea@gmail.com> Created attachment 94494 [details] [review] Bug 22284: (follow-up) Squash multiple follow-ups * Bug 22284: (follow-up) Use GetReserveControlBranch in Koha::Item->pickup_locations Signed-off-by: Josef Moravec <josef.moravec@gmail.com> * Bug 22284: (follow-up) Fix tests Signed-off-by: Josef Moravec <josef.moravec@gmail.com> * Bug 22284: (follow-up) Fix typo in request.tt Signed-off-by: Josef Moravec <josef.moravec@gmail.com> * Bug 22284: (follow-up) Filter pickup on specific item click Signed-off-by: Josef Moravec <josef.moravec@gmail.com> * Bug 22284: (follow-up) Fix typos transfered -> transferred Signed-off-by: Josef Moravec <josef.moravec@gmail.com> Signed-off-by: Liz Rea <wizzyrea@gmail.com> Created attachment 94495 [details] [review] Bug 22284: (Do not push) Schema update Signed-off-by: Josef Moravec <josef.moravec@gmail.com> Signed-off-by: Liz Rea <wizzyrea@gmail.com> (In reply to Katrin Fischer from comment #243) > Patch doesn't apply, can you please rebase? > > > Apply? [(y)es, (n)o, (i)nteractive] y > Applying: Bug 22284: New ft_local_hold_group column, and 'holgroup' enum > option > Applying: Bug 22284: Add new checkbox in Library Groups > Applying: Bug 22284: New hold group options in smart-rules > Applying: Bug 22284: Control hold groups in C4::Reserves > Applying: Bug 22284: New methods in Koha::Library::Groups and Koha::Library > Using index info to reconstruct a base tree... > M Koha/Library.pm > M t/db_dependent/Koha/Libraries.t > Falling back to patching base and 3-way merge... > Auto-merging t/db_dependent/Koha/Libraries.t > CONFLICT (content): Merge conflict in t/db_dependent/Koha/Libraries.t > Auto-merging Koha/Library.pm > CONFLICT (content): Merge conflict in Koha/Library.pm > error: Failed to merge in the changes. > Patch failed at 0001 Bug 22284: New methods in Koha::Library::Groups and > Koha::Library > The copy of the patch that failed is found in: .git/rebase-apply/patch > When you have resolved this problem run "git bz apply --continue". > If you would prefer to skip this patch, instead run "git bz apply --skip". > To restore the original branch and stop patching run "git bz apply --abort". > Patch left in > /tmp/Bug-22284-New-methods-in-KohaLibraryGroups-and-Koh-yTz2MT.patch Patches rebased! Created attachment 96437 [details] [review] Bug 22284: New ft_local_hold_group column, and 'holgroup' enum option In order to add groups for holds, this patch provides a new column in library_groups to identify them. Also provides new enum option for hold_fulfillment_policy column of default_branch_circ_rules, default_circ_rules, default_branch_item_rules and branch_item_rules. To test: 1) check that there is no ft_local_hold_group column in library_groups and that there are no 'holdgroup' enum option in hold_fulfillment_policy in default_branch_circ_rules, default_circ_rules, default_branch_item_rules and branch_item_rules 2) apply this patch 3) perl installer/data/mysql/updatedatabase.pl && dbic SUCCESS => !(step 1) ;-D 4) Sign off Sponsored-by: VOKAL Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: Josef Moravec <josef.moravec@gmail.com> Signed-off-by: Liz Rea <wizzyrea@gmail.com> Created attachment 96438 [details] [review] Bug 22284: Add new checkbox in Library Groups This patch adds a new checkbox option 'Is local hold group' when you create a new root group. To test: 1) In intranet go to Library Groups in admin module 2) Add new root group 3) Check that there is no 'Is local hold group' checkbox 4) Apply this patch 5) Add new root group SUCCESS => a new 'Is local hold group' checkbox appears 6) Select 'Is local hold group' and save 7) Click edit button for root group SUCCESS => 'Is local hold group' is checked 8) Sign off Sponsored-by: VOKAL Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: Josef Moravec <josef.moravec@gmail.com> Signed-off-by: Liz Rea <wizzyrea@gmail.com> Created attachment 96439 [details] [review] Bug 22284: New hold group options in smart-rules This patch adds new options for 'Default checkout, hold and return policy' and 'Default holds policy by item type', both in 'Hold policy' and 'Hold pickup library match' columns To test: 1) Go to 'Circulation and fines rules' in admin 2) Check that there are no options for hold group in 'Hold policy' and 'Hold pickup library match' columns 3) Apply this patch SUCCESS => in 'Hold policy' you should see 'From local hold group' option, and in 'Hold pickup library match' you should see "Item's hold group" 4) Select any of those options and save SUCCESS => options should be selected when page reloads 5) Sign off Sponsored-by: VOKAL Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: Josef Moravec <josef.moravec@gmail.com> Signed-off-by: Liz Rea <wizzyrea@gmail.com> Created attachment 96440 [details] [review] Bug 22284: Control hold groups in C4::Reserves This patch modifies C4::Reserves to control when hold group options where selected in smart rules. In CanItemBeReserved adds 2 new error status messages 1) branchNotInHoldGroup: when a patron's homebranch is not in item's hold group 2) pickupNotInHoldGroup: when a selected pickup location is not in item's hold group Also CheckReserves is modified when item's priority is defined, to control by hold group when required. Finally, IsAvailableForItemLevelRequest was also modified to control by hold group when required. To test: 1) Apply this patch 2) prove t/db_dependent/Holds.t SUCCESS => Result: PASS 3) Sign off Sponsored-by: VOKAL Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: Josef Moravec <josef.moravec@gmail.com> Signed-off-by: Liz Rea <wizzyrea@gmail.com> Created attachment 96441 [details] [review] Bug 22284: New methods in Koha::Library::Groups and Koha::Library This patch adds new methods in Koha::Library::Groups and Koha::Library. 1) For Koha::Library::Groups adds get_root_ancestor that returns all root groups for a given search parameters, for example Koha::Library::Groups->get_root_ancestor( { id => $group_id } ) 2) For Koha::Library adds 2.1) get_hold_libraries: returns all libraries (including self) that belongs to the same holdgroups. If $self belongs to several holdgroups it will return a distinct list of all libraries belonging to them. 2.2) validate_hold_sibling: Returns 1 if the given parameters matches any of the libraries that belong to any of the holdgroups this library belongs. For example $library->validate_hold_sibling( { branchcode => $branchcode } ) To test: 1) apply this patch 2) prove t/db_dependent/Koha/Libraries.t t/db_dependent/LibraryGroups.t SUCCESS => green letters :-D 3) Sign off Sponsored-by: VOKAL Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: Josef Moravec <josef.moravec@gmail.com> Signed-off-by: Liz Rea <wizzyrea@gmail.com> Created attachment 96442 [details] [review] Bug 22284: Opac pickup_locations This patch modifies Koha::Libraries->pickup_location and moves most of the logic to Koha::Item and Koha::Biblio in preparation for api endpoints in the future. There where 2 methods added 1) Koha::Item->pickup_locations that given a patron, returns all pickup locations of this item, considering hold fulfillment rules, and hold allowed rules. 2) Koha::Biblio->pickup_locations that given a patron, returns a distinct list of libraries returned by each of this biblio items pickup location. Koha::Libraries->pickup_location analyzes input param and calls Koha::Item->pickup_locations or Koha::Biblio->pickup_locations as needed. Also in opac-reserve.tt the way options where obtained to fill the pickup location select was modified to pass the patron as a parameter. To test: 1) opac: try to place hold on a item and check that all libraries are shown in the pickup location select. 2) intranet: in Library groups, add 2 root groups marked as local hold group and add different libraries to each. 3) opac: login as a user of a library belonging to one hold group, and search try to place a hold on an item belongin to the other hold group. 4) intranet: in Circulation and fines rules, play with 'Hold policy' and 'Hold pickup library match' rules. 5) opac: On each modification of the rules reload the page. SUCCESS => Every time you reload the page, the number of pickup locations showed in select varies. 6) prove t/db_dependent/Koha/Biblios.t t/db_dependent/Koha/Items.t SUCCESS => Result: PASS 7) Sign off Sponsored-by: VOKAL Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: Josef Moravec <josef.moravec@gmail.com> Signed-off-by: Liz Rea <wizzyrea@gmail.com> Created attachment 96443 [details] [review] Bug 22284: (follow-up) Default to circulation control branch when no hold libraries are found Signed-off-by: Josef Moravec <josef.moravec@gmail.com> Signed-off-by: Liz Rea <wizzyrea@gmail.com> Created attachment 96444 [details] [review] Bug 22284: New message, new column and filter pickup locations in reserve/request.tt This patch adds a new message to 'Hold' column in 'Place a hold on a specific item' table. The message is "Cannot place hold from patrons's library". It appears when patron's homebranch is not in item's hold group, and hold_fulfillment_policy is set to 'holdgroup'. This patch also adds a new column "Allowed pickup locations" that lists allowed pickup locations per item. Finally, the select that displays pickup locations is filtered by allowed pickup locations, when multi_hold is not enabled To test: 1) Apply this patch 2) In library groups add a root group and check it as hold group. 3) Add two libraries to the group 4) In circulation and fines rules, in 'Default checkout, hold and return policy', in Hold policy change the value to 'From local hold group' 5) Search a patron from a different library than step 3, select one and click 'search to hold' 6) Search by location for items in any library of step 3 7) On any item, clic on 'Place hold for ...' SUCCESS => when the page is loaded, in the 'Place a hold on a specific item', you should see the message "Cannot place hold from patrons's library" in 'Hold' column => You should see a new column called "Allowed pickup locations" and the message is "Any library" 8) In circulation and fines rules, in 'Default checkout, hold and return policy', in 'Hold policy' change the value again to 'From any library' and change 'Hold pickup library match' to "Item's hold group" 8) Repeat steps 5 to 7 SUCCESS => when the page is loaded, you should see the "Pickup at" select filtered by libraries in hold group => You should see in "Allowed pickup locations" a coma separated list of the libraries in item's hold group => If biblio has an item whose control branch is not in a hold group, you should see the control branch name in "Allowed pickup locations" 9) Sign off Sponsored-by: VOKAL Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: Josef Moravec <josef.moravec@gmail.com> Signed-off-by: Liz Rea <wizzyrea@gmail.com> Created attachment 96445 [details] [review] Bug 22284: Filter item's pickup locations in holds_table.inc This patch filters pickup locations per item in holds_table.inc To test: 1) Go to Circulation and fines rules and change max holds allowed to > 0, and change 'Hold pickup library match' to homebranch 2) Search for a patron 3) Click on "Search to hold" 4) Search for any bilio 5) Place hold on a specific item CHECK => Item's pickup location is a select with all libraries 6) Apply this patch 7) Repeat steps 2 to 5 SUCCESS => Item's pickup location is filtered by allowed pickup locations 8) Sign off Sponsored-by: VOKAL Signed-off-by: Josef Moravec <josef.moravec@gmail.com> Signed-off-by: Liz Rea <wizzyrea@gmail.com> Created attachment 96446 [details] [review] Bug 22284: (follow-up) Squash multiple follow-ups * Bug 22284: (follow-up) Remove commented warn and address test failures Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: Josef Moravec <josef.moravec@gmail.com> * Bug 22284: (follow-up) fix test count after merge Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: Josef Moravec <josef.moravec@gmail.com> * Bug 22284: (follow-up) fixes after 15496 Signed-off-by: Josef Moravec <josef.moravec@gmail.com> * Bug 22284: (follow-up) fixes after 18936 Signed-off-by: Josef Moravec <josef.moravec@gmail.com> * Bug 22284: (follow-up) Remove HomeOrHolding from reserves Signed-off-by: Josef Moravec <josef.moravec@gmail.com> Signed-off-by: Liz Rea <wizzyrea@gmail.com> Created attachment 96447 [details] [review] Bug 22284: Add "patron's hold group" as new hold_fulfillment_policy option This patch adds "patron's hold group" as a new option to Hold pickup library match To test: 1. Set ReservesControlBranch preference to item. 2. Create a hold group 3. Go to circulation and fines rules SUCCESS => in 'Hold pickup library match' there is a new option called "patron's hold group" 4. In a library not in hold group set 'Hold policy' to "any" and 'Hold pickup library match' to "patron's hold group" 5. Search for a user in the hold group 6. 'Search to hold' for items of the library of step 4 7. Select an item and 'Place hold for [user]' SUCCESS => in 'Pickup at' you should see patron's hold group as options 8. In OPAC sign in as the same user of step 5 9. Search for the item in step 7 SUCCESS => in 'Pick up locations' you should see patron's hold group as options 10. Sign off Signed-off-by: Josef Moravec <josef.moravec@gmail.com> Signed-off-by: Liz Rea <wizzyrea@gmail.com> Created attachment 96448 [details] [review] Bug 22284: (follow-up) Squash multiple follow-ups * Bug 22284: (follow-up) Use GetReserveControlBranch in Koha::Item->pickup_locations Signed-off-by: Josef Moravec <josef.moravec@gmail.com> * Bug 22284: (follow-up) Fix tests Signed-off-by: Josef Moravec <josef.moravec@gmail.com> * Bug 22284: (follow-up) Fix typo in request.tt Signed-off-by: Josef Moravec <josef.moravec@gmail.com> * Bug 22284: (follow-up) Filter pickup on specific item click Signed-off-by: Josef Moravec <josef.moravec@gmail.com> * Bug 22284: (follow-up) Fix typos transfered -> transferred Signed-off-by: Josef Moravec <josef.moravec@gmail.com> Signed-off-by: Liz Rea <wizzyrea@gmail.com> Created attachment 96449 [details] [review] Bug 22284: (Do not push) Schema update Signed-off-by: Josef Moravec <josef.moravec@gmail.com> Signed-off-by: Liz Rea <wizzyrea@gmail.com> Created attachment 96450 [details] [review] Bug 22284: (follow-up) Anotate boolean values in Koha::Schema::Result::LibraryGroup Created attachment 96544 [details] [review] Bug 22284: New ft_local_hold_group column, and 'holgroup' enum option In order to add groups for holds, this patch provides a new column in library_groups to identify them. Also provides new enum option for hold_fulfillment_policy column of default_branch_circ_rules, default_circ_rules, default_branch_item_rules and branch_item_rules. To test: 1) check that there is no ft_local_hold_group column in library_groups and that there are no 'holdgroup' enum option in hold_fulfillment_policy in default_branch_circ_rules, default_circ_rules, default_branch_item_rules and branch_item_rules 2) apply this patch 3) perl installer/data/mysql/updatedatabase.pl && dbic SUCCESS => !(step 1) ;-D 4) Sign off Sponsored-by: VOKAL Signed-off-by: Josef Moravec <josef.moravec@gmail.com> Signed-off-by: Liz Rea <wizzyrea@gmail.com> Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Created attachment 96545 [details] [review] Bug 22284: Add new checkbox in Library Groups This patch adds a new checkbox option 'Is local hold group' when you create a new root group. To test: 1) In intranet go to Library Groups in admin module 2) Add new root group 3) Check that there is no 'Is local hold group' checkbox 4) Apply this patch 5) Add new root group SUCCESS => a new 'Is local hold group' checkbox appears 6) Select 'Is local hold group' and save 7) Click edit button for root group SUCCESS => 'Is local hold group' is checked 8) Sign off Sponsored-by: VOKAL Signed-off-by: Josef Moravec <josef.moravec@gmail.com> Signed-off-by: Liz Rea <wizzyrea@gmail.com> Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Created attachment 96546 [details] [review] Bug 22284: New hold group options in smart-rules This patch adds new options for 'Default checkout, hold and return policy' and 'Default holds policy by item type', both in 'Hold policy' and 'Hold pickup library match' columns To test: 1) Go to 'Circulation and fines rules' in admin 2) Check that there are no options for hold group in 'Hold policy' and 'Hold pickup library match' columns 3) Apply this patch SUCCESS => in 'Hold policy' you should see 'From local hold group' option, and in 'Hold pickup library match' you should see "Item's hold group" 4) Select any of those options and save SUCCESS => options should be selected when page reloads 5) Sign off Sponsored-by: VOKAL Signed-off-by: Josef Moravec <josef.moravec@gmail.com> Signed-off-by: Liz Rea <wizzyrea@gmail.com> Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Created attachment 96547 [details] [review] Bug 22284: Control hold groups in C4::Reserves This patch modifies C4::Reserves to control when hold group options where selected in smart rules. In CanItemBeReserved adds 2 new error status messages 1) branchNotInHoldGroup: when a patron's homebranch is not in item's hold group 2) pickupNotInHoldGroup: when a selected pickup location is not in item's hold group Also CheckReserves is modified when item's priority is defined, to control by hold group when required. Finally, IsAvailableForItemLevelRequest was also modified to control by hold group when required. To test: 1) Apply this patch 2) prove t/db_dependent/Holds.t SUCCESS => Result: PASS 3) Sign off Sponsored-by: VOKAL Signed-off-by: Josef Moravec <josef.moravec@gmail.com> Signed-off-by: Liz Rea <wizzyrea@gmail.com> Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Created attachment 96548 [details] [review] Bug 22284: New methods in Koha::Library::Groups and Koha::Library This patch adds new methods in Koha::Library::Groups and Koha::Library. 1) For Koha::Library::Groups adds get_root_ancestor that returns all root groups for a given search parameters, for example Koha::Library::Groups->get_root_ancestor( { id => $group_id } ) 2) For Koha::Library adds 2.1) get_hold_libraries: returns all libraries (including self) that belongs to the same holdgroups. If $self belongs to several holdgroups it will return a distinct list of all libraries belonging to them. 2.2) validate_hold_sibling: Returns 1 if the given parameters matches any of the libraries that belong to any of the holdgroups this library belongs. For example $library->validate_hold_sibling( { branchcode => $branchcode } ) To test: 1) apply this patch 2) prove t/db_dependent/Koha/Libraries.t t/db_dependent/LibraryGroups.t SUCCESS => green letters :-D 3) Sign off Sponsored-by: VOKAL Signed-off-by: Josef Moravec <josef.moravec@gmail.com> Signed-off-by: Liz Rea <wizzyrea@gmail.com> Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Created attachment 96549 [details] [review] Bug 22284: Opac pickup_locations This patch modifies Koha::Libraries->pickup_location and moves most of the logic to Koha::Item and Koha::Biblio in preparation for api endpoints in the future. There where 2 methods added 1) Koha::Item->pickup_locations that given a patron, returns all pickup locations of this item, considering hold fulfillment rules, and hold allowed rules. 2) Koha::Biblio->pickup_locations that given a patron, returns a distinct list of libraries returned by each of this biblio items pickup location. Koha::Libraries->pickup_location analyzes input param and calls Koha::Item->pickup_locations or Koha::Biblio->pickup_locations as needed. Also in opac-reserve.tt the way options where obtained to fill the pickup location select was modified to pass the patron as a parameter. To test: 1) opac: try to place hold on a item and check that all libraries are shown in the pickup location select. 2) intranet: in Library groups, add 2 root groups marked as local hold group and add different libraries to each. 3) opac: login as a user of a library belonging to one hold group, and search try to place a hold on an item belongin to the other hold group. 4) intranet: in Circulation and fines rules, play with 'Hold policy' and 'Hold pickup library match' rules. 5) opac: On each modification of the rules reload the page. SUCCESS => Every time you reload the page, the number of pickup locations showed in select varies. 6) prove t/db_dependent/Koha/Biblios.t t/db_dependent/Koha/Items.t SUCCESS => Result: PASS 7) Sign off Sponsored-by: VOKAL Signed-off-by: Josef Moravec <josef.moravec@gmail.com> Signed-off-by: Liz Rea <wizzyrea@gmail.com> Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Created attachment 96550 [details] [review] Bug 22284: (follow-up) Default to circulation control branch when no hold libraries are found Signed-off-by: Josef Moravec <josef.moravec@gmail.com> Signed-off-by: Liz Rea <wizzyrea@gmail.com> Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Created attachment 96551 [details] [review] Bug 22284: New message, new column and filter pickup locations in reserve/request.tt This patch adds a new message to 'Hold' column in 'Place a hold on a specific item' table. The message is "Cannot place hold from patrons's library". It appears when patron's homebranch is not in item's hold group, and hold_fulfillment_policy is set to 'holdgroup'. This patch also adds a new column "Allowed pickup locations" that lists allowed pickup locations per item. Finally, the select that displays pickup locations is filtered by allowed pickup locations, when multi_hold is not enabled To test: 1) Apply this patch 2) In library groups add a root group and check it as hold group. 3) Add two libraries to the group 4) In circulation and fines rules, in 'Default checkout, hold and return policy', in Hold policy change the value to 'From local hold group' 5) Search a patron from a different library than step 3, select one and click 'search to hold' 6) Search by location for items in any library of step 3 7) On any item, clic on 'Place hold for ...' SUCCESS => when the page is loaded, in the 'Place a hold on a specific item', you should see the message "Cannot place hold from patrons's library" in 'Hold' column => You should see a new column called "Allowed pickup locations" and the message is "Any library" 8) In circulation and fines rules, in 'Default checkout, hold and return policy', in 'Hold policy' change the value again to 'From any library' and change 'Hold pickup library match' to "Item's hold group" 8) Repeat steps 5 to 7 SUCCESS => when the page is loaded, you should see the "Pickup at" select filtered by libraries in hold group => You should see in "Allowed pickup locations" a coma separated list of the libraries in item's hold group => If biblio has an item whose control branch is not in a hold group, you should see the control branch name in "Allowed pickup locations" 9) Sign off Sponsored-by: VOKAL Signed-off-by: Josef Moravec <josef.moravec@gmail.com> Signed-off-by: Liz Rea <wizzyrea@gmail.com> Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Created attachment 96552 [details] [review] Bug 22284: Filter item's pickup locations in holds_table.inc This patch filters pickup locations per item in holds_table.inc To test: 1) Go to Circulation and fines rules and change max holds allowed to > 0, and change 'Hold pickup library match' to homebranch 2) Search for a patron 3) Click on "Search to hold" 4) Search for any bilio 5) Place hold on a specific item CHECK => Item's pickup location is a select with all libraries 6) Apply this patch 7) Repeat steps 2 to 5 SUCCESS => Item's pickup location is filtered by allowed pickup locations 8) Sign off Sponsored-by: VOKAL Signed-off-by: Josef Moravec <josef.moravec@gmail.com> Signed-off-by: Liz Rea <wizzyrea@gmail.com> Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Created attachment 96553 [details] [review] Bug 22284: (follow-up) Squash multiple follow-ups * Bug 22284: (follow-up) Remove commented warn and address test failures * Bug 22284: (follow-up) fix test count after merge * Bug 22284: (follow-up) fixes after 15496 * Bug 22284: (follow-up) fixes after 18936 * Bug 22284: (follow-up) Remove HomeOrHolding from reserves Signed-off-by: Liz Rea <wizzyrea@gmail.com> Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Created attachment 96554 [details] [review] Bug 22284: Add "patron's hold group" as new hold_fulfillment_policy option This patch adds "patron's hold group" as a new option to Hold pickup library match To test: 1. Set ReservesControlBranch preference to item. 2. Create a hold group 3. Go to circulation and fines rules SUCCESS => in 'Hold pickup library match' there is a new option called "patron's hold group" 4. In a library not in hold group set 'Hold policy' to "any" and 'Hold pickup library match' to "patron's hold group" 5. Search for a user in the hold group 6. 'Search to hold' for items of the library of step 4 7. Select an item and 'Place hold for [user]' SUCCESS => in 'Pickup at' you should see patron's hold group as options 8. In OPAC sign in as the same user of step 5 9. Search for the item in step 7 SUCCESS => in 'Pick up locations' you should see patron's hold group as options 10. Sign off Signed-off-by: Josef Moravec <josef.moravec@gmail.com> Signed-off-by: Liz Rea <wizzyrea@gmail.com> Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Created attachment 96555 [details] [review] Bug 22284: (follow-up) Squash multiple follow-ups * Bug 22284: (follow-up) Use GetReserveControlBranch in Koha::Item->pickup_locations * Bug 22284: (follow-up) Fix tests * Bug 22284: (follow-up) Fix typo in request.tt * Bug 22284: (follow-up) Filter pickup on specific item click * Bug 22284: (follow-up) Fix typos transfered -> transferred Signed-off-by: Josef Moravec <josef.moravec@gmail.com> Signed-off-by: Liz Rea <wizzyrea@gmail.com> Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Created attachment 96556 [details] [review] Bug 22284: Schema update Signed-off-by: Josef Moravec <josef.moravec@gmail.com> Signed-off-by: Liz Rea <wizzyrea@gmail.com> Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Created attachment 96557 [details] [review] Bug 22284: (follow-up) Anotate boolean values in Koha::Schema::Result::LibraryGroup Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Created attachment 96558 [details] [review] Bug 22284: (QA follow-up) Make pickup locations be Koha::Library objects This patch makes the following methods return array references of Koha::Library objects instead or unblessed objects; - Koha::Item->pickup_locations - Koha::Biblio->pickup_locations - Koha::Libraries->pickup_locations Bonus: - The template plugin is adjusted to unbless things to keep behavior - Tests are moved to the right .t file. - Tests for the new behavior are added. Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Nice work everyone! Pushed to master for 20.05 # Failed test 'No tests run for subtest "pickup_locations() tests"' # at t/db_dependent/Template/Plugin/Branches.t line 151. Can't call method "unblessed" on unblessed reference at /kohadevbox/koha/Koha/Template/Plugin/Branches.pm line 93. # Looks like your test exited with 255 just after 3. There is a (random?) failure on Master_D9 run 1053: koha_1 | # Failed test 'ReservesControlBranch: ItemHomeLibrary, biblio1, patron1 should return 6 but returns 7' koha_1 | # at t/db_dependent/Koha/Biblio.t line 419. koha_1 | koha_1 | # Failed test 'ReservesControlBranch: PatronLibrary, biblio1, patron1 should return 6 but returns 7' koha_1 | # at t/db_dependent/Koha/Biblio.t line 419. koha_1 | # Looks like you planned 29 tests but ran 31. koha_1 | # Looks like you failed 2 tests of 31 run. koha_1 | koha_1 | # Failed test 'pickup_locations' koha_1 | # at t/db_dependent/Koha/Biblio.t line 436. Another one (Koha_Master_D9 run 1061): koha_1 | # Failed test 'item1, patron1, holdallowed: homebranch, hold_fulfillment_policy: any should return 3 but returns 4' koha_1 | # at t/db_dependent/Koha/Item.t line 308. koha_1 | koha_1 | # Failed test 'item1, patron1, holdallowed: any, hold_fulfillment_policy: any should return 3 but returns 4' koha_1 | # at t/db_dependent/Koha/Item.t line 308. koha_1 | koha_1 | # Failed test 'item1, patron1, holdallowed: holdgroup, hold_fulfillment_policy: any should return 3 but returns 4' koha_1 | # at t/db_dependent/Koha/Item.t line 308. koha_1 | koha_1 | # Failed test 'item1, patron4, holdallowed: any, hold_fulfillment_policy: any should return 3 but returns 4' koha_1 | # at t/db_dependent/Koha/Item.t line 308. koha_1 | koha_1 | # Failed test 'item3, patron1, holdallowed: any, hold_fulfillment_policy: any should return 3 but returns 4' koha_1 | # at t/db_dependent/Koha/Item.t line 308. koha_1 | koha_1 | # Failed test 'item3, patron4, holdallowed: any, hold_fulfillment_policy: any should return 3 but returns 4' koha_1 | # at t/db_dependent/Koha/Item.t line 308. koha_1 | koha_1 | # Failed test 'item3, patron4, holdallowed: holdgroup, hold_fulfillment_policy: any should return 3 but returns 4' koha_1 | # at t/db_dependent/Koha/Item.t line 308. koha_1 | # Looks like you planned 114 tests but ran 121. koha_1 | # Looks like you failed 7 tests of 121 run. koha_1 | koha_1 | # Failed test 'pickup_locations' koha_1 | # at t/db_dependent/Koha/Item.t line 332. koha_1 | # Looks like you failed 1 test of 4. koha_1 | [12:19:06] t/db_dependent/Koha/Item.t .............................................. Not backported to 19.11.x We still have failing tests related to these patches: t/db_dependent/Koha/Libraries.t .. 1/9 # Looks like you planned 11 tests but ran 2. # Failed test 'Given BranchTransferLimitsType = itemtype and item-level_itypes = ON' # at t/db_dependent/Koha/Libraries.t line 258. # Looks like you planned 4 tests but ran 2. # Looks like you failed 1 test of 2 run. # Failed test 'UseBranchTransferLimits = ON' # at t/db_dependent/Koha/Libraries.t line 422. # Looks like you failed 1 test of 2. t/db_dependent/Koha/Libraries.t .. 6/9 # Failed test 'pickup_locations' # at t/db_dependent/Koha/Libraries.t line 423. Can't call method "branchcode" on unblessed reference at t/db_dependent/Koha/Libraries.t line 195. (In reply to Jonathan Druart from comment #292) > We still have failing tests related to these patches: > > t/db_dependent/Koha/Libraries.t .. 1/9 # Looks like you planned > 11 tests but ran 2. > > # Failed test 'Given BranchTransferLimitsType = itemtype and > item-level_itypes = ON' > # at t/db_dependent/Koha/Libraries.t line 258. > # Looks like you planned 4 tests but ran 2. > # Looks like you failed 1 test of 2 run. > > # Failed test 'UseBranchTransferLimits = ON' > # at t/db_dependent/Koha/Libraries.t line 422. > # Looks like you failed 1 test of 2. > t/db_dependent/Koha/Libraries.t .. 6/9 > # Failed test 'pickup_locations' > # at t/db_dependent/Koha/Libraries.t line 423. > Can't call method "branchcode" on unblessed reference at > t/db_dependent/Koha/Libraries.t line 195. I'll be working on it this morning (In reply to Jonathan Druart from comment #290) > Another one (Koha_Master_D9 run 1061): > > koha_1 | # Failed test 'item1, patron1, holdallowed: homebranch, > hold_fulfillment_policy: any should return 3 but returns 4' > koha_1 | # at t/db_dependent/Koha/Item.t line 308. > koha_1 | > koha_1 | # Failed test 'item1, patron1, holdallowed: any, > hold_fulfillment_policy: any should return 3 but returns 4' > koha_1 | # at t/db_dependent/Koha/Item.t line 308. > koha_1 | > koha_1 | # Failed test 'item1, patron1, holdallowed: holdgroup, > hold_fulfillment_policy: any should return 3 but returns 4' > koha_1 | # at t/db_dependent/Koha/Item.t line 308. > koha_1 | > koha_1 | # Failed test 'item1, patron4, holdallowed: any, > hold_fulfillment_policy: any should return 3 but returns 4' > koha_1 | # at t/db_dependent/Koha/Item.t line 308. > koha_1 | > koha_1 | # Failed test 'item3, patron1, holdallowed: any, > hold_fulfillment_policy: any should return 3 but returns 4' > koha_1 | # at t/db_dependent/Koha/Item.t line 308. > koha_1 | > koha_1 | # Failed test 'item3, patron4, holdallowed: any, > hold_fulfillment_policy: any should return 3 but returns 4' > koha_1 | # at t/db_dependent/Koha/Item.t line 308. > koha_1 | > koha_1 | # Failed test 'item3, patron4, holdallowed: holdgroup, > hold_fulfillment_policy: any should return 3 but returns 4' > koha_1 | # at t/db_dependent/Koha/Item.t line 308. > koha_1 | # Looks like you planned 114 tests but ran 121. > koha_1 | # Looks like you failed 7 tests of 121 run. > koha_1 | > koha_1 | # Failed test 'pickup_locations' > koha_1 | # at t/db_dependent/Koha/Item.t line 332. > koha_1 | # Looks like you failed 1 test of 4. > koha_1 | [12:19:06] t/db_dependent/Koha/Item.t > .............................................. These ones are still failing. Agustín, could you take a look at them? (In reply to Jonathan Druart from comment #294) > (In reply to Jonathan Druart from comment #290) > > Another one (Koha_Master_D9 run 1061): > > > > koha_1 | # Failed test 'item1, patron1, holdallowed: homebranch, > > hold_fulfillment_policy: any should return 3 but returns 4' > > koha_1 | # at t/db_dependent/Koha/Item.t line 308. > > koha_1 | > > koha_1 | # Failed test 'item1, patron1, holdallowed: any, > > hold_fulfillment_policy: any should return 3 but returns 4' > > koha_1 | # at t/db_dependent/Koha/Item.t line 308. > > koha_1 | > > koha_1 | # Failed test 'item1, patron1, holdallowed: holdgroup, > > hold_fulfillment_policy: any should return 3 but returns 4' > > koha_1 | # at t/db_dependent/Koha/Item.t line 308. > > koha_1 | > > koha_1 | # Failed test 'item1, patron4, holdallowed: any, > > hold_fulfillment_policy: any should return 3 but returns 4' > > koha_1 | # at t/db_dependent/Koha/Item.t line 308. > > koha_1 | > > koha_1 | # Failed test 'item3, patron1, holdallowed: any, > > hold_fulfillment_policy: any should return 3 but returns 4' > > koha_1 | # at t/db_dependent/Koha/Item.t line 308. > > koha_1 | > > koha_1 | # Failed test 'item3, patron4, holdallowed: any, > > hold_fulfillment_policy: any should return 3 but returns 4' > > koha_1 | # at t/db_dependent/Koha/Item.t line 308. > > koha_1 | > > koha_1 | # Failed test 'item3, patron4, holdallowed: holdgroup, > > hold_fulfillment_policy: any should return 3 but returns 4' > > koha_1 | # at t/db_dependent/Koha/Item.t line 308. > > koha_1 | # Looks like you planned 114 tests but ran 121. > > koha_1 | # Looks like you failed 7 tests of 121 run. > > koha_1 | > > koha_1 | # Failed test 'pickup_locations' > > koha_1 | # at t/db_dependent/Koha/Item.t line 332. > > koha_1 | # Looks like you failed 1 test of 4. > > koha_1 | [12:19:06] t/db_dependent/Koha/Item.t > > .............................................. > > These ones are still failing. > Agustín, could you take a look at them? ASAP (In reply to Agustín Moyano from comment #295) > (In reply to Jonathan Druart from comment #294) > > (In reply to Jonathan Druart from comment #290) > > > Another one (Koha_Master_D9 run 1061): > > > > > > koha_1 | # Failed test 'item1, patron1, holdallowed: homebranch, > > > hold_fulfillment_policy: any should return 3 but returns 4' > > > koha_1 | # at t/db_dependent/Koha/Item.t line 308. > > > koha_1 | > > > koha_1 | # Failed test 'item1, patron1, holdallowed: any, > > > hold_fulfillment_policy: any should return 3 but returns 4' > > > koha_1 | # at t/db_dependent/Koha/Item.t line 308. > > > koha_1 | > > > koha_1 | # Failed test 'item1, patron1, holdallowed: holdgroup, > > > hold_fulfillment_policy: any should return 3 but returns 4' > > > koha_1 | # at t/db_dependent/Koha/Item.t line 308. > > > koha_1 | > > > koha_1 | # Failed test 'item1, patron4, holdallowed: any, > > > hold_fulfillment_policy: any should return 3 but returns 4' > > > koha_1 | # at t/db_dependent/Koha/Item.t line 308. > > > koha_1 | > > > koha_1 | # Failed test 'item3, patron1, holdallowed: any, > > > hold_fulfillment_policy: any should return 3 but returns 4' > > > koha_1 | # at t/db_dependent/Koha/Item.t line 308. > > > koha_1 | > > > koha_1 | # Failed test 'item3, patron4, holdallowed: any, > > > hold_fulfillment_policy: any should return 3 but returns 4' > > > koha_1 | # at t/db_dependent/Koha/Item.t line 308. > > > koha_1 | > > > koha_1 | # Failed test 'item3, patron4, holdallowed: holdgroup, > > > hold_fulfillment_policy: any should return 3 but returns 4' > > > koha_1 | # at t/db_dependent/Koha/Item.t line 308. > > > koha_1 | # Looks like you planned 114 tests but ran 121. > > > koha_1 | # Looks like you failed 7 tests of 121 run. > > > koha_1 | > > > koha_1 | # Failed test 'pickup_locations' > > > koha_1 | # at t/db_dependent/Koha/Item.t line 332. > > > koha_1 | # Looks like you failed 1 test of 4. > > > koha_1 | [12:19:06] t/db_dependent/Koha/Item.t > > > .............................................. > > > > These ones are still failing. > > Agustín, could you take a look at them? > > ASAP Hi Jonathan, I cannot reproduce failure kohadev-koha@cffdb0ae6429:/kohadevbox/koha$ cat /etc/issue Debian GNU/Linux 9 \n \l kohadev-koha@cffdb0ae6429:/kohadevbox/koha$ prove t/db_dependent/Holds.t t/db_dependent/Koha/Biblios.t t/db_dependent/Koha/Items.t t/db_dependent/Koha/Libraries.t t/db_dependent/LibraryGroups.t t/db_dependent/Holds.t ........... ok t/db_dependent/Koha/Biblios.t .... ok t/db_dependent/Koha/Items.t ...... ok t/db_dependent/Koha/Libraries.t .. ok t/db_dependent/LibraryGroups.t ... ok All tests successful. Files=5, Tests=106, 30 wallclock secs ( 0.08 usr 0.02 sys + 23.13 cusr 3.29 csys = 26.52 CPU) Result: PASS I'm testing in koha-testing-docker with Debian 9.. how are those tests run? It's t/db_dependent/Koha/Item.t, but it's failing randomly. Failing on jenkins on D9: https://jenkins.koha-community.org/job/Koha_Master_D9/1061/consoleFull And more recently: https://jenkins.koha-community.org/job/Koha_Master_D9/1132/consoleFull You certainly need to launch them several times to see them fail (I cannot test right now). They seem to fail quite consistently on U18: https://jenkins.koha-community.org/job/Koha_Master_U18/606/consoleFull https://jenkins.koha-community.org/job/Koha_Master_U18/607/consoleFull A different failure from jenkins (D9 run 1139): https://jenkins.koha-community.org/job/Koha_Master_D9/1139/consoleFull koha_1 | koha_1 | # Failed test 'Patron can place hold if no circ_rules where defined' koha_1 | # at t/db_dependent/Holds.t line 1095. koha_1 | # Structures begin differing at: koha_1 | # $got->{status} = 'libraryNotPickupLocation' koha_1 | # $expected->{status} = 'OK' koha_1 | koha_1 | # Failed test 'Patron cannot place hold because pickup location is not part of hold group' koha_1 | # at t/db_dependent/Holds.t line 1122. koha_1 | # Structures begin differing at: koha_1 | # $got->{status} = 'libraryNotPickupLocation' koha_1 | # $expected->{status} = 'pickupNotInHoldGroup' koha_1 | koha_1 | # Failed test 'Patron can place hold if default_branch_circ_rules is set to "any" for library 2' koha_1 | # at t/db_dependent/Holds.t line 1142. koha_1 | # Structures begin differing at: koha_1 | # $got->{status} = 'libraryNotPickupLocation' koha_1 | # $expected->{status} = 'OK' koha_1 | koha_1 | # Failed test 'Patron cannot place hold if hold_fulfillment_policy is set to "hold group" for library 2' koha_1 | # at t/db_dependent/Holds.t line 1162. koha_1 | # Structures begin differing at: koha_1 | # $got->{status} = 'libraryNotPickupLocation' koha_1 | # $expected->{status} = 'pickupNotInHoldGroup' koha_1 | koha_1 | # Failed test 'Patron can place hold if hold_fulfillment_policy is set to "any" for itemtype 2' koha_1 | # at t/db_dependent/Holds.t line 1182. koha_1 | # Structures begin differing at: koha_1 | # $got->{status} = 'libraryNotPickupLocation' koha_1 | # $expected->{status} = 'OK' koha_1 | koha_1 | # Failed test 'Patron cannot place hold if hold_fulfillment_policy is set to "hold group" for itemtype 2' koha_1 | # at t/db_dependent/Holds.t line 1202. koha_1 | # Structures begin differing at: koha_1 | # $got->{status} = 'libraryNotPickupLocation' koha_1 | # $expected->{status} = 'pickupNotInHoldGroup' koha_1 | koha_1 | # Failed test 'Patron can place hold if hold_fulfillment_policy is set to "any" for itemtype 2 and library 2' koha_1 | # at t/db_dependent/Holds.t line 1222. koha_1 | # Structures begin differing at: koha_1 | # $got->{status} = 'libraryNotPickupLocation' koha_1 | # $expected->{status} = 'OK' koha_1 | koha_1 | # Failed test 'Patron cannot place hold if hold_fulfillment_policy is set to "hold group" for itemtype 2 and library 2' koha_1 | # at t/db_dependent/Holds.t line 1242. koha_1 | # Structures begin differing at: koha_1 | # $got->{status} = 'libraryNotPickupLocation' koha_1 | # $expected->{status} = 'pickupNotInHoldGroup' koha_1 | # Looks like you failed 8 tests of 9. koha_1 | koha_1 | # Failed test 'CanItemBeReserved / pickup_not_in_hold_group' koha_1 | # at t/db_dependent/Holds.t line 1249. koha_1 | # Looks like you failed 1 test of 61. koha_1 | [15:06:03] t/db_dependent/Holds.t (In reply to Jonathan Druart from comment #299) > A different failure from jenkins (D9 run 1139): > > https://jenkins.koha-community.org/job/Koha_Master_D9/1139/consoleFull > > > koha_1 | > koha_1 | # Failed test 'Patron can place hold if no circ_rules > where defined' > koha_1 | # at t/db_dependent/Holds.t line 1095. > koha_1 | # Structures begin differing at: > koha_1 | # $got->{status} = 'libraryNotPickupLocation' > koha_1 | # $expected->{status} = 'OK' > koha_1 | > koha_1 | # Failed test 'Patron cannot place hold because pickup > location is not part of hold group' > koha_1 | # at t/db_dependent/Holds.t line 1122. > koha_1 | # Structures begin differing at: > koha_1 | # $got->{status} = 'libraryNotPickupLocation' > koha_1 | # $expected->{status} = 'pickupNotInHoldGroup' > koha_1 | > koha_1 | # Failed test 'Patron can place hold if > default_branch_circ_rules is set to "any" for library 2' > koha_1 | # at t/db_dependent/Holds.t line 1142. > koha_1 | # Structures begin differing at: > koha_1 | # $got->{status} = 'libraryNotPickupLocation' > koha_1 | # $expected->{status} = 'OK' > koha_1 | > koha_1 | # Failed test 'Patron cannot place hold if > hold_fulfillment_policy is set to "hold group" for library 2' > koha_1 | # at t/db_dependent/Holds.t line 1162. > koha_1 | # Structures begin differing at: > koha_1 | # $got->{status} = 'libraryNotPickupLocation' > koha_1 | # $expected->{status} = 'pickupNotInHoldGroup' > koha_1 | > koha_1 | # Failed test 'Patron can place hold if > hold_fulfillment_policy is set to "any" for itemtype 2' > koha_1 | # at t/db_dependent/Holds.t line 1182. > koha_1 | # Structures begin differing at: > koha_1 | # $got->{status} = 'libraryNotPickupLocation' > koha_1 | # $expected->{status} = 'OK' > koha_1 | > koha_1 | # Failed test 'Patron cannot place hold if > hold_fulfillment_policy is set to "hold group" for itemtype 2' > koha_1 | # at t/db_dependent/Holds.t line 1202. > koha_1 | # Structures begin differing at: > koha_1 | # $got->{status} = 'libraryNotPickupLocation' > koha_1 | # $expected->{status} = 'pickupNotInHoldGroup' > koha_1 | > koha_1 | # Failed test 'Patron can place hold if > hold_fulfillment_policy is set to "any" for itemtype 2 and library 2' > koha_1 | # at t/db_dependent/Holds.t line 1222. > koha_1 | # Structures begin differing at: > koha_1 | # $got->{status} = 'libraryNotPickupLocation' > koha_1 | # $expected->{status} = 'OK' > koha_1 | > koha_1 | # Failed test 'Patron cannot place hold if > hold_fulfillment_policy is set to "hold group" for itemtype 2 and library 2' > koha_1 | # at t/db_dependent/Holds.t line 1242. > koha_1 | # Structures begin differing at: > koha_1 | # $got->{status} = 'libraryNotPickupLocation' > koha_1 | # $expected->{status} = 'pickupNotInHoldGroup' > koha_1 | # Looks like you failed 8 tests of 9. > koha_1 | > koha_1 | # Failed test 'CanItemBeReserved / pickup_not_in_hold_group' > koha_1 | # at t/db_dependent/Holds.t line 1249. > koha_1 | # Looks like you failed 1 test of 61. > koha_1 | [15:06:03] t/db_dependent/Holds.t Hi Jonathan, I think I spotted this failure.. should I make a new bug to fix this tests? The random failure on Item.t I'm at a loss. Supposedly I delete every circulation_rule, every item, every issue and every patron, but it seems not everything gets deleted.. or that's what I guess. Do you have any tip of what could be wrong? (In reply to Agustín Moyano from comment #300) > (In reply to Jonathan Druart from comment #299) > > A different failure from jenkins (D9 run 1139): > > > > https://jenkins.koha-community.org/job/Koha_Master_D9/1139/consoleFull > > > > > > koha_1 | > > koha_1 | # Failed test 'Patron can place hold if no circ_rules > > where defined' > > koha_1 | # at t/db_dependent/Holds.t line 1095. > > koha_1 | # Structures begin differing at: > > koha_1 | # $got->{status} = 'libraryNotPickupLocation' > > koha_1 | # $expected->{status} = 'OK' > > koha_1 | > > koha_1 | # Failed test 'Patron cannot place hold because pickup > > location is not part of hold group' > > koha_1 | # at t/db_dependent/Holds.t line 1122. > > koha_1 | # Structures begin differing at: > > koha_1 | # $got->{status} = 'libraryNotPickupLocation' > > koha_1 | # $expected->{status} = 'pickupNotInHoldGroup' > > koha_1 | > > koha_1 | # Failed test 'Patron can place hold if > > default_branch_circ_rules is set to "any" for library 2' > > koha_1 | # at t/db_dependent/Holds.t line 1142. > > koha_1 | # Structures begin differing at: > > koha_1 | # $got->{status} = 'libraryNotPickupLocation' > > koha_1 | # $expected->{status} = 'OK' > > koha_1 | > > koha_1 | # Failed test 'Patron cannot place hold if > > hold_fulfillment_policy is set to "hold group" for library 2' > > koha_1 | # at t/db_dependent/Holds.t line 1162. > > koha_1 | # Structures begin differing at: > > koha_1 | # $got->{status} = 'libraryNotPickupLocation' > > koha_1 | # $expected->{status} = 'pickupNotInHoldGroup' > > koha_1 | > > koha_1 | # Failed test 'Patron can place hold if > > hold_fulfillment_policy is set to "any" for itemtype 2' > > koha_1 | # at t/db_dependent/Holds.t line 1182. > > koha_1 | # Structures begin differing at: > > koha_1 | # $got->{status} = 'libraryNotPickupLocation' > > koha_1 | # $expected->{status} = 'OK' > > koha_1 | > > koha_1 | # Failed test 'Patron cannot place hold if > > hold_fulfillment_policy is set to "hold group" for itemtype 2' > > koha_1 | # at t/db_dependent/Holds.t line 1202. > > koha_1 | # Structures begin differing at: > > koha_1 | # $got->{status} = 'libraryNotPickupLocation' > > koha_1 | # $expected->{status} = 'pickupNotInHoldGroup' > > koha_1 | > > koha_1 | # Failed test 'Patron can place hold if > > hold_fulfillment_policy is set to "any" for itemtype 2 and library 2' > > koha_1 | # at t/db_dependent/Holds.t line 1222. > > koha_1 | # Structures begin differing at: > > koha_1 | # $got->{status} = 'libraryNotPickupLocation' > > koha_1 | # $expected->{status} = 'OK' > > koha_1 | > > koha_1 | # Failed test 'Patron cannot place hold if > > hold_fulfillment_policy is set to "hold group" for itemtype 2 and library 2' > > koha_1 | # at t/db_dependent/Holds.t line 1242. > > koha_1 | # Structures begin differing at: > > koha_1 | # $got->{status} = 'libraryNotPickupLocation' > > koha_1 | # $expected->{status} = 'pickupNotInHoldGroup' > > koha_1 | # Looks like you failed 8 tests of 9. > > koha_1 | > > koha_1 | # Failed test 'CanItemBeReserved / pickup_not_in_hold_group' > > koha_1 | # at t/db_dependent/Holds.t line 1249. > > koha_1 | # Looks like you failed 1 test of 61. > > koha_1 | [15:06:03] t/db_dependent/Holds.t > > Hi Jonathan, > > I think I spotted this failure.. should I make a new bug to fix this tests? > > The random failure on Item.t I'm at a loss. Supposedly I delete every > circulation_rule, every item, every issue and every patron, but it seems not > everything gets deleted.. or that's what I guess. > > Do you have any tip of what could be wrong? Never mind, I think I nailed it!.. I'll submit patches in following bug! The only D10 failures we have are coming from here: 12:56:37 koha_1 | # Failed test 'ReservesControlBranch: ItemHomeLibrary, biblio1, patron1 should return 6 but returns 7' 12:56:37 koha_1 | # at t/db_dependent/Koha/Biblio.t line 418. 12:56:37 koha_1 | 12:56:37 koha_1 | # Failed test 'ReservesControlBranch: PatronLibrary, biblio1, patron1 should return 6 but returns 7' 12:56:37 koha_1 | # at t/db_dependent/Koha/Biblio.t line 418. 12:56:37 koha_1 | # Looks like you planned 29 tests but ran 31. 12:56:37 koha_1 | # Looks like you failed 2 tests of 31 run. 12:56:37 koha_1 | 12:56:37 koha_1 | # Failed test 'pickup_locations' 12:56:37 koha_1 | # at t/db_dependent/Koha/Biblio.t line 435. 12:56:46 koha_1 | # Looks like you failed 1 test of 12. 12:56:46 koha_1 | [10:56:45] t/db_dependent/Koha/Biblio.t That would be great to see them fixed for 20.05. (In reply to Jonathan Druart from comment #302) > The only D10 failures we have are coming from here: > > 12:56:37 koha_1 | # Failed test 'ReservesControlBranch: > ItemHomeLibrary, biblio1, patron1 should return 6 but returns 7' > 12:56:37 koha_1 | # at t/db_dependent/Koha/Biblio.t line 418. > 12:56:37 koha_1 | > 12:56:37 koha_1 | # Failed test 'ReservesControlBranch: > PatronLibrary, biblio1, patron1 should return 6 but returns 7' > 12:56:37 koha_1 | # at t/db_dependent/Koha/Biblio.t line 418. > 12:56:37 koha_1 | # Looks like you planned 29 tests but ran 31. > 12:56:37 koha_1 | # Looks like you failed 2 tests of 31 run. > 12:56:37 koha_1 | > 12:56:37 koha_1 | # Failed test 'pickup_locations' > 12:56:37 koha_1 | # at t/db_dependent/Koha/Biblio.t line 435. > 12:56:46 koha_1 | # Looks like you failed 1 test of 12. > 12:56:46 koha_1 | [10:56:45] t/db_dependent/Koha/Biblio.t > > That would be great to see them fixed for 20.05. I opened bug 25540. Few days left before 20.05! There was a typo "Koka" vs "Koha" in this patch, fixed on bug 26900. That should have been caught earlier, and it highlights that some of the code is not covered by tests. That would be good to provide them. Created attachment 115202 [details] [review] Bug 22284: (QA follow-up) Fix test failures Only libraries that are a pickup location will be returned as a pickup location (In reply to Nick Clemens from comment #305) > Created attachment 115202 [details] [review] [review] > Bug 22284: (QA follow-up) Fix test failures > > Only libraries that are a pickup location will be returned as a pickup > location Failing on Jenkins To test: prove -v t/db_dependent/Koha/Biblio.t it succeeds (maybe) add a library that is not a pickup_location prove the test it fails apply the patch prove! it passes! (In reply to Nick Clemens from comment #306) > (In reply to Nick Clemens from comment #305) > > Created attachment 115202 [details] [review] [review] [review] > > Bug 22284: (QA follow-up) Fix test failures > > > > Only libraries that are a pickup location will be returned as a pickup > > location > > Failing on Jenkins > > To test: > prove -v t/db_dependent/Koha/Biblio.t > it succeeds (maybe) > add a library that is not a pickup_location > prove the test > it fails > apply the patch > prove! > it passes! 20.05, not master *** Bug 15708 has been marked as a duplicate of this bug. *** |