[Addressbook, part 1] Rework the ownership model for customer profiles

Sounds good!

We are developing a usecase where we want to limit certain users to only be able to use specific addresses for billing and shipping (a B2B scenario), and not allow them to edit those addresses.
We currently control the addresses in their profile and limit access via hook_ENTITY_TYPE_access().
The proposed solution will work well for them.

Will the reused-address edit & copy-back box processing respect Profile access or be controllable some other way?
Will the source Profile address dropdown content be easily controllable (if it needs it to be)?
thanks

I have a B2B type scenario that I plan on implementing too. I am going to create a Company entity that users can be linked to. A Company will store its own billing / shipping profiles. When a customer (or employee on the admin side) goes to create an order, I'd like to be able to show Company addresses in the profile widget (if user belongs to a Company). User's can have their own personal addresses as well (normal commerce functionality).

The scenario from #2 would also be applicable for this too, because we may want to restrict who can edit their company addresses.

The big reason for doing this is because it isn't terribly uncommon for companies to change addresses. If we were to just copy the company profiles to each user that belongs to the company, we'd end up with a lot of stale profiles if the company changes addresses.

I'm hoping this scenario will be something I can achieve with the upcoming changes to profile.

Reviewing and taking a stab at this.

Created #3039163: Create a "data" field using a map data type..

When editing reused profile information the addressbook profile is not automatically updated. Instead, we make it an explicit operation (a checkbox that can be on/off by default), which copies back

For updating the "source" profile. We somehow need to keep a reference to the original profile entity. We need to introduce a $profile->data field. The source ID can stick there to allow updating the addressbook reference. There is also a need for $profile->data to hold a flag for when converting profiles after anonymous checkout.

Here is a starter patch. It abstracts the three times we retrieve a billing profile and push it into an InlineForm. For now, \Drupal\commerce_order\CustomerBillingProfileTrait::ensureParentProfileCopy provides the logic that:

* Takes billing profile value passed from the "parent" entity (order, payment method)
* If it is not null, return that value. It has a billing profile it owns.
* If it is null, load the user's default profile, If there is not a default, create a stub entity
* Create a duplicate of the existing profile entity and change its owner to anonymous.

The existing tests seem to pass with this, which they should. The next step is to make more assertions specifically around billing profiles.

I found a bug in Address which prevents the default address widget from populating, because the cloned profile is technically new: #2838457: Re-enable the default value functionality for Address fields.

This is a patch which provides the start of a basic test. The address never has a value due to this bug.

This is officially blocked on the Address issue for a default value. Here is an updated patch where the problem was resolved locally. Test passes and profile is reused, data is exposed in the form.

Talked w/ Bojan and I got a bit ahead of myself. The above focuses more on reuse versus ownership alone. There is still a blocker here, on Profile's data field. New patches incoming.

Here is an initial patch which ensures all billing profiles generated are set to uid = 0.

Checkout panes generate profile stubs belonging to uid = 0 instead of the order or payment method's owner. Orders and payment methods ensure the profile owner is uid = 0.

That leaves the next steps to be:

1) Checkbox in CustomerProfile that says "Add this address to my address book", sets $profile->data flag
2) Code that detects the flag and creates a duplicate of the order billing profile, then assigns it to the user

For #2, that can be an order placed subscriber for checkout panes. For payment method add forms outside of checkout... TBD.

PATCH WARNING: This adds hook_base_field_info to add a data field to profile for testing. This is so that we can write tests that pass until #3039163: Create a "data" field using a map data type. lands.

This adds a checkbox which allows saving an address entered at checkout to the user's addressbook. There is an event subscriber that creates a copy of the order's billing profile to the user's address book when flagged.

---

There seems to be a legitimate regression here, though.

1) Drupal\Tests\commerce_promotion\FunctionalJavascript\CouponRedemptionPaneTest::testCheckoutSubmit
Failed asserting that two strings are equal.
--- Expected
+++ Actual
@@ @@
-'John'
+'Johnny'

The test code:

    // Go back and edit the billing information, but don't submit it.
    $this->getSession()->getPage()->clickLink('Go back');
    $address_prefix = 'payment_information[billing_information][address][0][address]';
    $this->getSession()->getPage()->fillField($address_prefix . '[given_name]', 'John');
    $this->getSession()->getPage()->fillField($address_prefix . '[family_name]', 'Smith');
....
    $page->fillField('Coupon code', $coupon->getCode());
    $page->pressButton('Apply coupon');
    $this->waitForAjaxToFinish();
...
    $page = $this->getSession()->getPage();
    $given_name_field = $page->findField('payment_information[billing_information][address][0][address][given_name]');
    $family_name_field = $page->findField('payment_information[billing_information][address][0][address][family_name]');
    $this->assertEquals($given_name_field->getValue(), 'Johnny');
    $this->assertEquals($family_name_field->getValue(), 'Appleseed');

Submitting the coupon pane is causing the entire form to submit and change the billing information.

+++ b/modules/order/src/Plugin/Commerce/InlineForm/CustomerProfile.php
@@ -92,6 +92,10 @@ class CustomerProfile extends EntityInlineFormBase {
+    $inline_form['add_to_addressbook'] = [
+      '#type' => 'checkbox',
+      '#title' => $this->t('Add this address to my address book'),
+    ];

This needs to check if the profile has been flagged or not and set it as a default value.

Adds the checkbox.

Picking this back up today.

Fixes a logic check which should resolve most failures (ie: order does not have a billing profile attached.)

A quick rundown of follow-ups for the next patch:

- Add a Kernel test against BillingProfileConvertSubscriber
- Ensure added Functional* tests assert the checkbox works during checkout to convert billing profiles

Adds kernel tests for: - Add a Kernel test against BillingProfileConvertSubscriber.

Lint fix, clean up functional tests.

+++ b/modules/order/src/EventSubscriber/BillingProfileConvertSubscriber.php
@@ -0,0 +1,115 @@
+    $data = $billing_profile->get('data')->first()->getValue();
+    return isset($data['add_to_addressbook']) && $data['add_to_addressbook'] === TRUE;

This strict test is causing the copy test failures. It is not being saved as a strict boolean value.

This should bring us back to just the failure for CouponRedemptionPaneTest::testCheckoutSubmit where submitting a coupon causes the profile field values to submit as well, when they previously did not.

This marks \Drupal\Tests\commerce_promotion\FunctionalJavascript\CouponRedemptionPaneTest::testCheckoutSubmit as skipped, for now.

It also fixes a bug where the "Add to addressbook" was always selected due to `__isset` call.

Handing over to bojanz for review.

Our site needs different fields for company and person like this:

I would guess it may be possible then shipping/billing profiles are separated in combination with "Conditional fields" module.

I struggle to understand current state of profiles and how I am supposed to make it work. Hopefully, profiles will improve some day.
The most complex part is why shipping and billing is the same profile by design, then information on both have to be totally different depending on various situations.

Note on testing: ensure you get latest -dev of Profile module as well, as indicated in #29.

Is a new Commerce patch imminent or is this one pretty stable?

Would it also make sense to move to a standard entity reference field? It seems that the solution outlined in the issue description would no longer need entity_reference_revisions...

I ask because using a standard reference field would simplify the setup and drop a dependency on something we don't appear to be utilizing anymore (and make writing order & order profile migrations simpler).

and why can't we require Drupal 8.7 already now?

Our plan is to support both, and document when each option makes sense.

Glad to hear this. With this you will cover my country, since every shop has two profiles here. And it makes total sense, don't understand the other possibility. Maybe laws, local traditions or what not different, don't yet know why single is ok in other places. Looking forward for that documentation.

Its two profiles, because payment/shipping is in two steps. And those are two steps, because we have gazillion payment and shipping options. Maybe small countries have a lot of options, and big countries/international can only use few universal options, so they can fit those options on a single page. But for us it becomes very natural to have two profiles, because shipping and payment becomes very diverged.

And another big lack of commerce becomes again lack of those shipping and payment options. But I hope Commerce will get better and become more popular so those payment/shipping providers will create modules in some time.

can be done with separate form modes

Its ok, as long as it can be done.

You'd put "delivery instructions" on the shipment itself, so they're not profile material.

Yes, I already have those fields on shipment. But shipment itself is lacking a bit since shipping methods diverge too as I described here #3044641: Allow the shipping method to determine shipment type

Sorry, I write out of topic. The thing I understand now - it will be possible to have different fields on billing and shipping profile. That makes everything else mostly possible. Thanks.

@bojanz

Is the patch you are dropping going to be compatible with 8.7? I updated to 8.7 a few days ago and did have problems because with the DB schema updates making menu link content revisionable because I had a custom image field on my menu content, but that's taken care of now thanks to the work of the community, I would rather just stay at 8.7 at this point, but having registered users not have to type in their address over and over is critical, so I may have to roll back if there is no other way.

Thanks for your work.