Follow-up to #2704023: Fix contact names created from a captured mail message

Problem/Motivation

Steps to reproduce:
1. Enable collect_demo
2. Go to admin/config/system/inmail/paste

3. "Load example" and click "Process email"
4. Repeat the step 3
5. You should get an exception: Recoverable fatal error: Object of class Drupal\Core\TypedData\Plugin\DataType\StringData could not be converted to string in drupal_schema_get_field_value() (line 229 of core/includes/schema.inc).

Proposed resolution

Add tests and ContactMatcher::updateContact() method, so it handles contact updates properly.

From #21 of #2704023: Fix contact names created from a captured mail message:

As starting point, have a look at \Drupal\Core\Entity\ContentEntityStorageBase::hasFieldValueChanged and the equals method that uses there (except that we'll probably need more like a contains() method, we will need to do something like that I think to figure out if the field changed. But getting that right is going to be complicated and require a ton of test coverage.

CommentFileSizeAuthor
#16 interdiff.txt15.58 KBslashrsm
#16 2708357_16.patch16.81 KBslashrsm
#12 2708357_12.patch4.44 KBslashrsm
#10 contact_updates_are_not-2708357-10.patch4.55 KBmbovan
#9 contact_updates_are_not-2708357-9.patch4.55 KBmbovan
#7 contact_updates_are_not-2708357-7-interdiff.txt1.98 KBmbovan
#7 contact_updates_are_not-2708357-7.patch5.58 KBmbovan
#5 contact_updates_are_not-2708357-5-interdiff.txt4.56 KBmbovan
#5 contact_updates_are_not-2708357-5.patch4.44 KBmbovan
#3 collect_2708357_contact_update_3.patch1.54 KBmiro_dietiker
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

mbovan created an issue. See original summary.

mbovan’s picture

mbovan CreditAttribution: mbovan at MD Systems GmbH commented
Component: Miscellaneous » Processing
Issue summary: View changes
Status: Needs review » Active
miro_dietiker’s picture

miro_dietiker
Location Switzerland
CreditAttribution: miro_dietiker at MD Systems GmbH commented
Priority: Normal » Critical
Status: Active » Needs review
FileSize
collect_2708357_contact_update_3.patch1.54 KB

Hmm...

Raising to critical. Submitting the same mail twice is my standard test to see activity cumulating.
Now activity can not attach to existing contacts anymore.. :-O

I guess the code attached will trigger the bug.

Status: Needs review » Needs work

The last submitted patch, 3: collect_2708357_contact_update_3.patch, failed testing.

mbovan’s picture

mbovan CreditAttribution: mbovan at MD Systems GmbH commented
Status: Needs work » Needs review
FileSize
contact_updates_are_not-2708357-5.patch4.44 KB
contact_updates_are_not-2708357-5-interdiff.txt4.56 KB

Tried to simplify and fix updateContact() as well as tests.

The bug should be fixed now... Tests fails are related to recently committed #2691491: Add name field to the contact entity

Status: Needs review » Needs work

The last submitted patch, 5: contact_updates_are_not-2708357-5.patch, failed testing.

mbovan’s picture

mbovan CreditAttribution: mbovan at MD Systems GmbH commented
Status: Needs work » Needs review
FileSize
contact_updates_are_not-2708357-7.patch5.58 KB
contact_updates_are_not-2708357-7-interdiff.txt1.98 KB

Updated the tests with changes made in #2691491: Add name field to the contact entity

Status: Needs review » Needs work

The last submitted patch, 7: contact_updates_are_not-2708357-7.patch, failed testing.

mbovan’s picture

mbovan CreditAttribution: mbovan at MD Systems GmbH commented
Status: Needs work » Needs review
FileSize
contact_updates_are_not-2708357-9.patch4.55 KB

Rerolled based on #12 in #2710733: Update contact matchers with name fields changes.

mbovan’s picture

mbovan CreditAttribution: mbovan at MD Systems GmbH commented
FileSize
contact_updates_are_not-2708357-10.patch4.55 KB

Tested locally after #2710733: Update contact matchers with name fields changes was committed. Recreating/uploading for tests.

Status: Needs review » Needs work

The last submitted patch, 10: contact_updates_are_not-2708357-10.patch, failed testing.

slashrsm’s picture

slashrsm CreditAttribution: slashrsm at MD Systems GmbH commented
FileSize
2708357_12.patch4.44 KB

Reroll. Will still fail.

slashrsm’s picture

slashrsm CreditAttribution: slashrsm at MD Systems GmbH commented
Status: Needs work » Needs review
mbovan’s picture

mbovan CreditAttribution: mbovan at MD Systems GmbH commented

Retested, seems to be passing and review-ready.

miro_dietiker’s picture

miro_dietiker
Location Switzerland
CreditAttribution: miro_dietiker at MD Systems GmbH commented 
+++ b/crm/src/Plugin/collect/Processor/ContactMatcher.php
@@ -186,26 +187,49 @@ class ContactMatcher extends ProcessorBase {
-    if ($has_changed) {
-      $existing_contact->setNewRevision();
-      $existing_contact->save();
...
+    $existing_contact->save();

You skip change check and always save. And not as a new revision?
Now it's always updated and a revert no more possible by user...

We need to make it configurable if an update happens or not. And update should not be the default at all on match.

slashrsm’s picture

slashrsm CreditAttribution: slashrsm at MD Systems GmbH commented
FileSize
2708357_16.patch16.81 KB
interdiff.txt15.58 KB

What about something like this? Also made update configurable.

There is another problem. Due to the structure of data that MailContactMatcher returns for name field will this type of fields always trigger change/save. We could improve this to some extent, but this could result in the loss of data when there will be existing data on name properties that matcher doesn't handle properly (all except give name). We could also hard code special check for name fields and only compare give name to determine if there was an update or not.

mbovan’s picture

mbovan CreditAttribution: mbovan at MD Systems GmbH commented
Status: Needs review » Needs work 
  1. +++ b/crm/src/Plugin/collect/Processor/ContactMatcher.php
@@ -189,22 +184,28 @@ class ContactMatcher extends ProcessorBase {
       if ($field_name == 'user_uri') {
-        $this->updateUserUri($existing_contact, $template_contact);
-        continue;
+        $has_changed |= $this->updateUserUri($existing_contact, $template_contact);
+      }
+      elseif (!$template_contact->get($field_name)->equals($existing_contact->get($field_name))) {

    By removing "continue" here aren't we getting into possibility to override user_uri (again) in the elseif? 

  2. +++ b/crm/src/Plugin/collect/Processor/ContactMatcher.php
@@ -189,22 +184,28 @@ class ContactMatcher extends ProcessorBase {
+    if ($has_changed && $this->getConfigurationItem('update_contact')) {

    I think we should add a test case when "update_contact" is FALSE, as of now it doesn't seem that we save a new contact when there is a match on existing one and updating is turned off.

miro_dietiker’s picture

miro_dietiker
Location Switzerland
CreditAttribution: miro_dietiker at MD Systems GmbH commented

Why should we create a new entity if update is disabled?

It just means we are not updating matching fields on the entity with values.
BTW independently of the matching values this, we should allow the matched entity to be connected with the User URI.
But we still match and connect the activity to the contact identified.