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)
(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.
Created attachment 144307 [details] [review] Bug 32357: Set borrower_message_preferences.days_in_advance default to NULL
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.
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
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.
I've changed the assignee.
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
(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?
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>
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.
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>
Pushed to master for 23.05. Nice work everyone, thanks!
Many hands makes light work, thankyou everyone! Pushed to 22.11.x for the next release