Let's move C4::Reserves::CancelReserve code to Koha::Hold->cancel
Created attachment 65607 [details] [review] Bug 19059: Move C4::Reserves::CancelReserve to Koha::Hold->cancel This patch adds a new Koha::Hold->cancel method and replaces the calls to C4::Reserves::CancelReserve with it. Test plan: - Add and cancel holds - Change priority of holds
Created attachment 65608 [details] [review] Bug 19059: Remove CancelReserve - move tests Move the existing tests
Created attachment 65609 [details] [review] Bug 19059: Remove CancelReserve - add new tests This is bonus, let's add new tests
Created attachment 65610 [details] [review] Bug 19059: Remove CancelReserve - do not log DELETE There is something wrong with the DELETE log, it should be replaced with a FILLED log. Anyway, here we do not want it to be logged.
Created attachment 65611 [details] [review] Bug 19059: Remove CancelReserve It's done!
Created attachment 65630 [details] [review] Bug 19059: Move C4::Reserves::CancelReserve to Koha::Hold->cancel This patch adds a new Koha::Hold->cancel method and replaces the calls to C4::Reserves::CancelReserve with it. Test plan: - Add and cancel holds - Change priority of holds
Created attachment 65631 [details] [review] Bug 19059: Remove CancelReserve - move tests Move the existing tests
Created attachment 65632 [details] [review] Bug 19059: Remove CancelReserve - add new tests This is bonus, let's add new tests
Created attachment 65633 [details] [review] Bug 19059: Remove CancelReserve - do not log DELETE There is something wrong with the DELETE log, it should be replaced with a FILLED log. Anyway, here we do not want it to be logged.
Created attachment 65634 [details] [review] Bug 19059: Remove CancelReserve It's done!
Created attachment 65636 [details] [review] [SIGNED-OFF] Bug 19059: Move C4::Reserves::CancelReserve to Koha::Hold->cancel This patch adds a new Koha::Hold->cancel method and replaces the calls to C4::Reserves::CancelReserve with it. Test plan: - Add and cancel holds - Change priority of holds Signed-off-by: Owen Leonard <oleonard@myacpl.org>
Created attachment 65637 [details] [review] [SIGNED-OFF] Bug 19059: Remove CancelReserve - move tests Move the existing tests Signed-off-by: Owen Leonard <oleonard@myacpl.org>
Created attachment 65638 [details] [review] [SIGNED-OFF] Bug 19059: Remove CancelReserve - add new tests This is bonus, let's add new tests Signed-off-by: Owen Leonard <oleonard@myacpl.org>
Created attachment 65639 [details] [review] [SIGNED-OFF] Bug 19059: Remove CancelReserve - do not log DELETE There is something wrong with the DELETE log, it should be replaced with a FILLED log. Anyway, here we do not want it to be logged. Signed-off-by: Owen Leonard <oleonard@myacpl.org>
Created attachment 65640 [details] [review] [SIGNED-OFF] Bug 19059: Remove CancelReserve It's done! Signed-off-by: Owen Leonard <oleonard@myacpl.org>
Lots of complaints from koha-qa.pl that should be easy to fix.
Created attachment 66783 [details] [review] Bug 19059: Fix compilation issues
(In reply to Kyle M Hall from comment #16) > Lots of complaints from koha-qa.pl that should be easy to fix. Ooops! Sorry about that. Fixed now.
QA: Looking here now
Created attachment 66930 [details] [review] Bug 19059: [QA Follow-up] Typo holds for hold Resolves: The method found is not covered by tests at C4/Reserves.pm line 815. Test plan: Run t/db_dependent/Holds/CancelReserves.t Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Looks good to me. Few minor points left: FIXME in sub ModReserveFill + # FIXME Must call Koha::Hold->cancel ? => No, should call ->filled and add the correct log Not clear to me. Please clarify. sub _move_to_old The name suggests a move, but you are only copying data. circ/branchtransfers.pl + } # FIXME else? Can you fix it now? You add the if. Actually, the script always assumed that CancelReserve worked. So why not only test $holds->count for the call to cancel ? Same remark for other occurrences. Incl. circ/returns.pl
Will pass qa after feedback and adjustments.
Created attachment 66975 [details] [review] Bug 19059: Move C4::Reserves::CancelReserve to Koha::Hold->cancel This patch adds a new Koha::Hold->cancel method and replaces the calls to C4::Reserves::CancelReserve with it. Test plan: - Add and cancel holds - Change priority of holds Signed-off-by: Owen Leonard <oleonard@myacpl.org> Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Created attachment 66976 [details] [review] Bug 19059: Remove CancelReserve - move tests Move the existing tests Signed-off-by: Owen Leonard <oleonard@myacpl.org> Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Created attachment 66977 [details] [review] Bug 19059: Remove CancelReserve - add new tests This is bonus, let's add new tests Signed-off-by: Owen Leonard <oleonard@myacpl.org> Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Created attachment 66978 [details] [review] Bug 19059: Remove CancelReserve - do not log DELETE There is something wrong with the DELETE log, it should be replaced with a FILLED log. Anyway, here we do not want it to be logged. Signed-off-by: Owen Leonard <oleonard@myacpl.org> Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Created attachment 66979 [details] [review] Bug 19059: Remove CancelReserve It's done! Signed-off-by: Owen Leonard <oleonard@myacpl.org> Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Created attachment 66980 [details] [review] Bug 19059: Fix compilation issues Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Created attachment 66981 [details] [review] Bug 19059: [QA Follow-up] Typo holds for hold Resolves: The method found is not covered by tests at C4/Reserves.pm line 815. Test plan: Run t/db_dependent/Holds/CancelReserves.t Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
(In reply to Marcel de Rooy from comment #22) > Will pass qa after feedback and adjustments. Added my signoff lines. Please look at comment21. In view of the fact that I will not be able to respond immediately in the coming weeks, passing QA now. The points mentioned are no real blockers..
(In reply to Marcel de Rooy from comment #21) > Looks good to me. Few minor points left: > > FIXME in sub ModReserveFill > + # FIXME Must call Koha::Hold->cancel ? => No, should call ->filled and > add the correct log > Not clear to me. Please clarify. If we call ->cancel, we will log a "cancel" log. We should call a new method ->fill[ed] to add the "FILLED" log (or whatever, but not a cancel log) > sub _move_to_old > The name suggests a move, but you are only copying data. Yes, I preferred to stick on the existing pattern (Koha::Patrons), I would like to have again 1 more subroutine of this kind to know what is best. > circ/branchtransfers.pl > + } # FIXME else? > Can you fix it now? You add the if. Actually, the script always assumed that > CancelReserve worked. So why not only test $holds->count for the call to > cancel ? > Same remark for other occurrences. Incl. circ/returns.pl I wanted to avoid a crash with the $holds->next->cancel. Moreover previous code displayed a "ok" message even if something went wrong. It may be unnecessary, but I preferred to add it.
Pushed to master for 17.11, thanks to everybody involved!
Enhancement not pushed to 17.05.x