Bug 11005

Sponsored by Joensuu Regional Library

Deployed the properly formatted patch.
Applied on our testing environment and works fine.

Can someone delete this attachement "Refactored the IsBranchTransferAllowed() and it's dependencies. Created test cases." and this Comment?

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

Created attachment 21999 [details]
Manual test cases

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"

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.

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.

Please provide a patch with changes only.
Reindentation should be in a separate patch.

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

Still valid?