Skip to content

SELL-1150: Update exchange.graphql to reflect artsy/exchange#271#1375

Merged
ashkan18 merged 2 commits into
artsy:masterfrom
dleve123:update-exchange-schema-after-seller-accept-mutation
Nov 14, 2018
Merged

SELL-1150: Update exchange.graphql to reflect artsy/exchange#271#1375
ashkan18 merged 2 commits into
artsy:masterfrom
dleve123:update-exchange-schema-after-seller-accept-mutation

Conversation

@dleve123
Copy link
Copy Markdown
Contributor

@dleve123 dleve123 commented Nov 9, 2018 •
edited
Loading

Jira: https://artsyproduct.atlassian.net/browse/SELL-1150

Adds sellerAcceptOfferMutation to MP, so that it can be consumed by force.

WARNING: If this deployed to production before, Exchange https://github.com/artsy/exchange/commit/b03a4a6f51cb88ca5d9637f5012586b8a0306af0 is deployed to production, then MP might break as MP-prod will have a newer version of Exchange's schema than Exchange-prod.

N.B: local MP is on my dev branch, integrating with Gravity staging.

screen shot 2018-11-09 at 6 21 47 pm

@peril-staging
Copy link
Copy Markdown
Contributor

peril-staging Bot commented Nov 9, 2018

Danger run resulted in 1 warning; to find out more, see the checks page.

Generated by 🚫 dangerJS

@dleve123 dleve123 changed the title Update exchange.graphql to reflect artsy/exchange#271 [WIP] Update exchange.graphql to reflect artsy/exchange#271 Nov 9, 2018
@dleve123 dleve123 force-pushed the update-exchange-schema-after-seller-accept-mutation branch 2 times, most recently from 12e17e6 to ab36649 Compare November 9, 2018 22:18
@dleve123 dleve123 changed the title [WIP] Update exchange.graphql to reflect artsy/exchange#271 Update exchange.graphql to reflect artsy/exchange#271 Nov 9, 2018
@dleve123 dleve123 changed the title Update exchange.graphql to reflect artsy/exchange#271 SELL-1150: Update exchange.graphql to reflect artsy/exchange#271 Nov 9, 2018
@dleve123 dleve123 force-pushed the update-exchange-schema-after-seller-accept-mutation branch 2 times, most recently from 3122931 to 9cde0b6 Compare November 13, 2018 17:15
@dleve123 dleve123 assigned ashkan18 and unassigned sepans Nov 13, 2018
@dleve123 dleve123 requested a review from orta November 13, 2018 17:52
@dleve123
Copy link
Copy Markdown
Contributor Author

dleve123 commented Nov 13, 2018

@ashkan18 This is ready for a review btw!

starsirius
starsirius reviewed Nov 14, 2018
View reviewed changes
Comment thread src/schema/ecommerce/seller_accept_offer_mutation.ts Outdated
${SellerOrderFields}
lineItems{
edges{
node{
Copy link
Copy Markdown
Member

@starsirius starsirius Nov 14, 2018

Choose a reason for hiding this comment

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

#minor. Missing whitespace before { above. We could also move the lineItems to SellerOrderField, like BuyerOrderField.

starsirius
starsirius approved these changes Nov 14, 2018
View reviewed changes
Copy link
Copy Markdown
Member

@starsirius starsirius left a comment

Choose a reason for hiding this comment

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

Looks good to me! Also, in the PR description, I imagine you meant consumed by "Volt".

... consumed by force.

ashkan18
ashkan18 approved these changes Nov 14, 2018
View reviewed changes
Copy link
Copy Markdown
Contributor

@ashkan18 ashkan18 left a comment

Choose a reason for hiding this comment

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

other than @starsirius's #minor comment looks 💯 to me! 🙌

@sepans
Copy link
Copy Markdown
Contributor

sepans commented Nov 14, 2018

We were thinking if it makes more sense for the client (volt here) to run the mutation by order_id or offer_id? InitialOfferMutation accepts order_id so do we need to make them consistent or keep it as is since they are being called from the different clients with different contexts?

@ashkan18
Copy link
Copy Markdown
Contributor

ashkan18 commented Nov 14, 2018

I think for sellerAcceptOfferMutation makes sense to get offerId so we know which offer is actually being accepted, thinking of the case of old open tab in the browser or something were someone tries to accept an already out of date offer.
initialOffer gets orderId since its adding the first offer to the order. we are actually renaming that one to addOfferToPendingOrder as part of https://github.com/artsy/exchange/pull/283 or another PR

@dleve123 @sepans
Add SellerAcceptOffer mutation
808fe5b 
[Finishes https://artsyproduct.atlassian.net/browse/SELL-1150]

* Adds new OfferMutationInputType
* Copy exchange's schema into MP

Co-authored-by: sepans <sepans@sepans.com>
@dleve123 dleve123 force-pushed the update-exchange-schema-after-seller-accept-mutation branch from 9cde0b6 to 808fe5b Compare November 14, 2018 15:43
@ashkan18
Copy link
Copy Markdown
Contributor

ashkan18 commented Nov 14, 2018

merge on green

@dleve123
Copy link
Copy Markdown
Contributor Author

dleve123 commented Nov 14, 2018

@ashkan18 I addressed @starsirius's comment moving line_items to SellerOrderFields in 9b8589b.

Please check it out, and rebase and/or merge in.

@ashkan18 ashkan18 merged commit 7b336f2 into artsy:master Nov 14, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@starsirius starsirius starsirius approved these changes

@sepans sepans Awaiting requested review from sepans

@orta orta Awaiting requested review from orta

+1 more reviewer

@ashkan18 ashkan18 ashkan18 approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@ashkan18 ashkan18

Labels

Jira Synced

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@dleve123 @sepans @ashkan18 @starsirius