amzn / amazon-payments-magento-2-plugin

Extension to enable Amazon Pay on Magento 2
https://amzn.github.io/amazon-payments-magento-2-plugin/
Apache License 2.0
109 stars 77 forks source link

[GRAPHQL] query checkoutSessionDetails doesn't adhere to GraphQl best practices #1218

Closed dimitriBouteille closed 7 months ago

dimitriBouteille commented 1 year ago

Hi,

Why return an answer in json format? This is clearly not the best practice in graphQl. Best practice would be to return one type per possible value.

❌ Bad practice :(

type CheckoutSessionDetailsOutput {
    response: String!
}

Capture d’écran du 2023-10-16 11-31-58

✅ Good practice !

type CheckoutSessionDetailsOutput {
    billing: AmazonPayCheckoutSessionDetailAddress
    shipping: AmazonPayCheckoutSessionDetailAddress
    payment: String
}

type AmazonPayCheckoutSessionDetailAddress {
    city: String
    firstname: String
    lastname: String
    country_id: String
    street: [String]
    postcode: String
    company: String
    telephone: String
    region: String
    region_id: Int
    region_code: String
    email: String
}
sgabhart22 commented 1 year ago

Hi @dimitriBouteille ,

Frankly, I do not recall why we chose to format the response this way; I've looked back through our internal tickets and can't find any relevant discussion. Unfortunately at this point, the GraphQL API has been live for some time, so modifying the response type would break compatibility with headless/PWA merchants already using this API. We could potentially look into adding some additional queries to return more conventional GraphQL responses, and leave the current query marked as deprecated though.

Thanks, Spencer

dimitriBouteille commented 1 year ago

The solution might be to create a new query:

type Query {
    amazonPayCheckoutSessionDetailsV2(amazonSessionId: String!): AmazonCheckoutSessionDetailsV2Output
        @resolver(class:"Amazon\\Pay\\Model\\Resolver\\CheckoutSessionDetailsV2")
}

type AmazonCheckoutSessionDetailsV2Output {
    billing: AmazonPayCheckoutSessionDetailAddress
    shipping: AmazonPayCheckoutSessionDetailAddress
    payment: String
}

type AmazonPayCheckoutSessionDetailAddress {
    city: String
    firstname: String
    lastname: String
    country_id: String
    street: [String]
    postcode: String
    company: String
    telephone: String
    region: String
    region_id: Int
    region_code: String
    email: String
}
sgabhart22 commented 1 year ago

@dimitriBouteille That's pretty much what I had in mind! We'll discuss this internally and see if we can get something a little more friendly included in an upcoming release.

Thanks, Spencer

dimitriBouteille commented 1 year ago

Hi @sgabhart22

I create pull request: https://github.com/amzn/amazon-payments-magento-2-plugin/pull/1221

sgabhart22 commented 1 year ago

Hello again @dimitriBouteille ,

Thanks for all the effort on this, looks very nice on a first run through! One of our internal devs recently created a PR for the same issue, so we'll likely compare the two and take the best of both worlds. And we'll be sure to mention you in our release notes in case your commits don't get pulled in directly!

Thanks again, Spencer

dimitriBouteille commented 1 year ago

Hi @sgabhart22

Any news ? I am setting up Amazon Pay on my site and would like to use this feature with these changes (without having to make a preference)

sgabhart22 commented 1 year ago

Sorry for the delay @dimitriBouteille , we've been trying to get a major release out and one of the larger changes is taking a bit longer to cover and test edge cases. In the meantime, I'll attach a patch here of what the changes will be for this issue. You should be able to remove this patch after 5.17.0 is live.

gh-1218.patch.txt

Thanks, Spencer

akshitaWaldia commented 7 months ago

This has been added in as part of 5.17.0, closing this out. Thank you @dimitriBouteille !