WordPress to Jetpack flow: main flow implementation by Gio2018 · Pull Request #19563 · wordpress-mobile/WordPress-iOS

Gio2018 · 2022-11-06T22:20:44Z

Ref #19517
Ref #19518

This PR implements the remaining wp-to-jp flow

NOTE: the UI is not finalized (text and images can be missing or wrong in the notifications and all done screens), nor optimized for landscape and multiple screens, the notifications permissions is not there yet, and the migration flow is shown by default at each launch. All these points will be addressed in separate PRs

To test:
Scenario A - Notifications permissions undetermined (user has not made a decision)
Case 1: skip notifications permission decision:

checkout this branch and build/run the Jetpack target
In the welcome screen, tap Continue and make sure the Notifications permissions screen appears
In the Notifications permissions screen, tap decide later and make sure the all done screen appears.
In the all done screen, tap Finish and make sure the migration UI gets dismissed and the app UI appears.

Case 2: skip notifications permission decision:
same as above, except on step 3, tap "Continue" and make sure the notifications permission alert shows up, then:

Make a decision
Regardless the decision, make sure the all done screen appears, and continue from step 4 above

Scenario B - Notifications permissions determined (user has already made a decision)
Same as Scenario A - Case 1, except step 3 is missing: after tapping Continue in the welcome screen, make sure the all done screen appears. After that, continue normally with step 4

Optional: do a smoke test and make sure the app behaves correctly.

Regression Notes

Potential unintended areas of impact
None other the migration itself
What I did to test those areas of impact (or what existing automated tests I relied on)
What automated tests I added (or what prevented me from doing so)

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.~~ Not released

…p-to-jp migration

…troller for wp-to-jp migration

…types

…to-jp migration-notifications permissions step

…ation-notifications permissions step

…ration-final step

…al step

…migration flow, if needed

…guration types

…uration types

wpmobilebot · 2022-11-06T22:42:22Z

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 pr19563-670a52e on your iPhone

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

wpmobilebot · 2022-11-06T22:45:48Z

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 pr19563-670a52e on your iPhone

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

…ler, add some comments

salimbraksa · 2022-11-07T14:22:15Z

...Classes/ViewRelated/WordPress-to-Jetpack Migration/Common/MigrationDependencyContainer.swift

+
+    func makeInitialViewController() -> UIViewController {
+        MigrationNavigationController(coordinator: migrationCoordinator,
+                                      migrationStack: [.welcome: makeWelcomeViewController(),


I'm not opposed to this approach of having a dictionary mapping MigrationStep to a UIViewController but I just want to point out that these view controllers will stay in memory even when they're dismissed ( not visible ).

Perhaps we can optimize this by injecting a closure like this:

let closure = { [weak self] (step: MigrationStep) -> UIViewController? in switch step { case .welcome: return self?.makeWelcomeViewController() ... } }

I like this idea: I added a factory that does exactly that, and we don't retain any unused vc in memory!

salimbraksa · 2022-11-07T14:30:44Z

.../ViewRelated/WordPress-to-Jetpack Migration/Common/Navigation/MigrationFlowCoordinator.swift

+import Combine
+
+/// Coordinator for the migration to jetpack flow
+final class MigrationFlowCoordinator: ObservableObject {


My understanding of Coordinator objects in the MVVM-C architecture is that they can instantiate and work with UIViewController objects ( e.g. this article and this ). That's why in the previous implementation, the coordinator was responsible for mapping a MigrationStep to a UIViewController and also pushing the view controllers to the navigation stack.

But I also like the idea of a UI-agnostic coordinator. But in this case, this object seems to me more of a ViewModel than a Coordinator. Maybe we should rename it to MigrationFlowViewModel?

What do you think?

Thanks for the thoughtful design review and the following discussion we had on Slack! As discussed, we'll leave the current naming for now and re-consider it together with Tanner (avoid direct pinging on purpose 😄 ) when he comes back from the meetup!

salimbraksa · 2022-11-07T14:46:22Z

...k/Classes/ViewRelated/WordPress-to-Jetpack Migration/Welcome/MigrationWelcomeViewModel.swift

+        },
+                                                                                      secondaryHandler: {
+
+        }))


This is just a nice-to-have, but I honestly find this code a bit hard to read 🙈

Maybe we should breakdown this complex expression into multiple variables, something like this:

let headerConfig = MigrationHeaderConfiguration(step: .welcome, multiSite: blogListDataSource.visibleBlogsCount > 1) let actionsConfig = MigrationActionsViewConfiguration(step: .welcome, primaryHandler:...) configuration = MigrationStepConfiguration(headerConfiguration: headerConfig, actionsConfig: actionsConfig)

Totally agree. I did this in all view models

Gio2018 · 2022-11-07T21:49:18Z

Thank you for the thorough review @salimbraksa !!
I addressed the comments, change base to trunk and put it in ready for review, now ready for another pass!!

… is always instantiated on the main thread

…ission screen if users have already decided

…ontrollers on the main thread

…ix indentation

salimbraksa · 2022-11-08T14:10:35Z

...Classes/ViewRelated/WordPress-to-Jetpack Migration/Common/MigrationDependencyContainer.swift

-                                      migrationStack: [.welcome: makeWelcomeViewController(),
-                                                       .notifications: makeNotificationsViewController(),
-                                                       .done: makeDoneViewController()])
+                                      factory: MigrationViewControllerFactory(coordinator: migrationCoordinator))


