Bug 33223 - Koha::Patron->notice_email_address isn't consistently used
Summary: Koha::Patron->notice_email_address isn't consistently used
Status: RESOLVED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Notices (show other bugs)
Version: unspecified
Hardware: All All
: P5 - low normal (vote)
Assignee: Martin Renvoize
QA Contact: Marcel de Rooy
URL:
Keywords:
Depends on: 3150 18398 33192
Blocks: 12532
  Show dependency treegraph
 
Reported: 2023-03-14 11:14 UTC by Martin Renvoize
Modified: 2024-02-06 23:11 UTC (History)
5 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:
23.05.00


Attachments
Bug 33223: Replace 'first_valid' with 'notice' for email addresses (5.34 KB, patch)
2023-03-14 15:09 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 33223: Replace 'first_valid' with 'notice' for email addresses (5.49 KB, patch)
2023-05-10 16:17 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 33223: Replace 'first_valid' with 'notice' for email addresses (5.97 KB, patch)
2023-05-10 17:03 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 33223: Fix sendshelf (1.51 KB, patch)
2023-05-10 17:03 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 33223: Replace 'first_valid' with 'notice' for email addresses (6.02 KB, patch)
2023-05-11 03:19 UTC, David Cook
Details | Diff | Splinter Review
Bug 33223: Fix sendshelf (1.57 KB, patch)
2023-05-11 03:19 UTC, David Cook
Details | Diff | Splinter Review
Bug 33223: Replace 'first_valid' with 'notice' for email addresses (6.11 KB, patch)
2023-05-12 07:42 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 33223: Fix sendshelf (1.66 KB, patch)
2023-05-12 07:42 UTC, Marcel de Rooy
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Martin Renvoize 2023-03-14 11:14:49 UTC
As highlighted in bug 33192, we're not consistently using the Koha::Patron->notice_email_address method to get our 'to' address for notices.

This bug serves to catch the last remaining cases where it's not being used and should be.

Copying the list from the above bug, notice_email_address is used in:

C4::Auth_With_shibboleth
C4::Letters (as the default)
C4::Auth - For 2FA
C4::Auth_with_ldap
C4::Reserves
shareshelf.pl
memberentry.pl
opac-registration-varify.pl
two_factor_auth.pl
notice.pl
memberentry.pl
pendingreserves.pl
Koha::Ticket
Patron::Import.pm
suggestion.pl
overdue_notices.pl
TalkingTech_iteive_outbound.pl
notice_unprocessed_suggestions.pm

first_valid_email is used in:

clubs-tab.tt
waiting_holds.inc
transferstoreceive.tt
opac-sendbasket.pl
Koha::SharedContent
Comment 1 Martin Renvoize 2023-03-14 15:09:03 UTC
Created attachment 148168 [details] [review]
Bug 33223: Replace 'first_valid' with 'notice' for email addresses

This patch replaces the uses of first_valid_email_address with
notice_email_address in waiting_holds, transferstoreceive, clubs and
sendbasket so that we take EmailFieldPrimary into account for these
notices too.
Comment 2 David Cook 2023-03-28 12:57:12 UTC
I'll be wanting to look more at these. Sounds very useful.
Comment 3 David Cook 2023-05-03 04:55:19 UTC
Bug 33223 - Koha::Patron->notice_email_address isn't consistently used

148168 - Bug 33223: Replace 'first_valid' with 'notice' for email addresses

