Bug 22284 - Add ability to define groups of locations for hold pickup
Summary: Add ability to define groups of locations for hold pickup
Status: RESOLVED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Hold requests (show other bugs)
Version: master
Hardware: All All
: P5 - low enhancement (vote)
Assignee: Agustín Moyano
QA Contact: Tomás Cohen Arazi
URL:
Keywords:
: 20747 (view as bug list)
Depends on: 15496 18930 22688
Blocks: 24657 25421 25540 24350
  Show dependency treegraph
 
Reported: 2019-02-07 08:00 UTC by Kyle M Hall
Modified: 2020-05-19 11:04 UTC (History)
13 users (show)

See Also:
Change sponsored?: Sponsored
Patch complexity: Medium patch
Who signed the patch off:
Text to go in the release notes:
Adds the ability to define groups of libraries for use in holds policy.
Version(s) released in:
20.05.00


Attachments
Bug 22284: New ft_local_hold_group column, and 'holgroup' enum option (6.80 KB, patch)
2019-04-17 06:24 UTC, Agustín Moyano
Details | Diff | Splinter Review
Bug 22284: (follow-up) Add schema modifications (6.67 KB, patch)
2019-04-17 06:24 UTC, Agustín Moyano
Details | Diff | Splinter Review
Bug 22284: Add new checkbox in Library Groups (8.48 KB, patch)
2019-04-17 06:24 UTC, Agustín Moyano
Details | Diff | Splinter Review
Bug 22284: New methods in Koha::Library::Groups and Koha::Library (7.75 KB, patch)
2019-04-17 06:24 UTC, Agustín Moyano
Details | Diff | Splinter Review
Bug 22284: New hold group options in smart-rules (8.00 KB, patch)
2019-04-17 06:24 UTC, Agustín Moyano
Details | Diff | Splinter Review
Bug 22284: Control hold groups in C4::Reserves (22.82 KB, patch)
2019-04-17 06:24 UTC, Agustín Moyano
Details | Diff | Splinter Review
Bug 22284: New messages in reserve/request.tt (2.89 KB, patch)
2019-04-17 06:24 UTC, Agustín Moyano
Details | Diff | Splinter Review
Bug 22284: Opac pickup_locations (44.99 KB, patch)
2019-04-17 06:24 UTC, Agustín Moyano
Details | Diff | Splinter Review
Bug 22284: New methods in Koha::Library::Groups and Koha::Library (7.58 KB, patch)
2019-04-20 20:13 UTC, Agustín Moyano
Details | Diff | Splinter Review
Bug 22284: (follow-up) Remove commented warn and address test failures (3.82 KB, patch)
2019-04-23 19:24 UTC, Agustín Moyano
Details | Diff | Splinter Review
Bug 22284: New ft_local_hold_group column, and 'holgroup' enum option (6.80 KB, patch)
2019-04-25 16:30 UTC, Agustín Moyano
Details | Diff | Splinter Review
Bug 22284: (follow-up) Add schema modifications (6.67 KB, patch)
2019-04-25 16:30 UTC, Agustín Moyano
Details | Diff | Splinter Review
Bug 22284: Add new checkbox in Library Groups (8.48 KB, patch)
2019-04-25 16:30 UTC, Agustín Moyano
Details | Diff | Splinter Review
Bug 22284: New hold group options in smart-rules (8.00 KB, patch)
2019-04-25 16:31 UTC, Agustín Moyano
Details | Diff | Splinter Review
Bug 22284: Control hold groups in C4::Reserves (22.82 KB, patch)
2019-04-25 16:31 UTC, Agustín Moyano
Details | Diff | Splinter Review
Bug 22284: New messages in reserve/request.tt (2.89 KB, patch)
2019-04-25 16:31 UTC, Agustín Moyano
Details | Diff | Splinter Review
Bug 22284: Opac pickup_locations (44.99 KB, patch)
2019-04-25 16:31 UTC, Agustín Moyano
Details | Diff | Splinter Review
Bug 22284: New methods in Koha::Library::Groups and Koha::Library (7.58 KB, patch)
2019-04-25 16:31 UTC, Agustín Moyano
Details | Diff | Splinter Review
Bug 22284: (follow-up) Remove commented warn and address test failures (3.82 KB, patch)
2019-04-25 16:31 UTC, Agustín Moyano
Details | Diff | Splinter Review
Bug 22284: (follow-up) fix test count after merge (630 bytes, patch)
2019-04-25 16:31 UTC, Agustín Moyano
Details | Diff | Splinter Review
Bug 22284: New ft_local_hold_group column, and 'holgroup' enum option (6.63 KB, patch)
2019-04-26 17:23 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 22284: (follow-up) Add schema modifications (6.73 KB, patch)
2019-04-26 17:23 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 22284: Add new checkbox in Library Groups (8.52 KB, patch)
2019-04-26 17:23 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 22284: New hold group options in smart-rules (8.04 KB, patch)
2019-04-26 17:23 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 22284: Control hold groups in C4::Reserves (22.80 KB, patch)
2019-04-26 17:23 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 22284: New messages in reserve/request.tt (2.95 KB, patch)
2019-04-26 17:23 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 22284: Opac pickup_locations (45.01 KB, patch)
2019-04-26 17:23 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 22284: New methods in Koha::Library::Groups and Koha::Library (7.61 KB, patch)
2019-04-26 17:23 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 22284: (follow-up) Remove commented warn and address test failures (3.87 KB, patch)
2019-04-26 17:23 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 22284: (follow-up) fix test count after merge (685 bytes, patch)
2019-04-26 17:23 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 22284: (follow-up) fixes after 15496 (729 bytes, patch)
2019-04-26 17:34 UTC, Agustín Moyano
Details | Diff | Splinter Review
Bug 22284: New ft_local_hold_group column, and 'holgroup' enum option (6.65 KB, patch)
2019-04-26 19:16 UTC, Agustín Moyano
Details | Diff | Splinter Review
Bug 22284: (follow-up) Add schema modifications (6.73 KB, patch)
2019-04-26 19:16 UTC, Agustín Moyano
Details | Diff | Splinter Review
Bug 22284: Add new checkbox in Library Groups (8.54 KB, patch)
2019-04-26 19:16 UTC, Agustín Moyano
Details | Diff | Splinter Review
Bug 22284: New hold group options in smart-rules (8.05 KB, patch)
2019-04-26 19:16 UTC, Agustín Moyano
Details | Diff | Splinter Review
Bug 22284: Control hold groups in C4::Reserves (22.80 KB, patch)
2019-04-26 19:17 UTC, Agustín Moyano
Details | Diff | Splinter Review
Bug 22284: New messages in reserve/request.tt (3.03 KB, patch)
2019-04-26 19:17 UTC, Agustín Moyano
Details | Diff | Splinter Review
Bug 22284: Opac pickup_locations (45.05 KB, patch)
2019-04-26 19:17 UTC, Agustín Moyano
Details | Diff | Splinter Review
Bug 22284: New methods in Koha::Library::Groups and Koha::Library (7.63 KB, patch)
2019-04-26 19:17 UTC, Agustín Moyano
Details | Diff | Splinter Review
Bug 22284: (follow-up) Remove commented warn and address test failures (3.87 KB, patch)
2019-04-26 19:17 UTC, Agustín Moyano
Details | Diff | Splinter Review
Bug 22284: (follow-up) fix test count after merge (685 bytes, patch)
2019-04-26 19:17 UTC, Agustín Moyano
Details | Diff | Splinter Review
Bug 22284: (follow-up) fixes after 15496 (729 bytes, patch)
2019-04-26 19:17 UTC, Agustín Moyano
Details | Diff | Splinter Review
Bug 22284: New ft_local_hold_group column, and 'holgroup' enum option (6.65 KB, patch)
2019-04-30 02:44 UTC, Agustín Moyano
Details | Diff | Splinter Review
Bug 22284: (follow-up) Add schema modifications (6.73 KB, patch)
2019-04-30 02:44 UTC, Agustín Moyano
Details | Diff | Splinter Review
Bug 22284: Add new checkbox in Library Groups (8.54 KB, patch)
2019-04-30 02:44 UTC, Agustín Moyano
Details | Diff | Splinter Review
Bug 22284: New hold group options in smart-rules (8.05 KB, patch)
2019-04-30 02:44 UTC, Agustín Moyano
Details | Diff | Splinter Review
Bug 22284: Control hold groups in C4::Reserves (22.80 KB, patch)
2019-04-30 02:44 UTC, Agustín Moyano
Details | Diff | Splinter Review
Bug 22284: New methods in Koha::Library::Groups and Koha::Library (7.63 KB, patch)
2019-04-30 02:44 UTC, Agustín Moyano
Details | Diff | Splinter Review
Bug 22284: Opac pickup_locations (45.05 KB, patch)
2019-04-30 02:45 UTC, Agustín Moyano
Details | Diff | Splinter Review
Bug 22284: (follow-up) Default to circulation control branch when no hold libraries are found (961 bytes, patch)
2019-04-30 02:45 UTC, Agustín Moyano
Details | Diff | Splinter Review
Bug 22284: New message, new column and filter pickup locations in reserve/request.tt (6.72 KB, patch)
2019-04-30 02:45 UTC, Agustín Moyano
Details | Diff | Splinter Review
Bug 22284: (follow-up) Remove commented warn and address test failures (3.87 KB, patch)
2019-04-30 02:45 UTC, Agustín Moyano
Details | Diff | Splinter Review
Bug 22284: (follow-up) fix test count after merge (685 bytes, patch)
2019-04-30 02:45 UTC, Agustín Moyano
Details | Diff | Splinter Review
Bug 22284: (follow-up) fixes after 15496 (729 bytes, patch)
2019-04-30 02:45 UTC, Agustín Moyano
Details | Diff | Splinter Review
Bug 22284: (follow-up) fixes after 18936 (44.27 KB, patch)
2019-04-30 05:07 UTC, Agustín Moyano
Details | Diff | Splinter Review
Bug 22284: New ft_local_hold_group column, and 'holgroup' enum option (3.90 KB, patch)
2019-04-30 05:13 UTC, Agustín Moyano
Details | Diff | Splinter Review
Bug 22284: (follow-up) Add schema modifications (6.73 KB, patch)
2019-04-30 05:13 UTC, Agustín Moyano
Details | Diff | Splinter Review
Bug 22284: Add new checkbox in Library Groups (8.54 KB, patch)
2019-04-30 05:13 UTC, Agustín Moyano
Details | Diff | Splinter Review
Bug 22284: New hold group options in smart-rules (5.85 KB, patch)
2019-04-30 05:13 UTC, Agustín Moyano
Details | Diff | Splinter Review
Bug 22284: Control hold groups in C4::Reserves (22.80 KB, patch)
2019-04-30 05:13 UTC, Agustín Moyano
Details | Diff | Splinter Review
Bug 22284: New methods in Koha::Library::Groups and Koha::Library (7.63 KB, patch)
2019-04-30 05:14 UTC, Agustín Moyano
Details | Diff | Splinter Review
Bug 22284: Opac pickup_locations (45.05 KB, patch)
2019-04-30 05:14 UTC, Agustín Moyano
Details | Diff | Splinter Review
Bug 22284: (follow-up) Default to circulation control branch when no hold libraries are found (961 bytes, patch)
2019-04-30 05:14 UTC, Agustín Moyano
Details | Diff | Splinter Review
Bug 22284: New message, new column and filter pickup locations in reserve/request.tt (6.72 KB, patch)
2019-04-30 05:14 UTC, Agustín Moyano
Details | Diff | Splinter Review
Bug 22284: (follow-up) Remove commented warn and address test failures (2.39 KB, patch)
2019-04-30 05:14 UTC, Agustín Moyano
Details | Diff | Splinter Review
Bug 22284: (follow-up) fix test count after merge (685 bytes, patch)
2019-04-30 05:14 UTC, Agustín Moyano
Details | Diff | Splinter Review
Bug 22284: (follow-up) fixes after 15496 (729 bytes, patch)
2019-04-30 05:14 UTC, Agustín Moyano
Details | Diff | Splinter Review
Bug 22284: (follow-up) fixes after 18936 (44.27 KB, patch)
2019-04-30 05:14 UTC, Agustín Moyano
Details | Diff | Splinter Review
Bug 22284: New ft_local_hold_group column, and 'holgroup' enum option (3.90 KB, patch)
2019-04-30 14:58 UTC, Agustín Moyano
Details | Diff | Splinter Review
Bug 22284: (follow-up) Add schema modifications (6.73 KB, patch)
2019-04-30 14:58 UTC, Agustín Moyano
Details | Diff | Splinter Review
Bug 22284: Add new checkbox in Library Groups (8.54 KB, patch)
2019-04-30 14:59 UTC, Agustín Moyano
Details | Diff | Splinter Review
Bug 22284: New hold group options in smart-rules (5.85 KB, patch)
2019-04-30 14:59 UTC, Agustín Moyano
Details | Diff | Splinter Review
Bug 22284: Control hold groups in C4::Reserves (22.80 KB, patch)
2019-04-30 14:59 UTC, Agustín Moyano
Details | Diff | Splinter Review
Bug 22284: New methods in Koha::Library::Groups and Koha::Library (7.63 KB, patch)
2019-04-30 14:59 UTC, Agustín Moyano
Details | Diff | Splinter Review
Bug 22284: Opac pickup_locations (45.05 KB, patch)
2019-04-30 14:59 UTC, Agustín Moyano
Details | Diff | Splinter Review
Bug 22284: (follow-up) Default to circulation control branch when no hold libraries are found (961 bytes, patch)
2019-04-30 14:59 UTC, Agustín Moyano
Details | Diff | Splinter Review
Bug 22284: New message, new column and filter pickup locations in reserve/request.tt (6.35 KB, patch)
2019-04-30 14:59 UTC, Agustín Moyano
Details | Diff | Splinter Review
Bug 22284: Filter item's pickup locations in holds_table.inc (1.80 KB, patch)
2019-04-30 14:59 UTC, Agustín Moyano
Details | Diff | Splinter Review
Bug 22284: (follow-up) Remove commented warn and address test failures (2.39 KB, patch)
2019-04-30 14:59 UTC, Agustín Moyano
Details | Diff | Splinter Review
Bug 22284: (follow-up) fix test count after merge (685 bytes, patch)
2019-04-30 14:59 UTC, Agustín Moyano
Details | Diff | Splinter Review
Bug 22284: (follow-up) fixes after 15496 (729 bytes, patch)
2019-04-30 15:00 UTC, Agustín Moyano
Details | Diff | Splinter Review
Bug 22284: (follow-up) fixes after 18936 (45.35 KB, patch)
2019-04-30 15:00 UTC, Agustín Moyano
Details | Diff | Splinter Review
MARC records for testing, compatible with default branches (528.89 KB, application/octet-stream)
2019-05-09 19:29 UTC, Liz Rea
Details
Bug 22284: New ft_local_hold_group column, and 'holgroup' enum option (3.90 KB, patch)
2019-05-16 14:18 UTC, Agustín Moyano
Details | Diff | Splinter Review
Bug 22284: (follow-up) Add schema modifications (6.73 KB, patch)
2019-05-16 14:18 UTC, Agustín Moyano
Details | Diff | Splinter Review
Bug 22284: Add new checkbox in Library Groups (8.54 KB, patch)
2019-05-16 14:18 UTC, Agustín Moyano
Details | Diff | Splinter Review
Bug 22284: New hold group options in smart-rules (5.85 KB, patch)
2019-05-16 14:18 UTC, Agustín Moyano
Details | Diff | Splinter Review
Bug 22284: Control hold groups in C4::Reserves (22.79 KB, patch)
2019-05-16 14:18 UTC, Agustín Moyano
Details | Diff | Splinter Review
Bug 22284: New methods in Koha::Library::Groups and Koha::Library (7.63 KB, patch)
2019-05-16 14:18 UTC, Agustín Moyano
Details | Diff | Splinter Review
Bug 22284: Opac pickup_locations (45.05 KB, patch)
2019-05-16 14:18 UTC, Agustín Moyano
Details | Diff | Splinter Review
Bug 22284: (follow-up) Default to circulation control branch when no hold libraries are found (961 bytes, patch)
2019-05-16 14:19 UTC, Agustín Moyano
Details | Diff | Splinter Review
Bug 22284: New message, new column and filter pickup locations in reserve/request.tt (6.35 KB, patch)
2019-05-16 14:19 UTC, Agustín Moyano
Details | Diff | Splinter Review
Bug 22284: Filter item's pickup locations in holds_table.inc (1.80 KB, patch)
2019-05-16 14:19 UTC, Agustín Moyano
Details | Diff | Splinter Review
Bug 22284: (follow-up) Remove commented warn and address test failures (2.39 KB, patch)
2019-05-16 14:19 UTC, Agustín Moyano
Details | Diff | Splinter Review
Bug 22284: (follow-up) fix test count after merge (685 bytes, patch)
2019-05-16 14:19 UTC, Agustín Moyano
Details | Diff | Splinter Review
Bug 22284: (follow-up) fixes after 15496 (729 bytes, patch)
2019-05-16 14:19 UTC, Agustín Moyano
Details | Diff | Splinter Review
Bug 22284: (follow-up) fixes after 18936 (45.35 KB, patch)
2019-05-16 14:19 UTC, Agustín Moyano
Details | Diff | Splinter Review
Bug 22284: (follow-up) Remove HomeOrHolding from reserves (3.49 KB, patch)
2019-05-28 04:53 UTC, Agustín Moyano
Details | Diff | Splinter Review
Bug 22284: Add "patron's hold group" as new hold_fulfillment_policy option (6.64 KB, patch)
2019-05-28 04:53 UTC, Agustín Moyano
Details | Diff | Splinter Review
Bug 22284: (follow-up) Use GetReserveControlBranch in Koha::Item->pickup_locations (1.21 KB, patch)
2019-05-30 03:22 UTC, Agustín Moyano
Details | Diff | Splinter Review
Bug 22284: (follow-up) Fix tests (58.69 KB, patch)
2019-05-30 03:22 UTC, Agustín Moyano
Details | Diff | Splinter Review
Bug 22284: Add "patron's hold group" as new hold_fulfillment_policy option (7.39 KB, patch)
2019-05-30 03:40 UTC, Agustín Moyano
Details | Diff | Splinter Review
Bug 22284: (follow-up) Use GetReserveControlBranch in Koha::Item->pickup_locations (1.21 KB, patch)
2019-05-30 03:40 UTC, Agustín Moyano
Details | Diff | Splinter Review
Bug 22284: (follow-up) Fix tests (58.69 KB, patch)
2019-05-30 03:40 UTC, Agustín Moyano
Details | Diff | Splinter Review
Bug 22284: (follow-up) Fix typo in request.tt (1.28 KB, patch)
2019-06-11 03:05 UTC, Agustín Moyano
Details | Diff | Splinter Review
Bug 22284: (follow-up) Filter pickup on specific item click (4.02 KB, patch)
2019-06-12 03:26 UTC, Agustín Moyano
Details | Diff | Splinter Review
Bug 22284: New ft_local_hold_group column, and 'holgroup' enum option (3.95 KB, patch)
2019-07-04 05:27 UTC, Josef Moravec
Details | Diff | Splinter Review
Bug 22284: Add new checkbox in Library Groups (8.59 KB, patch)
2019-07-04 05:27 UTC, Josef Moravec
Details | Diff | Splinter Review
Bug 22284: New hold group options in smart-rules (5.90 KB, patch)
2019-07-04 05:27 UTC, Josef Moravec
Details | Diff | Splinter Review
Bug 22284: Control hold groups in C4::Reserves (22.84 KB, patch)
2019-07-04 05:27 UTC, Josef Moravec
Details | Diff | Splinter Review
Bug 22284: New methods in Koha::Library::Groups and Koha::Library (7.69 KB, patch)
2019-07-04 05:27 UTC, Josef Moravec
Details | Diff | Splinter Review
Bug 22284: Opac pickup_locations (45.10 KB, patch)
2019-07-04 05:27 UTC, Josef Moravec
Details | Diff | Splinter Review
Bug 22284: (follow-up) Default to circulation control branch when no hold libraries are found (1016 bytes, patch)
2019-07-04 05:27 UTC, Josef Moravec
Details | Diff | Splinter Review
Bug 22284: New message, new column and filter pickup locations in reserve/request.tt (6.41 KB, patch)
2019-07-04 05:28 UTC, Josef Moravec
Details | Diff | Splinter Review
Bug 22284: Filter item's pickup locations in holds_table.inc (1.86 KB, patch)
2019-07-04 05:28 UTC, Josef Moravec
Details | Diff | Splinter Review
Bug 22284: (follow-up) Remove commented warn and address test failures (2.45 KB, patch)
2019-07-04 05:28 UTC, Josef Moravec
Details | Diff | Splinter Review
Bug 22284: (follow-up) fix test count after merge (741 bytes, patch)
2019-07-04 05:28 UTC, Josef Moravec
Details | Diff | Splinter Review
Bug 22284: (follow-up) fixes after 15496 (784 bytes, patch)
2019-07-04 05:28 UTC, Josef Moravec
Details | Diff | Splinter Review
Bug 22284: (follow-up) fixes after 18936 (45.41 KB, patch)
2019-07-04 05:28 UTC, Josef Moravec
Details | Diff | Splinter Review
Bug 22284: (follow-up) Remove HomeOrHolding from reserves (3.54 KB, patch)
2019-07-04 05:28 UTC, Josef Moravec
Details | Diff | Splinter Review
Bug 22284: Add "patron's hold group" as new hold_fulfillment_policy option (7.45 KB, patch)
2019-07-04 05:28 UTC, Josef Moravec
Details | Diff | Splinter Review
Bug 22284: (follow-up) Use GetReserveControlBranch in Koha::Item->pickup_locations (1.26 KB, patch)
2019-07-04 05:28 UTC, Josef Moravec
Details | Diff | Splinter Review
Bug 22284: (follow-up) Fix tests (58.75 KB, patch)
2019-07-04 05:29 UTC, Josef Moravec
Details | Diff | Splinter Review
Bug 22284: (follow-up) Fix typo in request.tt (1.33 KB, patch)
2019-07-04 05:29 UTC, Josef Moravec
Details | Diff | Splinter Review
Bug 22284: (follow-up) Filter pickup on specific item click (4.07 KB, patch)
2019-07-04 05:29 UTC, Josef Moravec
Details | Diff | Splinter Review
Bug 22284: (Do not push) Schema update (1.55 KB, patch)
2019-07-04 05:29 UTC, Josef Moravec
Details | Diff | Splinter Review
Bug 22284: (follow-up) Fix typos transfered -> transferred (1.39 KB, patch)
2019-07-04 05:32 UTC, Josef Moravec
Details | Diff | Splinter Review
Bug 22284: New ft_local_hold_group column, and 'holgroup' enum option (3.94 KB, patch)
2019-07-22 18:41 UTC, Agustín Moyano
Details | Diff | Splinter Review
Bug 22284: Add new checkbox in Library Groups (8.58 KB, patch)
2019-07-22 18:41 UTC, Agustín Moyano
Details | Diff | Splinter Review
Bug 22284: New hold group options in smart-rules (5.90 KB, patch)
2019-07-22 18:41 UTC, Agustín Moyano
Details | Diff | Splinter Review
Bug 22284: Control hold groups in C4::Reserves (22.83 KB, patch)
2019-07-22 18:41 UTC, Agustín Moyano
Details | Diff | Splinter Review
Bug 22284: New methods in Koha::Library::Groups and Koha::Library (7.67 KB, patch)
2019-07-22 18:41 UTC, Agustín Moyano
Details | Diff | Splinter Review
Bug 22284: Opac pickup_locations (44.96 KB, patch)
2019-07-22 18:41 UTC, Agustín Moyano
Details | Diff | Splinter Review
Bug 22284: (follow-up) Default to circulation control branch when no hold libraries are found (1016 bytes, patch)
2019-07-22 18:41 UTC, Agustín Moyano
Details | Diff | Splinter Review
Bug 22284: New message, new column and filter pickup locations in reserve/request.tt (6.51 KB, patch)
2019-07-22 18:41 UTC, Agustín Moyano
Details | Diff | Splinter Review
Bug 22284: Filter item's pickup locations in holds_table.inc (1.94 KB, patch)
2019-07-22 18:41 UTC, Agustín Moyano
Details | Diff | Splinter Review
Bug 22284: (follow-up) Remove commented warn and address test failures (2.45 KB, patch)
2019-07-22 18:41 UTC, Agustín Moyano
Details | Diff | Splinter Review
Bug 22284: (follow-up) fix test count after merge (741 bytes, patch)
2019-07-22 18:41 UTC, Agustín Moyano
Details | Diff | Splinter Review
Bug 22284: (follow-up) fixes after 15496 (784 bytes, patch)
2019-07-22 18:42 UTC, Agustín Moyano
Details | Diff | Splinter Review
Bug 22284: (follow-up) fixes after 18936 (45.37 KB, patch)
2019-07-22 18:42 UTC, Agustín Moyano
Details | Diff | Splinter Review
Bug 22284: (follow-up) Remove HomeOrHolding from reserves (3.54 KB, patch)
2019-07-22 18:42 UTC, Agustín Moyano
Details | Diff | Splinter Review
Bug 22284: Add "patron's hold group" as new hold_fulfillment_policy option (7.41 KB, patch)
2019-07-22 18:42 UTC, Agustín Moyano
Details | Diff | Splinter Review
Bug 22284: (follow-up) Use GetReserveControlBranch in Koha::Item->pickup_locations (1.26 KB, patch)
2019-07-22 18:42 UTC, Agustín Moyano
Details | Diff | Splinter Review
Bug 22284: (follow-up) Fix tests (58.73 KB, patch)
2019-07-22 18:42 UTC, Agustín Moyano
Details | Diff | Splinter Review
Bug 22284: (follow-up) Fix typo in request.tt (1.33 KB, patch)
2019-07-22 18:42 UTC, Agustín Moyano
Details | Diff | Splinter Review
Bug 22284: (follow-up) Filter pickup on specific item click (4.06 KB, patch)
2019-07-22 18:42 UTC, Agustín Moyano
Details | Diff | Splinter Review
Bug 22284: (Do not push) Schema update (1.55 KB, patch)
2019-07-22 18:42 UTC, Agustín Moyano
Details | Diff | Splinter Review
Bug 22284: (follow-up) Fix typos transfered -> transferred (1.39 KB, patch)
2019-07-22 18:42 UTC, Agustín Moyano
Details | Diff | Splinter Review
Bug 22284: (follow-up) fixes after 18936 (45.48 KB, patch)
2019-07-23 17:18 UTC, Agustín Moyano
Details | Diff | Splinter Review
Bug 22284: (follow-up) Remove HomeOrHolding from reserves (3.54 KB, patch)
2019-07-23 17:19 UTC, Agustín Moyano
Details | Diff | Splinter Review
Bug 22284: Add "patron's hold group" as new hold_fulfillment_policy option (7.41 KB, patch)
2019-07-23 17:19 UTC, Agustín Moyano
Details | Diff | Splinter Review
Bug 22284: (follow-up) Use GetReserveControlBranch in Koha::Item->pickup_locations (1.26 KB, patch)
2019-07-23 17:19 UTC, Agustín Moyano
Details | Diff | Splinter Review
Bug 22284: (follow-up) Fix tests (58.73 KB, patch)
2019-07-23 17:19 UTC, Agustín Moyano
Details | Diff | Splinter Review
Bug 22284: (follow-up) Fix typo in request.tt (1.33 KB, patch)
2019-07-23 17:19 UTC, Agustín Moyano
Details | Diff | Splinter Review
Bug 22284: (follow-up) Filter pickup on specific item click (4.06 KB, patch)
2019-07-23 17:19 UTC, Agustín Moyano
Details | Diff | Splinter Review
Bug 22284: (Do not push) Schema update (1.55 KB, patch)
2019-07-23 17:20 UTC, Agustín Moyano
Details | Diff | Splinter Review
Bug 22284: (follow-up) Fix typos transfered -> transferred (1.39 KB, patch)
2019-07-23 17:20 UTC, Agustín Moyano
Details | Diff | Splinter Review
Bug 22284: New ft_local_hold_group column, and 'holgroup' enum option (3.94 KB, patch)
2019-09-03 14:42 UTC, Agustín Moyano
Details | Diff | Splinter Review
Bug 22284: Add new checkbox in Library Groups (8.58 KB, patch)
2019-09-03 14:42 UTC, Agustín Moyano
Details | Diff | Splinter Review
Bug 22284: New hold group options in smart-rules (5.90 KB, patch)
2019-09-03 14:43 UTC, Agustín Moyano
Details | Diff | Splinter Review
Bug 22284: Control hold groups in C4::Reserves (22.83 KB, patch)
2019-09-03 14:43 UTC, Agustín Moyano
Details | Diff | Splinter Review
Bug 22284: New methods in Koha::Library::Groups and Koha::Library (7.67 KB, patch)
2019-09-03 14:43 UTC, Agustín Moyano
Details | Diff | Splinter Review
Bug 22284: Opac pickup_locations (44.91 KB, patch)
2019-09-03 14:43 UTC, Agustín Moyano
Details | Diff | Splinter Review
Bug 22284: (follow-up) Default to circulation control branch when no hold libraries are found (1016 bytes, patch)
2019-09-03 14:44 UTC, Agustín Moyano
Details | Diff | Splinter Review
Bug 22284: New message, new column and filter pickup locations in reserve/request.tt (7.11 KB, patch)
2019-09-03 14:44 UTC, Agustín Moyano
Details | Diff | Splinter Review
Bug 22284: Filter item's pickup locations in holds_table.inc (1.94 KB, patch)
2019-09-03 14:44 UTC, Agustín Moyano
Details | Diff | Splinter Review
Bug 22284: (follow-up) Remove commented warn and address test failures (2.45 KB, patch)
2019-09-03 14:44 UTC, Agustín Moyano
Details | Diff | Splinter Review
Bug 22284: (follow-up) fix test count after merge (741 bytes, patch)
2019-09-03 14:45 UTC, Agustín Moyano
Details | Diff | Splinter Review
Bug 22284: (follow-up) fixes after 15496 (784 bytes, patch)
2019-09-03 14:45 UTC, Agustín Moyano
Details | Diff | Splinter Review
Bug 22284: (follow-up) fixes after 18936 (45.48 KB, patch)
2019-09-03 14:45 UTC, Agustín Moyano
Details | Diff | Splinter Review
Bug 22284: (follow-up) Remove HomeOrHolding from reserves (3.54 KB, patch)
2019-09-03 14:45 UTC, Agustín Moyano
Details | Diff | Splinter Review
Bug 22284: Add "patron's hold group" as new hold_fulfillment_policy option (7.41 KB, patch)
2019-09-03 14:45 UTC, Agustín Moyano
Details | Diff | Splinter Review
Bug 22284: (follow-up) Use GetReserveControlBranch in Koha::Item->pickup_locations (1.26 KB, patch)
2019-09-03 14:46 UTC, Agustín Moyano
Details | Diff | Splinter Review
Bug 22284: (follow-up) Fix tests (58.73 KB, patch)
2019-09-03 14:46 UTC, Agustín Moyano
Details | Diff | Splinter Review
Bug 22284: (follow-up) Fix typo in request.tt (4.18 KB, patch)
2019-09-03 14:46 UTC, Agustín Moyano
Details | Diff | Splinter Review
Bug 22284: (follow-up) Filter pickup on specific item click (1.25 KB, patch)
2019-09-03 14:46 UTC, Agustín Moyano
Details | Diff | Splinter Review
Bug 22284: (Do not push) Schema update (1.55 KB, patch)
2019-09-03 14:46 UTC, Agustín Moyano
Details | Diff | Splinter Review
Bug 22284: (follow-up) Fix typos transfered -> transferred (1.39 KB, patch)
2019-09-03 14:47 UTC, Agustín Moyano
Details | Diff | Splinter Review
Bug 22284: New ft_local_hold_group column, and 'holgroup' enum option (3.94 KB, patch)
2019-09-12 17:58 UTC, Agustín Moyano
Details | Diff | Splinter Review
Bug 22284: Add new checkbox in Library Groups (8.58 KB, patch)
2019-09-12 17:58 UTC, Agustín Moyano
Details | Diff | Splinter Review
Bug 22284: New hold group options in smart-rules (5.90 KB, patch)
2019-09-12 17:58 UTC, Agustín Moyano
Details | Diff | Splinter Review
Bug 22284: Control hold groups in C4::Reserves (22.83 KB, patch)
2019-09-12 17:58 UTC, Agustín Moyano
Details | Diff | Splinter Review
Bug 22284: New methods in Koha::Library::Groups and Koha::Library (7.67 KB, patch)
2019-09-12 17:59 UTC, Agustín Moyano
Details | Diff | Splinter Review
Bug 22284: Opac pickup_locations (44.91 KB, patch)
2019-09-12 17:59 UTC, Agustín Moyano
Details | Diff | Splinter Review
Bug 22284: (follow-up) Default to circulation control branch when no hold libraries are found (1016 bytes, patch)
2019-09-12 17:59 UTC, Agustín Moyano
Details | Diff | Splinter Review
Bug 22284: New message, new column and filter pickup locations in reserve/request.tt (7.11 KB, patch)
2019-09-12 17:59 UTC, Agustín Moyano
Details | Diff | Splinter Review
Bug 22284: Filter item's pickup locations in holds_table.inc (1.94 KB, patch)
2019-09-12 17:59 UTC, Agustín Moyano
Details | Diff | Splinter Review
Bug 22284: (follow-up) Squash multiple follow-ups (50.31 KB, patch)
2019-09-12 18:00 UTC, Agustín Moyano
Details | Diff | Splinter Review
Bug 22284: Add "patron's hold group" as new hold_fulfillment_policy option (7.41 KB, patch)
2019-09-12 18:00 UTC, Agustín Moyano
Details | Diff | Splinter Review
Bug 22284: (follow-up) Squash multiple follow-ups (65.95 KB, patch)
2019-09-12 18:00 UTC, Agustín Moyano
Details | Diff | Splinter Review
Bug 22284: (Do not push) Schema update (1.55 KB, patch)
2019-09-12 18:01 UTC, Agustín Moyano
Details | Diff | Splinter Review
Bug 22284: New ft_local_hold_group column, and 'holgroup' enum option (4.00 KB, patch)
2019-09-16 20:25 UTC, Liz Rea
Details | Diff | Splinter Review
Bug 22284: Add new checkbox in Library Groups (8.63 KB, patch)
2019-09-16 20:25 UTC, Liz Rea
Details | Diff | Splinter Review
Bug 22284: New hold group options in smart-rules (5.95 KB, patch)
2019-09-16 20:25 UTC, Liz Rea
Details | Diff | Splinter Review
Bug 22284: Control hold groups in C4::Reserves (22.89 KB, patch)
2019-09-16 20:26 UTC, Liz Rea
Details | Diff | Splinter Review
Bug 22284: New methods in Koha::Library::Groups and Koha::Library (7.73 KB, patch)
2019-09-16 20:26 UTC, Liz Rea
Details | Diff | Splinter Review
Bug 22284: Opac pickup_locations (44.99 KB, patch)
2019-09-16 20:26 UTC, Liz Rea
Details | Diff | Splinter Review
Bug 22284: (follow-up) Default to circulation control branch when no hold libraries are found (1.04 KB, patch)
2019-09-16 20:26 UTC, Liz Rea
Details | Diff | Splinter Review
Bug 22284: New message, new column and filter pickup locations in reserve/request.tt (7.16 KB, patch)
2019-09-16 20:26 UTC, Liz Rea
Details | Diff | Splinter Review
Bug 22284: Filter item's pickup locations in holds_table.inc (1.98 KB, patch)
2019-09-16 20:26 UTC, Liz Rea
Details | Diff | Splinter Review
Bug 22284: (follow-up) Squash multiple follow-ups (50.41 KB, patch)
2019-09-16 20:26 UTC, Liz Rea
Details | Diff | Splinter Review
Bug 22284: Add "patron's hold group" as new hold_fulfillment_policy option (7.49 KB, patch)
2019-09-16 20:26 UTC, Liz Rea
Details | Diff | Splinter Review
Bug 22284: (follow-up) Squash multiple follow-ups (66.04 KB, patch)
2019-09-16 20:26 UTC, Liz Rea
Details | Diff | Splinter Review
Bug 22284: (Do not push) Schema update (1.59 KB, patch)
2019-09-16 20:26 UTC, Liz Rea
Details | Diff | Splinter Review
Bug 22284: New ft_local_hold_group column, and 'holgroup' enum option (3.98 KB, patch)
2019-10-21 18:02 UTC, Agustín Moyano
Details | Diff | Splinter Review
Bug 22284: Add new checkbox in Library Groups (8.62 KB, patch)
2019-10-21 18:03 UTC, Agustín Moyano
Details | Diff | Splinter Review
Bug 22284: New hold group options in smart-rules (5.94 KB, patch)
2019-10-21 18:04 UTC, Agustín Moyano
Details | Diff | Splinter Review
Bug 22284: Control hold groups in C4::Reserves (22.88 KB, patch)
2019-10-21 18:04 UTC, Agustín Moyano
Details | Diff | Splinter Review
Bug 22284: New methods in Koha::Library::Groups and Koha::Library (7.67 KB, patch)
2019-10-21 18:05 UTC, Agustín Moyano
Details | Diff | Splinter Review
Bug 22284: Opac pickup_locations (44.80 KB, patch)
2019-10-21 18:06 UTC, Agustín Moyano
Details | Diff | Splinter Review
Bug 22284: (follow-up) Default to circulation control branch when no hold libraries are found (1.04 KB, patch)
2019-10-21 18:06 UTC, Agustín Moyano
Details | Diff | Splinter Review
Bug 22284: New message, new column and filter pickup locations in reserve/request.tt (7.15 KB, patch)
2019-10-21 18:07 UTC, Agustín Moyano
Details | Diff | Splinter Review
Bug 22284: Filter item's pickup locations in holds_table.inc (1.98 KB, patch)
2019-10-21 18:08 UTC, Agustín Moyano
Details | Diff | Splinter Review
Bug 22284: (follow-up) Squash multiple follow-ups (50.36 KB, patch)
2019-10-21 18:08 UTC, Agustín Moyano
Details | Diff | Splinter Review
Bug 22284: Add "patron's hold group" as new hold_fulfillment_policy option (7.45 KB, patch)
2019-10-21 18:09 UTC, Agustín Moyano
Details | Diff | Splinter Review
Bug 22284: (follow-up) Squash multiple follow-ups (66.54 KB, patch)
2019-10-21 18:10 UTC, Agustín Moyano
Details | Diff | Splinter Review
Bug 22284: (Do not push) Schema update (1.59 KB, patch)
2019-10-21 18:11 UTC, Agustín Moyano
Details | Diff | Splinter Review
Bug 22284: New ft_local_hold_group column, and 'holgroup' enum option (3.98 KB, patch)
2019-12-18 17:56 UTC, Agustín Moyano
Details | Diff | Splinter Review
Bug 22284: Add new checkbox in Library Groups (8.62 KB, patch)
2019-12-18 17:57 UTC, Agustín Moyano
Details | Diff | Splinter Review
Bug 22284: New hold group options in smart-rules (5.94 KB, patch)
2019-12-18 17:57 UTC, Agustín Moyano
Details | Diff | Splinter Review
Bug 22284: Control hold groups in C4::Reserves (23.04 KB, patch)
2019-12-18 17:57 UTC, Agustín Moyano
Details | Diff | Splinter Review
Bug 22284: New methods in Koha::Library::Groups and Koha::Library (7.63 KB, patch)
2019-12-18 17:57 UTC, Agustín Moyano
Details | Diff | Splinter Review
Bug 22284: Opac pickup_locations (44.65 KB, patch)
2019-12-18 17:57 UTC, Agustín Moyano
Details | Diff | Splinter Review
Bug 22284: (follow-up) Default to circulation control branch when no hold libraries are found (1.04 KB, patch)
2019-12-18 17:58 UTC, Agustín Moyano
Details | Diff | Splinter Review
Bug 22284: New message, new column and filter pickup locations in reserve/request.tt (7.26 KB, patch)
2019-12-18 17:58 UTC, Agustín Moyano
Details | Diff | Splinter Review
Bug 22284: Filter item's pickup locations in holds_table.inc (1.98 KB, patch)
2019-12-18 17:58 UTC, Agustín Moyano
Details | Diff | Splinter Review
Bug 22284: (follow-up) Squash multiple follow-ups (50.36 KB, patch)
2019-12-18 17:58 UTC, Agustín Moyano
Details | Diff | Splinter Review
Bug 22284: Add "patron's hold group" as new hold_fulfillment_policy option (7.45 KB, patch)
2019-12-18 17:58 UTC, Agustín Moyano
Details | Diff | Splinter Review
Bug 22284: (follow-up) Squash multiple follow-ups (66.54 KB, patch)
2019-12-18 17:58 UTC, Agustín Moyano
Details | Diff | Splinter Review
Bug 22284: (Do not push) Schema update (1.59 KB, patch)
2019-12-18 17:59 UTC, Agustín Moyano
Details | Diff | Splinter Review
Bug 22284: (follow-up) Anotate boolean values in Koha::Schema::Result::LibraryGroup (888 bytes, patch)
2019-12-18 17:59 UTC, Agustín Moyano
Details | Diff | Splinter Review
Bug 22284: New ft_local_hold_group column, and 'holgroup' enum option (3.98 KB, patch)
2019-12-21 10:53 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 22284: Add new checkbox in Library Groups (8.62 KB, patch)
2019-12-21 10:53 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 22284: New hold group options in smart-rules (5.94 KB, patch)
2019-12-21 10:53 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 22284: Control hold groups in C4::Reserves (23.04 KB, patch)
2019-12-21 10:53 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 22284: New methods in Koha::Library::Groups and Koha::Library (7.63 KB, patch)
2019-12-21 10:53 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 22284: Opac pickup_locations (44.65 KB, patch)
2019-12-21 10:54 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 22284: (follow-up) Default to circulation control branch when no hold libraries are found (1.09 KB, patch)
2019-12-21 10:54 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 22284: New message, new column and filter pickup locations in reserve/request.tt (7.26 KB, patch)
2019-12-21 10:54 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 22284: Filter item's pickup locations in holds_table.inc (2.04 KB, patch)
2019-12-21 10:54 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 22284: (follow-up) Squash multiple follow-ups (50.00 KB, patch)
2019-12-21 10:54 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 22284: Add "patron's hold group" as new hold_fulfillment_policy option (7.51 KB, patch)
2019-12-21 10:54 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 22284: (follow-up) Squash multiple follow-ups (66.35 KB, patch)
2019-12-21 10:54 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 22284: Schema update (1.63 KB, patch)
2019-12-21 10:54 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 22284: (follow-up) Anotate boolean values in Koha::Schema::Result::LibraryGroup (943 bytes, patch)
2019-12-21 10:54 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 22284: (QA follow-up) Make pickup locations be Koha::Library objects (48.10 KB, patch)
2019-12-21 10:55 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Kyle M Hall 2019-02-07 08:00:43 UTC
hiding libraries outside group for holds dev
Comment 1 Katrin Fischer 2019-02-07 08:35:19 UTC
I know you probably just wanted the bug number, but to avoid duplicates and because I am curious... can we get a bit more description? :)
Comment 2 Tomás Cohen Arazi 2019-03-02 12:50:35 UTC
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)
Comment 3 Katrin Fischer 2019-04-08 00:46:35 UTC
Thx, Tomas!
Comment 4 Tomás Cohen Arazi 2019-04-15 23:05:07 UTC
*** Bug 20747 has been marked as a duplicate of this bug. ***
Comment 5 Agustín Moyano 2019-04-17 06:24:28 UTC Comment hidden (obsolete)
Comment 6 Agustín Moyano 2019-04-17 06:24:33 UTC Comment hidden (obsolete)
Comment 7 Agustín Moyano 2019-04-17 06:24:36 UTC Comment hidden (obsolete)
Comment 8 Agustín Moyano 2019-04-17 06:24:40 UTC Comment hidden (obsolete)
Comment 9 Agustín Moyano 2019-04-17 06:24:44 UTC Comment hidden (obsolete)
Comment 10 Agustín Moyano 2019-04-17 06:24:48 UTC Comment hidden (obsolete)
Comment 11 Agustín Moyano 2019-04-17 06:24:52 UTC Comment hidden (obsolete)
Comment 12 Agustín Moyano 2019-04-17 06:24:56 UTC Comment hidden (obsolete)
Comment 13 Kyle M Hall 2019-04-19 13:12:18 UTC
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.
Comment 14 Agustín Moyano 2019-04-20 20:13:58 UTC Comment hidden (obsolete)
Comment 15 Agustín Moyano 2019-04-20 20:19:50 UTC
Hello Kyle,

Unused "use" sentences removed and clarified the variable.

Sorry about that.

Thanks.
Comment 16 Josef Moravec 2019-04-23 08:36:15 UTC
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 17 Josef Moravec 2019-04-23 08:37:08 UTC
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
Comment 18 Agustín Moyano 2019-04-23 19:24:56 UTC
Created attachment 88561 [details] [review]
Bug 22284: (follow-up) Remove commented warn and address test failures
Comment 19 Agustín Moyano 2019-04-23 22:10:53 UTC
(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]
Comment 20 Agustín Moyano 2019-04-25 16:30:52 UTC
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
Comment 21 Agustín Moyano 2019-04-25 16:30:55 UTC
Created attachment 88794 [details] [review]
Bug 22284: (follow-up) Add schema modifications

Sponsored-by: VOKAL
Comment 22 Agustín Moyano 2019-04-25 16:30:59 UTC
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
Comment 23 Agustín Moyano 2019-04-25 16:31:02 UTC
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
Comment 24 Agustín Moyano 2019-04-25 16:31:06 UTC
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
Comment 25 Agustín Moyano 2019-04-25 16:31:09 UTC
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
Comment 26 Agustín Moyano 2019-04-25 16:31:13 UTC
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
Comment 27 Agustín Moyano 2019-04-25 16:31:17 UTC
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
Comment 28 Agustín Moyano 2019-04-25 16:31:20 UTC
Created attachment 88801 [details] [review]
Bug 22284: (follow-up) Remove commented warn and address test failures
Comment 29 Agustín Moyano 2019-04-25 16:31:24 UTC
Created attachment 88802 [details] [review]
Bug 22284: (follow-up) fix test count after merge
Comment 30 Agustín Moyano 2019-04-25 16:32:57 UTC
(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.
Comment 31 Liz Rea 2019-04-25 21:27:33 UTC
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.
Comment 32 Agustín Moyano 2019-04-25 21:55:30 UTC
(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
Comment 33 Liz Rea 2019-04-26 15:12:00 UTC
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!
Comment 34 Liz Rea 2019-04-26 15:48:02 UTC
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
Comment 35 Tomás Cohen Arazi 2019-04-26 17:23:14 UTC
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>
Comment 36 Tomás Cohen Arazi 2019-04-26 17:23:18 UTC
Created attachment 88940 [details] [review]
Bug 22284: (follow-up) Add schema modifications

Sponsored-by: VOKAL
Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 37 Tomás Cohen Arazi 2019-04-26 17:23:22 UTC
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>
Comment 38 Tomás Cohen Arazi 2019-04-26 17:23:26 UTC
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>
Comment 39 Tomás Cohen Arazi 2019-04-26 17:23:29 UTC
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>
Comment 40 Tomás Cohen Arazi 2019-04-26 17:23:33 UTC
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>
Comment 41 Tomás Cohen Arazi 2019-04-26 17:23:37 UTC
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>
Comment 42 Tomás Cohen Arazi 2019-04-26 17:23:41 UTC
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>
Comment 43 Tomás Cohen Arazi 2019-04-26 17:23:44 UTC
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>
Comment 44 Tomás Cohen Arazi 2019-04-26 17:23:48 UTC
Created attachment 88948 [details] [review]
Bug 22284: (follow-up) fix test count after merge

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 45 Agustín Moyano 2019-04-26 17:27:22 UTC
New dependency on bug 15496 only needs to apply last patch

88938 - Bug 15496: (QA follow-up) Fix remaining cases
Comment 46 Agustín Moyano 2019-04-26 17:34:02 UTC
Created attachment 88949 [details] [review]
Bug 22284: (follow-up) fixes after 15496
Comment 47 Agustín Moyano 2019-04-26 19:16:48 UTC
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>
Comment 48 Agustín Moyano 2019-04-26 19:16:51 UTC
Created attachment 88956 [details] [review]
Bug 22284: (follow-up) Add schema modifications

Sponsored-by: VOKAL
Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 49 Agustín Moyano 2019-04-26 19:16:55 UTC
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>
Comment 50 Agustín Moyano 2019-04-26 19:16:59 UTC
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>
Comment 51 Agustín Moyano 2019-04-26 19:17:03 UTC
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>
Comment 52 Agustín Moyano 2019-04-26 19:17:07 UTC
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>
Comment 53 Agustín Moyano 2019-04-26 19:17:11 UTC
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>
Comment 54 Agustín Moyano 2019-04-26 19:17:15 UTC
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>
Comment 55 Agustín Moyano 2019-04-26 19:17:18 UTC
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>
Comment 56 Agustín Moyano 2019-04-26 19:17:22 UTC
Created attachment 88964 [details] [review]
Bug 22284: (follow-up) fix test count after merge

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 57 Agustín Moyano 2019-04-26 19:17:26 UTC
Created attachment 88965 [details] [review]
Bug 22284: (follow-up) fixes after 15496
Comment 58 Jonathan Druart 2019-04-29 14:31:13 UTC
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.
Comment 59 Agustín Moyano 2019-04-29 15:14:56 UTC
(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.
Comment 60 Agustín Moyano 2019-04-30 02:44:25 UTC
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>
Comment 61 Agustín Moyano 2019-04-30 02:44:30 UTC
Created attachment 89061 [details] [review]
Bug 22284: (follow-up) Add schema modifications

Sponsored-by: VOKAL
Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 62 Agustín Moyano 2019-04-30 02:44:35 UTC
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>
Comment 63 Agustín Moyano 2019-04-30 02:44:42 UTC
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>
Comment 64 Agustín Moyano 2019-04-30 02:44:48 UTC
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>
Comment 65 Agustín Moyano 2019-04-30 02:44:57 UTC
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>
Comment 66 Agustín Moyano 2019-04-30 02:45:04 UTC
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>
Comment 67 Agustín Moyano 2019-04-30 02:45:10 UTC
Created attachment 89067 [details] [review]
Bug 22284: (follow-up) Default to circulation control branch when no hold libraries are found
Comment 68 Agustín Moyano 2019-04-30 02:45:16 UTC
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>
Comment 69 Agustín Moyano 2019-04-30 02:45:24 UTC
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>
Comment 70 Agustín Moyano 2019-04-30 02:45:31 UTC
Created attachment 89070 [details] [review]
Bug 22284: (follow-up) fix test count after merge

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 71 Agustín Moyano 2019-04-30 02:45:39 UTC
Created attachment 89071 [details] [review]
Bug 22284: (follow-up) fixes after 15496
Comment 72 Agustín Moyano 2019-04-30 05:07:16 UTC
Created attachment 89072 [details] [review]
Bug 22284: (follow-up) fixes after 18936
Comment 73 Agustín Moyano 2019-04-30 05:13:30 UTC
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>
Comment 74 Agustín Moyano 2019-04-30 05:13:37 UTC
Created attachment 89074 [details] [review]
Bug 22284: (follow-up) Add schema modifications

Sponsored-by: VOKAL
Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 75 Agustín Moyano 2019-04-30 05:13:43 UTC
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>
Comment 76 Agustín Moyano 2019-04-30 05:13:49 UTC
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>
Comment 77 Agustín Moyano 2019-04-30 05:13:56 UTC
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>
Comment 78 Agustín Moyano 2019-04-30 05:14:04 UTC
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>
Comment 79 Agustín Moyano 2019-04-30 05:14:17 UTC
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>
Comment 80 Agustín Moyano 2019-04-30 05:14:23 UTC
Created attachment 89080 [details] [review]
Bug 22284: (follow-up) Default to circulation control branch when no hold libraries are found
Comment 81 Agustín Moyano 2019-04-30 05:14:33 UTC
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>
Comment 82 Agustín Moyano 2019-04-30 05:14:38 UTC
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>
Comment 83 Agustín Moyano 2019-04-30 05:14:42 UTC
Created attachment 89083 [details] [review]
Bug 22284: (follow-up) fix test count after merge

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 84 Agustín Moyano 2019-04-30 05:14:47 UTC
Created attachment 89084 [details] [review]
Bug 22284: (follow-up) fixes after 15496
Comment 85 Agustín Moyano 2019-04-30 05:14:52 UTC
Created attachment 89085 [details] [review]
Bug 22284: (follow-up) fixes after 18936
Comment 86 Agustín Moyano 2019-04-30 14:58:50 UTC
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>
Comment 87 Agustín Moyano 2019-04-30 14:58:56 UTC
Created attachment 89123 [details] [review]
Bug 22284: (follow-up) Add schema modifications

Sponsored-by: VOKAL
Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 88 Agustín Moyano 2019-04-30 14:59:02 UTC
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>
Comment 89 Agustín Moyano 2019-04-30 14:59:07 UTC
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>
Comment 90 Agustín Moyano 2019-04-30 14:59:13 UTC
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>
Comment 91 Agustín Moyano 2019-04-30 14:59:20 UTC
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>
Comment 92 Agustín Moyano 2019-04-30 14:59:27 UTC
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>
Comment 93 Agustín Moyano 2019-04-30 14:59:33 UTC
Created attachment 89129 [details] [review]
Bug 22284: (follow-up) Default to circulation control branch when no hold libraries are found
Comment 94 Agustín Moyano 2019-04-30 14:59:39 UTC
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>
Comment 95 Agustín Moyano 2019-04-30 14:59:45 UTC
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
Comment 96 Agustín Moyano 2019-04-30 14:59:51 UTC
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>
Comment 97 Agustín Moyano 2019-04-30 14:59:57 UTC
Created attachment 89133 [details] [review]
Bug 22284: (follow-up) fix test count after merge

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 98 Agustín Moyano 2019-04-30 15:00:04 UTC
Created attachment 89134 [details] [review]
Bug 22284: (follow-up) fixes after 15496
Comment 99 Agustín Moyano 2019-04-30 15:00:11 UTC
Created attachment 89135 [details] [review]
Bug 22284: (follow-up) fixes after 18936
Comment 100 Agustín Moyano 2019-04-30 21:42:49 UTC
(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
Comment 101 Liz Rea 2019-04-30 22:57:47 UTC
i'll retest this tomorrow I think
Comment 102 Liz Rea 2019-05-09 14:53:47 UTC
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
Comment 103 Liz Rea 2019-05-09 19:28:09 UTC
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
Comment 104 Liz Rea 2019-05-09 19:29:06 UTC
Created attachment 89523 [details]
MARC records for testing, compatible with default branches
Comment 105 Agustín Moyano 2019-05-09 22:48:57 UTC
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
Comment 106 Liz Rea 2019-05-10 14:29:12 UTC
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
Comment 107 Agustín Moyano 2019-05-10 14:38:06 UTC
(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
Comment 108 Tomás Cohen Arazi 2019-05-10 15:50:34 UTC
(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.
Comment 109 Liz Rea 2019-05-11 01:38:21 UTC
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
Comment 110 Tomás Cohen Arazi 2019-05-14 20:10:33 UTC
(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.
Comment 111 Agustín Moyano 2019-05-16 14:18:13 UTC
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>
Comment 112 Agustín Moyano 2019-05-16 14:18:21 UTC
Created attachment 89837 [details] [review]
Bug 22284: (follow-up) Add schema modifications

Sponsored-by: VOKAL
Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 113 Agustín Moyano 2019-05-16 14:18:29 UTC
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>
Comment 114 Agustín Moyano 2019-05-16 14:18:37 UTC
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>
Comment 115 Agustín Moyano 2019-05-16 14:18:46 UTC
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>
Comment 116 Agustín Moyano 2019-05-16 14:18:52 UTC
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>
Comment 117 Agustín Moyano 2019-05-16 14:18:58 UTC
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>
Comment 118 Agustín Moyano 2019-05-16 14:19:05 UTC
Created attachment 89843 [details] [review]
Bug 22284: (follow-up) Default to circulation control branch when no hold libraries are found
Comment 119 Agustín Moyano 2019-05-16 14:19:11 UTC
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>
Comment 120 Agustín Moyano 2019-05-16 14:19:17 UTC
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
Comment 121 Agustín Moyano 2019-05-16 14:19:25 UTC
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>
Comment 122 Agustín Moyano 2019-05-16 14:19:32 UTC
Created attachment 89847 [details] [review]
Bug 22284: (follow-up) fix test count after merge

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 123 Agustín Moyano 2019-05-16 14:19:39 UTC
Created attachment 89848 [details] [review]
Bug 22284: (follow-up) fixes after 15496
Comment 124 Agustín Moyano 2019-05-16 14:19:45 UTC
Created attachment 89849 [details] [review]
Bug 22284: (follow-up) fixes after 18936
Comment 125 Liz Rea 2019-05-16 17:39:14 UTC
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
Comment 126 Agustín Moyano 2019-05-17 19:42:36 UTC
(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
Comment 127 Tomás Cohen Arazi 2019-05-21 13:42:55 UTC
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).
Comment 128 Agustín Moyano 2019-05-28 04:53:29 UTC
Created attachment 90133 [details] [review]
Bug 22284: (follow-up) Remove HomeOrHolding from reserves
Comment 129 Agustín Moyano 2019-05-28 04:53:34 UTC
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
Comment 130 Agustín Moyano 2019-05-28 04:54:15 UTC
Test will be fixed in next follow-up
Comment 131 Agustín Moyano 2019-05-30 03:22:03 UTC
Created attachment 90197 [details] [review]
Bug 22284: (follow-up) Use GetReserveControlBranch in Koha::Item->pickup_locations
Comment 132 Agustín Moyano 2019-05-30 03:22:11 UTC
Created attachment 90198 [details] [review]
Bug 22284: (follow-up) Fix tests
Comment 133 Agustín Moyano 2019-05-30 03:40:41 UTC
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
Comment 134 Agustín Moyano 2019-05-30 03:40:51 UTC
Created attachment 90200 [details] [review]
Bug 22284: (follow-up) Use GetReserveControlBranch in Koha::Item->pickup_locations
Comment 135 Agustín Moyano 2019-05-30 03:40:59 UTC
Created attachment 90201 [details] [review]
Bug 22284: (follow-up) Fix tests
Comment 136 Agustín Moyano 2019-05-30 22:25:37 UTC
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!
Comment 137 Liz Rea 2019-06-08 03:18:22 UTC
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
Comment 138 Agustín Moyano 2019-06-11 03:05:21 UTC
Created attachment 90469 [details] [review]
Bug 22284: (follow-up) Fix typo in request.tt
Comment 139 Agustín Moyano 2019-06-12 03:26:28 UTC
Created attachment 90525 [details] [review]
Bug 22284: (follow-up) Filter pickup on specific item click
Comment 140 Agustín Moyano 2019-06-12 03:31:17 UTC
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
Comment 141 Josef Moravec 2019-07-04 05:27:19 UTC
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>
Comment 142 Josef Moravec 2019-07-04 05:27:25 UTC
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>
Comment 143 Josef Moravec 2019-07-04 05:27:31 UTC
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>
Comment 144 Josef Moravec 2019-07-04 05:27:37 UTC
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>
Comment 145 Josef Moravec 2019-07-04 05:27:43 UTC
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>
Comment 146 Josef Moravec 2019-07-04 05:27:49 UTC
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>
Comment 147 Josef Moravec 2019-07-04 05:27:54 UTC
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>
Comment 148 Josef Moravec 2019-07-04 05:28:00 UTC
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>
Comment 149 Josef Moravec 2019-07-04 05:28:05 UTC
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>
Comment 150 Josef Moravec 2019-07-04 05:28:15 UTC
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>
Comment 151 Josef Moravec 2019-07-04 05:28:22 UTC
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>
Comment 152 Josef Moravec 2019-07-04 05:28:28 UTC
Created attachment 91272 [details] [review]
Bug 22284: (follow-up) fixes after 15496

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Comment 153 Josef Moravec 2019-07-04 05:28:34 UTC
Created attachment 91273 [details] [review]
Bug 22284: (follow-up) fixes after 18936

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Comment 154 Josef Moravec 2019-07-04 05:28:41 UTC
Created attachment 91274 [details] [review]
Bug 22284: (follow-up) Remove HomeOrHolding from reserves

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Comment 155 Josef Moravec 2019-07-04 05:28:48 UTC
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>
Comment 156 Josef Moravec 2019-07-04 05:28:57 UTC
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>
Comment 157 Josef Moravec 2019-07-04 05:29:05 UTC
Created attachment 91277 [details] [review]
Bug 22284: (follow-up) Fix tests

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Comment 158 Josef Moravec 2019-07-04 05:29:12 UTC
Created attachment 91278 [details] [review]
Bug 22284: (follow-up) Fix typo in request.tt

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Comment 159 Josef Moravec 2019-07-04 05:29:19 UTC
Created attachment 91279 [details] [review]
Bug 22284: (follow-up) Filter pickup on specific item click

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Comment 160 Josef Moravec 2019-07-04 05:29:25 UTC
Created attachment 91280 [details] [review]
Bug 22284: (Do not push) Schema update

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Comment 161 Josef Moravec 2019-07-04 05:32:35 UTC
Created attachment 91282 [details] [review]
Bug 22284: (follow-up) Fix typos transfered -> transferred

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Comment 162 Liz Rea 2019-07-16 16:21:40 UTC
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
Comment 163 Agustín Moyano 2019-07-22 18:41:01 UTC
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>
Comment 164 Agustín Moyano 2019-07-22 18:41:06 UTC
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>
Comment 165 Agustín Moyano 2019-07-22 18:41:11 UTC
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>
Comment 166 Agustín Moyano 2019-07-22 18:41:17 UTC
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>
Comment 167 Agustín Moyano 2019-07-22 18:41:22 UTC
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>
Comment 168 Agustín Moyano 2019-07-22 18:41:28 UTC
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>
Comment 169 Agustín Moyano 2019-07-22 18:41:33 UTC
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>
Comment 170 Agustín Moyano 2019-07-22 18:41:38 UTC
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>
Comment 171 Agustín Moyano 2019-07-22 18:41:43 UTC
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>
Comment 172 Agustín Moyano 2019-07-22 18:41:49 UTC
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>
Comment 173 Agustín Moyano 2019-07-22 18:41:55 UTC
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>
Comment 174 Agustín Moyano 2019-07-22 18:42:00 UTC
Created attachment 91712 [details] [review]
Bug 22284: (follow-up) fixes after 15496

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Comment 175 Agustín Moyano 2019-07-22 18:42:06 UTC
Created attachment 91713 [details] [review]
Bug 22284: (follow-up) fixes after 18936

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Comment 176 Agustín Moyano 2019-07-22 18:42:12 UTC
Created attachment 91714 [details] [review]
Bug 22284: (follow-up) Remove HomeOrHolding from reserves

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Comment 177 Agustín Moyano 2019-07-22 18:42:17 UTC
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>
Comment 178 Agustín Moyano 2019-07-22 18:42:22 UTC
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>
Comment 179 Agustín Moyano 2019-07-22 18:42:28 UTC
Created attachment 91717 [details] [review]
Bug 22284: (follow-up) Fix tests

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Comment 180 Agustín Moyano 2019-07-22 18:42:33 UTC
Created attachment 91718 [details] [review]
Bug 22284: (follow-up) Fix typo in request.tt

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Comment 181 Agustín Moyano 2019-07-22 18:42:38 UTC
Created attachment 91719 [details] [review]
Bug 22284: (follow-up) Filter pickup on specific item click

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Comment 182 Agustín Moyano 2019-07-22 18:42:43 UTC
Created attachment 91720 [details] [review]
Bug 22284: (Do not push) Schema update

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Comment 183 Agustín Moyano 2019-07-22 18:42:48 UTC
Created attachment 91721 [details] [review]
Bug 22284: (follow-up) Fix typos transfered -> transferred

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Comment 184 Agustín Moyano 2019-07-23 17:18:55 UTC
Created attachment 91750 [details] [review]
Bug 22284: (follow-up) fixes after 18936

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Comment 185 Agustín Moyano 2019-07-23 17:19:05 UTC
Created attachment 91751 [details] [review]
Bug 22284: (follow-up) Remove HomeOrHolding from reserves

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Comment 186 Agustín Moyano 2019-07-23 17:19:19 UTC
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>
Comment 187 Agustín Moyano 2019-07-23 17:19:28 UTC
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>
Comment 188 Agustín Moyano 2019-07-23 17:19:37 UTC
Created attachment 91754 [details] [review]
Bug 22284: (follow-up) Fix tests

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Comment 189 Agustín Moyano 2019-07-23 17:19:46 UTC
Created attachment 91755 [details] [review]
Bug 22284: (follow-up) Fix typo in request.tt

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Comment 190 Agustín Moyano 2019-07-23 17:19:52 UTC
Created attachment 91756 [details] [review]
Bug 22284: (follow-up) Filter pickup on specific item click

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Comment 191 Agustín Moyano 2019-07-23 17:20:01 UTC
Created attachment 91757 [details] [review]
Bug 22284: (Do not push) Schema update

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Comment 192 Agustín Moyano 2019-07-23 17:20:16 UTC
Created attachment 91758 [details] [review]
Bug 22284: (follow-up) Fix typos transfered -> transferred

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Comment 193 Liz Rea 2019-08-30 17:31:12 UTC
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
Comment 194 Agustín Moyano 2019-09-03 14:42:43 UTC
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>
Comment 195 Agustín Moyano 2019-09-03 14:42:59 UTC
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>
Comment 196 Agustín Moyano 2019-09-03 14:43:12 UTC
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>
Comment 197 Agustín Moyano 2019-09-03 14:43:27 UTC
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>
Comment 198 Agustín Moyano 2019-09-03 14:43:45 UTC
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>
Comment 199 Agustín Moyano 2019-09-03 14:43:59 UTC
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>
Comment 200 Agustín Moyano 2019-09-03 14:44:12 UTC
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>
Comment 201 Agustín Moyano 2019-09-03 14:44:27 UTC
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>
Comment 202 Agustín Moyano 2019-09-03 14:44:44 UTC
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>
Comment 203 Agustín Moyano 2019-09-03 14:44:51 UTC
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>
Comment 204 Agustín Moyano 2019-09-03 14:45:04 UTC
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>
Comment 205 Agustín Moyano 2019-09-03 14:45:21 UTC
Created attachment 92565 [details] [review]
Bug 22284: (follow-up) fixes after 15496

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Comment 206 Agustín Moyano 2019-09-03 14:45:31 UTC
Created attachment 92566 [details] [review]
Bug 22284: (follow-up) fixes after 18936

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Comment 207 Agustín Moyano 2019-09-03 14:45:44 UTC
Created attachment 92567 [details] [review]
Bug 22284: (follow-up) Remove HomeOrHolding from reserves

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Comment 208 Agustín Moyano 2019-09-03 14:45:57 UTC
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>
Comment 209 Agustín Moyano 2019-09-03 14:46:09 UTC
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>
Comment 210 Agustín Moyano 2019-09-03 14:46:20 UTC
Created attachment 92570 [details] [review]
Bug 22284: (follow-up) Fix tests

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Comment 211 Agustín Moyano 2019-09-03 14:46:31 UTC
Created attachment 92571 [details] [review]
Bug 22284: (follow-up) Fix typo in request.tt

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Comment 212 Agustín Moyano 2019-09-03 14:46:42 UTC
Created attachment 92572 [details] [review]
Bug 22284: (follow-up) Filter pickup on specific item click

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Comment 213 Agustín Moyano 2019-09-03 14:46:57 UTC
Created attachment 92573 [details] [review]
Bug 22284: (Do not push) Schema update

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Comment 214 Agustín Moyano 2019-09-03 14:47:07 UTC
Created attachment 92574 [details] [review]
Bug 22284: (follow-up) Fix typos transfered -> transferred

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Comment 215 Martin Renvoize 2019-09-12 16:08:04 UTC
Sponsored-by line is 'VOKAL'.. do they have an expanded name or is that it?
Comment 216 Martin Renvoize 2019-09-12 16:09:04 UTC
(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?
Comment 217 Agustín Moyano 2019-09-12 17:58:01 UTC
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>
Comment 218 Agustín Moyano 2019-09-12 17:58:19 UTC
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>
Comment 219 Agustín Moyano 2019-09-12 17:58:39 UTC
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>
Comment 220 Agustín Moyano 2019-09-12 17:58:58 UTC
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>
Comment 221 Agustín Moyano 2019-09-12 17:59:12 UTC
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>
Comment 222 Agustín Moyano 2019-09-12 17:59:22 UTC
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>
Comment 223 Agustín Moyano 2019-09-12 17:59:33 UTC
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>
Comment 224 Agustín Moyano 2019-09-12 17:59:45 UTC
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>
Comment 225 Agustín Moyano 2019-09-12 17:59:59 UTC
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>
Comment 226 Agustín Moyano 2019-09-12 18:00:20 UTC
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>
Comment 227 Agustín Moyano 2019-09-12 18:00:33 UTC
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>
Comment 228 Agustín Moyano 2019-09-12 18:00:48 UTC
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>
Comment 229 Agustín Moyano 2019-09-12 18:01:07 UTC
Created attachment 92764 [details] [review]
Bug 22284: (Do not push) Schema update

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Comment 230 Liz Rea 2019-09-16 20:25:36 UTC
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>
Comment 231 Liz Rea 2019-09-16 20:25:49 UTC
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>
Comment 232 Liz Rea 2019-09-16 20:25:55 UTC
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>
Comment 233 Liz Rea 2019-09-16 20:26:00 UTC
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>
Comment 234 Liz Rea 2019-09-16 20:26:06 UTC
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>
Comment 235 Liz Rea 2019-09-16 20:26:11 UTC
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>
Comment 236 Liz Rea 2019-09-16 20:26:17 UTC
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>
Comment 237 Liz Rea 2019-09-16 20:26:23 UTC
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>
Comment 238 Liz Rea 2019-09-16 20:26:28 UTC
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>
Comment 239 Liz Rea 2019-09-16 20:26:33 UTC
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>
Comment 240 Liz Rea 2019-09-16 20:26:38 UTC
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>
Comment 241 Liz Rea 2019-09-16 20:26:44 UTC
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>
Comment 242 Liz Rea 2019-09-16 20:26:49 UTC
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>
Comment 243 Katrin Fischer 2019-10-07 23:04:44 UTC
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
Comment 244 Agustín Moyano 2019-10-21 18:02:15 UTC
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>
Comment 245 Agustín Moyano 2019-10-21 18:03:02 UTC
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>
Comment 246 Agustín Moyano 2019-10-21 18:04:08 UTC
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>
Comment 247 Agustín Moyano 2019-10-21 18:04:58 UTC
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>
Comment 248 Agustín Moyano 2019-10-21 18:05:43 UTC
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>
Comment 249 Agustín Moyano 2019-10-21 18:06:28 UTC
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>
Comment 250 Agustín Moyano 2019-10-21 18:06:55 UTC
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>
Comment 251 Agustín Moyano 2019-10-21 18:07:30 UTC
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>
Comment 252 Agustín Moyano 2019-10-21 18:08:16 UTC
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>
Comment 253 Agustín Moyano 2019-10-21 18:08:57 UTC
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>
Comment 254 Agustín Moyano 2019-10-21 18:09:45 UTC
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>
Comment 255 Agustín Moyano 2019-10-21 18:10:28 UTC
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>
Comment 256 Agustín Moyano 2019-10-21 18:11:20 UTC
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>
Comment 257 Agustín Moyano 2019-10-21 18:23:04 UTC
(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!
Comment 258 Agustín Moyano 2019-12-18 17:56:53 UTC
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>
Comment 259 Agustín Moyano 2019-12-18 17:57:05 UTC
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>
Comment 260 Agustín Moyano 2019-12-18 17:57:17 UTC
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>
Comment 261 Agustín Moyano 2019-12-18 17:57:30 UTC
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>
Comment 262 Agustín Moyano 2019-12-18 17:57:42 UTC
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>
Comment 263 Agustín Moyano 2019-12-18 17:57:53 UTC
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>
Comment 264 Agustín Moyano 2019-12-18 17:58:03 UTC
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>
Comment 265 Agustín Moyano 2019-12-18 17:58:17 UTC
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>
Comment 266 Agustín Moyano 2019-12-18 17:58:26 UTC
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>
Comment 267 Agustín Moyano 2019-12-18 17:58:35 UTC
Created attachment 96446 [details] [review]
Bug 22284: (follow-up) Squash multiple follow-ups

  * Bug 22284: (follow-up) Remove commented warn and address test failures

    Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>

    Signed-off-by: Josef Moravec <josef.moravec@gmail.com>

  * Bug 22284: (follow-up) fix test count after merge

    Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>

    Signed-off-by: Josef Moravec <josef.moravec@gmail.com>

  * Bug 22284: (follow-up) fixes after 15496

    Signed-off-by: Josef Moravec <josef.moravec@gmail.com>

  * Bug 22284: (follow-up) fixes after 18936

    Signed-off-by: Josef Moravec <josef.moravec@gmail.com>

  * Bug 22284: (follow-up) Remove HomeOrHolding from reserves

    Signed-off-by: Josef Moravec <josef.moravec@gmail.com>

Signed-off-by: Liz Rea <wizzyrea@gmail.com>
Comment 268 Agustín Moyano 2019-12-18 17:58:45 UTC
Created attachment 96447 [details] [review]
Bug 22284: Add "patron's hold group" as new hold_fulfillment_policy option

This patch adds "patron's hold group" as a new option to Hold pickup library match

To test:
1. Set ReservesControlBranch preference to item.
2. Create a hold group
3. Go to circulation and fines rules
SUCCESS => in 'Hold pickup library match' there is a new option called "patron's hold group"
4. In a library not in hold group set 'Hold policy' to "any" and 'Hold pickup library match' to "patron's hold group"
5. Search for a user in the hold group
6. 'Search to hold' for items of the library of step 4
7. Select an item and 'Place hold for [user]'
SUCCESS => in 'Pickup at' you should see patron's hold group as options
8. In OPAC sign in as the same user of step 5
9. Search for the item in step 7
SUCCESS => in 'Pick up locations' you should see patron's hold group as options
10. Sign off

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>

Signed-off-by: Liz Rea <wizzyrea@gmail.com>
Comment 269 Agustín Moyano 2019-12-18 17:58:55 UTC
Created attachment 96448 [details] [review]
Bug 22284: (follow-up) Squash multiple follow-ups

  * Bug 22284: (follow-up) Use GetReserveControlBranch in Koha::Item->pickup_locations

    Signed-off-by: Josef Moravec <josef.moravec@gmail.com>

  * Bug 22284: (follow-up) Fix tests

    Signed-off-by: Josef Moravec <josef.moravec@gmail.com>

  * Bug 22284: (follow-up) Fix typo in request.tt

    Signed-off-by: Josef Moravec <josef.moravec@gmail.com>

  * Bug 22284: (follow-up) Filter pickup on specific item click

    Signed-off-by: Josef Moravec <josef.moravec@gmail.com>

  * Bug 22284: (follow-up) Fix typos transfered -> transferred

    Signed-off-by: Josef Moravec <josef.moravec@gmail.com>

Signed-off-by: Liz Rea <wizzyrea@gmail.com>
Comment 270 Agustín Moyano 2019-12-18 17:59:03 UTC
Created attachment 96449 [details] [review]
Bug 22284: (Do not push) Schema update

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>

Signed-off-by: Liz Rea <wizzyrea@gmail.com>
Comment 271 Agustín Moyano 2019-12-18 17:59:12 UTC
Created attachment 96450 [details] [review]
Bug 22284: (follow-up) Anotate boolean values in Koha::Schema::Result::LibraryGroup
Comment 272 Tomás Cohen Arazi 2019-12-21 10:53:26 UTC
Created attachment 96544 [details] [review]
Bug 22284: New ft_local_hold_group column, and 'holgroup' enum option

In order to add groups for holds, this patch provides a new column in
library_groups to identify them. Also provides new enum option for
hold_fulfillment_policy column of default_branch_circ_rules,
default_circ_rules, default_branch_item_rules and branch_item_rules.

To test:
1) check that there is no ft_local_hold_group column in library_groups
and that there are no 'holdgroup' enum option in hold_fulfillment_policy
in default_branch_circ_rules, default_circ_rules, default_branch_item_rules
and branch_item_rules
2) apply this patch
3) perl installer/data/mysql/updatedatabase.pl && dbic
SUCCESS => !(step 1) ;-D
4) Sign off

Sponsored-by: VOKAL

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Signed-off-by: Liz Rea <wizzyrea@gmail.com>
Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 273 Tomás Cohen Arazi 2019-12-21 10:53:33 UTC
Created attachment 96545 [details] [review]
Bug 22284: Add new checkbox in Library Groups

This patch adds a new checkbox option 'Is local hold group' when you create a new root group.

To test:
1) In intranet go to Library Groups in admin module
2) Add new root group
3) Check that there is no 'Is local hold group' checkbox
4) Apply this patch
5) Add new root group
SUCCESS => a new 'Is local hold group' checkbox appears
6) Select 'Is local hold group' and save
7) Click edit button for root group
SUCCESS => 'Is local hold group' is checked
8) Sign off

Sponsored-by: VOKAL

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Signed-off-by: Liz Rea <wizzyrea@gmail.com>
Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 274 Tomás Cohen Arazi 2019-12-21 10:53:40 UTC
Created attachment 96546 [details] [review]
Bug 22284: New hold group options in smart-rules

This patch adds new options for 'Default checkout, hold and return policy'
and 'Default holds policy by item type', both in 'Hold policy' and 'Hold
pickup library match' columns

To test:
1) Go to 'Circulation and fines rules' in admin
2) Check that there are no options for hold group in 'Hold policy' and 'Hold
pickup library match' columns
3) Apply this patch
SUCCESS => in 'Hold policy' you should see 'From local hold group' option, and in
'Hold pickup library match' you should see "Item's hold group"
4) Select any of those options and save
SUCCESS => options should be selected when page reloads
5) Sign off

Sponsored-by: VOKAL

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Signed-off-by: Liz Rea <wizzyrea@gmail.com>
Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 275 Tomás Cohen Arazi 2019-12-21 10:53:47 UTC
Created attachment 96547 [details] [review]
Bug 22284: Control hold groups in C4::Reserves

This patch modifies C4::Reserves to control when hold group options where selected
in smart rules.
In CanItemBeReserved adds 2 new error status messages
1) branchNotInHoldGroup: when a patron's homebranch is not in item's hold group
2) pickupNotInHoldGroup: when a selected pickup location is not in item's hold group
Also CheckReserves is modified when item's priority is defined, to control by hold
group when required.
Finally, IsAvailableForItemLevelRequest was also modified to control by hold group when
required.

