Convert MembershipLog.modified_date from date to timestamp so we record time as well #31799
Conversation
|
🤖 Thank you for contributing to CiviCRM! ❤️ We will need to test and review this PR. 👷 Introduction for new contributors...
Quick links for reviewers...
|
e75b04a to
f162ee7
Compare
|
It's a log table, each entry is added as a history and not modified afterwards so it doesn't make sense in this case. The modified_date refers to the date the membership was modified.
I don't think it matters as the DAO layer understands both. |
|
the membership test fails look related |
ab49191 to
d6c0e3a
Compare
| @@ -379,12 +379,12 @@ public function testSubmitRecurCompleteInstant(): void { | |||
| $log = $this->callAPISuccessGetSingle('MembershipLog', ['membership_id' => $membership['id'], 'options' => ['limit' => 1, 'sort' => 'id DESC']]); | |||
| $this->assertEquals(CRM_Utils_Time::date($nextYear . '-01-01'), $log['start_date']); | |||
| $this->assertEquals(CRM_Utils_Time::date($nextYear . '-01-31'), $log['end_date']); | |||
| $this->assertEquals(CRM_Utils_Time::date('Y-m-d'), $log['modified_date']); | |||
| $this->assertEquals(date('Y-m-d'), CRM_Utils_Time::date('Y-m-d', strtotime($log['modified_date']))); | |||
There was a problem hiding this comment.
Is there a reason not to just change equals part to compare against the time too? We're using frozen time (the use of CRM_Utils_Time) in these tests so second rollovers won't be an intermittent issue. By removing CRM_Utils_Time and just using date() it's thawing out the time.
| $this->assertEquals(date('Y-m-d'), CRM_Utils_Time::date('Y-m-d', strtotime($log['modified_date']))); | |
| $this->assertEquals(CRM_Utils_Time::date('Y-m-d H:i:s'), $log['modified_date']); |
I haven't run that but given that the PR is about ADDING a time component, it seems like we would want to test that time is getting used.
There was a problem hiding this comment.
Fair enough. Why do you say it's using frozen time? I can see the class but there are various options and I don't see where the input is set to frozen?
There was a problem hiding this comment.
Ah you're right it just sets a fixed "starting" time that the universe is relative to (in setUp). So if we do start getting second rollovers there is another method available to compare two numbers with a fudge factor: assertApproxEquals()
There was a problem hiding this comment.
Ok, I've updated to use assertApproxEquals with a tolerance of 20 seconds
8b99c2c to
33ff5e3
Compare
33ff5e3 to
5098c21
Compare
5098c21 to
2f207c1
Compare
| @@ -379,7 +379,7 @@ public function testSubmitRecurCompleteInstant(): void { | |||
| $log = $this->callAPISuccessGetSingle('MembershipLog', ['membership_id' => $membership['id'], 'options' => ['limit' => 1, 'sort' => 'id DESC']]); | |||
| $this->assertEquals(CRM_Utils_Time::date($nextYear . '-01-01'), $log['start_date']); | |||
| $this->assertEquals(CRM_Utils_Time::date($nextYear . '-01-31'), $log['end_date']); | |||
| $this->assertEquals(CRM_Utils_Time::date('Y-m-d'), $log['modified_date']); | |||
| $this->assertApproxEquals(strtotime(CRM_Utils_Time::date('Y-m-d H:i:s')), strtotime($log['modified_date']), 20); | |||
There was a problem hiding this comment.
I think this can be simplified to just CRM_Utils_Time::time() but I'll put it up as a followup.
Overview
Currently only the date is recorded in civicrm_membership_log. This is problematic if there are multiple changes on the same day because it is not clear which ones happened first. You can guess if you sort by table ID as well but it makes it much harder to correlate changes with other log entries.
Before
Only date recorded in MembershipLog.modified_date
After
Both date and time recorded in MembershipLog.modified_date
Technical Details
Converts database field to timestamp and checks that we are passing in full date/time as params when updating.
Comments