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

[Tracks] Use invariant locale when converting stat names from the enum types #12987

Merged
merged 1 commit into from
Nov 25, 2024

Conversation

hichamboushaba
Copy link
Member

@hichamboushaba hichamboushaba commented Nov 22, 2024

Closes: #12986

Description

This PR fixes an issue where some Tracks events are ignored when using Turkish language, when an event uses an i character, the cause is that we were using the device Locale when lowercasing from the enum name, but we should be using the invariant locale here.

Steps to reproduce

  1. Switch your phone's language to Turkish.
  2. Open the app.

Testing information

Check logcat and confirm that no EventNameException is thrown.

The tests that have been performed

^

  • I have considered if this change warrants release notes and have added them to RELEASE-NOTES.txt if necessary. Use the "[Internal]" label for non-user-facing changes.

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 big (tablet) and small (phone) in case of UI changes, and no regressions are added.

@hichamboushaba hichamboushaba added type: bug A confirmed bug. category: tracks Related to analytics, including Tracks Events. priority: high Affects lots of customers substantially, but not critically. labels Nov 22, 2024
@hichamboushaba hichamboushaba added this to the 21.2 ❄️ milestone Nov 22, 2024
@hichamboushaba hichamboushaba marked this pull request as ready for review November 22, 2024 11:06
@dangermattic
Copy link
Collaborator

dangermattic commented Nov 22, 2024

1 Warning
⚠️ This PR is assigned to the milestone 21.2 ❄️. The due date for this milestone has already passed.
Please assign it to a milestone with a later deadline or check whether the release for this milestone has already been finished.
1 Message
📖

This PR contains changes to Tracks-related logic. Please ensure (author and reviewer) the following are completed:

  • The tracks events must be validated in the Tracks system.
  • Verify the internal Tracks spreadsheet has also been updated.
  • Please consider registering any new events.
  • The PR must be assigned the category: tracks label.

Generated by 🚫 Danger

@wpmobilebot
Copy link
Collaborator

📲 You can test the changes from this Pull Request in WooCommerce-Wear Android by scanning the QR code below to install the corresponding build.
App Name WooCommerce-Wear Android
Platform⌚️ Wear OS
FlavorJalapeno
Build TypeDebug
Commit5ba58b2
Direct Downloadwoocommerce-wear-prototype-build-pr12987-5ba58b2.apk

@wpmobilebot
Copy link
Collaborator

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

App Name WooCommerce Android
Platform📱 Mobile
FlavorJalapeno
Build TypeDebug
Commit5ba58b2
Direct Downloadwoocommerce-prototype-build-pr12987-5ba58b2.apk

@JorgeMucientes JorgeMucientes self-assigned this Nov 25, 2024
Copy link
Contributor

@JorgeMucientes JorgeMucientes left a comment

Choose a reason for hiding this comment

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

Change makes sense and works as expected. Great job at catching this issue @hichamboushaba.

Out of curiosity. I wonder if instead of relying on Locale.ROOT we could rely better on Locale.ENGLISH since all tracking event names are in English

@hichamboushaba
Copy link
Member Author

Out of curiosity. I wonder if instead of relying on Locale.ROOT we could rely better on Locale.ENGLISH since all tracking event names are in English

I don't have a strong opinion about this, but personally I feel that Locale.ROOT is the better choice for handling locale operations targetted as a computer, vs real locales that should be used when the output is targetted at users.
For this case, our output is targetted at computer, so it made more sense (even though I'm not using it explicitly, Kotlin's extension will use it behind the scenes), C# also has a similar concept with its InvariantCulture, and it's recommended to use it when we need a stable output, even though the same article says that it's associated with the English language.

@hichamboushaba hichamboushaba merged commit d8a7b0d into release/21.2 Nov 25, 2024
23 of 24 checks passed
@hichamboushaba hichamboushaba deleted the fix-tracks branch November 25, 2024 11:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
category: tracks Related to analytics, including Tracks Events. priority: high Affects lots of customers substantially, but not critically. type: bug A confirmed bug.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants