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.

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

Account Creation: Fix broken flow after signing up from the WordPress app #21751

Merged
merged 10 commits into from
Oct 15, 2023
+105 −35
Merged

Account Creation: Fix broken flow after signing up from the WordPress app #21751

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
38687a6
Update: only navigate to the reader after sign up if it's available
hassaanelgarem Oct 12, 2023
b87ed54
Update: introduce operation queue logic to fetching remote flags
hassaanelgarem Oct 12, 2023
44d318a
Update: fix strong reference to self in closure
hassaanelgarem Oct 12, 2023
0815f63
Update: fix new site creation flow after sign up on WP
hassaanelgarem Oct 12, 2023
fee5a3e
Revert "Update: fix new site creation flow after sign up on WP"
hassaanelgarem Oct 12, 2023
e65bf8c
Tests: fix failures in `JetpackFeaturesRemovalCoordinatorTests`
hassaanelgarem Oct 12, 2023
c4bd935
Tests: fix failures in `RemoteFeatureFlagTests`
hassaanelgarem Oct 12, 2023
e5b010b
Update: fix typo
hassaanelgarem Oct 15, 2023
10ed17e
Add: release note
hassaanelgarem Oct 15, 2023
67e1e15
Merge branch 'trunk' into fix/21746-wp-sign-up-happy-path
hassaanelgarem Oct 15, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions RELEASE-NOTES.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
23.5
-----
* [*] Fix a crash when the blog's blogging prompt settings contain invalid JSON [#21677]
* [*] [WordPress-only] Fixes an issue where users would land on the Reader after signup while it should not be accessible. [#21751]
* [*] Block Editor: Split formatted text on triple Enter [https://github.com/WordPress/gutenberg/pull/53354]
* [*] Block Editor: Quote block: Ensure border is visible with block-based themes in dark [https://github.com/WordPress/gutenberg/pull/54964]
* [*] (Internal) Remove .nativePhotoPicker feature flag and the disabled code [#21681](https://github.com/wordpress-mobile/WordPress-iOS/pull/21681)
Expand Down
67 changes: 57 additions & 10 deletions WordPress/Classes/Stores/RemoteFeatureFlagStore.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,12 @@ class RemoteFeatureFlagStore {
/// Thread Safety Coordinator
private var queue: DispatchQueue
private var persistenceStore: UserPersistentRepository
private lazy var operationQueue: OperationQueue = {
var queue = OperationQueue()
queue.name = "Feature Flags Refresh Queue"
queue.maxConcurrentOperationCount = 1
return queue
}()

init(queue: DispatchQueue = .remoteFeatureFlagStoreQueue,
persistenceStore: UserPersistentRepository = UserDefaults.standard) {
Expand All @@ -23,18 +29,19 @@ class RemoteFeatureFlagStore {
/// - Parameter callback: An optional callback that can be used to update UI following the fetch. It is not called on the UI thread.
public func update(using remote: FeatureFlagRemote = FeatureFlagRemote(wordPressComRestApi: WordPressComRestApi.defaultApi()),
then callback: FetchCallback? = nil) {
DDLogInfo("🚩 Updating Remote Feature Flags with Device ID: \(deviceID)")
remote.getRemoteFeatureFlags(forDeviceId: deviceID) { [weak self] result in
let refreshOperation = FetchRemoteFeatureFlagsOperation(remote: remote,
deviceID: deviceID,
completion: { [weak self] result in
switch result {
case .success(let flags):
self?.cache = flags.dictionaryValue
DDLogInfo("🚩 Successfully updated local feature flags: \(flags)")
callback?()
case .failure(let error):
DDLogError("🚩 Unable to update Feature Flag Store: \(error.localizedDescription)")
callback?()
case .success(let flags):
self?.cache = flags
callback?()
case .failure:
callback?()
}
}
})
operationQueue.cancelAllOperations()
operationQueue.addOperation(refreshOperation)
}

/// Checks if the local cache has a value for a given `FeatureFlag`
Expand Down Expand Up @@ -85,3 +92,43 @@ extension RemoteFeatureFlagStore {
}
}
}

fileprivate final class FetchRemoteFeatureFlagsOperation: AsyncOperation {
private let remote: FeatureFlagRemote
private let deviceID: String
private let completion: OperationCompletionHandler

typealias OperationCompletionHandler = (Result<[String: Bool], Error>) -> ()

enum OperationError: Error {
case operationCanceled
}

init(remote: FeatureFlagRemote, deviceID: String, completion: @escaping OperationCompletionHandler) {
self.remote = remote
self.deviceID = deviceID
self.completion = completion
super.init()
}

override func main() {
DDLogInfo("🚩 Updating Remote Feature Flags with Device ID: \(deviceID)")
remote.getRemoteFeatureFlags(forDeviceId: deviceID) { [weak self] result in
if self?.isCancelled == true {
self?.state = .isFinished
DDLogInfo("🚩 Feature flags update operation canceled")
self?.completion(.failure(OperationError.operationCanceled))
return
}
switch result {
case .success(let flags):
DDLogInfo("🚩 Successfully updated local feature flags: \(flags.dictionaryValue)")
self?.completion(.success(flags.dictionaryValue))
case .failure(let error):
DDLogError("🚩 Unable to update Feature Flag Store: \(error.localizedDescription)")
self?.completion(.failure(error))
}
self?.state = .isFinished
Comment on lines +117 to +131
Copy link
Contributor

Choose a reason for hiding this comment

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

Unless I'm reading the code wrong, if self is nil, nothing will happen other than console logs.

Have you considered returning early with guard let self else { return }?

}
}
}
12 changes: 12 additions & 0 deletions WordPress/Classes/System/RootViewCoordinator.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -87,6 +87,18 @@ class RootViewCoordinator {
self.rootViewPresenter = nil
}

func showPostSignUpTabForNoSites() {
let appUIType = Self.appUIType(featureFlagStore: featureFlagStore)
switch appUIType {
case .normal:
rootViewPresenter?.showReaderTab()
case .simplified:
fallthrough
case .staticScreens:
rootViewPresenter?.showMySitesTab()
}
}

private func createPresenter(_ appType: AppUIType) -> RootViewPresenter {
switch appType {
case .normal:
Expand Down
2 changes: 1 addition & 1 deletion ...asses/ViewRelated/NUX/Post Signup Interstitial/PostSignUpInterstitialViewController.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -104,7 +104,7 @@ class PostSignUpInterstitialViewController: UIViewController {
@IBAction func cancel(_ sender: Any) {
dismiss?(.none)

RootViewCoordinator.sharedPresenter.showReaderTab()
RootViewCoordinator.shared.showPostSignUpTabForNoSites()

tracker.track(click: .dismiss, ifTrackingNotEnabled: {
WPAnalytics.track(.welcomeNoSitesInterstitialDismissed)
Expand Down
52 changes: 31 additions & 21 deletions WordPress/WordPressTest/JetpackFeaturesRemovalCoordinatorTests.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,7 @@ final class JetpackFeaturesRemovalCoordinatorTests: CoreDataTestCase {
let store = RemoteFeatureFlagStore(persistenceStore: mockUserDefaults)
let flags = generateFlags(phaseOne: false, phaseTwo: false, phaseThree: false, phaseFour: false, phaseNewUsers: false, phaseSelfHosted: false)
let remote = MockFeatureFlagRemote(flags: flags)
store.update(using: remote)
store.update(using: remote, waitOn: self)

// When
let phase = JetpackFeaturesRemovalCoordinator.generalPhase(featureFlagStore: store)
Expand All @@ -38,7 +38,7 @@ final class JetpackFeaturesRemovalCoordinatorTests: CoreDataTestCase {
let store = RemoteFeatureFlagStore(persistenceStore: mockUserDefaults)
let flags = generateFlags(phaseOne: true, phaseTwo: false, phaseThree: false, phaseFour: false, phaseNewUsers: false, phaseSelfHosted: false)
let remote = MockFeatureFlagRemote(flags: flags)
store.update(using: remote)
store.update(using: remote, waitOn: self)

// When
let phase = JetpackFeaturesRemovalCoordinator.generalPhase(featureFlagStore: store)
Expand All @@ -52,7 +52,7 @@ final class JetpackFeaturesRemovalCoordinatorTests: CoreDataTestCase {
let store = RemoteFeatureFlagStore(persistenceStore: mockUserDefaults)
let flags = generateFlags(phaseOne: false, phaseTwo: false, phaseThree: false, phaseFour: false, phaseNewUsers: true, phaseSelfHosted: false)
let remote = MockFeatureFlagRemote(flags: flags)
store.update(using: remote)
store.update(using: remote, waitOn: self)

// When
let phase = JetpackFeaturesRemovalCoordinator.generalPhase(featureFlagStore: store)
Expand All @@ -66,7 +66,7 @@ final class JetpackFeaturesRemovalCoordinatorTests: CoreDataTestCase {
let store = RemoteFeatureFlagStore(persistenceStore: mockUserDefaults)
let flags = generateFlags(phaseOne: true, phaseTwo: true, phaseThree: true, phaseFour: true, phaseNewUsers: true, phaseSelfHosted: false)
let remote = MockFeatureFlagRemote(flags: flags)
store.update(using: remote)
store.update(using: remote, waitOn: self)

// When
let phase = JetpackFeaturesRemovalCoordinator.generalPhase(featureFlagStore: store)
Expand All @@ -81,7 +81,7 @@ final class JetpackFeaturesRemovalCoordinatorTests: CoreDataTestCase {
let store = RemoteFeatureFlagStore(persistenceStore: mockUserDefaults)
let flags = generateFlags(phaseOne: false, phaseTwo: false, phaseThree: false, phaseFour: false, phaseNewUsers: false, phaseSelfHosted: true)
let remote = MockFeatureFlagRemote(flags: flags)
store.update(using: remote)
store.update(using: remote, waitOn: self)

// When
let phase = JetpackFeaturesRemovalCoordinator.generalPhase(featureFlagStore: store)
Expand All @@ -95,7 +95,7 @@ final class JetpackFeaturesRemovalCoordinatorTests: CoreDataTestCase {
let store = RemoteFeatureFlagStore(persistenceStore: mockUserDefaults)
let flags = generateFlags(phaseOne: true, phaseTwo: false, phaseThree: false, phaseFour: false, phaseNewUsers: false, phaseSelfHosted: true)
let remote = MockFeatureFlagRemote(flags: flags)
store.update(using: remote)
store.update(using: remote, waitOn: self)

// When
let phase = JetpackFeaturesRemovalCoordinator.generalPhase(featureFlagStore: store)
Expand All @@ -109,7 +109,7 @@ final class JetpackFeaturesRemovalCoordinatorTests: CoreDataTestCase {
let store = RemoteFeatureFlagStore(persistenceStore: mockUserDefaults)
let flags = generateFlags(phaseOne: true, phaseTwo: false, phaseThree: false, phaseFour: false, phaseNewUsers: false, phaseSelfHosted: false)
let remote = MockFeatureFlagRemote(flags: flags)
store.update(using: remote)
store.update(using: remote, waitOn: self)

// When
let phase = JetpackFeaturesRemovalCoordinator.generalPhase(featureFlagStore: store)
Expand All @@ -123,7 +123,7 @@ final class JetpackFeaturesRemovalCoordinatorTests: CoreDataTestCase {
let store = RemoteFeatureFlagStore(persistenceStore: mockUserDefaults)
let flags = generateFlags(phaseOne: false, phaseTwo: true, phaseThree: false, phaseFour: false, phaseNewUsers: false, phaseSelfHosted: false)
let remote = MockFeatureFlagRemote(flags: flags)
store.update(using: remote)
store.update(using: remote, waitOn: self)

// When
let phase = JetpackFeaturesRemovalCoordinator.generalPhase(featureFlagStore: store)
Expand All @@ -137,7 +137,7 @@ final class JetpackFeaturesRemovalCoordinatorTests: CoreDataTestCase {
let store = RemoteFeatureFlagStore(persistenceStore: mockUserDefaults)
let flags = generateFlags(phaseOne: true, phaseTwo: true, phaseThree: false, phaseFour: false, phaseNewUsers: false, phaseSelfHosted: false)
let remote = MockFeatureFlagRemote(flags: flags)
store.update(using: remote)
store.update(using: remote, waitOn: self)

// When
let phase = JetpackFeaturesRemovalCoordinator.generalPhase(featureFlagStore: store)
Expand All @@ -151,7 +151,7 @@ final class JetpackFeaturesRemovalCoordinatorTests: CoreDataTestCase {
let store = RemoteFeatureFlagStore(persistenceStore: mockUserDefaults)
let flags = generateFlags(phaseOne: false, phaseTwo: false, phaseThree: true, phaseFour: false, phaseNewUsers: false, phaseSelfHosted: false)
let remote = MockFeatureFlagRemote(flags: flags)
store.update(using: remote)
store.update(using: remote, waitOn: self)

// When
let phase = JetpackFeaturesRemovalCoordinator.generalPhase(featureFlagStore: store)
Expand All @@ -165,7 +165,7 @@ final class JetpackFeaturesRemovalCoordinatorTests: CoreDataTestCase {
let store = RemoteFeatureFlagStore(persistenceStore: mockUserDefaults)
let flags = generateFlags(phaseOne: true, phaseTwo: true, phaseThree: true, phaseFour: false, phaseNewUsers: false, phaseSelfHosted: false)
let remote = MockFeatureFlagRemote(flags: flags)
store.update(using: remote)
store.update(using: remote, waitOn: self)

// When
let phase = JetpackFeaturesRemovalCoordinator.generalPhase(featureFlagStore: store)
Expand All @@ -179,7 +179,7 @@ final class JetpackFeaturesRemovalCoordinatorTests: CoreDataTestCase {
let store = RemoteFeatureFlagStore(persistenceStore: mockUserDefaults)
let flags = generateFlags(phaseOne: false, phaseTwo: false, phaseThree: false, phaseFour: true, phaseNewUsers: false, phaseSelfHosted: false)
let remote = MockFeatureFlagRemote(flags: flags)
store.update(using: remote)
store.update(using: remote, waitOn: self)

// When
let phase = JetpackFeaturesRemovalCoordinator.generalPhase(featureFlagStore: store)
Expand All @@ -193,7 +193,7 @@ final class JetpackFeaturesRemovalCoordinatorTests: CoreDataTestCase {
let store = RemoteFeatureFlagStore(persistenceStore: mockUserDefaults)
let flags = generateFlags(phaseOne: true, phaseTwo: true, phaseThree: true, phaseFour: true, phaseNewUsers: false, phaseSelfHosted: false)
let remote = MockFeatureFlagRemote(flags: flags)
store.update(using: remote)
store.update(using: remote, waitOn: self)

// When
let phase = JetpackFeaturesRemovalCoordinator.generalPhase(featureFlagStore: store)
Expand All @@ -209,7 +209,7 @@ final class JetpackFeaturesRemovalCoordinatorTests: CoreDataTestCase {
let store = RemoteFeatureFlagStore(persistenceStore: mockUserDefaults)
let flags = generateFlags(phaseOne: false, phaseTwo: false, phaseThree: false, phaseFour: false, phaseNewUsers: false, phaseSelfHosted: false)
let remote = MockFeatureFlagRemote(flags: flags)
store.update(using: remote)
store.update(using: remote, waitOn: self)

// When
let phase = JetpackFeaturesRemovalCoordinator.siteCreationPhase(featureFlagStore: store)
Expand All @@ -225,7 +225,7 @@ final class JetpackFeaturesRemovalCoordinatorTests: CoreDataTestCase {
// When
var flags = generateFlags(phaseOne: true, phaseTwo: false, phaseThree: false, phaseFour: false, phaseNewUsers: false, phaseSelfHosted: false)
let remote = MockFeatureFlagRemote(flags: flags)
store.update(using: remote)
store.update(using: remote, waitOn: self)
var phase = JetpackFeaturesRemovalCoordinator.siteCreationPhase(featureFlagStore: store)

// Then
Expand All @@ -234,7 +234,7 @@ final class JetpackFeaturesRemovalCoordinatorTests: CoreDataTestCase {
// When
flags = generateFlags(phaseOne: false, phaseTwo: true, phaseThree: false, phaseFour: false, phaseNewUsers: false, phaseSelfHosted: false)
remote.flags = flags
store.update(using: remote)
store.update(using: remote, waitOn: self)
phase = JetpackFeaturesRemovalCoordinator.siteCreationPhase(featureFlagStore: store)

// Then
Expand All @@ -243,7 +243,7 @@ final class JetpackFeaturesRemovalCoordinatorTests: CoreDataTestCase {
// When
flags = generateFlags(phaseOne: false, phaseTwo: false, phaseThree: true, phaseFour: false, phaseNewUsers: false, phaseSelfHosted: false)
remote.flags = flags
store.update(using: remote)
store.update(using: remote, waitOn: self)
phase = JetpackFeaturesRemovalCoordinator.siteCreationPhase(featureFlagStore: store)

// Then
Expand All @@ -257,7 +257,7 @@ final class JetpackFeaturesRemovalCoordinatorTests: CoreDataTestCase {
// When
var flags = generateFlags(phaseOne: false, phaseTwo: false, phaseThree: false, phaseFour: true, phaseNewUsers: false, phaseSelfHosted: false)
let remote = MockFeatureFlagRemote(flags: flags)
store.update(using: remote)
store.update(using: remote, waitOn: self)
var phase = JetpackFeaturesRemovalCoordinator.siteCreationPhase(featureFlagStore: store)

// Then
Expand All @@ -266,7 +266,7 @@ final class JetpackFeaturesRemovalCoordinatorTests: CoreDataTestCase {
// When
flags = generateFlags(phaseOne: false, phaseTwo: false, phaseThree: false, phaseFour: false, phaseNewUsers: true, phaseSelfHosted: false)
remote.flags = flags
store.update(using: remote)
store.update(using: remote, waitOn: self)
phase = JetpackFeaturesRemovalCoordinator.siteCreationPhase(featureFlagStore: store)

// Then
Expand All @@ -280,7 +280,7 @@ final class JetpackFeaturesRemovalCoordinatorTests: CoreDataTestCase {
// When
var flags = generateFlags(phaseOne: true, phaseTwo: true, phaseThree: true, phaseFour: true, phaseNewUsers: false, phaseSelfHosted: false)
let remote = MockFeatureFlagRemote(flags: flags)
store.update(using: remote)
store.update(using: remote, waitOn: self)
var phase = JetpackFeaturesRemovalCoordinator.siteCreationPhase(featureFlagStore: store)

// Then
Expand All @@ -289,7 +289,7 @@ final class JetpackFeaturesRemovalCoordinatorTests: CoreDataTestCase {
// When
flags = generateFlags(phaseOne: true, phaseTwo: true, phaseThree: true, phaseFour: false, phaseNewUsers: true, phaseSelfHosted: false)
remote.flags = flags
store.update(using: remote)
store.update(using: remote, waitOn: self)
phase = JetpackFeaturesRemovalCoordinator.siteCreationPhase(featureFlagStore: store)

// Then
Expand Down Expand Up @@ -346,3 +346,13 @@ private extension Date {
from: self)
}
}

internal extension RemoteFeatureFlagStore {
func update(using remote: FeatureFlagRemote, waitOn test: XCTestCase) {
let exp = test.expectation(description: "Store finishes update")
update(using: remote) {
exp.fulfill()
}
test.wait(for: [exp], timeout: 1)
}
}
Comment on lines +350 to +358
Copy link
Contributor

Choose a reason for hiding this comment

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

Interesting approach. For a moment I got worried because it looked like the test was no longer testing application code, but then I looked at the implementation and realized it called the app code internally.

This has definitely value because it DRYes what would otherwise be a lot of repeated async expectations.

I wonder if the DRY could be pushed even further. Most tests have a similar arrange structure:

        let store = ...
        let flags = ...
        let remote = ...
        store.update(using: remote, waitOn: self)

6 changes: 3 additions & 3 deletions WordPress/WordPressTest/RemoteFeatureFlagTests.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,8 +22,8 @@ class RemoteFeatureFlagTests: XCTestCase {
exp.fulfill()
}

store.update(using: mock)
store.update(using: mock)
store.update(using: mock, waitOn: self)
store.update(using: mock, waitOn: self)

wait(for: [exp], timeout: 1.0)
}
Expand All @@ -42,7 +42,7 @@ class RemoteFeatureFlagTests: XCTestCase {
let mock = MockFeatureFlagRemote(mockFlags: MockFeatureFlag.remoteCases)
let store = RemoteFeatureFlagStore(persistenceStore: mockUserDefaults)

store.update(using: mock)
store.update(using: mock, waitOn: self)

// All of the remotely defined values should be present
XCTAssertTrue(store.hasValue(for: MockFeatureFlag.remotelyEnabledLocallyEnabledFeature.remoteKey))
Expand Down