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

chore: consistent artwork money fields resolver #6367

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

erikdstock
Copy link
Contributor

Downstream from some hackathon work... This PR updates our artwork and editionSet fields to use a consistent resolver for the money type, avoiding missing display values.

In order to preserve some behavior I clobbered that display value on one field where we are using the gravity price_display json value instead. There might be other cases where we want to verify that nothing is changing drastically - for example, are we querying any display fields where the format is hardcoded.

cc @artsy/emerald-devs

@erikdstock erikdstock requested a review from oxaudo January 22, 2025 00:06
@erikdstock erikdstock self-assigned this Jan 22, 2025
@artsy-peril
Copy link
Contributor

artsy-peril bot commented Jan 22, 2025


Warnings
⚠️ The V2 schema in this PR has breaking changes with Force. Remember to update the Force schema if necessary.

Field 'collection' was removed from object type 'Query'
Field 'collection' was removed from object type 'Viewer'

Generated by 🚫 dangerJS against d4007f9

Copy link
Member

@oxaudo oxaudo left a comment

Choose a reason for hiding this comment

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

This looks great to me. Thanks @erikdstock !

@@ -676,7 +676,7 @@ describe("Artwork type", () => {
minPrice: {
minor: 42000,
major: 420,
display: null,
display: "US$420",
Copy link
Member

Choose a reason for hiding this comment

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

❤️

resolve: (artwork) => {
const { price_paid_cents } = artwork
resolve: async (artwork, args, ctx, info) => {
const { price_paid_cents, price_paid_currency = "USD" } = artwork
Copy link
Member

Choose a reason for hiding this comment

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

Slightly surprised that we ask collectors to enter price in cents, but I guess it was here already.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It is probably calculated on the artwork from some input using the money helper gem

return (
moneyFields && {
...moneyFields,
// TODO: Display field legacy implementation maintained until we verify
Copy link
Member

Choose a reason for hiding this comment

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

Should we create a separate card for this?

@erikdstock
Copy link
Contributor Author

Now wondering if I should use the amount() helper to let money display values be formatted better...

@oxaudo
Copy link
Member

oxaudo commented Jan 23, 2025

Now wondering if I should use the amount() helper to let money display values be formatted better...

Good point @erikdstock. Think we should generate the same string as gets added for SippingInfo below in the same file:

let domesticShipping = amount(

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants