[Jetpack Content Migration Flow] Add Help Screen #19618

salimbraksa · 2022-11-17T16:05:34Z

Fixes #19617

Description

Display support screen when the user taps "Need help?" or "Me" avatar button in the Migration Welcome screen. This support screen is the same as the existing support screen except with few visual changes:

Add User Details view
Hide Logs / Debug section
Add Log out button

These visual changes are applied only when the support screen is accessed from the Migration flow.

Upcoming PR

Since the Log Out logic is already implemented in MeViewController, this PR is re-using that logic, see LogOutActionHandler. But this PR doesn't refactor MeViewController to use this new handler object, so the Log Out logic is duplicated for now. Will open another PR to fix this.

Screenshots

Support Screen during Migration Flow	Existing Support Screen outside Migration flow

Test Instructions:

Support Screen during Migration flow

Run Jetpack app and make sure you're logged in
Enable Migration Flow
1. Locate JetpackWindowManager.swift file
2. Change shouldShowMigrationUI value to true
3. Build and run Jetpack app
The Migration Welcome screen should show up
Tap "Need Help?" or "Me" avatar button
Expect the support screen to show up and it looks like the 1st screenshot
Tap Logout, then confirm logging out
Expect the Jetpack Prologue screen to show up

Support Screen outside Migration flow

From the Jetpack Prologue screen, log in to your account again
Under "My Site" tab, tap "Me" avatar button in the top-right corner
Tap "Help & Support" row
Expect the support screen to show up and looks like the 2nd screenshot

Regression Notes

Potential unintended areas of impact
It's worth testing that the existing support screen wasn't impacted. See Test Instructions > Support Screen outside Migration flow.
What I did to test those areas of impact (or what existing automated tests I relied on)
None.
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.

wpmobilebot · 2022-11-17T17:23:43Z

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 pr19618-4417b15 on your iPhone

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

wpmobilebot · 2022-11-17T17:23:45Z

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 pr19618-4417b15 on your iPhone

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

Gio2018

Thanks for working on this @salimbraksa !
Left a few comments, let me know when the state is final so I can do a full review!

Gio2018 · 2022-11-18T00:15:38Z

WordPress/Classes/ViewRelated/Support/LogOutActionHandler.swift

+    private struct Strings {
+        static let alertDefaultTitle = AppConstants.Logout.alertTitle
+        static let alertUnsavedTitleSingular = NSLocalizedString("You have changes to %d post that hasn't been uploaded to your site. Logging out now will delete those changes. Log out anyway?",
+                                                            comment: "Warning displayed before logging out. The %d placeholder will contain the number of local posts (SINGULAR!)")
+        static let alertUnsavedTitlePlural = NSLocalizedString("You have changes to %d posts that haven’t been uploaded to your site. Logging out now will delete those changes. Log out anyway?",
+                                                          comment: "Warning displayed before logging out. The %d placeholder will contain the number of local posts (PLURAL!)")
+        static let alertCancelAction = NSLocalizedString("Cancel", comment: "Verb. A button title. Tapping cancels an action.")
+        static let alertLogoutAction = NSLocalizedString("Log Out", comment: "Button for confirming logging out from WordPress.com account")
+    }


Should we use the new reverse dns notation here?

Since the log out logic is already implemented in MeViewController, I re-used the same logic for the support screen, so those strings aren't new. I think if we change the keys, then translators will have to provide a new translations for those strings, right? If so, is there way to change the keys without losing the existing translation?

I see. It's fine for me to keep them as they are, just pinging @AliSoftware since I saw a recent issue with localized strings, to make sure this won't cause any

Thanks for the ping!

Using the exact same strings as the MeViewController is fine… as long as you are really sure that they are used in the same kind of UI context (and that they should thus have the exact same translations in both places, because they really are about the exact same thing)

That being said, instead of copy/pasting the strings and comments in there, and thus duplicating the declaration of those NSLocalizedStrings (between the ones in the MeViewController and the ones here) — which could lead to a later PR changing the comment here, but not in the other place, and lead to more confusion / discrepancies by not having a single source of truth for those code declarations), I'd instead suggest to reference the existing constants from MeViewController (or move those constants into a common class / file / ViewModel / what have you, as long as you declare them only once and reference them from both places)

This might also be a more "dangerous" assumption to make for shorter copies (like "Cancel" and "Log Out") which seems like words that are small and common enough to be used in many other places in the app. So if another place in the app has NSLocalizedString("Cancel", comment: "Some completely different context"), maybe the translators would be tempted to translate it differently for one context than for the other, but if you use the same key for too many different contexts and places in the code, that might lead to translation inconsistencies, like what happened here in the past.

