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

Native currency names #49

Merged
merged 3 commits into from
Feb 10, 2020
Merged

Native currency names #49

merged 3 commits into from
Feb 10, 2020

Conversation

microposmp
Copy link
Contributor

Changed to use the english currency name. We got random results with the name on different machines.

@bleroy
Copy link
Member

bleroy commented Feb 9, 2020

What kind of issue did you get? This looks wrong.

@microposmp
Copy link
Contributor Author

The issue is that the call to CultureInfo.GetCulture returns cultures in different order on different machines.

In the function InitCurrencyCodeTable this introduces a problem when distinct currencies are retrieved.

private static void InitCurrencyCodeTable()
{
	lock (Obj)
	{
		bool valid(CultureInfo c) => !c.IsNeutralCulture && !c.EnglishName.StartsWith("Unknown Locale") && !c.EnglishName.StartsWith("Invariant Language");

		CurrencyTable = CultureInfo.GetCultures(CultureTypes.AllCultures)
			.Where(valid)
			.Select(c => new Currency(c)).Cast<ICurrency>()
			.Distinct(new CurrencyEqualityComparer())
			.ToDictionary(k => k.CurrencyIsoCode, e => e);

		CurrencyTable.Add("BTC", new Currency("BitCoin", "", "BTC", 8));
	}
}

The code block below returns only the cultures that have SEK as currency. On different machines (with the same user and CurrentCulture "sv-SE") this is returned in different order.

var cultures = CultureInfo.GetCultures(CultureTypes.AllCultures)
	.Where(valid)
	.Select(c => new Currency(c))
	.Where(c => c.CurrencyIsoCode == "SEK")
	.ToDictionary(k => k.Culture, e => e);

image

The currency is the same, but the native translation is different based on the culture. Will provide a new commit where the CurrentCulture currency is always used regardless of retrieval order.

@microposmp microposmp changed the title Currency name in english Native currency names Feb 9, 2020
@bleroy
Copy link
Member

bleroy commented Feb 10, 2020

Still unclear why this is a problem. Furthermore, the proposed fix also looks arbitrary and unreliable.

Read both native and english currency name from region settings. Make currency from system culture the default if no default is configured.
@microposmp
Copy link
Contributor Author

The problem for me was that I couldn't find Swedish Krona and configure it as default currency. The first loaded culture has a native name that I don't understand for the currency SEK.

Made some more changes to make it more reliable and should now offer a good startup experience in most regions.

  • Changed to use the system CurrentCulture currency as the DefaultCurrency if no default is configured. The first thing I do after enabling the commerce feature is go into settings and change to our local currency. I think it's a good default to use the system CurrentCulture currency as the default. It's still possible to override in commerce settings.
  • Now reads both native and English currency names from system region settings. The English name can be translated via the OrchardCore localization feature.
  • I've not found anywhere the native name is currently shown but kept the logic to replace the first loaded currency where the region has the same currency as the CurrentCulture currency. This is to make the currency name understandable to the user.

@microposmp microposmp mentioned this pull request Feb 10, 2020
2 tasks
Copy link
Member

@bleroy bleroy left a comment

Choose a reason for hiding this comment

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

Making the native name clear sounds good, but I think the default needs to be addressed differently.

Comment on lines 40 to 41
CurrencyTable.Remove(currentCultureCurrency.CurrencyIsoCode);
CurrencyTable.Add(currentCultureCurrency.CurrencyIsoCode, currentCultureCurrency);
Copy link
Member

Choose a reason for hiding this comment

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

I still don't understand how this works. Why would you replace this in a static method? You can't know that what is the current culture when this runs (which is pretty much non-deterministic) is going to remain the current culture. The current culture potentially changes all the time. The whole idea of defaulting to the current culture sounds good on the surface, but I don't think it holds water. The default culture of the site may be a better choice, but even if you do that, you would certainly not modify the table (especially not in a static method: all tenants share that if I'm not mistaken), you'd instead select the correct current culture when it's queried for.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

NativeName is currently never shown so this change is rolled back.

@@ -35,7 +36,7 @@ public ICurrency DefaultCurrency
{
var defaultIsoCode = _options?.DefaultCurrency;
return string.IsNullOrEmpty(defaultIsoCode)
? Currency.USDollar
? new Currency(CultureInfo.CurrentCulture)
Copy link
Member

Choose a reason for hiding this comment

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

See my other comment.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I agree with this. Have rolled back this change.

@bleroy bleroy merged commit 19a799e into OrchardCMS:master Feb 10, 2020
@bleroy
Copy link
Member

bleroy commented Feb 10, 2020

Thanks for the contribution!

@microposmp microposmp deleted the currency-name branch February 11, 2020 15:47
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

2 participants