We already have a method that does the same check, Koha::Hold->is_cancelable_from_opac
Created attachment 171063 [details] [review] Bug 30856: Move CanReserveBeCanceledFromOpac to Koha::Policy
How is this patch good enough? Do we want to raise exception from Koha::Policy when the parameter are not defined? I don't think so personally it's the responsibility of the caller. Please help this pass the finish line :)
Created attachment 171064 [details] [review] Bug 30856: Move CanReserveBeCanceledFromOpac to Koha::Policy
(In reply to Jonathan Druart from comment #3) > Created attachment 171064 [details] [review] [review] > Bug 30856: Move CanReserveBeCanceledFromOpac to Koha::Policy Why are we adding a whole new module for the hold policy? Instead of fetching the patron just to compare borrowernumber, why not: my $hold = Koha::Holds->find({ reserve_id => $reserve_id, borrowernumber => $borrowernumber }); If the borrowernumebr doesn't match, we don't find a hold, can't cancel? I agree the caller can be responsible for checking params. The ILSDI call at least also seems wrong, should check: $hold->cancellation_requestable_from_opac but that's for another bug
(In reply to Nick Clemens (kidclamp) from comment #4) > (In reply to Jonathan Druart from comment #3) > > Created attachment 171064 [details] [review] [review] [review] > > Bug 30856: Move CanReserveBeCanceledFromOpac to Koha::Policy > > Why are we adding a whole new module for the hold policy? I thought it was a good idea to have Koha::Policy::Holds for global hold policies, and a more specific Koha::Policy::Patrons::Holds for the "can patron do x for holds" policies. Also with all the info (what objects) in the name of the module, the method's names are shorter and more readable (IMO): Koha::Policy::Holds->can_patron_cancel_from_opac vs Koha::Policy::Holds::Patrons->can_cancel_from_opac It's there for discussion ofc :) Happy to discuss, continue and extend this Koha::Policy further. > Instead of fetching the patron just to compare borrowernumber, why not: > my $hold = Koha::Holds->find({ reserve_id => $reserve_id, borrowernumber => > $borrowernumber }); > > If the borrowernumebr doesn't match, we don't find a hold, can't cancel? For consistency. We will want to deal with Koha::Object-based objects, and most of the time we will have them before calling the methods. We need to fetch them from opac/opac-modrequest-suspend.pl because it's badly written, because of the redirect. The idea would be to have Policy class for checkouts, holds, courses, etc. And pass the Koha::Patron object and the other entity object (always using the same pattern).
(In reply to Nick Clemens (kidclamp) from comment #4) > Instead of fetching the patron just to compare borrowernumber, why not: > my $hold = Koha::Holds->find({ reserve_id => $reserve_id, borrowernumber => > $borrowernumber }); > > If the borrowernumebr doesn't match, we don't find a hold, can't cancel? I used this pattern a lot, and I like how concise it is: ``` my $hold = $patron->holds->find( $hold_id ); ```
(In reply to Nick Clemens (kidclamp) from comment #4) > (In reply to Jonathan Druart from comment #3) > > Created attachment 171064 [details] [review] [review] [review] [review] > > Bug 30856: Move CanReserveBeCanceledFromOpac to Koha::Policy > > Why are we adding a whole new module for the hold policy? I thought it was a good idea to have Koha::Policy::Holds for global hold policies, and a more specific Koha::Policy::Patrons::Holds for the "can patron do x for holds" policies. Also with all the info (what objects) in the name of the module, the method's names are shorter and more readable (IMO): Koha::Policy::Holds->can_patron_cancel_from_opac vs Koha::Policy::Patrons::Holds->can_cancel_from_opac It's there for discussion ofc :) Happy to discuss, continue and extend this Koha::Policy further. > Instead of fetching the patron just to compare borrowernumber, why not: > my $hold = Koha::Holds->find({ reserve_id => $reserve_id, borrowernumber => > $borrowernumber }); > > If the borrowernumebr doesn't match, we don't find a hold, can't cancel? For consistency. We will want to deal with Koha::Object-based objects, and most of the time we will have them before calling the methods. We need to fetch them from opac/opac-modrequest-suspend.pl because it's badly written, because of the redirect. The idea would be to have Policy class for checkouts, holds, courses, etc. And pass the Koha::Patron object and the other entity object (always using the same pattern).
(In reply to Tomás Cohen Arazi from comment #6) > (In reply to Nick Clemens (kidclamp) from comment #4) > > Instead of fetching the patron just to compare borrowernumber, why not: > > my $hold = Koha::Holds->find({ reserve_id => $reserve_id, borrowernumber => > > $borrowernumber }); > > > > If the borrowernumebr doesn't match, we don't find a hold, can't cancel? > > I used this pattern a lot, and I like how concise it is: > > ``` > my $hold = $patron->holds->find( $hold_id ); > ``` Can you please provide a follow-up on top of the patch using that? It's not clear how you make things easiest to read, especially if you want to keep the pattern ($who, $what) in the Koha::Policy namespace. If you want to remove the id comparison it will add an unnecessary DB hit.
To clarify what may not be obvious, I think (almost?) all the occurrences of `git grep 'sub can_'` should be moved to this Koha::Policy namespace (at least it's why I introduced it earlier this year).
Rethinking about it, and maybe I now understand: actually we don't need any methods but simply one single statement. Was it what you were both saying?
Replace ToggleSuspend( $reserve_id, $suspend_until ) if CanReserveBeCanceledFromOpac($reserve_id, $borrowernumber); with my $hold = Koha::Patrons->find($borrowernumber)->holds->find($reserve_id); ToggleSuspend( $reserve_id, $suspend_until ) if $hold && $hold->is_cancelable_from_opac I thought at the beginning that there were more logic behind the "is cancelable", but it's actually trivial.
(In reply to Jonathan Druart from comment #11) > Replace... +1
Created attachment 171195 [details] [review] Bug 30856: Remove C4::Reserves::CanReserveBeCanceledFromOpac This subroutine can easily be replaced and is not really needed. Test plan: No changes expected, try to suspend/resume holds from the OPAC Note that you cannot affect somebody's else holds. Note for QA: The extra fetch of Koha::Hold will be removed on bug 37868.
Created attachment 171429 [details] [review] Bug 30856: Remove C4::Reserves::CanReserveBeCanceledFromOpac This subroutine can easily be replaced and is not really needed. Test plan: No changes expected, try to suspend/resume holds from the OPAC Note that you cannot affect somebody's else holds. Note for QA: The extra fetch of Koha::Hold will be removed on bug 37868. Signed-off-by: Olivier V <olivier.vezina@inLibro.com>
Created attachment 171473 [details] [review] Bug 30856: Remove C4::Reserves::CanReserveBeCanceledFromOpac This subroutine can easily be replaced and is not really needed. Test plan: No changes expected, try to suspend/resume holds from the OPAC Note that you cannot affect somebody's else holds. Note for QA: The extra fetch of Koha::Hold will be removed on bug 37868. Signed-off-by: Olivier V <olivier.vezina@inLibro.com> Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Interesting that we already had the right method in place, but forgot to switch. Thanks for fixing it!
Pushed for 24.11! Well done everyone, thank you!
Internal architecture, nothing to add/edit in the manual.