Skip to content

Commit

Permalink
Fix phased rollout handling (#3625)
Browse files Browse the repository at this point in the history
Task/Issue URL:
https://app.asana.com/0/1201037661562251/1208867792197198/f
Tech Design URL: 
CC:

**Description**:

Fixes automatic update skipping phased rollout group setting.

**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**:
1. https://app.asana.com/0/0/1208882660080993/f

<!--
Tagging instructions
If this PR isn't ready to be merged for whatever reason it should be
marked with the `DO NOT MERGE` label (particularly if it's a draft)
If it's pending Product Review/PFR, please add the `Pending Product
Review` label.

If at any point it isn't actively being worked on/ready for
review/otherwise moving forward (besides the above PR/PFR exception)
strongly consider closing it (or not opening it in the first place). If
you decide not to close it, make sure it's labelled to make it clear the
PRs state and comment with more information.
-->

**Definition of Done**:

* [ ] Does this PR satisfy our [Definition of
Done](https://app.asana.com/0/1202500774821704/1207634633537039/f)?

---
###### Internal references:
[Pull Request Review
Checklist](https://app.asana.com/0/1202500774821704/1203764234894239/f)
[Software Engineering
Expectations](https://app.asana.com/0/59792373528535/199064865822552)
[Technical Design
Template](https://app.asana.com/0/59792373528535/184709971311943)
[Pull Request
Documentation](https://app.asana.com/0/1202500774821704/1204012835277482/f)
  • Loading branch information
quanganhdo authored Dec 5, 2024
1 parent 7b2a893 commit 786ca2d
Show file tree
Hide file tree
Showing 7 changed files with 49 additions and 27 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -299,6 +299,10 @@
argument = "-NSConstraintBasedLayoutVisualizeMutuallyExclusiveConstraints YES"
isEnabled = "YES">
</CommandLineArgument>
<CommandLineArgument
argument = "-SUEnableAutomaticChecks NO"
isEnabled = "YES">
</CommandLineArgument>
<CommandLineArgument
argument = "-com.apple.CoreData.ConcurrencyDebug 1"
isEnabled = "YES">
Expand Down
2 changes: 1 addition & 1 deletion DuckDuckGo/Preferences/Model/AboutPreferences.swift
Original file line number Diff line number Diff line change
Expand Up @@ -71,7 +71,7 @@ final class AboutPreferences: ObservableObject, PreferencesTabOpening {

#if SPARKLE
func checkForUpdate() {
updateController?.checkForUpdateIfNeeded()
updateController?.checkForUpdateSkippingRollout()
}

func runUpdate() {
Expand Down
5 changes: 0 additions & 5 deletions DuckDuckGo/Preferences/View/PreferencesAboutView.swift
Original file line number Diff line number Diff line change
Expand Up @@ -51,11 +51,6 @@ extension Preferences {
}
}
}
.onAppear {
#if SPARKLE && !DEBUG
model.checkForUpdate()
#endif
}
}
}

Expand Down
6 changes: 3 additions & 3 deletions DuckDuckGo/Updates/ReleaseNotesTabExtension.swift
Original file line number Diff line number Diff line change
Expand Up @@ -105,11 +105,11 @@ final class ReleaseNotesTabExtension: NavigationResponder {

@MainActor
func navigationDidFinish(_ navigation: Navigation) {
#if !DEBUG
guard NSApp.runType != .uiTests, navigation.url == .releaseNotes else { return }
let updateController = Application.appDelegate.updateController!
updateController.checkForUpdateIfNeeded()
#endif
if updateController.latestUpdate?.needsLatestReleaseNote == true {
updateController.checkForUpdateSkippingRollout()
}
}
}

Expand Down
2 changes: 1 addition & 1 deletion DuckDuckGo/Updates/ReleaseNotesUserScript.swift
Original file line number Diff line number Diff line change
Expand Up @@ -113,7 +113,7 @@ extension ReleaseNotesUserScript {
@MainActor
private func retryUpdate(params: Any, original: WKScriptMessage) async throws -> Encodable? {
DispatchQueue.main.async { [weak self] in
self?.updateController.checkForUpdateIfNeeded()
self?.updateController.checkForUpdateSkippingRollout()
}
return nil
}
Expand Down
10 changes: 7 additions & 3 deletions DuckDuckGo/Updates/Update.swift
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,7 @@ final class Update {
let date: Date
let releaseNotes: [String]
let releaseNotesPrivacyPro: [String]
let needsLatestReleaseNote: Bool

var title: String {
let formatter = DateFormatter()
Expand All @@ -47,20 +48,22 @@ final class Update {
build: String,
date: Date,
releaseNotes: [String],
releaseNotesPrivacyPro: [String]) {
releaseNotesPrivacyPro: [String],
needsLatestReleaseNote: Bool) {
self.isInstalled = isInstalled
self.type = type
self.version = version
self.build = build
self.date = date
self.releaseNotes = releaseNotes
self.releaseNotesPrivacyPro = releaseNotesPrivacyPro
self.needsLatestReleaseNote = needsLatestReleaseNote
}

}

extension Update {
convenience init(appcastItem: SUAppcastItem, isInstalled: Bool) {
convenience init(appcastItem: SUAppcastItem, isInstalled: Bool, needsLatestReleaseNote: Bool) {
let isCritical = appcastItem.isCriticalUpdate
let version = appcastItem.displayVersionString
let build = appcastItem.versionString
Expand All @@ -73,7 +76,8 @@ extension Update {
build: build,
date: date,
releaseNotes: releaseNotes,
releaseNotesPrivacyPro: releaseNotesPrivacyPro)
releaseNotesPrivacyPro: releaseNotesPrivacyPro,
needsLatestReleaseNote: needsLatestReleaseNote)
}
}

Expand Down
47 changes: 33 additions & 14 deletions DuckDuckGo/Updates/UpdateController.swift
Original file line number Diff line number Diff line change
Expand Up @@ -42,7 +42,8 @@ protocol UpdateControllerProtocol: AnyObject {

var lastUpdateCheckDate: Date? { get }

func checkForUpdateIfNeeded()
func checkForUpdateRespectingRollout()
func checkForUpdateSkippingRollout()
func runUpdate()

var areAutomaticUpdatesEnabled: Bool { get set }
Expand All @@ -64,13 +65,20 @@ final class UpdateController: NSObject, UpdateControllerProtocol {
struct UpdateCheckResult {
let item: SUAppcastItem
let isInstalled: Bool
let needsLatestReleaseNote: Bool

init(item: SUAppcastItem, isInstalled: Bool, needsLatestReleaseNote: Bool = false) {
self.item = item
self.isInstalled = isInstalled
self.needsLatestReleaseNote = needsLatestReleaseNote
}
}
private var cachedUpdateResult: UpdateCheckResult?

@Published private(set) var updateProgress = UpdateCycleProgress.default {
didSet {
if let cachedUpdateResult {
latestUpdate = Update(appcastItem: cachedUpdateResult.item, isInstalled: cachedUpdateResult.isInstalled)
latestUpdate = Update(appcastItem: cachedUpdateResult.item, isInstalled: cachedUpdateResult.isInstalled, needsLatestReleaseNote: cachedUpdateResult.needsLatestReleaseNote)
hasPendingUpdate = latestUpdate?.isInstalled == false && updateProgress.isDone && userDriver?.isResumable == true
needsNotificationDot = hasPendingUpdate
}
Expand Down Expand Up @@ -101,6 +109,7 @@ final class UpdateController: NSObject, UpdateControllerProtocol {
if oldValue != areAutomaticUpdatesEnabled {
userDriver?.cancelAndDismissCurrentUpdate()
try? configureUpdater()
checkForUpdateSkippingRollout()
}
}
}
Expand Down Expand Up @@ -129,6 +138,7 @@ final class UpdateController: NSObject, UpdateControllerProtocol {
super.init()

try? configureUpdater()
checkForUpdateRespectingRollout()
}

func checkNewApplicationVersion() {
Expand All @@ -142,10 +152,18 @@ final class UpdateController: NSObject, UpdateControllerProtocol {
}
}

func checkForUpdateIfNeeded() {
func checkForUpdateRespectingRollout() {
guard let updater, !updater.sessionInProgress else { return }

Logger.updates.log("Checking for updates respecting rollout")

updater.checkForUpdatesInBackground()
}

func checkForUpdateSkippingRollout() {
guard let updater, !updater.sessionInProgress else { return }

Logger.updates.log("Checking for updates")
Logger.updates.log("Checking for updates skipping rollout")

updater.checkForUpdates()
}
Expand All @@ -168,14 +186,6 @@ final class UpdateController: NSObject, UpdateControllerProtocol {
.assign(to: \.updateProgress, onWeaklyHeld: self)

try updater?.start()

#if DEBUG
updater?.automaticallyChecksForUpdates = false
updater?.automaticallyDownloadsUpdates = false
updater?.updateCheckInterval = 0
#else
checkForUpdateIfNeeded()
#endif
}

private func showUpdateNotificationIfNeeded() {
Expand Down Expand Up @@ -249,12 +259,18 @@ extension UpdateController: SPUUpdaterDelegate {

func updaterDidNotFindUpdate(_ updater: SPUUpdater, error: any Error) {
let nsError = error as NSError
guard let item = nsError.userInfo["SULatestAppcastItemFound"] as? SUAppcastItem else { return }
guard let item = nsError.userInfo[SPULatestAppcastItemFoundKey] as? SUAppcastItem else { return }

Logger.updates.log("Updater did not find update: \(String(describing: item.displayVersionString))(\(String(describing: item.versionString)))")
PixelKit.fire(DebugEvent(GeneralPixel.updaterDidNotFindUpdate, error: error))

cachedUpdateResult = UpdateCheckResult(item: item, isInstalled: true)
// Edge case: User upgrades to latest version within their rollout group
// But fetched release notes are outdated due to rollout group reset
let needsLatestReleaseNote = {
guard let reason = nsError.userInfo[SPUNoUpdateFoundReasonKey] as? Int else { return false }
return reason == Int(Sparkle.SPUNoUpdateFoundReason.onNewerThanLatestVersion.rawValue)
}()
cachedUpdateResult = UpdateCheckResult(item: item, isInstalled: true, needsLatestReleaseNote: needsLatestReleaseNote)
}

func updater(_ updater: SPUUpdater, didDownloadUpdate item: SUAppcastItem) {
Expand All @@ -274,6 +290,9 @@ extension UpdateController: SPUUpdaterDelegate {
if error == nil {
Logger.updates.log("Updater did finish update cycle")
updateProgress = .updateCycleDone
} else if let errorCode = (error as? NSError)?.code, errorCode == Int(Sparkle.SUError.noUpdateError.rawValue) {
Logger.updates.log("Updater did finish update cycle with no update found")
updateProgress = .updateCycleDone
} else {
Logger.updates.log("Updater did finish update cycle with error")
}
Expand Down

0 comments on commit 786ca2d

Please sign in to comment.