Bug 35969 - Improve error message, remove some logging when sending a cart from the OPAC
Summary: Improve error message, remove some logging when sending a cart from the OPAC
Status: Pushed to main
Alias: None
Product: Koha
Classification: Unclassified
Component: OPAC (show other bugs)
Version: Main
Hardware: All All
: P5 - low trivial (vote)
Assignee: Marcel de Rooy
QA Contact: Emily Lamancusa
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2024-02-01 07:57 UTC by Marcel de Rooy
Modified: 2024-04-12 13:27 UTC (History)
4 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Trivial patch
Documentation contact:
Documentation submission:
Text to go in the release notes:
Version(s) released in:
24.05.00


Attachments
Bug 35969: Add error code in opac-sendbasket.pl (4.22 KB, patch)
2024-02-01 08:44 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 35969: Add error code in opac-sendbasket.pl (4.23 KB, patch)
2024-02-06 22:07 UTC, David Nind
Details | Diff | Splinter Review
Bug 35969: (follow-up) Improve wording (1.82 KB, patch)
2024-02-07 13:54 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 35969: Add error code in opac-sendbasket.pl (4.25 KB, patch)
2024-03-25 10:01 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 35969: (follow-up) Improve wording (1.82 KB, patch)
2024-03-25 10:01 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 35969: Add error code in opac-sendbasket.pl (4.29 KB, patch)
2024-04-10 20:28 UTC, Emily Lamancusa
Details | Diff | Splinter Review
Bug 35969: (follow-up) Improve wording (1.85 KB, patch)
2024-04-10 20:28 UTC, Emily Lamancusa
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
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!