I like this, it's better than passing a closure 👍

salimbraksa · 2022-11-08T14:23:36Z

...Related/WordPress-to-Jetpack Migration/Common/Navigation/MigrationNavigationController.swift

-            self?.updateStack(for: step)
+            DispatchQueue.main.async {
+                self?.updateStack(for: step)
+            }


We can also use .receive(on:) if you prefer:

publisher .receive(on: DispatchQueue.main) .sink { value in }

Source: https://developer.apple.com/documentation/combine/fail/receive(on:options:)

salimbraksa · 2022-11-08T14:34:02Z

.../ViewRelated/WordPress-to-Jetpack Migration/Common/Navigation/MigrationFlowCoordinator.swift

 /// Coordinator for the migration to jetpack flow
 final class MigrationFlowCoordinator: ObservableObject {

    @Published private(set) var currentStep = MigrationStep.welcome


Perhaps we should add a documentation here mentioning that currentStep values are published in a background thread.

I'm pretty sure future developers can easily know this by reading the transitionToNextStep implementation details but it would be great to make finding about this detail extra easier.

Just realized there is a tiny UI glitch

salimbraksa · 2022-11-08T15:17:40Z

@Gio2018 The latest changes look good to me and I actually went ahead and approved the PR but then I discovered there is a tiny UI glitch where the app appears black for a second before the Welcome Screen shows up.

And I think the bug's root cause is:

init(coordinator: MigrationFlowCoordinator, factory: MigrationViewControllerFactory) {
   self.coordinator = coordinator
   self.factory = factory
   // `super.init(rootViewController:)` should be called for a smooth launch
   super.init(nibName: nil, bundle: nil)
   configure()
}

… need to hold a reference to it

…ller to avoid initial glitch

peril-wordpress-mobile · 2022-11-08T16:41:40Z

	Warnings
⚠️	PR has more than 500 lines of code changing. Consider splitting into smaller PRs if possible.

Generated by 🚫 dangerJS

Gio2018 · 2022-11-08T16:42:35Z

there is a tiny UI glitch

good catch @salimbraksa ! I went ahead and changed the init to assign a root right away!

…er()

…nd drop first state change, since it already corresponds to the same initial vc

salimbraksa · 2022-11-08T17:16:05Z

@Gio2018 LGTM! 🚀

Giorgio Ruscigno added 15 commits November 6, 2022 16:01

Add MigrationDpendencyContainer, dependency injection container for w…

9d3a0df

…p-to-jp migration

Add MigrationNavigationController, custom, state-based navigation con…

14ebd9d

…troller for wp-to-jp migration

Update MigrationFlowCoordinator, extract ui logic from coordinator

48a4c65

Add MigrationStepConfiguration, a refactor of existing configuration …

30a79ab

…types

Add MigrationStepView, a common base view for migration step screens

e2cdb12

Add MigrationNotificationsViewController, view controller for the wp-…

3b8216f

…to-jp migration-notifications permissions step

Add MigrationNotificationsViewModel, view model for the wp-to-jp migr…

45e1c2e

…ation-notifications permissions step

Add MigrationDoneViewController, view controller for the wp-to-jp mig…

8e7786a

…ration-final step

Add MigrationDoneViewModel, view model for the wp-to-jp migration-fin…

81dfb16

…al step

Update WindowManager, default optional completion to nil

58bffd0

Update JetpackWindowManager, refactor to enable showing the wp-to-jp …

6205faf

…migration flow, if needed

Update MigrationStep, add missing steps and extract in separate file

a89c8a2

Update MigrationActionsView, refactors based on the refactor of confi…

e73c00e

…guration types

Update MigrationHeaderView, refactors based on the refactor of config…

b1bb8e9

…uration types

Files and groups re-arranged in WordPress-to-Jetpack migration group

493c694

Gio2018 added [Type] Task Jetpack Focus labels Nov 6, 2022

Gio2018 added this to the 21.2 milestone Nov 6, 2022

Gio2018 self-assigned this Nov 6, 2022

Gio2018 changed the base branch from trunk to task/wp-migration-welcome November 6, 2022 22:22

Gio2018 marked this pull request as draft November 6, 2022 22:24

Giorgio Ruscigno added 4 commits November 6, 2022 17:49

Update MigrationStepConfiguration, put separate types in separate files

dadb84f

Update MigrationDoneViewController, MigrationNotificationsViewControl…

be02a8b

…ler, add some comments

Update MigrationDependencyContainer, remove incorrect comment

f258dd3

Update MigrationStepView, remove unnecessary settings

2660dca

salimbraksa reviewed Nov 7, 2022

View reviewed changes

Merge branch trunk into task/migration-notifications-premission

ff32f03

Gio2018 marked this pull request as ready for review November 7, 2022 21:47

Gio2018 requested a review from salimbraksa November 7, 2022 21:47

Giorgio Ruscigno added 4 commits November 7, 2022 15:57

Update JetpackWindowManager, simplify logic and make sure that the ui…

15e7a29

… is always instantiated on the main thread

Update MigrationFlowCoordinator, add logic to skip notifications perm…

3bf7ef7

…ission screen if users have already decided

Update MigrationNavigationController, always create and assign view c…

3387916

…ontrollers on the main thread

Update migration view models, add notification permission logic and f…

1d7ccdc

…ix indentation

salimbraksa reviewed Nov 8, 2022

View reviewed changes

salimbraksa previously approved these changes Nov 8, 2022

View reviewed changes

salimbraksa self-requested a review November 8, 2022 15:15

Giorgio Ruscigno added 2 commits November 8, 2022 10:40

Update MigrationDependencyContainer, changed to struct since we don't…

eabe689

… need to hold a reference to it

Update MigrationNavigationController, instantiate with rootViewContro…

959f687

…ller to avoid initial glitch

Giorgio Ruscigno added 3 commits November 8, 2022 11:08

Update MigrationViewControllerFactory, add method initialViewControll…

40127da

…er()

Update MigrationNavigationController, set initial root from factory a…

40be348

…nd drop first state change, since it already corresponds to the same initial vc

Update MigrationFlowCoordinator, add comments

313c203

salimbraksa approved these changes Nov 8, 2022

View reviewed changes

Giorgio Ruscigno added 3 commits November 8, 2022 11:49

Update JetpackWindowManager, small refactor

5d9d95f

Update JetpackWindowManager, small refactor

750bafa

Update JetpackWindowManager, disable migration UI

670a52e

Gio2018 merged commit b675bf8 into trunk Nov 8, 2022

Gio2018 deleted the task/migration-notifications-permission branch November 8, 2022 22:36

AliSoftware mentioned this pull request Nov 16, 2022

Fix a NSLocalizedStrings not using a literal value, leading to an export issue #19606

Merged

4 tasks

Conversation

Gio2018 commented Nov 6, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Regression Notes

Uh oh!

wpmobilebot commented Nov 6, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

wpmobilebot commented Nov 6, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

salimbraksa Nov 7, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Gio2018 Nov 7, 2022

Choose a reason for hiding this comment

Uh oh!

salimbraksa Nov 7, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Gio2018 Nov 7, 2022

Choose a reason for hiding this comment

Uh oh!

salimbraksa Nov 7, 2022

Choose a reason for hiding this comment

Uh oh!

Gio2018 Nov 7, 2022

Choose a reason for hiding this comment

Uh oh!

Gio2018 commented Nov 7, 2022

Uh oh!

salimbraksa Nov 8, 2022

Choose a reason for hiding this comment

Uh oh!

salimbraksa Nov 8, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

salimbraksa Nov 8, 2022

Choose a reason for hiding this comment

Uh oh!

Gio2018 Nov 8, 2022

Choose a reason for hiding this comment

Uh oh!

salimbraksa commented Nov 8, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

peril-wordpress-mobile bot commented Nov 8, 2022

Uh oh!

Gio2018 commented Nov 8, 2022

Uh oh!

salimbraksa commented Nov 8, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Gio2018 commented Nov 6, 2022 •

edited

Loading

wpmobilebot commented Nov 6, 2022 •

edited

Loading

wpmobilebot commented Nov 6, 2022 •

edited

Loading

salimbraksa Nov 7, 2022 •

edited

Loading

salimbraksa Nov 7, 2022 •

edited

Loading

salimbraksa Nov 8, 2022 •

edited

Loading

salimbraksa commented Nov 8, 2022 •

edited

Loading