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.

Sign up for GitHub

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

Jump to bottom

Send pixel on sync secure storage read failure #3530

Merged
merged 4 commits into from
Nov 5, 2024
Merged

Send pixel on sync secure storage read failure #3530

merged 4 commits into from
Nov 5, 2024

Conversation

graeme
Copy link
Contributor

@graeme graeme commented Nov 4, 2024 •
edited
Loading

Task/Issue URL: https://app.asana.com/0/1201493110486074/1208686320819590/f

Description:

On investigating a hard-to-reproduce issue with sync, I noticed there's a gap in error reporting when the secure storage (keychain) is not available. This adds a pixel for that case.

Steps to test this PR:
Just a pixel in an error case. Hard to test without altering code. But if you do want to do that:

  1. Enable sync
  2. Change BSK.DDGSync.SecureStorage.account() to throw every time
  3. Go to the Settings -> Sync screen
  4. You should see the sync_secure_storage_read_error Pixel in the debug console

Definition of Done (Internal Only):

Copy Testing:

  • Use of correct apostrophes in new copy, ie ’ rather than '

Orientation Testing:

  • Portrait
  • Landscape

Device Testing:

  • iPhone SE (1st Gen)
  • iPhone 8
  • iPhone X
  • iPhone 14 Pro
  • iPad

OS Testing:

  • iOS 15
  • iOS 16
  • iOS 17

Theme Testing:

  • Light theme
  • Dark theme
Internal references:

Software Engineering Expectations
Technical Design Template

@graeme graeme self-assigned this Nov 4, 2024
@graeme graeme mentioned this pull request Nov 4, 2024
Merged
6 tasks
@graeme graeme force-pushed the graeme/send-pixel-on-sync-secure-storage-failure branch from 98bf8bd to 9ed929b Compare November 5, 2024 10:25
@graeme graeme changed the base branch from main to release/7.144.0 November 5, 2024 10:26
@graeme graeme requested a review from amddg44 November 5, 2024 10:26
@graeme graeme marked this pull request as ready for review November 5, 2024 10:26
@graeme graeme requested a review from dus7 November 5, 2024 12:38
dus7
dus7 approved these changes Nov 5, 2024
View reviewed changes
Copy link
Contributor

@dus7 dus7 left a comment

Choose a reason for hiding this comment

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

LGTM. Highlighted one linter warning introduced.

