Allow to set a list of preferred countries

Pull request is available on github : https://github.com/bojanz/address/pull/25
And patch is attached.

Add summary to preferred countries. This patch is in relation and dependance with #2633912.

Compile commits. I only keep first patch related to #2633912.

I've used the patch of #5 on address-8.x-1.0-beta2 and it works. On the "Manage form view" settings page of the address field now also preferred countries can be selected. The default country and the preferred countries are now also displayed directly on the "Manage form view" which is a UX improvement I like.

There is one possible improvement I miss which would be a divider between preferred countries and the full list of the countries. The divider possibly should be managed in the field settings. What do you think?

Rerolling the patch #5 to apply to current dev version and adapting content of method settingsSummary() due to introduction of this method since patch #5 in the unpatched module to avoid duplication by patch.
The devider has not yet been added to the select box.

Patch no longer applies to dev. It also doesn't include an update hook for existing address fields.

For the divider, do we want to do this via:

A) option groups (e.g. making #options a nested array with 'Preferred' and 'Other' labels)
B) insert a bogus '-----' option that we disable
C) other

?

Thanks,
-Derek

In IRC, bojanz said:

people usually do B when they have to. A def sounds preferable

So here's a re-roll that:
- Works with #2689089: Define a form element type "address" to support #preferred_countries in the 'address' form element.
- Uses optgroup to separate preferred and other countries in the select list by defining #options as a nested array.

I began to port the changes to the automated test into tests/src/FunctionalJavascript/AddressDefaultWidgetTest.php. However, I couldn't get the new assertOptionOrder() method to work properly given the presence of optgroup tags. Frankly, none of the test assert methods related to select options that I tried seem to handle optgroup. :/ So, I just left those hunks of the patch out. If someone (especially if you're fluent in xpath hackery) wants to run with it, see 2626982-12.preferred_countries.broken-test.patch for my initial (failed) attempt.

Possible remaining todo items:
A) Should the labels for the optgroups be configurable? Currently, they're translatable, but not configurable in the UI.
B) Should the UI for selecting preferred countries in the form widget settings page honor the #available_countries list in the field settings?
C) Should we get the test for preferred_countries working again?

Re: @pianomansam in #9

It also doesn't include an update hook for existing address fields.

What do we need an update hook for? We're not changing any existing field/widget settings. We're just adding support for a new one. I don't think we want to touch existing field configuration for this change at all. Or are you talking about adding an empty DB update to trigger a cache rebuild to ensure the field widget settings config schema changes take effect?

Thanks,
-Derek

- Converted this to a proper issue summary and added the remaining tasks section with the todo items from comment #12 (and a few others I thought of).
- Crossed off one of those items by removing todo 12.B. Both of the widget settings that let you select countries now honor the available_countries field setting.

However, I'm not entirely sure the change here is the right one. I'd rather reuse AddressItem::getAvailableCountries() which has some caching and the whole AvailableCountriesEvent support. Sadly, I couldn't figure out how to access the underlying FieldItem from a given WidgetBase (at least not in settingsForm() -- some of the other methods have a list of items passed in).

I suppose I could duplicate the code from src/Plugin/views/filter/CountryCode.php that does some unholy things to enable this to work: $entity->get($field_name)->appendItem()->getAvailableCountries();, but that seems dirty and wrong. Maybe that's the best we can hope for. Seems pretty lame that the widget doesn't have a reference to the field anywhere. Maybe I'm missing something.

Re-roll against latest 8.x-1.x and adds support directly in the address_country form element.

The TODOs from the summary are still basically valid. In particular, I'm wondering about making these labels configurable, and if so, how.

But this is getting closer. Reviews welcome.

Thanks,
-Derek

And here's a version that adds support for this into the views CountryCode filter.

And here's a version that applies cleanly to 8.x-1.3 after the patch from #2514126-102: Add field settings for global overrides of required/optional behavior is committed.

This can be used for sites that want both features now.
And once #2514126 hopefully this could go in soon thereafter without another re-roll.

Argh, forgot to set the 'do not test' flag when I attached #17. Here's a re-upload of #16 so the bot doesn't get confused.

And here's a pair of patches that let you configure the optgroup labels for the views country filter. Again, there's a version that applies cleanly with [ #2514126-102] applied.

Do we want the field widget configuration to work roughly the same way for the optgroup labels in the form element? Or is there a cleaner/better way to let people configure this?

Thanks,
-Derek

Or, *gasp* should the label config (and country list?) only live in the widget, then in the views filter code, we do something (potentially unholy) to pull the widget settings out of the field that the filter is pointing to, and re-use the same labels from the widget? This sounds dreadful to implement, but perhaps more what users would expect.

Then again, I can imagine situations where you want your field UI to have 1 list of common/preferred countries for data entry, while your views filters might want a different set for searching. Or whatever.

I thought it was safer (and way easier to implement) to keep the config totally separate between widget settings and views filter settings. But if bojanz will only commit this if the 2 are merged, I'm willing to try. ;)

Thoughts?

Thanks!
-Derek

An important UI improvement I definitly need for my current project (or any other address field deployment).
Here's just a straight reroll of #20 so it applies against latest dev.

Thanks for the re-roll, @Pancho! I had sort of forgotten about this issue. ;)

I tried to interdiff between #20 and #23 and my copy of interdiff couldn't handle it. ;) So I'm attaching a "raw" interdiff from this:

diff 2626982-20.preferred_countries.patch 2626982-23.preferred_countries.patch > 2626982.20_23.interdiff.txt

I noticed we lost a minor (but IMHO useful) code comment, since AddressDefaultWidget::formElement() is pulling different settings from different places and it seems helpful to alert readers to this fact.

Unassigning, since I have no bandwidth to work on the rest of the remaining tasks right now.

Would love feedback from @bojanz about it, first.

Thanks,
-Derek

I tried to interdiff between #20 and #23 and my copy of interdiff couldn't handle it. ;)

That’s because the old one doesn’t apply anymore which is why we needed a reroll in the first place.
However theoretically it should be possible to have an interdiff compare the old patch against the old version with the new patch against current dev. I’ve just not yet figured out how.

Thanks for readding the comment I missed. I’m gonna provide screenshots as soon as I get to it.

I don't think we really need the "Preferred" optgroup at all. The preferred ones are at the top, it's obvious they're shortlisted, and everything else is visual clutter IMO.
Plus: The "Other" optgroup is fine as it is, and I don't think we need a configurable label for this one.

Here are some screenshots:

Before:

#24:

#26:

Further issues:

1. consider renaming to "Shortlisted countries" ("preferred" is a bit judgemental)
2. add all the same features to CountryItem; (fixed in #27); don't know about ZoneItem
3. automatically shortlist the default country if set (think that's obvious, once #2838457: Re-enable the default value functionality for Address fields is in)

Next patch, adding the same feature for CountryItem (#26-2).
Does the amount of duplicated code justify an AddressWidgetBase class, a trait or some helper method?

And of course we need formatPlural() for the settingsSummary.
That's it for the moment. Waiting for some feedback now, including on the remaining questions in #26 and #27.

All in all while #2838457: Re-enable the default value functionality for Address fields would be nice, too, I think this is even more helpful. On non-required fields, you often want the default country to be "none", so the other address fields don't pop up. The more is a shortlist of the most relevant countries a big plus in terms of usability.

Fixed another edge-case:
If all available countries are shortlisted, we don't want to show the "Other" optgroup either.

Re: #26: Cool, good point that we don't need an optgroup label for the "preferred" list. Nice thinking!

Re: #26.1: Absolutely. "Preferred" is indeed judgmental, as you say. That's why I wanted to make the label configurable. However, "shortlisted" is a bit awkward. Some possible alternatives:

1. Common countries
2. Commonly-used countries
3. Frequently-used countries
4. Regularly-used countries

I don't love any of these, but I don't have a better suggestion (yet).

Agreed that "Other" is fine, without a config knob, for the (remaining) opt group.

Re: #26.3: yup, sounds good.

Thanks!
-Derek

I'm fine with "Preferred countries" as originally proposed by @GoZ. I'm fine with "Top countries," too. The wording is secondary, now that's it is no more user-facing.

Split #3034910: [PP-2] Automatically add the default country, if set, to the shortlist of "preferred countries" to a separate followup.

So we're only missing tests. Will add some, before I'm out for a week (on holidays).

The patch wasn't yet ready, as there were several glitches in the exposed filter form, in some of the various permutations of "required", "multiple", and "limit". I reimplemented the whole logic in a separate trait, so we're not unnecessarily duplicating this code.

From my manual testing, it seems to work now, but please give it another round of manual tests as well. We should then have a few functional tests as well as unit tests for the various permutations. Unfortunately, I'm out for a week, on holidays, so unassigning. If someone wants to step up and go ahead with the tests, feel free!

Latest patch doesn't work in the latest commit (9cbec9f)

$ git apply 2626982-34.preferred_countries.patch
error: patch failed: config/schema/address.schema.yml:148
error: config/schema/address.schema.yml: patch does not apply
error: patch failed: src/Element/Country.php:91
error: src/Element/Country.php: patch does not apply
error: patch failed: src/Plugin/Field/FieldWidget/AddressDefaultWidget.php:101
error: src/Plugin/Field/FieldWidget/AddressDefaultWidget.php: patch does not apply
error: patch failed: src/Plugin/views/filter/CountryCode.php:11
error: src/Plugin/views/filter/CountryCode.php: patch does not apply

Indeed. Doesn't even work with patch -p1:

% patch -p1 < 2626982-34.preferred_countries.patch
patching file config/schema/address.schema.yml
Hunk #1 FAILED at 148.
Hunk #2 succeeded at 181 with fuzz 2.
1 out of 2 hunks FAILED -- saving rejects to file config/schema/address.schema.yml.rej
patching file src/Element/Address.php
Hunk #1 succeeded at 189 (offset 6 lines).
patching file src/Element/Country.php
Hunk #4 FAILED at 101.
1 out of 4 hunks FAILED -- saving rejects to file src/Element/Country.php.rej
patching file src/Plugin/Field/FieldWidget/AddressDefaultWidget.php
Hunk #1 FAILED at 101.
Hunk #2 FAILED at 110.
Hunk #3 FAILED at 122.
Hunk #4 FAILED at 153.
Hunk #5 succeeded at 145 with fuzz 2 (offset -87 lines).
4 out of 5 hunks FAILED -- saving rejects to file src/Plugin/Field/FieldWidget/AddressDefaultWidget.php.rej
patching file src/Plugin/Field/FieldWidget/CountryDefaultWidget.php
patching file src/Plugin/views/filter/CountryCode.php
Hunk #2 FAILED at 14.
Hunk #3 FAILED at 27.
2 out of 3 hunks FAILED -- saving rejects to file src/Plugin/views/filter/CountryCode.php.rej
patching file src/PreferredCountriesTrait.php

@Pancho: Any chance you're setup to easily re-roll this for the latest 8.x-1.x branch? If so, I'd love to help get it finished and into Git ASAP.

Thanks!
-Derek

Here's the rerolled patch. There's no way to create an interdiff because the last patch is not applying.

Thanks for all your work on this! I needed similar functionality but for webforms, and found I had to tweak this slightly in order for it to work. I've included my patches here: https://www.drupal.org/project/webform/issues/3097456

I'm sure this isn't necessary for the first build of this feature (or at all), and there are definitely some tweaks that need to be made, but I thought I'd share in case anyone else was looking for the same thing.

Thank you for this, working great with 8.x-1.8

This plays well with address fields added to standard entity forms.

Can this feature somehow be used with Drupal commerce's Supported shipping countries and Supported billing countries widgets that are displayed to customers on the store's checkout page?

I would like to allow customers to choose from a wide range of countries, but show the most important countries at the top of the dropdown. I can't find any display settings for those two address fields.

Tested the patch from #37 and it works great for me as well.

This is just what I was looking for, great feature.

Latest patch worked a treat. Would be great for this to get into a stable release.

Re-rolled for current branch and release (8.x-1.11).

During the editing of the shipping method got the error

TypeError: array_combine(): Argument #1 ($keys) must be of type array, null given in array_combine() (line 90 of modules/contrib/address/src/Element/Country.php).

Had no time to investigate it. Here is a quick fix for the error.

Fabulous, thanks for moving this forward!

Unfortunately, I still can't commit this without tests (as tagged), which the latest patch does not contain.

Also, the summary has some remaining tasks

With patch in #46, If I use the field in a render array, then I get error:

Simple form code:

/**
   * {@inheritdoc}
   */
  public function buildForm(array $form, FormStateInterface $form_state) {


    $form['address'] = [
      '#type' => 'address',
      '#title' => $this->t('Address'),
    ];

    $form['actions'] = [
      '#type' => 'actions',
    ];
    $form['actions']['submit'] = [
      '#type' => 'submit',
      '#value' => $this->t('Send'),
    ];

    return $form;
  }

The error:

Warning: Undefined array key "#preferred_countries" in Drupal\address\Element\Address::processAddress() (line 192 of modules/contrib/address/src/Element/Address.php).

Drupal\address\Element\Address::processAddress(Array, Object, Array)

Added 2 test cases for this: one for views exposed filter setting and the other for widget configuration and make sure selected countries are displayed as only options directly in select and not optgroup.
Also fixed code standards, added missing default values for Address and Country element info and fixed schema for views filter.

Agreed, this can be 2.0.x material at this point, since it's a potentially more "disruptive" new feature.

That said, I'm really excited about this, and will try to make time to review + test soon. Probably within the first week or two of December...

Thanks!
-Derek

Reworked #49 to apply to 2.0.x

Hi,

I've tested the preferred_countries-2626982-52.patch patch with the current 2.0.x branch and found that the patch doesn't apply due to code expected from the 8.x-1.x branch.

After applying the rejected hunks manually and resolving duplications in AddressDefaultWidget.php (merge defaultSettings(), settingsForm() and settingsSummary() methods), I've successfully tested manually the module with preferred countries field.

The attached patch includes these modifications and works as expected. Please find my re-rolled patch attached for review.

Reworked #52 to apply to 2.0.x.

Hello,

I am posting an updated version of the patch preferred_countries-2626982-53.patch. This new patch addresses the issues encountered with the previous submission (preferred_countries-2626982-52.patch).

Please find the attached re-rolled patch for review and testing. I welcome any feedback or further testing.

Reworked #52 to apply to 2.0.x.

Thanks, this applies and runs well against the latest dev. Would be nice to get this committed after all these years.

It seems like the pull request needs a rebase.

Just posting an updated patch for the latest version of the module.

#60 works like a charm in 2.0.4 release. Thanks.

This would still be super helpful and is RTBC.