diff --git a/BitwardenResources/Localizations/en.lproj/Localizable.strings b/BitwardenResources/Localizations/en.lproj/Localizable.strings index 97654f036a..0b4ddaf147 100644 --- a/BitwardenResources/Localizations/en.lproj/Localizable.strings +++ b/BitwardenResources/Localizations/en.lproj/Localizable.strings @@ -1142,6 +1142,12 @@ "OpeningCheckout" = "Opening checkout…"; "UpgradePending" = "Upgrade pending"; "YourUpgradeIsBeingProcessedDescriptionLong" = "Your upgrade is being processed. You'll remain on the free plan until it's complete. Sync now to check, or continue and it will update automatically."; +/* Generic button label for declining an action without dismissing it permanently. */ +"NotNow" = "Not now"; +/* Alert title shown when a sync attempt for a pending Premium upgrade fails. */ +"SyncUnsuccessful" = "Sync unsuccessful"; +/* Alert message shown when a sync attempt for a pending Premium upgrade fails. */ +"WeCouldntSyncYourVaultWithTheServerDescriptionLong" = "We couldn't sync your vault with the server. Your vault may not reflect your latest changes. You can try again now or it will sync automatically later."; "UpgradedToPremium" = "Upgraded to Premium"; "SubscriptionNeedsAttention" = "Your subscription needs attention"; "CheckYourPlanForDetails" = "Check your plan for details."; diff --git a/BitwardenShared/Core/Billing/Services/BillingService.swift b/BitwardenShared/Core/Billing/Services/BillingService.swift index a3ed59245d..6b6e8a0a96 100644 --- a/BitwardenShared/Core/Billing/Services/BillingService.swift +++ b/BitwardenShared/Core/Billing/Services/BillingService.swift @@ -376,7 +376,19 @@ class DefaultBillingService: BillingService { self.currentSyncSubscriber = Task { guard let publisher = try? await self.stateService.lastSyncTimePublisher() else { return } - for await _ in publisher.values { + // Snapshot the last-known sync time directly, rather than relying on + // whichever value the publisher happens to deliver first: its + // `CurrentValueSubject` backing replays the existing cached value + // immediately on subscribe (not evidence that a sync just happened), and a + // *different* account's sync can also re-emit this shared store without this + // account's own value having changed. Comparing each emission against the + // last value actually seen — rather than its position in the stream — means + // only a genuinely new sync for this account reaches + // `reconcilePendingUpgradeIfNeeded()`, regardless of subscription timing. + var lastSeenDate = try? await self.stateService.getLastSyncTime() + for await date in publisher.values { + guard date != lastSeenDate else { continue } + lastSeenDate = date await self.reconcilePendingUpgradeIfNeeded() } } @@ -398,11 +410,24 @@ class DefaultBillingService: BillingService { return } - guard await stateService.doesActiveAccountHavePremium() else { return } + // Reaching this point at all means a *new* sync just completed successfully — `start()` + // filters out both the initial replay and same-value re-emissions before invoking this + // method, and `lastSyncTimePublisher` never fires on failure — so the most recent + // attempt did not fail. Clear that regardless of whether the account has become Premium + // yet, so a stale failure doesn't linger after a later, unrelated sync has succeeded. + do { + try await billingStateService.setPremiumUpgradeLastSyncAttemptFailed(false) + } catch { + errorReporter.log(error: error) + } + + guard await stateService.doesActiveAccountHavePremium() else { + await refreshPremiumUpgradePendingStateSubject() + return + } do { try await billingStateService.setPremiumUpgradePending(false) - try await billingStateService.setPremiumUpgradeLastSyncAttemptFailed(false) try await billingStateService.setUpgradedToPremiumActionCardVisible(true) } catch { errorReporter.log(error: error) diff --git a/BitwardenShared/Core/Billing/Services/BillingServiceTests.swift b/BitwardenShared/Core/Billing/Services/BillingServiceTests.swift index 2642711938..de83f77fc9 100644 --- a/BitwardenShared/Core/Billing/Services/BillingServiceTests.swift +++ b/BitwardenShared/Core/Billing/Services/BillingServiceTests.swift @@ -608,6 +608,59 @@ struct BillingServiceTests { // swiftlint:disable:this type_body_length // MARK: start() + /// `start()` clears a stale `lastAttemptFailed` flag as soon as any sync succeeds, even + /// when the active account still isn't Premium yet — a later, unrelated sync succeeding + /// means the most recent attempt did not fail, regardless of whether Premium has been + /// granted. + @Test + func start_clearsLastAttemptFailedOnGenericSyncEvenWithoutPremium() async throws { + stateService.activeAccount = .fixture() + stateService.doesActiveAccountHavePremiumResult = false + + await subject.start() + // Wait for the subscription's baseline snapshot before sending a "new" value below — + // `start()` reads `getLastSyncTime()` exactly once, before subscribing to the publisher. + // Without this, the publisher's `CurrentValueSubject` backing could coalesce an + // immediate send with the not-yet-taken snapshot, making it ambiguous whether the send + // counts as a change (both nested `Task`s run on the cooperative pool, so a fixed + // number of `Task.yield()` calls from this `@MainActor` test isn't a reliable proxy). + try await waitForAsync { stateService.getLastSyncTimeCallCount == 1 } + + stateService.premiumUpgradePendingResult = true + stateService.premiumUpgradeLastSyncAttemptFailedResult = true + stateService.lastSyncTimeSubject.send(Date()) + + try await waitForAsync { stateService.premiumUpgradeLastSyncAttemptFailedResult == false } + #expect(stateService.premiumUpgradePendingResult == true) + #expect(stateService.upgradedToPremiumActionCardVisibleResult == false) + } + + /// `start()` does not clear a stale `lastAttemptFailed` flag just from subscribing to the + /// last-sync-time publisher — its `CurrentValueSubject` backing replays the existing cached + /// value immediately on subscribe, and that replay must not be mistaken for a new sync + /// completing. + @Test + func start_doesNotClearLastAttemptFailedOnInitialSubscriptionReplay() async throws { + stateService.activeAccount = .fixture() + stateService.doesActiveAccountHavePremiumResult = true + stateService.lastSyncTimeSubject.send(Date()) + stateService.premiumUpgradePendingResult = true + stateService.premiumUpgradeLastSyncAttemptFailedResult = true + + await subject.start() + try await waitForAsync { stateService.getLastSyncTimeCallCount == 1 } + + #expect(stateService.premiumUpgradeLastSyncAttemptFailedResult == true) + #expect(stateService.setPremiumUpgradeLastSyncAttemptFailedCallCount == 0) + + // A genuinely new sync, sent only once the initial subscription has settled, still + // clears the flag as expected. + stateService.lastSyncTimeSubject.send(Date()) + + try await waitForAsync { stateService.premiumUpgradeLastSyncAttemptFailedResult == false } + #expect(stateService.setPremiumUpgradeLastSyncAttemptFailedCallCount == 1) + } + /// `start()` resolves a pending Premium upgrade when a generic sync completes and the /// active account has since become Premium, by any means (not just the original checkout /// attempt's own subscription). @@ -617,6 +670,8 @@ struct BillingServiceTests { // swiftlint:disable:this type_body_length stateService.doesActiveAccountHavePremiumResult = false await subject.start() + // See the comment in `start_clearsLastAttemptFailedOnGenericSyncEvenWithoutPremium()`. + try await waitForAsync { stateService.getLastSyncTimeCallCount == 1 } stateService.premiumUpgradePendingResult = true stateService.premiumUpgradeLastSyncAttemptFailedResult = true diff --git a/BitwardenShared/Core/Platform/Services/TestHelpers/MockStateService.swift b/BitwardenShared/Core/Platform/Services/TestHelpers/MockStateService.swift index e64eaf9e44..7f32d62e2b 100644 --- a/BitwardenShared/Core/Platform/Services/TestHelpers/MockStateService.swift +++ b/BitwardenShared/Core/Platform/Services/TestHelpers/MockStateService.swift @@ -38,6 +38,8 @@ class MockStateService: StateService, ActiveAccountStateProvider, AutofillStateS var premiumUpgradeLastSyncAttemptFailedResult: Bool = false // swiftlint:disable:this identifier_name var premiumUpgradePendingResult: Bool = false // swiftlint:disable:next identifier_name + var setPremiumUpgradeLastSyncAttemptFailedCallCount = 0 + // swiftlint:disable:next identifier_name var setPremiumUpgradeLastSyncAttemptFailedResult: Result = .success(()) var setPremiumUpgradePendingResult: Result = .success(()) var setSubscriptionAttentionCardResult: Result = .success(()) @@ -93,6 +95,7 @@ class MockStateService: StateService, ActiveAccountStateProvider, AutofillStateS var getAccountHasBeenUnlockedInteractivelyResult: Result = .success(false) var getActiveAccountIdError: Error? var getBiometricAuthenticationEnabledResult: Result = .success(()) + var getLastSyncTimeCallCount = 0 var lastRequestToTurnOnCredentialProvider: Date? var lastSyncMonotonicTimeByUserId = [String: TimeInterval?]() var lastSyncTimeByUserId = [String: Date]() @@ -389,8 +392,13 @@ class MockStateService: StateService, ActiveAccountStateProvider, AutofillStateS } func getLastSyncTime(userId: String?) async throws -> Date? { + getLastSyncTimeCallCount += 1 let userId = try unwrapUserId(userId) - return lastSyncTimeByUserId[userId] + // Falls back to `lastSyncTimeSubject` for tests that drive it directly via `.send(_:)` + // instead of going through `setLastSyncTime(_:userId:)`, which keeps both stores in + // sync (mirroring `DefaultStateService`, where both calls read from the same + // underlying store). + return lastSyncTimeByUserId[userId] ?? lastSyncTimeSubject.value } func getLastSyncMonotonicTime(userId: String?) async throws -> TimeInterval? { @@ -663,6 +671,7 @@ class MockStateService: StateService, ActiveAccountStateProvider, AutofillStateS } func setPremiumUpgradeLastSyncAttemptFailed(_ failed: Bool) async throws { + setPremiumUpgradeLastSyncAttemptFailedCallCount += 1 try setPremiumUpgradeLastSyncAttemptFailedResult.get() premiumUpgradeLastSyncAttemptFailedResult = failed } @@ -775,6 +784,7 @@ class MockStateService: StateService, ActiveAccountStateProvider, AutofillStateS func setLastSyncTime(_ date: Date?, userId: String?) async throws { let userId = try unwrapUserId(userId) lastSyncTimeByUserId[userId] = date + lastSyncTimeSubject.value = date } func getLastUserShouldConnectToWatch() async -> Bool { diff --git a/BitwardenShared/UI/Billing/Extensions/Alert+Billing.swift b/BitwardenShared/UI/Billing/Extensions/Alert+Billing.swift index 9f61007273..3359e56717 100644 --- a/BitwardenShared/UI/Billing/Extensions/Alert+Billing.swift +++ b/BitwardenShared/UI/Billing/Extensions/Alert+Billing.swift @@ -60,6 +60,33 @@ extension Alert { ) } + /// An alert shown when a sync attempt for a pending Premium upgrade fails. + /// + /// - Parameter tryAgainHandler: A closure called when the user taps "Try again". + /// - Returns: An `Alert` with "Not now" and "Try again" actions. + /// + static func syncUnsuccessful( + tryAgainHandler: @escaping () async -> Void, + ) -> Alert { + Alert( + title: Localizations.syncUnsuccessful, + message: Localizations.weCouldntSyncYourVaultWithTheServerDescriptionLong, + alertActions: [ + AlertAction( + title: Localizations.notNow, + style: .cancel, + ), + AlertAction( + title: Localizations.tryAgain, + style: .default, + handler: { _, _ in + await tryAgainHandler() + }, + ), + ], + ) + } + /// An alert shown when a Premium upgrade is still being processed. /// /// - Parameter syncNowHandler: A closure called when the user taps "Sync now". diff --git a/BitwardenShared/UI/Billing/Extensions/Alert+BillingTests.swift b/BitwardenShared/UI/Billing/Extensions/Alert+BillingTests.swift index 1a3f26ed7d..4e4cc9e07c 100644 --- a/BitwardenShared/UI/Billing/Extensions/Alert+BillingTests.swift +++ b/BitwardenShared/UI/Billing/Extensions/Alert+BillingTests.swift @@ -47,6 +47,24 @@ class AlertBillingTests: BitwardenTestCase { XCTAssertEqual(tryAgainAction.style, .default) } + /// `syncUnsuccessful(tryAgainHandler:)` builds an `Alert` with the correct title, message, and actions. + func test_syncUnsuccessful() { + let subject = Alert.syncUnsuccessful {} + + XCTAssertEqual(subject.title, Localizations.syncUnsuccessful) + XCTAssertEqual(subject.message, Localizations.weCouldntSyncYourVaultWithTheServerDescriptionLong) + XCTAssertEqual(subject.preferredStyle, .alert) + XCTAssertEqual(subject.alertActions.count, 2) + + let notNowAction = subject.alertActions[0] + XCTAssertEqual(notNowAction.title, Localizations.notNow) + XCTAssertEqual(notNowAction.style, .cancel) + + let tryAgainAction = subject.alertActions[1] + XCTAssertEqual(tryAgainAction.title, Localizations.tryAgain) + XCTAssertEqual(tryAgainAction.style, .default) + } + /// `upgradePending(syncNowHandler:)` builds an `Alert` with the correct title, message, and actions. func test_upgradePending() { let subject = Alert.upgradePending {} diff --git a/BitwardenShared/UI/Vault/Vault/VaultList/VaultListProcessor.swift b/BitwardenShared/UI/Vault/Vault/VaultList/VaultListProcessor.swift index 1909035868..0749990112 100644 --- a/BitwardenShared/UI/Vault/Vault/VaultList/VaultListProcessor.swift +++ b/BitwardenShared/UI/Vault/Vault/VaultList/VaultListProcessor.swift @@ -703,11 +703,30 @@ extension VaultListProcessor { /// Streams live updates to the Premium upgrade pending state, hiding or re-evaluating the /// upsell action card immediately rather than waiting for the next screen appearance — /// needed because dismissing the "Upgrade Pending" alert returns to this same screen - /// without a fresh `.appeared`. + /// without a fresh `.appeared`. Also shows the "Sync Unsuccessful" alert the first time a + /// sync attempt for the pending upgrade fails. /// private func streamPremiumUpgradePendingState() async { + var lastAttemptFailed = false for await pendingState in services.billingService.premiumUpgradePendingStatePublisher().values { await updatePremiumUpgradeActionCardVisibility(isPending: pendingState.isPending) + + // Only show the alert on a false-to-true transition — the publisher replays its + // current value to this subscription immediately (covering a stale failure still + // persisted from a previous app session), but without tracking the transition, any + // later emission that still has `lastAttemptFailed: true` (e.g. triggered by an + // unrelated `isPending` change) would incorrectly re-show the alert. + if pendingState.lastAttemptFailed, !lastAttemptFailed { + coordinator.showAlert(.syncUnsuccessful { [weak self] in + // Reset the tracker before retrying — if the retry's own sync also fails, + // the resulting `lastAttemptFailed: true` emission must be treated as a new + // transition rather than a duplicate, or this explicit, user-initiated + // retry would fail with no feedback at all. + lastAttemptFailed = false + await self?.services.billingService.premiumStatusChanged() + }) + } + lastAttemptFailed = pendingState.lastAttemptFailed } } diff --git a/BitwardenShared/UI/Vault/Vault/VaultList/VaultListProcessorTests.swift b/BitwardenShared/UI/Vault/Vault/VaultList/VaultListProcessorTests.swift index 718234d3db..e61105423a 100644 --- a/BitwardenShared/UI/Vault/Vault/VaultList/VaultListProcessorTests.swift +++ b/BitwardenShared/UI/Vault/Vault/VaultList/VaultListProcessorTests.swift @@ -1146,6 +1146,119 @@ class VaultListProcessorTests: BitwardenTestCase { // swiftlint:disable:this typ task.cancel() } + /// `perform(_:)` with `.streamPremiumUpgradePendingState` shows the "Sync unsuccessful" + /// alert the first time the publisher reports a failed sync attempt. + @MainActor + func test_perform_streamPremiumUpgradePendingState_showsSyncUnsuccessfulAlert() throws { + let pendingStateSubject = CurrentValueSubject( + PremiumUpgradePendingState(isPending: true, lastAttemptFailed: false), + ) + billingService.premiumUpgradePendingStatePublisherReturnValue = pendingStateSubject.eraseToAnyPublisher() + + let task = Task { + await subject.perform(.streamPremiumUpgradePendingState) + } + defer { task.cancel() } + + pendingStateSubject.send(PremiumUpgradePendingState(isPending: true, lastAttemptFailed: true)) + waitFor(!coordinator.alertShown.isEmpty) + + XCTAssertEqual(coordinator.alertShown.count, 1) + XCTAssertEqual(coordinator.alertShown.last?.title, Localizations.syncUnsuccessful) + } + + /// `perform(_:)` with `.streamPremiumUpgradePendingState` does not re-show the "Sync + /// unsuccessful" alert on a later, duplicate emission that still reports a failed attempt. + @MainActor + func test_perform_streamPremiumUpgradePendingState_doesNotReshowSyncUnsuccessfulAlert() { + billingRepository.isInAppUpgradeAvailableReturnValue = true + let pendingStateSubject = CurrentValueSubject( + PremiumUpgradePendingState(isPending: true, lastAttemptFailed: false), + ) + billingService.premiumUpgradePendingStatePublisherReturnValue = pendingStateSubject.eraseToAnyPublisher() + + let task = Task { + await subject.perform(.streamPremiumUpgradePendingState) + } + defer { task.cancel() } + + pendingStateSubject.send(PremiumUpgradePendingState(isPending: true, lastAttemptFailed: true)) + waitFor(!coordinator.alertShown.isEmpty) + XCTAssertEqual(coordinator.alertShown.count, 1) + + // A duplicate emission with the same `lastAttemptFailed: true` (e.g. triggered by an + // unrelated `isPending` change) should not re-show the alert. Combine delivers a single + // publisher's emissions in order, so sending a distinct, independently-observable + // follow-up and waiting on it proves the duplicate before it was already processed — + // without that, `waitFor` could return before the duplicate was even handled. + pendingStateSubject.send(PremiumUpgradePendingState(isPending: true, lastAttemptFailed: true)) + pendingStateSubject.send(PremiumUpgradePendingState(isPending: false, lastAttemptFailed: true)) + waitFor(subject.state.shouldShowPremiumUpgradeActionCard) + + XCTAssertEqual(coordinator.alertShown.count, 1) + } + + /// `perform(_:)` with `.streamPremiumUpgradePendingState` — tapping "Try again" on the + /// "Sync unsuccessful" alert retries via `premiumStatusChanged()`. + /// + /// Kept synchronous (not `async`) deliberately: this test runs a background `Task` + /// consuming an infinite stream while also driving another async call (`tapAction`) from + /// the test body. If the test function itself were `async`, it would run on Swift's + /// cooperative thread pool, and the blocking `waitFor` helper below would starve that same + /// pool, preventing the background `Task` from ever being scheduled — a real deadlock this + /// test hit before being fixed. Running synchronously (on the XCTest main thread, not the + /// cooperative pool) avoids that; `tapAction`'s own async call is driven via a second `Task` + /// polled with `waitFor` instead of an inline `await`. + @MainActor + func test_perform_streamPremiumUpgradePendingState_syncUnsuccessfulAlert_tryAgain() throws { + let pendingStateSubject = CurrentValueSubject( + PremiumUpgradePendingState(isPending: true, lastAttemptFailed: true), + ) + billingService.premiumUpgradePendingStatePublisherReturnValue = pendingStateSubject.eraseToAnyPublisher() + + let task = Task { + await subject.perform(.streamPremiumUpgradePendingState) + } + defer { task.cancel() } + + waitFor(!coordinator.alertShown.isEmpty) + let alert = try XCTUnwrap(coordinator.alertShown.last) + + Task { + try await alert.tapAction(title: Localizations.tryAgain) + } + waitFor(billingService.premiumStatusChangedCallsCount == 1) + } + + /// `perform(_:)` with `.streamPremiumUpgradePendingState` re-shows the "Sync Unsuccessful" + /// alert if the user's own "Try again" retry also fails — the transition tracker must reset + /// before retrying, or this explicit, user-initiated retry would fail with no feedback. + @MainActor + func test_perform_streamPremiumUpgradePendingState_syncUnsuccessfulAlert_tryAgainFails() throws { + let pendingStateSubject = CurrentValueSubject( + PremiumUpgradePendingState(isPending: true, lastAttemptFailed: true), + ) + billingService.premiumUpgradePendingStatePublisherReturnValue = pendingStateSubject.eraseToAnyPublisher() + billingService.premiumStatusChangedClosure = { + pendingStateSubject.send(PremiumUpgradePendingState(isPending: true, lastAttemptFailed: true)) + } + + let task = Task { + await subject.perform(.streamPremiumUpgradePendingState) + } + defer { task.cancel() } + + waitFor(!coordinator.alertShown.isEmpty) + let alert = try XCTUnwrap(coordinator.alertShown.last) + + Task { + try await alert.tapAction(title: Localizations.tryAgain) + } + waitFor(coordinator.alertShown.count == 2) + + XCTAssertEqual(coordinator.alertShown.last?.title, Localizations.syncUnsuccessful) + } + /// `perform(_:)` with `.streamShowWebIcons` requests the value of the show /// web icons parameter from the state service. @MainActor