From 7baca8cfd8124640840f72fd136d8f9a5f84cf9e Mon Sep 17 00:00:00 2001 From: Jeremy Massel <1123407+jkmassel@users.noreply.github.com> Date: Wed, 28 Oct 2026 19:27:55 -0600 Subject: [PATCH] Fix three activity log problems from #26282 - "Delete Logs" on the Activity Logs screen now deletes every log file, as its alert says, and reloads the list from disk. Removes the now-unused `deleteArchivedLogs`. - The new support screen skips log files it can't read instead of failing the whole list. - Crash reports attach the log file that was being written at the time of the event, instead of always the newest one. --- .../Tests/Utility/LogFileStoreTests.swift | 2 +- .../Tests/Utility/LogFileTests.swift | 60 +++++++++++++++++++ .../Logging/EventLoggingDataProvider.swift | 18 ++++-- WordPress/Classes/Utility/Logging/WPLogger.h | 5 -- WordPress/Classes/Utility/Logging/WPLogger.m | 12 ---- .../SupportActivityLogView.swift | 17 ++++-- .../NewSupport/SupportDataProvider.swift | 15 ++++- 7 files changed, 99 insertions(+), 30 deletions(-) create mode 100644 Tests/KeystoneTests/Tests/Utility/LogFileTests.swift diff --git a/Tests/KeystoneTests/Tests/Utility/LogFileStoreTests.swift b/Tests/KeystoneTests/Tests/Utility/LogFileStoreTests.swift index df9c32a6d385..7e498825fd19 100644 --- a/Tests/KeystoneTests/Tests/Utility/LogFileStoreTests.swift +++ b/Tests/KeystoneTests/Tests/Utility/LogFileStoreTests.swift @@ -41,7 +41,7 @@ struct LogFileStoreTests { #expect(LogFileStore(fileLogger: makeFileLogger()).logFiles().isEmpty) } - /// Crash reports and Zendesk attach the file the app is writing to, which is the newest one. + /// Events sent now (Zendesk, non-fatal errors) attach the file the app is writing to, which is the newest one. @Test func eventLoggingAttachesTheNewestFile() { let newest = AppLogFile(url: URL(fileURLWithPath: "/tmp/logs/today.log"), creationDate: Date()) let older = AppLogFile( diff --git a/Tests/KeystoneTests/Tests/Utility/LogFileTests.swift b/Tests/KeystoneTests/Tests/Utility/LogFileTests.swift new file mode 100644 index 000000000000..ed48c131e6ad --- /dev/null +++ b/Tests/KeystoneTests/Tests/Utility/LogFileTests.swift @@ -0,0 +1,60 @@ +import Foundation +import Testing + +@testable import WordPress + +struct EventLoggingDataProviderTests { + private let now = Date(timeIntervalSince1970: 1_800_000_000) + + private func logFile(_ name: String, createdSecondsAgo seconds: TimeInterval?) -> AppLogFile { + AppLogFile( + url: URL(fileURLWithPath: "/logs/\(name).log"), + creationDate: seconds.map { now.addingTimeInterval(-$0) } + ) + } + + @Test func picksTheFileThatWasCurrentAtTheEventDate() { + // The app crashed an hour ago, then a new log file was started on relaunch. + let files = [logFile("relaunch", createdSecondsAgo: 60), logFile("crashed", createdSecondsAgo: 86_400)] + + let selected = EventLoggingDataProvider.logFile(in: files, at: now.addingTimeInterval(-3_600)) + + #expect(selected?.url.lastPathComponent == "crashed.log") + } + + @Test func picksTheNewestFileForTheCurrentSession() { + let files = [logFile("current", createdSecondsAgo: 60), logFile("older", createdSecondsAgo: 86_400)] + + let selected = EventLoggingDataProvider.logFile(in: files, at: now) + + #expect(selected?.url.lastPathComponent == "current.log") + } + + @Test func fallsBackToTheNewestFileWhenNoneQualifies() { + let files = [logFile("newest", createdSecondsAgo: nil), logFile("older", createdSecondsAgo: 60)] + + let selected = EventLoggingDataProvider.logFile(in: files, at: now.addingTimeInterval(-3_600)) + + #expect(selected?.url.lastPathComponent == "newest.log") + } + + @Test func returnsNilWithoutLogFiles() { + #expect(EventLoggingDataProvider.logFile(in: [], at: now) == nil) + } +} + +struct WpLogDataProviderTests { + @Test func skipsFilesThatNoLongerExist() throws { + let directory = FileManager.default.temporaryDirectory.appendingPathComponent(UUID().uuidString) + try FileManager.default.createDirectory(at: directory, withIntermediateDirectories: true) + defer { try? FileManager.default.removeItem(at: directory) } + + let existing = directory.appendingPathComponent("existing.log") + try Data("log".utf8).write(to: existing) + let missing = directory.appendingPathComponent("missing.log") + + let logs = WpLogDataProvider.applicationLogs(atPaths: [missing.path, existing.path]) + + #expect(logs.map(\.path.lastPathComponent) == ["existing.log"]) + } +} diff --git a/WordPress/Classes/Utility/Logging/EventLoggingDataProvider.swift b/WordPress/Classes/Utility/Logging/EventLoggingDataProvider.swift index 23ea93110f86..ea52e36a2bc3 100644 --- a/WordPress/Classes/Utility/Logging/EventLoggingDataProvider.swift +++ b/WordPress/Classes/Utility/Logging/EventLoggingDataProvider.swift @@ -5,9 +5,9 @@ import AutomatticEncryptedLogs struct EventLoggingDataProvider: EventLoggingDataSource { - typealias LogFilesCallback = (() -> [URL]) + typealias LogFilesCallback = (() -> [AppLogFile]) - /// A block that returns all existing log files + /// A block that returns all existing log files, newest first private let fetchLogFiles: LogFilesCallback? let loggingAuthenticationToken: String @@ -27,14 +27,22 @@ struct EventLoggingDataProvider: EventLoggingDataSource { self.fetchLogFiles = block } - /// The current session log will almost always be the correct one, because they're split by day + /// Returns the log file that was being written to at `date`. + /// + /// A crash is sent on the next launch, which may have started a new log file, so the newest file isn't always the + /// right one. The file that was current at `date` is the newest one created at or before it. If no file qualifies + /// (for example, because creation dates are unavailable), the newest file is returned. func logFilePath(forErrorLevel: EventLoggingErrorType, at date: Date) -> URL? { - return fetchLogFiles?().first + Self.logFile(in: fetchLogFiles?() ?? [], at: date)?.url + } + + static func logFile(in newestFirst: [AppLogFile], at date: Date) -> AppLogFile? { + newestFirst.first { $0.creationDate.map { $0 <= date } ?? false } ?? newestFirst.first } static func from(_ store: LogFileStore) -> EventLoggingDataSource { EventLoggingDataProvider { - store.logFiles().map(\.url) + store.logFiles() } } } diff --git a/WordPress/Classes/Utility/Logging/WPLogger.h b/WordPress/Classes/Utility/Logging/WPLogger.h index a96b7d24597f..f6dd2efcfefc 100644 --- a/WordPress/Classes/Utility/Logging/WPLogger.h +++ b/WordPress/Classes/Utility/Logging/WPLogger.h @@ -22,9 +22,4 @@ */ - (void)deleteAllLogs; -/** - * @brief Deletes all the old archived logs from the device - */ -- (void)deleteArchivedLogs; - @end diff --git a/WordPress/Classes/Utility/Logging/WPLogger.m b/WordPress/Classes/Utility/Logging/WPLogger.m index b25aba6faed1..8b5daf0efefa 100644 --- a/WordPress/Classes/Utility/Logging/WPLogger.m +++ b/WordPress/Classes/Utility/Logging/WPLogger.m @@ -84,18 +84,6 @@ - (void)deleteAllLogs WPLogWarning(@"All log files erased."); } -- (void)deleteArchivedLogs -{ - NSArray *logFiles = self.fileLogger.logFileManager.sortedLogFileInfos; - for (DDLogFileInfo *logFileInfo in logFiles) { - if (logFileInfo.isArchived) { - [[NSFileManager defaultManager] removeItemAtPath:logFileInfo.filePath error:nil]; - } - } - - WPLogWarning(@"All archived log files erased."); -} - #pragma mark - Public static methods + (void)configureLoggerLevelWithExtraDebug { diff --git a/WordPress/Classes/ViewRelated/Me/Help & Support/SupportActivityLogView.swift b/WordPress/Classes/ViewRelated/Me/Help & Support/SupportActivityLogView.swift index 9bf825e0899c..90c3cd5ef1e7 100644 --- a/WordPress/Classes/ViewRelated/Me/Help & Support/SupportActivityLogView.swift +++ b/WordPress/Classes/ViewRelated/Me/Help & Support/SupportActivityLogView.swift @@ -41,15 +41,18 @@ struct SupportActivityLogView: View { private class ActivityLogViewModel: ObservableObject { @Published private(set) var logFiles: [AppLogFile] = [] + private let store: LogFileStore private let dateFormatter: DateFormatter init(store: LogFileStore = .shared) { + self.store = store + dateFormatter = DateFormatter() dateFormatter.dateStyle = .medium dateFormatter.doesRelativeDateFormatting = true dateFormatter.timeStyle = .short - logFiles = store.logFiles() + reloadLogFiles() } func getFormattedDate(for logFile: AppLogFile) -> String { @@ -63,8 +66,12 @@ private class ActivityLogViewModel: ObservableObject { } func buttonDeleteLogsTapped() { - WPLogger.shared().deleteArchivedLogs() - logFiles = [] + WPLogger.shared().deleteAllLogs() + reloadLogFiles() + } + + private func reloadLogFiles() { + logFiles = store.logFiles() } } @@ -72,6 +79,6 @@ private enum Strings { static let title = NSLocalizedString("support.activityLogs.navigationTitle", value: "Activity Logs", comment: "Title shown in the navigation bar of the Activity Logs screen.") static let currentLog = NSLocalizedString("support.activityLogs.currentLogTilte", value: "Current", comment: "Label for the current activity log file in the list.") static let sectionFooter = NSLocalizedString("support.activityLogs.sectionFooter", value: "Up to seven days worth of logs are saved.", comment: "Footer text explaining the log retention policy in the Activity Logs screen.") - static let deleteTitle = NSLocalizedString("support.activityLogs.deleteLogsAlert.title", value: "Delete Logs", comment: "Title shown in the alert when attempting to clear old activity logs.") - static let deleteMessage = NSLocalizedString("support.activityLogs.deleteLogsAlert.message", value: "Are you sure you want to delete all activity logs?", comment: "Message shown in the alert when attempting to clear old activity logs.") + static let deleteTitle = NSLocalizedString("support.activityLogs.deleteLogsAlert.title", value: "Delete Logs", comment: "Title shown in the alert when attempting to delete all activity logs.") + static let deleteMessage = NSLocalizedString("support.activityLogs.deleteLogsAlert.message", value: "Are you sure you want to delete all activity logs?", comment: "Message shown in the alert when attempting to delete all activity logs.") } diff --git a/WordPress/Classes/ViewRelated/NewSupport/SupportDataProvider.swift b/WordPress/Classes/ViewRelated/NewSupport/SupportDataProvider.swift index 52364ec89232..0f5025b25e05 100644 --- a/WordPress/Classes/ViewRelated/NewSupport/SupportDataProvider.swift +++ b/WordPress/Classes/ViewRelated/NewSupport/SupportDataProvider.swift @@ -36,8 +36,19 @@ actor WpLogDataProvider: ApplicationLogDataProvider { } func fetchApplicationLogs() async throws -> [Support.ApplicationLog] { - try LogFileStore.shared.logFiles() - .compactMap { try ApplicationLog(filePath: $0.url.path) } + Self.applicationLogs(atPaths: LogFileStore.shared.logFiles().map(\.url.path)) + } + + /// A file can be removed between listing the folder and reading its attributes (for example, by CocoaLumberjack's + /// automatic cleanup), so files that can't be read are skipped rather than failing the whole list. + static func applicationLogs(atPaths paths: [String]) -> [Support.ApplicationLog] { + paths.compactMap { path in + do { + return try ApplicationLog(filePath: path) + } catch { + return nil + } + } } func deleteApplicationLogs(in logs: [Support.ApplicationLog]) async throws {