christianwach / civicrm-wp-profile-sync

Keeps a WordPress User profile in sync with a CiviCRM Contact and integrates WordPress and CiviCRM Entities with data synced via Advanced Custom Fields.
https://wordpress.org/plugins/civicrm-wp-profile-sync/
GNU General Public License v2.0
13 stars 17 forks source link

Primary Email update #39

Closed rbaugh closed 11 months ago

rbaugh commented 2 years ago

I have had an issue a couple of times where updating the primary email address for a contact doesn't update the corresponding WP user. It doesn't happen often, but does occur at times. Not sure if there are circumstances where this could occur that maybe I should be aware of.

One issue that seems to always be a problem is the UFMatch record. When updating a primary email on the contact, while the WP profile is updated to match, the UFMatch is not updated,

We use WPCivieRules to add WP Users on certain CiviRule actions. This extension provides a token to pass the username (UFMatch.name) property into emails or invoices. But when the primary email address is updated, the UFMatch.name doesn't get updated and can cause some issues with the use of the token.

Shouldn't the UFMatch.name be updated as well since this is the email that ideally is associated with the WP user?

christianwach commented 2 years ago

@rbaugh Sorry for the late reply - holidays eh?

Shouldn't the UFMatch.name be updated as well since this is the email that ideally is associated with the WP user?

Yes, it should be updated.

Are you raising the issue here because this plugin doesn't update it? Or because your CiviRule doesn't update it - and when it doesn't, then sync via this plugin fails?

rbaugh commented 2 years ago

@christianwach NP, I understand.

I was raising the issue as the UFMatch.name is not getting updated for me. So I am having to update the email in Civi, double check the WP profile and finally update UFMatch through API.

I only mentioned the WPCiviRules in that we use it in email templates to remind people of their login at times and when it doesn't get updated, it causes issues. Plus changing a primary email for someone and this not getting updated gives us issues when someone wants to get back in and uses the new email for a forgot password.

christianwach commented 2 years ago

It doesn't happen often, but does occur at times.

@rbaugh Do you have any idea of the context in which this happens? Is there anything related in your logs?

christianwach commented 2 years ago

@rbaugh FYI this sounds really hard to "blind debug"... I'd recommend putting code into your system that logs when the UFMatch table is updated and there's no email remaining in the record. The backtrace from that should tell you where the issue stems from.

One final thought - do you have any CiviRules that are unfinished? If so, delete them or finish configuring then - I've seen this cause all sorts of problems in various installs.

rbaugh commented 2 years ago

@christianwach I will check logs and see if I can find anything.

Overall, whenever I change the primary email for a contact, the WP profile is updated but not the UFMatch. This is consistently failing on the update, but I will need to see if there is anything in the logs. I will also see if I can debug this and if I find a culprit, I can look to submit an update or PR

christianwach commented 2 years ago

@rbaugh Thanks for this - I'll check later at my end and try to replicate.

christianwach commented 2 years ago

@rbaugh Okay, I've replicated this now - thanks for the report. I suspect this hasn't come up before because the UFMatch Email is rarely used - people probably simply use the Email attached to the Contact record. Indeed, in multi-domain situations, there are multiple UFMatch records and it then becomes tricky to decide which one to use.

I'll see if I can implement a quick fix - however it's a bit of a can of worms, this one... I'm not actually happy with the way that the CiviCRM Primary Email syncs with the WordPress User Email. I actually prefer the idea that this plugin allows an Email Location Type to be chosen specifying the Email to be synced - the same way it does for the Website Type. In many cases, it is not desirable for the WordPress User Email to be changed when changing the a Contact's Primary Email.

christianwach commented 2 years ago

FWIW I'd also suggest that the WPCiviRules Extension's token logic is somewhat shaky:

That aside, I still agree that this plugin should update the uf_name value correctly.

rbaugh commented 2 years ago

@christianwach I am glad you caught the issue. I could use another option for the token, but had already set it up and realized the problem as the email address in the body of the email was not matching the WP account.

I can see your point about npt updating WP, but if a member is updated in Civi and then tries to do a PW reset with the new email, it wouldn't work. One of our clients goes in and updates the member records as they do change emails because of schooling so having things sync and not have to make multiple edits is nice.

I can also create my own token for this, but was just using what was available. It wasn't until I had an issue that I realized it was pulling from the UFMatch and not as you stated by using the contact_id to derive the uf_id and pull the data from WP. This also becomes an issue when you merge contacts as the UF_Match doesn't update to use the primary either. Seems like it should just not set anything and just be a lookup only.

