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. ***

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.

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>