medusajs / medusa

Building blocks for digital commerce
https://medusajs.com
MIT License
24.33k stars 2.38k forks source link

Adding a custom currency makes the backend fail #4907

Open mortenengel opened 12 months ago

mortenengel commented 12 months ago

Bug report

Describe the bug

We are using medusa for an internal company store, where users are assigned points to shop for, rather than using real money. Instead of using one of the built in currencies, we wanted to create a "points" currency, as described in this article: https://docs.medusajs.com/modules/regions-and-currencies/currencies

After adding a new currency in the database, we can easily add it to a region, but after adding it to the region and trying to set a price on a product, we get the following error: Unexpected Application Error! Cannot read properties of undefined (reading 'name') TypeError: Cannot read properties of undefined (reading 'name') at wg (https://nordicsemi-medusa-admin.azurewebsites.net/app/251.d7cdb615.chunk.js:1:1246731) at aS (https://nordicsemi-medusa-admin.azurewebsites.net/app/main.f7477009.js:18:60913) at l (https://nordicsemi-medusa-admin.azurewebsites.net/app/main.f7477009.js:18:119434) at sq (https://nordicsemi-medusa-admin.azurewebsites.net/app/main.f7477009.js:18:99087) at https://nordicsemi-medusa-admin.azurewebsites.net/app/main.f7477009.js:18:98954 at sN (https://nordicsemi-medusa-admin.azurewebsites.net/app/main.f7477009.js:18:98961) at sO (https://nordicsemi-medusa-admin.azurewebsites.net/app/main.f7477009.js:18:95713) at sC (https://nordicsemi-medusa-admin.azurewebsites.net/app/main.f7477009.js:18:94268) at P (https://nordicsemi-medusa-admin.azurewebsites.net/app/main.f7477009.js:51:1364) at MessagePort.R (https://nordicsemi-medusa-admin.azurewebsites.net/app/main.f7477009.js:51:1894)

Looking into the code, I can see that both backend-admin and api (which in turn means frontend) uses the values directly from the currency.ts rather than taking it from the database. In addition to this, currency.ts holds more properties than can be added to the database.

I tried just changing the name and symbols of an already created currency in the database, to see if I would be able to use it this way - but the symbols are still grabbed from the file rather than the database.

It looks to me like it would not be possible for us to use points, without having them added to the currencies.ts file

I am guessing, a non-valid currency (points) is unwelcome as a pull request? Do you see any way for us to have this working properly, other than hardcoding everything in our storefront and ignoring the currency backend?

System information

Medusa version (including plugins): 1.14 Node.js version: 18.16 Database: postgres Operating system: linux on prod, windows development Browser (if relevant):

Steps to reproduce the behavior

  1. Create a migration for a new currency
  2. Enable the currency in the market
  3. Try to add a price in this currency

Expected behavior

Being able to add and utilize currencies like documentation states

Screenshots

admin error: image

doc: image

srindom commented 12 months ago

This erros is thrown in the frontend - would be relevant to figure out exactly where the error is coming from i.e. what "name" field is the admin dashboard trying to read. This looks like a deployment so figuring out what wg is in the source files might be tricky unless there's a sourcemap uploaded? You can try reproducing this locally and that should give some more insights into what's going on.

mortenengel commented 12 months ago

@srindom when running in dev mode locally I see this error: image

which is using NestedPrice: https://github.com/medusajs/medusa/blob/359bd290baa14cdb0d582d60fc76322dc8acb4ae/packages/admin-ui/ui/src/components/forms/general/prices-form/nested-price.tsx#L19

Which is using currencies directly from the file as mentioned - a lot of functionality is using currencies from the currencies.ts file rather than from the database (and the database does not even hold all necesary info).

srindom commented 12 months ago

I assume the error is on line 52 then.

What does the migration for your points currency look like - did you add a name column?

mortenengel commented 12 months ago

@srindom yes - I have actually deleted the migration now since it didn't work - but I didn't delete the contents of the database yet:

image

But.. correct me if I'm wrong - I don't think the migration will ever go and change the contents of the currencies.ts file, so as long as that file is used as much as it is for both API and backend UI, there's really no way of adding new currencies, without also having them added to that file.

srindom commented 12 months ago

Ahh hang on I see the issue now - it's not part of the currencies list in the Admin project: https://github.com/medusajs/medusa/blob/359bd290baa14cdb0d582d60fc76322dc8acb4ae/packages/admin-ui/ui/src/components/forms/general/prices-form/nested-price.tsx#L11

Would definitely make sense to allow currencies that don't appear in that list.

Could be a change like this in nested-price.tsx:

currencyPrice.currency_code.toUpperCase() in currencies ? currencies[currencyPrice.currency_code.toUpperCase()].name : currencyPrice.currency_code.toUpperCase()
mortenengel commented 12 months ago

That would fix it for this particular case - but the code base is full of uses from currencies.ts and currencies.ts holds more information than the database does as well (decimal digits, rounding, name_plural), so a lot of code can't even be done with just the database version.

On top of this initial problem - I don't get the symbols configured in database returned to the frontend via API (see my attempt to change the symbols for Euro manually in the database and restarting the backend, in order to see if that would change the symbols in the frontend - no luck)

srindom commented 12 months ago

Yeah - the proper fix would be to source the currencies from the backend instead of the hard coded currencies.ts file.

mortenengel commented 12 months ago

I agree - however I could fear the implications of doing such a change.

Currently I don't think my overall knowledge of the Medusa architecture is sufficient to make a pull request of sufficient quality. However I would be very willing to talk this through with you first.

kasperkristensen commented 11 months ago

Hi @mortenengel, just adding my two cents to the matter:

The currencies.ts file is not accessed during build time, so it's safe to move the currency info to the backend in this regard.

For your concern surrounding the runtime aspect, it is right that it would have an impact on performance, by requiring two roundtrips to the DB to fetch the information needed to render a currency amount correctly. This is obviously not ideal, but in the case of the admin, every request is cached using @tanstack/react-query, so the performance hit would be very minimal.

The ideal case would obviously be removing the need for having to fetch info such as how many decimal digits are used in a currency and leaving the conversion between the DB value and the presentational value to the server. But this would be a rather large breaking change, and properly not something we would be able to add at the moment, but we would need to wait for a 2.0.0 release to ship.

I still think moving this data away from the currencies.ts file in admin and into the DB would be a welcome addition, as it would allow the Medusa Admin to support cases like the one you have outlined.

As I see it the work could be broken down into the following steps:

  1. Extend the Currency model to include a decimal_digits field, and create a migration script to populate this new field for all existing currencies.
  2. Ensure that this new field is readily available on all endpoints where it makes sense, one case of this is to make sure that we allow loading the currency relation on region endpoints, so users can access the currency info needed to display region prices correctly. (This might already be the case but I haven't checked.)
  3. Replace the usage of currencies.ts with useAdminCurrencies from medusa-react across admin.

Ideally, we would also cache currency requests on the backend, as we already do for some other types of requests, but I don't see this as crucial for a first iteration as this would mostly be used in the admin which already implements its own caching mechanism. Until we can leave the conversion of prices entirely to the backend storefronts would most likely still be better off using the price utils from medusa-react or their own utility for transforming DB amounts to presentational amounts.

While not a perfect solution, I still think it would enable your use case without introducing any major breaking changes. If it is something you would be willing to work on I am sure we would be happy to assist you with anything that comes up. 😄

cc: @srindom

mortenengel commented 11 months ago

@kasperkristensen thank you for your detailed answer. Unfortunately we've moved a bit too close to deadline for us to include this in a first phase, and we will most likely start out using a lesser known currency as a fallback, in order for the points not to be confused with real money .

We may pick this up for a phase 2 - but as of right now, we won't be starting work on a fix.

Tombar commented 6 months ago

Hello, we are evaluating using MedusaJs for a new store that would also use points instead of a real currency and where wondering the current status of this ticket and if it would make sense to contribute a new "points" currency

mortenengel commented 6 months ago

Hi @Tombar To my knowledge it has not been fixed. However the values from the API are not sent as formatted strings, but currency code field and a value field - so what we did in our case was just change the frontend to disregard currency and just write points everywhere instead. The backend still writes the prices in our selected currency, but that's not too important for us.

netpoe commented 6 months ago

Hi @mortenengel, just adding my two cents to the matter:

The currencies.ts file is not accessed during build time, so it's safe to move the currency info to the backend in this regard.

For your concern surrounding the runtime aspect, it is right that it would have an impact on performance, by requiring two roundtrips to the DB to fetch the information needed to render a currency amount correctly. This is obviously not ideal, but in the case of the admin, every request is cached using @tanstack/react-query, so the performance hit would be very minimal.

The ideal case would obviously be removing the need for having to fetch info such as how many decimal digits are used in a currency and leaving the conversion between the DB value and the presentational value to the server. But this would be a rather large breaking change, and properly not something we would be able to add at the moment, but we would need to wait for a 2.0.0 release to ship.

I still think moving this data away from the currencies.ts file in admin and into the DB would be a welcome addition, as it would allow the Medusa Admin to support cases like the one you have outlined.

As I see it the work could be broken down into the following steps:

  1. Extend the Currency model to include a decimal_digits field, and create a migration script to populate this new field for all existing currencies.
  2. Ensure that this new field is readily available on all endpoints where it makes sense, one case of this is to make sure that we allow loading the currency relation on region endpoints, so users can access the currency info needed to display region prices correctly. (This might already be the case but I haven't checked.)
  3. Replace the usage of currencies.ts with useAdminCurrencies from medusa-react across admin.

Ideally, we would also cache currency requests on the backend, as we already do for some other types of requests, but I don't see this as crucial for a first iteration as this would mostly be used in the admin which already implements its own caching mechanism. Until we can leave the conversion of prices entirely to the backend storefronts would most likely still be better off using the price utils from medusa-react or their own utility for transforming DB amounts to presentational amounts.

While not a perfect solution, I still think it would enable your use case without introducing any major breaking changes. If it is something you would be willing to work on I am sure we would be happy to assist you with anything that comes up. 😄

cc: @srindom

I have more to add to these insights, since I intended to use MedusaJS on a crypto marketplace, I found it hard to do since my prices are in ETH, I need to setup my base currency in ETH. I created a migration to do so:

import { MigrationInterface, QueryRunner } from "typeorm";

export class AddEthereumCurrency1709007806066 implements MigrationInterface {

    public async up(queryRunner: QueryRunner): Promise<void> {
        await queryRunner.query(
            `INSERT INTO currency (code, symbol, symbol_native, name) VALUES ('eth', 'ETH', 'Ξ', 'Ethereum')`
        );
    }

    public async down(queryRunner: QueryRunner): Promise<void> {
        await queryRunner.query(
            `DELETE FROM currency WHERE code = 'eth'`
        );
    }

}

But then found out that the error that @mortenengel described happened to me as well.

I then edited the node_modules currencies file directly (bad practice, I know) to make the admin ui work again:

ETH: {
    symbol: "ETH",
    name: "Ethereum",
    symbol_native: "Ξ",
    decimal_digits: 8,
    rounding: 0,
    code: "ETH",
    name_plural: "Ethereum",
  },

And noticed that with decimal_digits: 8, I get this result in the database after setting a price for a product variant:

Screenshot 2024-02-27 at 09 14 01 Screenshot 2024-02-27 at 09 13 32

Which in my case, I then need to hack the frontend NextJS starter in order to display correct decimal prices:

Screenshot 2024-02-27 at 09 17 02
// I'm not proud of this hack and it is inconsistent. It is only for MVP display purposes since calculations are still wrong 😅

const convertToLocale = ({
  amount,
  currency_code,
  minimumFractionDigits,
  maximumFractionDigits,
  locale = "en-US",
}: ConvertToLocaleParams) => {
  if (currency_code === "eth") {
    return `ETH 0.${amount.toString()}`
  }
clemsos commented 5 months ago

+1 support in Admin UI for all currencies in the db will be a great addition.

ismathibrahim commented 5 months ago

+1 It would be nice to at least have all the currencies on the ISO in currencies.ts. the My country's currency (Maldivian Rufiyaa, MVR) is not on the list for some reason, so I'm unable to use the admin UI. Should I make a PR with MVR added to currencies.ts?

kings-joy commented 5 months ago

Will this be covered in Medusa 2 with new currency module? https://github.com/medusajs/medusa/pull/6536 +1 for virtual currencies and regions

Garabed96 commented 5 months ago

@kasperkristensen what do you mean its safe to move the currencies to the backend , if we did move currencies, could we link it to the .cache for all references or would we have to go through and create new api endpoints. I'm attempting to add a new Currency that doesn't exist and finding it quite challenging. Would appreciate any insight.

dokind commented 3 months ago

how to fix this bug very fast ? I only added mongolian Currency and it's not working

toooo commented 3 months ago

It does not work as per documentation, cause when you want to add price to product variant you get error Cannot read properties of undefined (reading 'symbol_native') "So, if you want to add more currencies, you can create a migration that inserts your currencies into the database." https://drive.google.com/file/d/15zmNwoV8VEdRu9hhpY58GzpmD538hXSv/view?usp=drivesdk

@kasperkristensen Do you know if this will land in medusa 2.0? But this would be a rather large breaking change, and properly not something we would be able to add at the moment, but we would need to wait for a 2.0.0 release to ship.