The last point and example is one more key reason why semantic keys are helpful and that it could still be a good idea to switch to them here 😉

If so, is there way to change the keys without losing the existing translation?

Not automatically… but almost.

It happens that GlotPress has a system they call "translation memory", and when it detects a source (English) copy for which they already had a translation for in the past, even if for a different key,… then it is automatically suggested to translators:

See how this recent key in GlotPress is not translated yet… but the "Suggestions from Translation Memory" section already have translations for it, one click away to be used via the "Copy" buttons

This means that, yes, if you change the key in this NSLocalizedString call to start using semantic keys here instead, the translation for that new key will start as empty / untranslated the first time that new key will be imported into GlotPress… but then it will be only one click away for polyglots to translate (picking if from the suggested translation from Translation Memory) as opposed to having to re-translate it from scratch. So that new key will get the existing translation from the old key pretty quickly, so I'd say in the end that's not that big a deal.

Thanks for clarifying @AliSoftware 🙇

I'd instead suggest to reference the existing constants from MeViewController (or move those constants into a common class / file / ViewModel / what have you, as long as you declare them only once and reference them from both places)

I totally agree with this and the log out logic duplication is temporary. Right now the type LogOutActionHandler is used only in SupportTableViewController but I'll open another PR so that MeViewController uses it as well. As a result, those strings declaration will no longer be duplicated.

Gio2018 · 2022-11-18T00:16:37Z

WordPress/Classes/ViewRelated/Support/SupportTableViewController.swift

+        static let wpAccount = NSLocalizedString("WordPress.com Account", comment: "WordPress.com sign-out section header title")
+        static let logOutButtonTitle = NSLocalizedString("Log Out", comment: "Button for confirming logging out from WordPress.com account")


same consideration here about the strings

Same as this reply

WordPress/Jetpack/Classes/System/JetpackWindowManager.swift

salimbraksa · 2022-11-18T16:23:32Z

WordPress/Classes/ViewRelated/Support/SupportTableViewController.swift

+        tableView.tableFooterView = UIView() // remove empty cells
+        if let headerConfig = configuration.meHeaderConfiguration {
+            let headerView = MeHeaderView()
+            headerView.update(with: headerConfig)


Ideally, the MeHeaderView should be initialized with the config param, but the MeHeaderView is used elsewhere and would require some refactoring to be done.

twstokes

This is looking really good to me @salimbraksa! Nice work. 👏 I'll let @Gio2018 have the final say on the review since he started it.

I tested:

Light / Dark mode
a11y / Dynamic Type
Accessing the Contact Support view
Comparing the differences between the view from the migration flow and within the app

One observation:

When the Support view is accessed within the app we allow a landscape orientation. It appears that we don't allow landscape when coming from the migration flow. However, when I rotate my device the Status Bar and the Home Bar still rotate. This is minor, but I wanted to bring it up in case we wanted to address it.

Gio2018

This works great @salimbraksa !

Left a few code comments; other than that I agree with what @twstokes observes:

When the Support view is accessed within the app we allow a landscape orientation. It appears that we don't allow landscape when coming from the migration flow. However, when I rotate my device the Status Bar and the Home Bar still rotate. This is minor, but I wanted to bring it up in case we wanted to address it.

Particularly, I agree that it's minor, but we can probably do a small change to fix the status bar.
This could be something like this:

In MigrationViewControllerFactory we define a subclass of UINavigationController
We override the supportedInterfaceOrientations property of this subclass, to only support .portrait on iPhone
inside makeSupportViewControllerRouter we use the above subclass instead of UINavigationController
wdyt?

Gio2018 · 2022-11-18T21:47:02Z

WordPress/Classes/ViewRelated/Me/Views/Header/MeHeaderView.swift

+extension MeHeaderView.Configuration {
+
+    init(account: WPAccount) {
+        self.init(
+            gravatarEmail: account.email,
+            username: account.username,
+            displayName: account.displayName
+        )
+    }
+}


Likely a matter of personal preference, so feel free to ignore this comment, but I find it a bit confusing to have a nested typer that then has an extension outside the containing type. Maybe we could move Configuration outside MeHeaderView? or just add the initializer in the type (or initializers, if we want to preserve the init with all the params)?

Regarding the nested type, it is inspired from some UIKit types, like UIButton.Configuration, but there are other types that aren't nested, like UINavigationBarAppearance, so I guess it's a matter of personal preference.

Would like to hear what @twstokes thinks about this and if he prefers Configuration to be outside MeHeaderView then we make it a convention moving forward.

👋 I personally don't mind nesting types that are tightly coupled with a parent type (e.g. Error, Configuration), though I recall the defaults of SwiftLint complaining and there being discussions as to why.

What jumps out to me most is

extension MeHeaderView.Configuration { and extension SupportTableViewController.Configuration {.

Since they have the same scope and are in the same file I'd bundle them with the type definition like @Gio2018 proposes:

or just add the initializer in the type (or initializers, if we want to preserve the init with all the params)?

then we make it a convention moving forward.

That'd be great to land on one!

The only reason behind the MeHeaderView.Configuration is add a convenience init without removing the default one the compiler generates. Here is an article about adding convenience inits to structs.

That being said, I pushed a commit 4417b15 adding the following changes:

Moved the Configuration type outside its parent type. For both MeHeaderView and SupportTableViewController. The types are named MeHeaderViewConfiguration and SupportTableViewController respectively.

I kept the convenience init(account: WPAccount) in extension MeHeaderViewConfiguration so that the default init isn't removed.

I bundled static func currentAccountConfiguration() with SupportTableViewControllerConfiguration. Previously it was in a separate extension.

so that the default init isn't removed.

These changes make sense, but I wanted to ask: Is the default init needed somewhere in the codebase currently?

For now, the default init is only used by the convenience init init(account: WPAccount). Having the default init can also help us mock MeHeaderViewConfiguration in case we need to write unit tests.

Gio2018 · 2022-11-18T21:51:36Z

WordPress/Classes/ViewRelated/Support/SupportTableViewController+Configuration.swift

+    }
+}
+
+extension SupportTableViewController.Configuration {


same considerations to Configuration in MeHeaderView apply here

salimbraksa · 2022-11-21T13:54:18Z

Thanks @twstokes @Gio2018 for your feedback and catching the orientation issue. This commit 69738d0 resolves the orientation issue by tweaking the preferredInterfaceOrientationForPresentation and supportedInterfaceOrientations implementations in MigrationNavigationController.

The idea is that MigrationNavigationController is only responsible for forcing the portrait orientation on its children, but presented view controllers should decide their orientation.

twstokes

I tested the changes from 69738d0 by rotating the Support view and it worked great. The Support view supported landscape and upon dismissal the view smoothly transitioned back to portrait for the migration flow.

Gio2018

This works as described, but, because we are only supporting portrait on iPhone in the migration flow, the animations between landscape and portrait feel a bit odd, particularly when we go back to the welcome screen (see animation below)
I think it would be preferable to just not support landscape at all, but since this could just be my personal feeling, I'll let @shaunandrews and @osullivanchris chime in for this.

osullivanchris · 2022-11-22T10:35:11Z

This works as described, but, because we are only supporting portrait on iPhone in the migration flow, the animations between landscape and portrait feel a bit odd, particularly when we go back to the welcome screen (see animation below)
I think it would be preferable to just not support landscape at all, but since this could just be my personal feeling, I'll let @shaunandrews and @osullivanchris chime in for this.

I see what you mean. It looks a bit funky in this case. But, maybe someone has a valid reason to type in landscape mode. Maybe when they want to write to support, or if they're on iPad. I think its overkill to add a portrait lock to those views. When they go back to the welcome screen its interesting that it displays in landscape for a moment then realising its locked to portrait. If it could immediately be in portrait mode that might be better. But not worth spending lots of time on.

salimbraksa · 2022-11-22T11:46:37Z

...maybe someone has a valid reason to type in landscape mode. Maybe when they want to write to support.

That's a good point 👍

...If it could immediately be in portrait mode that might be better.

I can look into this and see if it's easily doable. But I guess it's not blocking this PR, right? @Gio2018

twstokes · 2022-11-22T13:40:22Z

But, maybe someone has a valid reason to type in landscape mode.

I second this, and thought it more as a feature since the Support view already supports landscape well.

Gio2018 · 2022-11-22T17:01:32Z

Thanks for reviewing this @osullivanchris !!

A couple of additional thoughts

or if they're on iPad

in this case, we are not restricting to portrait only. The main reasoning behind avoiding landscape on iPhone was because most of the content of the migration views was not going to be visible and required to scroll. But then again, I can see both sides

Gio2018 · 2022-11-22T17:06:33Z

...maybe someone has a valid reason to type in landscape mode. Maybe when they want to write to support.

That's a good point 👍

...If it could immediately be in portrait mode that might be better.

I can look into this and see if it's easily doable. But I guess it's not blocking this PR, right? @Gio2018

yes not a blocker at all!

osullivanchris · 2022-11-22T17:20:40Z

Yep no blockers here for me. Whether we leave it as is, lock it to portrait (on iPhone), or do the small tweak that I suggested, I don't think the risk is high here. I'd prefer to ship as is rather than spend more than an hour on any fix 😄

salimbraksa added 8 commits November 17, 2022 01:05

Move MeHeaderView to another directory

2e24951

Add Configuration type for MeHeaderView

4641e7d

Add Configuration type for SupportTableViewController

ec2594a

Display Me Header in SupportTableViewController

02f04a5

Reuse Log Out Logic from MeViewController

91e0e0c

Add showsLogOutButton flag to SupportTableViewController Configuration

74eba9d

Hide or show log out button depending on the showsLogOutButton flag

5cb78c8

Add showsLogsSection flag to hide / show support information sections

82a4472

salimbraksa force-pushed the task/migration-support-screen branch from 16e8e41 to bedc8b4 Compare November 17, 2022 16:19

salimbraksa added 2 commits November 17, 2022 17:25

Progress presenting the support screen from the migration flow

b489bf1

Sync branch with trunk

6cf8bea

salimbraksa force-pushed the task/migration-support-screen branch from bedc8b4 to 6cf8bea Compare November 17, 2022 16:31

Add Support Screen presentation logic

3a3f519

salimbraksa force-pushed the task/migration-support-screen branch from 2a60c8a to 3a3f519 Compare November 17, 2022 20:24

Gio2018 reviewed Nov 18, 2022

View reviewed changes

Revert MigrationNavigationController and Podfile.lock changes

56cad8f

salimbraksa force-pushed the task/migration-support-screen branch from 39fe89d to 56cad8f Compare November 18, 2022 14:02

Fix trailing whitespace violation

94c4576

salimbraksa self-assigned this Nov 18, 2022

salimbraksa added [Type] Task Jetpack Focus labels Nov 18, 2022

salimbraksa added this to the 21.3 milestone Nov 18, 2022

salimbraksa added 2 commits November 18, 2022 15:20

Display support screen with insetGrouped style

abb923f

Revert shouldShowMigrationUI to false

f6f51b7

salimbraksa requested a review from Gio2018 November 18, 2022 16:01

salimbraksa marked this pull request as ready for review November 18, 2022 16:01

salimbraksa requested a review from twstokes November 18, 2022 16:03

salimbraksa commented Nov 18, 2022

View reviewed changes

twstokes reviewed Nov 18, 2022

View reviewed changes

Gio2018 reviewed Nov 18, 2022

View reviewed changes

Force portrait orientation for migration screens only

69738d0

salimbraksa force-pushed the task/migration-support-screen branch from b5ad613 to 69738d0 Compare November 21, 2022 13:46

twstokes approved these changes Nov 21, 2022

View reviewed changes

salimbraksa requested a review from Gio2018 November 21, 2022 15:13

Move Configuration types outside their parent type

4417b15

Gio2018 reviewed Nov 22, 2022

View reviewed changes

salimbraksa merged commit d0a97b0 into trunk Nov 22, 2022

salimbraksa deleted the task/migration-support-screen branch November 22, 2022 18:26

		static let wpAccount = NSLocalizedString("WordPress.com Account", comment: "WordPress.com sign-out section header title")
		static let logOutButtonTitle = NSLocalizedString("Log Out", comment: "Button for confirming logging out from WordPress.com account")

[Jetpack Content Migration Flow] Add Help Screen #19618

[Jetpack Content Migration Flow] Add Help Screen #19618

Uh oh!

Conversation

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

Uh oh!

Description

Upcoming PR

Screenshots

Test Instructions:

Regression Notes

PR submission checklist:

Uh oh!

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

Uh oh!

Uh oh!

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

Uh oh!

Uh oh!

Gio2018 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

AliSoftware Nov 18, 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

twstokes left a comment

Choose a reason for hiding this comment

Uh oh!

Gio2018 left a comment • 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

twstokes Nov 21, 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 21, 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!

Choose a reason for hiding this comment

Uh oh!

salimbraksa Nov 21, 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!

Choose a reason for hiding this comment

Uh oh!

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

Uh oh!

Uh oh!

twstokes left a comment

Choose a reason for hiding this comment

Uh oh!

Gio2018 left a comment

Choose a reason for hiding this comment

Uh oh!

osullivanchris commented Nov 22, 2022

Uh oh!

salimbraksa commented Nov 22, 2022

salimbraksa commented Nov 17, 2022 •

edited

Loading

wpmobilebot commented Nov 17, 2022 •

edited

Loading

wpmobilebot commented Nov 17, 2022 •

edited

Loading

AliSoftware Nov 18, 2022 •

edited

Loading

Gio2018 left a comment •

edited

Loading

twstokes Nov 21, 2022 •

edited

Loading

salimbraksa Nov 21, 2022 •

edited

Loading

salimbraksa Nov 21, 2022 •

edited

Loading

salimbraksa commented Nov 21, 2022 •

edited

Loading