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

Settings Sync - Refactor archive settings to be UserSettings #1234

Conversation

mchowning
Copy link
Contributor

@mchowning mchowning commented Aug 3, 2023

Description

This updates the archive settings to be user settings.

In doing this, I found that we were persisting some of the archive settings as localized strings. Not surprisingly, this can cause some issues when a user changes the language on their phone. This PR fixes this by migrating the settings to a new key in shared preferences that is no longer tied to the app's language.

I also updated our support logs so that the auto archive settings are no longer displayed with a localized string.

Testing Instructions

1. General refactor testing

For each setting in the "Auto Archive" section:

  1. Fresh install the app
  2. Verify that the setting has the same default value that it had before
  3. Change the setting to a new value
  4. Force close the app
  5. Verify that the setting has retained the new value
  6. Verify that the app's behavior reflects the updated setting value.

2. Language change bug fix

  1. Fresh install the app
  2. Go the Auto Archive settings
  3. Change the "Archive played episodes" and "Archive inactive episodes" settings to a non-default value
  4. Change the language of your phone to French
  5. Return to the Pocket Casts app
  6. Wait for the Pocket Casts app to update to the new language
  7. Confirm that the non-default settings are still visible on the Auto Archive settings screen (without this PR fix, the previously set settings would disappear).

3. Migration of archive settings

  1. Fresh install a build from main
  2. Change the "Archive played episodes" and "Archive inactive episodes" settings to a non-default value
  3. Install a build from this PR
  4. Verify that your updates to those settings are retained

4. Support logs no longer use localized strings for archive settings

  1. Change your phone's language to French
  2. View the app logs (Profile → "Help & feedback" → logs icon in the toolbar)
  3. Verify that the values for "Auto archive played episodes after" and "Auto archive inactive episodes after" are not in French.

Checklist

  • If this is a user-facing change, I have added an entry in CHANGELOG.md
  • Ensure the linter passes (./gradlew spotlessApply to automatically apply formatting/linting)
  • I have considered whether it makes sense to add tests for my changes
  • All strings that need to be localized are in modules/services/localization/src/main/res/values/strings.xml
  • Any jetpack compose components I added or changed are covered by compose previews

@@ -1,3 +1,9 @@
Unknown
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I know this won't be in 7.45, so putting this here in order to create a merge conflict when this gets merged to main. At that point, I'll add this release note under the correct release heading.

Comment on lines -320 to -321
output.append("Auto archive played episodes after: " + afterPlaying[settings.getAutoArchiveAfterPlaying().toIndex()]).append(eol)
output.append("Auto archive inactive episodes after: " + inactive[settings.getAutoArchiveInactive().toIndex()]).append(eol)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I changed this to no longer use localized strings here since localized strings just seem like they would make things more difficult for support. I'm using the analytics value because that seemed good enough, but if you think we'd be better off creating some even-more-readable support-specific strings, let me know.

@mchowning mchowning marked this pull request as ready for review August 3, 2023 01:53
@mchowning mchowning requested a review from a team as a code owner August 3, 2023 01:53
@ashiagr
Copy link
Contributor

ashiagr commented Aug 7, 2023

All tests pass.

This PR fixes this by migrating the settings to a new key in shared preferences that is no longer tied to the app's language.

Thank you for fixing it 🙇‍♀️ I've verified that it works correctly now.

@ashiagr ashiagr merged commit 5cfe14c into update/refactor-settings-for-sync_appearance Aug 7, 2023
@ashiagr ashiagr deleted the update/refactor-settings-for-sync_auto-archive branch August 7, 2023 05:37
@ashiagr ashiagr added this to the 7.47 ❄️ milestone Sep 4, 2023
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.

2 participants