Bug 32357 - Set borrower_message_preferences.days_in_advance default to NULL
Summary: Set borrower_message_preferences.days_in_advance default to NULL
Status: RESOLVED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Database (show other bugs)
Version: unspecified
Hardware: All All
: P5 - low normal (vote)
Assignee: Lari Taskula
QA Contact: Kyle M Hall
URL:
Keywords:
Depends on:
Blocks: 18595
  Show dependency treegraph
 
Reported: 2022-11-28 21:56 UTC by Lari Taskula
Modified: 2023-12-28 20:44 UTC (History)
3 users (show)

See Also:
Change sponsored?: ---
Patch complexity: ---
Documentation contact:
Documentation submission:
Text to go in the release notes:
This fixes the default value in the database for the 'Days in advance' field for patron messaging preferences so that it defaults to NULL instead of 0 (borrower_message_preferences table and the days_in_advance field).
Version(s) released in:
23.05.00,22.11.06


Attachments
Bug 32357: Set borrower_message_preferences.days_in_advance default to NULL (1.84 KB, patch)
2022-11-28 22:04 UTC, Lari Taskula
Details | Diff | Splinter Review
Bug 32357: Set borrower_message_preferences.days_in_advance default to NULL (2.08 KB, patch)
2022-11-28 22:26 UTC, Lari Taskula
Details | Diff | Splinter Review
Bug 32357: Set borrower_message_preferences.days_in_advance default to NULL (2.07 KB, patch)
2023-02-10 13:35 UTC, Lari Taskula
Details | Diff | Splinter Review
Bug 32357: Set borrower_message_preferences.days_in_advance default to NULL (2.12 KB, patch)
2023-04-05 20:55 UTC, David Nind
Details | Diff | Splinter Review
Bug 32357: Set borrower_message_preferences.days_in_advance default to NULL (2.17 KB, patch)
2023-04-28 17:38 UTC, Kyle M Hall
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Lari Taskula 2022-11-28 21:56:46 UTC
days_in_advance currently defaults to 0, but it should default to NULL because all but one entry in message_attribute has takes_days=1 making it the exception and not the default.

Current column default 0 is already ignored by C4::Members::Messaging::SetMessagingPreference, see https://github.com/Koha-Community/Koha/blob/8c367775470fcecdda54e28270b05d92834cd33f/C4/Members/Messaging.pm#L134 , so we should also make NULL default at database level instead.

