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

Improvements to directory.graphqls to include more configation settings #462

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

paales
Copy link

@paales paales commented Nov 6, 2020

Problem

To build proper customer address forms, we need to have some additional information.

Solution

I've split this PR in two commits, the first is a copy of the current schema in the magento repository, the second are my modifications on it: 5c8ab13

Introduce additional country fields:

  • add field to Country type: region_required
  • add field to Country type: postcode_required
  • add field to Country type: name
  • add field to Country type: eu
  • deprecate full_name_locale in favor of name
  • deprecate full_name_english in favor of name
  • add field to StoreConfig type: default_country
  • add field to StoreConfig type: allow_optional_state
  • adds field docs with sorting information: This should be a nice quality of life improvement for the frontend implementor so they don't have to do this on the frontend.

Requested Reviewers

@buskamuza Taggin you because you seem to be responsible for this component
@cpartica Tagging you of the discussion on Slack

- add field to Country type: region_required
- add field to Country type: postcode_required
- add field to Country type: name
- add field to Country type: eu
- deprecate full_name_locale in favor of name
- deprecate full_name_english in favor of name
- add field to StoreConfig: default_country
- add field to StoreConfig: allow_optional_state
- adds field docs with sorting information
@cpartica cpartica self-assigned this Nov 12, 2020
}

type Currency {
base_currency_code: String
Copy link
Contributor

Choose a reason for hiding this comment

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

there is a different issue with Currency
https://github.com/magento/architecture/pull/474/files
see what I proposed here
https://github.com/magento/architecture/pull/474/files#r556999960
I am looking at other APIs and Currency object is simple usually name, code and symbol

Copy link
Author

Choose a reason for hiding this comment

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

Agreed that should be fixed, if there is a conclusion in the other PR I can update this one.

two_letter_abbreviation: String @doc(description: "Country code according to ISO 3166-1 alpha-2")
three_letter_abbreviation: String @doc(description: "Country code according to ISO 3166-1 alpha-3")
name: String @doc(description: "Name of the country in the locale of the current store")
full_name_locale: String @deprecated(reason: "Use `name`")
Copy link
Contributor

Choose a reason for hiding this comment

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

everywhere else we have fullname used in customers. Of course that's not quite right but would make this an outlier

full_name_english: String @deprecated(reason: "Use `name`")
region_required: Boolean @doc(description: "Is a the region for this country")
postcode_required: Boolean @doc(description: "Is a postcode required for this country")
eu: Boolean @doc(description: "Country is a member of the European Union")
Copy link
Contributor

Choose a reason for hiding this comment

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

instead of having a specific region for EU, wouldn't it be better to have a "Zone" with value as EU or APAC EMEA, etc
There are other zones besides EU

Copy link
Author

Choose a reason for hiding this comment

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

In the backend of Magento there is a specific configuration field that allows you to check EU countries. But I agree that zone would be better, but that would require modifications on the other layers as well.


type StoreConfig {
default_country: String @doc(description: "Default country used for country fields")
allow_optional_state: String @doc(description: "Allow a customer to enter a state when it isn't required for a country")
Copy link
Contributor

Choose a reason for hiding this comment

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

what is the path in system config for this?

Copy link
Author

Choose a reason for hiding this comment

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

Uhh, don't know the exact path, but it's this field:

Schermafbeelding 2021-01-14 om 12 43 37

Comment on lines +30 to +32
name: String @doc(description: "Name of the country in the locale of the current store")
full_name_locale: String @deprecated(reason: "Use `name`")
full_name_english: String @deprecated(reason: "Use `name`")
Copy link
Author

Choose a reason for hiding this comment

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

@cpartica Something like this?

Suggested change
name: String @doc(description: "Name of the country in the locale of the current store")
full_name_locale: String @deprecated(reason: "Use `name`")
full_name_english: String @deprecated(reason: "Use `name`")
fullname: String @doc(description: "Name of the country in the locale of the current store")
full_name_locale: String @deprecated(reason: "Use `fullname`")
full_name_english: String @deprecated(reason: "Use `fullname`")

full_name_english: String @deprecated(reason: "Use `name`")
region_required: Boolean @doc(description: "Is a the region for this country")
postcode_required: Boolean @doc(description: "Is a postcode required for this country")
eu: Boolean @doc(description: "Country is a member of the European Union")
Copy link
Author

Choose a reason for hiding this comment

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

In the backend of Magento there is a specific configuration field that allows you to check EU countries. But I agree that zone would be better, but that would require modifications on the other layers as well.


type StoreConfig {
default_country: String @doc(description: "Default country used for country fields")
allow_optional_state: String @doc(description: "Allow a customer to enter a state when it isn't required for a country")
Copy link
Author

Choose a reason for hiding this comment

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

Uhh, don't know the exact path, but it's this field:

Schermafbeelding 2021-01-14 om 12 43 37

}

type Currency {
base_currency_code: String
Copy link
Author

Choose a reason for hiding this comment

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

Agreed that should be fixed, if there is a conclusion in the other PR I can update this one.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants