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

[Feature]: Support nested price conversion #1697

Closed
wants to merge 6 commits into from

Conversation

fengkx
Copy link

@fengkx fengkx commented Oct 14, 2023

Support nested price conversion Related issue #1407

Always convert to biggest value using Bellman–Ford algorithm.

For example, when using convert to HKD, find the shortest(Smallest Conversion Rate) from HKD to others currency. $K$
then the price from others to HKD is $\frac{1}{K}$ (largest)

  • Unit test, linter
  • Description, comment

@fengkx fengkx force-pushed the feat/nested-price-conversion branch from 818b905 to 14513ff Compare October 14, 2023 18:08
@fengkx fengkx marked this pull request as ready for review October 15, 2023 03:42
@fengkx fengkx changed the title [WIP] feat: nested price conversion [Feature]: Support nested price conversion Oct 15, 2023
@yagebu
Copy link
Member

yagebu commented Jan 6, 2024

Hi, thank you for the PR.

To allow for multiple currency conversions, I prefer a more explicit approach where one either explicitly adds additional prices via a plugin (as is already possible) or by explicitly specifying the currencies to convert via/to. With #1732 the latter will also be possible. Also, the implementation here does not seem very performant, on some reports the load times seem to have doubled. So I do not intend to merge your PR - does #1732 also work for your use case?

@fengkx
Copy link
Author

fengkx commented Jan 8, 2024

I use the master branch, But can't find the correct way to use #1732.
image

I have a long list of price directive created by bean-price daily. I want all Other currency be converted into CNY when I choose converted to CNY.

@yagebu
Copy link
Member

yagebu commented Jan 13, 2024

If you have conversion rates from your other currencies to, e.g., HKD (and then prices for HKD in CNY), you could choose HKD,CNY as conversions to get everything converted to CNY.

@fengkx
Copy link
Author

fengkx commented Jan 15, 2024

I am using the main branch, But I can't find the option in the UI.
image

What if I have many other currenecies (or stocks) which also want to convert to CNY?
If some of them are priced in USD while others are priced in HKD, what can I do the convert all of them at the same time?

@yagebu
Copy link
Member

yagebu commented Jan 25, 2024

I am using the main branch, But I can't find the option in the UI.

You can select multiple currencies from that dropdown

@fengkx
Copy link
Author

fengkx commented Jan 26, 2024

OK, I understand it now. The order in which I select affects the conversion. My pr uses the Bellman–Ford algorithm to find the shortest path, which makes it slow, while the current solution requires the user to find the correct path. I think the UI does not inform the user that the selection order would affect the results.

I have a question while I am devloping locally. How to set up the translations... It seems to require a POEDITOR_TOKEN

@yagebu
Copy link
Member

yagebu commented Feb 18, 2024

OK, I understand it now. The order in which I select affects the conversion. My pr uses the Bellman–Ford algorithm to find the shortest path, which makes it slow, while the current solution requires the user to find the correct path.

Exactly - it also allows/requires the user to pick a specific path. Finding shouldn't normally be hard AFAIKT, since it will usually be just via your operating currencies.

I think the UI does not inform the user that the selection order would affect the results.

That's why I added a help page to describe it - I don't think this can be easily shown in the UI

I have a question while I am devloping locally. How to set up the translations... It seems to require a POEDITOR_TOKEN

No, that token is only necessary to update the translations from poeditor.com (where they are edited). If you want to edit the translations, you'll have to do so at poeditor.

@yagebu yagebu closed this Feb 18, 2024
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.

2 participants