| Summary: | Improve error message, remove some logging when sending a cart from the OPAC | ||
|---|---|---|---|
| Product: | Koha | Reporter: | Marcel de Rooy <m.de.rooy> |
| Component: | OPAC | Assignee: | Marcel de Rooy <m.de.rooy> |
| Status: | Pushed to oldstable --- | QA Contact: | Emily Lamancusa (emlam) <emily.lamancusa> |
| Severity: | trivial | ||
| Priority: | P5 - low | CC: | david, emily.lamancusa, fridolin.somers, jonathan.druart, kyle, lucas |
| Version: | Main | ||
| Hardware: | All | ||
| OS: | All | ||
| Change sponsored?: | --- | Patch complexity: | Trivial patch |
| Documentation contact: | Documentation submission: | ||
| Text to go in the release notes: | Version(s) released in: |
24.05.00,23.11.06,23.05.12
|
|
| Circulation function: | |||
| Attachments: |
Bug 35969: Add error code in opac-sendbasket.pl
Bug 35969: Add error code in opac-sendbasket.pl Bug 35969: (follow-up) Improve wording Bug 35969: Add error code in opac-sendbasket.pl Bug 35969: (follow-up) Improve wording Bug 35969: Add error code in opac-sendbasket.pl Bug 35969: (follow-up) Improve wording |
||
|
Description
Marcel de Rooy
2024-02-01 07:57:39 UTC
Created attachment 161713 [details] [review] Bug 35969: Add error code in opac-sendbasket.pl No need to carp about empty basket or no reply address. Just add a simple explanation on the form. Note that it makes no sense to show the form if the patron has no email address. We need it as reply-to. Test plan: Try opac-sendbasket.pl?bib_list=null Remove email address from your account, try again. Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> This will conflict with bug 34478. Please keep in mind that we have something HUGE in the queue that MUST be high priority for everybody and pushed as soon as possible. Those kind of patches should definitely be delayed until 34478 is pushed. Marked as blocked. (In reply to Jonathan Druart from comment #2) > This will conflict with bug 34478. Please keep in mind that we have > something HUGE in the queue that MUST be high priority for everybody and > pushed as soon as possible. Those kind of patches should definitely be > delayed until 34478 is pushed. > > Marked as blocked. No reason to block every patch for something In discussion, right? Created attachment 161795 [details] [review] Bug 35969: Add error code in opac-sendbasket.pl No need to carp about empty basket or no reply address. Just add a simple explanation on the form. Note that it makes no sense to show the form if the patron has no email address. We need it as reply-to. Test plan: Try opac-sendbasket.pl?bib_list=null Remove email address from your account, try again. Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Signed-off-by: David Nind <david@davidnind.com> I've signed off, as I think having better messages is great. Some suggestions: There was an error sending the cart: No valid biblio records found. > There was an error sending the cart: no valid records were found. Since we do not have your email address, we cannot send a mail. We need your email address as reply-to address. > There was an error sending the cart: we cannot send an email as there is no email address recorded for your account (your email address is used for the reply to address). Testing notes (using KTD): 1. Go to http://127.0.0.1:8080/cgi-bin/koha/opac-sendbasket.pl?bib_list=null 2. Log in as koha/koha 3. Add an email address to the form and click 'Send': me@example.com 4. Error message displayed: "There was an error sending the cart." 5. Note the warning in the log: /var/log/koha/kohadev/plack-opac-error.log 6. Apply the patch 7. Repeat steps 1-3 8. Note that the error displayed is now: "Since we do not have your email address, we cannot send a mail. We need your email address as reply-to address." Created attachment 161804 [details] [review] Bug 35969: (follow-up) Improve wording As suggested by David Nind. Thanks! Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Needs a CSRF rebase. Also, the template has an else block that will never display because the script only sets two possible error values. Created attachment 163773 [details] [review] Bug 35969: Add error code in opac-sendbasket.pl No need to carp about empty basket or no reply address. Just add a simple explanation on the form. Note that it makes no sense to show the form if the patron has no email address. We need it as reply-to. Test plan: Try opac-sendbasket.pl?bib_list=null Remove email address from your account, try again. Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Signed-off-by: David Nind <david@davidnind.com> Created attachment 163774 [details] [review] Bug 35969: (follow-up) Improve wording As suggested by David Nind. Thanks! Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> (In reply to Kyle M Hall from comment #7) > Needs a CSRF rebase. Also, the template has an else block that will never > display because the script only sets two possible error values. Rebased. Yes you are right about this ELSE + [% ELSE %] + <p>There was an error sending the cart.</p> Just added for completeness. Things change over time and this catches the rest.. Doesnt hurt imo to keep it? Created attachment 164635 [details] [review] Bug 35969: Add error code in opac-sendbasket.pl No need to carp about empty basket or no reply address. Just add a simple explanation on the form. Note that it makes no sense to show the form if the patron has no email address. We need it as reply-to. Test plan: Try opac-sendbasket.pl?bib_list=null Remove email address from your account, try again. Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Signed-off-by: David Nind <david@davidnind.com> Signed-off-by: Emily Lamancusa <emily.lamancusa@montgomerycountymd.gov> Created attachment 164636 [details] [review] Bug 35969: (follow-up) Improve wording As suggested by David Nind. Thanks! Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Signed-off-by: Emily Lamancusa <emily.lamancusa@montgomerycountymd.gov> I agree with keeping the ELSE as a fallback - it doesn't hurt and it does make the code more robust. I squashed a capitalization fix into the follow-up, but otherwise it looks good, and passes the QA script. Setting to PQA Pushed for 24.05! Well done everyone, thank you! Pushed to 23.11.x for 23.11.06 Backported to 23.05.x for upcoming 23.05.12 |