Merge pull request #5241 from woocommerce/issue/2520-site-list-refresh

jaclync · web-flow · commit 506ca7448d97 · 2021-10-21T10:09:20.000+08:00
Keep sites in site picker up to date
diff --git a/Networking/Networking/Model/Copiable/Models+Copiable.generated.swift b/Networking/Networking/Model/Copiable/Models+Copiable.generated.swift
@@ -1138,6 +1138,42 @@ extension ShippingLabelPurchase {
     }
 }
 
+extension Site {
+    public func copy(
+        siteID: CopiableProp<Int64> = .copy,
+        name: CopiableProp<String> = .copy,
+        description: CopiableProp<String> = .copy,
+        url: CopiableProp<String> = .copy,
+        plan: CopiableProp<String> = .copy,
+        isWooCommerceActive: CopiableProp<Bool> = .copy,
+        isWordPressStore: CopiableProp<Bool> = .copy,
+        timezone: CopiableProp<String> = .copy,
+        gmtOffset: CopiableProp<Double> = .copy
+    ) -> Site {
+        let siteID = siteID ?? self.siteID
+        let name = name ?? self.name
+        let description = description ?? self.description
+        let url = url ?? self.url
+        let plan = plan ?? self.plan
+        let isWooCommerceActive = isWooCommerceActive ?? self.isWooCommerceActive
+        let isWordPressStore = isWordPressStore ?? self.isWordPressStore
+        let timezone = timezone ?? self.timezone
+        let gmtOffset = gmtOffset ?? self.gmtOffset
+
+        return Site(
+            siteID: siteID,
+            name: name,
+            description: description,
+            url: url,
+            plan: plan,
+            isWooCommerceActive: isWooCommerceActive,
+            isWordPressStore: isWordPressStore,
+            timezone: timezone,
+            gmtOffset: gmtOffset
+        )
+    }
+}
+
 extension SitePlugin {
     public func copy(
         siteID: CopiableProp<Int64> = .copy,
diff --git a/Networking/Networking/Model/Site.swift b/Networking/Networking/Model/Site.swift
@@ -3,7 +3,7 @@ import Codegen
 
 /// Represents a WordPress.com Site.
 ///
-public struct Site: Decodable, Equatable, GeneratedFakeable {
+public struct Site: Decodable, Equatable, GeneratedFakeable, GeneratedCopiable {
 
     /// WordPress.com Site Identifier.
     ///
diff --git a/RELEASE-NOTES.txt b/RELEASE-NOTES.txt
@@ -2,6 +2,7 @@
 
 7.9
 -----
+- [*] Fix: after disconnecting a site or connecting to a new site, the sites in site picker (Settings > Switch Store) should be updated accordingly. The only exception is when the newly disconnected site is the currently selected site. [https://github.com/woocommerce/woocommerce-ios/pull/5241]
 
 
 7.8
diff --git a/Storage/Storage/Tools/StorageType+Extensions.swift b/Storage/Storage/Tools/StorageType+Extensions.swift
@@ -26,6 +26,12 @@ public extension StorageType {
         return firstObject(ofType: Site.self, matching: predicate)
     }
 
+    /// Retrieves all stored sites.
+    ///
+    func loadAllSites() -> [Site] {
+        allObjects(ofType: Site.self, matching: nil, sortedBy: nil)
+    }
+
     // MARK: - Orders
 
     /// Retrieves the Stored Order.
diff --git a/WooCommerce/Classes/Authentication/Epilogue/StorePickerViewController.swift b/WooCommerce/Classes/Authentication/Epilogue/StorePickerViewController.swift
@@ -47,7 +47,7 @@ enum StorePickerConfiguration {
 
 /// Allows the user to pick which WordPress.com (OR) Jetpack-Connected-Store we should set up as the Main Store.
 ///
-class StorePickerViewController: UIViewController {
+final class StorePickerViewController: UIViewController {
 
     /// StorePickerViewController Delegate
     ///
@@ -243,8 +243,16 @@ private extension StorePickerViewController {
     }
 
     func refreshResults() {
-        try? resultsController.performFetch()
+        refetchSitesAndUpdateState()
         ServiceLocator.analytics.track(.sitePickerStoresShown, withProperties: ["num_of_stores": resultsController.numberOfObjects])
+
+        synchronizeSites { [weak self] _ in
+            self?.refetchSitesAndUpdateState()
+        }
+    }
+
+    func refetchSitesAndUpdateState() {
+        try? resultsController.performFetch()
         state = StorePickerState(sites: resultsController.fetchedObjects)
     }
 
@@ -262,6 +270,15 @@ private extension StorePickerViewController {
     }
 }
 
+// MARK: - Syncing
+//
+private extension StorePickerViewController {
+    func synchronizeSites(onCompletion: @escaping (Result<Void, Error>) -> Void) {
+        let action = AccountAction.synchronizeSites(selectedSiteID: currentlySelectedSite?.siteID, onCompletion: onCompletion)
+        ServiceLocator.stores.dispatch(action)
+    }
+}
+
 
 // MARK: - Notifications
 //
@@ -587,7 +604,7 @@ extension StorePickerViewController: UITableViewDataSource {
         cell.name = site.name
         cell.url = site.url
         cell.allowsCheckmark = state.multipleStoresAvailable
-        cell.displaysCheckmark = currentlySelectedSite == site
+        cell.displaysCheckmark = currentlySelectedSite?.siteID == site.siteID
 
         return cell
     }
diff --git a/WooCommerce/Classes/Yosemite/DefaultStoresManager.swift b/WooCommerce/Classes/Yosemite/DefaultStoresManager.swift
@@ -273,7 +273,7 @@ private extension DefaultStoresManager {
     /// Synchronizes the WordPress.com Sites, associated with the current credentials.
     ///
     func synchronizeSites(onCompletion: @escaping (Result<Void, Error>) -> Void) {
-        let action = AccountAction.synchronizeSites(onCompletion: onCompletion)
+        let action = AccountAction.synchronizeSites(selectedSiteID: sessionManager.defaultStoreID, onCompletion: onCompletion)
         dispatch(action)
     }
 
diff --git a/Yosemite/Yosemite/Actions/AccountAction.swift b/Yosemite/Yosemite/Actions/AccountAction.swift
@@ -10,7 +10,7 @@ public enum AccountAction: Action {
     case loadSite(siteID: Int64, onCompletion: (Site?) -> Void)
     case synchronizeAccount(onCompletion: (Result<Account, Error>) -> Void)
     case synchronizeAccountSettings(userID: Int64, onCompletion: (Result<AccountSettings, Error>) -> Void)
-    case synchronizeSites(onCompletion: (Result<Void, Error>) -> Void)
+    case synchronizeSites(selectedSiteID: Int64?, onCompletion: (Result<Void, Error>) -> Void)
     case synchronizeSitePlan(siteID: Int64, onCompletion: (Result<Void, Error>) -> Void)
     case updateAccountSettings(userID: Int64, tracksOptOut: Bool, onCompletion: (Result<Void, Error>) -> Void)
 }
diff --git a/Yosemite/Yosemite/Stores/AccountStore.swift b/Yosemite/Yosemite/Stores/AccountStore.swift
@@ -47,8 +47,8 @@ public class AccountStore: Store {
             synchronizeAccount(onCompletion: onCompletion)
         case .synchronizeAccountSettings(let userID, let onCompletion):
             synchronizeAccountSettings(userID: userID, onCompletion: onCompletion)
-        case .synchronizeSites(let onCompletion):
-            synchronizeSites(onCompletion: onCompletion)
+        case .synchronizeSites(let selectedSiteID, let onCompletion):
+            synchronizeSites(selectedSiteID: selectedSiteID, onCompletion: onCompletion)
         case .synchronizeSitePlan(let siteID, let onCompletion):
             synchronizeSitePlan(siteID: siteID, onCompletion: onCompletion)
         case .updateAccountSettings(let userID, let tracksOptOut, let onCompletion):
@@ -90,11 +90,11 @@ private extension AccountStore {
 
     /// Synchronizes the WordPress.com sites associated with the Network's Auth Token.
     ///
-    func synchronizeSites(onCompletion: @escaping (Result<Void, Error>) -> Void) {
+    func synchronizeSites(selectedSiteID: Int64?, onCompletion: @escaping (Result<Void, Error>) -> Void) {
         remote.loadSites { [weak self] result in
             switch result {
             case .success(let sites):
-                self?.upsertStoredSitesInBackground(readOnlySites: sites) {
+                self?.upsertStoredSitesInBackground(readOnlySites: sites, selectedSiteID: selectedSiteID) {
                     onCompletion(.success(()))
                 }
             case .failure(let error):
@@ -192,9 +192,17 @@ extension AccountStore {
 
     /// Updates (OR Inserts) the specified ReadOnly Site Entities into the Storage Layer.
     ///
-    func upsertStoredSitesInBackground(readOnlySites: [Networking.Site], onCompletion: @escaping () -> Void) {
+    func upsertStoredSitesInBackground(readOnlySites: [Networking.Site], selectedSiteID: Int64? = nil, onCompletion: @escaping () -> Void) {
         let derivedStorage = sharedDerivedStorage
         derivedStorage.perform {
+            // Deletes sites in storage that are not in `readOnlySites` and not the selected site.
+            let storageSites = derivedStorage.loadAllSites()
+            let readOnlySiteIDs = readOnlySites.map(\.siteID)
+            storageSites.filter { readOnlySiteIDs.contains($0.siteID) == false && $0.siteID != selectedSiteID }
+                .forEach { remotelyDeletedSite in
+                    derivedStorage.deleteObject(remotelyDeletedSite)
+                }
+
             for readOnlySite in readOnlySites {
                 let storageSite = derivedStorage.loadSite(siteID: readOnlySite.siteID) ?? derivedStorage.insertNewObject(ofType: Storage.Site.self)
                 storageSite.update(with: readOnlySite)
diff --git a/Yosemite/YosemiteTests/Mocks/MockStorageManager+Sample.swift b/Yosemite/YosemiteTests/Mocks/MockStorageManager+Sample.swift
@@ -131,6 +131,16 @@ extension MockStorageManager {
         return newOrder
     }
 
+    /// Inserts a new (Sample) site into the specified context.
+    ///
+    @discardableResult
+    func insertSampleSite(readOnlySite: Site) -> StorageSite {
+        let newSite = viewStorage.insertNewObject(ofType: StorageSite.self)
+        newSite.update(with: readOnlySite)
+
+        return newSite
+    }
+
     /// Inserts a new (Sample) PLugin into the specified context.
     ///
     @discardableResult
diff --git a/Yosemite/YosemiteTests/Stores/AccountStoreTests.swift b/Yosemite/YosemiteTests/Stores/AccountStoreTests.swift
@@ -1,3 +1,4 @@
+import Fakes
 import XCTest
 @testable import Yosemite
 @testable import Networking
@@ -7,7 +8,7 @@ import XCTest
 
 /// AccountStore Unit Tests
 ///
-class AccountStoreTests: XCTestCase {
+final class AccountStoreTests: XCTestCase {
 
     /// Mock Dispatcher!
     ///
@@ -216,7 +217,7 @@ class AccountStoreTests: XCTestCase {
 
         // When
         let result: Result<Void, Error> = waitFor { promise in
-            let action = AccountAction.synchronizeSites { result in
+            let action = AccountAction.synchronizeSites(selectedSiteID: nil) { result in
                 promise(result)
             }
             store.onAction(action)
@@ -236,7 +237,7 @@ class AccountStoreTests: XCTestCase {
 
         // When
         let result: Result<Void, Error> = waitFor { promise in
-            let action = AccountAction.synchronizeSites { result in
+            let action = AccountAction.synchronizeSites(selectedSiteID: nil) { result in
                 promise(result)
             }
             store.onAction(action)
@@ -247,6 +248,58 @@ class AccountStoreTests: XCTestCase {
         XCTAssertEqual(self.viewStorage.countObjects(ofType: Storage.Site.self), 2)
     }
 
+    /// Verifies that `synchronizeSites` deletes storage sites that do not exist remotely anymore.
+    ///
+    func test_synchronizeSites_deletes_sites_that_do_not_exist_remotely() {
+        // Given
+        let store = AccountStore(dispatcher: dispatcher, storageManager: storageManager, network: network)
+        let siteIDInStorageOnly = Int64(127)
+        storageManager.insertSampleSite(readOnlySite: Site.fake().copy(siteID: siteIDInStorageOnly))
+        network.simulateResponse(requestUrlSuffix: "me/sites", filename: "sites")
+        XCTAssertEqual(viewStorage.countObjects(ofType: Storage.Site.self), 1)
+        XCTAssertNotNil(viewStorage.loadSite(siteID: siteIDInStorageOnly))
+
+        // When
+        let result: Result<Void, Error> = waitFor { promise in
+            let action = AccountAction.synchronizeSites(selectedSiteID: nil) { result in
+                promise(result)
+            }
+            store.onAction(action)
+        }
+
+        // Then
+        XCTAssertTrue(result.isSuccess)
+        // `sites.json` contains 2 sites that do not match `siteIDInStorageOnly`.
+        XCTAssertEqual(self.viewStorage.countObjects(ofType: Storage.Site.self), 2)
+        XCTAssertNil(viewStorage.loadSite(siteID: siteIDInStorageOnly))
+    }
+
+    /// Verifies that `synchronizeSites` does not delete selected site after syncing and the selected site does not exist remotely anymore.
+    ///
+    func test_synchronizeSites_does_not_delete_selected_site_that_does_not_exist_remotely() {
+        // Given
+        let store = AccountStore(dispatcher: dispatcher, storageManager: storageManager, network: network)
+        let selectedSiteID = Int64(127)
+        storageManager.insertSampleSite(readOnlySite: Site.fake().copy(siteID: selectedSiteID))
+        network.simulateResponse(requestUrlSuffix: "me/sites", filename: "sites")
+        XCTAssertEqual(viewStorage.countObjects(ofType: Storage.Site.self), 1)
+        XCTAssertNotNil(viewStorage.loadSite(siteID: selectedSiteID))
+
+        // When
+        let result: Result<Void, Error> = waitFor { promise in
+            let action = AccountAction.synchronizeSites(selectedSiteID: selectedSiteID) { result in
+                promise(result)
+            }
+            store.onAction(action)
+        }
+
+        // Then
+        XCTAssertTrue(result.isSuccess)
+        // `sites.json` contains 2 sites that do not match `siteIDInStorageOnly`.
+        XCTAssertEqual(self.viewStorage.countObjects(ofType: Storage.Site.self), 3)
+        XCTAssertNotNil(viewStorage.loadSite(siteID: selectedSiteID))
+    }
+
     // MARK: - AccountAction.loadAccount
 
     func test_loadAccount_returns_expected_account() {

-Original file line number
+Diff line change
 .9
 -----
 +- [*] Fix: after disconnecting a site or connecting to a new site, the sites in site picker (Settings > Switch Store) should be updated accordingly. The only exception is when the newly disconnected site is the currently selected site. [https://github.com/woocommerce/woocommerce-ios/pull/5241]
 .8
Original file line number	Diff line number	Diff line change
`@@ -273,7 +273,7 @@ private extension DefaultStoresManager {`
`273`	`273`	`/// Synchronizes the WordPress.com Sites, associated with the current credentials.`
`274`	`274`	`///`
`275`	`275`	`func synchronizeSites(onCompletion: @escaping (Result<Void, Error>) -> Void) {`
`276`		`- let action = AccountAction.synchronizeSites(onCompletion: onCompletion)`
	`276`	`+ let action = AccountAction.synchronizeSites(selectedSiteID: sessionManager.defaultStoreID, onCompletion: onCompletion)`
`277`	`277`	`dispatch(action)`
`278`	`278`	`}`
`279`	`279`
Original file line number	Diff line number	Diff line change
`@@ -10,7 +10,7 @@ public enum AccountAction: Action {`
`10`	`10`	`case loadSite(siteID: Int64, onCompletion: (Site?) -> Void)`
`11`	`11`	`case synchronizeAccount(onCompletion: (Result<Account, Error>) -> Void)`
`12`	`12`	`case synchronizeAccountSettings(userID: Int64, onCompletion: (Result<AccountSettings, Error>) -> Void)`
`13`		`- case synchronizeSites(onCompletion: (Result<Void, Error>) -> Void)`
	`13`	`+ case synchronizeSites(selectedSiteID: Int64?, onCompletion: (Result<Void, Error>) -> Void)`
`14`	`14`	`case synchronizeSitePlan(siteID: Int64, onCompletion: (Result<Void, Error>) -> Void)`
`15`	`15`	`case updateAccountSettings(userID: Int64, tracksOptOut: Bool, onCompletion: (Result<Void, Error>) -> Void)`
`16`	`16`	`}`