The "Address book" tab added by Profile module is rough and only covers the "customer" profile type.
We need that tab to include any customer profile type, since shipment types might use their own profile types.
That means removing the Profile-provided UI and generating our own.

Parts:
- third party setting to flag profile types as used by commerce
- custom controller for listing these types

CommentFileSizeAuthor
#27 3059633-27.patch42.69 KBbojanz
#21 Screen Shot 2019-06-27 at 12.12.12 AM.png99.8 KBmglaman
#21 Screen Shot 2019-06-27 at 12.12.00 AM.png118.98 KBmglaman
#15 3059633-15.patch39 KBmglaman
#14 3059633-14.patch31.42 KBmglaman
#12 3059633-12.patch28.05 KBmglaman
#9 3059633-9.patch27.13 KBmglaman
#7 3059633-7.patch27.14 KBmglaman
#5 3059633-5.patch25.21 KBmglaman
#4 Screen Shot 2019-05-31 at 5.20.57 PM.png120.35 KBmglaman
#4 Screen Shot 2019-05-31 at 5.20.52 PM.png151.72 KBmglaman
#4 Screen Shot 2019-05-31 at 5.11.26 PM.png86.97 KBmglaman
#3 3059633-3.patch24.98 KBmglaman

Comments

mglaman created an issue. See original summary.

bojanz’s picture

bojanz commented
Title: Addresses /Address book UI » Provide a better addressbook UI for the user pages
Issue summary: View changes
mglaman’s picture

mglaman
Primary language English
Location WI, USA
commented
Status: Active » Needs review
StatusFileSize
new 3059633-3.patch24.98 KB 
  1. +++ b/modules/order/commerce_order.routing.yml
@@ -45,3 +45,17 @@ entity.commerce_order.user_view:
+  path: '/user/{user}/addresses'
+  defaults:
+    _controller: '\Drupal\commerce_order\Controller\Addresses::list'
+    _title: 'Your addresses'

    I went with Addresses because it seems like Addressbook v Address book has different variance.

    Amazon:
    Your addresses (vs address book)
    Payment options (vs payment method)

    New Egg
    My Address Book
    Payment Options

    Target
    Addresses
    Wallet

    Best Buy

    Addresses
    Payment Methods 

  2. +++ b/modules/order/config/install/profile.type.customer.yml
@@ -5,7 +5,10 @@ dependencies:
-label: Customer
+label: Address

    This was changed per some discussions w/ rszrama, but it still looks weird due to capitalization. Probably keep as Customer and add a display label to profile types? 

  3. +++ b/modules/order/config/schema/commerce_order.schema.yml
@@ -117,3 +117,12 @@ field.widget.settings.commerce_unit_price:
+# @todo: Feels like this should go into the root `commerce` module.
+profile.type.*.third_party.commerce_order:

    See this @todo 

  4. +++ b/modules/order/src/Controller/Addresses.php
@@ -0,0 +1,103 @@
+    // @todo figure out permissions. Do we want more granular for each type?
+    // decorate the route with all profile types which are used by commerce
+    // and check their specific permissions.
+    // @see \Drupal\profile\Access\ProfileAccessCheck.
+    return AccessResult::forbidden()->cachePerUser();

    Somewhere in here there needs to be access checks. Maybe not here but later on when we're processing each profile type?

    Or we could make sure the route is compiled with allowed profile types and check access that way. 

  5. +++ b/modules/order/src/EventSubscriber/ProfileLabelSubscriber.php
@@ -26,7 +27,9 @@ class ProfileLabelSubscriber implements EventSubscriberInterface {
+    $profile_type = ProfileType::load($profile->bundle());

    Use entity type manager to get storage properly. Lazy quick dev. 

  6. +++ b/modules/order/tests/src/Kernel/ProfileTypeThirdPartySettingsTest.php
@@ -0,0 +1,105 @@
+    $local_tasks_manager->clearCachedDefinitions();
+    $this->assertFalse($local_tasks_manager->hasDefinition(sprintf($derivative_key, $new_profile->id())));

    This fails because it was not updated to set to multiple, forgot to fix before upload.

mglaman’s picture

mglaman
Primary language English
Location WI, USA
commented
StatusFileSize
new Screen Shot 2019-05-31 at 5.11.26 PM.png86.97 KB
new Screen Shot 2019-05-31 at 5.20.52 PM.png151.72 KB
new Screen Shot 2019-05-31 at 5.20.57 PM.png120.35 KB

Here are screenshots.

mglaman’s picture

mglaman
Primary language English
Location WI, USA
commented
StatusFileSize
new 3059633-5.patch25.21 KB

Rerolled patch.

mglaman’s picture

mglaman
Primary language English
Location WI, USA
commented
Status: Needs review » Needs work 
  1. +++ b/modules/order/commerce_order.links.action.yml
@@ -15,3 +15,6 @@ entity.commerce_order_item_type.add_form:
+commerce_order.addresses_actions:
+  deriver: Drupal\commerce_order\Plugin\Derivative\AddressesLocalAction

    There should only be one action, "Add address"

    If one profile type, then show form. If more, show list of bundles. Like a typical addPage and addForm flow. 

  2. +++ b/modules/order/config/schema/commerce_order.schema.yml
@@ -117,3 +117,12 @@ field.widget.settings.commerce_unit_price:
+# @todo: Feels like this should go into the root `commerce` module.

    Remove @todo, we keep them here.

mglaman’s picture

mglaman
Primary language English
Location WI, USA
commented
Status: Needs work » Needs review
StatusFileSize
new 3059633-7.patch27.14 KB

Check point. Adds addPage and addForm concepts.

Status: Needs review » Needs work

The last submitted patch, 7: 3059633-7.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

mglaman’s picture

mglaman
Primary language English
Location WI, USA
commented
Status: Needs work » Needs review
StatusFileSize
new 3059633-9.patch27.13 KB

I need to add more CRUD tests for profiles. This just passes with basic interface navigation. It also needs manual testing. This has been just TDD thus far.

bojanz’s picture

bojanz commented

We should also rename classes that start with Addresses to AddressBook. It is confusing to have a plural class name, and we already have AddressBookSubscriber, and an address book as a code concept in general. The UI labels can stay "Addresses".

mglaman’s picture

mglaman
Primary language English
Location WI, USA
commented

Shoot I thought I had that in the comment.

+++ b/modules/order/commerce_order.links.action.yml
@@ -15,3 +15,9 @@ entity.commerce_order_item_type.add_form:
+  route_name: 'commerce_order.user_addresses.add_page'

+++ b/modules/order/commerce_order.links.task.yml
@@ -2,3 +2,8 @@ entity.commerce_order.collection:
+commerce_order.user_addresses:
+  route_name: commerce_order.user_addresses

Like this should be .address_book vs user_addresses and so forth.

@bojanz should I stuff all viable helpers (like loading valid profile types) into the AddressBookInterface? I didn't want to bloat it since it mostly works around managing individual profiles thus far.

mglaman’s picture

mglaman
Primary language English
Location WI, USA
commented
StatusFileSize
new 3059633-12.patch28.05 KB

Update. Renamed some things. Added a helper to get the valid profile types from the address book service.

mglaman’s picture

mglaman
Primary language English
Location WI, USA
commented

The immediate things that I can think of missing tests and reviews:

Clicking to add an address, one profile type. Submit form. Where do you end up?

Clicking to add an address, multiple types. Select type. Submit form. Where do you end up?

And then validating the addresses entered display.

I think the current profile entity form redirects you to the user canonical link. We'd want the addresses route.

mglaman’s picture

mglaman
Primary language English
Location WI, USA
commented
StatusFileSize
new 3059633-14.patch31.42 KB

New patch. Profiles a profile form for address book that redirects to the address book after saving. Needs to test multiple profile types and adding. Also editing.

mglaman’s picture

mglaman
Primary language English
Location WI, USA
commented
Assigned: mglaman » Unassigned
StatusFileSize
new 3059633-15.patch39 KB

Here is an updated patch which adds access control checks.

lisastreeter’s picture

lisastreeter commented

First tested with just the single 'customer' profile type and another non-commerce profile type on a plain install/Bartik theme. Everything looks good and is working smoothly for me.

Then updated the config for the second profile type to make it commerce_profile_type. That type was then properly combined into the main "Addresses" tab for the user. Both bundle types displayed as options for adding a new address, and I was taken back to addresses after submission. So still all good.

But...
Out of curiosity, I then tried setting my second profile type to not "Allow multiple profiles per user". (It only had one at the time.) My thinking is that you could want a single ("approved") billing address but multiple shipping addresses. When I did that, that section of addresses completely disappeared from my "Addresses". So it seems that the third party "commerce_profile_type" setting and the "Allow multiple profiles per user" settings are incompatible. I haven't had time yet to investigate why this is so.

bojanz’s picture

bojanz commented

#16 is a good point, we need to account for the "single" use case.

Also, our routes should use "address_book", not "addressbook", to match other code.

morbus iff’s picture

morbus iff
they/them
commented

I'd much prefer "address-book" to match the age-old "core prefers dashes for URLs" soft pattern.

@See
- admin/structure/display-modes
- admin/content/media-widget
- admin/content/media-widget-table
- etc.
- Default pathauto replacement character of -

(I might have misread you wrong; if the path of "addresses" isn't viable, I'd much rather see it be "address-book" instead of "address_book". If you're literally talking about route names vs. paths then, my mistake and sorry for the noise.)

bojanz’s picture

bojanz commented

It's about route names, the URLs are using "addresses" for now. We try to never use underscores in URLs.

mglaman’s picture

mglaman
Primary language English
Location WI, USA
commented 
+++ b/modules/order/tests/src/Functional/AddressBookTest.php
@@ -0,0 +1,354 @@
+  /**
+   * Test with a non-multiple profile type.
+   */
+  public function testWithNonMultipleProfileType() {
+    $bundle_entity_duplicator = $this->container->get('entity.bundle_entity_duplicator');
+    $customer_profile_type = ProfileType::load('customer');
+    $bundle_entity_duplicator->duplicate($customer_profile_type, [
+      'id' => 'shipping',
+      'label' => 'Shipping',
+      'multiple' => FALSE,
+    ]);
...
+    $this->drupalGet($customer->toUrl());
+    $this->assertSession()->linkExists('Test');
+    $this->assertSession()->linkExists('Shipping');
...
+    $this->getSession()->getPage()->clickLink('Addresses');
+    $this->assertSession()->linkExists('Add address');
+    $this->getSession()->getPage()->clickLink('Add address');

Weird. This test asserts profiles owned by commerce which are single use falling back to the default profile UI.

mglaman’s picture

mglaman
Primary language English
Location WI, USA
commented
StatusFileSize
new Screen Shot 2019-06-27 at 12.12.00 AM.png118.98 KB
new Screen Shot 2019-06-27 at 12.12.12 AM.png99.8 KB

I tried to reproduce #16 and cannot. HOWEVER

1. I used the Duplicate functionality to create my profile type
2. I had to run a cache rebuild for the local tasks to appear properly (profile problem, needs resolving and has been an age-old problem.)


rszrama’s picture

rszrama commented

fyi, bojanz's on vacation but would love to see if Lisa can still apply the patch clean against dev (now that address book is initially committed) and report on if she can reproduce what she saw in #16. (Also, from a wordsmithing standpoint, I think we'll need to revert to "address book" b/c I had a real hard time coming up with sentences for various UI labels / descriptions in the checkout and admin screens without referring to an address "book" ... seemed easier to resolve the discrepancy by just sticking w/ the old school address book label for now.)

bojanz’s picture

bojanz commented
Status: Needs review » Needs work

I am certain Lisa's bug is legit because we don't consider "single" profile types to be Commerce profile types:

+  public function getProfileTypes() {
+    /** @var \Drupal\profile\Entity\ProfileTypeInterface[] $profile_types */
+    $profile_types = $this->profileTypeStorage->loadByProperties([
+      'multiple' => TRUE,
+      'third_party_settings.commerce_order.commerce_profile_type' => TRUE,
+    ]);
+    return $profile_types;
+  }

See the "multiple" condition, which shouldn't be there.

(And as a sidenote, that method needs to be called loadTypes(), to match loadAll() and loadDefault()).

Updating status for that and #22. I had a reroll in progress but it will need to wait until my first day back (thursday).

lisastreeter’s picture

lisastreeter commented

Could not apply patch #15 on a clean site with commerce dev:

$ composer require 'drupal/commerce:2.x-dev'

Given @bojanz's comment, I will wait on the reroll before trying again:

I had a reroll in progress but it will need to wait until my first day back (thursday).

morbus iff’s picture

morbus iff
they/them
commented

I've marked #3068222: Customer profile display_label missing in shipped YAML as a duplicate of this one, but it does contain a one-line change that should probably be included in this one, for @bojanz's forthcoming reroll:

diff --git a/modules/order/config/install/profile.type.customer.yml b/modules/order/config/install/profile.type.customer.yml
index 68b2e02f..dd3c0038 100644
--- a/modules/order/config/install/profile.type.customer.yml
+++ b/modules/order/config/install/profile.type.customer.yml
@@ -6,6 +6,7 @@ dependencies:
       - commerce_order
 id: customer
 label: Customer
+display_label: 'Customer information'
 multiple: true
 registration: false
 roles: {  }
bojanz’s picture

bojanz commented

The part around customer profile types was the most unfinished (no profile type form checkbox, alters not modified to check for the flag, etc), so I've extracted that into a new issue which is now ready to land: #3072083: Allow any profile type to be used as a "customer" profile type.

The patch here will be rerolled once that happens. The goal of the reroll is to address the following problems:
1) We need to revert to the Address Book terminology in the UI, urls, code, as mentioned in #22.
2) The UI doesn't account for "single" profile types in any way. We need to show them in the address book tab, and we need to make sure they don't get add/delete/"set as default" links.
3) Since not every profile type is available on the "add" screen, the current single "Add address" link becomes confusing. We need to revert back to having an "Add address" link per profile type.
4) If the site has a single profile type, and that profile type is "single", we should hide the Address Book tab and allow the default profile UI to be used (tab with the profile type's display label). There is no point in having an address book for a single address.
5) The edit/delete/"set as default" operations currently use the default routes and forms, meaning that the customer is not redirected back to the Address book page, they're redirected to the user page instead. We need to provide our own routes/forms to fix that.
6) Now that we have our own operations, we need to make the listing use them. We need to either do a hook_entity_operations_alter() and swap them out if we detect the address book route, or replace the default profile view with our own view or twig-based listing. We used Twig for wishlist and it was quick and easy, so that's the plan here as well.

bojanz’s picture

bojanz commented
Status: Needs work » Needs review
StatusFileSize
new 3059633-27.patch42.69 KB

This addresses all points from #26, and works with the just released Profile 1.0. Should be close to final.

  • bojanz committed 3b08528 on 8.x-2.x 
    Issue #3059633 by mglaman, bojanz: Provide a better addressbook UI for...
bojanz’s picture

bojanz commented
Status: Needs review » Fixed

Improved the CSS a bit and committed.

I'm really happy with the non-Views implementation of this UI. It's short and to the point.
People swapping out the controller, overriding the CSS, should have an easy time customizing things.
Plus, we've opened the door to future improvements (modals, etc).

I'd like to thank rszrama for bikeshedding the smaller details (empty texts, relation to add links) in the last minute, as aways.

Followups:
#3077908: Update the Address Book service for Profile 1.0
#3077935: [Address Book] Add a CSS class to the default profile in the user pages

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.