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

added a new class `CiviCRM_WP_Profile_WC_Sync` in a separate class to… #8

Closed agileware closed 4 years ago

agileware commented 7 years ago

… provide two-way sync between primary phone, parimary address and billing address of CiviCRM user and billing phone, shipping addres and billing address of WP user metadata added by Woocommerce. Refs #24478

@christianwach Thanks very much for your suggestions.

  1. Actually The second PR in https://github.com/christianwach/civicrm-wp-profile-sync/pull/6 is a mistake. At that time the code was not ready to be the PR yet, I just wanted to push it to my fork so that it can be saved somewhere remotely. But I forgot that I did not close the first pull request. So the github assume that push is still for the first pull request. That is a total misunderstanding and sorry for any inconvenience caused.

  2. I have moved all my changes into a separate class in a separate file called woo-sync.php, without any changes on your original code, but just with a new line added, for making the new code in the new file running. Ref: https://github.com/christianwach/civicrm-wp-profile-sync/pull/6#issuecomment-266691484

  3. Also, I paid more attention on preventing the recursion while updating data in both WP or CiviCRM ends. The code was also syncing billing email between woo and CiviCRM. But I tried not to modify any code in your original civicrm-wp-profile-sync.php file, so I removed this function. As it would cause recursion and there is no good way (there are few ways but I do not think they are quite good options) to run private add and remove hooks functions outside of the class. Also the functions added will not trigger your sync hooks. It will just handles its own recursion prevention. Ref: https://github.com/christianwach/civicrm-wp-profile-sync/pull/6#issuecomment-251899779

  4. In addition, I have tried my best to make the coding style stay in consistency with the original code. If there is still any inconsistency, please point it out and i will fix them. Ref: https://github.com/christianwach/civicrm-wp-profile-sync/pull/6#issuecomment-266691484

Best wishes

christianwach commented 7 years ago

@agileware Thanks very much for the new PR and for implementing the changes I suggested. It looks much better to me!

What I would really appreciate now are the steps needed to test this with WooCommerce. I have a local install of Woo and Civi that I can test with, but I'd like to know how to set it up appropriately. Once I've tested the functionality, I'll make sure the code gets included in this plugin.

Cheers, Christian

agileware commented 7 years ago

@christianwach, thank you so much for your comment. I couldn't be happier to hear that.

  1. Brief explanation before functionality test.
    • The Woo will add extra user metadata for each user after installed. If we go to any user profile page after Woo's installation, we could see two new sections Customer Billing Address and Customer Shipping Address with other new-added user metadata fields under them.
  1. Brief explanation on what new feature Woo sync does.

    • two-way sync (edition, creation, deletio) between billing address in WP user metadata and the first-found address that is marked as Billing location for this contact in CiviCRM.
    • two-way sync (edition, creation, deletio) between shipping address in WP user metadata and the address that is marked as Primary location for this contact in CiviCRM.
    • two-way sync (edition, creation, deletio) between billing phone in WP user metadata and matched contact's primary phone in CiviCRM.
    • sync will ignore address and phone types in CiviCRM, only address that has been marked as billing or primary and only the primary phone will be focused.
    • sync will ignore first name, last name, email (as this function is provided by wp-profile-sync originally) and company fields in Woo-added user metadata.
  2. For now there is no settings required for this new feature.

  3. Test steps that I used are pretty much simulating how administrators and normal users are using this feature:

    1. Visit My account page as a visitor, register as a normal user of the site and log in using the email address.
    2. Edit shipping address and billing address in address section.
    3. Log into the site as administrator, in CiviCRM search for the contact that is synced by WP-profile, also check the address and phone num that have been synced.
    4. Edit the address information in CiviCRM (update or delete).
    5. Go to administrator all user profile page and check whether the change has been synchronised here.
    6. Visit the site as the normal user, go to my account address page and check changes from CiviCRM in here.
    7. If we can, we could also log in as administrator again and edit this user's metadata, then check if it has been synced in CiviCRM and normal user end.
  4. Further explanation.

    • As the feature is, at this stage, providing two-way sync between billing address and billing address in CiviCRM and shipping address and primary address in CiviCRM. There could be a situation that an address in CiviCRM is both billing address and primary address. In that case, any changes will be synced to/from that address.
    • if user provides the billing address first, new address created in CiviCRM will be automatically marked as primary address as well. Then any changes in shipping address will be synced to this address. This issue, that this stage, might require administrator to fix.
  5. A new commit pushed to the PR as I found a bug that the new feature is not hooked into user metadata adding process. So the first edition of addresses from My account page is not detected by the code and will not be synced. After hooking into added_user_meta, it is fixed.

Best wishes

christianwach commented 7 years ago

@agileware Many thanks for the detailed information! I have more than enough to test now.

Cheers, Christian

christianwach commented 7 years ago

@agileware Just to let you know that I'm working my way through this in my spare time and am making headway.

christianwach commented 7 years ago

@agileware You can find my progress integrating your work into the existing plugin by looking at this development branch. In summary, I merged your code, added documentation and changed the way it is initialised. I also fixed a couple of minor issues.

The only issue that I've encountered so far (as you note in an inline comment) is the "state/county" sync. WooCommerce doesn't have UK counties loaded by default and the plugin that they recommend has different codes to those stored in CiviCRM. Somerset, for example, is SOM in CiviCRM and SO in Woo. SO in CiviCRM is Somali, which is definitely not in the UK!

I'll revisit states for other countries and see if there are any further mismatches. It may be that we have to provide our own modified list via the woocommerce_states filter so that CiviCRM and Woo have the same codes.

Apart from that, this is a great enhancement - thanks for making it happen!

christianwach commented 7 years ago

I'll revisit states for other countries and see if there are any further mismatches.

Having said this, there is a hell of a lot of data to wade through... and I've found plenty of mismatches already. I wonder what other strategies there could be rather than a manual review?

agileware commented 7 years ago

@christianwach Thanks a lot for your effort for adding this feature into the plugin.

Apology for not responding to your comments. Somehow I couldn't receive the notification, so I just check this conversation randomly.

Yes that state/county short names is problematic. During development I was only testing states in AU and US, but I did realize that there are many mismatches. To be honest, I haven't figured out a good solution to that, as initially I thought that is an universal rule for these short names... So I did not worry about this bit very much.

I guess manually create this mapping for popular countries could be the best trade-off that I can think of for now.

Again, much appreciation for your work.

christianwach commented 7 years ago

@agileware So I've been look at this a bit more and have fixed a few things, most notably accounting for times when Civi's API returns data where the value is (string) "null", which was kind of a WTF moment for me. Never knew it did that.

I'm running into a bit an issue with addresses in that my Woo install initially had wc_shipping_enabled() as false. This meant that only the billing_address was available in '/my-account' although both billing and shipping were available in '/wp-admin' (huh?!). In Civi, existing users had billing_address = shipping_address and nothing I could do would separate these two addresses. I assume this is what you mean by "this stage, might require administrator to fix" in that I have to do this manually in Civi. I wonder if we should test wc_shipping_enabled() and act accordingly? It might clarify the sync procedures in the different situations...

So my thinking at present is that:

  1. if wc_shipping_enabled() returns true and a Civi contact has a single conflated primary (shipping) and billing address, then we should format their addresses such that primary is of location_type = "Home" (and that this is the shipping address) and billing is of location_type = "Billing" (duh) and separate them out - assuming they have provided both addresses in Woo.
  2. if wc_shipping_enabled() returns false and a Civi contact has both primary (shipping) and billing address, we should mark billing as primary but keep the address with location_type = "Home" in case wc_shipping_enabled() is switched again.

Having written all that, I wonder if it wouldn't just be easier to auto-create a new location_type = "Shipping" assuming the Civi API allows this? There is, after all, no reason why shipping_address has to be the same as location_type = "Home". This would cut through the assumptions that are being made about the mapping between Civi's addresses and Woo's addresses and allow us to sidestep the fact that Woo has no concept of a "Home" address.

I've updated the https://github.com/christianwach/civicrm-wp-profile-sync/tree/woosync branch if you want to check out the changes thus far.

Cheers, Christian

christianwach commented 7 years ago

@agileware I've run into another problem that I'm hoping you can help me unpick: when WordPress and CiviCRM are configured to use a language other than English, then querying for location_type_id = "Billing" and/or location_type_id = "Home" does not always work.

I happened across this because I run a WordPress/CiviCRM install in German where, for example, "Home" is actually "Startseite". "Billing" appears to be the same but this may not be the case for other languages. So it seems to me that we either have to use WordPress translations via __() or CiviCRM translations via ts() to find equivalences and therefore the relevant location_type_id. It's either that or we have to provide a WordPress admin page where we ask users to identify the relevant Location Types manually - and when they have done, we store the mapped IDs.

Thoughts?

christianwach commented 7 years ago

Further information on this issue:

Seems like we're going to be hamstrung until these issues are resolved in core. The question then remains: how to do something now that doesn't get screwed over if/when the core issue is resolved?

christianwach commented 7 years ago

After reading the tickets more closely, it seems to me that the cleanest approach is to create a new location_type = "Shipping" on plugin activation (though subsequent checks may be necessary because there are no guarantees that plugin upgrades trigger the activation method) with the is_reserved flag set to true (same as location_type = "Billing" which is part of the default install) so that it cannot be deleted. This means we can ignore the is_primary flag completely.

Since WooCommerce requires only the "Billing" address (because it can be configured for shops with no physical goods and therefore no need for a "Shipping" address) the new location_type should only be created if wc_shipping_enabled() === true.

The sync should therefore become clearer:

I think this is the way to go and would be happy to include this schema in a future release of this plugin. Doing so any other way would introduce potential upgrade woes that I'm certain neither of us has the time to attend to!

christianwach commented 7 years ago

@agileware Any thoughts on this?

jusfreeman commented 6 years ago

@christianwach sorry for not replying earlier - we changed out Github user name and as a result, lost all notifications.

Should we pick this up and continue work / review on it?

Agileware Ref: CIVICRM-565

mecachisenros commented 6 years ago

@jusfreeman FYI phone/address/email two way sync feature is implemented in my fork of Veda Consulting WooCommerce integration, and I believe it was merged.

jusfreeman commented 6 years ago

@mecachisenros thanks for the tip, that's great. We'll follow your fork for the time being. Have a fabulous day mate!

mecachisenros commented 6 years ago

Another FYI there's another fork which seems to support campaigns and it's more active :)

jusfreeman commented 6 years ago

@mecachisenros forking heck! It's forking everywhere :)

christianwach commented 4 years ago

Closing since this functionality is provided by other plugins.