Summary: | A patron can set everybody's checkout notes | ||
---|---|---|---|
Product: | Koha | Reporter: | Joonas Kylmälä <joonas.kylmala> |
Component: | OPAC | Assignee: | Jonathan Druart <jonathan.druart> |
Status: | Pushed to oldoldoldstable --- | QA Contact: | Tomás Cohen Arazi (tcohen) <tomascohen> |
Severity: | major | ||
Priority: | P5 - low | CC: | 1joynelson, andrew, fridolin.somers, jonathan.druart, lucas, m.de.rooy, nick, nugged, tomascohen, victor, wainuiwitikapark |
Version: | Main | ||
Hardware: | All | ||
OS: | All | ||
GIT URL: | Change sponsored?: | --- | |
Patch complexity: | Trivial patch | Documentation contact: | |
Documentation submission: | Text to go in the release notes: | ||
Version(s) released in: |
22.05.00,21.11.02,21.05.09,20.11.14,19.11.25
|
Circulation function: | |
Attachments: |
Bug 29544: Ensure logged in user is allowed to modify checkout note
Bug 29544: Fix opac-issue-note.pl Bug 29544: Ensure logged in user is allowed to modify checkout note Bug 29544: Fix opac-issue-note.pl Bug 29544: Ensure logged in user is allowed to modify checkout note Bug 29544: Fix opac-issue-note.pl [OPTIONAL] Bug 29544: (QA follow-up) Simplify code |
Description
Joonas Kylmälä
2021-11-20 21:19:35 UTC
Created attachment 127930 [details] [review] Bug 29544: Ensure logged in user is allowed to modify checkout note This patch is not great and keep the script weak. Anything else to suggest? Could we compare logged-in borrowernumber with issue's one ? Like in opac/svc/checkout_notes : https://git.koha-community.org/Koha-community/Koha/src/commit/5b9902487b76d0adcd2e40442165171b5942af46/opac/svc/checkout_notes#L73 My patch actually modifies opac/svc/checkout_notes, that was impacted as well, even with the test on borrowernumber. opac/opac-issue-note.pl is used when JS is disabled. Code is duplicated and buggy, we need to provide a simple fix for backporting but a refactoring should be considered. Created attachment 128148 [details] [review] Bug 29544: Fix opac-issue-note.pl We must check if logged in user is trying to modify one of their checkouts +if ( $action eq 'issuenote' && C4::Context->preference('AllowCheckoutNotes') && $issue ) { Do we need this ? There is an early exit "if !$issue" Created attachment 128946 [details] [review] Bug 29544: Ensure logged in user is allowed to modify checkout note Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de> Created attachment 128947 [details] [review] Bug 29544: Fix opac-issue-note.pl We must check if logged in user is trying to modify one of their checkouts Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de> (In reply to Fridolin Somers from comment #6) > +if ( $action eq 'issuenote' && > C4::Context->preference('AllowCheckoutNotes') && $issue ) { > > Do we need this ? > There is an early exit "if !$issue" I see what you mean, but on the other side it doesn't hurt, right? (In reply to Katrin Fischer from comment #9) > (In reply to Fridolin Somers from comment #6) > > +if ( $action eq 'issuenote' && > > C4::Context->preference('AllowCheckoutNotes') && $issue ) { > > > > Do we need this ? > > There is an early exit "if !$issue" > > I see what you mean, but on the other side it doesn't hurt, right? It does not ;) Some notes: - Works as expected - Shouldn't we actually remove the 'hidden' feature completely? Created attachment 129336 [details] [review] Bug 29544: Ensure logged in user is allowed to modify checkout note Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de> Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Created attachment 129337 [details] [review] Bug 29544: Fix opac-issue-note.pl We must check if logged in user is trying to modify one of their checkouts Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de> Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Created attachment 129338 [details] [review] [OPTIONAL] Bug 29544: (QA follow-up) Simplify code I think this is a better approach for the same thing. Posting it just in case it helps. Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> (In reply to Tomás Cohen Arazi from comment #14) > Created attachment 129338 [details] [review] [review] > [OPTIONAL] Bug 29544: (QA follow-up) Simplify code > > I think this is a better approach for the same thing. Posting it just in > case it helps. > > Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Ha nice, i like this new approach. Backported: Pushed to 20.11.x-security branch for 20.11.14 Even with the optional patch, it's applies perfectly without conflicts on 20.11.x :) Backported to 19.11.x-security branch on the Security repo for 19.11.25 security release. Pushed to security/21.05.x for 21.05.09 |