https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=34972
--- Comment #14 from Marcel de Rooy <[email protected]> --- Hi Emily, Brave effort to try improve here :) This code is a mine field. Some small observations: my $patron = Koha::Patrons->find( $nextreservinfo ); Does not look good. We should just pass the borrowernumber to find. Cleaner code. L615 my ( $messages, $nextreservinfo ) = GetOtherReserves($reserve->{itemnumber}); Confusing that we start here with another $messages while the former one came from AddReturn. General I am just wondering how much sense it makes to still have GetOtherReserves. Couldnt we just obsolete it here now? Note that the name is quite misleading. IIUC it checks if the next reserve is waiting or transit. Around the first call I am wondering what happens now if you came from the $cancel_reserve branch of the if statement. After that you go to GetOtherReserves. Which changed behavior and does nothing now. Did it formerly handle the next reserve after the cancelled one? If I am coming from the else branch, I know if the hold is waiting or transit (look at diffBranchSend). So why still call GetOtherReserves to find out what you already know? Around L600 the second call. There is just one route to the call now. You already know again diffBranchSend. Why call GetOtherReserves? Appreciate some feedback. Thanks. -- You are receiving this mail because: You are watching all bug changes. _______________________________________________ Koha-bugs mailing list [email protected] https://lists.koha-community.org/cgi-bin/mailman/listinfo/koha-bugs website : http://www.koha-community.org/ git : http://git.koha-community.org/ bugs : http://bugs.koha-community.org/
