Currency handling with money filters

[timdmackey]

On the Slack workspace, a developer raised an issue where the Send a PDF invoice when an order is created task was returning incorrect prices. As noted in the discussion, the issue is caused by Shopify's API presenting currency values in their dollars and cents format, rather than cents only as the Shopify theming system does. This creates a conflict in how currencies are calculated, resulting in any currencies formatted with money filters being 100x too small.

The simple solution for most currencies is to multiple the currency value by 100 before using a money filter, like so: {{ price | times: 100 | money_with_currency }}. However, there are several currencies supported by Shopify which are not divisible by 100, which makes this solution not entirely reliable. I've attempted to make a complete list of supported currencies where this would be an issue, and this is what I came up with. It may not be exhaustive:

• CNY - Chinese yuan - 10 Jiao ver yuan
• MGA - Malagasy ariary - 5 iraimbilanja per ariary
• UGX - Ugandan shilling - no subdivision
• VUV - Vanuatu vatu - no subdivision
• VND - Vietnamese đồng - 10 hào per đồng

Before creating a pull request to fix this particular task, a more robust solution is required. I am currently at a loss as to the proper way to fix this.

References
List of Global Currencies: https://en.wikipedia.org/wiki/List_of_circulating_currencies

Shopify Supported Currencies: https://help.shopify.com/en/manual/payments/shopify-payments/multi-currency#supported-currencies

[isaacbowen]

This is phenomenal sleuthing - thank you, @timdmackey!

Pretty sure the right solution here is to solve this at the platform level. Backwards-compatibility is mandatory, so it's likely that we'll end up with a new account-level setting that determines the behavior of this filter. New Mechanic accounts will have this setting configured in a way that results in the intuitively-expected behavior of money_with_currency; old accounts will have the setting configured to maintain the current behavior.

@timdmackey @mattsodomsky check me on this - sane?

[mattsodomsky]

@isaacbowen What is the current behaviour of this filter? What will be the new behaviour? I want to make sure we are all on the same page!

[isaacbowen]

Ah yes. :) Current behavior: the input is divided by 100 (literally input.to_f / 100.0), and then rendered with the current shop's money_with_currency_format value, from the REST API.

This behavior was inherited from Shopify's storefront liquid rendering (see their docs); I'm proposing that we change our default behavior so that it just uses the input value transparently (i.e. without dividing by 100), while allowing older stores to keep the current behavior via a new account-level setting.

cc @tekhaus, because I wouldn't mind your eyes on this too

[timdmackey]

@isaacbowen, your suggestion was my first inclination. Seems like a good solution to me! Like you though, I'm definitely cautious about making a change like this, as it's really important to avoid any unintended consequences. Messing with prices is pretty fundamental!

I'd imagine similar issues have come up before regarding Shopify's API, I wonder what other people have done? With the strangeness of those few currencies I mentioned, I also wonder how the calculation you described applies in those cases.

Additionally, when this change is made someone will need to go through the current task library and update any uses of the money filter so they output correctly.

[isaacbowen]

Additionally, when this change is made someone will need to go through the current task library and update any uses of the money filter so they output correctly.

Wellllll it's worse than that, since an account-level setting would mean that stock tasks would need to dynamically account for whatever that setting is currently at.

... That's too much complexity at the task level, so I don't think this route is tenable. Maybe it's time for a new filter entirely.

[mattsodomsky]

Additionally, when this change is made someone will need to go through the current task library and update any uses of the money filter so they output correctly.

Wellllll it's worse than that, since an account-level setting would mean that stock tasks would need to dynamically account for whatever that setting is currently at.

... That's too much complexity at the task level, so I don't think this route is tenable. Maybe it's time for a new filter entirely.

A new filter seems like the way to go.

[timdmackey]

Looks like there are currently 5 tasks in the library that use money filters:

• send-an-email-when-a-products-price-goes-below-its-cost - Correct filter use
• send-email-when-an-order-is-tagged - Correct filter use
• unpaid-order-reminders - Mixed: 1 Correct filter use, and 1 INCORRECT filter use (does not times by 100)
• generate-a-report-of-orders-that-still-require-payment - 1 INCORRECT filter use (does not times by 100)
• send-a-pdf-invoice-when-an-order-is-created - 5 INCORRECT filter uses (does not times by 100)

[isaacbowen]

@timdmackey 🙏 Thanks for cataloging those - super useful. We're going to figure out a new filter (honestly, the platform deserves one anyway - we need something with flexibility around multi-currency), and we'll update this issue when it's out.

[isaacbowen]

tracking this in https://mechanic.canny.io/feature-requests/p/a-new-liquid-filter-for-currency-formatting

[isaacbowen]

Taking this conversation to Slack: https://usemechanic.slack.com/archives/C01KF3B4PUK/p1613593798007500

[tekhaus]

This issue can be closed with this update, yes? https://mechanic.canny.io/changelog/new-liquid-filter-currency

[timdmackey]

Relevant tasks in the task library need to be updated with the new filter. I believe there are a couple others aside from the one mentioned in the original post, which output incorrect prices because of the original discrepancy with the money filter.

[tekhaus]

Sounds like some PR's are in your future, ha!

But yes, good point, and you've nicely listed them all out previously. @isaacbowen what say you? Shall this issue stay open until relevant PRs are attached for the affected tasks?

[timdmackey]

Thanks for the reminder! I had forgotten about my original issue here on GitHub :)

[timdmackey]

Oh boy...I just started working on a pull request, and the first instance of | money I came across is actually | money_with_currency...which doesn't yet have an equivalent with the currency filter. How should I proceed @isaacbowen? do we need a currency_with_iso_code filter? I could just write it like the following, but this isn't as clean – i.e. change this:

{{ price | money_with_currency }}

to this:

{{ price | currency }} {{ shop.currency }}

There are also a few assignments, which would mean changing this:

{% assign formatted_price = price | money_with_currency %}

to this:

{% assign formatted_price = price | currency | append: " " | append: shop.currency %}

[isaacbowen]

@timdmackey It's not as clean, true, but I think it's the direction that I want to go in. So, that's my vote: go with the styles that you gave in your examples. (To explain my preference for not building this into the currency filter: I can imagine enough other ways that authors would want to pull in the ISO code that I don't feel comfortable adding support for one particular way.)

I'll update learn.mechanic.dev with your suggestions, letting people know that they can pull in the ISO code in these ways.

[timdmackey]

Sounds good! Offhand, do you have any opinion on whether to hard-code the currency ISO or expose it as an option? I'll have to look at all the tasks to see whether different contexts might want to have different approaches. I'm thinking something like this as a possibility:

// at the top of the task code:
{% if options.currency_iso_code %}
  {% assign currency_iso_code = options.currency_iso_code | upcase %}
{% else %}
  {% assign currency_iso_code = shop.currency %}
{% endif %}

// Outputting prices:
{{ price | currency: currency_iso_code }} {{ currency_iso_code }}

[isaacbowen]

In places where it's not a surprising thing to see in the task options, sure! I realize that's a slightly hand-wavey answer, but that's how I'd think about it: don't add it if it's something that would raise an eyebrow when scanning task options, do add it if it feels like it belongs in the options list.

By the way! You can use the "default" filter to simplify that code a tad:

// at the top of the task code:
{% assign currency_iso_code = options.currency_iso_code | upcase | default: shop.currency %}

// Outputting prices:
{{ price | currency: currency_iso_code }} {{ currency_iso_code }}

Also I see you and your comment syntax, lol. (Context, for onlookers, and here's our Slack if you need it. 😜)

[timdmackey]

Haha, I was wondering if you'd say anything about the comments 😆 . Sounds good! Thanks for the reminder about the default filter, I always forget about that.