Bug 19301

Summary: Move C4::Reserves::OnShelfHoldsAllowed to the Koha namespace
Product: Koha Reporter: Jonathan Druart <jonathan.druart>
Component: Architecture, internals, and plumbingAssignee: Jonathan Druart <jonathan.druart>
Status: CLOSED FIXED QA Contact: Marcel de Rooy <m.de.rooy>
Severity: enhancement    
Priority: P5 - low CC: alexbuckley, m.de.rooy
Version: unspecified   
Hardware: All   
OS: All   
See Also: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=4319
Change sponsored?: --- Patch complexity: Small patch
Documentation contact: Documentation submission:
Text to go in the release notes:
Version(s) released in:
Bug Depends on: 4319, 19298, 19300    
Bug Blocks: 17728, 19297, 19302    
Attachments: Bug 19301: Move C4::Reserves::OnShelfHoldsAllowed to get_onshelfholds_policy
Bug 19301: Remove C4::Reserves::OnShelfHoldsAllowed
Bug 19301: Move C4::Reserves::OnShelfHoldsAllowed to get_onshelfholds_policy
Bug 19301: Remove C4::Reserves::OnShelfHoldsAllowed
Bug 19301: Move C4::Reserves::OnShelfHoldsAllowed to get_onshelfholds_policy
Bug 19301: Remove C4::Reserves::OnShelfHoldsAllowed
Bug 19301: Move C4::Reserves::OnShelfHoldsAllowed to get_onshelfholds_policy
Bug 19301: Remove C4::Reserves::OnShelfHoldsAllowed
Bug 19301: (QA follow-up) Add POD, use statements and correct typo
Bug 19301: (QA follow-up) Add a few simple tests

Description Jonathan Druart 2017-09-12 21:36:37 UTC

    
Comment 1 Jonathan Druart 2017-09-13 19:23:10 UTC
Created attachment 67122 [details] [review]
Bug 19301: Move C4::Reserves::OnShelfHoldsAllowed to get_onshelfholds_policy

Following the pattern introduced by bug 19300, we are going to move the
OnShelfHoldsAllowed logic to Koha::IssuingRules->get_onshelfholds_policy

Test plan:
Make sure the onshelfholds policy is correct when placing a hold
Comment 2 Jonathan Druart 2017-09-13 19:23:13 UTC
Created attachment 67123 [details] [review]
Bug 19301: Remove C4::Reserves::OnShelfHoldsAllowed

At this point the subroutine is not used anymore
Comment 3 Alex Buckley 2017-09-27 04:11:39 UTC
I had issues with applying this patch:

Apply? [(y)es, (n)o, (i)nteractive] i
Applying: Bug 19301: Move C4::Reserves::OnShelfHoldsAllowed to get_onshelfholds_policy
fatal: sha1 information is lacking or useless (C4/Reserves.pm).
Repository lacks necessary blobs to fall back on 3-way merge.
Cannot fall back to three-way merge.
Patch failed at 0001 Bug 19301: Move C4::Reserves::OnShelfHoldsAllowed to get_onshelfholds_policy
The copy of the patch that failed is found in:
   /home/vagrant/kohaclone/.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-19301-Move-C4ReservesOnShelfHoldsAllowed-to-ge-7LzZnO.patch
Comment 4 Jonathan Druart 2017-09-27 13:11:15 UTC
(In reply to Alex Buckley from comment #3)
> I had issues with applying this patch:

One dependency was missing, please try again on top of 19298 and 19300.
Comment 5 Alex Buckley 2017-09-27 23:45:47 UTC
Created attachment 67420 [details] [review]
Bug 19301: Move C4::Reserves::OnShelfHoldsAllowed to get_onshelfholds_policy

Following the pattern introduced by bug 19300, we are going to move the
OnShelfHoldsAllowed logic to Koha::IssuingRules->get_onshelfholds_policy

Test plan:
Make sure the onshelfholds policy is correct when placing a hold

Signed-off-by: Alex Buckley <alexbuckley@catalyst.net.nz>
Comment 6 Alex Buckley 2017-09-27 23:47:18 UTC
Created attachment 67421 [details] [review]
Bug 19301: Remove C4::Reserves::OnShelfHoldsAllowed

At this point the subroutine is not used anymore

Signed-off-by: Alex Buckley <alexbuckley@catalyst.net.nz>
Comment 7 Marcel de Rooy 2017-12-15 09:20:28 UTC
May I suggest to rebase this one on top of 4319 (which has taken quite some time already) ?
Comment 8 Jonathan Druart 2017-12-18 14:16:46 UTC
Created attachment 69846 [details] [review]
Bug 19301: Move C4::Reserves::OnShelfHoldsAllowed to get_onshelfholds_policy

Following the pattern introduced by bug 19300, we are going to move the
OnShelfHoldsAllowed logic to Koha::IssuingRules->get_onshelfholds_policy

Test plan:
Make sure the onshelfholds policy is correct when placing a hold

Signed-off-by: Alex Buckley <alexbuckley@catalyst.net.nz>
Comment 9 Jonathan Druart 2017-12-18 14:16:51 UTC
Created attachment 69847 [details] [review]
Bug 19301: Remove C4::Reserves::OnShelfHoldsAllowed

At this point the subroutine is not used anymore

Signed-off-by: Alex Buckley <alexbuckley@catalyst.net.nz>
Comment 10 Jonathan Druart 2017-12-18 14:17:39 UTC
Rebased on top of bug 4319.
Comment 11 Marcel de Rooy 2017-12-22 07:36:41 UTC
QA: Looking here now
Comment 12 Marcel de Rooy 2017-12-22 09:34:01 UTC
Created attachment 70106 [details] [review]
Bug 19301: Move C4::Reserves::OnShelfHoldsAllowed to get_onshelfholds_policy

Following the pattern introduced by bug 19300, we are going to move the
OnShelfHoldsAllowed logic to Koha::IssuingRules->get_onshelfholds_policy

Test plan:
Make sure the onshelfholds policy is correct when placing a hold

Signed-off-by: Alex Buckley <alexbuckley@catalyst.net.nz>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 13 Marcel de Rooy 2017-12-22 09:34:05 UTC
Created attachment 70107 [details] [review]
Bug 19301: Remove C4::Reserves::OnShelfHoldsAllowed

At this point the subroutine is not used anymore

Signed-off-by: Alex Buckley <alexbuckley@catalyst.net.nz>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 14 Marcel de Rooy 2017-12-22 09:34:09 UTC
Created attachment 70108 [details] [review]
Bug 19301: (QA follow-up) Add POD, use statements and correct typo

Add POD for new sub in Koha/IssuingRules.pm.
Adding use Koha::IssuingRules to opac-detail and opac-MARCdetail.
Adding use Koha::Items to opac-detail and opac-MARCdetail.
Correct typo $items => $item in opac-MARCdetail.pl.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 15 Marcel de Rooy 2017-12-22 09:34:13 UTC
Created attachment 70109 [details] [review]
Bug 19301: (QA follow-up) Add a few simple tests

We removed tests from Reserves.t, but would be nice to test the new
sub in IssuingRules.t too.
Adding a subtest there.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 16 Jonathan Druart 2018-02-13 17:17:43 UTC
Pushed to master for 18.05, thanks to everybody involved!