Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: PX-5157 - Add Bulk Edit endpoint #4269

Merged
merged 9 commits into from
Aug 3, 2022
3 changes: 2 additions & 1 deletion README.md
Original file line number Diff line number Diff line change
Expand Up @@ -90,7 +90,7 @@ _If you're new to GraphQL, you can checkout [Artsy's GraphQL Workshop](https://g

For `GraphQL Endpoint`, set it to `http://localhost:3000/` if you want to query for something that lives in v1 of the schema, otherwise, set it to `http://localhost:3000/v2`.

**Note that `/v2` is the default** and it is rare to query against `/v1`.
**Note that `/v2` is the default** and it is rare to query against `/v1`.

### Introspection Setup

Expand Down Expand Up @@ -138,6 +138,7 @@ If any of these queries fail, it's probable that you misconfigured your

- [Intro to GraphQL](https://github.com/artsy/graphql-workshop)
- [How we use DataLoaders](docs/dataloaders.md)
- [(Beginner friendly) Adding a gravity endpoint into Metaphysics](docs/adding_a_new_gravity_endpoint_into_metaphysics.md)
- [Adding a GraphQL micro-service to Metaphysics](docs/adding_a_new_graphql_microservice.md)
- [Adding a rest micro-service to Metaphysics](docs/adding_a_new_rest_microservice.md)
- [Debugging with VS Code](docs/debugging_with_vscode.md)
Expand Down
53 changes: 53 additions & 0 deletions _schemaV2.graphql
Original file line number Diff line number Diff line change
Expand Up @@ -10898,6 +10898,11 @@ type Mutation {
updateNotificationPreferences(
input: updateNotificationPreferencesMutationInput!
): updateNotificationPreferencesMutationPayload

# Update all artworks that belong to the partner
updatePartnerArtworks(
input: UpdatePartnerArtworksMutationInput!
): UpdatePartnerArtworksMutationPayload
updateSavedSearch(input: UpdateSavedSearchInput!): UpdateSavedSearchPayload
updateSmsSecondFactor(
input: UpdateSmsSecondFactorInput!
Expand Down Expand Up @@ -11705,6 +11710,22 @@ type PartnerArtworkGrid implements ArtworkContextGrid {
title: String
}

type PartnerArtworks {
Mitch-Henson marked this conversation as resolved.
Show resolved Hide resolved
errors: PartnerArtworksError

# A globally unique ID.
id: ID!

# A type-specific ID likely used as a database ID.
internalID: ID!
success: Int
}

type PartnerArtworksError {
Mitch-Henson marked this conversation as resolved.
Show resolved Hide resolved
count: Int
ids: [String]
}

type PartnerCategory {
cached: Int
categoryType: PartnerCategoryType
Expand Down Expand Up @@ -15450,6 +15471,38 @@ type updateNotificationPreferencesMutationPayload {
): [NotificationPreference!]!
}

type UpdatePartnerArtworksMutationFailure {
mutationError: GravityMutationError
}

input UpdatePartnerArtworksMutationInput {
# Whether Artsy domestic shipping should be enabled
artsyShippingDomestic: Boolean

# Whether Artsy international shipping should be enabled
artsyShippingInternational: Boolean
clientMutationId: String
mzikherman marked this conversation as resolved.
Show resolved Hide resolved

# ID of the partner
id: String!

# The partner location ID to assign
location: String
}

type UpdatePartnerArtworksMutationPayload {
clientMutationId: String
partnerArtworksOrError: UpdatePartnerArtworksMutationType
}

type UpdatePartnerArtworksMutationSuccess {
partnerArtworks: PartnerArtworks
}

union UpdatePartnerArtworksMutationType =
UpdatePartnerArtworksMutationFailure
| UpdatePartnerArtworksMutationSuccess

# Autogenerated input type of UpdateSavedSearch
input UpdateSavedSearchInput {
attributes: SearchCriteriaAttributes
Expand Down
78 changes: 78 additions & 0 deletions docs/adding_a_new_gravity_endpoint_into_metaphysics.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,78 @@
## Adding a gravity endpoint into Metaphysics

This doc is aimed at helping backend devs or beginner front end devs on how to add an existing gravity endpoint into Metaphysics.

This article assumes that you already have your local machine setup and have your graphiql.app or other app (Postman, Insomnia etc) setup. If you need help with that, you can start [here](https://github.com/artsy/metaphysics#setting-up-your-local-graphiql)

### Steps to add an existing endpoint

1. Find an existing PR or commit which adds something similar to what you want. If you are looking for a PUT endpoint, then [this](https://github.com/artsy/metaphysics/commit/987b21f501bb3483d196720d4a252c25b704352e) is a great example.
Mitch-Henson marked this conversation as resolved.
Show resolved Hide resolved
1. Follow the structure of the PR to add your own endpoint and input/outputs. Some further tips can be found below.
1. After the code has been written, you can run `yarn dump:local` in the Metaphysics directory to generate the new schema files. This script (and other useful scripts) can be found in the `package.json` file.
Mitch-Henson marked this conversation as resolved.
Show resolved Hide resolved
1. Start your local server with `yarn start`
1. With your favourite graphql app, you should be able to hit the local server to refresh the schema. In insomnia, you can click on the "schema" button in the GraphQL panel to manually refresh this, if you don't have auto-refresh turned on (also found in this list)
1. You should be able to hit your local metaphysics, which will hit staging gravity by default.
Mitch-Henson marked this conversation as resolved.
Show resolved Hide resolved
1. Work through the errors, making sure to read the message in its entirety. Also don't hesitate to add it to the errors section below if useful.
Copy link
Member

Choose a reason for hiding this comment

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

This point is also a bit confusing because the above describes setup, and now suddenly there are errors. Maybe another point can be added in between that says. "Now, develop your feature!" (or something).

Also it's not so clear where the dev should be seeing errors. Are they server errors, typescript errors, graphql schema errors? If this ambiguity forces too much explaination here, best to just omit this point completely.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I based this more or less off of my own experience, so it can definitely be improved for someone else.

With this section I was trying to prepare the developer to see errors (you are right in that this should be specified for what errors and where) but also to encourage further additions to the documentation if someone comes across an error that could be documented to help someone else get setup quicker.

I don't mind if we remove this point from these steps, however I believe there definitely needs to be a section on debugging and error expectations


### Tips on writing and debugging locally

- GraphQL is strongly typed and needs to know the exact inputs and outputs of the endpoint as it mediates between the clients and the REST endpoint. This is why you need to set up a union type which instructs GraphQL on what both responses will look like. This allows GraphQL tools to know if the request you are making (in Insomnia, for example) is valid, even before you send the request.
Copy link
Member

@damassi damassi Aug 1, 2022

Choose a reason for hiding this comment

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

Going from setup to writing mutation-handling Union types is a very steep curve!

As we want this document to be instructive (rather than intimidating) its best to keep this all very incremental versus skipping ahead. Meaning:

  • Add an example that shows how to add a new loader pointing at a Gravity GET endpoint
  • Show how to add a new field to a section within the schema (say, artwork.ts), and resolve it
  • Show how to add a new POST endpoint in prep for a mutation
  • Show had to add a new mutation (in the simplest way possible; no fancy error handling is necessary yet; see all of the mutations in the me folder for an example)
  • THEN show how to handle errors in a way that is more sophisticated.

But even so, this thingOrError pattern isn't necessarily something we should advocate for up front in a getting started doc. In many ways it's overly complicated for everyday mutations and should be used more on a need to know basis.

(Worth noting that we have a full blog post on this here: https://artsy.github.io/blog/2018/10/19/where-art-thou-my-error/)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for the article link, I will have to check it out and also link it in this doc.

I guess I'm simultaneously both the best and worst person to make this doc. The best person because I know almost nothing about GraphQL and can give feedback on whether something is too complicated or not, and also the worst person because I know nothing about GraphQL and so I shouldn't be teaching others about things I don't really understand.

The thingOrError I thought was standard and came from the example PR I was basing my changes on. Happy to learn the easier way on how to do this!

- You can always add some more console logs to any resolver to help see what is coming back from the REST endpoint, e.g.

```typescript
outputFields: {
partnerArtworksOrError: {
type: UpdatePartnerArtworksMutationType,
resolve: (result) => result,
},
},
```

can be changed to

```typescript
outputFields: {
partnerArtworksOrError: {
type: UpdatePartnerArtworksMutationType,
resolve: (result) => {
console.log(result)
Copy link
Member

Choose a reason for hiding this comment

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

For devs who use VSCode, one can boot MP in the VSCode console and have automatic debugger support by adding a breakpoint. Maybe we should lean into vs console.log-style debugging.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

What are you thoughts about leaving the console log in because it is very basic, but also including a link to the debugger approach?

Copy link
Member

Choose a reason for hiding this comment

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

That sounds good 👍 (And I didn't know about this doc!)

return result
},
},
},
```

- You don't need to restart the server once you make changes. The updates get triggered automatically once you save your changes locally and hit your local metaphysics server again. You should see some lines similar to these below, which shows that it has been updated
Mitch-Henson marked this conversation as resolved.
Show resolved Hide resolved

```
[@artsy/express-reloadable] File /Users/mitchellhenson/dev/metaphysics/src/schema/v2/partnerArtworksMutation.ts has changed.
[V1] [FEATURE] Enabling Schema Stitching
[V2] [FEATURE] Enabling Schema Stitching
```

- You should be able to see the request that you are making to the gravity REST endpoint in the GraphQL output. This can potentially help pinpoint errors, if it is related to the endpoint URL. This code below is taken from the bottom of a GraphQL request made locally.

```
"extensions": {
"requests": {
"gravity": {
"requests": {
"partner/5a0323e68b0c146d7634f081/artworks?artsy_shipping_domestic=true": {
"time": "0s 495.276ms",
"cache": false,
"length": "0 B"
}
}
}
},
"requestID": "72c50460-119f-11ed-9554-25745ca04f3b"
}
```

### Potential Errors

This below error points to the fact that the `UpdatePartnerArtworksMutationType` was not resolving for either the `SuccessType` or the `FailureType`. GraphQL is very strict and it needs to categorize everything into types and fails when it can't. The solution to this was to ensure that the `SuccessType` was correctly defined. This means that the `isTypeOf` check needs to look for a success criteria in the response. In this scenario, it was incorrect and needed to be updated to `isTypeOf: (data) => data.success`.
Copy link
Member

@damassi damassi Aug 1, 2022

Choose a reason for hiding this comment

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

Similar to the comment above, this error is a bit too specific and tied to product work for a getting started doc! If we're trying to onboard devs we need to start with the simplest use-case first and then incrementally increase complexity. Starting here is too scary and specific 👻

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is based on my personal experience and I wanted to have a potential errors section where we can list generic errors that might help people find the root cause of their own errors, without relying on someone with more GraphQL knowledge. I didn't pay attention to all the errors that I experienced before I decided to write this doc so unfortunately I only have this one. I found it super helpful after understanding the error on how the code determines whether something is a success or an error. I think it is useful here (something along these lines, of the response not being a success or an error) however happy to move it somewhere else talking generally about the job of each function


```json
"message": "Abstract type UpdatePartnerArtworksMutationType must resolve to an Object type at runtime for field UpdatePartnerArtworksMutationPayload.partnerArtworksRequestOrError with value { success: 0, errors: { count: 4, ids: [Array] }, clientMutationId: undefined }, received \"undefined\". Either the UpdatePartnerArtworksMutationType type should provide a \"resolveType\" function or each possible type should provide an \"isTypeOf\" function.",
```
5 changes: 5 additions & 0 deletions src/lib/loaders/loaders_with_authentication/gravity.ts
Original file line number Diff line number Diff line change
Expand Up @@ -314,6 +314,11 @@ export default (accessToken, userID, opts) => {
{},
{ headers: true }
),
updatePartnerArtworksLoader: gravityLoader(
(id) => `partner/${id}/artworks`,
{},
{ method: "PUT" }
),
partnerInquirerCollectorProfileLoader: gravityLoader<
any,
{ partnerId: string; inquiryId: string }
Expand Down
136 changes: 136 additions & 0 deletions src/schema/v2/partnerArtworksMutation.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,136 @@
import {
GraphQLBoolean,
GraphQLInt,
GraphQLList,
GraphQLNonNull,
GraphQLString,
} from "graphql"
import { mutationWithClientMutationId } from "graphql-relay"
import { ResolverContext } from "types/graphql"
import {
GravityMutationErrorType,
formatGravityError,
} from "lib/gravityErrorHandler"
import { GraphQLObjectType } from "graphql"
import { InternalIDFields } from "./object_identification"
import { GraphQLUnionType } from "graphql"
interface Input {
id: string
artsyShippingDomestic: boolean | null
artsyShippingInternational: boolean | null
location: string | null
}

const PartnerArtworksType = new GraphQLObjectType<any, ResolverContext>({
Mitch-Henson marked this conversation as resolved.
Show resolved Hide resolved
name: "PartnerArtworks",
fields: () => ({
...InternalIDFields,
Mitch-Henson marked this conversation as resolved.
Show resolved Hide resolved
success: { type: GraphQLInt },
errors: {
type: new GraphQLObjectType({
name: "PartnerArtworksError",
fields: {
count: { type: GraphQLInt },
ids: { type: GraphQLList(GraphQLString) },
},
}),
},
}),
})

const UpdatePartnerArtworksMutationSuccessType = new GraphQLObjectType<
any,
ResolverContext
>({
name: "UpdatePartnerArtworksMutationSuccess",
isTypeOf: (data) => data.success || data.errors,
Mitch-Henson marked this conversation as resolved.
Show resolved Hide resolved
fields: () => ({
partnerArtworks: {
type: PartnerArtworksType,
resolve: (partnerArtworks) => partnerArtworks,
},
}),
})

const UpdatePartnerArtworksMutationFailureType = new GraphQLObjectType<
any,
ResolverContext
>({
name: "UpdatePartnerArtworksMutationFailure",
isTypeOf: (data) => {
return data._type === "GravityMutationError"
},
fields: () => ({
mutationError: {
type: GravityMutationErrorType,
resolve: (err) => err,
},
}),
})

const UpdatePartnerArtworksMutationType = new GraphQLUnionType({
name: "UpdatePartnerArtworksMutationType",
types: [
UpdatePartnerArtworksMutationSuccessType,
UpdatePartnerArtworksMutationFailureType,
],
})

export const updatePartnerArtworksMutation = mutationWithClientMutationId<
Input,
any | null,
ResolverContext
>({
name: "UpdatePartnerArtworksMutation",
description: "Update all artworks that belong to the partner",
inputFields: {
id: {
type: new GraphQLNonNull(GraphQLString),
description: "ID of the partner",
},
artsyShippingDomestic: {
type: GraphQLBoolean,
description: "Whether Artsy domestic shipping should be enabled",
},
artsyShippingInternational: {
type: GraphQLBoolean,
description: "Whether Artsy international shipping should be enabled",
},
location: {
type: GraphQLString,
description: "The partner location ID to assign",
},
},
outputFields: {
partnerArtworksOrError: {
type: UpdatePartnerArtworksMutationType,
resolve: (result) => result,
},
},
mutateAndGetPayload: (
{ id, artsyShippingDomestic, artsyShippingInternational, location },
{ updatePartnerArtworksLoader }
) => {
if (!updatePartnerArtworksMutation) {
Mitch-Henson marked this conversation as resolved.
Show resolved Hide resolved
throw new Error(
"You need to be signed in as an admin to perform this action"
)
}

const gravityOptions = {
artsy_shipping_domestic: artsyShippingDomestic,
artsy_shipping_international: artsyShippingInternational,
location,
}
return updatePartnerArtworksLoader?.(id, gravityOptions)
Mitch-Henson marked this conversation as resolved.
Show resolved Hide resolved
.then((result) => result)
.catch((error) => {
const formattedErr = formatGravityError(error)
if (formattedErr) {
return { ...formattedErr, _type: "GravityMutationError" }
} else {
throw new Error(error)
}
})
},
})
2 changes: 2 additions & 0 deletions src/schema/v2/schema.ts
Original file line number Diff line number Diff line change
Expand Up @@ -147,6 +147,7 @@ import { toggleFeatureFlagMutation } from "./admin/mutations/toggleFeatureFlagMu
import { MatchConnection } from "./Match"
import { PartnerArtistDocumentsConnection } from "./partnerArtistDocumentsConnection"
import { PartnerShowDocumentsConnection } from "./partnerShowDocumentsConnection"
import { updatePartnerArtworksMutation } from "./partnerArtworksMutation"

const PrincipalFieldDirective = new GraphQLDirective({
name: "principalField",
Expand Down Expand Up @@ -313,6 +314,7 @@ export default new GraphQLSchema({
updateUserSaleProfile: updateUserSaleProfileMutation,
updateMyUserProfile: UpdateMyUserProfileMutation,
updateNotificationPreferences: updateNotificationPreferencesMutation,
updatePartnerArtworks: updatePartnerArtworksMutation,
deleteMyUserProfileIcon: deleteCollectorProfileIconMutation,
requestPriceEstimate: requestPriceEstimateMutation,
},
Expand Down