magento / architecture

A place where Magento architectural discussions happen
274 stars 154 forks source link

negotiable-quote-graphql-schema-improvement #422

Closed RakeshJesadiya closed 4 years ago

RakeshJesadiya commented 4 years ago

Problem

The NegotiableQuote Graphql architectural schema was not aligned with the implementation. Need to change the type to implementation fit.

Solution

During the Development time of the Negotiable Quote GraphQl, We have faced ENUMERATION TYPE Null Issue for the NegotiableQuoteHistoryStatusChange with old_status because old_status will be return null for the first iteration of history log of the current Quote and this will throw the error. We have added String type instead of ENUM.

Update schema to reflect the implementation of items object and NegotiableQuoteHistoryStatusChange.

Requested Reviewers

@paliarush @DrewML

dani97 commented 4 years ago

@paliarush @DrewML https://spec.graphql.org/June2018/#sec-Enum-Value from Graphql spec we cannot make it null, can we add a ENUM value that represents null

RakeshJesadiya commented 4 years ago

@DrewML I have updated the schema file again. Keep Enum Status type as it, I have resolved the issue for that and modify total object with Money Object is not mandatory. In the history log most of the time, old and new price value will not set, and its display 0, if we want to keep Money! The output looks like,


"total": {
              "old_price": {
                "value": 0,
                "currency": "USD"
              },
              "new_price": {
                "value": 0,
                "currency": "USD"
              }
            },

If an old and new price is not set its always display as
 "total": {
              "old_price": null,
              "new_price": null
}
DrewML commented 4 years ago

In the history log most of the time, old and new price value will not set, and its display 0, if we want to keep Money! The output looks like,

Agree with your changes there - let's go with your switch to nullable, and use null to present no value (since 0.00 has specific meaning)

RakeshJesadiya commented 4 years ago

@DrewML @paliarush I have updated the schema.graphql with the removal of the addNegotiableQuoteItems schema. Buyer Can't able to add items from the LUMA storefront. The only Seller Can Add a new item to the existing quote. schema is unused for the storefront GraphQl API.

RakeshJesadiya commented 4 years ago

@DrewML We have to change the mandatory type to the Close negotiable quote. closed_quotes: [NegotiableQuote]! to closed_quotes: [NegotiableQuote!]

If the user passes the quote_ids[] in the mutation that was already closed, We need to return as an empty array only. An empty array will be displayed in the graphql as null.

Example, Negotiable quote with id 10 is already closed, if the user passes mutation with quote_ids: [10] response will be null instead of NegotiableQuote object.

DrewML commented 4 years ago

@DrewML @paliarush I have updated the schema.graphql with the removal of the addNegotiableQuoteItems schema. Buyer Can't able to add items from the LUMA storefront. The only Seller Can Add a new item to the existing quote. schema is unused for the storefront GraphQl API.

Thanks @RakeshJesadiya - great catch! I confirmed this with @mbrinton01

DrewML commented 4 years ago

Example, Negotiable quote with id 10 is already closed, if the user passes mutation with quote_ids: [10] response will be null instead of NegotiableQuote object.

I wonder if this is the desirable behavior. I'm curious if it would make more sense to just return the quote with an ID of 10 here. What's the thinking behind returning null for that instead?

RakeshJesadiya commented 4 years ago

@DrewML Please check the modified schema file. I am not sure how we can filter out the Output in graphql schema. Is it acceptable criteria?

DrewML commented 4 years ago

Reverted the approval review state for now while we're still committing changes and discussing

RakeshJesadiya commented 4 years ago

@DrewML @melnikovi I have reverted back the changes for the Close and Delete Negotiable quote. I hope this is the last final changes for the Negotiable Quote. Waiting for your comment and approvals.