To test:
1) Apply this patch
2) prove t/db_dependent/Holds.t
SUCCESS => Result: PASS
3) Sign off

Sponsored-by: VOKAL

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Signed-off-by: Liz Rea <wizzyrea@gmail.com>
Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 276 Tomás Cohen Arazi 2019-12-21 10:53:54 UTC
Created attachment 96548 [details] [review]
Bug 22284: New methods in Koha::Library::Groups and Koha::Library

This patch adds new methods in Koha::Library::Groups and Koha::Library.
1) For Koha::Library::Groups adds get_root_ancestor that returns all root
groups for a given search parameters, for example
Koha::Library::Groups->get_root_ancestor( { id => $group_id } )
2) For Koha::Library adds
2.1) get_hold_libraries: returns all libraries (including self) that
belongs to the same holdgroups. If $self belongs to several holdgroups
it will return a distinct list of all libraries belonging to them.
2.2) validate_hold_sibling: Returns 1 if the given parameters matches any of the
libraries that belong to any of the holdgroups this library belongs. For example
$library->validate_hold_sibling( { branchcode => $branchcode } )

To test:
1) apply this patch
2) prove t/db_dependent/Koha/Libraries.t t/db_dependent/LibraryGroups.t
SUCCESS => green letters :-D
3) Sign off

Sponsored-by: VOKAL

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Signed-off-by: Liz Rea <wizzyrea@gmail.com>
Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 277 Tomás Cohen Arazi 2019-12-21 10:54:01 UTC
Created attachment 96549 [details] [review]
Bug 22284: Opac pickup_locations

This patch modifies Koha::Libraries->pickup_location and moves most of the logic to
Koha::Item and Koha::Biblio in preparation for api endpoints in the future.

There where 2 methods added

1) Koha::Item->pickup_locations that given a patron, returns all pickup locations of
this item, considering hold fulfillment rules, and hold allowed rules.

2) Koha::Biblio->pickup_locations that given a patron, returns a distinct list of
libraries returned by each of this biblio items pickup location.

Koha::Libraries->pickup_location analyzes input param and calls Koha::Item->pickup_locations
or Koha::Biblio->pickup_locations as needed.

Also in opac-reserve.tt the way options where obtained to fill the pickup location select
was modified to pass the patron as a parameter.

To test:
1) opac: try to place hold on a item and check that all libraries are shown in the
pickup location select.
2) intranet: in Library groups, add 2 root groups marked as local hold group and
add different libraries to each.
3) opac: login as a user of a library belonging to one hold group, and search try to
place a hold on an item belongin to the other hold group.
4) intranet: in Circulation and fines rules, play with 'Hold policy' and 'Hold pickup
library match' rules.
5) opac: On each modification of the rules reload the page.
SUCCESS => Every time you reload the page, the number of pickup locations showed in
select varies.
6) prove t/db_dependent/Koha/Biblios.t t/db_dependent/Koha/Items.t
SUCCESS => Result: PASS
7) Sign off

Sponsored-by: VOKAL

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Signed-off-by: Liz Rea <wizzyrea@gmail.com>
Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 278 Tomás Cohen Arazi 2019-12-21 10:54:08 UTC
Created attachment 96550 [details] [review]
Bug 22284: (follow-up) Default to circulation control branch when no hold libraries are found

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Signed-off-by: Liz Rea <wizzyrea@gmail.com>
Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 279 Tomás Cohen Arazi 2019-12-21 10:54:15 UTC
Created attachment 96551 [details] [review]
Bug 22284: New message, new column and filter pickup locations in reserve/request.tt

This patch adds a new message to 'Hold' column in 'Place a hold on a specific item' table.

The message is "Cannot place hold from patrons's library". It appears when patron's homebranch is not in item's hold group, and hold_fulfillment_policy is set to 'holdgroup'.

This patch also adds a new column "Allowed pickup locations" that lists allowed pickup locations per item.

Finally, the select that displays pickup locations is filtered by allowed pickup locations, when multi_hold is not enabled

To test:
1) Apply this patch
2) In library groups add a root group and check it as hold group.
3) Add two libraries to the group
4) In circulation and fines rules, in 'Default checkout, hold and return policy', in Hold policy change the value to 'From local hold group'
5) Search a patron from a different library than step 3, select one and click 'search to hold'
6) Search by location for items in any library of step 3
7) On any item, clic on 'Place hold for ...'
SUCCESS => when the page is loaded, in the 'Place a hold on a specific item', you should see the message "Cannot place hold from patrons's library" in 'Hold' column
	=> You should see a new column called "Allowed pickup locations" and the message is "Any library"
8) In circulation and fines rules, in 'Default checkout, hold and return policy', in 'Hold policy' change the value again to 'From any library' and change 'Hold pickup library match' to "Item's hold group"
8) Repeat steps 5 to 7
SUCCESS => when the page is loaded, you should see the "Pickup at" select filtered by libraries in hold group
	=> You should see in "Allowed pickup locations" a coma separated list of the libraries in item's hold group
	=> If biblio has an item whose control branch is not in a hold group, you should see the control branch name in "Allowed pickup locations"
9) Sign off

Sponsored-by: VOKAL

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Signed-off-by: Liz Rea <wizzyrea@gmail.com>
Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 280 Tomás Cohen Arazi 2019-12-21 10:54:22 UTC
Created attachment 96552 [details] [review]
Bug 22284: Filter item's pickup locations in holds_table.inc

This patch filters pickup locations per item in holds_table.inc

To test:
1) Go to Circulation and fines rules and change max holds allowed to > 0, and change 'Hold pickup library match' to homebranch
2) Search for a patron
3) Click on "Search to hold"
4) Search for any bilio
5) Place hold on a specific item
CHECK => Item's pickup location is a select with all libraries
6) Apply this patch
7) Repeat steps 2 to 5
SUCCESS => Item's pickup location is filtered by allowed pickup locations
8) Sign off

Sponsored-by: VOKAL

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Signed-off-by: Liz Rea <wizzyrea@gmail.com>
Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 281 Tomás Cohen Arazi 2019-12-21 10:54:29 UTC
Created attachment 96553 [details] [review]
Bug 22284: (follow-up) Squash multiple follow-ups

  * Bug 22284: (follow-up) Remove commented warn and address test failures
  * Bug 22284: (follow-up) fix test count after merge
  * Bug 22284: (follow-up) fixes after 15496
  * Bug 22284: (follow-up) fixes after 18936
  * Bug 22284: (follow-up) Remove HomeOrHolding from reserves

Signed-off-by: Liz Rea <wizzyrea@gmail.com>
Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 282 Tomás Cohen Arazi 2019-12-21 10:54:36 UTC
Created attachment 96554 [details] [review]
Bug 22284: Add "patron's hold group" as new hold_fulfillment_policy option

This patch adds "patron's hold group" as a new option to Hold pickup library match

To test:
1. Set ReservesControlBranch preference to item.
2. Create a hold group
3. Go to circulation and fines rules
SUCCESS => in 'Hold pickup library match' there is a new option called "patron's hold group"
4. In a library not in hold group set 'Hold policy' to "any" and 'Hold pickup library match' to "patron's hold group"
5. Search for a user in the hold group
6. 'Search to hold' for items of the library of step 4
7. Select an item and 'Place hold for [user]'
SUCCESS => in 'Pickup at' you should see patron's hold group as options
8. In OPAC sign in as the same user of step 5
9. Search for the item in step 7
SUCCESS => in 'Pick up locations' you should see patron's hold group as options
10. Sign off

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Signed-off-by: Liz Rea <wizzyrea@gmail.com>
Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 283 Tomás Cohen Arazi 2019-12-21 10:54:42 UTC
Created attachment 96555 [details] [review]
Bug 22284: (follow-up) Squash multiple follow-ups

  * Bug 22284: (follow-up) Use GetReserveControlBranch in Koha::Item->pickup_locations
  * Bug 22284: (follow-up) Fix tests
  * Bug 22284: (follow-up) Fix typo in request.tt
  * Bug 22284: (follow-up) Filter pickup on specific item click
  * Bug 22284: (follow-up) Fix typos transfered -> transferred

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Signed-off-by: Liz Rea <wizzyrea@gmail.com>
Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 284 Tomás Cohen Arazi 2019-12-21 10:54:49 UTC
Created attachment 96556 [details] [review]
Bug 22284: Schema update

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Signed-off-by: Liz Rea <wizzyrea@gmail.com>
Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 285 Tomás Cohen Arazi 2019-12-21 10:54:56 UTC
Created attachment 96557 [details] [review]
Bug 22284: (follow-up) Anotate boolean values in Koha::Schema::Result::LibraryGroup

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 286 Tomás Cohen Arazi 2019-12-21 10:55:03 UTC
Created attachment 96558 [details] [review]
Bug 22284: (QA follow-up) Make pickup locations be Koha::Library objects

This patch makes the following methods return array references of
Koha::Library objects instead or unblessed objects;

- Koha::Item->pickup_locations
- Koha::Biblio->pickup_locations
- Koha::Libraries->pickup_locations

Bonus:

- The template plugin is adjusted to unbless things to keep behavior
- Tests are moved to the right .t file.
- Tests for the new behavior are added.

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 287 Martin Renvoize 2020-01-03 12:56:26 UTC
Nice work everyone!

Pushed to master for 20.05
Comment 288 Jonathan Druart 2020-01-06 22:28:57 UTC
#   Failed test 'No tests run for subtest "pickup_locations() tests"'
#   at t/db_dependent/Template/Plugin/Branches.t line 151.
Can't call method "unblessed" on unblessed reference at /kohadevbox/koha/Koha/Template/Plugin/Branches.pm line 93.
# Looks like your test exited with 255 just after 3.
Comment 289 Jonathan Druart 2020-01-07 21:58:34 UTC
There is a (random?) failure on Master_D9 run 1053:

koha_1       |     #   Failed test 'ReservesControlBranch: ItemHomeLibrary, biblio1, patron1 should return 6 but returns 7'
koha_1       |     #   at t/db_dependent/Koha/Biblio.t line 419.
koha_1       | 
koha_1       |     #   Failed test 'ReservesControlBranch: PatronLibrary, biblio1, patron1 should return 6 but returns 7'
koha_1       |     #   at t/db_dependent/Koha/Biblio.t line 419.
koha_1       |     # Looks like you planned 29 tests but ran 31.
koha_1       |     # Looks like you failed 2 tests of 31 run.
koha_1       | 
koha_1       | #   Failed test 'pickup_locations'
koha_1       | #   at t/db_dependent/Koha/Biblio.t line 436.
Comment 290 Jonathan Druart 2020-01-10 12:52:10 UTC
Another one (Koha_Master_D9 run 1061):

koha_1       |     #   Failed test 'item1, patron1, holdallowed: homebranch, hold_fulfillment_policy: any should return 3 but returns 4'
koha_1       |     #   at t/db_dependent/Koha/Item.t line 308.
koha_1       | 
koha_1       |     #   Failed test 'item1, patron1, holdallowed: any, hold_fulfillment_policy: any should return 3 but returns 4'
koha_1       |     #   at t/db_dependent/Koha/Item.t line 308.
koha_1       | 
koha_1       |     #   Failed test 'item1, patron1, holdallowed: holdgroup, hold_fulfillment_policy: any should return 3 but returns 4'
koha_1       |     #   at t/db_dependent/Koha/Item.t line 308.
koha_1       | 
koha_1       |     #   Failed test 'item1, patron4, holdallowed: any, hold_fulfillment_policy: any should return 3 but returns 4'
koha_1       |     #   at t/db_dependent/Koha/Item.t line 308.
koha_1       | 
koha_1       |     #   Failed test 'item3, patron1, holdallowed: any, hold_fulfillment_policy: any should return 3 but returns 4'
koha_1       |     #   at t/db_dependent/Koha/Item.t line 308.
koha_1       | 
koha_1       |     #   Failed test 'item3, patron4, holdallowed: any, hold_fulfillment_policy: any should return 3 but returns 4'
koha_1       |     #   at t/db_dependent/Koha/Item.t line 308.
koha_1       | 
koha_1       |     #   Failed test 'item3, patron4, holdallowed: holdgroup, hold_fulfillment_policy: any should return 3 but returns 4'
koha_1       |     #   at t/db_dependent/Koha/Item.t line 308.
koha_1       |     # Looks like you planned 114 tests but ran 121.
koha_1       |     # Looks like you failed 7 tests of 121 run.
koha_1       | 
koha_1       | #   Failed test 'pickup_locations'
koha_1       | #   at t/db_dependent/Koha/Item.t line 332.
koha_1       | # Looks like you failed 1 test of 4.
koha_1       | [12:19:06] t/db_dependent/Koha/Item.t ..............................................
Comment 291 Joy Nelson 2020-01-14 00:13:31 UTC
Not backported to 19.11.x
Comment 292 Jonathan Druart 2020-01-14 10:49:48 UTC
We still have failing tests related to these patches:

t/db_dependent/Koha/Libraries.t .. 1/9             # Looks like you planned 11 tests but ran 2.

        #   Failed test 'Given BranchTransferLimitsType = itemtype and item-level_itypes = ON'
        #   at t/db_dependent/Koha/Libraries.t line 258.
        # Looks like you planned 4 tests but ran 2.
        # Looks like you failed 1 test of 2 run.

    #   Failed test 'UseBranchTransferLimits = ON'
    #   at t/db_dependent/Koha/Libraries.t line 422.
    # Looks like you failed 1 test of 2.
