Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Admin][Zones] Choosing disabled country as a zone member made possible for admin user #14058

Merged

Conversation

TheMilek
Copy link
Member

@TheMilek TheMilek commented Jun 6, 2022

Q A
Branch? 1.11
Bug fix? yes
New feature? no
BC breaks? no
Deprecations? no
License MIT

Right now we’re experiencing a bug-o-feature, where when you disable a country that is a zone member, the members list on the Zone shows empty dropdown and field is unable to be controlled.

Changes made:

  • adding disabled countries as zone members made possible for admin users
  • search dropdown added instead of select dropdown field

Before:
502e0587-5f2a-46e6-bf0a-978a51e0462c
After:
Zrzut ekranu 2022-06-7 o 15 24 15

@TheMilek TheMilek added UX Issues and PRs aimed at improving User eXperience. Admin AdminBundle related issues and PRs. labels Jun 6, 2022
@TheMilek TheMilek requested a review from a team as a code owner June 6, 2022 18:22
@TheMilek TheMilek force-pushed the selecting-disabled-country-allowed-for-admin branch 2 times, most recently from c5fa95b to 93eb48e Compare June 6, 2022 20:56
@TheMilek TheMilek force-pushed the selecting-disabled-country-allowed-for-admin branch 3 times, most recently from b2aede0 to 9d43959 Compare June 7, 2022 13:45
@TheMilek TheMilek changed the title [WIP][Admin][Zones] Choosing disabled country as a zone member made possible for admin user [Admin][Zones] Choosing disabled country as a zone member made possible for admin user Jun 7, 2022
@TheMilek TheMilek force-pushed the selecting-disabled-country-allowed-for-admin branch 2 times, most recently from 30b6d6b to 59f9069 Compare June 8, 2022 07:38
$resolver
->setDefaults([
'attr' => ['class' => 'country_search_dropdown ui fluid search selection dropdown'],
'enabled' => 'false',
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can add here almost the same comment as in the previous iteration: #14058 (comment)

CountryCodeChoiceType is used in several different places if I'm correct, but you want to change the behaviour only for selecting countries for zones, so this parameter should be disabled only in ZoneType, at least for now

Copy link
Member Author

@TheMilek TheMilek Jun 8, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can't resolve it in ZoneType as it is only parent of members and we need to add enabled key to his child code - sorry for not letting you know in the previous comment. Yeah, but you are right, CountryCodeChoiceType has impact on other forms as it is in CountryChoiceType - I must have missed it. I decided to make custom form type here that is only referencing code child field in zoneMembers form. What do you think about it? CountryCodeChoiceType and CountryChoiceType can be then used anywhere else and we still achieve wanted behavior

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm almost sure that if you will change this line: https://github.com/Sylius/Sylius/blob/master/src/Sylius/Bundle/AddressingBundle/Form/Type/ZoneType.php#L104 by adding there an enabled parameter, it will work correctly:

ZoneInterface::TYPE_COUNTRY => ['label' => 'sylius.form.zone.types.country', 'enabled' => false],

And you will not need to add a custom form type. Did you check this approach?

@TheMilek TheMilek force-pushed the selecting-disabled-country-allowed-for-admin branch 5 times, most recently from 5aaa116 to c6a8cb7 Compare June 8, 2022 11:04
@TheMilek TheMilek force-pushed the selecting-disabled-country-allowed-for-admin branch from c6a8cb7 to 2093c5c Compare June 8, 2022 11:06
@GSadee GSadee merged commit 7a1fb9c into Sylius:1.11 Jun 8, 2022
@GSadee
Copy link
Member

GSadee commented Jun 8, 2022

Thank you, Kamil! 🎉

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Admin AdminBundle related issues and PRs. UX Issues and PRs aimed at improving User eXperience.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants