Bug 27574

Summary: Don't use NOW() in Koha::Accounts
Product: Koha Reporter: Martin Renvoize <martin.renvoize>
Component: DatabaseAssignee: Martin Renvoize <martin.renvoize>
Status: CLOSED INVALID QA Contact: Testopia <testopia>
Severity: normal    
Priority: P5 - low CC: fridolin.somers, jonathan.druart, tomascohen
Version: MainKeywords: Academy
Hardware: All   
OS: All   
See Also: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=27511
Change sponsored?: --- Patch complexity: ---
Documentation contact: Documentation submission:
Text to go in the release notes:
Version(s) released in:
Bug Depends on:    
Bug Blocks: 27573    
Attachments: Bug 27574: Update \NOW() to dt_from_string for Accounts
Bug 27574: Update \NOW() to dt_from_string for Accounts

Description Martin Renvoize 2021-01-29 08:28:39 UTC
Using NOW() to set a date field should be avoided as the SQL server timezone may not match reflect the koha instance timezone.
Comment 1 Martin Renvoize 2021-01-29 08:49:55 UTC
Created attachment 116011 [details] [review]
Bug 27574: Update \NOW() to dt_from_string for Accounts

This patch replaces all instances of setting the date/timestamp fields
for accounts related tables from the SQL literal \'NOW()' to
Koha::DateUtils dt_from_string so we get properly localised dates.
Comment 2 Martin Renvoize 2021-01-29 12:53:14 UTC
Working on unit tests for this
Comment 3 Martin Renvoize 2021-01-29 13:34:38 UTC
Hmm... after discussing this with Jonathan we agree this is perhaps a solved problem already with bug 20123
Comment 4 Martin Renvoize 2021-01-29 13:35:45 UTC
Created attachment 116029 [details] [review]
Bug 27574: Update \NOW() to dt_from_string for Accounts

This patch replaces all instances of setting the date/timestamp fields
for accounts related tables from the SQL literal \'NOW()' to
Koha::DateUtils dt_from_string so we get properly localised dates.

We also update the unit tests
Comment 5 Tomás Cohen Arazi 2021-01-29 19:58:10 UTC
Are we sure? Aren't we setting the timezone on connection time? In that case, wouldn't NOW() be already using the timezone? I'm worried we are double escaping here (hehe).