magento / architecture

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

GiftWrapping changes on proposed schema #416

Closed avattam06 closed 4 years ago

avattam06 commented 4 years ago

Problem

Some changes on the Gift wrapping schema are added which are based on https://github.com/magento/architecture/blob/master/design-documents/graph-ql/coverage/gift-wrapping.md. Submitting the changes for review as part of this PR https://github.com/magento/partners-magento2ee/pull/246.

Solution

Submitting the new changes for review.

Requested Reviewers

@paliarush @prabhuram93

avattam06 commented 4 years ago

@paliarush, On line 201 for setGiftOptionsOnCart mutation name, Can we change it to setGiftOptionsOnAllCart or setGiftOptionsOnWholeCart as setgiftOptionsonCart will add the provided giftwrapping ID on the whole cart but not on the item level.

Because we are using updateCartItems to add gift wrapping on to the cart items.

Is it better to cover adding giftWrapping on item level and on order level on the same mutation setGiftOptionsOnAllCart

Please suggest your thoughts on this?

avattam06 commented 4 years ago

@paliarush, On line 201 for setGiftOptionsOnCart mutation name, Can we change it to setGiftOptionsOnAllCart or setGiftOptionsOnWholeCart as setgiftOptionsonCart will add the provided giftwrapping ID on the whole cart but not on the item level.

Because we are using updateCartItems to add gift wrapping on to the cart items.

Is it better to cover adding giftWrapping on item level and on order level on the same mutation setGiftOptionsOnAllCart

Please suggest your thoughts on this?

Giftwrapping Schema @paliarush

avattam06 commented 4 years ago

@nrkapoor, Added new schema changes for the gift-wrapping on whole cart and item level. Please check.

paliarush commented 4 years ago

@avattam06 regarding https://github.com/magento/architecture/pull/416#issuecomment-670090794 I don't see a strong reason to change the name of setGiftOptionsOnCart. The two proposed names look strange to me. We can clarify the effect of this mutation via docs.

paliarush commented 4 years ago

@avattam06 the merge is blocked because of the conflicts.