Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Extend token date handling to 'most' date fields #21584

Merged
merged 1 commit into from
Sep 28, 2021
Merged

Extend token date handling to 'most' date fields #21584

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 7 additions & 1 deletion CRM/Core/EntityTokens.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -63,7 +63,13 @@ public function evaluateToken(TokenRow $row, $entity, $field, $prefetch = NULL)
\CRM_Utils_Money::format($fieldValue, $this->getCurrency($row)));
}
if ($this->isDateField($field)) {
return $row->format('text/plain')->tokens($entity, $field, \CRM_Utils_Date::customFormat($fieldValue));
try {
return $row->format('text/plain')
->tokens($entity, $field, new DateTime($fieldValue));
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So I guess there are a few cases, depending on the type of the underlying field:

  • timestamp or datetime fields (eg civicrm_contribution.receive_date) -- the token's default format should indicate both date and time
  • date or time fields (eg civicrm_contact.birth_date) -- the token's default format should indicate date xor time (respectively)

My intuition is that this step here is the lossy part - ie we know the kind of MySQL $field we're looking at, but we output a PHP DateTime object. But DateTime doesn't distinguish between temporal subtypes (time/date/datetime/timestamp). The lossiness will create a quirk where most datetime/timestamp data shows the time - but there is a 1/86,400 chance of displaying without the time. (Admittedly, that's not very high, but it is harder to reason about.)

I think the only thing we could really do here is pass-through a richer object. For example, with something like https://gist.github.com/totten/185f4d7bf87e22f7fc363c85c756ba32, we could add a hint of the true type of the data:

...->tokens($entity, $field, new MaskedDateTime($fieldValue, NULL, 'date'));
...->tokens($entity, $field, new MaskedDateTime($fieldValue, NULL, 'time'));
...->tokens($entity, $field, new MaskedDateTime($fieldValue, NULL, 'datetime'));
...->tokens($entity, $field, new MaskedDateTime($fieldValue, NULL, 'timestamp'));

Or we could attach a default format with something like: https://gist.github.com/totten/878177535912382fa298f80c5e456033

...->tokens($entity, $field, new FormattedDateTime($fieldValue, NULL, $config->datetimePartial));
...->tokens($entity, $field, new FormattedDateTime($fieldValue, NULL, $config->datetimeTime));
...->tokens($entity, $field, new FormattedDateTime($fieldValue, NULL, $config->datetimeFull));

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@totten yeah the other thing is that how you use the data is a better indicator of whether you want the time to show than the metadata of the field. For example it's pretty commont to exclude time when recording contributions - but if you DO include it it's likely you would want to show it.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@totten so in fact it's probably not the case that 'most datetime/timestamp data shows the time' - that would be true for timestamp but for many date time fields having no time is the norm not the exception

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The patch has six changes to the test-suite here which seem to be counter-examples... Skimming civicrm-core@master:TokenConsistency.php, it looks to me like most of the examples do have times.

The exception is that civicrm_case.start_date and end_date are pure-dates. But this makes sense because the MySQL type is date (ie those fields don't store times).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@totten I meant the norm in terms of how people actually use the database - ie it's hugely common not to record time unless it's meaningful

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just noting from chat - pretty much every database I've ever seen has a gazillion date time records where the time is not filled in - eg. pretty much all imported contribution records have 'midnight' for their time. This is common across contributions, recurrings, pledges, etc etc

On the flip side a time of 'midnight' is really only meaningful in an event that really truly does start or end at midnight - which is rare but could be the case in a multizone event. Mitigating this

  • we haven't added the timezone support yet & would provide a different default if timezone info was present
  • currently events don't seem to rely on the default formatting for start or end date (they have logic to figure out if the date is the same & if exclude date)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Clicking-around to spot-check, there a few distinct behaviors:

  • Activity:
    • When creating a new record, both date+time are mandatory
  • Contribution:
    • When creating a new record, both date+time are mandatory for "Date Received". But for "Receipt Date", the time-component is optional. (If blank, it's saved as 12:00 AM.)
    • When viewing or editing a record, it shows the time as 12:00 AM.
    • When you cancel a record, the "Cancelled Date" is generally optional. If completely blank, it saves with current date+time. If time is omitted, the time will save as 12:00 AM.
    • When viewing or editing a cancelled record, it shows the time as 12:00 AM.
  • Event:
    • When creating a new record, the "Start Date" is generally required, and the "End Date" is generally optional. In both fields, the time component is optional. (If blank, it's saved as 12:00 AM.)
    • When viewing or editing the record (with public or private pages), the 12:00 AM does show up.
    • When using "New Event Registration", the "Registration Date" is prepopulated with both date+time. However, you can delete the "time" component. (If blank, it's saved as 12:00 AM.)
    • When viewing or editing a registration, it shows the time as 12:00 AM.
  • Mailing:
    • When submitting a new mailing, you choose a radio-button - either (a) now or (b) datetime. If you give datetime, then both date+time are mandatory.
  • Membership:
    • This does not have any datetime fields. (It only uses date fields.)
  • Pledge:
    • When creating a new record, the "Pledge Made" and "Payments Start" look like date fields. Under the hood, they are saved with 12:00 AM. This is a consistent facade - the list/view/edit screens consistently hide the time component.

(The above is about the backend UI. Separately, a very large portion of temporal data is specified under-the-hood -- eg logging (*.created_date, *.modified_date, civicrm_subscription_history, civicrm_mailing_job.start_date, ad nauseum); also, records are often processed by self-service flows which autopopulate temporal data completely.)

It seems to me that:

  • The "Pledge" entity is unusual. It has datetime fields with a consistent facade -- a simulacrum of date.
  • For every other entity, the datetime fields are... well... date+time (together, as whole).
  • When datetime is manually entered by a staff user, the time component is sometimes optional. But this is always flaky -- because blank time is coerced to 12:00 AM (which appears in search/browse/view/edit UIs; and which is indistinguishable from true 12:00 AM).
  • This is a bit of a conundrum. I don't think we should encourage the idea of 12:00 AM as a magical value - because (in practice) it doesn't get that treatment consistently, and (in principal) it's ambiguous and it complicates timezone-correctness. OTOH, I think you're right (in a probabilistic sense) that 12:00 AM is almost never literal - it is more likely to indicate a backend-user skipped that particular datum. Whatsmore, datetime fields are prone to timezone problems... so maybe, on the whole, it's more accurate to show only the date...

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@totten yeah - that's where I got to with it - the other big data source of data is imports & other input forms - which have a higher chance of having no time attached. The bit about 'not much happens at midnight' means that where contributions don't have a time (1% of them in the DB I just checked) they probably didn't happen at midnight (0% in the db I just checked)

What we are setting is a default that should be somewhat intuitive from a user point of view -- but imagine if you actually did have an event that started at midnight - probably at that point we are having the conversation about
a) helping people choose a format with the timezone included and
b) getting this working #21599 so it can output in the selected timezone

  • but in those cases we aren't using the defaults.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My 2 cents is that for the purposes of filling tokens, it's safe to assume that a time of 00:00:00 can be treated like "date only", as we are talking about a display value here.

}
catch (Exception $e) {
Civi::log()->info('invalid date token');
}
}
$row->format('text/plain')->tokens($entity, $field, (string) $fieldValue);
}
Expand Down
4 changes: 4 additions & 0 deletions Civi/Token/TokenProcessor.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -452,6 +452,10 @@ private function filterTokenValue($value, ?array $filter, TokenRow $row) {

if ($value instanceof \DateTime && $filter === NULL) {
$filter = ['crmDate'];
if ($value->format('His') === '000000') {
// if time is 'midnight' default to just date.
$filter[1] = 'Full';
}
}

switch ($filter[0] ?? NULL) {
Expand Down
2 changes: 1 addition & 1 deletion Civi/Token/TokenRow.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -272,7 +272,7 @@ public function fill($format = NULL) {
$htmlTokens[$entity][$field] = \CRM_Utils_String::purifyHTML($value);
}
else {
$htmlTokens[$entity][$field] = htmlentities($value);
$htmlTokens[$entity][$field] = is_object($value) ? $value : htmlentities($value);
}
}
}
Expand Down
24 changes: 12 additions & 12 deletions tests/phpunit/CRM/Activity/Form/Task/PDFLetterCommonTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,7 @@ public function testCreateDocumentBasicTokens(): void {
$activity = $this->activityCreate(['campaign_id' => $this->campaignCreate()]);
$data = [
['Subject: {activity.subject}', 'Subject: Discussion on warm beer'],
['Date: {activity.activity_date_time}', 'Date: ' . CRM_Utils_Date::customFormat(date('Ymd')) . ' 12:00 AM'],
['Date: {activity.activity_date_time}', 'Date: ' . CRM_Utils_Date::customFormat(date('Ymd'))],
['Duration: {activity.duration}', 'Duration: 90'],
['Location: {activity.location}', 'Location: Baker Street'],
['Details: {activity.details}', 'Details: Lets schedule a meeting'],
Expand Down Expand Up @@ -127,17 +127,17 @@ public function testCreateDocumentSpecialTokens(): void {
$this->markTestIncomplete('special tokens not yet merged - see https://github.com/civicrm/civicrm-core/pull/12012');
$activity = $this->activityCreate();
$data = [
['Source First Name: {activity.source_first_name}', "Source First Name: Anthony"],
['Target N First Name: {activity.target_N_first_name}', "Target N First Name: Julia"],
['Target 0 First Name: {activity.target_0_first_name}', "Target 0 First Name: Julia"],
['Target 1 First Name: {activity.target_1_first_name}', "Target 1 First Name: Julia"],
['Target 2 First Name: {activity.target_2_first_name}', "Target 2 First Name: "],
['Assignee N First Name: {activity.target_N_first_name}', "Assignee N First Name: Julia"],
['Assignee 0 First Name: {activity.target_0_first_name}', "Assignee 0 First Name: Julia"],
['Assignee 1 First Name: {activity.target_1_first_name}', "Assignee 1 First Name: Julia"],
['Assignee 2 First Name: {activity.target_2_first_name}', "Assignee 2 First Name: "],
['Assignee Count: {activity.assignees_count}', "Assignee Count: 1"],
['Target Count: {activity.targets_count}', "Target Count: 1"],
['Source First Name: {activity.source_first_name}', 'Source First Name: Anthony'],
['Target N First Name: {activity.target_N_first_name}', 'Target N First Name: Julia'],
['Target 0 First Name: {activity.target_0_first_name}', 'Target 0 First Name: Julia'],
['Target 1 First Name: {activity.target_1_first_name}', 'Target 1 First Name: Julia'],
['Target 2 First Name: {activity.target_2_first_name}', 'Target 2 First Name: '],
['Assignee N First Name: {activity.target_N_first_name}', 'Assignee N First Name: Julia'],
['Assignee 0 First Name: {activity.target_0_first_name}', 'Assignee 0 First Name: Julia'],
['Assignee 1 First Name: {activity.target_1_first_name}', 'Assignee 1 First Name: Julia'],
['Assignee 2 First Name: {activity.target_2_first_name}', 'Assignee 2 First Name: '],
['Assignee Count: {activity.assignees_count}', 'Assignee Count: 1'],
['Target Count: {activity.targets_count}', 'Target Count: 1'],
];
$html_message = "\n" . implode("\n", CRM_Utils_Array::collect('0', $data)) . "\n";
$form = $this->getFormObject('CRM_Activity_Form_Task_PDF');
Expand Down
6 changes: 3 additions & 3 deletions tests/phpunit/CRM/Contribute/ActionMapping/ByTypeTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -297,13 +297,13 @@ public function testTokenRendering(): void {
$this->callAPISuccess('job', 'send_reminder', []);
$expected = [
'first name = Alice',
'receive_date = February 1st, 2015 12:00 AM',
'receive_date = February 1st, 2015',
'contribution status id = 1',
'new style status = Completed',
'new style label = Completed Label**',
'id ' . $this->ids['Contribution']['alice'],
'id - not valid for action schedule',
'cancel date August 9th, 2021 12:00 AM',
'cancel date August 9th, 2021',
'source SSF',
'financial type id = 1',
'financial type name = Donation',
Expand Down Expand Up @@ -349,7 +349,7 @@ public function testTokenRendering(): void {
TRUE
);
$expected = [
'receive_date = February 1st, 2015 12:00 AM',
'receive_date = February 1st, 2015',
'new style status = Completed',
'contribution status id = 1',
'id ' . $this->ids['Contribution']['alice'],
Expand Down
2 changes: 1 addition & 1 deletion tests/phpunit/CRM/Event/Form/Task/BadgeTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -91,7 +91,7 @@ protected function getAvailableTokens(): array {
'{event.start_date}' => 'October 21st',
'{participant.status_id}' => 2,
'{participant.role_id}' => 1,
'{participant.register_date}' => 'February 19th, 2007 12:00 AM',
'{participant.register_date}' => 'February 19th, 2007',
'{participant.source}' => 'Wimbeldon',
'{participant.fee_level}' => 'low',
'{participant.fee_amount}' => NULL,
Expand Down
7 changes: 5 additions & 2 deletions tests/phpunit/CRM/Utils/TokenConsistencyTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -288,6 +288,7 @@ protected function getContributionRecurID(): int {
'start_date' => '2021-07-23 15:39:20',
'end_date' => '2021-07-26 18:07:20',
'cancel_date' => '2021-08-19 09:12:45',
'next_sched_contribution_date' => '2021-09-08',
'cancel_reason' => 'Because',
'amount' => 5990.99,
'currency' => 'EUR',
Expand Down Expand Up @@ -340,9 +341,9 @@ protected function getExpectedContributionRecurTokenOutPut(): string {
2
Yes
15

September 8th, 2021
0
January 3rd, 2020 12:00 AM
January 3rd, 2020
Yes
1
2
Expand Down Expand Up @@ -602,6 +603,8 @@ public function getDomainTokens(): array {

/**
* Test that domain tokens are consistently rendered.
*
* @throws \API_Exception
*/
public function testEventTokenConsistency(): void {
$mut = new CiviMailUtils($this);
Expand Down