Summary: | Set borrower_message_preferences.days_in_advance default to NULL | ||
---|---|---|---|
Product: | Koha | Reporter: | Lari Taskula <lari.taskula> |
Component: | Database | Assignee: | Lari Taskula <lari.taskula> |
Status: | CLOSED FIXED | QA Contact: | Kyle M Hall (khall) <kyle> |
Severity: | normal | ||
Priority: | P5 - low | CC: | david, kyle, martin.renvoize |
Version: | unspecified | ||
Hardware: | All | ||
OS: | All | ||
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
|
Circulation function: | |||
Bug Depends on: | |||
Bug Blocks: | 18595 | ||
Attachments: |
Bug 32357: Set borrower_message_preferences.days_in_advance default to NULL
Bug 32357: Set borrower_message_preferences.days_in_advance default to NULL Bug 32357: Set borrower_message_preferences.days_in_advance default to NULL Bug 32357: Set borrower_message_preferences.days_in_advance default to NULL Bug 32357: Set borrower_message_preferences.days_in_advance default to NULL |
Description
Lari Taskula
2022-11-28 21:56:46 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. 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 |