Address updater feedback #3690

quanganhdo · 2024-12-31T04:16:21Z

Task/Issue URL: https://app.asana.com/0/1201048563534612/1209026627874986/f
Tech Design URL:
CC:

Description:

Addresses some updater feedback:

"Install Now" menu item does nothing
Incorrect Release Notes screen state

Also adds more logging to help track down the edge cases (non-producible on my machine, but some internal users kept running into them)

Optional E2E tests:

Run PIR E2E tests
Check this to run the Personal Information Removal end to end tests. If updating CCF, or any PIR related code, tick this.

Steps to test this PR:

Download this test build: https://staticcdn.duckduckgo.com/macos-desktop-browser/a81f9679-e41d-404d-81a6-fd4a461d82f7/testbuilds/5ab32a1/duckduckgo-1.120.0.333.dmg
Try different scenarios; use this as reference: https://app.asana.com/0/1199230911884351/1208882660080993/f
Run this Terminal command: log show --info --predicate 'subsystem == "Updates"' --style compact --last 24h > ~/Downloads/updater.log
Give the log file a quick skim; all major update steps should be there.

Definition of Done:

Does this PR satisfy our Definition of Done?

Internal references:

Pull Request Review Checklist
Software Engineering Expectations
Technical Design Template
Pull Request Documentation

quanganhdo · 2024-12-31T04:23:07Z

DuckDuckGo/Updates/ReleaseNotesTabExtension.swift

+            self.init(status: updateController.updateProgress.toStatus,
                      currentVersion: currentVersion,
-                      lastUpdate: lastUpdate)
+                      lastUpdate: lastUpdate,
+                      automaticUpdate: updateController.areAutomaticUpdatesEnabled)


This happens when Release Notes screen is displayed but the appcast hasn't been loaded yet. We get both incorrect status (updateReady), wrong button text (Restart to Update / Update DuckDuckGo), and the button doesn't even work.

For more details: https://app.asana.com/0/0/1209058536716904/f

quanganhdo · 2024-12-31T04:23:55Z

DuckDuckGo/Updates/ReleaseNotesTabExtension.swift

@@ -198,7 +199,7 @@ private extension UpdateCycleProgress {
        case .downloadDidStart, .downloading: return .updateDownloading
        case .extractionDidStart, .extracting, .readyToInstallAndRelaunch, .installationDidStart, .installing: return .updatePreparing
        case .updaterError: return .updateError
-        case .updateCycleNotStarted, .updateCycleDone: return .updateReady
+        case .updateCycleNotStarted, .updateCycleDone: return .loaded


These should've been defaulted to loaded as we only want updateReady if we truly have a pending update.

quanganhdo · 2024-12-31T04:25:02Z

DuckDuckGo/Updates/ReleaseNotesUserScript.swift

+        guard let updateController = Application.appDelegate.updateController else {
+            return
+        }


Just to satisfy the compiler as AppDelegate.updateController is implicitly unwrapped already.

quanganhdo · 2024-12-31T04:28:12Z

DuckDuckGo/Updates/UpdateController.swift

+    func updater(_ updater: SPUUpdater, willInstallUpdateOnQuit item: SUAppcastItem, immediateInstallationBlock immediateInstallHandler: @escaping () -> Void) -> Bool {
+        Logger.updates.log("Updater will install update on quit: \(item.displayVersionString, privacy: .public)(\(item.versionString, privacy: .public))")
+        userDriver?.configureResumeBlock(immediateInstallHandler)
+        return true


This delegate method isn't called in my testing scenarios, but when it does in prod, let's make sure we use the immediateInstallHandler to set up our resume block for Install Now.

quanganhdo · 2024-12-31T04:33:36Z

DuckDuckGo/Updates/UpdateUserDriver.swift

            reply(.dismiss)
        }

-        onSkipping = { reply(.skip) }
+        onDismiss = { reply(.dismiss) }


Skip can ignore the update, which we don't want.

quanganhdo · 2024-12-31T04:34:30Z

DuckDuckGo/Updates/UpdateUserDriver.swift

+        if !applicationTerminated {
+            Logger.updates.log("Updater re-sent a quit event")
+            retryTerminatingApplication()
+        }


Not strictly necessary, but there seems to be no harm in doing this.

quanganhdo · 2025-01-01T04:49:30Z

DuckDuckGo/Updates/ReleaseNotesTabExtension.swift

        case .downloadDidStart, .downloading: return .updateDownloading
        case .extractionDidStart, .extracting, .readyToInstallAndRelaunch, .installationDidStart, .installing: return .updatePreparing
        case .updaterError: return .updateError
-        case .updateCycleNotStarted, .updateCycleDone: return .updateReady
+        case .updateCycleDone: return .loaded


updateReady should never be returned here unless we're truly ready to proceed with an update.

…asure)

jotaemepereira

@quanganhdo I went through the different scenarios on: https://app.asana.com/0/1199230911884351/1208882660080993/f, and everything work as expected. Let’s merge this into the release branch 👍🏼

quanganhdo changed the base branch from main to anh/fixes/frozen-about December 31, 2024 04:17

quanganhdo force-pushed the anh/fixes/updater branch from 3aee5b7 to 120d1f5 Compare December 31, 2024 04:18

quanganhdo changed the base branch from anh/fixes/frozen-about to main December 31, 2024 04:18

quanganhdo changed the base branch from main to anh/fixes/frozen-about December 31, 2024 04:18

quanganhdo commented Dec 31, 2024

View reviewed changes

quanganhdo requested a review from tomasstrba December 31, 2024 04:35

Base automatically changed from anh/fixes/frozen-about to main December 31, 2024 11:04

quanganhdo commented Jan 1, 2025

View reviewed changes

quanganhdo added 11 commits January 1, 2025 23:09

Do not show menu item if resume block is nil

5f3c18c

Use .loaded instead of .updateReady as default state

07e380a

.dismiss instead of .skip when toggling update setting

3d901ba

Update resume block when install-on-quit update is ready (for good me…

1c77b97

…asure)

Retry a quit event just in case

4dfb9f1

Add more logging

01eea2a

updateCycleNotStarted should return loading

e31f33f

Add updateCycleDone reason

065775b

Log all pixel parameters

9e80ffa

Fix private log

ac2af47

Fix Swiftlint

4fed45f

quanganhdo force-pushed the anh/fixes/updater branch from 77d244a to 4fed45f Compare January 1, 2025 16:09

quanganhdo changed the base branch from main to release/1.120.0 January 1, 2025 16:09

quanganhdo requested a review from jotaemepereira January 1, 2025 16:20

jotaemepereira approved these changes Jan 2, 2025

View reviewed changes

jotaemepereira merged commit 90087ad into release/1.120.0 Jan 2, 2025
31 checks passed

jotaemepereira deleted the anh/fixes/updater branch January 2, 2025 13:34

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Address updater feedback #3690

Address updater feedback #3690

quanganhdo commented Dec 31, 2024 •

edited

Loading

quanganhdo Dec 31, 2024 •

edited

Loading

quanganhdo Dec 31, 2024

quanganhdo Dec 31, 2024 •

edited

Loading

quanganhdo Dec 31, 2024

quanganhdo Dec 31, 2024

quanganhdo Dec 31, 2024

quanganhdo Jan 1, 2025

jotaemepereira left a comment

Address updater feedback #3690

Address updater feedback #3690

Conversation

quanganhdo commented Dec 31, 2024 • edited Loading

Internal references:

quanganhdo Dec 31, 2024 • edited Loading

Choose a reason for hiding this comment

quanganhdo Dec 31, 2024

Choose a reason for hiding this comment

quanganhdo Dec 31, 2024 • edited Loading

Choose a reason for hiding this comment

quanganhdo Dec 31, 2024

Choose a reason for hiding this comment

quanganhdo Dec 31, 2024

Choose a reason for hiding this comment

quanganhdo Dec 31, 2024

Choose a reason for hiding this comment

quanganhdo Jan 1, 2025

Choose a reason for hiding this comment

jotaemepereira left a comment

Choose a reason for hiding this comment

quanganhdo commented Dec 31, 2024 •

edited

Loading

quanganhdo Dec 31, 2024 •

edited

Loading

quanganhdo Dec 31, 2024 •

edited

Loading