-
Notifications
You must be signed in to change notification settings - Fork 89
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
Bulk update mutation tweaks #4277
Conversation
Just a note on that peril warning above ☝️ since volt is the only consumer of these fields, this should be a-ok |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The naming changes look much better than what I came up with and are really nicely standardised. I think there needs to be some small changes relating to the gravity response but otherwise looks great
type BulkUpdatePartnerArtworksResponse { | ||
count: Int | ||
ids: [String] | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't errors
also be here? It is a part of the response and we seem to be using it in the formattedReturn
|
||
const formattedReturn = { | ||
updatedPartnerArtworks: { count: gravityResponse.success, ids: [] }, | ||
skippedPartnerArtworks: gravityResponse.errors, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@lilyfromseattle maybe let's be explicit here so future devs don't have to go digging into the gravity response format
skippedPartnerArtworks: { count: gravityResponse.errors.count, ids: gravityResponse.errors.ids }
Currently blocking merging, which we need to unblock Lily's Volt work!
@ansor4 and I made some tweaks to the bulk update mutation to simplify the structure and make the naming more consistent. If this looks all good to you I can continue with my front end changes once its merged and released! If there are any issues or questions we can discuss at the sync I scheduled for tomorrow morning.