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

Added missing place order address fields, added new StoreConfig fields #463

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

paales
Copy link

@paales paales commented Nov 6, 2020

Problem

  • We're missing lots of fields to place orders, this attends to add them back.
  • I've added new StoreConfig fields to provide the information which fields should be required in forms.
  • I've deprecated the CountryCodeEnum because already have the countries query.
  • I've deprecated country_id fields on input types, as they should be country_code.
  • I've deprecated country_code fields on return types, as GraphQL should be a graph and return the full object when possible. Returning a reference to another object is more REST thinking.
  • There seem to be two similar customer.graphqls documents, one should probably be cleaned up?

Solution

~ = deprecated
+ = added

  • + type CartAddressInput: prefix
  • + type CartAddressInput: middlename
  • + type CartAddressInput: suffix
  • + type CartAddressInput: gender
  • + type CartAddressInput: date_of_birth
  • + type CartAddressInput: fax
  • + type CartAddressInput: vat_id
  • + type CartAddressInterface: prefix
  • + type CartAddressInterface: middlename
  • + type CartAddressInterface: suffix
  • + type CartAddressInterface: gender
  • + type CartAddressInterface: region_v2
  • ~ type CartAddressInterface: region
  • + type CartAddressInterface: country_v2
  • ~ type CartAddressInterface: country
  • + type CartAddressInterface: date_of_birth
  • + type CartAddressInterface: fax
  • + type CartAddressInterface: vat_id
  • + type GenderEnum: OTHER
  • ~ type CountryCodeEnum: deprecated whole type
  • + type CustomerAddressInput: country_code_v2
  • ~ type CustomerAddressInput: country_code
  • + type CustomerAddress: country
  • ~ type CustomerAddress: country_id
  • ~ type CustomerAddress: country_code
  • + AddressFieldVisibilityEnum
  • + StoreConfig: address_prefix
  • + StoreConfig: address_prefix_options
  • + StoreConfig: address_middlename
  • + StoreConfig: address_suffix
  • + StoreConfig: address_suffix_options
  • + StoreConfig: address_date_of_birth
  • + StoreConfig: address_gender
  • + StoreConfig: address_telephone
  • + StoreConfig: address_fax
  • + StoreConfig: address_company
  • + StoreConfig: address_vat_id
  • + CompanyLegalAddress: country
  • ~ CompanyLegalAddress: country_code
  • + CompanyLegalAddressCreateInput: country_code
  • ~ CompanyLegalAddressCreateInput: country_id
  • + CompanyLegalAddressUpdateInput: country_code
  • ~ CompanyLegalAddressUpdateInput: country_id
  • + OrderAddress: country
  • ~ OrderAddress: country_code

Requested Reviewers

@DrewML tagged you because you are mentioned in the Component Assignments for the checkout frontend.
@paliarush tagged you because you are mentioned in the Component Assignments for the checkout

Closing notes:

What do you guys think? I'm open to discussions, but I think this cleans stuff up a lot. There are a few things I find difficult to oversee:

  • How does this functionality relate to the commerce edition? Especially for custom customer attributes?
  • How does this work with the separation between different modules. I'm now including the Country and Region types in the customer and cart sections?

`~` = deprecated
`+` = added

- + type CartAddressInput: prefix
- + type CartAddressInput: middlename
- + type CartAddressInput: suffix
- + type CartAddressInput: gender
- + type CartAddressInput: date_of_birth
- + type CartAddressInput: fax
- + type CartAddressInput: vat_id
- + type CartAddressInterface: prefix
- + type CartAddressInterface: middlename
- + type CartAddressInterface: suffix
- + type CartAddressInterface: gender
- + type CartAddressInterface: region_v2
- ~ type CartAddressInterface: region
- + type CartAddressInterface: country_v2
- ~ type CartAddressInterface: country
- + type CartAddressInterface: date_of_birth
- + type CartAddressInterface: fax
- + type CartAddressInterface: vat_id
- + type GenderEnum: OTHER
- ~ type CountryCodeEnum: deprecated whole type
- + type CustomerAddressInput: country_code_v2
- ~ type CustomerAddressInput: country_code
- + type CustomerAddress: country
- ~ type CustomerAddress: country_id
- ~ type CustomerAddress: country_code
- + AddressFieldVisibilityEnum
- + StoreConfig: address_prefix
- + StoreConfig: address_prefix_options
- + StoreConfig: address_middlename
- + StoreConfig: address_suffix
- + StoreConfig: address_suffix_options
- + StoreConfig: address_date_of_birth
- + StoreConfig: address_gender
- + StoreConfig: address_telephone
- + StoreConfig: address_fax
- + StoreConfig: address_company
- + StoreConfig: address_vat_id
- + CompanyLegalAddress: country
- ~ CompanyLegalAddress: country_code
- + CompanyLegalAddressCreateInput: country_code
- ~ CompanyLegalAddressCreateInput: country_id
- + CompanyLegalAddressUpdateInput: country_code
- ~ CompanyLegalAddressUpdateInput: country_id
- + OrderAddress: country
- ~ OrderAddress: country_code
@@ -27,6 +27,7 @@ type Mutation {
resetPassword(email: String!, resetPasswordToken: String!, newPassword: String!): Boolean @resolver(class: "\\Magento\\CustomerGraphQl\\Model\\Resolver\\ResetPassword") @doc(description: "Reset a customer's password using the reset password token that the customer received in an email after requesting it using requestPasswordResetEmail.")
}

# todo: We have a problem here that some fields should be required, but that is a breaking schema change?
input CustomerAddressInput {
Copy link
Author

Choose a reason for hiding this comment

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

I've currently left this Todo in place, because the fields shouldn't have been made optional from the start. All input types should be non nullable by default and be made nullable when they actually can be nullable.

@paales paales changed the title feat: added missing fields and refactored part of the schema Added missing place order address fields, added new StoreConfig fields Nov 6, 2020
@cpartica cpartica self-assigned this Nov 12, 2020
@fooman
Copy link

fooman commented Feb 18, 2021

If we are making changes to the customer inputs I strongly suggest dropping a lot of the required fields and making them optional. The current GraphQL API is very opinionated and introduces requirements which Magento core may not need (OR should not need).

A few fields which should not be required:

  • region (any and all queries/mutations related to it - there are some countries which do not have regions or are not widely used in addresses)
  • postcode (see here for a list of countries without)
  • phone numbers
  • I would even go as far as saying first name / last name should not be required see here for reasoning that a full name field can be a better choice (even if you don't agree the API should not make adopting something like this harder)

Please also get the input from the PWA Studio people to make sure that changes to region and countries mean they are still easily cacheable by apollo (in other words we don't end up needing v3).

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.

5 participants