Bug 19059 - Move C4::Reserves::CancelReserve to the Koha::Hold->cancel
Summary: Move C4::Reserves::CancelReserve to the Koha::Hold->cancel
Status: Pushed to Master
Alias: None
Product: Koha
Classification: Unclassified
Component: Architecture, internals, and plumbing (show other bugs)
Version: master
Hardware: All All
: P5 - low enhancement (vote)
Assignee: Jonathan Druart
QA Contact: Marcel de Rooy
URL:
Whiteboard:
Keywords:
Depends on: 19058
Blocks: 17728
  Show dependency treegraph
 
Reported: 2017-08-07 21:04 UTC by Jonathan Druart
Modified: 2017-09-12 15:46 UTC (History)
2 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Small patch
Bot Control: ---
When did the bot last check this:
Who signed the patch off:
Text to go in the release notes:


Attachments
Bug 19059: Move C4::Reserves::CancelReserve to Koha::Hold->cancel (32.69 KB, patch)
2017-08-07 21:11 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 19059: Remove CancelReserve - move tests (14.67 KB, patch)
2017-08-07 21:11 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 19059: Remove CancelReserve - add new tests (4.14 KB, patch)
2017-08-07 21:11 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 19059: Remove CancelReserve - do not log DELETE (1.36 KB, patch)
2017-08-07 21:11 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 19059: Remove CancelReserve (2.47 KB, patch)
2017-08-07 21:11 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 19059: Move C4::Reserves::CancelReserve to Koha::Hold->cancel (32.52 KB, patch)
2017-08-08 12:00 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 19059: Remove CancelReserve - move tests (14.67 KB, patch)
2017-08-08 12:00 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 19059: Remove CancelReserve - add new tests (4.14 KB, patch)
2017-08-08 12:00 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 19059: Remove CancelReserve - do not log DELETE (1.36 KB, patch)
2017-08-08 12:00 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 19059: Remove CancelReserve (2.47 KB, patch)
2017-08-08 12:00 UTC, Jonathan Druart
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 19059: Move C4::Reserves::CancelReserve to Koha::Hold->cancel (32.46 KB, patch)
2017-08-08 14:00 UTC, Owen Leonard
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 19059: Remove CancelReserve - move tests (14.71 KB, patch)
2017-08-08 14:00 UTC, Owen Leonard
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 19059: Remove CancelReserve - add new tests (4.19 KB, patch)
2017-08-08 14:00 UTC, Owen Leonard
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 19059: Remove CancelReserve - do not log DELETE (1.41 KB, patch)
2017-08-08 14:00 UTC, Owen Leonard
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 19059: Remove CancelReserve (2.52 KB, patch)
2017-08-08 14:00 UTC, Owen Leonard
Details | Diff | Splinter Review
Bug 19059: Fix compilation issues (2.15 KB, patch)
2017-09-01 19:53 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 19059: [QA Follow-up] Typo holds for hold (993 bytes, patch)
2017-09-07 12:36 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 19059: Move C4::Reserves::CancelReserve to Koha::Hold->cancel (32.54 KB, patch)
2017-09-08 07:49 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 19059: Remove CancelReserve - move tests (14.80 KB, patch)
2017-09-08 07:49 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 19059: Remove CancelReserve - add new tests (4.28 KB, patch)
2017-09-08 07:49 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 19059: Remove CancelReserve - do not log DELETE (1.49 KB, patch)
2017-09-08 07:49 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 19059: Remove CancelReserve (2.61 KB, patch)
2017-09-08 07:49 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 19059: Fix compilation issues (2.24 KB, patch)
2017-09-08 07:49 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 19059: [QA Follow-up] Typo holds for hold (993 bytes, patch)
2017-09-08 07:49 UTC, Marcel de Rooy
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Jonathan Druart 2017-08-07 21:04:19 UTC
Let's move C4::Reserves::CancelReserve code to Koha::Hold->cancel
Comment 1 Jonathan Druart 2017-08-07 21:11:42 UTC Comment hidden (obsolete)
Comment 2 Jonathan Druart 2017-08-07 21:11:46 UTC Comment hidden (obsolete)
Comment 3 Jonathan Druart 2017-08-07 21:11:49 UTC Comment hidden (obsolete)
Comment 4 Jonathan Druart 2017-08-07 21:11:53 UTC Comment hidden (obsolete)
Comment 5 Jonathan Druart 2017-08-07 21:11:56 UTC Comment hidden (obsolete)
Comment 6 Jonathan Druart 2017-08-08 12:00:20 UTC
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
Comment 7 Jonathan Druart 2017-08-08 12:00:24 UTC
Created attachment 65631 [details] [review]
Bug 19059: Remove CancelReserve - move tests

Move the existing tests
Comment 8 Jonathan Druart 2017-08-08 12:00:28 UTC
Created attachment 65632 [details] [review]
Bug 19059: Remove CancelReserve - add new tests

This is bonus, let's add new tests
Comment 9 Jonathan Druart 2017-08-08 12:00:31 UTC
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.
Comment 10 Jonathan Druart 2017-08-08 12:00:35 UTC
Created attachment 65634 [details] [review]
Bug 19059: Remove CancelReserve

It's done!
Comment 11 Owen Leonard 2017-08-08 14:00:38 UTC
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>
Comment 12 Owen Leonard 2017-08-08 14:00:41 UTC
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>
Comment 13 Owen Leonard 2017-08-08 14:00:44 UTC
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>
Comment 14 Owen Leonard 2017-08-08 14:00:47 UTC
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>
Comment 15 Owen Leonard 2017-08-08 14:00:50 UTC
Created attachment 65640 [details] [review]
[SIGNED-OFF] Bug 19059: Remove CancelReserve

It's done!

Signed-off-by: Owen Leonard <oleonard@myacpl.org>
Comment 16 Kyle M Hall 2017-09-01 18:39:57 UTC
Lots of complaints from koha-qa.pl that should be easy to fix.
Comment 17 Jonathan Druart 2017-09-01 19:53:10 UTC
Created attachment 66783 [details] [review]
Bug 19059: Fix compilation issues
Comment 18 Jonathan Druart 2017-09-01 19:53:55 UTC
(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.
Comment 19 Marcel de Rooy 2017-09-07 11:39:46 UTC
QA: Looking here now
Comment 20 Marcel de Rooy 2017-09-07 12:36:38 UTC
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>
Comment 21 Marcel de Rooy 2017-09-07 12:41:52 UTC
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
Comment 22 Marcel de Rooy 2017-09-07 12:43:39 UTC
Will pass qa after feedback and adjustments.
Comment 23 Marcel de Rooy 2017-09-08 07:49:10 UTC
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>
Comment 24 Marcel de Rooy 2017-09-08 07:49:14 UTC
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>
Comment 25 Marcel de Rooy 2017-09-08 07:49:18 UTC
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>
Comment 26 Marcel de Rooy 2017-09-08 07:49:21 UTC
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>
Comment 27 Marcel de Rooy 2017-09-08 07:49:25 UTC
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>
Comment 28 Marcel de Rooy 2017-09-08 07:49:29 UTC
Created attachment 66980 [details] [review]
Bug 19059: Fix compilation issues

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 29 Marcel de Rooy 2017-09-08 07:49:33 UTC
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>
Comment 30 Marcel de Rooy 2017-09-08 07:51:31 UTC
(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..
Comment 31 Jonathan Druart 2017-09-11 21:11:24 UTC
(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.
Comment 32 Jonathan Druart 2017-09-12 15:46:43 UTC
Pushed to master for 17.11, thanks to everybody involved!