[Bug 30856] New: Remove CanReserveBeCanceledFromOpac
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=30856 Bug ID: 30856 Summary: Remove CanReserveBeCanceledFromOpac Change sponsored?: --- Product: Koha Version: unspecified Hardware: All OS: All Status: NEW Severity: enhancement Priority: P5 - low Component: Architecture, internals, and plumbing Assignee: koha-bugs@lists.koha-community.org Reporter: tomascohen@gmail.com QA Contact: testopia@bugs.koha-community.org We already have a method that does the same check, Koha::Hold->is_cancelable_from_opac -- You are receiving this mail because: You are the assignee for the bug. You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=30856 Tomás Cohen Arazi <tomascohen@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Assignee|koha-bugs@lists.koha-commun |tomascohen@gmail.com |ity.org | Status|NEW |ASSIGNED Depends on| |17728 Referenced Bugs: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=17728 [Bug 17728] Move C4::Reserves code to the Koha namespace -- You are receiving this mail because: You are watching all bug changes. You are the assignee for the bug.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=30856 --- Comment #1 from Jonathan Druart <jonathan.druart@gmail.com> --- Created attachment 171063 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=171063&action=edit Bug 30856: Move CanReserveBeCanceledFromOpac to Koha::Policy -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=30856 Jonathan Druart <jonathan.druart@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |jonathan.druart@gmail.com Status|ASSIGNED |In Discussion --- Comment #2 from Jonathan Druart <jonathan.druart@gmail.com> --- 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 :) -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=30856 Jonathan Druart <jonathan.druart@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #171063|0 |1 is obsolete| | --- Comment #3 from Jonathan Druart <jonathan.druart@gmail.com> --- Created attachment 171064 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=171064&action=edit Bug 30856: Move CanReserveBeCanceledFromOpac to Koha::Policy -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=30856 Nick Clemens (kidclamp) <nick@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |nick@bywatersolutions.com --- Comment #4 from Nick Clemens (kidclamp) <nick@bywatersolutions.com> --- (In reply to Jonathan Druart from comment #3)
Created attachment 171064 [details] [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 -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=30856 --- Comment #5 from Jonathan Druart <jonathan.druart@gmail.com> --- (In reply to Nick Clemens (kidclamp) from comment #4)
(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?
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). -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=30856 Tomás Cohen Arazi <tomascohen@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |tomascohen@gmail.com --- Comment #6 from Tomás Cohen Arazi <tomascohen@gmail.com> --- (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 ); ``` -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=30856 --- Comment #7 from Jonathan Druart <jonathan.druart@gmail.com> --- (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::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). -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=30856 --- Comment #8 from Jonathan Druart <jonathan.druart@gmail.com> --- (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. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=30856 --- Comment #9 from Jonathan Druart <jonathan.druart@gmail.com> --- 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). -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=30856 --- Comment #10 from Jonathan Druart <jonathan.druart@gmail.com> --- 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? -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=30856 --- Comment #11 from Jonathan Druart <jonathan.druart@gmail.com> --- 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. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=30856 --- Comment #12 from Nick Clemens (kidclamp) <nick@bywatersolutions.com> --- (In reply to Jonathan Druart from comment #11)
Replace...
+1 -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=30856 Jonathan Druart <jonathan.druart@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|In Discussion |Needs Signoff -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=30856 Jonathan Druart <jonathan.druart@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #171064|0 |1 is obsolete| | --- Comment #13 from Jonathan Druart <jonathan.druart@gmail.com> --- Created attachment 171195 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=171195&action=edit 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. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=30856 Jonathan Druart <jonathan.druart@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Depends on|17728 | Blocks| |17728 Referenced Bugs: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=17728 [Bug 17728] [Omnibus] Move C4::Reserves code to the Koha namespace -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=30856 Jonathan Druart <jonathan.druart@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |paul.derscheid@lmscloud.de -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=30856 Jonathan Druart <jonathan.druart@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Assignee|tomascohen@gmail.com |jonathan.druart@gmail.com -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=30856 Olivier V <ovezina@inlibro.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #171195|0 |1 is obsolete| | --- Comment #14 from Olivier V <ovezina@inlibro.com> --- Created attachment 171429 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=171429&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=30856 Olivier V <ovezina@inlibro.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Needs Signoff |Signed Off CC| |ovezina@inlibro.com -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=30856 Tomás Cohen Arazi <tomascohen@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Patch complexity|--- |Trivial patch 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=30856 Tomás Cohen Arazi <tomascohen@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #171429|0 |1 is obsolete| | --- Comment #15 from Tomás Cohen Arazi <tomascohen@gmail.com> --- Created attachment 171473 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=171473&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=30856 Tomás Cohen Arazi <tomascohen@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Blocks| |37868 Referenced Bugs: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=37868 [Bug 37868] Remove C4::Reserves::ToggleSuspend -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=30856 --- Comment #16 from Katrin Fischer <katrin.fischer@bsz-bw.de> --- Interesting that we already had the right method in place, but forgot to switch. Thanks for fixing it! -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=30856 Katrin Fischer <katrin.fischer@bsz-bw.de> changed: What |Removed |Added ---------------------------------------------------------------------------- Version(s)| |24.11.00 released in| | Status|Passed QA |Pushed to main -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=30856 --- Comment #17 from Katrin Fischer <katrin.fischer@bsz-bw.de> --- Pushed for 24.11! Well done everyone, thank you! -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=30856 Martin Renvoize (ashimema) <martin.renvoize@ptfs-europe.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Keywords| |release-notes-needed -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=30856 Jonathan Druart <jonathan.druart@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Keywords|release-notes-needed | -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=30856 Lucas Gass (lukeg) <lucas@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Pushed to main |RESOLVED Resolution|--- |FIXED CC| |lucas@bywatersolutions.com -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=30856 Katrin Fischer <katrin.fischer@bsz-bw.de> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|RESOLVED |Needs documenting Resolution|FIXED |--- -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=30856 Caroline Cyr La Rose <caroline.cyr-la-rose@inlibro.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Resolution|--- |FIXED Status|Needs documenting |RESOLVED CC| |caroline.cyr-la-rose@inlibr | |o.com --- Comment #18 from Caroline Cyr La Rose <caroline.cyr-la-rose@inlibro.com> --- Internal architecture, nothing to add/edit in the manual. -- You are receiving this mail because: You are watching all bug changes.
participants (1)
-
bugzilla-daemon＠bugs.koha-community.org