[Bug 19059] New: Move C4::Reserves::CancelReserve to the Koha::Hold-> cancel
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=19059 Bug ID: 19059 Summary: Move C4::Reserves::CancelReserve to the Koha::Hold->cancel Change sponsored?: --- Product: Koha Version: master Hardware: All OS: All Status: ASSIGNED Severity: enhancement Priority: P5 - low Component: Architecture, internals, and plumbing Assignee: jonathan.druart@bugs.koha-community.org Reporter: jonathan.druart@bugs.koha-community.org QA Contact: testopia@bugs.koha-community.org Depends on: 19058 Blocks: 17728 Let's move C4::Reserves::CancelReserve code to Koha::Hold->cancel Referenced Bugs: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=17728 [Bug 17728] Move C4::Reserves code to the Koha namespace https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=19058 [Bug 19058] Move C4::Reserves::GetReserveId to Koha::Holds -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=19059 Jonathan Druart <jonathan.druart@bugs.koha-community.org> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|ASSIGNED |Needs Signoff -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=19059 --- Comment #1 from Jonathan Druart <jonathan.druart@bugs.koha-community.org> --- Created attachment 65607 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=65607&action=edit 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 -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=19059 --- Comment #2 from Jonathan Druart <jonathan.druart@bugs.koha-community.org> --- Created attachment 65608 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=65608&action=edit Bug 19059: Remove CancelReserve - move tests Move the existing tests -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=19059 --- Comment #3 from Jonathan Druart <jonathan.druart@bugs.koha-community.org> --- Created attachment 65609 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=65609&action=edit Bug 19059: Remove CancelReserve - add new tests This is bonus, let's add new tests -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=19059 --- Comment #4 from Jonathan Druart <jonathan.druart@bugs.koha-community.org> --- Created attachment 65610 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=65610&action=edit 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. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=19059 --- Comment #5 from Jonathan Druart <jonathan.druart@bugs.koha-community.org> --- Created attachment 65611 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=65611&action=edit Bug 19059: Remove CancelReserve It's done! -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=19059 Jonathan Druart <jonathan.druart@bugs.koha-community.org> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #65607|0 |1 is obsolete| | Attachment #65608|0 |1 is obsolete| | Attachment #65609|0 |1 is obsolete| | Attachment #65610|0 |1 is obsolete| | Attachment #65611|0 |1 is obsolete| | --- Comment #6 from Jonathan Druart <jonathan.druart@bugs.koha-community.org> --- Created attachment 65630 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=65630&action=edit 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 -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=19059 --- Comment #7 from Jonathan Druart <jonathan.druart@bugs.koha-community.org> --- Created attachment 65631 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=65631&action=edit Bug 19059: Remove CancelReserve - move tests Move the existing tests -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=19059 --- Comment #8 from Jonathan Druart <jonathan.druart@bugs.koha-community.org> --- Created attachment 65632 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=65632&action=edit Bug 19059: Remove CancelReserve - add new tests This is bonus, let's add new tests -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=19059 --- Comment #9 from Jonathan Druart <jonathan.druart@bugs.koha-community.org> --- Created attachment 65633 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=65633&action=edit 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. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=19059 --- Comment #10 from Jonathan Druart <jonathan.druart@bugs.koha-community.org> --- Created attachment 65634 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=65634&action=edit Bug 19059: Remove CancelReserve It's done! -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=19059 Owen Leonard <oleonard@myacpl.org> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Needs Signoff |Signed Off Patch complexity|--- |Small patch -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=19059 Owen Leonard <oleonard@myacpl.org> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #65630|0 |1 is obsolete| | Attachment #65631|0 |1 is obsolete| | Attachment #65632|0 |1 is obsolete| | Attachment #65633|0 |1 is obsolete| | Attachment #65634|0 |1 is obsolete| | --- Comment #11 from Owen Leonard <oleonard@myacpl.org> --- Created attachment 65636 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=65636&action=edit [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> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=19059 --- Comment #12 from Owen Leonard <oleonard@myacpl.org> --- Created attachment 65637 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=65637&action=edit [SIGNED-OFF] Bug 19059: Remove CancelReserve - move tests Move the existing tests Signed-off-by: Owen Leonard <oleonard@myacpl.org> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=19059 --- Comment #13 from Owen Leonard <oleonard@myacpl.org> --- Created attachment 65638 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=65638&action=edit [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> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=19059 --- Comment #14 from Owen Leonard <oleonard@myacpl.org> --- Created attachment 65639 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=65639&action=edit [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> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=19059 --- Comment #15 from Owen Leonard <oleonard@myacpl.org> --- Created attachment 65640 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=65640&action=edit [SIGNED-OFF] Bug 19059: Remove CancelReserve It's done! Signed-off-by: Owen Leonard <oleonard@myacpl.org> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=19059 Kyle M Hall <kyle@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Signed Off |Failed QA CC| |kyle@bywatersolutions.com --- Comment #16 from Kyle M Hall <kyle@bywatersolutions.com> --- Lots of complaints from koha-qa.pl that should be easy to fix. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=19059 --- Comment #17 from Jonathan Druart <jonathan.druart@bugs.koha-community.org> --- Created attachment 66783 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=66783&action=edit Bug 19059: Fix compilation issues -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=19059 Jonathan Druart <jonathan.druart@bugs.koha-community.org> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Failed QA |Signed Off --- Comment #18 from Jonathan Druart <jonathan.druart@bugs.koha-community.org> --- (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. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=19059 Marcel de Rooy <m.de.rooy@rijksmuseum.nl> changed: What |Removed |Added ---------------------------------------------------------------------------- QA Contact|testopia@bugs.koha-communit |m.de.rooy@rijksmuseum.nl |y.org | CC| |m.de.rooy@rijksmuseum.nl --- Comment #19 from Marcel de Rooy <m.de.rooy@rijksmuseum.nl> --- QA: Looking here now -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=19059 --- Comment #20 from Marcel de Rooy <m.de.rooy@rijksmuseum.nl> --- Created attachment 66930 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=66930&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=19059 --- Comment #21 from 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 -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=19059 --- Comment #22 from Marcel de Rooy <m.de.rooy@rijksmuseum.nl> --- Will pass qa after feedback and adjustments. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=19059 Marcel de Rooy <m.de.rooy@rijksmuseum.nl> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Signed Off |Passed QA -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=19059 Marcel de Rooy <m.de.rooy@rijksmuseum.nl> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #65636|0 |1 is obsolete| | Attachment #65637|0 |1 is obsolete| | Attachment #65638|0 |1 is obsolete| | Attachment #65639|0 |1 is obsolete| | Attachment #65640|0 |1 is obsolete| | Attachment #66783|0 |1 is obsolete| | Attachment #66930|0 |1 is obsolete| | --- Comment #23 from Marcel de Rooy <m.de.rooy@rijksmuseum.nl> --- Created attachment 66975 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=66975&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=19059 --- Comment #24 from Marcel de Rooy <m.de.rooy@rijksmuseum.nl> --- Created attachment 66976 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=66976&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=19059 --- Comment #25 from Marcel de Rooy <m.de.rooy@rijksmuseum.nl> --- Created attachment 66977 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=66977&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=19059 --- Comment #26 from Marcel de Rooy <m.de.rooy@rijksmuseum.nl> --- Created attachment 66978 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=66978&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=19059 --- Comment #27 from Marcel de Rooy <m.de.rooy@rijksmuseum.nl> --- Created attachment 66979 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=66979&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=19059 --- Comment #28 from Marcel de Rooy <m.de.rooy@rijksmuseum.nl> --- Created attachment 66980 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=66980&action=edit Bug 19059: Fix compilation issues Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=19059 --- Comment #29 from Marcel de Rooy <m.de.rooy@rijksmuseum.nl> --- Created attachment 66981 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=66981&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=19059 --- Comment #30 from 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.. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=19059 --- Comment #31 from Jonathan Druart <jonathan.druart@bugs.koha-community.org> --- (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. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=19059 Jonathan Druart <jonathan.druart@bugs.koha-community.org> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Passed QA |Pushed to Master --- Comment #32 from Jonathan Druart <jonathan.druart@bugs.koha-community.org> --- Pushed to master for 17.11, thanks to everybody involved! -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=19059 Bug 19059 depends on bug 19058, which changed state. Bug 19058 Summary: Move C4::Reserves::GetReserveId to Koha::Holds https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=19058 What |Removed |Added ---------------------------------------------------------------------------- Status|Pushed to Master |RESOLVED Resolution|--- |FIXED -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=19059 Fridolin SOMERS <fridolin.somers@biblibre.com> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |fridolin.somers@biblibre.co | |m Resolution|--- |FIXED Status|Pushed to Master |RESOLVED --- Comment #33 from Fridolin SOMERS <fridolin.somers@biblibre.com> --- Enhancement not pushed to 17.05.x -- You are receiving this mail because: You are watching all bug changes.
participants (1)
-
bugzilla-daemon＠bugs.koha-community.org