progressivetech / net.ourpowerbase.sumfields

The Summary Fields extension creates read-only custom data fields that extend Contact and are automatically populated with up-to-date totals such as total lifetime contributions, last contribution amount, last attended event, etc.
Other
8 stars 29 forks source link

Summary date fields cause error when merging contacts, botches the merge #98

Closed freeform-sg closed 1 year ago

freeform-sg commented 1 year ago

If you try to merge a contact with a "Last Contribution Date", or "Date of Last Membership Payment" the merge breaks part way through, leaving contacts partially merged (will fail to complete the merge, but the duplicate will have its email address and cms reference deleted, and you'll be unable to delete the cms contact). The error is:

value: yyyy-mm-dd 00:00:00 is not of the right field data type: Date

Example: value: 2023-02-19 19:25:17 is not of the right field data type: Date

This began happening with one of the CiviCRM releases after 5.58.1 (we are seeing the issue on 5.59.3)

jmcclelland commented 1 year ago

Thanks for the report. I'm not sure why this is happening and whether or not it's a sumfields bug or a CiviCRM core bug??

But... one fix would be to simply eliminate summary fields from the merge screen so you are never allowed to merge these fields. I've been contemplating this change for a while. If both records have summary fields, merging them will result in stale summary fields anyway. And most people I train are just befuddled with what to do with the summary fields on the merge screen so I typically advise against merging and then re-generating summary fields when you are done.

On the other hand, if only one record has summary fields, merging them properly preserves that data.

What do you think? Would dropping summary fields from the merge screen solve your problem or does your workflow depend on merging them?

freeform-sg commented 1 year ago

Thanks for the quick response. In our case, the extension had been installed such a long time ago that the current staff aren't sure if anyone is even looking at those fields for anything anymore, so we are investigating if it can be removed altogether. In the meantime, I simply instructed them not to merge summary date fields, and cautioned that any summary data could be stale anyway depending on the contacts being merged.

I also wasn't sure if this was a core issue or not - it seems odd that it would be expecting a simple date instead of a full date/time, but at the time I couldn't find the change log that would help pin-point where it changed and whether it was done so for a particular reason. I am out of resources to investigate further on this, unfortunately.

jmcclelland commented 1 year ago

Well, I'm really striking out with this one. I could not find an adequate hook that would allow me to omit the summary fields in the display of fields to change. So, I tried to find the core bug that might be responsible, but wasn't able to replicate the problem either. What version of CiviCRM Core are you running? And what version of Summary Fields? And if you have a complete back trace handy that would be amazing.

AsylumSeekersCentre commented 1 year ago

I've just reproduced this bug with Summary Fields version 5.0.2 on CiviCRM 5.61.2.

Here is the error message (including back trace) from the Drupal log:

$Fatal Error Details = array:3 [ "message" => "value: 2023-06-01 15:16:00 is not of the right field data type: Date" "code" => null "exception" => CRM_Core_Exception {
#1139 -errorData: array:1 [ "error_code" => 0 ] 

#cause: null -_trace: null 
#message: "value: 2023-06-01 15:16:00 is not of the right field data type: Date" 
#code: 0 
#file: "/path/to/installation/sites/all/modules/civicrm/CRM/Core/BAO/CustomValueTable.php" 
#line: 657 trace: { 
/path/to/installation/sites/all/modules/civicrm/CRM/Core/BAO/CustomValueTable.php:657 {
  CRM_Core_BAO_CustomValueTable::setValues(&$params) › elseif (CRM_Utils_Type::escape($fieldValue['value'], $dataType, FALSE) === NULL) { › throw new CRM_Core_Exception(ts('value: %1 is not of the right field data type: %2', › [ } 
/path/to/installation/sites/all/modules/civicrm/CRM/Dedupe/Merger.php:2054 { …} 
/path/to/installation/sites/all/modules/civicrm/CRM/Contact/Form/Merge.php:324 { …} 
/path/to/installation/sites/all/modules/civicrm/CRM/Core/Form.php:612 { …} 
/path/to/installation/sites/all/modules/civicrm/CRM/Core/StateMachine.php:144 { …} 
/path/to/installation/sites/all/modules/civicrm/CRM/Core/QuickForm/Action/Next.php:43 { …} 
/path/to/installation/sites/all/modules/civicrm/packages/HTML/QuickForm/Controller.php:203 { …} 
/path/to/installation/sites/all/modules/civicrm/packages/HTML/QuickForm/Page.php:103 { …} 
/path/to/installation/sites/all/modules/civicrm/CRM/Core/Controller.php:355 { …} 
/path/to/installation/sites/all/modules/civicrm/CRM/Utils/Wrapper.php:98 { …} 
/path/to/installation/sites/all/modules/civicrm/CRM/Core/Invoke.php:292 { …} 
/path/to/installation/sites/all/modules/civicrm/CRM/Core/Invoke.php:69 { …} 
/path/to/installation/sites/all/modules/civicrm/CRM/Core/Invoke.php:36 { …} 
/path/to/installation/sites/all/modules/civicrm/drupal/civicrm.module:471 { …} 
/path/to/installation/includes/menu.inc:527 { …} 
/path/to/installation/index.php:21 { …} } } ] 
AsylumSeekersCentre commented 1 year ago

It seems to be failing validation due to these regex matches:

https://github.com/civicrm/civicrm-core/blob/master/CRM/Utils/Type.php#L430-L431

Called from here:

https://github.com/civicrm/civicrm-core/blob/master/CRM/Utils/Type.php#L241

The $data variable at that point is a string in this format:

2023-06-01 15:16:00

It looks like the regex is expecting the digits without any of the other characters. I haven't looked further into it to figure out where the date should be converted.

jmcclelland commented 1 year ago

Thanks for the additional information. I managed to replicate and submitted a patch to core. I'm closing - I'm not sure if my patch will be accepted, but I'm fairly sure this is a core bug not a summary fields bug.

yashodha commented 1 year ago

@jmcclelland this sure is a core bug and not related to the extension. I recently encountered the same while merging contacts with CiviCRM custom fields of datatype 'SelectCountry' that were view only.

jmcclelland commented 1 year ago

Thanks for the nudge @yashodha - I got stuck on the PR because I wasn't sure which direction to go in. I just bumped it in an effort to get a suggestion - hopefully we can resolve the core bug!

jmcclelland commented 1 year ago

At last - the core fix has been merged and should be available starting with CiviCRM 5.64. With that fix, summary fields should no longer be visible on the merge screen and should never merge. Thanks everyone who helped identify and move this forward.

One final note: it also means that when you merge two contacts with contributions, the summary fields will be inaccurate (that was the case before) - however, the next time a contribution is added the totals should be corrected (or you can manually re-save the sum fields configuration screen to re-generate all summaries).