christianwach commented 2 years ago

@rbaugh My issue with the Primary Email being synced to the WordPress User is that it doesn't properly take into account the logic in CiviCRM that is used in Mailings. Someone may wish to change the Email they want to receive Mailings at but not wish to change their WordPress Email. My goal here is to suggest people use a custom Location Type (e.g. "CMS" or "WordPress" or similar) and then sync that Email, leaving the CiviCRM Mailing logic out of the equation.

christianwach commented 2 years ago

And then there's the added complication of the "Bulk Email" attribute 😆 - as I said, it's a can of worms!

christianwach commented 2 years ago

The logic appears to be this...

If you are using the CiviMail component to send mailings to contacts, this field (the Bulk Mailings checkbox) provides additional control over which email address is used. By default, CiviMail sends mail to each contact's preferred email address. If the contact has multiple locations, then the preferred email of the primary location is used. However, if the contact prefers to have CiviMail ('bulk') mailings set to an alternate email address - check the 'Use for Bulk Mailings' box next to that email address.

And of course multiple Emails can be selected for Bulk Mailings... which makes me think that the custom Location Type sync is probably the lightest touch sync. Your CiviCRM admins would still be able to update the WordPress User Email from CiviCRM, they'd just be doing it without affecting the Contact's choices regarding Mailings.

christianwach commented 2 years ago

This also becomes an issue when you merge contacts as the UF_Match doesn't update to use the primary either.

@rbaugh Just swerving back to this for a moment... do you mean this a result of this plugin overlooking the UFMatch Email update or is this something that happens in CiviCRM more generally?

rbaugh commented 2 years ago

The mailings itself isn't the issue, at least not what sent got me to this issue ticket. We have an email template that is using the CiviRule token in the footer as a reminder to someone what email they used for when they are logging into their account. While the email address this message is being sent to may be the same email address, it also may not be and we are just adding it in as a reminder.

The issue for us was that the UFMatch wasn't updated and the token was pulling from UFMatch and providing the old email their WP profile was connected to. So if they used that email to do a PW reset, it wouldn't work as the email had been updated through Civi and synced to the WP profile.

We were using the WPCiviRules to create the WP account and so the member doesn't really know their UN and only knows their email. When the email changes, they likely won't think to check old emails for WP reset so keeping the email in sync is key. Having a new Location Type, or setting, to specify which email should be used for the CMS account would be good and what I think you are getting at.

christianwach commented 2 years ago

Thanks for the clear explanation of your scenario 👍

I'm on the case with a fix that works on WordPress single site, multisite and multi-domain.

rbaugh commented 2 years ago

@rbaugh My issue with the Primary Email being synced to the WordPress User is that it doesn't properly take into account the logic in CiviCRM that is used in Mailings. Someone may wish to change the Email they want to receive Mailings at but not wish to change their WordPress Email. My goal here is to suggest people use a custom Location Type (e.g. "CMS" or "WordPress" or similar) and then sync that Email, leaving the CiviCRM Mailing logic out of the equation.

Didn't catch this reply earlier. I get what you mean by the sync of primary to WP/CMS. Having a flag for CMS like the is primary would be useful. Only one can be used and could be set as desired. If only one email, then is becomes primary and CMS user email.

Appreciate getting a patch in for this. I haven't had a chance to get to debugging and looking into the code.

christianwach commented 2 years ago

@rbaugh I've added a branch with what I think is a fix:

https://github.com/christianwach/civicrm-wp-profile-sync/tree/ufmatch

Can you check if that fixes things for you?

@kcristiano @danaskallman I suspect this thread may be of interest to you both since my fix:

Thoughts?

rbaugh commented 2 years ago

@christianwach I'll test this out later today.

kcristiano commented 2 years ago

@christianwach

I do think we should respect the domain id - why is it updating regardless of the domain id? CiviCRM MultiDomain does not have to be a WP multi-Site. I've even had WP and Drupal sites connected with MultiDomain and that would surely break that configuration,

The second point I also see an issue with depending on the configuration.

I do think we have to respect the domain id with uf_match. This may work for a specific WP Multisite config, but how would it work with WP Multi-Site connected to multiple CiviCRM DB (no multi-Domain)

I am unsure of the 3rd point - can you share an example?

christianwach commented 2 years ago

I do think we should respect the domain id - why is it updating regardless of the domain id?

@kcristiano Here's a scenario:

Now we have data corruption.

If the UFMatch record is retrieved without reference to the Domain (or searches all Domains) then the WordPress User is found and their Email is updated.

kcristiano commented 2 years ago

and if we update all domains and that is not the scenario wouldn't we have data corruption?

I think this needs testing before merging/releasing - certainly before releasing. Happy to help I just need to get past a few other issues first

christianwach commented 2 years ago

Having mulled this over all evening, I'm wondering what use there is in multiple entries in the uf_match table (with different Domain IDs) linking the Contact and the User?

If the purpose of the uf_match table is to look up the connection between Contact and User, then the only use case I can see (as you say @kcristiano) is where each entry connects the Contact to a User in a different CMS. I'm currently struggling to think of another purpose.

if we update all domains and that is not the scenario wouldn't we have data corruption?

Hmm, well if I'm reading the code right, it seems that the Multisite Extension itself implements a blanket update of the uf_name value on all UFMatch records when a UFMatch record is updated. Which suggests that this plugin actually only needs to update one and the Multisite Extension will handle updating the rest!

If there is purpose to UFMatch beyond discovering the Contact-to-User link, I can't seem to identify an important one - even though I've scanned every usage of UFMatch records in CiviCRM Core. The only examples of any significance that I can find where the uf_name value is used is in:

It seems to me that the first can be ignored... and the logic in the second method could be shifted to querying via the CRM_Utils_System_* classes. I proposed removing the unused language column from the uf_match table a while back... I wonder if it's worth proposing the removal of uf_name too?

christianwach commented 2 years ago

how would it work with WP Multi-Site connected to multiple CiviCRM DB (no multi-Domain)?

Good question. I'll look into this when I get a chance.

kcristiano commented 2 years ago

The primary use case is One CiviCRM Multi-Domain connected to multiple single site installs (regardless of the CMS) with different users. That is the exception I am thinking of.

I also don't see much value in uf_name either

rbaugh commented 2 years ago

@christianwach @kcristiano If the uf_name isn't really used anywhere else, then I think maybe this doesn't need to be part of the table since it can be derived anyways. Not sure if the purpose was to limit another lookup, but can be handled if this field wasn't used anymore. This wouldn't be a problem anymore either as we wouldn't need to worry about updating it when the CMS profile email is updated. Not sure if any other extensions are using this field or not.

I had just been using the WPCiviRules token since it was there, but I don't have an issue creating my own token to get the CMS user email to be able to add it to emails.

I do like having another attribute on an email address to signify which email is to be used for the CMS. This way it isn't always contingent on just the primary email although others may not agree with it.

christianwach commented 2 years ago

I do like having another attribute on an email address to signify which email is to be used for the CMS.

@rbaugh To be clear, I'm not proposing an additional attribute (that would require changes in CiviCRM Core) but instead propose to map a Location Type so that the Email with that Location Type is synced to the CMS - the same way this plugin does for the Website Type. People can then use an existing Location Type or create a custom one.

christianwach commented 2 years ago

Not sure if any other extensions are using this field or not.

Yes, this could be a problem - which is why (for the time being) I'm looking at the best way to do this. There'll be an update to the ufmatch branch in due course when I've figured out the interaction with the CiviCRM Multisite Extension.

rbaugh commented 2 years ago

@christianwach I understand that the UFMatch and the attribute comments would be a core issue/change. I was just commenting and rationalizing against this issue I added in. I am not as familiar with core history and code as you are and really thought the uf_name was something utilized more but apparently it isn't.

I appreciate the time on this and enjoy having the community around core that help make this more useful.

christianwach commented 2 years ago

The primary use case is one CiviCRM Multi-Domain connected to multiple single site installs (regardless of the CMS) with different users.

@kcristiano Okay that seems a reasonable assumption... I've been checking my Multisite/MultiDomain installs and I can't find an example where there are multiple UFMatch entries. However, that's not to say that there cannot be multiple UFMatch entries - I still think the Multisite use case I laid out above remains problematic, i.e. when the CiviCRM Domains are tied to WordPress Subsites in a Multisite context and a admin edits a Contact in CiviCRM where the Contact doesn't have a UFMatch record for that Domain.

christianwach commented 2 years ago

I've been looking again at this yesterday and today as I was keen to get it in to the next release, but have come across some oddities that may mean postponing it - mainly to do with the way the CiviCRM Multisite Extension handles UFMatch updates.

The commit that I force-pushed to the ufmatch branch should "just work" for installations without the CiviCRM Multisite Extension, but for those with, they will need the updated code in this Merge Request.

@rbaugh & @kcristiano I'd really appreciate it if you could test this in your context(s) before I do the next release 👍

andyburnsco commented 2 years ago

I've tested this in multisite setting with both PR's. I did not see the email update in multisite when a user had 2 uf_match entries domain 1 and domain 21 (only a user on domain 21) when doing any of the following:

No email update on domain 21 in this case.

However, contact_id, does successfully update as I've observed previously a uf_match mismatch if a user record is merged on a domain other than the one it is a user on. That's critical for obvious reasons if the linkage is severed, e.g. civicrm groups sync, etc will fail to work.

However, that's not to say that there cannot be multiple UFMatch entries - I still think the Multisite use case I laid out above remains problematic, i.e. when the CiviCRM Domains are tied to WordPress Subsites in a Multisite context and a admin edits a Contact in CiviCRM where the Contact doesn't have a UFMatch record for that Domain.

This relates to the "Merging a uf_match record on a domain ID the user is not a user on" issue that I've never opened up :/ (usually the parent domain ID) does not update the contact_id uf_match entry for the domain the user exists on. I observe that this is NOT the case with 2 PR's. This appears to resolve that.

Before merge

image

After merge

image

christianwach commented 2 years ago

I did not see the email update in multisite when a user had 2 uf_match entries domain 1 and domain 21 (only a user on domain 21) when doing any of the following:

@andyburnsco Hi Andy, thanks for testing, much appreciated. Before I dive in with further testing myself based on your results, can I check with you that your "Sync CMS Email" setting is set to "No" on all the Domains you're working with?

Screenshot 2022-09-08 at 09 06 48

I discovered recently that this plugin doesn't properly override this in WordPress Multisite/Multi-Domain installations and that the Profile Sync settings need to be saved on each Sub-site because CiviCRM settings are specific to each Domain. FWIW it's hard to see how this plugin can automate this because switch_to_blog() does not switch the CiviCRM Domain ID - it has to be done for each Domain/Sub-site - and so looping through Sub-sites programatically won't work.

One other point to mention is that I haven't tested any of this with Contact merging but have put this on my to-do list.

andyburnsco commented 2 years ago

For the 2 domains tested, in UI it showed "No". I do see it is not consistent throughout all of the domains.

Though upon re-saving those 2 domains, I see in the db it changed from image

to

image

I've now set an override in civicrm.settings.php to achieve consistency: $civicrm_setting['CiviCRM Preferences']['syncCMSEmail'] = '0';

I tried inspecting wp_options for both domains to see if the profile sync settings was already saved but I'm not seeing an any entries. I know I've saved at network level: https://example.org/wp-admin/network/settings.php?page=cwps_network and likely domain 1 but not others.

Unfortunately, I can't do more testing till the 13th, I'm about to head out for vacation right now and not bringing the PC :) Can resume then if this is still ongoing at that time.

christianwach commented 2 years ago

Thanks @andyburnsco that's very useful. When the setting was updated, did the UFMatch values change as expected?

I tried inspecting wp_options for both domains to see if the profile sync settings was already saved but I'm not seeing an any entries.

When network-activated, you'll find the Profile Sync settings under the cwps_settings meta key in wp_sitemeta. These propagate to each Sub-site Profile Sync Settings Page, but (as I said above) don't automatically update CiviCRM's settings unless saved on each Sub-site. I'll try and address this alongside these UFMatch issues.

I've now set an override in civicrm.settings.php

Ah, now this is a good idea 👍

Have a great holiday.

rbaugh commented 2 years ago

@christianwach I was able to test the UFMatch branch and it looks like it is working as expected for me. I was able to switch the primary email used as well as making updates to the email itself and the changes were updated for the WP profile and the UFMatch to match the CiviCRM changes.

When I updated the email from the WP side, the CiviCRM record is updated but the UFMatch is not carrying across.

So CiviCRM changes are showing correctly for UFMatch, but WP changes are not.

christianwach commented 2 years ago

I had to do a release yesterday but didn't include this as I think it still needs a lot more testing.

christianwach commented 2 years ago

When I updated the email from the WP side, the CiviCRM record is updated but the UFMatch is not carrying across.

@rbaugh I've added some commits to master that should update UFMatch records when sync happen in that direction.

rbaugh commented 2 years ago

@christianwach, Thanks for continuing the work on this. I will check this out and see how things work.

rbaugh commented 11 months ago

@christianwach It seems I didn't report back on this over the past year. But this has been working and updating as expected.