|
Description
Jonathan Druart
2017-08-07 21:04:19 UTC
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 |