t/db_dependent/Koha/Libraries.t .. 6/9 
#   Failed test 'pickup_locations'
#   at t/db_dependent/Koha/Libraries.t line 423.
Can't call method "branchcode" on unblessed reference at t/db_dependent/Koha/Libraries.t line 195.
Comment 293 Tomás Cohen Arazi 2020-01-14 11:07:24 UTC
(In reply to Jonathan Druart from comment #292)
> We still have failing tests related to these patches:
> 
> t/db_dependent/Koha/Libraries.t .. 1/9             # Looks like you planned
> 11 tests but ran 2.
> 
>         #   Failed test 'Given BranchTransferLimitsType = itemtype and
> item-level_itypes = ON'
>         #   at t/db_dependent/Koha/Libraries.t line 258.
>         # Looks like you planned 4 tests but ran 2.
>         # Looks like you failed 1 test of 2 run.
> 
>     #   Failed test 'UseBranchTransferLimits = ON'
>     #   at t/db_dependent/Koha/Libraries.t line 422.
>     # Looks like you failed 1 test of 2.
> t/db_dependent/Koha/Libraries.t .. 6/9 
> #   Failed test 'pickup_locations'
> #   at t/db_dependent/Koha/Libraries.t line 423.
> Can't call method "branchcode" on unblessed reference at
> t/db_dependent/Koha/Libraries.t line 195.

I'll be working on it this morning
Comment 294 Jonathan Druart 2020-02-11 17:18:59 UTC
(In reply to Jonathan Druart from comment #290)
> Another one (Koha_Master_D9 run 1061):
> 
> koha_1       |     #   Failed test 'item1, patron1, holdallowed: homebranch,
> hold_fulfillment_policy: any should return 3 but returns 4'
> koha_1       |     #   at t/db_dependent/Koha/Item.t line 308.
> koha_1       | 
> koha_1       |     #   Failed test 'item1, patron1, holdallowed: any,
> hold_fulfillment_policy: any should return 3 but returns 4'
> koha_1       |     #   at t/db_dependent/Koha/Item.t line 308.
> koha_1       | 
> koha_1       |     #   Failed test 'item1, patron1, holdallowed: holdgroup,
> hold_fulfillment_policy: any should return 3 but returns 4'
> koha_1       |     #   at t/db_dependent/Koha/Item.t line 308.
> koha_1       | 
> koha_1       |     #   Failed test 'item1, patron4, holdallowed: any,
> hold_fulfillment_policy: any should return 3 but returns 4'
> koha_1       |     #   at t/db_dependent/Koha/Item.t line 308.
> koha_1       | 
> koha_1       |     #   Failed test 'item3, patron1, holdallowed: any,
> hold_fulfillment_policy: any should return 3 but returns 4'
> koha_1       |     #   at t/db_dependent/Koha/Item.t line 308.
> koha_1       | 
> koha_1       |     #   Failed test 'item3, patron4, holdallowed: any,
> hold_fulfillment_policy: any should return 3 but returns 4'
> koha_1       |     #   at t/db_dependent/Koha/Item.t line 308.
> koha_1       | 
> koha_1       |     #   Failed test 'item3, patron4, holdallowed: holdgroup,
> hold_fulfillment_policy: any should return 3 but returns 4'
> koha_1       |     #   at t/db_dependent/Koha/Item.t line 308.
> koha_1       |     # Looks like you planned 114 tests but ran 121.
> koha_1       |     # Looks like you failed 7 tests of 121 run.
> koha_1       | 
> koha_1       | #   Failed test 'pickup_locations'
> koha_1       | #   at t/db_dependent/Koha/Item.t line 332.
> koha_1       | # Looks like you failed 1 test of 4.
> koha_1       | [12:19:06] t/db_dependent/Koha/Item.t
> ..............................................

These ones are still failing.
Agustín, could you take a look at them?
Comment 295 Agustín Moyano 2020-02-11 17:57:05 UTC
(In reply to Jonathan Druart from comment #294)
> (In reply to Jonathan Druart from comment #290)
> > Another one (Koha_Master_D9 run 1061):
> > 
> > koha_1       |     #   Failed test 'item1, patron1, holdallowed: homebranch,
> > hold_fulfillment_policy: any should return 3 but returns 4'
> > koha_1       |     #   at t/db_dependent/Koha/Item.t line 308.
> > koha_1       | 
> > koha_1       |     #   Failed test 'item1, patron1, holdallowed: any,
> > hold_fulfillment_policy: any should return 3 but returns 4'
> > koha_1       |     #   at t/db_dependent/Koha/Item.t line 308.
> > koha_1       | 
> > koha_1       |     #   Failed test 'item1, patron1, holdallowed: holdgroup,
> > hold_fulfillment_policy: any should return 3 but returns 4'
> > koha_1       |     #   at t/db_dependent/Koha/Item.t line 308.
> > koha_1       | 
> > koha_1       |     #   Failed test 'item1, patron4, holdallowed: any,
> > hold_fulfillment_policy: any should return 3 but returns 4'
> > koha_1       |     #   at t/db_dependent/Koha/Item.t line 308.
> > koha_1       | 
> > koha_1       |     #   Failed test 'item3, patron1, holdallowed: any,
> > hold_fulfillment_policy: any should return 3 but returns 4'
> > koha_1       |     #   at t/db_dependent/Koha/Item.t line 308.
> > koha_1       | 
> > koha_1       |     #   Failed test 'item3, patron4, holdallowed: any,
> > hold_fulfillment_policy: any should return 3 but returns 4'
> > koha_1       |     #   at t/db_dependent/Koha/Item.t line 308.
> > koha_1       | 
> > koha_1       |     #   Failed test 'item3, patron4, holdallowed: holdgroup,
> > hold_fulfillment_policy: any should return 3 but returns 4'
> > koha_1       |     #   at t/db_dependent/Koha/Item.t line 308.
> > koha_1       |     # Looks like you planned 114 tests but ran 121.
> > koha_1       |     # Looks like you failed 7 tests of 121 run.
> > koha_1       | 
> > koha_1       | #   Failed test 'pickup_locations'
> > koha_1       | #   at t/db_dependent/Koha/Item.t line 332.
> > koha_1       | # Looks like you failed 1 test of 4.
> > koha_1       | [12:19:06] t/db_dependent/Koha/Item.t
> > ..............................................
> 
> These ones are still failing.
> Agustín, could you take a look at them?

ASAP
Comment 296 Agustín Moyano 2020-02-12 18:51:37 UTC
(In reply to Agustín Moyano from comment #295)
> (In reply to Jonathan Druart from comment #294)
> > (In reply to Jonathan Druart from comment #290)
> > > Another one (Koha_Master_D9 run 1061):
> > > 
> > > koha_1       |     #   Failed test 'item1, patron1, holdallowed: homebranch,
> > > hold_fulfillment_policy: any should return 3 but returns 4'
> > > koha_1       |     #   at t/db_dependent/Koha/Item.t line 308.
> > > koha_1       | 
> > > koha_1       |     #   Failed test 'item1, patron1, holdallowed: any,
> > > hold_fulfillment_policy: any should return 3 but returns 4'
> > > koha_1       |     #   at t/db_dependent/Koha/Item.t line 308.
> > > koha_1       | 
> > > koha_1       |     #   Failed test 'item1, patron1, holdallowed: holdgroup,
> > > hold_fulfillment_policy: any should return 3 but returns 4'
> > > koha_1       |     #   at t/db_dependent/Koha/Item.t line 308.
> > > koha_1       | 
> > > koha_1       |     #   Failed test 'item1, patron4, holdallowed: any,
> > > hold_fulfillment_policy: any should return 3 but returns 4'
> > > koha_1       |     #   at t/db_dependent/Koha/Item.t line 308.
> > > koha_1       | 
> > > koha_1       |     #   Failed test 'item3, patron1, holdallowed: any,
> > > hold_fulfillment_policy: any should return 3 but returns 4'
> > > koha_1       |     #   at t/db_dependent/Koha/Item.t line 308.
> > > koha_1       | 
> > > koha_1       |     #   Failed test 'item3, patron4, holdallowed: any,
> > > hold_fulfillment_policy: any should return 3 but returns 4'
> > > koha_1       |     #   at t/db_dependent/Koha/Item.t line 308.
> > > koha_1       | 
> > > koha_1       |     #   Failed test 'item3, patron4, holdallowed: holdgroup,
> > > hold_fulfillment_policy: any should return 3 but returns 4'
> > > koha_1       |     #   at t/db_dependent/Koha/Item.t line 308.
> > > koha_1       |     # Looks like you planned 114 tests but ran 121.
> > > koha_1       |     # Looks like you failed 7 tests of 121 run.
> > > koha_1       | 
> > > koha_1       | #   Failed test 'pickup_locations'
> > > koha_1       | #   at t/db_dependent/Koha/Item.t line 332.
> > > koha_1       | # Looks like you failed 1 test of 4.
> > > koha_1       | [12:19:06] t/db_dependent/Koha/Item.t
> > > ..............................................
> > 
> > These ones are still failing.
> > Agustín, could you take a look at them?
> 
> ASAP

Hi Jonathan, I cannot reproduce failure

kohadev-koha@cffdb0ae6429:/kohadevbox/koha$ cat /etc/issue                                                                                         
Debian GNU/Linux 9 \n \l                                                                                                                           
                                                                                                                                                   
kohadev-koha@cffdb0ae6429:/kohadevbox/koha$ prove t/db_dependent/Holds.t t/db_dependent/Koha/Biblios.t t/db_dependent/Koha/Items.t t/db_dependent/Koha/Libraries.t t/db_dependent/LibraryGroups.t                                                                                                     
t/db_dependent/Holds.t ........... ok                                                                                                              
t/db_dependent/Koha/Biblios.t .... ok                                                                                                              
t/db_dependent/Koha/Items.t ...... ok                                                                                                              
t/db_dependent/Koha/Libraries.t .. ok                                                                                                              
t/db_dependent/LibraryGroups.t ... ok     
All tests successful.
Files=5, Tests=106, 30 wallclock secs ( 0.08 usr  0.02 sys + 23.13 cusr  3.29 csys = 26.52 CPU)
Result: PASS

I'm testing in koha-testing-docker with Debian 9.. how are those tests run?
Comment 297 Jonathan Druart 2020-02-12 19:13:18 UTC
It's t/db_dependent/Koha/Item.t, but it's failing randomly.

Failing on jenkins on D9:
https://jenkins.koha-community.org/job/Koha_Master_D9/1061/consoleFull
And more recently: https://jenkins.koha-community.org/job/Koha_Master_D9/1132/consoleFull
You certainly need to launch them several times to see them fail (I cannot test right now).
Comment 299 Jonathan Druart 2020-02-13 15:38:06 UTC
A different failure from jenkins (D9 run 1139):

https://jenkins.koha-community.org/job/Koha_Master_D9/1139/consoleFull


koha_1       | 
koha_1       |     #   Failed test 'Patron can place hold if no circ_rules where defined'
koha_1       |     #   at t/db_dependent/Holds.t line 1095.
koha_1       |     #     Structures begin differing at:
koha_1       |     #          $got->{status} = 'libraryNotPickupLocation'
koha_1       |     #     $expected->{status} = 'OK'
koha_1       | 
koha_1       |     #   Failed test 'Patron cannot place hold because pickup location is not part of hold group'
koha_1       |     #   at t/db_dependent/Holds.t line 1122.
koha_1       |     #     Structures begin differing at:
koha_1       |     #          $got->{status} = 'libraryNotPickupLocation'
koha_1       |     #     $expected->{status} = 'pickupNotInHoldGroup'
koha_1       | 
koha_1       |     #   Failed test 'Patron can place hold if default_branch_circ_rules is set to "any" for library 2'
koha_1       |     #   at t/db_dependent/Holds.t line 1142.
koha_1       |     #     Structures begin differing at:
koha_1       |     #          $got->{status} = 'libraryNotPickupLocation'
koha_1       |     #     $expected->{status} = 'OK'
koha_1       | 
koha_1       |     #   Failed test 'Patron cannot place hold if hold_fulfillment_policy is set to "hold group" for library 2'
koha_1       |     #   at t/db_dependent/Holds.t line 1162.
koha_1       |     #     Structures begin differing at:
koha_1       |     #          $got->{status} = 'libraryNotPickupLocation'
koha_1       |     #     $expected->{status} = 'pickupNotInHoldGroup'
koha_1       | 
koha_1       |     #   Failed test 'Patron can place hold if hold_fulfillment_policy is set to "any" for itemtype 2'
koha_1       |     #   at t/db_dependent/Holds.t line 1182.
koha_1       |     #     Structures begin differing at:
koha_1       |     #          $got->{status} = 'libraryNotPickupLocation'
koha_1       |     #     $expected->{status} = 'OK'
koha_1       | 
koha_1       |     #   Failed test 'Patron cannot place hold if hold_fulfillment_policy is set to "hold group" for itemtype 2'
koha_1       |     #   at t/db_dependent/Holds.t line 1202.
koha_1       |     #     Structures begin differing at:
koha_1       |     #          $got->{status} = 'libraryNotPickupLocation'
koha_1       |     #     $expected->{status} = 'pickupNotInHoldGroup'
koha_1       | 
koha_1       |     #   Failed test 'Patron can place hold if hold_fulfillment_policy is set to "any" for itemtype 2 and library 2'
koha_1       |     #   at t/db_dependent/Holds.t line 1222.
koha_1       |     #     Structures begin differing at:
koha_1       |     #          $got->{status} = 'libraryNotPickupLocation'
koha_1       |     #     $expected->{status} = 'OK'
koha_1       | 
koha_1       |     #   Failed test 'Patron cannot place hold if hold_fulfillment_policy is set to "hold group" for itemtype 2 and library 2'
koha_1       |     #   at t/db_dependent/Holds.t line 1242.
koha_1       |     #     Structures begin differing at:
koha_1       |     #          $got->{status} = 'libraryNotPickupLocation'
koha_1       |     #     $expected->{status} = 'pickupNotInHoldGroup'
koha_1       |     # Looks like you failed 8 tests of 9.
koha_1       | 
koha_1       | #   Failed test 'CanItemBeReserved / pickup_not_in_hold_group'
koha_1       | #   at t/db_dependent/Holds.t line 1249.
koha_1       | # Looks like you failed 1 test of 61.
koha_1       | [15:06:03] t/db_dependent/Holds.t
Comment 300 Agustín Moyano 2020-02-13 16:32:02 UTC
(In reply to Jonathan Druart from comment #299)
> A different failure from jenkins (D9 run 1139):
> 
> https://jenkins.koha-community.org/job/Koha_Master_D9/1139/consoleFull
> 
> 
> koha_1       | 
> koha_1       |     #   Failed test 'Patron can place hold if no circ_rules
> where defined'
> koha_1       |     #   at t/db_dependent/Holds.t line 1095.
> koha_1       |     #     Structures begin differing at:
> koha_1       |     #          $got->{status} = 'libraryNotPickupLocation'
> koha_1       |     #     $expected->{status} = 'OK'
> koha_1       | 
> koha_1       |     #   Failed test 'Patron cannot place hold because pickup
> location is not part of hold group'
> koha_1       |     #   at t/db_dependent/Holds.t line 1122.
> koha_1       |     #     Structures begin differing at:
> koha_1       |     #          $got->{status} = 'libraryNotPickupLocation'
> koha_1       |     #     $expected->{status} = 'pickupNotInHoldGroup'
> koha_1       | 
> koha_1       |     #   Failed test 'Patron can place hold if
> default_branch_circ_rules is set to "any" for library 2'
> koha_1       |     #   at t/db_dependent/Holds.t line 1142.
> koha_1       |     #     Structures begin differing at:
> koha_1       |     #          $got->{status} = 'libraryNotPickupLocation'
> koha_1       |     #     $expected->{status} = 'OK'
> koha_1       | 
> koha_1       |     #   Failed test 'Patron cannot place hold if
> hold_fulfillment_policy is set to "hold group" for library 2'
> koha_1       |     #   at t/db_dependent/Holds.t line 1162.
> koha_1       |     #     Structures begin differing at:
> koha_1       |     #          $got->{status} = 'libraryNotPickupLocation'
> koha_1       |     #     $expected->{status} = 'pickupNotInHoldGroup'
> koha_1       | 
> koha_1       |     #   Failed test 'Patron can place hold if
> hold_fulfillment_policy is set to "any" for itemtype 2'
> koha_1       |     #   at t/db_dependent/Holds.t line 1182.
> koha_1       |     #     Structures begin differing at:
> koha_1       |     #          $got->{status} = 'libraryNotPickupLocation'
> koha_1       |     #     $expected->{status} = 'OK'
> koha_1       | 
> koha_1       |     #   Failed test 'Patron cannot place hold if
> hold_fulfillment_policy is set to "hold group" for itemtype 2'
> koha_1       |     #   at t/db_dependent/Holds.t line 1202.
> koha_1       |     #     Structures begin differing at:
> koha_1       |     #          $got->{status} = 'libraryNotPickupLocation'
> koha_1       |     #     $expected->{status} = 'pickupNotInHoldGroup'
> koha_1       | 
> koha_1       |     #   Failed test 'Patron can place hold if
> hold_fulfillment_policy is set to "any" for itemtype 2 and library 2'
> koha_1       |     #   at t/db_dependent/Holds.t line 1222.
> koha_1       |     #     Structures begin differing at:
> koha_1       |     #          $got->{status} = 'libraryNotPickupLocation'
> koha_1       |     #     $expected->{status} = 'OK'
> koha_1       | 
> koha_1       |     #   Failed test 'Patron cannot place hold if
> hold_fulfillment_policy is set to "hold group" for itemtype 2 and library 2'
> koha_1       |     #   at t/db_dependent/Holds.t line 1242.
> koha_1       |     #     Structures begin differing at:
> koha_1       |     #          $got->{status} = 'libraryNotPickupLocation'
> koha_1       |     #     $expected->{status} = 'pickupNotInHoldGroup'
> koha_1       |     # Looks like you failed 8 tests of 9.
> koha_1       | 
> koha_1       | #   Failed test 'CanItemBeReserved / pickup_not_in_hold_group'
> koha_1       | #   at t/db_dependent/Holds.t line 1249.
> koha_1       | # Looks like you failed 1 test of 61.
> koha_1       | [15:06:03] t/db_dependent/Holds.t

Hi Jonathan,

I think I spotted this failure.. should I make a new bug to fix this tests?

The random failure on Item.t I'm at a loss. Supposedly I delete every circulation_rule, every item, every issue and every patron, but it seems not everything gets deleted.. or that's what I guess.

Do you have any tip of what could be wrong?
Comment 301 Agustín Moyano 2020-02-13 17:07:20 UTC
(In reply to Agustín Moyano from comment #300)
> (In reply to Jonathan Druart from comment #299)
> > A different failure from jenkins (D9 run 1139):
> > 
> > https://jenkins.koha-community.org/job/Koha_Master_D9/1139/consoleFull
> > 
> > 
> > koha_1       | 
> > koha_1       |     #   Failed test 'Patron can place hold if no circ_rules
> > where defined'
> > koha_1       |     #   at t/db_dependent/Holds.t line 1095.
> > koha_1       |     #     Structures begin differing at:
> > koha_1       |     #          $got->{status} = 'libraryNotPickupLocation'
> > koha_1       |     #     $expected->{status} = 'OK'
> > koha_1       | 
> > koha_1       |     #   Failed test 'Patron cannot place hold because pickup
> > location is not part of hold group'
> > koha_1       |     #   at t/db_dependent/Holds.t line 1122.
> > koha_1       |     #     Structures begin differing at:
> > koha_1       |     #          $got->{status} = 'libraryNotPickupLocation'
> > koha_1       |     #     $expected->{status} = 'pickupNotInHoldGroup'
> > koha_1       | 
> > koha_1       |     #   Failed test 'Patron can place hold if
> > default_branch_circ_rules is set to "any" for library 2'
> > koha_1       |     #   at t/db_dependent/Holds.t line 1142.
> > koha_1       |     #     Structures begin differing at:
> > koha_1       |     #          $got->{status} = 'libraryNotPickupLocation'
> > koha_1       |     #     $expected->{status} = 'OK'
> > koha_1       | 
> > koha_1       |     #   Failed test 'Patron cannot place hold if
> > hold_fulfillment_policy is set to "hold group" for library 2'
> > koha_1       |     #   at t/db_dependent/Holds.t line 1162.
> > koha_1       |     #     Structures begin differing at:
> > koha_1       |     #          $got->{status} = 'libraryNotPickupLocation'
> > koha_1       |     #     $expected->{status} = 'pickupNotInHoldGroup'
> > koha_1       | 
> > koha_1       |     #   Failed test 'Patron can place hold if
> > hold_fulfillment_policy is set to "any" for itemtype 2'
> > koha_1       |     #   at t/db_dependent/Holds.t line 1182.
> > koha_1       |     #     Structures begin differing at:
> > koha_1       |     #          $got->{status} = 'libraryNotPickupLocation'
> > koha_1       |     #     $expected->{status} = 'OK'
> > koha_1       | 
> > koha_1       |     #   Failed test 'Patron cannot place hold if
> > hold_fulfillment_policy is set to "hold group" for itemtype 2'
> > koha_1       |     #   at t/db_dependent/Holds.t line 1202.
> > koha_1       |     #     Structures begin differing at:
> > koha_1       |     #          $got->{status} = 'libraryNotPickupLocation'
> > koha_1       |     #     $expected->{status} = 'pickupNotInHoldGroup'
> > koha_1       | 
> > koha_1       |     #   Failed test 'Patron can place hold if
> > hold_fulfillment_policy is set to "any" for itemtype 2 and library 2'
> > koha_1       |     #   at t/db_dependent/Holds.t line 1222.
> > koha_1       |     #     Structures begin differing at:
> > koha_1       |     #          $got->{status} = 'libraryNotPickupLocation'
> > koha_1       |     #     $expected->{status} = 'OK'
> > koha_1       | 
> > koha_1       |     #   Failed test 'Patron cannot place hold if
> > hold_fulfillment_policy is set to "hold group" for itemtype 2 and library 2'
> > koha_1       |     #   at t/db_dependent/Holds.t line 1242.
> > koha_1       |     #     Structures begin differing at:
> > koha_1       |     #          $got->{status} = 'libraryNotPickupLocation'
> > koha_1       |     #     $expected->{status} = 'pickupNotInHoldGroup'
> > koha_1       |     # Looks like you failed 8 tests of 9.
> > koha_1       | 
> > koha_1       | #   Failed test 'CanItemBeReserved / pickup_not_in_hold_group'
> > koha_1       | #   at t/db_dependent/Holds.t line 1249.
> > koha_1       | # Looks like you failed 1 test of 61.
> > koha_1       | [15:06:03] t/db_dependent/Holds.t
> 
> Hi Jonathan,
> 
> I think I spotted this failure.. should I make a new bug to fix this tests?
> 
> The random failure on Item.t I'm at a loss. Supposedly I delete every
> circulation_rule, every item, every issue and every patron, but it seems not
> everything gets deleted.. or that's what I guess.
> 
> Do you have any tip of what could be wrong?

Never mind, I think I nailed it!.. I'll submit patches in following bug!
Comment 302 Jonathan Druart 2020-04-13 10:28:02 UTC
The only D10 failures we have are coming from here:

12:56:37 koha_1       |     #   Failed test 'ReservesControlBranch: ItemHomeLibrary, biblio1, patron1 should return 6 but returns 7'
12:56:37 koha_1       |     #   at t/db_dependent/Koha/Biblio.t line 418.
12:56:37 koha_1       | 
12:56:37 koha_1       |     #   Failed test 'ReservesControlBranch: PatronLibrary, biblio1, patron1 should return 6 but returns 7'
12:56:37 koha_1       |     #   at t/db_dependent/Koha/Biblio.t line 418.
12:56:37 koha_1       |     # Looks like you planned 29 tests but ran 31.
12:56:37 koha_1       |     # Looks like you failed 2 tests of 31 run.
12:56:37 koha_1       | 
12:56:37 koha_1       | #   Failed test 'pickup_locations'
12:56:37 koha_1       | #   at t/db_dependent/Koha/Biblio.t line 435.
12:56:46 koha_1       | # Looks like you failed 1 test of 12.
12:56:46 koha_1       | [10:56:45] t/db_dependent/Koha/Biblio.t

That would be great to see them fixed for 20.05.
Comment 303 Jonathan Druart 2020-05-19 10:27:37 UTC
(In reply to Jonathan Druart from comment #302)
> The only D10 failures we have are coming from here:
> 
> 12:56:37 koha_1       |     #   Failed test 'ReservesControlBranch:
> ItemHomeLibrary, biblio1, patron1 should return 6 but returns 7'
> 12:56:37 koha_1       |     #   at t/db_dependent/Koha/Biblio.t line 418.
> 12:56:37 koha_1       | 
> 12:56:37 koha_1       |     #   Failed test 'ReservesControlBranch:
> PatronLibrary, biblio1, patron1 should return 6 but returns 7'
> 12:56:37 koha_1       |     #   at t/db_dependent/Koha/Biblio.t line 418.
> 12:56:37 koha_1       |     # Looks like you planned 29 tests but ran 31.
> 12:56:37 koha_1       |     # Looks like you failed 2 tests of 31 run.
> 12:56:37 koha_1       | 
> 12:56:37 koha_1       | #   Failed test 'pickup_locations'
> 12:56:37 koha_1       | #   at t/db_dependent/Koha/Biblio.t line 435.
> 12:56:46 koha_1       | # Looks like you failed 1 test of 12.
> 12:56:46 koha_1       | [10:56:45] t/db_dependent/Koha/Biblio.t
> 
> That would be great to see them fixed for 20.05.

I opened bug 25540. Few days left before 20.05!