When creating an inquiry-offer-order, associate the offer and order in the mutation #2974

erikdstock · 2021-02-04T00:26:36Z

This pr creates a createInquiryOfferOrder mutation to make additional calls to both the exchange createInquiryOffer... with impulse before and after the exchange call, failing if any request along the way fails. We ultimately return the exchange type; this could mean returning a data...orderOrError.error from exchange or no data but an errors key in the graphql response.

It relies on order.internalID being selected in the query - we currently have some commented-out code where we tried and failed to use a WrapQuery transform to achieve this automatically but it didn't work so far. So for now just make sure you request the order.internalID. We got this working with WrapQuery 🎉

Consumers will have to handle a rejected request, a success with errors, and a success with data.commerceCreateInquiryOfferOrderWithArtwork.orderOrError.error.

Co-authored-by: sepans sepans@sepans.com and also part of a mob session with @artsy/purchase-devs.

erikdstock · 2021-02-04T15:10:12Z

No idea why the schema has been ruined by my pr and not the first time weirdness has happened when it dumped in the commit hook. I'll dig in and fix.

src/lib/stitching/exchange/stitching.ts

sepans · 2021-02-04T19:52:10Z

src/lib/stitching/exchange/stitching.ts

+      Mutation: {
+        // monkey-patch the existing exchange mutation
+        // to add operations dependent on impulse
+        commerceCreateInquiryOfferOrderWithArtwork: {


question @starsirius: here we are basically re-using the exchange name. Is this name good or you have other suggestions?

I see this follows the pattern so I'm 👍 !

starsirius · 2021-02-04T19:55:36Z

src/lib/stitching/exchange/stitching.ts

+            } = args
+
+            try {
+              await conversationLoader(impulseConversationId)


This is interesting. We are talking to both Exchange and Impulse, and before we create an Exchange order with a given Impulse conversation id, we want to verify if it's legit (e.g. conversation exists, user is authorized to access it, etc.).

I'm thinking verifying this kind of external id can be optional and we can leave it to the actual service (i.e. Impulse in this case) to verify eventually. For example, if an invalid Impulse conversation id is provided, the create conversation order loader below should fail. We can still be extra cautious here and monitor the performance.

@starsirius we just wanted to make sure we verify the validity of the conversation ID and authenticate the user before creating the exchange order (deligateToSchema function below) since we cannot roll back the exchange mutation.

Our case here would be to validate that the 'conversation id' is something more than just an arbitrary string. Either exchange could do it (call impulse) or metaphysics could do it, which gives us the additional user access check before any data is changed.

Yeah I'm fine being extra cautious here! 👍 I was mainly thinking it's hard to control external records (e.g. impulse conversations can be deleted in the future) so a broken link may not be that bad as long as the external service does proper check like authorization. And in our case, we cannot roll back the exchange order just created but it would eventually expire and buyers have to start a new one (after we do this).

mzikherman · 2021-02-04T21:17:18Z

You may need to pull latest. I'm noticing you're editing the stitching.ts file for Exchange, where those have been moved into the v1/v2 folders. Let me know if you need help rebasing. (That will hopefully clean up this schema diff, something's in a weird state).

…ulse Co-authored-by: sepans <sepans@sepans.com>

mzikherman · 2021-02-04T22:16:53Z

src/lib/stitching/exchange/v2/stitching.ts

+      Mutation: {
+        // monkey-patch the existing exchange mutation
+        // to add operations dependent on impulse
+        commerceCreateInquiryOfferOrderWithArtwork: {


wondering if this needs to be stitched in at all? this looks like a new mutation, which requires data from two services. Should it perhaps be a regular mutation in Metaphysics? it can use fetch or something to hit the Exchange GraphQL endpoint (although there's a more graphql-tools-y way of hitting a GraphQL endpoint in an ad-hoc way).

We used fetch to meet a similar challenge in causality's stitching file (maybe lot.ts actually) but the issue there is passing the queried fields on to fetch assuming we want to return the same exchange type. In causality we solved this by doing querying every field but it might be worth looking into.

mzikherman · 2021-02-04T22:18:53Z

src/lib/stitching/exchange/v2/stitching.ts

@@ -349,6 +350,93 @@ export const exchangeStitchingEnvironment = ({
          },
        },
      },
+      Mutation: {
+        // monkey-patch the existing exchange mutation


instead of monkey-patching, this mutation can be excluded from being included with the Exchange schema (via this transform, and then this mutation can be defined more conventionally inside MP.

We started out with that approach, then decided it might be more clear to follow the exchange convention. So something like Mutation.createInquiryOffer but defined in this stitching file?

I'm not following the stitching use-case exactly. Per https://github.com/artsy/metaphysics/pull/2974/files#r570581737 basically, I would probably stick with a regular Metaphysics mutation at this point.

erikdstock

Ready for final review!

I changed the naming to createInquiryOffer and got WrapQuery working to add an Order.internalID to the underlying delegateToSchema call. I did not move the mutation into the local schema (cc @mzikherman) because the tradeoffs for this were harsh when doing it for causality - because the local schema doesn’t have access to not-yet-merged schemas i am pretty sure wouldn’t be able to refer to the CommerceCreateOrderOverMutationResponse or other types so would have to come up with a completely new one. In the case of the Lot type we did keep it there in part because there was also a SaleArtwork on the lot type (see lot.ts). Happy to discuss this further if it helps or folks have other ideas.

I also noticed that we don’t have access to the (presumably newer) graphql-tools FilterInterfaceFields (eg Mutation.commerceCreateInquiryOrderOfferEtc) so i don’t think I can rename/remove the underlying query. If that is an issue we can discuss an alternative.

erikdstock · 2021-02-05T19:05:18Z

src/lib/stitching/exchange/v2/stitching.ts

+    extend type Mutation {
+      createInquiryOffer(
+        input: CommerceCreateInquiryOfferOrderWithArtworkInput!
+      ): CommerceCreateInquiryOfferOrderWithArtworkPayload


Note here that we are still referencing the underlying exchange mutation types, but with a new name for the Mutation rather than monkey-patching.

Ah, so that's why you were doing this in the stitching file (vs. a regular old MP mutation).

erikdstock · 2021-02-05T19:05:46Z

src/lib/stitching/exchange/v2/stitching.ts

+              info,
+              transforms: [
+                // add orderOrError.order.internalID to the Order selectionSet
+                new WrapQuery(


This now works 🧑‍🤝‍🧑

mzikherman · 2021-02-05T19:16:50Z

src/lib/loaders/loaders_with_authentication/impulse.ts

@@ -50,5 +50,10 @@ export default (accessToken, userID, opts) => {
      },
      { method: "POST" }
    ),
+    conversationCreateConversationOrderLoader: impulseLoader(


i feel like createConversationOrderLoader makes more sense, but I see all these impulse ones have the prefix conversation-.

happy to break this convention if you say it's ok!

mzikherman · 2021-02-05T19:18:27Z

src/lib/stitching/exchange/v2/stitching.ts

+            } = args
+
+            try {
+              await conversationLoader(impulseConversationId)


Am I missing something, or is there a return value of this missing? Or is it just doing an exist-y check?

just an exist-y check - though we could make more explicit what is going on.

mzikherman · 2021-02-05T19:19:43Z

Looks great!

starsirius · 2021-02-05T19:20:08Z

src/lib/stitching/exchange/v2/stitching.ts

@@ -349,6 +356,87 @@ export const exchangeStitchingEnvironment = ({
          },
        },
      },
+      Mutation: {
+        createInquiryOffer: {


Since "offer" can mean 2 different things in Exchange: a "mode" of an order model and a separate model associated with an order. Let's maybe change this to createInquiryOfferOrder to be consistent.

starsirius · 2021-02-05T19:21:53Z

src/lib/stitching/exchange/__tests__/stitching.test.ts

    ...restOfResolveArgs,
  })
 })
+
+describe("commerceCreateInquiryOfferOrderWithArtwork", () => {


This name would need to change accordingly.

starsirius

Have 1 naming comment otherwise looks good to me! It's also amazing that WrapQuery was figured out! 💯

erikdstock · 2021-02-05T20:10:51Z

#squashongreen

erikdstock self-assigned this Feb 4, 2021

artsy-peril bot added the Jira Synced label Feb 4, 2021

erikdstock commented Feb 4, 2021

View reviewed changes

src/lib/stitching/exchange/stitching.ts Outdated Show resolved Hide resolved

sepans reviewed Feb 4, 2021

View reviewed changes

starsirius reviewed Feb 4, 2021

View reviewed changes

erikdstock requested a review from damassi February 4, 2021 20:03

erikdstock and others added 2 commits February 4, 2021 16:20

feat: create and associate inquiry x offer-order using exchange x imp…

fccb599

…ulse Co-authored-by: sepans <sepans@sepans.com>

rebase, update test and cleanup

feaf75b

erikdstock force-pushed the erikdstock/purchase-2381 branch from 6b9e7aa to feaf75b Compare February 4, 2021 22:11

mzikherman reviewed Feb 4, 2021

View reviewed changes

erikdstock added 2 commits February 5, 2021 12:01

use WrapQuery working?

86aedc6

rename mutation

6219222

erikdstock commented Feb 5, 2021

View reviewed changes

mzikherman reviewed Feb 5, 2021

View reviewed changes

starsirius reviewed Feb 5, 2021

View reviewed changes

starsirius approved these changes Feb 5, 2021

View reviewed changes

Mutation.createInquiryOffer > createInquiryOfferOrder

8d4449e

artsy-peril bot added the Squash On Green A label to indicate that Peril should squash-merge this PR when all statuses are green label Feb 5, 2021

artsy-peril bot merged commit f5b662d into master Feb 5, 2021

artsy-peril bot mentioned this pull request Feb 5, 2021

Deploy #2976

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

When creating an inquiry-offer-order, associate the offer and order in the mutation #2974

When creating an inquiry-offer-order, associate the offer and order in the mutation #2974

erikdstock commented Feb 4, 2021 •

edited

erikdstock commented Feb 4, 2021

sepans Feb 4, 2021

starsirius Feb 4, 2021

starsirius Feb 4, 2021

sepans Feb 4, 2021 •

edited

erikdstock Feb 4, 2021

starsirius Feb 4, 2021

mzikherman commented Feb 4, 2021 •

edited

mzikherman Feb 4, 2021

erikdstock Feb 4, 2021

mzikherman Feb 4, 2021

erikdstock Feb 4, 2021

mzikherman Feb 4, 2021

erikdstock left a comment •

edited

erikdstock Feb 5, 2021

mzikherman Feb 5, 2021

erikdstock Feb 5, 2021

mzikherman Feb 5, 2021

mzikherman Feb 5, 2021

erikdstock Feb 5, 2021

mzikherman Feb 5, 2021

erikdstock Feb 5, 2021

mzikherman commented Feb 5, 2021

starsirius Feb 5, 2021

starsirius Feb 5, 2021

starsirius left a comment

erikdstock commented Feb 5, 2021

When creating an inquiry-offer-order, associate the offer and order in the mutation #2974

When creating an inquiry-offer-order, associate the offer and order in the mutation #2974

Conversation

erikdstock commented Feb 4, 2021 • edited

erikdstock commented Feb 4, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sepans Feb 4, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mzikherman commented Feb 4, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

erikdstock left a comment • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mzikherman commented Feb 5, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

starsirius left a comment

Choose a reason for hiding this comment

erikdstock commented Feb 5, 2021

erikdstock commented Feb 4, 2021 •

edited

sepans Feb 4, 2021 •

edited

mzikherman commented Feb 4, 2021 •

edited

erikdstock left a comment •

edited