Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Enforce stricter hasPendingUpdate check #3541

Merged

Conversation

quanganhdo
Copy link
Member

@quanganhdo quanganhdo commented Nov 12, 2024

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

Description:

Fixes Install Now menu item does nothing by making sure we can resume the update process. As a bonus, this fixes an edge case where the user toggles back and forth between automatic & manual update checkboxes and that causes the upgrade process to go weird.

Steps to test this PR:

  1. Use this build: https://staticcdn.duckduckgo.com/macos-desktop-browser/a81f9679-e41d-404d-81a6-fd4a461d82f7/testbuilds/3447618/duckduckgo-1.114.0.305.dmg
  2. Test different scenarios (automatic/manual update, trigger the update from different entry points, etc)

Definition of Done:


Internal references:

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

@quanganhdo
Copy link
Member Author

quanganhdo commented Nov 12, 2024

This stack of pull requests is managed by Graphite. Learn more about stacking.

Join @quanganhdo and the rest of your teammates on Graphite Graphite

@quanganhdo quanganhdo marked this pull request as ready for review November 12, 2024 15:58
Copy link
Contributor

@tomasstrba tomasstrba left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM! ✅
Do you think it would make sense to merge to the release branch so we apply this fix one release cycle sooner?

@@ -71,7 +71,7 @@ final class UpdateController: NSObject, UpdateControllerProtocol {
didSet {
if let cachedUpdateResult {
latestUpdate = Update(appcastItem: cachedUpdateResult.item, isInstalled: cachedUpdateResult.isInstalled)
hasPendingUpdate = latestUpdate?.isInstalled == false && updateProgress.isIdle
hasPendingUpdate = latestUpdate?.isInstalled == false && updateProgress.isDone && userDriver?.isResumable == true
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just optional: It would help with readability if this is wrapped up to a computed property and documented.

@quanganhdo quanganhdo changed the base branch from main to release/1.114.0 November 13, 2024 14:34
@quanganhdo quanganhdo force-pushed the anh/11-12-enforce_stricter_haspendingupdate_check branch from a11d344 to b89d4e3 Compare November 13, 2024 14:36
@quanganhdo quanganhdo merged commit 6fe2acd into release/1.114.0 Nov 14, 2024
25 of 26 checks passed
@quanganhdo quanganhdo deleted the anh/11-12-enforce_stricter_haspendingupdate_check branch November 14, 2024 02:16
samsymons added a commit that referenced this pull request Nov 14, 2024
# By Graeme Arthur (2) and others
# Via GitHub
* main:
  Disable reordering when sort by name is enabled (#3552)
  Move FE Overlay Pixel to the Native layer (#3550)
  Bump version to 1.114.0 (307)
  bump bsk for text zoom feature definition (#3496)
  Add support for local overrides for feature flags (#3545)
  Speculative password import prompt crash fix (#3542)
  Send error with fetch device failure pixel (#3546)
  DuckPlayer Overlay Pixels (#3538)
  Bump version to 1.114.0 (306)
  Enforce stricter hasPendingUpdate check (#3541)
  Implement Fire Window UI tests (#3544)
  Add cohort assignment pixel for NTP Search Box Experiment (#3539)
  Prevent Freemium PIR Pixel Firing for All Users (#3543)

# Conflicts:
#	DuckDuckGo.xcodeproj/project.xcworkspace/xcshareddata/swiftpm/Package.resolved
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants