https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=7376 --- Comment #139 from Baptiste Wojtkowski (bwoj) <baptiste.wojtkowski@biblibre.com> --- @Marcel de Rooy Hereafter my comments to all remarks from QA concerning the provided follow up :)
In the course of time, both module and tests have been modified quite a bit over these patches. I feel that it needs a squash. Do you agree?
I think it has become necessary, hence, concerning my patch, feeld free
Please clarify this comment a bit further. It seems to be quite crucial here.
I updated a few comments in the follow up
During the development it looks like both directions were examined, later on this was reverted again? Why is the check as we do now?
I do not have access to the exact answer but since the info of the transferbranch is used by AddReturn, there is no very satisfying solution: - Either the transferbranch is calculated and transferred to CanBookBeReturned - Or the transferbranch is calculated twice
my $hbr = Koha::CirculationRules->get_return_branch_policy($item); # get the proper branch to which to return the item my $returnbranch = $hbr ne 'noreturn' ? $item->$hbr : $branch; # if $hbr was "noreturn" or any other non-item table value, then it should 'float' (i.e. stay at this branch)
[...]
# check if the return is allowed at this branch my ($returnallowed, $message) = CanBookBeReturned($item, $branch, $returnbranch);
=> Looks like we should not pass $returnbranch if it equals $branch, right?
If both branches are equal, then the transfer is possible and passing $returnbranch has no impact. I don't think it is more clear or secure if we add a check before the call.
Would it be useful to include a test case for: get_return_branch_policy noreturn: do not return, let item remain where checked in (floating collections) ?
I added the tests, but we have to test addReturn, so I added it in a separate test :) -- You are receiving this mail because: You are watching all bug changes.