Restrict independent watch functionality to Plus users #994

mchowning · 2023-05-24T15:33:58Z

Description

This prevents users from accessing the wear app if they are not Plus users since v1 of the wear app will not have control-the-phone-app-from-the-watch-app functionality for free users.

Getting this working well turned out to be more difficult than I expected. In particular, getting the "logging in" screen to show and block the user until the refresh was complete was the tricky part. I'm mentioning this just to call out that this PR probably merits some extra testing.

A couple of issues you may see:

Because the button for proceeding to log in is off screen, I also made the text on that screen clickable to reduce the chance of a user get hung up on that screen.
I've noticed that occasionally when installing the app from Android Studio on top of a previous install, then logging out, and then logging back in with google, the app could get stuck on a logging in screen. I've only seen this a couple of times, and I have never reproduced it on a clean install.
Because of this issue you may see crashes when logging out (but only on debug builds). For testing purposes, I just updated the RxJavaUncaughtExceptionHandling object to not throw any exceptions because anytime someone logs into the watch app with a free account, they get immediately logged out (which can result in a crash on debug builds). I'm starting to wonder if maybe this crash-only-in-debug idea of mine wasn't such a great idea.
I had it happen a couple of times where the watch app would log in automatically even though the phone app was not logged in. I haven't been able to track down the cause of this (and it doesn't seem like a big problem since the user can just log out if they want to switch accounts).
Note that, currently, once you log in using a google account, you cannot then switch to a different google account without reinstalling the app.

Testing Instructions

For each of the following scenarios:

Logging in automatically because your watch-connected phone is logged in
Logging in using a Google account
Logging in using email and password

Verify that:

if it is a free account, the logging in screen will be displayed, and then the user is returned to the UI prompting the user to log in.
If it is a plus account, the logging in screen is displayed, and that you are taken into the app once the refresh is complete

In addition, also test that we are handling accounts that change from Plus to Free appropriately:

Sign in with a Plus account
Change the account to being Free
Perform a manual refresh from the Settings screen
Verify that you are returned to the RequirePlusScreen

Phone app

Because I made a small change to how we report a user's subscription status, it would be good to do some smoke-testing around logging into and out of some Plus and Free accounts on the phone app.

Screenshots or Screencast

Plus	Free
Screen.Recording.2023-05-24.at.9.17.11.PM.mov	Screen.Recording.2023-05-24.at.9.38.14.PM.mov

Checklist

If this is a user-facing change, I have added an entry in CHANGELOG.md
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

I have tested any UI changes...

with different themes
with a landscape orientation
with the device set to have a large display and font size
for accessibility with TalkBack

mchowning · 2023-05-24T15:38:19Z

...ain/java/au/com/shiftyjelly/pocketcasts/repositories/subscription/SubscriptionManagerImpl.kt