Core/SyncErrorHandler.swift Outdated
@@ -100,6 +100,8 @@ public class SyncErrorHandler: EventMapping<SyncError> {
Pixel.fire(pixel: .syncFailedToLoadAccount, error: error)
case .failedToSetupEngine:
Pixel.fire(pixel: .syncFailedToSetupEngine, error: error)
case .failedToReadSecureStore(let status):
Copy link
Contributor

Choose a reason for hiding this comment

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

status is not used.

graeme added a commit to duckduckgo/BrowserServicesKit that referenced this pull request Nov 5, 2024
@graeme
Send pixel on sync secure storage read failure (#1058)
56dbee7 
<!--
Note: This checklist is a reminder of our shared engineering
expectations.
-->

Please review the release process for BrowserServicesKit
[here](https://app.asana.com/0/1200194497630846/1200837094583426).

**Required**:

Task/Issue URL:
https://app.asana.com/0/1201493110486074/1208686320819590/f
iOS PR: duckduckgo/iOS#3530
macOS PR: duckduckgo/macos-browser#3497
What kind of version bump will this require?: Minor

**Description:**

On investigating a hard-to-reproduce issue with sync, I noticed there's
a gap in error reporting when the secure storage (keychain) is not
available. This adds a pixel for that case.

**Steps to test this PR:**
Just a pixel in an error case. Hard to test without altering code. But
if you do want to do that:

Enable sync
Change BSK.DDGSync.SecureStorage.account() to throw every time
Go to the Settings -> Sync screen
You should see the `sync_secure_storage_read_error` Pixel in the debug
console

**OS Testing**:

* [ ] iOS 14
* [ ] iOS 15
* [ ] iOS 16
* [ ] macOS 10.15
* [ ] macOS 11
* [ ] macOS 12

---
###### Internal references:
[Software Engineering
Expectations](https://app.asana.com/0/59792373528535/199064865822552)
[Technical Design
Template](https://app.asana.com/0/59792373528535/184709971311943)
@graeme graeme merged commit 66c516a into release/7.144.0 Nov 5, 2024
13 checks passed
@graeme graeme deleted the graeme/send-pixel-on-sync-secure-storage-failure branch November 5, 2024 14:43
@graeme graeme restored the graeme/send-pixel-on-sync-secure-storage-failure branch November 5, 2024 16:51
@graeme graeme mentioned this pull request Nov 5, 2024
Merged
14 tasks
graeme added a commit that referenced this pull request Nov 5, 2024
@graeme @afterxleep
Send pixel on sync secure storage failure (#3542)
d9a02a3 
Task/Issue URL:
https://app.asana.com/0/414235014887631/1208700858621924/f

**Description**:

This was reviewed here: #3530 and
merged already to the release branch, but there's been conflicts since.
So it needs an extra PR to resolve them.

**Steps to test this PR**:
1. Just make sure this compiles and is pointing to latest BSK release.

**Definition of Done (Internal Only)**:

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

**Copy Testing**:

* [ ] Use of correct apostrophes in new copy, ie `’` rather than `'`

**Orientation Testing**:

* [ ] Portrait
* [ ] Landscape

**Device Testing**:

* [ ] iPhone SE (1st Gen)
* [ ] iPhone 8
* [ ] iPhone X
* [ ] iPhone 14 Pro
* [ ] iPad

**OS Testing**:

* [ ] iOS 15
* [ ] iOS 16
* [ ] iOS 17

**Theme Testing**:

* [ ] Light theme
* [ ] Dark theme

---
###### Internal references:
[Software Engineering
Expectations](https://app.asana.com/0/59792373528535/199064865822552)
[Technical Design
Template](https://app.asana.com/0/59792373528535/184709971311943)

---------

Co-authored-by: Daniel Bernal <[email protected]>
samsymons added a commit that referenced this pull request Nov 5, 2024
@samsymons
Merge branch 'main' into dominik/xcode-16
22a19c9 
# By Daniel Bernal (7) and others
# Via Daniel Bernal (3) and others
* main: (29 commits)
  Send pixel on sync secure storage failure (#3542)
  Onboarding Add to Dock Refactor for Intro scenario (#3538)
  Update C-S-S to 6.29.0 (#3541)
  Change save password Never for Site button to Not Now (#3471)
  Release 7.144.0-1 (#3540)
  UserDefaults misbehavior monitoring (#3510)
  Send pixel on sync secure storage read failure (#3530)
  Remove NewTabPage retain cycles (#3532)
  Update release notes (#3529)
  Release 7.144.0-0 (#3528)
  Add Privacy Config feature to control ad attribution reporting (#3506)
  Validate VPN errors before re-throwing them (#3513)
  Allowing users to delete suggestions on macOS (#3465)
  Update build number
  Update build number
  Bump rexml from 3.3.8 to 3.3.9 (#3495)
  Release 7.142.1-1 (#3525)
  Add a debouncer to NavBars animator (#3519)
  Release 7.143.0-1 (#3516)
  Update to subscription cookie (#3512)
  ...

# Conflicts:
#	DuckDuckGo.xcodeproj/project.pbxproj
#	DuckDuckGo.xcodeproj/project.xcworkspace/xcshareddata/swiftpm/Package.resolved
samsymons added a commit that referenced this pull request Nov 6, 2024
@samsymons
Merge branch 'main' into sam/migrate-daily-and-count-type-to-legacy-s…
074f339 
…tatus

# By Graeme Arthur (4) and others
# Via GitHub (1) and Graeme Arthur (1)
* main:
  Adding app backgrounded result to rule compilation (#3533)
  Send pixel on sync secure storage failure (#3542)
  Onboarding Add to Dock Refactor for Intro scenario (#3538)
  Update C-S-S to 6.29.0 (#3541)
  Change save password Never for Site button to Not Now (#3471)
  Release 7.144.0-1 (#3540)
  UserDefaults misbehavior monitoring (#3510)
  Send pixel on sync secure storage read failure (#3530)
  Remove NewTabPage retain cycles (#3532)
  Update release notes (#3529)

# Conflicts:
#	DuckDuckGo.xcodeproj/project.pbxproj
#	DuckDuckGo.xcodeproj/project.xcworkspace/xcshareddata/swiftpm/Package.resolved
samsymons added a commit that referenced this pull request Nov 6, 2024
@samsymons
Merge branch 'main' into sam/fix-memory-pressure-monitor
facee90 
* main:
  Update Ruby to 3.3.4 (#3547)
  Onboarding Add To Dock Pixels (#3543)
  Switch to free runners for tests that run on Maestro (#3546)
  Fix email protection test (#3539)
  Update BSK for PixelKit suffix change (#3534)
  Adding app backgrounded result to rule compilation (#3533)
  Send pixel on sync secure storage failure (#3542)
  Onboarding Add to Dock Refactor for Intro scenario (#3538)
  Update C-S-S to 6.29.0 (#3541)
  Change save password Never for Site button to Not Now (#3471)
  Release 7.144.0-1 (#3540)
  UserDefaults misbehavior monitoring (#3510)
  Send pixel on sync secure storage read failure (#3530)
  Remove NewTabPage retain cycles (#3532)
  Update release notes (#3529)
samsymons added a commit that referenced this pull request Nov 6, 2024
@samsymons
Merge branch 'main' into sam/vpn-clean-up
9b9be64 
# By Graeme Arthur (4) and others
# Via GitHub (1) and Graeme Arthur (1)
* main:
  Fix VPN memory pressure monitor (#3535)
  Update Ruby to 3.3.4 (#3547)
  Onboarding Add To Dock Pixels (#3543)
  Switch to free runners for tests that run on Maestro (#3546)
  Fix email protection test (#3539)
  Update BSK for PixelKit suffix change (#3534)
  Adding app backgrounded result to rule compilation (#3533)
  Send pixel on sync secure storage failure (#3542)
  Onboarding Add to Dock Refactor for Intro scenario (#3538)
  Update C-S-S to 6.29.0 (#3541)
  Change save password Never for Site button to Not Now (#3471)
  Release 7.144.0-1 (#3540)
  UserDefaults misbehavior monitoring (#3510)
  Send pixel on sync secure storage read failure (#3530)
  Remove NewTabPage retain cycles (#3532)
  Update release notes (#3529)
  Release 7.144.0-0 (#3528)
  Add Privacy Config feature to control ad attribution reporting (#3506)

# Conflicts:
#	DuckDuckGo.xcodeproj/project.pbxproj
#	DuckDuckGo.xcodeproj/project.xcworkspace/xcshareddata/swiftpm/Package.resolved
samsymons added a commit that referenced this pull request Nov 6, 2024
@samsymons
Merge branch 'main' into sam/remove-test-rollout-pixel
6a62d10 
# By Graeme Arthur (4) and others
# Via GitHub (1) and Graeme Arthur (1)
* main:
  VPN clean-up (#3502)
  add daily pixel for deleting suggestions (#3531)
  Fix VPN memory pressure monitor (#3535)
  Update Ruby to 3.3.4 (#3547)
  Onboarding Add To Dock Pixels (#3543)
  Switch to free runners for tests that run on Maestro (#3546)
  Fix email protection test (#3539)
  Update BSK for PixelKit suffix change (#3534)
  Adding app backgrounded result to rule compilation (#3533)
  Send pixel on sync secure storage failure (#3542)
  Onboarding Add to Dock Refactor for Intro scenario (#3538)
  Update C-S-S to 6.29.0 (#3541)
  Change save password Never for Site button to Not Now (#3471)
  Release 7.144.0-1 (#3540)
  UserDefaults misbehavior monitoring (#3510)
  Send pixel on sync secure storage read failure (#3530)
  Remove NewTabPage retain cycles (#3532)
  Update release notes (#3529)

# Conflicts:
#	DuckDuckGo.xcodeproj/project.pbxproj
#	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
Reviewers

@dus7 dus7 dus7 approved these changes

@amddg44 amddg44 Awaiting requested review from amddg44

Assignees

@graeme graeme

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@graeme @dus7