Order Address type is added for shipping and billing address on orders #401
Conversation
avattam06
changed the title
| lastname: String @doc(description: "The family name of the person associated with the shipping/billing address") | ||
| middlename: String @doc(description: "The middle name of the person associated with the shipping/billing address") | ||
| region: String @doc(description: "The state or province name") | ||
| region_id: Int @doc(description: "The unique ID for a pre-defined region") |
There was a problem hiding this comment.
Seems like region_id probably isn't needed here (any region data should be available from the OrderAddress.region field
There was a problem hiding this comment.
Whoops - I thought OrderAddress.region was assigned an Object Type, but it's just a String.
Instead, let's switch region_id field to an ID type. The long-term plan is for unique identifiers to be backed by the ID type, and this will make OrderAddress.region_id ready for that change.
There was a problem hiding this comment.
Sounds good to me, I will change the 'OrderAddress.region_id' to type 'ID'
| ## OrderAddress type | ||
| ```graphql | ||
|
|
||
| type OrderAddress @doc(description: "OrderAddress contains detailed information about the order billing and shipping addresses"){ |
There was a problem hiding this comment.
Are any of the fields in this type required in Magento? If any of these fields assigned to a scalar type are required in Magento, we should make them non-nullable here.
There was a problem hiding this comment.
The fields which we can make non-nullable mostly depend on the country we choose on the address. You can see in the above screenshots for more clear context. Probably, we can leave the fields 'Postcode', 'State', 'Street address' as nullable so that we make other fields as non-nullable. I am not sure about the 'City' field though. What do you think @DrewML?
There was a problem hiding this comment.
Thanks for the screenshots - super helpful!
The fields which we can make non-nullable mostly depend on the country we choose on the address
For any cases where it seems ambiguous, we can play it safe and stick to nullable for now to avoid a breaking change later. Let's just do non-null for fields that are, in all cases, required.
|
Great work @avattam06! Few small comments, but looks great overall |
| region: String @doc(description: "The state or province name") | ||
| region_id: ID @doc(description: "The unique ID for a pre-defined region") | ||
| country_code: CountryCodeEnum @doc(description: "The customer's order country") | ||
| street: [String]! @doc(description: "An array of strings that define the street number and name") |
There was a problem hiding this comment.
Let's make street a non-nullable list as well:
street: [String!]!That just makes sure a null can't be hiding in the list of street items.
|
The team is planning to add some more changes to 'PaymentMethod' and 'CommentItem' namespaces as @paliarush suggesting. We will add those changes too in this PR and then we can merge this. |
|
@avattam06 Sounds good - I'll hold off on merging for now |
The team decided to cover it on a new PR, thanks for waiting @DrewML, you can merge it now. |
Problem
We have some fields like default_shipping, default_billing, and extension attributes on the 'CustomerAddress' type, which are not required on the orders schema. So it is better to create a new address type for orders with the necessary fields.
Solution
Proposing a new 'OrderAddress' type for shipping and billing address for the order on the customer orders mutation
Requested Reviewers
@nrkapoor @paliarush @joni-jones