@@ -69,7 +69,7 @@ class SubscriptionManagerImpl @Inject constructor(
        if (cachedStatus != null) {
            accept(Optional.of(cachedStatus))
        } else {
-            accept(Optional.of(SubscriptionStatus.Free()))
+            accept(Optional.of(null))


Made this change so that a user who is not logged in does not have SubscriptionStatus.Free. This matters because when a Plus user logs in, there is a brief period of time where the user is signed in, but we haven't gotten their subscription status from the subscription API call yet. Before this change, the user would be signed into a plus account, but the app would report their subscription status as free. With this change, we now know that we don't have their subscription status yet because it will be null.

mchowning · 2023-05-25T01:18:36Z

I've noticed that occasionally when installing the app from Android Studio on top of a previous install, then logging out, and then logging back in with google, the app could get stuck on a logging in screen. I've only seen this a couple of times, and I have never reproduced it on a clean install.

I still haven't managed to reproduce this issue, but I improved the logic a bit in f4ac9b8, and I think that might help avoid the issue from happening.

ashiagr · 2023-05-25T10:49:41Z

Great job in managing the tricky navigation required for this task!

I tested the app for each of the listed scenarios and found it working well:

Scenario	Free Account (logging in screen displayed, and user is returned to the UI prompting the user to log in)	Plus Account ( logging in screen displayed, and user is taken to the app once the refresh is complete)
Logging in automatically because your watch-connected phone is logged in	✅	✅
Logging in using a Google account	✅	✅
Logging in using email and password	✅	✅

I've noticed that occasionally when installing the app from Android Studio on top of a previous install, then logging out, and then logging back in with google, the app could get stuck on a logging in screen. I've only seen this a couple of times, and I have never reproduced it on a clean install.

I was stuck at the login screen at times, I will continue to test and share the steps to reproduce once I find a pattern. It might have happened because the account might have many podcasts and I see we already have a task to address it:

Add some kind of animation spinner to the logging in screen when an image is shown so the user knows something is happening if the refresh can take a really long time on long accounts (need to do some testing here).

but I think something else is also going on here.

I'm not approving the PR yet as I'd like to give some more time for testing it and also review the code changes.

mchowning · 2023-05-25T13:34:57Z

I was stuck at the login screen at times, I will continue to test and share the steps to reproduce once I find a pattern.

I've noticed that when I change my emulator to have a slow internet connection, the refresh sometimes takes extremely long (as in, multiple minutes). It's weird because as I slow down the emulator, everything seems to work fine, and then suddenly there's a huge drop off in the speed once I slow it down past a certain point. I'm not sure what's going on there either, but just sharing in case it gives you any hints as you test further.

I'm not approving the PR yet as I'd like to give some more time for testing it and also review the code changes.

👍 Absolutely! Please take your time and test as much as you want. I've run into so many bugs trying to implement this, and I really appreciate you taking the time to carefully test this.

mchowning · 2023-05-25T17:57:29Z

An additional scenario worth testing is:

Sign in with a Plus account
Change the account to being Free
Perform a manual refresh from the Settings screen
Verify that you are returned to the RequirePlusScreen

This is working fine for me, but I just wanted to call it out as an additional test scenario, so I'll add this to the PR description.

mchowning · 2023-05-25T19:34:36Z

I found that there is an issue where when a user signs in with a google account, the app still uses the same google account and idToken on subsequent sign in attempts even if the sign in attempt occurs after the idToken has expired (1 hour expiration). This isn't something we're doing but is happening within the google sign in libraries and horologist.

I think that's the cause of some of the issues I've seen (and why I found them so hard to reproduce). I've done an update in 519a189 that avoids that issue (and also fixes #993 by allowing the user to select a different account if they want when they sign back in). When the user selects an account, we get a fresh idToken.

ashiagr

Sign in with a Plus account
Change the account to being Free ...

This worked as expected, thanks for listing it as an additional testing scenario 👍

I found that there is an issue where when a user signs in with a google account, the app still uses the same google account and idToken on subsequent sign in attempts even if the sign in attempt occurs after the idToken has expired (1 hour expiration). This isn't something we're doing but is happening within the google sign in libraries and horologist.

I think that's the cause of some of the issues I've seen (and why I found them so hard to reproduce). I've done an update in 519a189 that avoids that issue (and also fixes #993 by allowing the user to select a different account if they want when they sign back in). When the user selects an account, we get a fresh idToken.

You nailed it! 🌟 🌟 🌟 I was able to switch between Google accounts today.

I've noticed that when I change my emulator to have a slow internet connection, the refresh sometimes takes extremely long (as in, multiple minutes). It's weird because as I slow down the emulator, everything seems to work fine, and then suddenly there's a huge drop off in the speed once I slow it down past a certain point. I'm not sure what's going on there either, but just sharing in case it gives you any hints as you test further.

I think that might have been the issue. So that I got a better load and refresh performance, I tested the PR on a release apk and things were so much smoother.

By now, I have tested it multiple times and it behaved correctly each time. I also smoke-tested phone app with different account types and it also worked correctly. Code changes look good as well. With this, I'm approving the PR.

mchowning added 8 commits May 23, 2023 12:34

Limit watch to Plus users

899c191

Make sure refresh is in progress when starting LoggingInScreen

6e73d9c

Improve comment

1bf30c2

Simplify logic for showing LoggingInScreen

68be976

Remove not-signed-in subscription status

e63bc44

Remove RequirePlusViewModel

b462224

Remove unused variable

7fcf91d

Avoid Kotlin warning about not using assigned value

cfd4965

mchowning added the [Area] Wear OS watch app label May 24, 2023

mchowning commented May 24, 2023

View reviewed changes

mchowning marked this pull request as ready for review May 24, 2023 15:40

mchowning requested a review from a team as a code owner May 24, 2023 15:40

mchowning mentioned this pull request May 24, 2023

Project Issue: Initial Release of Wear OS Watch App ⌚️ #825

Closed

56 tasks

mchowning added 3 commits May 24, 2023 17:43

Update UI for RequirePlusScreen

fa89591

Add Toast when signing into Free account on watch

d96f4f5

Improve sign on flow logic

f4ac9b8

Scroll to top when returning to RequirePlusScreen

151c933

Clear previous accounts from googleSignInClient

519a189

mchowning mentioned this pull request May 25, 2023

Watch automatically selects previously selected Google accounts when signing in #993

Closed

1 task

ashiagr approved these changes May 26, 2023

View reviewed changes

ashiagr merged commit 817a7af into main May 26, 2023

ashiagr deleted the add/require-plus-for-watch branch May 26, 2023 10:11

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Restrict independent watch functionality to Plus users #994

Restrict independent watch functionality to Plus users #994

mchowning commented May 24, 2023 •

edited

Loading

mchowning May 24, 2023

mchowning commented May 25, 2023 •

edited

Loading

ashiagr commented May 25, 2023

mchowning commented May 25, 2023

mchowning commented May 25, 2023 •

edited

Loading

mchowning commented May 25, 2023

ashiagr left a comment

Restrict independent watch functionality to Plus users #994

Restrict independent watch functionality to Plus users #994

Conversation

mchowning commented May 24, 2023 • edited Loading

Description

Testing Instructions

Phone app

Screenshots or Screencast

Checklist

I have tested any UI changes...

mchowning May 24, 2023

Choose a reason for hiding this comment

mchowning commented May 25, 2023 • edited Loading

ashiagr commented May 25, 2023

mchowning commented May 25, 2023

mchowning commented May 25, 2023 • edited Loading

mchowning commented May 25, 2023

ashiagr left a comment

Choose a reason for hiding this comment

mchowning commented May 24, 2023 •

edited

Loading

mchowning commented May 25, 2023 •

edited

Loading

mchowning commented May 25, 2023 •

edited

Loading