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

isDiscounted compares 2 strings instead of 2 numbers #2354

Open
wants to merge 1 commit into
base: v1.x-2022-07
Choose a base branch
from

Conversation

superjova
Copy link

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.
Copy link
Contributor

@frehner frehner left a comment

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
Copy link

pixelchutes commented Dec 16, 2022

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.

None yet

3 participants