Description
David Cook
2021-09-17 03:03:06 UTC
Created attachment 148054 [details] [review] Bug 29046: Add 'EmailFieldPrecidence' preference This patch adds a new EmailFieldPrecidence systempreference to allow users to set the precicence in which patron email fields should be used for communications. Created attachment 148055 [details] [review] Bug 29046: Use EmailFieldPrecidence in first_valid_email_address This patch update the Koha::Patron->first_valid_email_address method to use the newly defined EmailFieldPrecidence preference. Created attachment 148056 [details] [review] Bug 29046: Add 'EmailFieldPrecidence' preference This patch adds a new EmailFieldPrecidence systempreference to allow users to set the precicence in which patron email fields should be used for communications. Created attachment 148057 [details] [review] Bug 29046: Use EmailFieldPrecedence in first_valid_email_address This patch update the Koha::Patron->first_valid_email_address method to use the newly defined EmailFieldPrecedence preference. Created attachment 148145 [details] [review] Bug 29046: Add 'EmailFieldPrecidence' preference This patch adds a new EmailFieldPrecidence systempreference to allow users to set the precicence in which patron email fields should be used for communications. Signed-off-by: Caroline Cyr La Rose <caroline.cyr-la-rose@inlibro.com> Created attachment 148146 [details] [review] Bug 29046: Use EmailFieldPrecedence in first_valid_email_address This patch update the Koha::Patron->first_valid_email_address method to use the newly defined EmailFieldPrecedence preference. Signed-off-by: Caroline Cyr La Rose <caroline.cyr-la-rose@inlibro.com> Created attachment 148147 [details] [review] Bug 29046: Change link in EmailFieldPrecedence and add link in EmailFieldPrimary This patch modifies the link in EmailFieldPrecedence to reflect the new name of EmailFieldPrimary. It also adds a note in the latter to set the 'first valid' order in the former. To test: 1. Go to Administration > Global system preferences 2. Search for EmailFieldPrecedence 3. Click on the link in the description --> It should lead to EmailFieldPrimary 4. Read the note in EmailFieldPrimary, make sure the grammar and spelling are correct. 5. Click the link to EmailFieldPrecedence --> It should lead to EmailFieldPrecedence I tested the new syspref and it works. 1. Add primary, secondary and alternate email addresses in a patron's account - Primary email = primary@... - Secondary email = secondary@... - Email, under Alternate address = alternate@... 2. Set EmailFieldPrimary to first valid 3. In the patron's account click More > Send welcome email 4. Check the message_queue, the to_address should be primary@... select to_address, message_transport_type, letter_code, time_queued from message_queue where date(time_queued) = curdate(); 5. Set EmailFieldPrecedence to emailpro|B_email|email 6. Repeat steps 3 and 4, the to_address should be secondary@... 7. Set EmailFieldPrecedence to B_email|email|emailpro 8. Repeat steps 3 and 4, the to_address should be alternate@... 9. Set EmailFieldPrecedence to email|emailpro|B_email 10. Remove the primary email from the patron's account 14. Repeat steps 3 and 4, the to_address should be secondary@... 15. Remove the secondary email from the patron's account 16. Repeat steps 3 and 4, the to_address should be alternate@... Try other orders in EmailFieldPrecedence and removing the emails in a different order. I added a link from EmailFieldPrimary to the new syspref and changed the link to reflect the changes in the dependant bug 33192... Created attachment 148155 [details] [review] Bug 29046: Change link in EmailFieldPrecedence and add link in EmailFieldPrimary This patch modifies the link in EmailFieldPrecedence to reflect the new name of EmailFieldPrimary. It also adds a note in the latter to set the 'first valid' order in the former. To test: 1. Go to Administration > Global system preferences 2. Search for EmailFieldPrecedence 3. Click on the link in the description --> It should lead to EmailFieldPrimary 4. Read the note in EmailFieldPrimary, make sure the grammar and spelling are correct. 5. Click the link to EmailFieldPrecedence --> It should lead to EmailFieldPrecedence Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Thanks for the follow-up Caroline, looks great. Signing off on that bit and updating the bug status :) Overall this looks good, but we got one blocker: 1) Unit tests are missing. 2) I wonder if we could simplify the system preference description a little: Use the following patron email fields in order of highest precedence first [email|emailpro|B_email] when selecting which email address to use to send patron notices. 2 things: * I think we should list the valid options in the system preference so that when one was deleted, it can be restored more easily. * I have no idea how to translate highest precedence... but what it means is that the first field with an email address will be used, right? Maybe we could phrase it along the lines of: When EmailFieldPrimary is set to 'first valid', check the patron email address fields in the following order and use the first valid email address found: * Not blocker: For languages we can sort the entries with drag&drop. Could this be a possible option here? Or maybe I could file as a separate bug? Created attachment 149385 [details] [review] Bug 29046: Add 'EmailFieldPrecedence' preference This patch adds a new EmailFieldPrecidence systempreference to allow users to set the precicence in which patron email fields should be used for communications. Signed-off-by: Caroline Cyr La Rose <caroline.cyr-la-rose@inlibro.com> Created attachment 149386 [details] [review] Bug 29046: Use EmailFieldPrecedence in first_valid_email_address This patch update the Koha::Patron->first_valid_email_address method to use the newly defined EmailFieldPrecedence preference. Signed-off-by: Caroline Cyr La Rose <caroline.cyr-la-rose@inlibro.com> Created attachment 149387 [details] [review] Bug 29046: Change link in EmailFieldPrecedence and add link in EmailFieldPrimary This patch modifies the link in EmailFieldPrecedence to reflect the new name of EmailFieldPrimary. It also adds a note in the latter to set the 'first valid' order in the former. To test: 1. Go to Administration > Global system preferences 2. Search for EmailFieldPrecedence 3. Click on the link in the description --> It should lead to EmailFieldPrimary 4. Read the note in EmailFieldPrimary, make sure the grammar and spelling are correct. 5. Click the link to EmailFieldPrecedence --> It should lead to EmailFieldPrecedence Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Created attachment 149959 [details] [review] Bug 29046: Add 'EmailFieldPrecedence' preference This patch adds a new EmailFieldPrecidence systempreference to allow users to set the precicence in which patron email fields should be used for communications. Signed-off-by: Caroline Cyr La Rose <caroline.cyr-la-rose@inlibro.com> Created attachment 149960 [details] [review] Bug 29046: Use EmailFieldPrecedence in first_valid_email_address This patch update the Koha::Patron->first_valid_email_address method to use the newly defined EmailFieldPrecedence preference. Signed-off-by: Caroline Cyr La Rose <caroline.cyr-la-rose@inlibro.com> Created attachment 149961 [details] [review] Bug 29046: Change link in EmailFieldPrecedence and add link in EmailFieldPrimary This patch modifies the link in EmailFieldPrecedence to reflect the new name of EmailFieldPrimary. It also adds a note in the latter to set the 'first valid' order in the former. To test: 1. Go to Administration > Global system preferences 2. Search for EmailFieldPrecedence 3. Click on the link in the description --> It should lead to EmailFieldPrimary 4. Read the note in EmailFieldPrimary, make sure the grammar and spelling are correct. 5. Click the link to EmailFieldPrecedence --> It should lead to EmailFieldPrecedence Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Created attachment 149962 [details] [review] Bug 29046: Unit tests Created attachment 149963 [details] [review] Bug 29046: Reword system preference. (In reply to Katrin Fischer from comment #11) > Overall this looks good, but we got one blocker: > > 1) Unit tests are missing. Done > 2) I wonder if we could simplify the system preference description a little: > > Use the following patron email fields in order of highest precedence first > [email|emailpro|B_email] when selecting which email address to use to send > patron notices. > > 2 things: > * I think we should list the valid options in the system preference so that > when one was deleted, it can be restored more easily. > > * I have no idea how to translate highest precedence... but what it means is > that the first field with an email address will be used, right? Maybe we > could phrase it along the lines of: > > When EmailFieldPrimary is set to 'first valid', check the patron email > address fields in the following order and use the first valid email address > found: Done > * Not blocker: For languages we can sort the entries with drag&drop. Could > this be a possible option here? Or maybe I could file as a separate bug? Please do this on another bug... this was meant as a starting point.. I don't have time to polish it any further I'm afraid. > > * Not blocker: For languages we can sort the entries with drag&drop. Could > > this be a possible option here? Or maybe I could file as a separate bug? > > Please do this on another bug... this was meant as a starting point.. I > don't have time to polish it any further I'm afraid. Specifically marked it as 'not blocker' just wanted to get your opinion. I am still not sure if you like the idea, but filed separately now: bug 33592 I have tests failing with this patch set applied that pass without it :( prove t/db_dependent/Koha/Patron.t t/db_dependent/Koha/Patrons.t t/db_dependent/Koha/Patron.t ... 10/23 Use of uninitialized value in split at /kohadevbox/koha/Koha/Patron.pm line 1440. Use of uninitialized value in split at /kohadevbox/koha/Koha/Patron.pm line 1440. t/db_dependent/Koha/Patron.t ... 13/23 WARNING: The key derivation method "opensslv1" is deprecated. Using -pbkdf=>'pbkdf2' would be better. Pass -nodeprecate=>1 to inhibit this message. at /kohadevbox/koha/Koha/Patron.pm line 2423. t/db_dependent/Koha/Patron.t ... ok t/db_dependent/Koha/Patrons.t .. 31/43 Use of uninitialized value in split at /kohadevbox/koha/Koha/Patron.pm line 1440. t/db_dependent/Koha/Patrons.t .. 32/43 Use of uninitialized value in split at /kohadevbox/koha/Koha/Patron.pm line 1440. # Failed test 'One notice queued when NotifyPasswordChange enabled' # at t/db_dependent/Koha/Patrons.t line 1972. # got: '0' # expected: '1' # Looks like you planned 16 tests but ran 15. # Looks like you failed 1 test of 15 run. # Failed test '->set_password' # at t/db_dependent/Koha/Patrons.t line 1976. Can't call method "status" on an undefined value at t/db_dependent/Koha/Patrons.t line 1974. # Looks like your test exited with 11 just after 33. t/db_dependent/Koha/Patrons.t .. Dubious, test returned 11 (wstat 2816, 0xb00) Failed 11/43 subtests Test Summary Report ------------------- t/db_dependent/Koha/Patrons.t (Wstat: 2816 Tests: 33 Failed: 1) Failed test: 33 Non-zero exit status: 11 Parse errors: Bad plan. You planned 43 tests but ran 33. My fault - it works now after running the database update :) Created attachment 150124 [details] [review] Bug 29046: Add 'EmailFieldPrecedence' preference This patch adds a new EmailFieldPrecidence systempreference to allow users to set the precicence in which patron email fields should be used for communications. Signed-off-by: Caroline Cyr La Rose <caroline.cyr-la-rose@inlibro.com> Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de> Created attachment 150125 [details] [review] Bug 29046: Use EmailFieldPrecedence in first_valid_email_address This patch update the Koha::Patron->first_valid_email_address method to use the newly defined EmailFieldPrecedence preference. Signed-off-by: Caroline Cyr La Rose <caroline.cyr-la-rose@inlibro.com> Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de> Created attachment 150126 [details] [review] Bug 29046: Change link in EmailFieldPrecedence and add link in EmailFieldPrimary This patch modifies the link in EmailFieldPrecedence to reflect the new name of EmailFieldPrimary. It also adds a note in the latter to set the 'first valid' order in the former. To test: 1. Go to Administration > Global system preferences 2. Search for EmailFieldPrecedence 3. Click on the link in the description --> It should lead to EmailFieldPrimary 4. Read the note in EmailFieldPrimary, make sure the grammar and spelling are correct. 5. Click the link to EmailFieldPrecedence --> It should lead to EmailFieldPrecedence Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de> Created attachment 150127 [details] [review] Bug 29046: Unit tests Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de> Created attachment 150128 [details] [review] Bug 29046: Reword system preference. Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de> Created attachment 150129 [details] [review] Bug 29046: Fix and amend system preference description The first line would not show in the system preference editor due to missing quotes. Also added information on the option names, link to the schema and an example. Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de> I am so excited for this one to get in. Comment on attachment 150125 [details] [review] Bug 29046: Use EmailFieldPrecedence in first_valid_email_address Review of attachment 150125 [details] [review]: ----------------------------------------------------------------- ::: Koha/Patron.pm @@ +1439,5 @@ > + > + my @fields = split /\s*\|\s*/, > + C4::Context->preference('EmailFieldPrecedence'); > + for my $field (@fields) { > + $email = $self->$field; You could crash Koha by putting junk into EmailFieldPrecedence. I think either @fields should be validated or $self->$field should be validated. Despite my above comment, I think this should go ahead. It's a valuable contribution. Pushed to master for 23.05. Nice work everyone, thanks! Enhancement - not backporting to 22.11.x *** Bug 30665 has been marked as a duplicate of this bug. *** |