Bug 11005 - Centralize the UseBranchTransferLimits-check under one function
Summary: Centralize the UseBranchTransferLimits-check under one function
Status: Failed QA
Alias: None
Product: Koha
Classification: Unclassified
Component: Architecture, internals, and plumbing (show other bugs)
Version: master
Hardware: All All
: P3 enhancement (vote)
Assignee: Olli-Antti Kivilahti
QA Contact: Testopia
URL:
Whiteboard:
Keywords:
Depends on:
Blocks: 10993
  Show dependency treegraph
 
Reported: 2013-10-07 14:15 UTC by Olli-Antti Kivilahti
Modified: 2016-10-04 16:33 UTC (History)
4 users (show)

See Also:
Change sponsored?: Sponsored
Patch complexity: Small patch
Bot Control: ---
When did the bot last check this:
Who signed the patch off:
Text to go in the release notes:


Attachments
Refactored the IsBranchTransferAllowed() and it's dependencies. Created test cases. (15.06 KB, patch)
2013-10-07 14:26 UTC, Olli-Antti Kivilahti
Details | Diff | Splinter Review
Bug 11005 [ENH] - Centralize the UseBranchTransferLimits-check under one function (14.96 KB, patch)
2013-10-15 09:11 UTC, Olli-Antti Kivilahti
Details | Diff | Splinter Review
Bug 11005 [ENH] - Centralize the UseBranchTransferLimits-check under one function (16.83 KB, patch)
2013-10-15 09:27 UTC, Olli-Antti Kivilahti
Details | Diff | Splinter Review
Manual test cases (352 bytes, text/plain)
2013-10-15 10:28 UTC, Olli-Antti Kivilahti
Details
Bug 11005 [ENH] - Centralize the UseBranchTransferLimits-check under one function (38.03 KB, patch)
2013-10-23 15:12 UTC, Olli-Antti Kivilahti
Details | Diff | Splinter Review
Bug 11005 [ENH] - Centralize the UseBranchTransferLimits-check under one function (37.89 KB, patch)
2014-06-26 04:20 UTC, Chris Cormack
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
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?