From d44016fd3773c7a7cc15496ffbe3b9d239a12d3b Mon Sep 17 00:00:00 2001 From: Ryan Lepinski Date: Mon, 23 Sep 2024 23:52:46 +0200 Subject: [PATCH] [MOBILE-4696] Fix automation engine energy use when offline (#3210) * [MOBILE-4696] Fix automation engine energy use when offline * Test * Fix tests * Fix --- .../Automation/Engine/AutomationEngine.swift | 47 ++++---- .../Engine/AutomationExecutor.swift | 6 +- .../Engine/AutomationPreparer.swift | 3 +- .../AutomationRemoteDataAccess.swift | 2 +- .../Engine/AutomationPreparerTest.swift | 48 ++++++++ .../TestFrequencyLimitsManager.swift | 2 +- Airship/AirshipCore/Source/RemoteData.swift | 54 +++------ .../Source/RemoteDataProtocol.swift | 27 ++++- .../Source/RemoteDataProvider.swift | 9 +- .../Source/RemoteDataProviderProtocol.swift | 3 +- .../AirshipCore/Tests/RemoteDataTest.swift | 104 +++++++++--------- .../AirshipCore/Tests/TestRemoteData.swift | 15 +-- 12 files changed, 173 insertions(+), 147 deletions(-) diff --git a/Airship/AirshipAutomation/Source/Automation/Engine/AutomationEngine.swift b/Airship/AirshipAutomation/Source/Automation/Engine/AutomationEngine.swift index e73f30c27..886d9a091 100644 --- a/Airship/AirshipAutomation/Source/Automation/Engine/AutomationEngine.swift +++ b/Airship/AirshipAutomation/Source/Automation/Engine/AutomationEngine.swift @@ -407,7 +407,6 @@ fileprivate extension AutomationEngine { return } - guard try await self.store.getSchedule(scheduleID: scheduleID) == data else { @@ -564,39 +563,41 @@ fileprivate extension AutomationEngine { triggerSessionID: data.triggerSessionID ) - AirshipLogger.trace("Preparing schedule \(data) result: \(prepareResult)") - if case .cancel = prepareResult { - try await self.store.deleteSchedules(scheduleIDs: [data.schedule.identifier]) - return nil - } - - let updated = try await self.updateState(identifier: data.schedule.identifier) { [date] data in - guard data.isInState([.triggered]) else { return } + AirshipLogger.trace("Finished preparing schedule \(data) result: \(prepareResult)") - switch (prepareResult) { - case .prepared(let preparedSchedule): + switch prepareResult { + case .prepared(let preparedSchedule): + let updated = try await self.updateState(identifier: data.schedule.identifier) { [date] data in data.prepared(info: preparedSchedule.info, date: date.now) - case .invalidate, .cancel: - break - case .skip: - data.prepareCancelled(date: date.now, penalize: false) - case .penalize: - data.prepareCancelled(date: date.now, penalize: true) } - } - switch prepareResult { - case .prepared(let info): + // Make sure its updated + guard let updated else { + await preparer.cancelled(schedule: data.schedule) + return nil + } + return PreparedData( - scheduleData: updated ?? data, - preparedSchedule: info + scheduleData: updated, + preparedSchedule: preparedSchedule ) case .invalidate: await self.startTaskToProcessTriggeredSchedule( scheduleID: data.schedule.identifier ) return nil - case .cancel, .skip, .penalize: + case .cancel: + try await self.store.deleteSchedules(scheduleIDs: [data.schedule.identifier]) + return nil + case .skip: + _ = try await self.updateState(identifier: data.schedule.identifier) { [date] data in + data.prepareCancelled(date: date.now, penalize: false) + } + return nil + case .penalize: + _ = try await self.updateState(identifier: data.schedule.identifier) { [date] data in + data.prepareCancelled(date: date.now, penalize: true) + } return nil } } diff --git a/Airship/AirshipAutomation/Source/Automation/Engine/AutomationExecutor.swift b/Airship/AirshipAutomation/Source/Automation/Engine/AutomationExecutor.swift index 1b795e15f..c9f166dce 100644 --- a/Airship/AirshipAutomation/Source/Automation/Engine/AutomationExecutor.swift +++ b/Airship/AirshipAutomation/Source/Automation/Engine/AutomationExecutor.swift @@ -64,11 +64,7 @@ final class AutomationExecutor: AutomationExecutorProtocol { @MainActor func isValid(schedule: AutomationSchedule) async -> Bool { - guard await self.remoteDataAccess.isCurrent(schedule: schedule) else { - return false - } - - return true + return await self.remoteDataAccess.isCurrent(schedule: schedule) } @MainActor diff --git a/Airship/AirshipAutomation/Source/Automation/Engine/AutomationPreparer.swift b/Airship/AirshipAutomation/Source/Automation/Engine/AutomationPreparer.swift index f743a5228..40d00f1fd 100644 --- a/Airship/AirshipAutomation/Source/Automation/Engine/AutomationPreparer.swift +++ b/Airship/AirshipAutomation/Source/Automation/Engine/AutomationPreparer.swift @@ -110,8 +110,7 @@ struct AutomationPreparer: AutomationPreparerProtocol { ) } catch { AirshipLogger.error("Failed to fetch frequency checker for schedule \(schedule.identifier) error: \(error)") - await self.remoteDataAccess.notifyOutdated(schedule: schedule) - return .success(result: .invalidate) + return .success(result: .skip) } let deviceInfoProvider = self.deviceInfoProviderFactory( diff --git a/Airship/AirshipAutomation/Source/RemoteData/AutomationRemoteDataAccess.swift b/Airship/AirshipAutomation/Source/RemoteData/AutomationRemoteDataAccess.swift index 5ad3dbf05..fdc1cd723 100644 --- a/Airship/AirshipAutomation/Source/RemoteData/AutomationRemoteDataAccess.swift +++ b/Airship/AirshipAutomation/Source/RemoteData/AutomationRemoteDataAccess.swift @@ -106,7 +106,7 @@ final class AutomationRemoteDataAccess: AutomationRemoteDataAccessProtocol { return true } - // if we are connected wait for refresh + // if we are connected wait for refresh attempt if (await network.isConnected) { await remoteData.waitRefreshAttempt(source: source) } diff --git a/Airship/AirshipAutomation/Tests/Automation/Engine/AutomationPreparerTest.swift b/Airship/AirshipAutomation/Tests/Automation/Engine/AutomationPreparerTest.swift index b155e8cd9..a05abb9ef 100644 --- a/Airship/AirshipAutomation/Tests/Automation/Engine/AutomationPreparerTest.swift +++ b/Airship/AirshipAutomation/Tests/Automation/Engine/AutomationPreparerTest.swift @@ -342,6 +342,54 @@ final class AutomationPreparerTest: XCTestCase { XCTAssertNotNil(prepared.frequencyChecker) } + func testPrepareMessageCheckerError() async throws { + let automationSchedule = AutomationSchedule( + identifier: UUID().uuidString, + data: .inAppMessage( + InAppMessage(name: "name", displayContent: .custom(.null)) + ), + triggers: [], + created: Date(), + lastUpdated: Date(), + audience: AutomationAudience( + audienceSelector: DeviceAudienceSelector(), + missBehavior: .penalize + ), + campaigns: .string("campaigns"), + frequencyConstraintIDs: ["constraint"] + ) + + self.remoteDataAccess.contactIDBlock = { _ in + return "contact ID" + } + + self.remoteDataAccess.requiresUpdateBlock = { _ in + return false + } + + self.remoteDataAccess.bestEffortRefreshBlock = { _ in + return true + } + + self.audienceChecker.onEvaluate = { _, _, provider in + return true + } + + await self.frequencyLimits.setCheckerBlock { _ in + throw AirshipErrors.error("Failed") + } + + let triggerSessionID = UUID().uuidString + + let result = await self.preparer.prepare( + schedule: automationSchedule, + triggerContext: triggerContext, + triggerSessionID: triggerSessionID + ) + + XCTAssertTrue(result.isSkipped) + } + func testAdditionalAudienceMiss() async throws { let automationSchedule = AutomationSchedule( identifier: UUID().uuidString, diff --git a/Airship/AirshipAutomation/Tests/Test Utils/TestFrequencyLimitsManager.swift b/Airship/AirshipAutomation/Tests/Test Utils/TestFrequencyLimitsManager.swift index cce457f0e..095ae2b80 100644 --- a/Airship/AirshipAutomation/Tests/Test Utils/TestFrequencyLimitsManager.swift +++ b/Airship/AirshipAutomation/Tests/Test Utils/TestFrequencyLimitsManager.swift @@ -13,7 +13,7 @@ final actor TestFrequencyLimitManager: FrequencyLimitManagerProtocol { private var checkerBlock: (@Sendable ([String]) async throws -> FrequencyCheckerProtocol)? - func setCheckerBlock(_ checkerBlock: @Sendable @escaping ([String]) -> FrequencyCheckerProtocol) { + func setCheckerBlock(_ checkerBlock: @Sendable @escaping ([String]) throws -> FrequencyCheckerProtocol) { self.checkerBlock = checkerBlock } diff --git a/Airship/AirshipCore/Source/RemoteData.swift b/Airship/AirshipCore/Source/RemoteData.swift index d2a59c695..429ad92f5 100644 --- a/Airship/AirshipCore/Source/RemoteData.swift +++ b/Airship/AirshipCore/Source/RemoteData.swift @@ -199,7 +199,9 @@ final class RemoteData: AirshipComponent, RemoteDataProtocol { public func notifyOutdated(remoteDataInfo: RemoteDataInfo) async { for provider in self.providers { if (provider.source == remoteDataInfo.source) { - await provider.notifyOutdated(remoteDataInfo: remoteDataInfo) + if (await provider.notifyOutdated(remoteDataInfo: remoteDataInfo)) { + enqueueRefreshTask() + } return } } @@ -313,14 +315,13 @@ final class RemoteData: AirshipComponent, RemoteDataProtocol { return success ? .success : .failure } - @discardableResult - public func refresh() async -> Bool { - return await self.refresh(sources: self.providers.map { $0.source}) - } - - @discardableResult - public func refresh(source: RemoteDataSource) async -> Bool { - return await self.refresh(sources: [source]) + public func forceRefresh() async { + self.updateChangeToken() + enqueueRefreshTask() + let sources = self.providers.map { $0.source } + for source in sources { + await self.waitRefreshAttempt(source: source) + } } public func waitRefresh(source: RemoteDataSource) async { @@ -331,9 +332,9 @@ final class RemoteData: AirshipComponent, RemoteDataProtocol { source: RemoteDataSource, maxTime: TimeInterval? ) async { - AirshipLogger.trace("Waiting for remote data to refresh \(source)") + AirshipLogger.trace("Waiting for remote data to refresh succesfully \(source)") await waitRefreshStatus(source: source, maxTime: maxTime) { status in - status != .none + status == .success } } @@ -345,7 +346,7 @@ final class RemoteData: AirshipComponent, RemoteDataProtocol { source: RemoteDataSource, maxTime: TimeInterval? ) async { - AirshipLogger.trace("Waiting for remote data to refresh successfully \(source)") + AirshipLogger.trace("Waiting for remote refresh attempt \(source)") await waitRefreshStatus(source: source, maxTime: maxTime) { status in status != .none } @@ -384,34 +385,7 @@ final class RemoteData: AirshipComponent, RemoteDataProtocol { AirshipLogger.trace("Remote data refresh: \(source), status: \(result)") } - - private func refresh(sources: [RemoteDataSource]) async -> Bool { - // Refresh task will refresh all remoteDataHandlers. If we only care about - // a subset of the sources, we need filter out those results and collect - // the expected count of sources. - var cancellable: AnyCancellable? - let result = await withCheckedContinuation { continuation in - cancellable = self.refreshResultSubject - .filter { result in - sources.contains(result.source) - } - .collect(sources.count) - .map { results in - !results.contains { result in - result.result == .failed - } - } - .first() - .sink { result in - continuation.resume(returning: result) - } - - enqueueRefreshTask() - } - cancellable?.cancel() - return result - } - + public func payloads(types: [String]) async -> [RemoteDataPayload] { var payloads: [RemoteDataPayload] = [] for provider in self.providers { diff --git a/Airship/AirshipCore/Source/RemoteDataProtocol.swift b/Airship/AirshipCore/Source/RemoteDataProtocol.swift index b530e1228..b155ebc0c 100644 --- a/Airship/AirshipCore/Source/RemoteDataProtocol.swift +++ b/Airship/AirshipCore/Source/RemoteDataProtocol.swift @@ -18,14 +18,29 @@ public protocol RemoteDataProtocol: AnyObject, Sendable { func publisher(types: [String]) -> AnyPublisher<[RemoteDataPayload], Never> func payloads(types: [String]) async -> [RemoteDataPayload] + /// Waits for a successful refresh + /// - Parameters: + /// - source: The remote data source. + /// - maxTime: The max time to wait func waitRefresh(source: RemoteDataSource, maxTime: TimeInterval?) async - func waitRefreshAttempt(source: RemoteDataSource, maxTime: TimeInterval?) async + + /// Waits for a successful refresh + /// - Parameters: + /// - source: The remote data source. func waitRefresh(source: RemoteDataSource) async - func waitRefreshAttempt(source: RemoteDataSource) async - @discardableResult - func refresh() async -> Bool + /// Waits for a refresh attempt for the session. + /// - Parameters: + /// - source: The remote data source. + /// - maxTime: The max time to wait + func waitRefreshAttempt(source: RemoteDataSource, maxTime: TimeInterval?) async + + /// Waits for a refresh attempt for the session. + /// - Parameters: + /// - source: The remote data source. + func waitRefreshAttempt(source: RemoteDataSource) async - @discardableResult - func refresh(source: RemoteDataSource) async -> Bool + /// Forces a refresh attempt. This should generally never be called externally. Currently being exposed for + /// test apps. + func forceRefresh() async } diff --git a/Airship/AirshipCore/Source/RemoteDataProvider.swift b/Airship/AirshipCore/Source/RemoteDataProvider.swift index 79cf655ef..ee11ef676 100644 --- a/Airship/AirshipCore/Source/RemoteDataProvider.swift +++ b/Airship/AirshipCore/Source/RemoteDataProvider.swift @@ -117,10 +117,13 @@ actor RemoteDataProvider: RemoteDataProviderProtocol { } } - func notifyOutdated(remoteDataInfo: RemoteDataInfo) { - if (self.refreshState?.remoteDataInfo == remoteDataInfo) { - self.refreshState = nil + func notifyOutdated(remoteDataInfo: RemoteDataInfo) -> Bool { + guard self.refreshState?.remoteDataInfo == remoteDataInfo else { + return false } + + self.refreshState = nil + return true } func isCurrent(locale: Locale, randomeValue: Int) async -> Bool { diff --git a/Airship/AirshipCore/Source/RemoteDataProviderProtocol.swift b/Airship/AirshipCore/Source/RemoteDataProviderProtocol.swift index 8ac862a14..102b6828d 100644 --- a/Airship/AirshipCore/Source/RemoteDataProviderProtocol.swift +++ b/Airship/AirshipCore/Source/RemoteDataProviderProtocol.swift @@ -16,7 +16,8 @@ protocol RemoteDataProviderProtocol: Actor { /// Notifies that the remote-data info is outdated. This will cause the next refresh to /// to fetch data. /// - Parameter remoteDataInfo: The remote data info. - func notifyOutdated(remoteDataInfo: RemoteDataInfo) + /// - Returns true if cleared, otherwise false. + func notifyOutdated(remoteDataInfo: RemoteDataInfo) -> Bool /// Checks if the source is current. /// - Parameter locale: The current locale. diff --git a/Airship/AirshipCore/Tests/RemoteDataTest.swift b/Airship/AirshipCore/Tests/RemoteDataTest.swift index 171e5e934..6ae7e7f88 100644 --- a/Airship/AirshipCore/Tests/RemoteDataTest.swift +++ b/Airship/AirshipCore/Tests/RemoteDataTest.swift @@ -119,6 +119,7 @@ final class RemoteDataTest: AirshipBaseTest { await self.contactProvider.setNotifyOutdatedCallback { @Sendable info in XCTAssertEqual(remoteDataInfo, info) expectation.fulfill() + return true } await self.remoteData.notifyOutdated(remoteDataInfo: remoteDataInfo) @@ -137,6 +138,7 @@ final class RemoteDataTest: AirshipBaseTest { await self.appProvider.setNotifyOutdatedCallback { @Sendable info in XCTAssertEqual(remoteDataInfo, info) expectation.fulfill() + return true } await self.remoteData.notifyOutdated(remoteDataInfo: remoteDataInfo) @@ -144,6 +146,24 @@ final class RemoteDataTest: AirshipBaseTest { await self.fulfillmentCompat(of: [expectation], timeout: 10) } + func testNotifyOutdatedEnqueusRefreshTask() async { + let remoteDataInfo = RemoteDataInfo( + url: URL(string: "example://")!, + lastModifiedTime: nil, + source: .app + ) + + XCTAssertEqual(0, testWorkManager.workRequests.count) + + await self.appProvider.setNotifyOutdatedCallback { _ in return false } + await self.remoteData.notifyOutdated(remoteDataInfo: remoteDataInfo) + XCTAssertEqual(0, testWorkManager.workRequests.count) + + await self.appProvider.setNotifyOutdatedCallback { _ in return true } + await self.remoteData.notifyOutdated(remoteDataInfo: remoteDataInfo) + XCTAssertEqual(1, testWorkManager.workRequests.count) + } + func testIsCurrentContact() async { let remoteDataInfo = RemoteDataInfo( url: URL(string: "example://")!, @@ -304,7 +324,7 @@ final class RemoteDataTest: AirshipBaseTest { } - func testRefreshSuccess() async { + func testForceRefresh() async { await self.contactProvider.setRefreshCallback { @Sendable _, _, _ in return .newData } @@ -317,56 +337,10 @@ final class RemoteDataTest: AirshipBaseTest { let remoteData = self.remoteData Task.detached { - let result = await remoteData?.refresh() - XCTAssertTrue(result == true) - refreshFinished.fulfill() - } - - await self.fulfillmentCompat(of: [refreshFinished]) - - XCTAssertEqual(1, testWorkManager.workRequests.count) - } - - func testRefreshFailed() async { - await self.contactProvider.setRefreshCallback { @Sendable _, _, _ in - return .failed - } - await self.appProvider.setRefreshCallback{ @Sendable _, _, _ in - return .newData - } - - self.testWorkManager.autoLaunchRequests = true - let refreshFinished = expectation(description: "refresh finished") - let remoteData = self.remoteData - Task.detached { - let result = await remoteData!.refresh() - XCTAssertFalse(result) - refreshFinished.fulfill() - } - - await self.fulfillmentCompat(of: [refreshFinished]) - XCTAssertEqual(1, testWorkManager.workRequests.count) - } - - func testRefreshSource() async { - await self.contactProvider.setRefreshCallback { @Sendable _, _, _ in - return .failed - } - await self.appProvider.setRefreshCallback{ @Sendable _, _, _ in - return .newData - } - - self.testWorkManager.autoLaunchRequests = true - let refreshFinished = expectation(description: "refresh finished") - let remoteData = self.remoteData - Task.detached { - let result = await remoteData!.refresh(source: .app) - XCTAssertTrue(result) + await remoteData?.forceRefresh() refreshFinished.fulfill() } - await self.launchRefreshTask() - await self.fulfillmentCompat(of: [refreshFinished]) XCTAssertEqual(1, testWorkManager.workRequests.count) @@ -496,7 +470,33 @@ final class RemoteDataTest: AirshipBaseTest { XCTAssertNotEqual(last, changeToken.value) } + func testWaitForRefresh() async { + await self.contactProvider.setRefreshCallback{ _, _, _ in + return .failed + } + await self.appProvider.setRefreshCallback{ _, _, _ in + return .failed + } + + let finished = AirshipMainActorValue(false) + Task { + await self.remoteData.waitRefresh(source: .app) + await finished.set(true) + } + + await self.launchRefreshTask() + var isFinished = await finished.value + XCTAssertFalse(isFinished) + + await self.appProvider.setRefreshCallback{ _, _, _ in + return .newData + } + + await self.launchRefreshTask() + isFinished = await finished.value + XCTAssertTrue(isFinished) + } @discardableResult private func launchRefreshTask() async -> AirshipWorkResult { @@ -523,8 +523,8 @@ fileprivate actor TestRemoteDataProvider: RemoteDataProviderProtocol { private var payloads: [RemoteDataPayload] = [] var enabled: Bool - private var notifyOutdatedCallback: ((RemoteDataInfo) -> Void)? - func setNotifyOutdatedCallback(callback: @escaping (RemoteDataInfo) -> Void) { + private var notifyOutdatedCallback: ((RemoteDataInfo) -> Bool)? + func setNotifyOutdatedCallback(callback: @escaping (RemoteDataInfo) -> Bool) { self.notifyOutdatedCallback = callback } @@ -551,8 +551,8 @@ fileprivate actor TestRemoteDataProvider: RemoteDataProviderProtocol { return payloads.filter { types.contains($0.type) }.sortedByType(types) } - func notifyOutdated(remoteDataInfo: RemoteDataInfo) { - self.notifyOutdatedCallback!(remoteDataInfo) + func notifyOutdated(remoteDataInfo: RemoteDataInfo) -> Bool { + return self.notifyOutdatedCallback!(remoteDataInfo) } func isCurrent(locale: Locale, randomeValue: Int) async -> Bool { diff --git a/Airship/AirshipCore/Tests/TestRemoteData.swift b/Airship/AirshipCore/Tests/TestRemoteData.swift index f3c47149c..aaaf76350 100644 --- a/Airship/AirshipCore/Tests/TestRemoteData.swift +++ b/Airship/AirshipCore/Tests/TestRemoteData.swift @@ -6,6 +6,8 @@ import Combine final class TestRemoteData: NSObject, RemoteDataProtocol, @unchecked Sendable { + func forceRefresh() async { + } var waitForRefreshAttemptBlock: ((RemoteDataSource, TimeInterval?) -> Void)? var waitForRefreshBlock: ((RemoteDataSource, TimeInterval?) -> Void)? @@ -23,8 +25,6 @@ final class TestRemoteData: NSObject, RemoteDataProtocol, @unchecked Sendable { var status: [RemoteDataSource : RemoteDataSourceStatus] = [:] - var refreshBlock: ((RemoteDataSource) -> Bool)? - var remoteDataRefreshInterval: TimeInterval = 0 var isContactSourceEnabled: Bool = false func setContactSourceEnabled(enabled: Bool) { @@ -71,17 +71,6 @@ final class TestRemoteData: NSObject, RemoteDataProtocol, @unchecked Sendable { } } - func refresh() async -> Bool { - return true - } - - func refresh(source: AirshipCore.RemoteDataSource) async -> Bool { - guard let block = self.refreshBlock else { - return true - } - - return block(source) - } func waitRefresh( source: AirshipCore.RemoteDataSource,