Bug 35969

Summary: Improve error message, remove some logging when sending a cart from the OPAC
Product: Koha Reporter: Marcel de Rooy <m.de.rooy>
Component: OPACAssignee: Marcel de Rooy <m.de.rooy>
Status: Pushed to oldstable --- QA Contact: Emily Lamancusa <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
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
There is no need for log lines like:
[WARN] Error sending mail: empty basket at /usr/share/perl5/CGI/Compile.pm line 151.
Comment 1 Marcel de Rooy 2024-02-01 08:44:29 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>
Comment 2 Jonathan Druart 2024-02-01 08:50:02 UTC Comment hidden (obsolete)
Comment 3 Marcel de Rooy 2024-02-01 08:51:35 UTC Comment hidden (obsolete)
Comment 4 David Nind 2024-02-06 22:07:54 UTC
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>
Comment 5 David Nind 2024-02-06 22:31:39 UTC
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."
Comment 6 Marcel de Rooy 2024-02-07 13:54:19 UTC
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>
Comment 7 Kyle M Hall 2024-03-22 11:17:38 UTC
Needs a CSRF rebase. Also, the template has an else block that will never display because the script only sets two possible error values.
Comment 8 Marcel de Rooy 2024-03-25 10:01:54 UTC
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>
Comment 9 Marcel de Rooy 2024-03-25 10:01:56 UTC
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>
Comment 10 Marcel de Rooy 2024-03-25 10:04:07 UTC
(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?
Comment 11 Emily Lamancusa 2024-04-10 20:28:50 UTC
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>
Comment 12 Emily Lamancusa 2024-04-10 20:28:52 UTC
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>
Comment 13 Emily Lamancusa 2024-04-10 20:49:43 UTC
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
Comment 14 Katrin Fischer 2024-04-12 09:28:14 UTC
Pushed for 24.05!

Well done everyone, thank you!
Comment 15 Fridolin Somers 2024-05-23 12:42:37 UTC
Pushed to 23.11.x for 23.11.06
Comment 16 Lucas Gass 2024-05-28 17:59:29 UTC
Backported to 23.05.x for upcoming 23.05.12