Skip to content

Commit

Permalink
[PM-15416] Change pull-to-refresh to not force a sync (#1172)
Browse files Browse the repository at this point in the history
  • Loading branch information
KatherineInCode authored Dec 2, 2024
1 parent 4d9d912 commit c621b02
Show file tree
Hide file tree
Showing 11 changed files with 38 additions and 33 deletions.
10 changes: 5 additions & 5 deletions BitwardenShared/Core/Tools/Repositories/SendRepository.swift
Original file line number Diff line number Diff line change
Expand Up @@ -44,9 +44,9 @@ public protocol SendRepository: AnyObject {
/// Performs an API request to sync the user's send data. The publishers in the repository can
/// be used to subscribe to the send data, which are updated as a result of the request.
///
/// - Parameter isManualRefresh: Whether the sync is being performed as a manual refresh.
/// - Parameter forceSync: Whether the sync should be forced.
///
func fetchSync(isManualRefresh: Bool) async throws
func fetchSync(forceSync: Bool) async throws

/// Performs an API request to remove the password on the provided send.
///
Expand Down Expand Up @@ -216,10 +216,10 @@ class DefaultSendRepository: SendRepository {

// MARK: API Methods

func fetchSync(isManualRefresh: Bool) async throws {
func fetchSync(forceSync: Bool) async throws {
let allowSyncOnRefresh = try await stateService.getAllowSyncOnRefresh()
if !isManualRefresh || allowSyncOnRefresh {
try await syncService.fetchSync(forceSync: isManualRefresh)
if !forceSync || allowSyncOnRefresh {
try await syncService.fetchSync(forceSync: forceSync)
}
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -163,35 +163,35 @@ class SendRepositoryTests: BitwardenTestCase { // swiftlint:disable:this type_bo
XCTAssertFalse(isVerified)
}

/// `fetchSync(isManualRefresh:)` while manual refresh is allowed does perform a sync.
/// `fetchSync(forceSync:)` while manual refresh is allowed does perform a sync.
func test_fetchSync_manualRefreshAllowed_success() async throws {
await stateService.addAccount(.fixture())
stateService.allowSyncOnRefresh = ["1": true]
syncService.fetchSyncResult = .success(())

try await subject.fetchSync(isManualRefresh: true)
try await subject.fetchSync(forceSync: true)

XCTAssertTrue(syncService.didFetchSync)
}

/// `fetchSync(isManualRefresh:)` while manual refresh is not allowed does not perform a sync.
/// `fetchSync(forceSync:)` while manual refresh is not allowed does not perform a sync.
func test_fetchSync_manualRefreshNotAllowed_success() async throws {
await stateService.addAccount(.fixture())
stateService.allowSyncOnRefresh = [:]
syncService.fetchSyncResult = .success(())

try await subject.fetchSync(isManualRefresh: true)
try await subject.fetchSync(forceSync: true)

XCTAssertFalse(syncService.didFetchSync)
}

/// `fetchSync(isManualRefresh:)` and a failure performs a sync and throws the error.
/// `fetchSync(forceSync:)` and a failure performs a sync and throws the error.
func test_fetchSync_failure() async throws {
await stateService.addAccount(.fixture())
stateService.allowSyncOnRefresh = ["1": true]
syncService.fetchSyncResult = .failure(BitwardenTestError.example)
await assertAsyncThrows {
try await subject.fetchSync(isManualRefresh: true)
try await subject.fetchSync(forceSync: true)
}
XCTAssertTrue(syncService.didFetchSync)
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@ class MockSendRepository: SendRepository {
var doesActiveAccountHaveVerifiedEmailResult: Result<Bool, Error> = .success(true)

var fetchSyncCalled = false
var fetchSyncIsManualRefresh: Bool?
var fetchSyncForceSync: Bool?
var fetchSyncResult: Result<Void, Error> = .success(())

var searchSendSearchText: String?
Expand Down Expand Up @@ -81,9 +81,9 @@ class MockSendRepository: SendRepository {
try doesActiveAccountHaveVerifiedEmailResult.get()
}

func fetchSync(isManualRefresh: Bool) async throws {
func fetchSync(forceSync: Bool) async throws {
fetchSyncCalled = true
fetchSyncIsManualRefresh = isManualRefresh
fetchSyncForceSync = forceSync
try fetchSyncResult.get()
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -44,6 +44,7 @@ class MockVaultRepository: VaultRepository {
var fetchFoldersResult: Result<[FolderView], Error> = .success([])

var fetchSyncCalled = false
var fetchSyncForceSync: Bool?
var fetchSyncResult: Result<[VaultListSection]?, Error> = .success([])

var getActiveAccountIdResult: Result<String, StateServiceError> = .failure(.noActiveAccount)
Expand Down Expand Up @@ -182,10 +183,11 @@ class MockVaultRepository: VaultRepository {
}

func fetchSync(
isManualRefresh _: Bool,
forceSync: Bool,
filter _: VaultFilterType
) async throws -> [VaultListSection]? {
fetchSyncCalled = true
fetchSyncForceSync = forceSync
return try fetchSyncResult.get()
}

Expand Down
8 changes: 4 additions & 4 deletions BitwardenShared/Core/Vault/Repositories/VaultRepository.swift
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@ public protocol VaultRepository: AnyObject {
/// - Returns: If a sync is performed without error, this returns `[VaultListSection]` to display.
///
@discardableResult
func fetchSync(isManualRefresh: Bool, filter: VaultFilterType) async throws -> [VaultListSection]?
func fetchSync(forceSync: Bool, filter: VaultFilterType) async throws -> [VaultListSection]?

// MARK: Data Methods

Expand Down Expand Up @@ -928,10 +928,10 @@ extension DefaultVaultRepository: VaultRepository {
// MARK: API Methods

@discardableResult
func fetchSync(isManualRefresh: Bool, filter: VaultFilterType) async throws -> [VaultListSection]? {
func fetchSync(forceSync: Bool, filter: VaultFilterType) async throws -> [VaultListSection]? {
let allowSyncOnRefresh = try await stateService.getAllowSyncOnRefresh()
guard !isManualRefresh || allowSyncOnRefresh else { return nil }
try await syncService.fetchSync(forceSync: isManualRefresh)
guard !forceSync || allowSyncOnRefresh else { return nil }
try await syncService.fetchSync(forceSync: forceSync)
let ciphers = try await cipherService.fetchAllCiphers()
let collections = try await collectionService.fetchAllCollections(includeReadOnly: true)
let folders = try await folderService.fetchAllFolders()
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -807,13 +807,13 @@ class VaultRepositoryTests: BitwardenTestCase { // swiftlint:disable:this type_b
XCTAssertEqual(clientService.mockVault.clientFolders.decryptedFolders, folders)
}

/// `fetchSync(isManualRefresh:)` only syncs when expected.
/// `fetchSync(forceSync:)` only syncs when expected.
func test_fetchSync() async throws {
stateService.activeAccount = .fixture()

// If it's not a manual refresh, it should sync.
let automaticSections = try await subject.fetchSync(
isManualRefresh: false,
forceSync: false,
filter: .allVaults
)
XCTAssertTrue(syncService.didFetchSync)
Expand All @@ -824,7 +824,7 @@ class VaultRepositoryTests: BitwardenTestCase { // swiftlint:disable:this type_b
syncService.didFetchSync = false
stateService.allowSyncOnRefresh["1"] = true
let manualSections = try await subject.fetchSync(
isManualRefresh: true,
forceSync: true,
filter: .myVault
)
XCTAssertTrue(syncService.didFetchSync)
Expand All @@ -834,7 +834,10 @@ class VaultRepositoryTests: BitwardenTestCase { // swiftlint:disable:this type_b
// it should not sync.
syncService.didFetchSync = false
stateService.allowSyncOnRefresh["1"] = false
let nilSections = try await subject.fetchSync(isManualRefresh: true, filter: .allVaults)
let nilSections = try await subject.fetchSync(
forceSync: true,
filter: .allVaults
)
XCTAssertFalse(syncService.didFetchSync)
XCTAssertNil(nilSections)
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -114,7 +114,7 @@ final class SendListProcessor: StateProcessor<SendListState, SendListAction, Sen
///
private func refresh() async {
do {
try await services.sendRepository.fetchSync(isManualRefresh: true)
try await services.sendRepository.fetchSync(forceSync: false)
} catch {
let alert = Alert.networkResponseError(error) { [weak self] in
await self?.refresh()
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -61,11 +61,12 @@ class SendListProcessorTests: BitwardenTestCase { // swiftlint:disable:this type
XCTAssertTrue(subject.state.isSendDisabled)
}

/// `perform(_:)` with `refresh` calls the refresh method.
/// `perform(_:)` with `refresh` requests a fetch sync update, but does not force a sync.
func test_perform_refresh() async {
await subject.perform(.refresh)

XCTAssertTrue(sendRepository.fetchSyncCalled)
XCTAssertEqual(sendRepository.fetchSyncForceSync, false)
}

/// `perform(_:)` with `search(_:)` and an empty search query returns early.
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -203,7 +203,7 @@ final class VaultGroupProcessor: StateProcessor<
///
private func refreshVaultGroup() async {
do {
try await services.vaultRepository.fetchSync(isManualRefresh: true, filter: state.vaultFilterType)
try await services.vaultRepository.fetchSync(forceSync: true, filter: state.vaultFilterType)
} catch {
coordinator.showAlert(.networkResponseError(error))
services.errorReporter.log(error: error)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -65,7 +65,7 @@ final class VaultListProcessor: StateProcessor<
override func perform(_ effect: VaultListEffect) async {
switch effect {
case .appeared:
await refreshVault(isManualRefresh: false)
await refreshVault()
await handleNotifications()
await checkPendingLoginRequests()
await checkPersonalOwnershipPolicy()
Expand All @@ -86,7 +86,7 @@ final class VaultListProcessor: StateProcessor<
case .refreshAccountProfiles:
await refreshProfileState()
case .refreshVault:
await refreshVault(isManualRefresh: true)
await refreshVault()
case let .search(text):
state.searchResults = await searchVault(for: text)
case .streamAccountSetupProgress:
Expand Down Expand Up @@ -195,12 +195,10 @@ extension VaultListProcessor {

/// Refreshes the vault's contents.
///
/// - Parameter isManualRefresh: Whether the sync is being performed as a manual refresh.
///
private func refreshVault(isManualRefresh: Bool) async {
private func refreshVault() async {
do {
guard let sections = try await services.vaultRepository.fetchSync(
isManualRefresh: isManualRefresh,
forceSync: false,
filter: state.vaultFilterType
) else { return }
state.loadingState = .data(sections)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -421,12 +421,13 @@ class VaultListProcessorTests: BitwardenTestCase { // swiftlint:disable:this typ
XCTAssertEqual(subject.state.url, url)
}

/// `perform(_:)` with `.refreshed` requests a fetch sync update with the vault repository.
/// `perform(_:)` with `.refreshed` requests a fetch sync update, but does not force a sync.
@MainActor
func test_perform_refresh() async {
await subject.perform(.refreshVault)

XCTAssertTrue(vaultRepository.fetchSyncCalled)
XCTAssertEqual(vaultRepository.fetchSyncForceSync, false)
}

/// `perform(_:)` with `.refreshed` records an error if applicable.
Expand Down

0 comments on commit c621b02

Please sign in to comment.