Bug 11005

Summary: Centralize the UseBranchTransferLimits-check under one function
Product: Koha Reporter: Olli-Antti Kivilahti <olli-antti.kivilahti>
Component: Architecture, internals, and plumbingAssignee: Olli-Antti Kivilahti <olli-antti.kivilahti>
Status: Failed QA --- QA Contact: Testopia <testopia>
Severity: enhancement    
Priority: P3 CC: jonathan.druart, kyle, paul.poulain, veron
Version: Main   
Hardware: All   
OS: All   
Change sponsored?: Sponsored Patch complexity: Small patch
Documentation contact: Documentation submission:
Text to go in the release notes:
Version(s) released in:
Bug Depends on:    
Bug Blocks: 10993    
Attachments: Refactored the IsBranchTransferAllowed() and it's dependencies. Created test cases.
Bug 11005 [ENH] - Centralize the UseBranchTransferLimits-check under one function
Bug 11005 [ENH] - Centralize the UseBranchTransferLimits-check under one function
Manual test cases
Bug 11005 [ENH] - Centralize the UseBranchTransferLimits-check under one function
Bug 11005 [ENH] - Centralize the UseBranchTransferLimits-check under one function

Description Olli-Antti Kivilahti 2013-10-07 14:15:09 UTC
Currently the IsBranchTransferAllowed() doesn't take into account multiple different global preferences and the check for IsBranchTransferAllowed() is really messy with multiple if-else statements.
Move these global parameter checks to the core IsBranchTransferAllowed()-function to make using the function clearer.
Comment 1 Olli-Antti Kivilahti 2013-10-07 14:17:28 UTC
Sponsored by Joensuu Regional Library
Comment 2 Olli-Antti Kivilahti 2013-10-07 14:26:34 UTC Comment hidden (obsolete)
Comment 3 Olli-Antti Kivilahti 2013-10-15 09:11:16 UTC Comment hidden (obsolete)
Comment 4 Olli-Antti Kivilahti 2013-10-15 09:19:14 UTC
Deployed the properly formatted patch.
Applied on our testing environment and works fine.
Comment 5 Olli-Antti Kivilahti 2013-10-15 09:27:07 UTC Comment hidden (obsolete)
Comment 6 Olli-Antti Kivilahti 2013-10-15 09:31:10 UTC
Can someone delete this attachement "Refactored the IsBranchTransferAllowed() and it's dependencies. Created test cases." and this Comment?
Comment 7 Jonathan Druart 2013-10-15 09:46:17 UTC
(In reply to Olli-Antti Kivilahti from comment #6)
> Can someone delete this attachement "Refactored the
> IsBranchTransferAllowed() and it's dependencies. Created test cases." and
> this Comment?

You cannot delete an attachment but you can obsolete it:
click on "Details" > edit details > check the 'obsolete' cb and submit.
Comment 8 Olli-Antti Kivilahti 2013-10-15 10:28:56 UTC
Created attachment 21999 [details]
Manual test cases
Comment 9 Paul Poulain 2013-10-22 23:58:20 UTC
QA comments:
 * 4 spaces instead of tabs, please
 * I think the sub CheckBranchTransferAllowed should be named CanBookBeTransferred, that would be more consistent with ...BeIssued and ...BeRenewed (and also C4/Reserves.pm::CanBookBeReserved
 * there's an unconditional warn:
             warn "NotAllowed: $messages->{'NotAllowed'} to  " . $branches->{ $messages->{'NotAllowed'} }->{'branchname'};
=> please add a condition:
   warn ... if $ENV{DEBUG}

 * +1 for .t, looks great ! Note that it's MARC21 specific, I think you could/should skip some tests if systempreference("marcflavour") eq "UNIMARC"
Comment 10 Olli-Antti Kivilahti 2013-10-23 15:12:45 UTC Comment hidden (obsolete)
Comment 11 Olli-Antti Kivilahti 2013-10-23 15:31:34 UTC
Fixed indentations from tabs to spaces.

Renamed CheckBranchTransferAllowed() -> CanItemBeTransferred()
CanBookBeTransferred refers to a title level check, when the function only checks item level transferrability.
Like in C4::Reserves::CanItemBeReserved() vs CanBookBeReserved().

Added a Unimarc example to the unit tests.
Uncertain whether or not the different Marc flavour actually prevents the test from working, since the itemtype-related Marc modification is pulled dynamically from the Koha to Marc mappings. Expecting these to alter based on the chosen Marc flavour.
Comment 12 Kyle M Hall 2013-10-25 16:00:47 UTC
perl t/db_dependent/Circulation/CanItemBeTransferred.t
1..20
Running tests for ccode
ok 1 - Successful branch transfer, full parameters
not ok 2 - Failing branch transfer, full parameters
#   Failed test 'Failing branch transfer, full parameters'
#   at t/db_dependent/Circulation/CanItemBeTransferred.t line 25.
#          got: '1'
#     expected: 'CPL->IPT->FANTASY'
ok 3 - Successful branch transfer, full parameters, no Biblio defined
not ok 4 - Failing branch transfer, full parameters, no Biblio defined
#   Failed test 'Failing branch transfer, full parameters, no Biblio defined'
#   at t/db_dependent/Circulation/CanItemBeTransferred.t line 31.
#          got: '1'
#     expected: 'CPL->IPT->FANTASY'
ok 5 - Successful branch transfer, using defaults for $fromBranch
not ok 6 - Failing branch transfer, using defaults for $fromBranch
#   Failed test 'Failing branch transfer, using defaults for $fromBranch'
#   at t/db_dependent/Circulation/CanItemBeTransferred.t line 37.
#          got: '1'
#     expected: 'CPL->IPT->FANTASY'
ok 7 - Successful branch transfer, using minimum parameters
not ok 8 - Failing branch transfer, using minimum parameters
#   Failed test 'Failing branch transfer, using minimum parameters'
#   at t/db_dependent/Circulation/CanItemBeTransferred.t line 43.
#          got: '1'
#     expected: 'CPL->IPT->FANTASY'
ok 9 - Successful branch transfer, using minimum parameters
ok 10 - Not failing branch transfer, because CCODE cannot be found from the item and it is not a part of the biblio.
Running tests for itemtype
ok 11 - Successful branch transfer, full parameters
not ok 12 - Failing branch transfer, full parameters
#   Failed test 'Failing branch transfer, full parameters'
#   at t/db_dependent/Circulation/CanItemBeTransferred.t line 63.
#          got: '1'
#     expected: 'CPL->IPT->BK'
ok 13 - Successful branch transfer, full parameters, no Biblio defined
not ok 14 - Failing branch transfer, full parameters, no Biblio defined
#   Failed test 'Failing branch transfer, full parameters, no Biblio defined'
#   at t/db_dependent/Circulation/CanItemBeTransferred.t line 69.
#          got: '1'
#     expected: 'CPL->IPT->BK'
ok 15 - Successful branch transfer, using defaults for $fromBranch
not ok 16 - Failing branch transfer, using defaults for $fromBranch
#   Failed test 'Failing branch transfer, using defaults for $fromBranch'
#   at t/db_dependent/Circulation/CanItemBeTransferred.t line 75.
#          got: '1'
#     expected: 'CPL->IPT->BK'
ok 17 - Successful branch transfer, using minimum parameters
not ok 18 - Failing branch transfer, using minimum parameters
#   Failed test 'Failing branch transfer, using minimum parameters'
#   at t/db_dependent/Circulation/CanItemBeTransferred.t line 81.
#          got: '1'
#     expected: 'CPL->IPT->BK'
ok 19 - Successful branch transfer, using minimum parameters, itemtype is pulled from Biblio
not ok 20 - Failing branch transfer, using minimum parameters, itemtype is pulled from Biblio
#   Failed test 'Failing branch transfer, using minimum parameters, itemtype is pulled from Biblio'
#   at t/db_dependent/Circulation/CanItemBeTransferred.t line 87.
#          got: '1'
#     expected: 'CPL->IPT->BK'
# Looks like you failed 9 tests of 20.
Comment 13 Jonathan Druart 2013-12-19 12:38:15 UTC
Please provide a patch with changes only.
Reindentation should be in a separate patch.
Comment 14 Chris Cormack 2014-06-26 04:20:18 UTC
Created attachment 29269 [details] [review]
Bug 11005 [ENH] - Centralize the UseBranchTransferLimits-check under one function

Created a C4::Circulation::CanItemBeTransferred() as a more convenient branch transfer check
than the previous multiline if-else mess. This is a more pleasant and readable approach to generalizing
branch transfer checks accross Koha.
Includes a error message as the return value so the precise reasons for denial need not be constructed
when needed.

Also a small fix to branchtransfers.pl to make a warning happen only during debugging.

Unit tests included which test the new C4 function.

--
Rebased by chrisc@catalyst.net.nz fixing conflict.
This patch mixes whitespace clean up and functional changes, however if you view it with
git show -w

You can see just the actual changes.

Currently is failing the tests on my dev set up


Current status: Failed QA
Comment 15 Marc VĂ©ron 2016-10-04 16:33:55 UTC
Still valid?