https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=41879
Brendan Lawlor <[email protected]> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Signed Off |Failed QA --- Comment #13 from Brendan Lawlor <[email protected]> --- One thing I noticed is the logic for moving an item level hold and for moving a bib level hold are duplicated, just inside a loop for the bib level move. It's worth considering refactoring into a helper function so that any future changes to the hold fulfillment policy logic would only need to be updated in one place. Not necessarily a blocker, but it's good to know. Hold.pm lines 217-226: # need to make sure the item is being moved to a valid pickup location my $branchitemrule = C4::Circulation::GetBranchItemRule( Koha::Policy::Holds->holds_control_library( $item, $patron ), $item->effective_itemtype ); my $policy = $branchitemrule->{hold_fulfillment_policy}; if ( ( $policy eq 'homebranch' || $policy eq 'holdingbranch' ) && $self->branchcode ne $item->$policy ) { return { success => 0, error => 'invalidPickupBranch' }; } Hold.pm lines 234-246 my $valid_pickup_item = 0; foreach my $candidate_item (@items) { my $branchitemrule = C4::Circulation::GetBranchItemRule( Koha::Policy::Holds->holds_control_library( $candidate_item, $patron ), $candidate_item->effective_itemtype ); my $policy = $branchitemrule->{hold_fulfillment_policy}; next if ( $policy eq 'homebranch' || $policy eq 'holdingbranch' ) && $self->branchcode ne $candidate_item->$policy; $valid_pickup_item = 1; last; } One other minor thing, and maybe it could be done on the follow up bug about the error message, but if you attempt to move a hold to a bib with no items the error message is misleading. One or more holds were not moved due to following errors: Hold ID: 2 Target item/record is an invalid pickup location In this case the real reason it can't be transferred is there are are no items on the bib record at all. Other than that everything works as advertised and the QA scripts are happy. The unit tests cover the scenario in the test plan. Not sure if it's really important, but the tests do not specifically cover the scenario of moving a hold to a bib with no items. -- You are receiving this mail because: You are watching all bug changes. _______________________________________________ Koha-bugs mailing list -- [email protected] To unsubscribe send an email to [email protected] website : http://www.koha-community.org/ git : http://git.koha-community.org/ bugs : http://bugs.koha-community.org/
