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

Paywalls: fix finding locales with different regions #3633

Merged
merged 3 commits into from
Feb 5, 2024

Conversation

NachoSoto
Copy link
Contributor

@NachoSoto NachoSoto commented Feb 5, 2024

This was a regression introduced by #3605.

Example:

Consider a paywall configuration with these locales: en_US, de_DE.
Running on a device with locale en_IN for example.

Before #3605, PaywallData.localizedConfiguration would have returned the en_US localization. However, that change made it so that it could no longer find a matching localization with a different region.

This fix improves the locale lookup so that if it doesn't find a matching localization with both language AND region, it then tries to look it up with only the language, before falling back to the default localization.

This was a regression introduced by #3605.

### Example:

Consider a paywall configuration with these locales: `en_US`, `de_DE`.
Running on a device with locale `en_IN` for example.

Before #3605, `PaywallData.localizedConfiguration` would have returned the `en_US` localization. However, that change made it so that it could no longer find a matching localization with a different region.'

This fix improves the locale lookup so that if it doesn't find a matching localization with both language AND region, it then tries to look it up with only the language, before falling back to the default localization.
@NachoSoto NachoSoto requested review from a team February 5, 2024 19:17
var result = [.current] + Locale.preferredLocales

if let withoutRegion = Locale.current.removingRegion {
result.append(withoutRegion)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is the fix

expect(enConfig.title) == "Paywall"
}

func testLocalesOrderedByPriority() {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Now we also have a test for this logic

func testLocalizedConfigurationFallsBackToLanguageWithDifferentRegion() throws {
let paywall: PaywallData = try self.decodeFixture("PaywallData-Sample1")

let enConfig = try XCTUnwrap(paywall.localizedConfiguration(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.

And also have a test for this method, which we didn't have before.

let enConfig = try XCTUnwrap(paywall.localizedConfiguration(for: [
.init(identifier: "en_IN"),
.init(identifier: "en-IN"),
.init(identifier: "en-IN").removingRegion
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The fact that this is included in Paywall.localizedConfiguration is tested below.

.init(identifier: "en-IN"),
.init(identifier: "en-IN").removingRegion
].compactMap { $0 }))
expect(enConfig.title) == "Paywall"
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This was a failing test before the fix.

Copy link
Contributor

@tonidero tonidero left a comment

Choose a reason for hiding this comment

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

LGTM!

Copy link
Contributor

@joshdholtz joshdholtz left a comment

Choose a reason for hiding this comment

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

Ship ship!

@NachoSoto NachoSoto enabled auto-merge (squash) February 5, 2024 22:34
NachoSoto added a commit to RevenueCat/purchases-android that referenced this pull request Feb 5, 2024
Test coverage to ensure that Android isn't affected by RevenueCat/purchases-ios#3633.
I've also extracted `getDefaultLocales` and  `localizedConfiguration(List<Locale>)` so we can test those.
@NachoSoto NachoSoto merged commit 6e98400 into main Feb 5, 2024
24 checks passed
@NachoSoto NachoSoto deleted the paywalls-language-bug branch February 5, 2024 23:26
NachoSoto added a commit to RevenueCat/purchases-android that referenced this pull request Feb 6, 2024
Test coverage to ensure that Android isn't affected by
RevenueCat/purchases-ios#3633.

I've also extracted `getDefaultLocales` and
`localizedConfiguration(List<Locale>)` so we can test those.
@samkass
Copy link

samkass commented Feb 7, 2024

After upgrading to this version, I still see this problem. RevenueCat only allows German localization for "German (Germany)" and my iOS application (and my iPhone's settings) is localized for "German". Even after 4.36.0 is applied, I am seeing the English Paywall when my phone is set to "German". When I set the phone to "German (Germany)" I see German. When I set it to "German (Switzerland)" I also see English. I sent an extensive steps to reproduce, expected and observed behavior, and test cases via email attached to my support ticket on this topic.

NachoSoto added a commit that referenced this pull request Feb 7, 2024
Will make debugging issues like #3633 and #3633 (comment) easier.

```
VERBOSE: Looking up localized configuration for ["de_SW", "de"]
VERBOSE: Found localized configuration for 'de'
```
@NachoSoto
Copy link
Contributor Author

Sorry this is causing you trouble. I haven't been able to reproduce that exact setup, it uses the german localization for me with de and de_DE.

Do you mind filing a separate GitHub issue and providing more details to help us look into it? Specifically wondering what iOS version you're using.

I've also sent #3649 which adds some verbose logs to help us diagnose it. Would you be able to run the app using that branch? Make sure you set Purchases.logLevel = .verbose.

Thanks 👍🏻

NachoSoto added a commit that referenced this pull request Feb 7, 2024
Will make debugging issues like #3633 and
#3633 (comment)
easier.

```
VERBOSE: Looking up localized configuration for ["de_CH", "de"]
VERBOSE: Found localized configuration for 'de'
```
@samkass
Copy link

samkass commented Feb 7, 2024

Sorry this is causing you trouble. I haven't been able to reproduce that exact setup, it uses the german localization for me with de and de_DE.

Do you mind filing a separate GitHub issue and providing more details to help us look into it? Specifically wondering what iOS version you're using.

I've also sent #3649 which adds some verbose logs to help us diagnose it. Would you be able to run the app using that branch? Make sure you set Purchases.logLevel = .verbose.

Thanks 👍🏻

Added #3655. To your specific question, I'm using the latest iOS 17.2.1. In the Simulator, this happens on iOS 17.x and 16.x, but not 15.x.

NachoSoto added a commit that referenced this pull request Feb 7, 2024
Fixes #3655.
Follow up to #3633.

This improves upon that PR by using `Locale.current.removingRegion` **before** trying to look up `Locale.preferredLocales`.

This fixes this scenario:
- Configure your phone with: `en_UK` and `es_ES`
- Launch paywall with localization `en_US` and `es_ES`

Prior to this change, we'd be looking up localizations in this order:
- `en_UK`
- `es_ES`
- `en` (no region)

Because there _is_ a localization for `es_ES` we'd return that one before attempting to look up `Locale.current.removingRegion`.
NachoSoto added a commit that referenced this pull request Feb 8, 2024
#3657)

Fixes #3655.
Follow up to #3633.

This improves upon that PR by using `Locale.current.removingRegion` for
each locale **before** trying to look up `Locale.preferredLocales`.

This fixes this scenario:
- Configure your phone with: `en_IN` and `es_ES`
- Launch paywall with localization `en_US` and `es_ES`

Prior to this change, we'd be looking up localizations in this order:
- `en_IN`
- `es_ES`
- `en` (no region)

Because there _is_ a localization for `es_ES` we'd return that one
before attempting to look up `Locale.current.removingRegion`.

Example of the new log:
> VERBOSE: Looking up localized configuration for ["en_IN", "es_ES"],
searching for ["en_IN", "en", "es_ES", "es"]
> VERBOSE: Found localized configuration for 'en'
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants