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

Fix amount text field breaking when decimal separator has spaces #14057

Merged

Conversation

staskus
Copy link
Collaborator

@staskus staskus commented Sep 27, 2024

Closes: #14045

Description

The amount of text field breaks when the decimal separator has spaces. It breaks a few text fields - including adding custom amounts and coupons.

image

PriceFieldFormatter and PriceInputFormatter don't take into consideration that the decimal separator has spaces.

Solution

As I tested, even the native number formatter ignores spaces in the decimal separator when converting numbers back to decimal. I think the best option is to ignore spaces in separators when formatting the amount in the text field. It helps avoid issues between number conversions. In labels, we can continue displaying spaces.

The solution is to use decimal and thousands separators without spaces in PriceFieldFormatter and PriceInputFormatter. I also explored a possibility of removing one of these formatters but they have quite a different behavior around the edges so I left it outside the scope.

Steps to reproduce

Coupons

  1. Edit store settings (WooCommerce -> Settings) to have decimal separator with spaces (". ")
  2. Go to Menu
  3. Coupons
  4. Add coupon
  5. Fixed cart discount
  6. try to add a price that has a decimal point
  7. Confirm the value is added as expected

Custom Amount

  1. Edit store settings (WooCommerce -> Settings) to have decimal separator with spaces (". ")
  2. Menu
  3. Payments
  4. Collect Payment
  5. Enter custom amount with decimal values
  6. Confirm the value is added as expected

Testing information

  • Added unit tests for these cases.
  • Additionally tested other price fields (Product Creation, Discounts)
  • Tested spaces in thousands separator since this is a more common case

Screenshots

Custom Amount - Decimals

Custom.Amount.-.Decimals.mp4

Coupon - Decimals

Coupon.-.Decimals.mp4

Coupon - Decimal and Thousands

Coupon.-.Decimal.and.Thousands.mp4

  • I have considered if this change warrants user-facing release notes and have added them to RELEASE-NOTES.txt if necessary.

Reviewer (or Author, in the case of optional code reviews):

Please make sure these conditions are met before approving the PR, or request changes if the PR needs improvement:

  • The PR is small and has a clear, single focus, or a valid explanation is provided in the description. If needed, please request to split it into smaller PRs.
  • Ensure Adequate Unit Test Coverage: The changes are reasonably covered by unit tests or an explanation is provided in the PR description.
  • Manual Testing: The author listed all the tests they ran, including smoke tests when needed (e.g., for refactorings). The reviewer confirmed that the PR works as expected on all devices (phone/tablet) and no regressions are added.

@staskus staskus added type: bug A confirmed bug. feature: coupons Related to basic fulfillment such as order tracking. feature: order creation All tasks related to creating an order labels Sep 27, 2024
@staskus staskus added this to the 20.7 milestone Sep 27, 2024
@staskus staskus changed the title Amount text field breaks when decimal separator has spaces Fix amount text field breaking when decimal separator has spaces Sep 27, 2024
@wpmobilebot
Copy link
Collaborator

WooCommerce iOS📲 You can test the changes from this Pull Request in WooCommerce iOS by scanning the QR code below to install the corresponding build.

App NameWooCommerce iOS WooCommerce iOS
Build Numberpr14057-7ca9a81
Version20.5
Bundle IDcom.automattic.alpha.woocommerce
Commit7ca9a81
App Center BuildWooCommerce - Prototype Builds #11036
Automatticians: You can use our internal self-serve MC tool to give yourself access to App Center if needed.

Copy link
Contributor

@iamgabrielma iamgabrielma left a comment

Choose a reason for hiding this comment

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

Works nicely, thanks for addressing this! I found some oddities when testing, but nothing with priority or in the scope of this PR, I've reported them separately:

I'll go ahead and merge the changes. Left a few comments, but feel free to address them when you're back from AFK 🙇

Comment on lines +12 to +14
var sanitizedGroupingSeparator: String {
return groupingSeparator.replacingOccurrences(of: " ", with: "")
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
var sanitizedGroupingSeparator: String {
return groupingSeparator.replacingOccurrences(of: " ", with: "")
}

This one is currently not used anywhere, so perhaps we want to remove it? If we don't, maybe we should rename it from sanitizedGroupingSeparator to sanitizedThousandSeparator?

The CodingKey in the CurrencySettings doesn't seem to follow the decoding pattern for this value: If we check the Networking.SystemStatus+Settings it's initially decoded as case thousandSeparator = "thousand_separator", but at some point we started to name it "groupingSeparator" in CurrencySettings (perhaps because of the native NumberFormatter?) so perhaps makes sense to refactor the name here and there for consistency, wdyt?

@@ -51,7 +51,7 @@ struct PriceInputFormatter: UnitInputFormatter {

let formattedText = latinText
// Replace any characters not in the set of 0-9 or minus symbol with the current decimal separator configured on website
.replacingOccurrences(of: "[^0-9-]", with: currencySettings.decimalSeparator, options: .regularExpression)
.replacingOccurrences(of: "[^0-9-]", with: currencySettings.sanitizedDecimalSeparator, options: .regularExpression)
Copy link
Contributor

Choose a reason for hiding this comment

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

💯

Comment on lines +159 to +163
// When
let input = "189.20"

// Then formatting ignores spaces in store decimal separator settings
XCTAssertEqual(formatter.format(input: input), "189.20")
Copy link
Contributor

Choose a reason for hiding this comment

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

We're not testing for space sanitization here, are we? If the input simulates what the merchant introduces in the textfield, and then this input is sanitized within formatter.format(value), shouldn't we pass the value in as malformed to the formatter? (The test still succeeds)

Suggested change
// When
let input = "189.20"
// Then formatting ignores spaces in store decimal separator settings
XCTAssertEqual(formatter.format(input: input), "189.20")
// When
let input = "189. 20"
// Then formatting ignores spaces in store decimal separator settings
XCTAssertEqual(formatter.format(input: input), "189.20")

@iamgabrielma iamgabrielma merged commit c8aa730 into trunk Sep 30, 2024
24 of 27 checks passed
@iamgabrielma iamgabrielma deleted the fix/14045-decimals-with-spaces-break-amount-text-field branch September 30, 2024 09:18
@staskus
Copy link
Collaborator Author

staskus commented Oct 15, 2024

@iamgabrielma thanks for taking care of this - reviewing, creating additional tasks, and merging!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature: coupons Related to basic fulfillment such as order tracking. feature: order creation All tasks related to creating an order type: bug A confirmed bug.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Adding price decimal adds white space before decimal point
3 participants