Skip to content

Jetpack Content Migration: Refactor "Open WordPress" screen to appear only when the app launches #19961

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 3 commits into from
Jan 20, 2023
Merged

Jetpack Content Migration: Refactor "Open WordPress" screen to appear only when the app launches #19961

merged 3 commits into from
Jan 20, 2023

Conversation

@salimbraksa
Copy link
Contributor

@salimbraksa salimbraksa commented Jan 20, 2023 •
edited
Loading

Fixes #19950

Description

This PR fixes an issue where the Login Flow was restarting every time the app enters the foreground. The solution that resolves this issue is showing the "Load WordPress" screen only if the rootViewController is nil.

Before After
Simulator.Screen.Recording.-.iPhone.14.-.2023-01-19.at.12.40.53.mp4
CleanShot.2023-01-20.at.13.19.40.mp4

Test Instructions

Case 1: WordPress app installed

  1. Clean install WordPress and log in.
  2. Clean install the Jetpack app and run it.
  3. Expect the "Load WordPress" screen to appear.
  4. Tap "No Thanks".
  5. Tap "Continue with WordPress.com".
  6. Move the Jetpack app to background and open it again.
  7. Expect to see the same screen you saw before moving the app to background.

Case 2: WordPress app not installed

  1. Delete the WordPress app.
  2. Kill the Jetpack app and run it again.
  3. Expect the Login Prologue screen to appear.
  4. Tap "Continue with WordPress.com".
  5. Move the Jetpack app to background and open it again.
  6. Expect to see the same screen you saw before moving the app to background.

Bonus

Although this PR shouldn't impact the content migration logic, but it would be great to make sure the migration flow is still working as expected. 🙇

Regression Notes

  1. Potential unintended areas of impact
    This PR only impacts when the "Load WordPress" is shown. Previously it was shown every time the app enters foreground, now it only appears at launch.

  2. What I did to test those areas of impact (or what existing automated tests I relied on)
    Manual testing.

  3. What automated tests I added (or what prevented me from doing so)
    None.

PR submission checklist:

  • I have completed the Regression Notes.
  • I have considered adding unit tests for my changes.
  • I have considered adding accessibility improvements for my changes.
  • I have considered if this change warrants user-facing release notes and have added them to RELEASE-NOTES.txt if necessary.

@salimbraksa salimbraksa mentioned this pull request Jan 20, 2023
Closed
4 tasks
@salimbraksa salimbraksa changed the title Jetpack Contnet Migration: Fix Login flow restarting every time the app enters the foreground Jetpack Content Migration: Fix Login flow restarting every time the app enters the foreground Jan 20, 2023
@salimbraksa salimbraksa self-assigned this Jan 20, 2023
@salimbraksa salimbraksa added this to the 21.5 milestone Jan 20, 2023
@salimbraksa salimbraksa marked this pull request as ready for review January 20, 2023 12:26
@salimbraksa salimbraksa requested a review from twstokes January 20, 2023 12:26
@peril-wordpress-mobile
Copy link

peril-wordpress-mobile bot commented Jan 20, 2023

Warnings
⚠️ This PR is assigned to a milestone which is closing in less than 4 days Please, make sure to get it merged by then or assign it to a later expiring milestone

Generated by 🚫 dangerJS

@salimbraksa salimbraksa requested review from dvdchr and mokagio January 20, 2023 12:29
@salimbraksa salimbraksa changed the title Jetpack Content Migration: Fix Login flow restarting every time the app enters the foreground Jetpack Content Migration: Refactor "Open WordPress" screen to appear only when the app launches Jan 20, 2023
@wpmobilebot
Copy link
Contributor

wpmobilebot commented Jan 20, 2023 •
edited
Loading

You can test the changes in Jetpack from this Pull Request by:
  • Clicking here or scanning the QR code below to access App Center
  • Then installing the build number pr19961-86a0ed4 on your iPhone

If you need access to App Center, please ask a maintainer to add you.

@wpmobilebot
Copy link
Contributor

wpmobilebot commented Jan 20, 2023 •
edited
Loading

You can test the changes in WordPress from this Pull Request by:
  • Clicking here or scanning the QR code below to access App Center
  • Then installing the build number pr19961-86a0ed4 on your iPhone

If you need access to App Center, please ask a maintainer to add you.

twstokes
twstokes approved these changes Jan 20, 2023
View reviewed changes
Copy link
Contributor

@twstokes twstokes left a comment

Choose a reason for hiding this comment

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

This LGTM @salimbraksa! 🚀

I tested:

  • The migration flow
  • "No thanks" -> logging in
  • "No thanks" -> restart -> migrating
  • Clean install without WordPress installed
  • On iPad

And didn't spot any issues. 👍

Currently the PR is targeted for trunk so we'll need to update that to the 21.5 release. Thanks!

@dvdchr dvdchr self-assigned this Jan 20, 2023
dvdchr
dvdchr requested changes Jan 20, 2023
View reviewed changes
Copy link
Contributor

@dvdchr dvdchr left a comment

Choose a reason for hiding this comment

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

I've also tested the steps, and it fixes the issue as described. 👍🏼

⚠️ However, it looks like there are some regressions in the flow. Tapping the "Open WordPress" button when the WordPress app is logged out leads to the "Load WordPress" screen again. Here's a recording:

Simulator.Screen.Recording.-.iPhone.14.Pro.-.2023-01-20.at.22.25.07.mp4

I suspect this is because after returning to WordPress and getting a migration error, the sign-in UI is not shown because the Load WordPress UI is currently being shown.

Additionally, when testing the local draft scenario (refer to no. 7 from the table below), it unexpectedly showed me the 🔁 Open WordPress flow when it should've been the 🔐 Sign-in flow. I'm suspecting the root cause is the same as the above 🤔

Here's a table:

No. WordPress app Account state Scenario Expected flow Result
1 Not installed n/a n/a 🔐 Sign-in flow unchanged
2 Incompatible Any Any 🔐 Sign-in flow unchanged
3 Compatible Logged out Never opened 🔁 Open WordPress unchanged
4 Compatible Logged out Log in > then tap the Jetpack banner 🟢 Normal migration unchanged
5 Compatible Logged in Never opened 🔁 Open WordPress unchanged
6 Compatible Logged in Tap the Jetpack banner 🟢 Normal migration unchanged
7 Compatible Logged in Airplane mode > create local draft > tap the Jetpack banner 🔐 Sign-in flow 🔁 Open WordPress
8 Compatible Logged in Log out 🔐 Sign-in flow unchanged

@dvdchr dvdchr self-requested a review January 20, 2023 15:34
@dvdchr
Copy link
Contributor

dvdchr commented Jan 20, 2023

👋🏼 @salimbraksa, @twstokes. I've pushed f73e480 which should address all the issues I've raised in my previous comment.

Before After
Simulator.Screen.Recording.-.iPhone.14.Pro.-.2023-01-20.at.22.25.07.mp4
Simulator.Screen.Recording.-.iPhone.14.Pro.-.2023-01-20.at.23.17.00.mp4.mp4

Here are three test scenarios to verify:

1️⃣ Verifying that the Open WordPress loop is fixed:

  • Clean install WordPress. Don't log in.
  • Clean install Jetpack, and open it.
  • 🔍 Verify that the Load WordPress flow is shown.
  • Tap on the Open WordPress button.
  • 🔍 After returning from the WordPress app, verify that the 🔐 Sign-in flow is shown.

2️⃣ Verifying the local draft scenario shows the 🔐 Sign-in flow:

  • Clean install WordPress and log in to your WordPress.com account.
  • Turn on airplane mode / disable wi-fi and publish a local post.
  • Clean install Jetpack, and open it.
  • 🔍 Verify that the Load WordPress flow is shown.
  • Tap on the Open WordPress button.
  • 🔍 After returning from the WordPress app, verify that the 🔐 Sign-in flow is shown.

3️⃣ Verifying that the original issue remains fixed:

  • Clean install WordPress. Don't log in.
  • Clean install Jetpack, and open it.
  • 🔍 Verify that the Load WordPress flow is shown.
  • Tap on the No thanks button.
  • Tap Continue with WordPress.com.
  • Minimize the Jetpack app, and then open it again.
  • 🔍 Verify that the app is still showing the unified login flow.

Lastly, here's the updated table for the overall migration pathways:

No. WordPress app Account state Scenario in WP Flow after f73e480
1 Not installed n/a n/a 🔐 Sign-in flow
2 Incompatible Any Any 🔐 Sign-in flow
3 Compatible Logged out Never opened 🔁 Open WordPress
4 Compatible Logged out Log in > then tap the Jetpack banner 🟢 Normal migration
5 Compatible Logged in Never opened 🔁 Open WordPress
6 Compatible Logged in Tap the Jetpack banner 🟢 Normal migration
7 Compatible Logged in Airplane mode > create local draft > tap the Jetpack banner 🔐 Sign-in flow
8 Compatible Logged in From scenario no. 5: Switch to WP > Log out > Switch to JP > Tap Open WordPress 🔐 Sign-in flow
9 Compatible Logged in From scenario no. 5: Tap 'No thanks' > Tap 'Log in with WordPress.com' > Background the app > Foreground the app again Stay in Unified Login flow

@twstokes
Copy link
Contributor

twstokes commented Jan 20, 2023 •
edited
Loading

Thank you @dvdchr for the catch and fix!

It appears what's left is to target this for the release/21.5 branch which will require a rebase. I'll be happy to help test all these scenarios again once that's done. 🙇

I'll also be happy to wrangle this and force-push it - so just let me know. 👍

Update: This is done and I'm going to re-test now.

@twstokes twstokes force-pushed the fix/19950-jp-migration-login-bug-alt branch from f73e480 to 86a0ed4 Compare January 20, 2023 18:52
@twstokes twstokes changed the base branch from trunk to release/21.5 January 20, 2023 18:52
@salimbraksa
Copy link
Contributor Author

salimbraksa commented Jan 20, 2023 •
edited
Loading

Thanks @twstokes for rebasing this PR. I was about to work on that. I'll help with testing as well.

@twstokes
Copy link
Contributor

twstokes commented Jan 20, 2023

I tested:

  • Tests 6-9
  • PR test cases 1-2

And all succeeded. 🎉

@salimbraksa
Copy link
Contributor Author

salimbraksa commented Jan 20, 2023

I tested the following:

  • The 1️⃣ 2️⃣ and 3️⃣ scenarios from this comment
  • Tests 1-5

All good 🚀

@salimbraksa salimbraksa merged commit 56698f1 into release/21.5 Jan 20, 2023
@salimbraksa salimbraksa deleted the fix/19950-jp-migration-login-bug-alt branch January 20, 2023 20:08
mokagio pushed a commit that referenced this pull request Jan 24, 2023
@salimbraksa @mokagio
Jetpack Content Migration: Refactor "Open WordPress" screen to appear…
e8b152c 
… only when the app launches (#19961)
@mokagio mokagio mentioned this pull request Jan 24, 2023
Merged
mokagio added a commit that referenced this pull request Jan 24, 2023
@mokagio
Cherry-pick #19961 into hotfix 21.5.1 (#19972)
5195282
This was referenced Jan 24, 2023
Merged
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@dvdchr dvdchr dvdchr approved these changes

@twstokes twstokes twstokes approved these changes

@mokagio mokagio Awaiting requested review from mokagio

Assignees

@dvdchr dvdchr

@salimbraksa salimbraksa

Labels

Jetpack App Jetpack Content Migration Flow [Type] Bug

Projects

None yet

Milestone

21.5

Development

Successfully merging this pull request may close these issues.

Jetpack: Login flow restarted if the app leaves the foreground

5 participants

@salimbraksa @wpmobilebot @dvdchr @twstokes