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

Currency validation support #4430

Closed
wants to merge 2 commits into from
Closed

Conversation

olegL1337
Copy link

Add Currency format to TextBoxExtensions #4188

#4188
So I added Currency option to ValidationType enum and modified ValidateTextBox metod with adding new option to switch-case clause. Because I could not modify StringExtensions to add isCurrency mehtod, I decided to simply add modifications in such switch-case clause.

Regex pattern I used: @"(^\d*\.\d{2}$)"
Mathes: $100.00, $100, $10.25
Non-Matches: 100., $10.233, $10.

  • Tested code with current supported SDKs
  • New component
    • Pull Request has been submitted to the documentation repository instructions. Link:
    • Added description of major feature to project description for NuGet package (4000 total character limit, so don't push entire description over that)
    • If control, added to Visual Studio Design project
  • Sample in sample app has been added / updated (for bug fixes / features)
  • New major technical changes in the toolkit have or will be added to the Wiki e.g. build changes, source generators, testing infrastructure, sample creation changes, etc...
  • Tests for the changes have been added (for bug fixes / features) (if applicable)
  • Header has been added to all new source files (run build/UpdateHeaders.bat)
  • Contains NO breaking changes

@ghost
Copy link

ghost commented Dec 16, 2021

Thanks olegL1337 for opening a Pull Request! The reviewers will test the PR and highlight if there is any conflict or changes required. If the PR is approved we will proceed to merge the pull request 🙌

@ghost ghost requested review from michael-hawker and azchohfi December 16, 2021 12:03
@net-foundation-cla
Copy link

net-foundation-cla bot commented Dec 16, 2021

CLA assistant check
All CLA requirements met.

@net-foundation-cla
Copy link

CLA assistant check
Thank you for your submission, we really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.

❌ olegL1337 sign now
You have signed the CLA already but the status is still pending? Let us recheck it.

@Arlodotexe Arlodotexe self-requested a review December 16, 2021 17:18
// @"(^\d*\.\d{2}$)" regex pattern to detect currency sign with currency value
// Mathes: $100.00, $100, $10.25
// Non-Matches: 100., $10.233, $10.
regexMatch = Regex.IsMatch(textBox.Text, @"(^\d*\.\d{2}$)");
Copy link
Member

@Arlodotexe Arlodotexe Dec 16, 2021

Choose a reason for hiding this comment

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

We should handle currency symbols and decimal separators other than USD. Luckily, dotnet has this functionality built in and we don't need to fiddle with Regex.

System.Globalization.NumberFormatInfo should have everything we need for this.

Firing up an empty console app, here's what we have to work with.

string FormatCurrency(string value)
{
    // Parse the entered data into a decimal, allowing existing currency symbols.
    decimal.TryParse(value, NumberStyles.Currency, out var amount);

    // Format the decimal into a human readable string for the current culture.
    return amount.ToString("C2", CultureInfo.CurrentCulture);
}

// Stress testing USD
FormatCurrency("500"); // $500.00
FormatCurrency("5"); // $5.00
FormatCurrency("5.00"); // $5.00
FormatCurrency(".05"); // $0.05
FormatCurrency(".5"); // $0.50
FormatCurrency("$.5"); // $0.50

// With other cultures
FormatCurrency("5"); // nl-NL, € 5,00
FormatCurrency("5"); // fr-FR, 5,00 €
FormatCurrency("5"); // ja-JP, ¥5.00
FormatCurrency("5"); // en-GB, £5.00

Copy link
Member

@michael-hawker michael-hawker left a comment

Choose a reason for hiding this comment

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

Thanks @olegL1337 for the PR. Agree with @Arlodotexe that we want to make sure to handle localized scenarios. It'd be good to put some localized examples in some unit tests to make sure we guard against changes in the future as well.

Also FYI @NaftoliOst who was interested in helping with this feature too in case they want to review or have any suggestions.

Comment on lines +56 to +60
/// <summary>
/// Currency validation
/// </summary>
Currency,

Copy link
Member

Choose a reason for hiding this comment

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

Should be added at the end so not a breaking change.

@NaftoliOst
Copy link

Yes, I wrote the code for this, but didn't get around to testing it, which is why I haven't submitted a PR.

This is the Regex I used to match all currencies:

+        /// <summary>
+        /// Regular expression for matching a decimal with or without a currency character.
+        /// </summary>
+        internal const string CurrencyRegex = @"^\p{Sc}?\d+\.?\d*$";

@michael-hawker
Copy link
Member

Thanks for chiming in @NaftoliOst! I think as @Arlodotexe called out, if there's actual .NET APIs which can do this validation without RegEx, it'd be great to try using those instead and help guard us against localization/regional issues.

@Arlodotexe
Copy link
Member

Arlodotexe commented Dec 17, 2021

Regex is a tricky thing to globalize. Not all currencies use a decimal as their separator (if any separator), many place the currency symbol in different places with different spacing.

Since we're just checking if a string is valid as a currency, we don't need to use regex. Try this:

    // Parse the entered data into a decimal, allowing existing currency symbols. Uses current UI culture.
    regexMatch = decimal.TryParse(value, NumberStyles.Currency, out _);

May also need to rename the regexMatch bool to isValid or something :)

