Skip to content

isDiscounted compares 2 strings instead of 2 numbers#2354

Open
superjova wants to merge 1 commit into
Shopify:v1.x-2022-07from
superjova:patch-1
Open

isDiscounted compares 2 strings instead of 2 numbers#2354
superjova wants to merge 1 commit into
Shopify:v1.x-2022-07from
superjova:patch-1

Conversation

@superjova

Copy link
Copy Markdown

While working with the demo-store template, I was having issues getting the discounts to show up.

My product:

  • price: 50.0
  • compareAtPrice: 100.0

This is an item that I was expecting to show a sale price. I loaded the demo-store and was unable to see the sale. I debugged the issue to the utils isDiscounted() function.

export function isDiscounted(price: MoneyV2, compareAtPrice: MoneyV2) {
  if (compareAtPrice?.amount > price?.amount) {
    return true;
  }
  return false;
}

We see that we are comparing a MoneyV2.amount with another MoneyV2.amount. What's wrong with this?

export declare type MoneyV2 = {
    __typename?: 'MoneyV2';
    /** Decimal money amount. */
    amount: Scalars['Decimal'];
    /** Currency of the money. */
    currencyCode: CurrencyCode;
};

In the type definition, we see that amount: Scalars['Decimal']. However, when I access the attribute, I actually am getting back a string representation of a decimal.

So for my example

compareAtPrice?.amount > price?.amount
// "100.0" > "50.0"
// => false

Instead, let's cast the strings to their decimals and compare.

Description

Additional context


Before submitting the PR, please make sure you do the following:

  • Read the Contributing Guidelines
  • Provide a description in this PR that addresses what the PR is solving, or reference the issue that it solves (e.g. fixes #123)
  • Update docs in this repository according to your change
  • Run yarn changeset add if this PR cause a version bump based on Keep a Changelog and adheres to Semantic Versioning

While working with the demo-store template, I was having issues getting the discounts to show up.

My product:
- price: 50.0
- compareAtPrice: 100.0

This is an item that I was expecting to show a sale price. I loaded the demo-store and was unable to see the sale. I debugged the issue to the utils isDiscounted() function.

```
export function isDiscounted(price: MoneyV2, compareAtPrice: MoneyV2) {
  if (compareAtPrice?.amount > price?.amount) {
    return true;
  }
  return false;
}
```

We see that we are comparing a MoneyV2.amount with another MoneyV2.amount. What's wrong with this?

```
export declare type MoneyV2 = {
    __typename?: 'MoneyV2';
    /** Decimal money amount. */
    amount: Scalars['Decimal'];
    /** Currency of the money. */
    currencyCode: CurrencyCode;
};
```

In the type definition, we see that amount: Scalars['Decimal']. However, when I access the attribute, I actually am getting back a string representation of a decimal.

So for my example
```
compareAtPrice?.amount > price?.amount
// "100.0" > "50.0"
// => false
```

Instead, let's cast the strings to their decimals and compare.

@frehner frehner left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Thanks! Do you mind adding a changelog as well? It should be documented in the CONTRIBUTING doc

@pixelchutes

pixelchutes commented Dec 16, 2022

Copy link
Copy Markdown

Thanks for reporting this @superjova!

I also ran into this bug and had to abandon the Hydrogen provided utility for the time being.

Can confirm that casting works as expected with decimal amount:

const isDiscounted = (price, compareAtPrice) => Number(compareAtPrice?.amount) > Number(price?.amount);

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants