Bug 37757 - notice_email_address explodes if EmailFieldPrimary is not valid
Summary: notice_email_address explodes if EmailFieldPrimary is not valid
Status: Pushed to main
Alias: None
Product: Koha
Classification: Unclassified
Component: Architecture, internals, and plumbing (show other bugs)
Version: unspecified
Hardware: All All
: P5 - low normal
Assignee: Tomás Cohen Arazi
QA Contact: Martin Renvoize
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2024-08-28 13:07 UTC by Tomás Cohen Arazi
Modified: 2024-09-13 19:24 UTC (History)
3 users (show)

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


Attachments
Bug 37757: Unit tests (1.65 KB, patch)
2024-08-28 18:49 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 37757: Update EmailFieldPrimary value (3.97 KB, patch)
2024-08-28 18:49 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 37757: Make notice_email_address() handle EmailFieldPrimary values more robustly (2.64 KB, patch)
2024-08-28 18:49 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 37757: (follow-up) Warn on bad values and survive (2.38 KB, patch)
2024-08-28 18:49 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 37757: Unit tests (1.69 KB, patch)
2024-09-06 23:17 UTC, David Nind
Details | Diff | Splinter Review
Bug 37757: Update EmailFieldPrimary value (4.01 KB, patch)
2024-09-06 23:17 UTC, David Nind
Details | Diff | Splinter Review
Bug 37757: Make notice_email_address() handle EmailFieldPrimary values more robustly (2.69 KB, patch)
2024-09-06 23:17 UTC, David Nind
Details | Diff | Splinter Review
Bug 37757: (follow-up) Warn on bad values and survive (2.42 KB, patch)
2024-09-06 23:17 UTC, David Nind
Details | Diff | Splinter Review
Bug 37757: Unit tests (1.76 KB, patch)
2024-09-13 11:53 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 37757: Update EmailFieldPrimary value (4.08 KB, patch)
2024-09-13 11:53 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 37757: Make notice_email_address() handle EmailFieldPrimary values more robustly (2.75 KB, patch)
2024-09-13 11:53 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 37757: (follow-up) Warn on bad values and survive (2.49 KB, patch)
2024-09-13 11:53 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 37757: (follow-up) Adjust existing tests (2.67 KB, patch)
2024-09-13 19:15 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Tomás Cohen Arazi 2024-08-28 13:07:05 UTC
Found this in the wild on `22.11`, from the AutoEmailPrimaryAddress era. Which had the syspref set to an empty string. And was able to reproduce on `main`.

To reproduce:
1. Have EmailFieldPrimary set to an empty string or NULL
   $ ktd --shell
  k$ koha-mysql kohadev
   > UPDATE systempreferences SET value='' WHERE variable='EmailFieldPrimary';
2. Enable `AutoEmailNewUser`
3. Create a new user
=> FAIL: Koha explodes like:
```
The method Koha::Patron-> is not covered by tests!

Trace begun at /kohadevbox/koha/Koha/Object.pm line 992
Koha::Object::AUTOLOAD('Koha::Patron=HASH(0xaaaae37ac550)') called at /kohadevbox/koha/Koha/Patron.pm line 1662
Koha::Patron::notice_email_address('Koha::Patron=HASH(0xaaaae37ac550)') called at /kohadevbox/koha/members/memberentry.pl line 448
...
```
Comment 1 Tomás Cohen Arazi 2024-08-28 18:49:00 UTC
Created attachment 170840 [details] [review]
Bug 37757: Unit tests
Comment 2 Tomás Cohen Arazi 2024-08-28 18:49:02 UTC
Created attachment 170841 [details] [review]
Bug 37757: Update EmailFieldPrimary value
Comment 3 Tomás Cohen Arazi 2024-08-28 18:49:05 UTC
Created attachment 170842 [details] [review]
Bug 37757: Make notice_email_address() handle EmailFieldPrimary values more robustly

This patchset makes:

* The EmailFieldPrimary value 'OFF' be replaced by an empty string
* Koha::Patron->notice_email_address() handle the empty case more
  gracefully

To test:
1. On main, manually set the EmailFieldPrimary syspref to empty
   $ ktd --shell
  k$ koha-mysql kohadev
   > UPDATE systempreferences SET value='' WHERE variable='EmailFieldPrimary';
2. Enable `AutoEmailNewUser`
3. Create a new user
=> FAIL: Koha explodes like:
```
The method Koha::Patron-> is not covered by tests!

Trace begun at /kohadevbox/koha/Koha/Object.pm line 992
Koha::Object::AUTOLOAD('Koha::Patron=HASH(0xaaaae37ac550)') called at /kohadevbox/koha/Koha/Patron.pm line 1662
Koha::Patron::notice_email_address('Koha::Patron=HASH(0xaaaae37ac550)') called at /kohadevbox/koha/members/memberentry.pl line 448
...

4. Apply this patches
5. Run:
  k$ updatedatabase
=> SUCCESS: Update succeeds
6. Restart all and retry creating a patron
=> SUCCESS: It worked!
7. Sign off :-D
Comment 4 Tomás Cohen Arazi 2024-08-28 18:49:07 UTC
Created attachment 170843 [details] [review]
Bug 37757: (follow-up) Warn on bad values and survive
Comment 5 David Nind 2024-09-06 23:17:08 UTC
Created attachment 171160 [details] [review]
Bug 37757: Unit tests

Signed-off-by: David Nind <david@davidnind.com>
Comment 6 David Nind 2024-09-06 23:17:10 UTC
Created attachment 171161 [details] [review]
Bug 37757: Update EmailFieldPrimary value

Signed-off-by: David Nind <david@davidnind.com>
Comment 7 David Nind 2024-09-06 23:17:13 UTC
Created attachment 171162 [details] [review]
Bug 37757: Make notice_email_address() handle EmailFieldPrimary values more robustly

This patchset makes:

* The EmailFieldPrimary value 'OFF' be replaced by an empty string
* Koha::Patron->notice_email_address() handle the empty case more
  gracefully

To test:
1. On main, manually set the EmailFieldPrimary syspref to empty
   $ ktd --shell
  k$ koha-mysql kohadev
   > UPDATE systempreferences SET value='' WHERE variable='EmailFieldPrimary';
2. Enable `AutoEmailNewUser`
3. Create a new user
=> FAIL: Koha explodes like:
```
The method Koha::Patron-> is not covered by tests!

Trace begun at /kohadevbox/koha/Koha/Object.pm line 992
Koha::Object::AUTOLOAD('Koha::Patron=HASH(0xaaaae37ac550)') called at /kohadevbox/koha/Koha/Patron.pm line 1662
Koha::Patron::notice_email_address('Koha::Patron=HASH(0xaaaae37ac550)') called at /kohadevbox/koha/members/memberentry.pl line 448
...

4. Apply this patches
5. Run:
  k$ updatedatabase
=> SUCCESS: Update succeeds
6. Restart all and retry creating a patron
=> SUCCESS: It worked!
7. Sign off :-D

Signed-off-by: David Nind <david@davidnind.com>
Comment 8 David Nind 2024-09-06 23:17:15 UTC
Created attachment 171163 [details] [review]
Bug 37757: (follow-up) Warn on bad values and survive

Signed-off-by: David Nind <david@davidnind.com>
Comment 9 Martin Renvoize 2024-09-13 11:53:36 UTC
Created attachment 171447 [details] [review]
Bug 37757: Unit tests

Signed-off-by: David Nind <david@davidnind.com>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 10 Martin Renvoize 2024-09-13 11:53:39 UTC
Created attachment 171448 [details] [review]
Bug 37757: Update EmailFieldPrimary value

Signed-off-by: David Nind <david@davidnind.com>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 11 Martin Renvoize 2024-09-13 11:53:41 UTC
Created attachment 171449 [details] [review]
Bug 37757: Make notice_email_address() handle EmailFieldPrimary values more robustly

This patchset makes:

* The EmailFieldPrimary value 'OFF' be replaced by an empty string
* Koha::Patron->notice_email_address() handle the empty case more
  gracefully

To test:
1. On main, manually set the EmailFieldPrimary syspref to empty
   $ ktd --shell
  k$ koha-mysql kohadev
   > UPDATE systempreferences SET value='' WHERE variable='EmailFieldPrimary';
2. Enable `AutoEmailNewUser`
3. Create a new user
=> FAIL: Koha explodes like:
```
The method Koha::Patron-> is not covered by tests!

Trace begun at /kohadevbox/koha/Koha/Object.pm line 992
Koha::Object::AUTOLOAD('Koha::Patron=HASH(0xaaaae37ac550)') called at /kohadevbox/koha/Koha/Patron.pm line 1662
Koha::Patron::notice_email_address('Koha::Patron=HASH(0xaaaae37ac550)') called at /kohadevbox/koha/members/memberentry.pl line 448
...

4. Apply this patches
5. Run:
  k$ updatedatabase
=> SUCCESS: Update succeeds
6. Restart all and retry creating a patron
=> SUCCESS: It worked!
7. Sign off :-D

Signed-off-by: David Nind <david@davidnind.com>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 12 Martin Renvoize 2024-09-13 11:53:43 UTC
Created attachment 171450 [details] [review]
Bug 37757: (follow-up) Warn on bad values and survive

Signed-off-by: David Nind <david@davidnind.com>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 13 Martin Renvoize 2024-09-13 11:54:19 UTC
Good spot, all working as described and unit tested.

Passing QA
Comment 14 Katrin Fischer 2024-09-13 14:11:59 UTC
Pushed for 24.11!

Well done everyone, thank you!
Comment 15 Katrin Fischer 2024-09-13 15:53:07 UTC
It looks like this change broke tests, please fix!

#   Failed test 'SendQueuedMessages fails when no to_address, patron notice email and RedirectGuaranteeEmail is not set'
17:11:40  koha_1       |     #   at t/db_dependent/Letters.t line 1158.
17:11:40  koha_1       |     # found warning: Invalid value for EmailFieldPrimary (OFF) at /kohadevbox/koha/Koha/Patron.pm line 1645.
17:11:40  koha_1       |     # found warning: FAIL: No 'to_address', email address or guarantors email address for borrowernumber (1606) at /kohadevbox/koha/C4/Letters.pm line 1394.
17:11:40  koha_1       |     # expected to find warning: (?^u:No 'to_address', email address or guarantors email address for borrowernumber)
17:11:40  koha_1       | 
17:11:40  koha_1       |     #   Failed test 'SendQueuedMessages is using the mocked send_or_die routine'
17:11:40  koha_1       |     #   at t/db_dependent/Letters.t line 1169.
17:11:40  koha_1       |     # found warning: Invalid value for EmailFieldPrimary (OFF) at /kohadevbox/koha/Koha/Patron.pm line 1645.
17:11:40  koha_1       |     # found warning: Invalid value for EmailFieldPrimary (OFF) at /kohadevbox/koha/Koha/Patron.pm line 1645.
17:11:40  koha_1       |     # found warning: Invalid value for EmailFieldPrimary (OFF) at /kohadevbox/koha/Koha/Patron.pm line 1645.
17:11:40  koha_1       |     # found warning: Fake send_or_die at t/db_dependent/Letters.t line 45.
17:11:40  koha_1       |     # expected to find warning: (?^u:Fake send_or_die)
17:11:40  koha_1       | 
17:11:40  koha_1       |     #   Failed test 'SendQueuedMessages is using the mocked send_or_die routine'
17:11:40  koha_1       |     #   at t/db_dependent/Letters.t line 1201.
17:11:40  koha_1       |     # found warning: Invalid value for EmailFieldPrimary (OFF) at /kohadevbox/koha/Koha/Patron.pm line 1645.
17:11:40  koha_1       |     # found warning: Invalid value for EmailFieldPrimary (OFF) at /kohadevbox/koha/Koha/Patron.pm line 1645.
17:11:40  koha_1       |     # found warning: Invalid value for EmailFieldPrimary (OFF) at /kohadevbox/koha/Koha/Patron.pm line 1645.
17:11:40  koha_1       |     # found warning: Fake send_or_die at t/db_dependent/Letters.t line 45.
17:11:40  koha_1       |     # expected to find warning: (?^u:Fake send_or_die)
17:11:40  koha_1       | 
17:11:40  koha_1       |     #   Failed test 'SendQueuedMessages is using the mocked send_or_die routine'
17:11:40  koha_1       |     #   at t/db_dependent/Letters.t line 1233.
17:11:40  koha_1       |     # found warning: Invalid value for EmailFieldPrimary (OFF) at /kohadevbox/koha/Koha/Patron.pm line 1645.
17:11:40  koha_1       |     # found warning: Invalid value for EmailFieldPrimary (OFF) at /kohadevbox/koha/Koha/Patron.pm line 1645.
17:11:40  koha_1       |     # found warning: Fake send_or_die at t/db_dependent/Letters.t line 45.
17:13:26  koha_1       |     # expected to find warning: (?^u:Fake send_or_die)
17:13:26  koha_1       |     # Looks like you failed 4 tests of 19.
17:13:26  koha_1       | 
17:13:26  koha_1       | #   Failed test 'Test guarantor handling in SendQueuedMessages'
17:13:26  koha_1       | #   at t/db_dependent/Letters.t line 1262.
17:13:26  koha_1       | 
17:13:26  koha_1       |     #   Failed test 'SendQueuedMessages with limit 1'
17:13:26  koha_1       |     #   at t/db_dependent/Letters.t line 1402.
17:13:26  koha_1       |     # found warning: Invalid value for EmailFieldPrimary (OFF) at /kohadevbox/koha/Koha/Patron.pm line 1645.
17:13:26  koha_1       |     # found warning: Fake send_or_die at t/db_dependent/Letters.t line 45.
17:13:26  koha_1       |     # expected to find warning: (?^u:Fake send_or_die)
17:13:26  koha_1       | 
17:13:26  koha_1       |     #   Failed test 'SendQueuedMessages with limit 2'
17:13:26  koha_1       |     #   at t/db_dependent/Letters.t line 1409.
17:13:26  koha_1       |     # found warning: Invalid value for EmailFieldPrimary (OFF) at /kohadevbox/koha/Koha/Patron.pm line 1645.
17:13:26  koha_1       |     # found warning: Fake send_or_die at t/db_dependent/Letters.t line 45.
17:13:26  koha_1       |     # found warning: Invalid value for EmailFieldPrimary (OFF) at /kohadevbox/koha/Koha/Patron.pm line 1645.
17:13:26  koha_1       |     # found warning: Fake send_or_die at t/db_dependent/Letters.t line 45.
17:13:26  koha_1       |     # expected to find warning: (?^u:Fake send_or_die)
17:13:26  koha_1       |     # expected to find warning: (?^u:Fake send_or_die)
17:13:26  koha_1       | 
17:13:26  koha_1       |     #   Failed test 'SendQueuedMessages with limit 3'
17:13:26  koha_1       |     #   at t/db_dependent/Letters.t line 1416.
17:13:26  koha_1       |     # found warning: Invalid value for EmailFieldPrimary (OFF) at /kohadevbox/koha/Koha/Patron.pm line 1645.
17:13:26  koha_1       |     # found warning: Fake send_or_die at t/db_dependent/Letters.t line 45.
17:13:26  koha_1       |     # found warning: Invalid value for EmailFieldPrimary (OFF) at /kohadevbox/koha/Koha/Patron.pm line 1645.
17:13:26  koha_1       |     # found warning: Fake send_or_die at t/db_dependent/Letters.t line 45.
17:13:26  koha_1       |     # expected to find warning: (?^u:Fake send_or_die)
17:13:26  koha_1       |     # expected to find warning: (?^u:Fake send_or_die)
17:13:26  koha_1       | 
17:13:26  koha_1       |     #   Failed test 'SendQueuedMessages with limit 2 and domain limits'
17:13:26  koha_1       |     #   at t/db_dependent/Letters.t line 1447.
17:13:26  koha_1       |     # found warning: Invalid value for EmailFieldPrimary (OFF) at /kohadevbox/koha/Koha/Patron.pm line 1645.
17:13:26  koha_1       |     # found warning: Sending messages: domain wrong.net reached limit of 6/1m at /kohadevbox/koha/Koha/Notice/Util.pm line 97.
17:13:26  koha_1       |     # found warning: Fake send_or_die at t/db_dependent/Letters.t line 45.
17:13:26  koha_1       |     # found warning: Invalid value for EmailFieldPrimary (OFF) at /kohadevbox/koha/Koha/Patron.pm line 1645.
17:13:26  koha_1       |     # found warning: Sending messages: domain fake1.domain reached limit of 1/1m at /kohadevbox/koha/Koha/Notice/Util.pm line 97.
17:13:26  koha_1       |     # found warning: Fake send_or_die at t/db_dependent/Letters.t line 45.
17:13:26  koha_1       |     # expected to find warning: (?^u:wrong.net reached limit)
17:13:26  koha_1       |     # expected to find warning: (?^u:Fake send_or_die)
17:13:26  koha_1       |     # expected to find warning: (?^u:fake1.domain reached limit)
17:13:26  koha_1       |     # expected to find warning: (?^u:Fake send_or_die)
17:13:26  koha_1       |     # Looks like you failed 4 tests of 18.
17:13:26  koha_1       | 
17:13:26  koha_1       | #   Failed test 'Test limit parameter for SendQueuedMessages'
17:13:26  koha_1       | #   at t/db_dependent/Letters.t line 1452.
17:13:26  koha_1       | # Looks like you failed 2 tests of 103.
17:13:26  koha_1       | [15:11:39] t/db_dependent/Letters.t .................................................... 
17:13:26  koha_1       | Dubious, test returned 2 (wstat 512, 0x200)
17:13:26  koha_1       | Failed 2/103 subtests
Comment 16 Katrin Fischer 2024-09-13 15:54:31 UTC
This one too:

17:05:38  koha_1       |     #   Failed test 'WELCOME notice Prepared, Enqueued and Send'
17:05:38  koha_1       |     #   at t/db_dependent/Auth_with_shibboleth.t line 233.
17:05:38  koha_1       |     # found warning: Invalid value for EmailFieldPrimary (OFF) at /kohadevbox/koha/Koha/Patron.pm line 1645.
17:05:38  koha_1       |     # found warning: GetPreparedLetter called at t/db_dependent/Auth_with_shibboleth.t line 62.
17:05:38  koha_1       |     # found warning: EnqueueLetter called at t/db_dependent/Auth_with_shibboleth.t line 66.
17:05:38  koha_1       |     # found warning: SendQueuedMessages called with message_id: 42 at t/db_dependent/Auth_with_shibboleth.t line 72.
17:05:38  koha_1       |     # expected to find warning: GetPreparedLetter called
17:05:38  koha_1       |     # expected to find warning: EnqueueLetter called
17:05:38  koha_1       |     # expected to find warning: SendQueuedMessages called with message_id: 42
17:05:38  koha_1       |     # Looks like you failed 1 test of 52.
17:05:38  koha_1       | 
17:05:38  koha_1       | #   Failed test 'checkpw_shib tests'
17:05:38  koha_1       | #   at t/db_dependent/Auth_with_shibboleth.t line 291.
Comment 17 Tomás Cohen Arazi 2024-09-13 19:15:55 UTC
Created attachment 171496 [details] [review]
Bug 37757: (follow-up) Adjust existing tests

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 18 Tomás Cohen Arazi 2024-09-13 19:24:00 UTC
Follow-up pushed to master. I think we might need to revisit koha-misc4dev's default configuration.