Blocking Bug 18595 (because without this change some tests there will fail unless we replicate the misuse of default values)
Comment 1 Lari Taskula 2022-11-28 21:58:54 UTC
(In reply to Lari Taskula from comment #0)
> because all but one entry in message_attribute has takes_days=1 making it
> the exception and not the default.

I of course meant to say that only one entry in message_attribute has takes_days=1 making it the exception and not the default.
Comment 2 Lari Taskula 2022-11-28 22:04:16 UTC
Created attachment 144307 [details] [review]
Bug 32357: Set borrower_message_preferences.days_in_advance default to NULL
Comment 3 Lari Taskula 2022-11-28 22:23:41 UTC
Not sure how to test it thoroughly, but any case using SetMessagingPreference() should not be affected as mentioned in comment #1 - as far as I understand only direct SQL INSERT to borrower_message_preferences and inserts with DBIx object (or TestBuilder) may cause regression. I found five tests making such insertions using
$ grep -rn 'BorrowerMessagePreference'
and
$ grep -rn 'borrower_message_preferences'

t/db_dependent/cronjobs/advance_notices_digest.t
t/db_dependent/Letters/TemplateToolkit.t
t/db_dependent/Koha/CurbsidePickups.t
t/db_dependent/Koha/Patrons.t
t/db_dependent/Patron/Messaging.t

I'll add these to the patch description.
Comment 4 Lari Taskula 2022-11-28 22:26:37 UTC
Created attachment 144309 [details] [review]
Bug 32357: Set borrower_message_preferences.days_in_advance default to NULL

To test:
1. prove t/db_dependent/cronjobs/advance_notices_digest.t
2. prove t/db_dependent/Letters/TemplateToolkit.t
3. prove t/db_dependent/Koha/CurbsidePickups.t
4. prove t/db_dependent/Koha/Patrons.t
5. prove t/db_dependent/Patron/Messaging.t
Comment 5 David Nind 2022-11-28 23:55:08 UTC
Tests pass both before and after the patch is applied.

However, I don't think the databaseupdate updates existing installations. I had to do a reset_all to replicate a new installation (using koha-testing-docker).

Not that I really know how all this should work!

Testing notes:
1. Access the database: koha-mysql kohadev
2. Show the current database configuration - the default before the patch is applied for days_in_advance is 0. SQL query: show columns from borrower_message_preferences;
3. Apply the patch.
4. Update the database (updatedatabase).
5. Repeat steps 1-2 - the default for days_in_advance should now be NULL.[1]
6. Tests should pass:
   . prove t/db_dependent/cronjobs/advance_notices_digest.t
   . prove t/db_dependent/Letters/TemplateToolkit.t
   . prove t/db_dependent/Koha/CurbsidePickups.t
   . prove t/db_dependent/Koha/Patrons.t
   . prove t/db_dependent/Patron/Messaging.t

[1] The updatedatabase command didn't update the existing installation, I had to do a reset_all to get the database to update.
Comment 6 David Nind 2022-11-28 23:57:40 UTC
I've changed the assignee.
Comment 7 Lari Taskula 2023-02-10 13:35:17 UTC
Created attachment 146475 [details] [review]
Bug 32357: Set borrower_message_preferences.days_in_advance default to NULL

To test:
1. prove t/db_dependent/cronjobs/advance_notices_digest.t
2. prove t/db_dependent/Letters/TemplateToolkit.t
3. prove t/db_dependent/Koha/CurbsidePickups.t
4. prove t/db_dependent/Koha/Patrons.t
5. prove t/db_dependent/Patron/Messaging.t
Comment 8 Lari Taskula 2023-02-10 13:39:02 UTC
(In reply to David Nind from comment #5)
> However, I don't think the databaseupdate updates existing installations. I
> had to do a reset_all to replicate a new installation (using
> koha-testing-docker).
True! I didn't know but there seems to be a difference in atomicupdate filenames ending between .perl and .pl, as seen on C4::Installer::run_atomic_updates(). I renamed it to *.pl and at least on my old dev installation it now applies with the updatedatabase script. Maybe someone knows why this is?
Comment 9 David Nind 2023-04-05 20:55:09 UTC
Created attachment 149223 [details] [review]
Bug 32357: Set borrower_message_preferences.days_in_advance default to NULL

To test:
1. prove t/db_dependent/cronjobs/advance_notices_digest.t
2. prove t/db_dependent/Letters/TemplateToolkit.t
3. prove t/db_dependent/Koha/CurbsidePickups.t
4. prove t/db_dependent/Koha/Patrons.t
5. prove t/db_dependent/Patron/Messaging.t

Signed-off-by: David Nind <david@davidnind.com>
Comment 10 David Nind 2023-04-05 20:57:16 UTC
Testing notes (using KTD):

1. Access the database: koha-mysql kohadev

2. Show the current database configuration - the default before the patch is applied for days_in_advance is 0. SQL query: show columns from borrower_message_preferences;

3. Apply the patch.

4. Update the database (updatedatabase).

5. Repeat steps 1-2 - the default for days_in_advance should now be NULL.

6. Tests should pass:
   . prove t/db_dependent/cronjobs/advance_notices_digest.t
   . prove t/db_dependent/Letters/TemplateToolkit.t
   . prove t/db_dependent/Koha/CurbsidePickups.t
   . prove t/db_dependent/Koha/Patrons.t
   . prove t/db_dependent/Patron/Messaging.t

Note: the database update now updates the existing installation.
Comment 11 Kyle M Hall 2023-04-28 17:38:10 UTC
Created attachment 150405 [details] [review]
Bug 32357: Set borrower_message_preferences.days_in_advance default to NULL

To test:
1. prove t/db_dependent/cronjobs/advance_notices_digest.t
2. prove t/db_dependent/Letters/TemplateToolkit.t
3. prove t/db_dependent/Koha/CurbsidePickups.t
4. prove t/db_dependent/Koha/Patrons.t
5. prove t/db_dependent/Patron/Messaging.t

Signed-off-by: David Nind <david@davidnind.com>
Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 12 Tomás Cohen Arazi 2023-05-12 20:54:29 UTC
Pushed to master for 23.05.

Nice work everyone, thanks!
Comment 13 Martin Renvoize 2023-05-16 06:45:18 UTC
Many hands makes light work, thankyou everyone!

Pushed to 22.11.x for the next release