Apply? [(y)es, (n)o, (i)nteractive] y
Applying: Bug 33223: Replace 'first_valid' with 'notice' for email addresses
Using index info to reconstruct a base tree...
M       koha-tmpl/intranet-tmpl/prog/en/modules/circ/transferstoreceive.tt
M       opac/opac-sendbasket.pl
Falling back to patching base and 3-way merge...
Auto-merging opac/opac-sendbasket.pl
CONFLICT (content): Merge conflict in opac/opac-sendbasket.pl
Auto-merging koha-tmpl/intranet-tmpl/prog/en/modules/circ/transferstoreceive.tt
error: Failed to merge in the changes.
Patch failed at 0001 Bug 33223: Replace 'first_valid' with 'notice' for email addresses
hint: Use 'git am --show-current-patch' to see the failed patch
When you have resolved this problem run "git bz apply --continue".
If you would prefer to skip this patch, instead run "git bz apply --skip".
To restore the original branch and stop patching run "git bz apply --abort".
Patch left in /tmp/Bug-33223-Replace-firstvalid-with-notice-for-email-t8SwfM.patch
Comment 4 Martin Renvoize 2023-05-10 16:17:42 UTC
Created attachment 151023 [details] [review]
Bug 33223: Replace 'first_valid' with 'notice' for email addresses

This patch replaces the uses of first_valid_email_address with
notice_email_address in waiting_holds, transferstoreceive, clubs and
sendbasket so that we take EmailFieldPrimary into account for these
notices too.
Comment 5 Martin Renvoize 2023-05-10 17:03:39 UTC
Created attachment 151024 [details] [review]
Bug 33223: Replace 'first_valid' with 'notice' for email addresses

This patch replaces the uses of first_valid_email_address with
notice_email_address in waiting_holds, transferstoreceive, clubs and
sendbasket so that we take EmailFieldPrimary into account for these
notices too.
Comment 6 Martin Renvoize 2023-05-10 17:03:42 UTC
Created attachment 151025 [details] [review]
Bug 33223: Fix sendshelf

Two new occurences of first_valid crept in, this patch corrects those to
also use notice_
Comment 7 David Cook 2023-05-11 03:19:48 UTC
Created attachment 151041 [details] [review]
Bug 33223: Replace 'first_valid' with 'notice' for email addresses

This patch replaces the uses of first_valid_email_address with
notice_email_address in waiting_holds, transferstoreceive, clubs and
sendbasket so that we take EmailFieldPrimary into account for these
notices too.

Signed-off-by: David Cook <dcook@prosentient.com.au>
Comment 8 David Cook 2023-05-11 03:19:51 UTC
Created attachment 151042 [details] [review]
Bug 33223: Fix sendshelf

Two new occurences of first_valid crept in, this patch corrects those to
also use notice_

Signed-off-by: David Cook <dcook@prosentient.com.au>
Comment 9 David Cook 2023-05-11 03:20:39 UTC
Patches apply and change looks good upon review.
Comment 10 Marcel de Rooy 2023-05-12 07:19:58 UTC
Looking here, although I feel that this patch is somewhere between bug and enh actually.
Comment 11 Marcel de Rooy 2023-05-12 07:34:45 UTC
Looking at what remains for first_valid_email i would opt for removing the OFF option of EmailFieldPrimary and default it to the email field.

Here it is only used as boolean:
$params->{is_email_required} = 0 unless $self->first_valid_email_address();

Koha/SharedContent.pm:        $mana_email = $borrower->first_valid_email_address
This one could be replaced imo by notice_email.

But yes, not here :)
Comment 12 Marcel de Rooy 2023-05-12 07:42:45 UTC
Created attachment 151117 [details] [review]
Bug 33223: Replace 'first_valid' with 'notice' for email addresses

This patch replaces the uses of first_valid_email_address with
notice_email_address in waiting_holds, transferstoreceive, clubs and
sendbasket so that we take EmailFieldPrimary into account for these
notices too.

Signed-off-by: David Cook <dcook@prosentient.com.au>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 13 Marcel de Rooy 2023-05-12 07:42:48 UTC
Created attachment 151118 [details] [review]
Bug 33223: Fix sendshelf

Two new occurences of first_valid crept in, this patch corrects those to
also use notice_

Signed-off-by: David Cook <dcook@prosentient.com.au>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 14 Tomás Cohen Arazi 2023-05-16 18:19:22 UTC
Pushed to master for 23.05.

Nice work everyone, thanks!
Comment 15 Matt Blenkinsop 2023-06-12 10:06:45 UTC
Missing dependencies - not backporting to 22.11.x