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] Improve locale consistency #4158

Merged
merged 57 commits into from
Aug 20, 2024

Conversation

joshdholtz
Copy link
Member

@joshdholtz joshdholtz commented Aug 8, 2024

⚠️ Waiting for #4188 to be merged first (because needs all the tests to pass)

Motivation

Fix cases where paywall was showing multiple languages at once and random languages.

Description

👉 For a demo of the problem and solution, look in Slack in the paywalls or SDK channel for the latest Loom that I posted

Flow of paywall localization logic (since it can be confusing)

  1. PaywallView starts with Locale.current but then calls offering.validatedPaywall() which will return the new displayedLocale: Locale variable
  2. PaywallData+Validation returns displayedLocale if its a valid paywall, otherwise it return Locale.current for fallback paywall
  3. PaywallData+Localization has a lot of the new logic
    • New locale: Locale computer property that it determines the locale from the localizedConfiguration or localizedConfigurationsByTier
    • New defaultLocalizedConfiguration that uses the developer set defaultLocale (from PaywallData) to find the default localization
    • Updated localizedConfiguration<Value>() to take a new defaultLocalization to put in its priority (and log it if its used). If for some reason that's not found, it will use the legacy fallbackLocalization (which is the first localization it finds or english)
  4. After all of that, the displayedLocale is passed into the actually SwiftUI LoadedOfferingPaywallView to show this locale everywhere

Problem 1 - Inconsistent languages displayed at once

The user defined localizations and the pre-localized content in the SDK (like links in the footer and package duration names) could should different localizations. The choice of which locale was happening in two different spots.

Since the SDK has all possible localizations that the dashboard offers, the locale SDK displays from the dashboard will be chosen for the pre-localized content.

Problem 2 - Showing random languages if not supported

We decided early on to not use a default_locale but that has hurt us. The solution to use was use localizations.first to show the first locale in the list and the intention that this was the first locale the user did in the paywall. That was not the case, however. The order of the localizations list was indeterministic so it should should a random language (even to the point where different random languages would show the the same user).

We are now introducing a default_locale that the developer can set in the paywall. If the locale the user is requesting is not translated by the developer, it will use the default_locale

Problem 3 - Footer not using correct localization from the bundle

There were times where some parts of the footer itself (mainly the restore button) would be a different language than the terms of service and privacy policy buttons. This was due to a code issue where we were using Bundle.module. Instead, we needed to specify a localization bundle specifically that we wanted to use.

@joshdholtz joshdholtz added the pr:fix A bug fix label Aug 8, 2024
@joshdholtz joshdholtz requested review from a team August 8, 2024 16:55
@joshdholtz joshdholtz force-pushed the paywall-locale-choosing-improvements branch from f6eeca4 to d330125 Compare August 8, 2024 16:58
@joshdholtz joshdholtz marked this pull request as ready for review August 8, 2024 16:58
locale: Locale = .current
locale: Locale,
Copy link
Member Author

Choose a reason for hiding this comment

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

  1. Defaulting to .current is hard to traceback for decision making and debugging to getting rid of it

if let paywall = self.paywall {
switch paywall.validate() {
case let .success(template):
return (paywall, template, nil)
return (paywall, paywall.locale ?? locale, template, nil)
Copy link
Member Author

Choose a reason for hiding this comment

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

paywall.locale SHOULD always exist at this point but needed to do something like this to make it happy 🤷‍♂️

Comment on lines -123 to +134
Text("All subscriptions", bundle: .module)
Text("All subscriptions", bundle: bundle)
Copy link
Member Author

Choose a reason for hiding this comment

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

Things like this were the cause of not putting correct localizations and so needing to "add" the localizations to Xcode

Comment on lines 19 to 24
var locale: Locale? {
let singleTier = self.localizedConfiguration(for: Self.localesOrderedByPriority)?.0
let multiTier = self.localizedConfigurationByTier(for: Self.localesOrderedByPriority)?.0

return singleTier ?? multiTier
}
Copy link
Member Author

Choose a reason for hiding this comment

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

This is slightly gross but it gets the job done 🤷‍♂️

Copy link
Member Author

Choose a reason for hiding this comment

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

One thing I don't love here is how all of these variables are computed 😬 If they are called multiple times, it reruns all of this logic again. I don't really feel comfortable changing all of that in this PR yet

Copy link
Contributor

Choose a reason for hiding this comment

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

I guess the computed properties might support changing localizations on the fly. Not sure how often that happens or if it's even a use case worth supporting though.

Copy link
Contributor

@jamesrb1 jamesrb1 left a comment

Choose a reason for hiding this comment

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

Very nice!!

table: nil)
}

// Returns the localized string
return value(
locale: locale,
// Or defaults to english
default: value(locale: Self.defaultLocale, default: nil)
// Or falls back to first localization or english
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
// Or falls back to first localization or english
// Or falls back to first localization, and if unavailable, english

@@ -29,7 +30,8 @@ struct PaywallViewConfiguration {
fonts: PaywallFontProvider = DefaultPaywallFontProvider(),
displayCloseButton: Bool = false,
introEligibility: TrialOrIntroEligibilityChecker? = nil,
purchaseHandler: PurchaseHandler
purchaseHandler: PurchaseHandler,
locale: Locale = .current
Copy link
Contributor

Choose a reason for hiding this comment

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

Based on your loom, is a default of .current safe?

Copy link
Member Author

Choose a reason for hiding this comment

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

That's a fair point! I removed it 😊

internal func localizedConfiguration(for preferredLocales: [Locale]) -> LocalizedConfiguration? {
internal func localizedConfiguration(
for preferredLocales: [Locale]
) -> (Locale, LocalizedConfiguration)? {
Copy link
Contributor

@jamesrb1 jamesrb1 Aug 9, 2024

Choose a reason for hiding this comment

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

Consider labeling the tuple parameters so that we can refer to them by name rather than order (ie instead of ?.0, ?.locale).

Suggested change
) -> (Locale, LocalizedConfiguration)? {
) -> (locale: Locale, localizedConfiguration: LocalizedConfiguration)? {

@@ -37,6 +37,9 @@ public struct PaywallData {
set { self._revision = newValue }
}

/// The default localue identifier for this paywall.
Copy link
Contributor

Choose a reason for hiding this comment

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

typo

Suggested change
/// The default localue identifier for this paywall.
/// The default locale identifier for this paywall.

Copy link
Contributor

Choose a reason for hiding this comment

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

I guess the computed properties might support changing localizations on the fly. Not sure how often that happens or if it's even a use case worth supporting though.

@@ -44,7 +44,8 @@ struct LoadingPaywallView: View {
fonts: DefaultPaywallFontProvider(),
displayCloseButton: self.displayCloseButton,
introEligibility: Self.introEligibility,
purchaseHandler: Self.purchaseHandler
purchaseHandler: Self.purchaseHandler,
locale: Locale.current
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we be defaulting to Locale.preferredLocales.first ?? Locale.current? (access restrictions aside.)

Copy link
Member Author

Choose a reason for hiding this comment

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

Ah, probably! The loading view is all skeleton stuff anyway so there isn't actually any text anywhere but that does feel better 😇

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.

Not an expert, but this makes sense. Great fixes!

@joshdholtz joshdholtz force-pushed the paywall-locale-choosing-improvements branch from 4349304 to faa499e Compare August 15, 2024 12:50
@joshdholtz
Copy link
Member Author

@RCGitBot please test

@joshdholtz joshdholtz force-pushed the paywall-locale-choosing-improvements branch from b755f25 to dfc4342 Compare August 19, 2024 03:15
@joshdholtz
Copy link
Member Author

@RCGitBot please test

Base automatically changed from fix-failing-tests-that-rcbot-does to main August 20, 2024 12:44
@joshdholtz
Copy link
Member Author

@RCGitBot please test

@joshdholtz joshdholtz merged commit 1fa69d4 into main Aug 20, 2024
33 checks passed
@joshdholtz joshdholtz deleted the paywall-locale-choosing-improvements branch August 20, 2024 14:44
joshdholtz added a commit that referenced this pull request Aug 22, 2024
**This is an automatic release.**

### New Features
* Price rounding logic (#4132) via James Borthwick (@jamesrb1)
### Bugfixes
* [Customer Center] Migrate to List style (#4190) via Cody Kerns
(@codykerns)
* [Paywalls] Improve locale consistency (#4158) via Josh Holtz
(@joshdholtz)
* Set Paywalls Tester deployment target to iOS 15 (#4196) via James
Borthwick (@jamesrb1)
* [Customer Center] Hide Contact Support button if URL can't be created
(#4192) via Cesar de la Vega (@vegaro)
* Fix the setting for SKIP_INSTALL in Xcode project (#4195) via Andy
Boedo (@aboedo)
* [Customer Center] Improving customer center buttons (#4165) via Cody
Kerns (@codykerns)
* Revert workaround for iOS 18 beta 5 SwiftUI crash (#4173) via Mark
Villacampa (@MarkVillacampa)
* [Paywalls] Make iOS version calculation lazy (#4163) via Mark
Villacampa (@MarkVillacampa)
* [Paywalls] Observe `PurchaseHandler` when owned externally (#4097) via
James Borthwick (@jamesrb1)
### Other Changes
* [Customer Center] Clean up colors in WrongPlatformView and
NoSubscriptionsView (#4204) via Cesar de la Vega (@vegaro)
* Fix failing `all-tests` and retry more flaky tests (#4188) via Josh
Holtz (@joshdholtz)
* Compatibility content unavailable improvements (#4197) via James
Borthwick (@jamesrb1)
* Create lane to enable customer center (#4191) via Cesar de la Vega
(@vegaro)
* XCFramework artifacts in CircleCI (#4189) via Andy Boedo (@aboedo)
* [Customer Center] CustomerCenterViewModel checks whether the app is
the latest version (#4169) via JayShortway (@JayShortway)
* export RevenueCatUI xcframework (#4172) via Andy Boedo (@aboedo)
* Corrects references from ManageSubscriptionsButtonStyle to
ButtonsStyle. (#4186) via JayShortway (@JayShortway)
* Speed up carthage installation tests (#4184) via Andy Boedo (@aboedo)
* Customer center improvements (#4166) via James Borthwick (@jamesrb1)
* replace `color(from colorInformation:)` global with extension (#4183)
via Andy Boedo (@aboedo)
* Fix tests in main (#4174) via Andy Boedo (@aboedo)
* Enable customer center tests (#4171) via James Borthwick (@jamesrb1)
* [Customer Center] Initial implementation (#3967) via Cesar de la Vega
(@vegaro)

---------

Co-authored-by: Josh Holtz <me@joshholtz.com>
nyeu pushed a commit that referenced this pull request Oct 2, 2024
⚠️ Waiting for #4188 to be merged first (because needs all the tests to
pass)

## Motivation

Fix cases where paywall was showing multiple languages at once and
random languages.

## Description

👉 For a demo of the problem and solution, look in Slack in the paywalls
or SDK channel for the latest Loom that I posted

### Flow of paywall localization logic (since it can be confusing)

1.
[PaywallView](https://github.com/RevenueCat/purchases-ios/pull/4158/files#diff-ea75fe7280803e15b975d702a16c6851ffc2bbed01b7b196bf0fb80d75b4b332)
starts with `Locale.current` but then calls
`offering.validatedPaywall()` which will return the new
`displayedLocale: Locale` variable
2.
[PaywallData+Validation](https://github.com/RevenueCat/purchases-ios/pull/4158/files#diff-452790cf5287a44f75a4b2260254db2281a066faeea2b3e441a007ec60522b6b)
returns `displayedLocale` if its a valid paywall, otherwise it return
`Locale.current` for fallback paywall
3.
[PaywallData+Localization](https://github.com/RevenueCat/purchases-ios/pull/4158/files#diff-e620baead94f7acad56d931c485d7691c4f909be3fe625e104528a2702cca2a0)
has a lot of the new logic
- New `locale: Locale` computer property that it determines the locale
from the `localizedConfiguration` or `localizedConfigurationsByTier`
- New ` defaultLocalizedConfiguration` that uses the developer set
`defaultLocale` (from `PaywallData`) to find the default localization
- Updated `localizedConfiguration<Value>()` to take a new
`defaultLocalization` to put in its priority (and log it if its used).
If for some reason that's not found, it will use the legacy
`fallbackLocalization` (which is the first localization it finds or
english)
4. After all of that, the `displayedLocale` is passed into the actually
SwiftUI `LoadedOfferingPaywallView` to show this locale everywhere

### Problem 1 - Inconsistent languages displayed at once

The user defined localizations and the pre-localized content in the SDK
(like links in the footer and package duration names) could should
different localizations. The choice of which locale was happening in two
different spots.

Since the SDK has all possible localizations that the dashboard offers,
the locale SDK displays from the dashboard will be chosen for the
pre-localized content.

### Problem 2 - Showing random languages if not supported

We decided early on to not use a `default_locale` but that has hurt us.
The solution to use was use `localizations.first` to show the first
locale in the list and the intention that this was the first locale the
user did in the paywall. That was not the case, however. The order of
the localizations list was indeterministic so it should should a random
language (even to the point where different random languages would show
the the same user).

We are now introducing a `default_locale` that the developer can set in
the paywall. If the locale the user is requesting is not translated by
the developer, it will use the `default_locale`

### Problem 3 - Footer not using correct localization from the bundle

There were times where some parts of the footer itself (mainly the
restore button) would be a different language than the terms of service
and privacy policy buttons. This was due to a code issue where we were
using `Bundle.module`. Instead, we needed to specify a localization
bundle specifically that we wanted to use.
nyeu pushed a commit that referenced this pull request Oct 2, 2024
**This is an automatic release.**

### New Features
* Price rounding logic (#4132) via James Borthwick (@jamesrb1)
### Bugfixes
* [Customer Center] Migrate to List style (#4190) via Cody Kerns
(@codykerns)
* [Paywalls] Improve locale consistency (#4158) via Josh Holtz
(@joshdholtz)
* Set Paywalls Tester deployment target to iOS 15 (#4196) via James
Borthwick (@jamesrb1)
* [Customer Center] Hide Contact Support button if URL can't be created
(#4192) via Cesar de la Vega (@vegaro)
* Fix the setting for SKIP_INSTALL in Xcode project (#4195) via Andy
Boedo (@aboedo)
* [Customer Center] Improving customer center buttons (#4165) via Cody
Kerns (@codykerns)
* Revert workaround for iOS 18 beta 5 SwiftUI crash (#4173) via Mark
Villacampa (@MarkVillacampa)
* [Paywalls] Make iOS version calculation lazy (#4163) via Mark
Villacampa (@MarkVillacampa)
* [Paywalls] Observe `PurchaseHandler` when owned externally (#4097) via
James Borthwick (@jamesrb1)
### Other Changes
* [Customer Center] Clean up colors in WrongPlatformView and
NoSubscriptionsView (#4204) via Cesar de la Vega (@vegaro)
* Fix failing `all-tests` and retry more flaky tests (#4188) via Josh
Holtz (@joshdholtz)
* Compatibility content unavailable improvements (#4197) via James
Borthwick (@jamesrb1)
* Create lane to enable customer center (#4191) via Cesar de la Vega
(@vegaro)
* XCFramework artifacts in CircleCI (#4189) via Andy Boedo (@aboedo)
* [Customer Center] CustomerCenterViewModel checks whether the app is
the latest version (#4169) via JayShortway (@JayShortway)
* export RevenueCatUI xcframework (#4172) via Andy Boedo (@aboedo)
* Corrects references from ManageSubscriptionsButtonStyle to
ButtonsStyle. (#4186) via JayShortway (@JayShortway)
* Speed up carthage installation tests (#4184) via Andy Boedo (@aboedo)
* Customer center improvements (#4166) via James Borthwick (@jamesrb1)
* replace `color(from colorInformation:)` global with extension (#4183)
via Andy Boedo (@aboedo)
* Fix tests in main (#4174) via Andy Boedo (@aboedo)
* Enable customer center tests (#4171) via James Borthwick (@jamesrb1)
* [Customer Center] Initial implementation (#3967) via Cesar de la Vega
(@vegaro)

---------

Co-authored-by: Josh Holtz <me@joshholtz.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
pr:fix A bug fix
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants