Skip to content

Commit 81ab990

Browse files
jaclyncjaclync
authored
Merge pull request #5467 from woocommerce/feat/5364-jcp-workaround-behind-feature-flag
Jetpack CP: only perform JCP workaround behind a feature flag
2 parents e5f3bdd + 0e69bd6 commit 81ab990

File tree

5 files changed

+106
-25
lines changed

5 files changed

+106
-25
lines changed

‎WooCommerce/Classes/Authentication/Epilogue/StorePickerViewController.swift‎

Lines changed: 4 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -274,7 +274,10 @@ private extension StorePickerViewController {
274274
//
275275
private extension StorePickerViewController {
276276
func synchronizeSites(onCompletion: @escaping (Result<Void, Error>) -> Void) {
277-
let action = AccountAction.synchronizeSites(selectedSiteID: currentlySelectedSite?.siteID, onCompletion: onCompletion)
277+
let action = AccountAction
278+
.synchronizeSites(selectedSiteID: currentlySelectedSite?.siteID,
279+
isJetpackConnectionPackageSupported: ServiceLocator.featureFlagService.isFeatureFlagEnabled(.jetpackConnectionPackageSupport),
280+
onCompletion: onCompletion)
278281
ServiceLocator.stores.dispatch(action)
279282
}
280283
}

‎WooCommerce/Classes/Yosemite/DefaultStoresManager.swift‎

Lines changed: 8 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -284,7 +284,10 @@ private extension DefaultStoresManager {
284284
/// Synchronizes the WordPress.com Sites, associated with the current credentials.
285285
///
286286
func synchronizeSites(onCompletion: @escaping (Result<Void, Error>) -> Void) {
287-
let action = AccountAction.synchronizeSites(selectedSiteID: sessionManager.defaultStoreID, onCompletion: onCompletion)
287+
let action = AccountAction
288+
.synchronizeSites(selectedSiteID: sessionManager.defaultStoreID,
289+
isJetpackConnectionPackageSupported: ServiceLocator.featureFlagService.isFeatureFlagEnabled(.jetpackConnectionPackageSupport),
290+
onCompletion: onCompletion)
288291
dispatch(action)
289292
}
290293

@@ -449,7 +452,10 @@ private extension DefaultStoresManager {
449452
/// If the site does not exist in storage, it synchronizes the site asynchronously.
450453
///
451454
func restoreSessionSiteAndSynchronizeIfNeeded(with siteID: Int64) {
452-
let action = AccountAction.loadAndSynchronizeSiteIfNeeded(siteID: siteID) { [weak self] result in
455+
let isJCPEnabled = ServiceLocator.featureFlagService.isFeatureFlagEnabled(.jetpackConnectionPackageSupport)
456+
let action = AccountAction
457+
.loadAndSynchronizeSiteIfNeeded(siteID: siteID,
458+
isJetpackConnectionPackageSupported: isJCPEnabled) { [weak self] result in
453459
guard let self = self else { return }
454460
guard case .success(let site) = result else {
455461
return

‎Yosemite/Yosemite/Actions/AccountAction.swift‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -7,10 +7,10 @@ import Networking
77
//
88
public enum AccountAction: Action {
99
case loadAccount(userID: Int64, onCompletion: (Account?) -> Void)
10-
case loadAndSynchronizeSiteIfNeeded(siteID: Int64, onCompletion: (Result<Site, Error>) -> Void)
10+
case loadAndSynchronizeSiteIfNeeded(siteID: Int64, isJetpackConnectionPackageSupported: Bool, onCompletion: (Result<Site, Error>) -> Void)
1111
case synchronizeAccount(onCompletion: (Result<Account, Error>) -> Void)
1212
case synchronizeAccountSettings(userID: Int64, onCompletion: (Result<AccountSettings, Error>) -> Void)
13-
case synchronizeSites(selectedSiteID: Int64?, onCompletion: (Result<Void, Error>) -> Void)
13+
case synchronizeSites(selectedSiteID: Int64?, isJetpackConnectionPackageSupported: Bool, onCompletion: (Result<Void, Error>) -> Void)
1414
case synchronizeSitePlan(siteID: Int64, onCompletion: (Result<Void, Error>) -> Void)
1515
case updateAccountSettings(userID: Int64, tracksOptOut: Bool, onCompletion: (Result<Void, Error>) -> Void)
1616
}

‎Yosemite/Yosemite/Stores/AccountStore.swift‎

Lines changed: 10 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -43,14 +43,16 @@ public class AccountStore: Store {
4343
switch action {
4444
case .loadAccount(let userID, let onCompletion):
4545
loadAccount(userID: userID, onCompletion: onCompletion)
46-
case .loadAndSynchronizeSiteIfNeeded(let siteID, let onCompletion):
47-
loadAndSynchronizeSiteIfNeeded(siteID: siteID, onCompletion: onCompletion)
46+
case .loadAndSynchronizeSiteIfNeeded(let siteID, let isJetpackConnectionPackageSupported, let onCompletion):
47+
loadAndSynchronizeSiteIfNeeded(siteID: siteID, isJetpackConnectionPackageSupported: isJetpackConnectionPackageSupported, onCompletion: onCompletion)
4848
case .synchronizeAccount(let onCompletion):
4949
synchronizeAccount(onCompletion: onCompletion)
5050
case .synchronizeAccountSettings(let userID, let onCompletion):
5151
synchronizeAccountSettings(userID: userID, onCompletion: onCompletion)
52-
case .synchronizeSites(let selectedSiteID, let onCompletion):
53-
synchronizeSites(selectedSiteID: selectedSiteID, onCompletion: onCompletion)
52+
case .synchronizeSites(let selectedSiteID, let isJetpackConnectionPackageSupported, let onCompletion):
53+
synchronizeSites(selectedSiteID: selectedSiteID,
54+
isJetpackConnectionPackageSupported: isJetpackConnectionPackageSupported,
55+
onCompletion: onCompletion)
5456
case .synchronizeSitePlan(let siteID, let onCompletion):
5557
synchronizeSitePlan(siteID: siteID, onCompletion: onCompletion)
5658
case .updateAccountSettings(let userID, let tracksOptOut, let onCompletion):
@@ -92,11 +94,11 @@ private extension AccountStore {
9294

9395
/// Returns the site if it exists in storage already. Otherwise, it synchronizes the WordPress.com sites and returns the site if it exists.
9496
///
95-
func loadAndSynchronizeSiteIfNeeded(siteID: Int64, onCompletion: @escaping (Result<Site, Error>) -> Void) {
97+
func loadAndSynchronizeSiteIfNeeded(siteID: Int64, isJetpackConnectionPackageSupported: Bool, onCompletion: @escaping (Result<Site, Error>) -> Void) {
9698
if let site = storageManager.viewStorage.loadSite(siteID: siteID)?.toReadOnly() {
9799
onCompletion(.success(site))
98100
} else {
99-
synchronizeSites(selectedSiteID: siteID) { [weak self] result in
101+
synchronizeSites(selectedSiteID: siteID, isJetpackConnectionPackageSupported: isJetpackConnectionPackageSupported) { [weak self] result in
100102
guard let self = self else { return }
101103
guard let site = self.storageManager.viewStorage.loadSite(siteID: siteID)?.toReadOnly() else {
102104
return onCompletion(.failure(SynchronizeSiteError.unknownSite))
@@ -108,7 +110,7 @@ private extension AccountStore {
108110

109111
/// Synchronizes the WordPress.com sites associated with the Network's Auth Token.
110112
///
111-
func synchronizeSites(selectedSiteID: Int64?, onCompletion: @escaping (Result<Void, Error>) -> Void) {
113+
func synchronizeSites(selectedSiteID: Int64?, isJetpackConnectionPackageSupported: Bool, onCompletion: @escaping (Result<Void, Error>) -> Void) {
112114
remote.loadSites()
113115
.flatMap { result -> AnyPublisher<Result<[Site], Error>, Never> in
114116
switch result {
@@ -124,7 +126,7 @@ private extension AccountStore {
124126
// As a workaround, we need to make 2 other API requests (ref p91TBi-6lK-p2):
125127
// - Check if WooCommerce plugin is active via `wc/v3/settings` endpoint
126128
// - Fetch site metadata like the site name, description, and URL via `wp/v2/settings` endpoint
127-
if site.isJetpackCPConnected {
129+
if site.isJetpackCPConnected, isJetpackConnectionPackageSupported {
128130
let wcAvailabilityPublisher = self.remote.checkIfWooCommerceIsActive(for: site.siteID)
129131
let wpSiteSettingsPublisher = self.remote.fetchWordPressSiteSettings(for: site.siteID)
130132
return Publishers.Zip3(sitePublisher, wcAvailabilityPublisher, wpSiteSettingsPublisher)

‎Yosemite/YosemiteTests/Stores/AccountStoreTests.swift‎

Lines changed: 82 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -219,7 +219,7 @@ final class AccountStoreTests: XCTestCase {
219219

220220
// When
221221
let result: Result<Void, Error> = waitFor { promise in
222-
let action = AccountAction.synchronizeSites(selectedSiteID: nil) { result in
222+
let action = AccountAction.synchronizeSites(selectedSiteID: nil, isJetpackConnectionPackageSupported: false) { result in
223223
promise(result)
224224
}
225225
store.onAction(action)
@@ -243,7 +243,7 @@ final class AccountStoreTests: XCTestCase {
243243

244244
// When
245245
let result: Result<Void, Error> = waitFor { promise in
246-
let action = AccountAction.synchronizeSites(selectedSiteID: nil) { result in
246+
let action = AccountAction.synchronizeSites(selectedSiteID: nil, isJetpackConnectionPackageSupported: false) { result in
247247
promise(result)
248248
}
249249
store.onAction(action)
@@ -283,7 +283,7 @@ final class AccountStoreTests: XCTestCase {
283283

284284
// When
285285
let result: Result<Void, Error> = waitFor { promise in
286-
let action = AccountAction.synchronizeSites(selectedSiteID: nil) { result in
286+
let action = AccountAction.synchronizeSites(selectedSiteID: nil, isJetpackConnectionPackageSupported: true) { result in
287287
promise(result)
288288
}
289289
store.onAction(action)
@@ -309,6 +309,53 @@ final class AccountStoreTests: XCTestCase {
309309
XCTAssertEqual(jetpackSite.siteID, siteIDOfJetpackSite)
310310
}
311311

312+
/// Verifies that `synchronizeSites` effectively persists all sites when the response contains a JCP site while the feature flag is off.
313+
/// The JCP site is still persisted but the information is not accurate.
314+
///
315+
func test_synchronizeSites_effectively_persists_sites_with_jcp_feature_off() throws {
316+
// Given
317+
let siteIDOfJCPSite = Int64(255)
318+
let siteIDOfJetpackSite = Int64(166)
319+
let remote = MockAccountRemote()
320+
remote.loadSitesResult = .success([
321+
Site.fake().copy(siteID: siteIDOfJCPSite,
322+
name: "old name",
323+
description: "old description",
324+
url: "oldurl",
325+
isJetpackThePluginInstalled: false,
326+
isJetpackConnected: true),
327+
Site.fake().copy(siteID: siteIDOfJetpackSite, isJetpackThePluginInstalled: true, isJetpackConnected: true)
328+
])
329+
330+
let store = AccountStore(dispatcher: dispatcher, storageManager: storageManager, network: network, remote: remote)
331+
XCTAssertEqual(viewStorage.countObjects(ofType: Storage.Site.self), 0)
332+
333+
// When
334+
let result: Result<Void, Error> = waitFor { promise in
335+
let action = AccountAction.synchronizeSites(selectedSiteID: nil, isJetpackConnectionPackageSupported: false) { result in
336+
promise(result)
337+
}
338+
store.onAction(action)
339+
}
340+
341+
// Then
342+
XCTAssertEqual(remote.invocations, [.loadSites])
343+
344+
XCTAssertTrue(result.isSuccess)
345+
XCTAssertEqual(viewStorage.countObjects(ofType: Storage.Site.self), 2)
346+
347+
XCTAssertEqual(viewStorage.countObjects(ofType: Storage.Site.self, matching: jcpSitePredicate), 1)
348+
let jcpSite = try XCTUnwrap(viewStorage.firstObject(ofType: Storage.Site.self, matching: jcpSitePredicate))
349+
XCTAssertEqual(jcpSite.siteID, siteIDOfJCPSite)
350+
XCTAssertEqual(jcpSite.name, "old name")
351+
XCTAssertEqual(jcpSite.tagline, "old description")
352+
XCTAssertEqual(jcpSite.url, "oldurl")
353+
354+
XCTAssertEqual(viewStorage.countObjects(ofType: Storage.Site.self, matching: jetpackSitePredicate), 1)
355+
let jetpackSite = try XCTUnwrap(viewStorage.firstObject(ofType: Storage.Site.self, matching: jetpackSitePredicate))
356+
XCTAssertEqual(jetpackSite.siteID, siteIDOfJetpackSite)
357+
}
358+
312359
/// Verifies that `synchronizeSites` effectively persists a Jetpack Connection Package site with original metadata when WP site settings request fails.
313360
///
314361
func test_synchronizeSites_persists_a_jetpack_cp_site_with_existing_metadata_when_wp_settings_request_fails() throws {
@@ -331,7 +378,7 @@ final class AccountStoreTests: XCTestCase {
331378

332379
// When
333380
let result: Result<Void, Error> = waitFor { promise in
334-
let action = AccountAction.synchronizeSites(selectedSiteID: nil) { result in
381+
let action = AccountAction.synchronizeSites(selectedSiteID: nil, isJetpackConnectionPackageSupported: true) { result in
335382
promise(result)
336383
}
337384
store.onAction(action)
@@ -371,7 +418,7 @@ final class AccountStoreTests: XCTestCase {
371418

372419
// When
373420
let result: Result<Void, Error> = waitFor { promise in
374-
let action = AccountAction.synchronizeSites(selectedSiteID: nil) { result in
421+
let action = AccountAction.synchronizeSites(selectedSiteID: nil, isJetpackConnectionPackageSupported: true) { result in
375422
promise(result)
376423
}
377424
store.onAction(action)
@@ -402,7 +449,7 @@ final class AccountStoreTests: XCTestCase {
402449

403450
// When
404451
let result: Result<Void, Error> = waitFor { promise in
405-
let action = AccountAction.synchronizeSites(selectedSiteID: nil) { result in
452+
let action = AccountAction.synchronizeSites(selectedSiteID: nil, isJetpackConnectionPackageSupported: false) { result in
406453
promise(result)
407454
}
408455
store.onAction(action)
@@ -428,7 +475,7 @@ final class AccountStoreTests: XCTestCase {
428475

429476
// When
430477
let result: Result<Void, Error> = waitFor { promise in
431-
let action = AccountAction.synchronizeSites(selectedSiteID: selectedSiteID) { result in
478+
let action = AccountAction.synchronizeSites(selectedSiteID: selectedSiteID, isJetpackConnectionPackageSupported: false) { result in
432479
promise(result)
433480
}
434481
store.onAction(action)
@@ -504,7 +551,7 @@ final class AccountStoreTests: XCTestCase {
504551
// When
505552
let result: Result<Yosemite.Site, Error> = waitFor { promise in
506553
group.notify(queue: .main) {
507-
let action = AccountAction.loadAndSynchronizeSiteIfNeeded(siteID: siteID) { result in
554+
let action = AccountAction.loadAndSynchronizeSiteIfNeeded(siteID: siteID, isJetpackConnectionPackageSupported: false) { result in
508555
XCTAssertTrue(Thread.isMainThread)
509556
promise(result)
510557
}
@@ -533,7 +580,7 @@ final class AccountStoreTests: XCTestCase {
533580
// When
534581
let result: Result<Yosemite.Site, Error> = waitFor { promise in
535582
group.notify(queue: .main) {
536-
let action = AccountAction.loadAndSynchronizeSiteIfNeeded(siteID: 9999) { result in
583+
let action = AccountAction.loadAndSynchronizeSiteIfNeeded(siteID: 9999, isJetpackConnectionPackageSupported: false) { result in
537584
XCTAssertTrue(Thread.isMainThread)
538585
promise(result)
539586
}
@@ -567,7 +614,8 @@ final class AccountStoreTests: XCTestCase {
567614
let siteIDInSimulatedResponse = Int64(1112233334444555)
568615
let result: Result<Yosemite.Site, Error> = waitFor { promise in
569616
group.notify(queue: .main) {
570-
let action = AccountAction.loadAndSynchronizeSiteIfNeeded(siteID: siteIDInSimulatedResponse) { result in
617+
let action = AccountAction.loadAndSynchronizeSiteIfNeeded(siteID: siteIDInSimulatedResponse,
618+
isJetpackConnectionPackageSupported: false) { result in
571619
XCTAssertTrue(Thread.isMainThread)
572620
promise(result)
573621
}
@@ -595,7 +643,7 @@ final class AccountStoreTests: XCTestCase {
595643

596644
// When
597645
let _: Void = waitFor { promise in
598-
let action = AccountAction.loadAndSynchronizeSiteIfNeeded(siteID: 123) { result in
646+
let action = AccountAction.loadAndSynchronizeSiteIfNeeded(siteID: 123, isJetpackConnectionPackageSupported: true) { result in
599647
promise(())
600648
}
601649
accountStore.onAction(action)
@@ -606,6 +654,28 @@ final class AccountStoreTests: XCTestCase {
606654
[.loadSites, .checkIfWooCommerceIsActive(siteID: siteIDOfJCPSite), .fetchWordPressSiteSettings(siteID: siteIDOfJCPSite)])
607655
}
608656

657+
func test_loadAndSynchronizeSiteIfNeeded_makes_1_network_request_when_one_site_is_jetpack_cp_connected_with_jcp_feature_off() throws {
658+
// Given
659+
let network = MockNetwork()
660+
let remote = MockAccountRemote()
661+
remote.loadSitesResult = .success([
662+
Site.fake().copy(siteID: 1, isJetpackThePluginInstalled: true, isJetpackConnected: true),
663+
Site.fake().copy(siteID: 2, isJetpackThePluginInstalled: false, isJetpackConnected: true)
664+
])
665+
let accountStore = AccountStore(dispatcher: dispatcher, storageManager: storageManager, network: network, remote: remote)
666+
667+
// When
668+
let _: Void = waitFor { promise in
669+
let action = AccountAction.loadAndSynchronizeSiteIfNeeded(siteID: 123, isJetpackConnectionPackageSupported: false) { result in
670+
promise(())
671+
}
672+
accountStore.onAction(action)
673+
}
674+
675+
// Then
676+
XCTAssertEqual(remote.invocations, [.loadSites])
677+
}
678+
609679
func test_loadAndSynchronizeSiteIfNeeded_makes_1_network_requests_when_all_sites_have_jetpack_plugin() throws {
610680
// Given
611681
let network = MockNetwork()
@@ -619,7 +689,7 @@ final class AccountStoreTests: XCTestCase {
619689

620690
// When
621691
let _: Void = waitFor { promise in
622-
let action = AccountAction.loadAndSynchronizeSiteIfNeeded(siteID: 123) { result in
692+
let action = AccountAction.loadAndSynchronizeSiteIfNeeded(siteID: 123, isJetpackConnectionPackageSupported: true) { result in
623693
promise(())
624694
}
625695
accountStore.onAction(action)

0 commit comments

Comments
 (0)