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)
> 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!