[Bug 11005] New: Centralize the UseBranchTransferLimits-check under one function
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=11005 Bug ID: 11005 Summary: Centralize the UseBranchTransferLimits-check under one function Change sponsored?: --- Product: Koha Version: master Hardware: All OS: All Status: NEW Severity: enhancement Priority: P5 - low Component: Architecture, internals, and plumbing Assignee: gmcharlt@gmail.com Reporter: olli-antti.kivilahti@jns.fi QA Contact: testopia@bugs.koha-community.org 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. -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=11005 Olli-Antti Kivilahti <olli-antti.kivilahti@jns.fi> changed: What |Removed |Added ---------------------------------------------------------------------------- Priority|P5 - low |P3 Patch complexity|--- |Small patch Depends on| |7376 Change sponsored?|--- |Sponsored Assignee|gmcharlt@gmail.com |olli-antti.kivilahti@jns.fi -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=11005 --- Comment #1 from Olli-Antti Kivilahti <olli-antti.kivilahti@jns.fi> --- Sponsored by Joensuu Regional Library -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=11005 Olli-Antti Kivilahti <olli-antti.kivilahti@jns.fi> changed: What |Removed |Added ---------------------------------------------------------------------------- Blocks| |10993 -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=11005 --- Comment #2 from Olli-Antti Kivilahti <olli-antti.kivilahti@jns.fi> --- Created attachment 21851 --> http://bugs.koha-community.org/bugzilla3/attachment.cgi?id=21851&action=edit Refactored the IsBranchTransferAllowed() and it's dependencies. Created test cases. -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=11005 Olli-Antti Kivilahti <olli-antti.kivilahti@jns.fi> changed: What |Removed |Added ---------------------------------------------------------------------------- Blocks| |11020 -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=11005 Olli-Antti Kivilahti <olli-antti.kivilahti@jns.fi> changed: What |Removed |Added ---------------------------------------------------------------------------- Blocks| |11024 -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=11005 Jonathan Druart <jonathan.druart@biblibre.com> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |jonathan.druart@biblibre.co | |m -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=11005 --- Comment #3 from Olli-Antti Kivilahti <olli-antti.kivilahti@jns.fi> --- Created attachment 21993 --> http://bugs.koha-community.org/bugzilla3/attachment.cgi?id=21993&action=edit Bug 11005 [ENH] - Centralize the UseBranchTransferLimits-check under one function Created a C4::Circulation::CheckBranchTransferAllowed() 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. Unit tests included which test the new C4 function. -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=11005 Olli-Antti Kivilahti <olli-antti.kivilahti@jns.fi> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|NEW |Needs Signoff --- Comment #4 from Olli-Antti Kivilahti <olli-antti.kivilahti@jns.fi> --- Deployed the properly formatted patch. Applied on our testing environment and works fine. -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=11005 Olli-Antti Kivilahti <olli-antti.kivilahti@jns.fi> changed: What |Removed |Added ---------------------------------------------------------------------------- Blocks| |7376 Depends on|7376 | -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=11005 Olli-Antti Kivilahti <olli-antti.kivilahti@jns.fi> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #21993|0 |1 is obsolete| | --- Comment #5 from Olli-Antti Kivilahti <olli-antti.kivilahti@jns.fi> --- Created attachment 21995 --> http://bugs.koha-community.org/bugzilla3/attachment.cgi?id=21995&action=edit Bug 11005 [ENH] - Centralize the UseBranchTransferLimits-check under one function Created a C4::Circulation::CheckBranchTransferAllowed() 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. Unit tests included which test the new C4 function. -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=11005 --- Comment #6 from Olli-Antti Kivilahti <olli-antti.kivilahti@jns.fi> --- Can someone delete this attachement "Refactored the IsBranchTransferAllowed() and it's dependencies. Created test cases." and this Comment? -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=11005 --- Comment #7 from Jonathan Druart <jonathan.druart@biblibre.com> --- (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. -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=11005 Olli-Antti Kivilahti <olli-antti.kivilahti@jns.fi> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #21851|0 |1 is obsolete| | --- Comment #8 from Olli-Antti Kivilahti <olli-antti.kivilahti@jns.fi> --- Created attachment 21999 --> http://bugs.koha-community.org/bugzilla3/attachment.cgi?id=21999&action=edit Manual test cases -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=11005 Paul Poulain <paul.poulain@biblibre.com> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |paul.poulain@biblibre.com --- Comment #9 from Paul Poulain <paul.poulain@biblibre.com> --- 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" -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=11005 Olli-Antti Kivilahti <olli-antti.kivilahti@jns.fi> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #21995|0 |1 is obsolete| | --- Comment #10 from Olli-Antti Kivilahti <olli-antti.kivilahti@jns.fi> --- Created attachment 22320 --> http://bugs.koha-community.org/bugzilla3/attachment.cgi?id=22320&action=edit 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. -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=11005 --- Comment #11 from Olli-Antti Kivilahti <olli-antti.kivilahti@jns.fi> --- 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. -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=11005 Kyle M Hall <kyle@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Needs Signoff |Failed QA CC| |kyle@bywatersolutions.com --- Comment #12 from Kyle M Hall <kyle@bywatersolutions.com> --- 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. -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=11005 Olli-Antti Kivilahti <olli-antti.kivilahti@jns.fi> changed: What |Removed |Added ---------------------------------------------------------------------------- Blocks|7376, 11020, 11024 | -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=11005 --- Comment #13 from Jonathan Druart <jonathan.druart@biblibre.com> --- Please provide a patch with changes only. Reindentation should be in a separate patch. -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=11005 Chris Cormack <chris@bigballofwax.co.nz> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #22320|0 |1 is obsolete| | --- Comment #14 from Chris Cormack <chris@bigballofwax.co.nz> --- Created attachment 29269 --> http://bugs.koha-community.org/bugzilla3/attachment.cgi?id=29269&action=edit 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 -- You are receiving this mail because: You are watching all bug changes.
participants (1)
-
bugzilla-daemon＠bugs.koha-community.org