edit: @michael-hawker beat me to it while I was spell checking 😄

@NaftoliOst
Copy link

I wasn't aware of the .NET APIs for currency validation with localized formats, thanks for pointing that out @michael-hawker and @Arlodotexe. It definitely make more sense to make use of that ready built method that takes into account local formats and has stood the test of time.

I would also suggest putting the code in an IsCurrency extension method in the StringExtensions.cs file and then call that method here in the TextBoxExtensions.Regex.Internals.cs file
regexMatch = textBox.Text.IsCurrency();

That would be consistent with the other validation checks ( IsDecimal() IsPhoneNumber() etc.) and allows the IsCurrency method to be used in future for other use cases, should the need arise, without having to rewrite the code.

// @"(^\d*\.\d{2}$)" regex pattern to detect currency sign with currency value
// Mathes: $100.00, $100, $10.25
// Non-Matches: 100., $10.233, $10.
regexMatch = Regex.IsMatch(textBox.Text, @"^\$( )*\d*(.\d{1,2})?$");
Copy link
Member

Choose a reason for hiding this comment

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

Until we can reference this new extension method, let's use the .NET Api directly so we can close this off :)

Suggested change
regexMatch = Regex.IsMatch(textBox.Text, @"^\$( )*\d*(.\d{1,2})?$");
// Parse the entered data into a decimal, allowing existing currency symbols. Uses current UI culture.
regexMatch = decimal.TryParse(value, NumberStyles.Currency, out _);

Choose a reason for hiding this comment

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

The TryParse method doesn't accept 3 parameters, there needs to be an IFormatProvider parameter as well.

I've written the code for this with CultureInfo.CurrentCulture as the IFormatProvider like you did in the PR for the new extension method (unless @olegL1337 is still planning on updating this PR)

My only question is, it this the best way to validate the currencies? with the IFormatProvider set to CultureInfo.CurrentCulture the validation will only pass for the user's local currency. Is there a way of including all currency cultures so that any currency will pass as valid?

Copy link
Member

Choose a reason for hiding this comment

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

Afaik, there isn't a CultureInfo that represents "all" cultures. However, we can get all available cultures and check against each one separately.

static decimal? TryParseCurrency(string str)
{
	foreach (var culture in CultureInfo.GetCultures(CultureTypes.AllCultures))
	{
		if (decimal.TryParse(str, NumberStyles.Currency, culture, out var amount))
				return amount;
	}

	return null;
}

See it in action: https://dotnetfiddle.net/HW8wZ4

Choose a reason for hiding this comment

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

Got it!
So do you think it would be better to implement it like that? The trade off would be the efficiency, particularly when it needs to loop though all of the cultures at each keystroke of user input. Is that something you think we should be concerned about @Arlodotexe?

BTW I've submitted a PR here that closes this issue if @olegL1337 isn't planning on going further with this PR

Copy link
Member

Choose a reason for hiding this comment

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

This feels this needs more options.

Perhaps we add a way to provide the current or a specific culture for ValidationType.Currency, and add an additional ValidationType.AnyCurrency if they'd like to check all cultures. @michael-hawker what do you think?

@michael-hawker
Copy link
Member

@olegL1337 going to close this PR as we haven't heard from you and it has been picked up by @NaftoliOst now. Thanks! See #4469 for now.

@michael-hawker michael-hawker removed this from the 8.0 milestone Feb 3, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants