Bugfix FXIOS-16737 Stop the daily usage ping from following the technical data opt-out - #35515
Bugfix FXIOS-16737 Stop the daily usage ping from following the technical data opt-out#35515razvanlitianu wants to merge 4 commits into
Conversation
…ical data opt-out
💪 Quality guardian1 tests files modified. You're a champion of test coverage! 🚀 🧹 Tidy commitJust 5 file(s) touched. Thanks for keeping it clean and review-friendly! 💬 Description craftsmanGreat PR description! Reviewers salute you 🫡 ✅ Code coverage
Client.app: Coverage: 46.08
Generated by 🚫 Danger Swift against 114cd43 |
There was a problem hiding this comment.
🟡 Changes recommended
The Settings daily-usage-ping row construction still triggers Experiments.setTelemetrySetting indirectly via SendDataSetting, which can incorrectly disable experiments telemetry when the daily ping is toggled off.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
Fixes FXIOS-16737 where opting out of “Send technical and interaction data” during Terms of Use onboarding unintentionally disabled the Daily Usage Ping due to sendDailyUsagePing falling back to sendUsageData. The change makes the Daily Usage Ping read only its own preference and uses a single shared default value.
Changes:
- Remove the
prefSendDailyUsagePingfallback-to-prefSendUsageDatalogic inTelemetryWrapper.initGleanand centralize the default behavior. - Add
AppConstants.defaultSendDailyUsagePingand use it in both telemetry resolution and the Settings toggle row. - Add unit tests covering the daily usage ping preference resolution, and reorder test setup to avoid Nimbus/Experiments initialization crashes.
File summaries
| File | Description |
|---|---|
| firefox-ios/firefox-ios-tests/Tests/ClientTests/TelemetryWrapperTests.swift | Reorders setup and adds unit tests for daily usage ping pref behavior. |
| firefox-ios/Client/Telemetry/TelemetryWrapper.swift | Removes fallback logic and introduces shouldSendDailyUsagePing(prefs:) resolver. |
| firefox-ios/Client/Frontend/Settings/Main/AppSettingsTableViewController.swift | Uses centralized default for the daily usage ping setting row. |
| BrowserKit/Sources/Common/Constants/AppConstants.swift | Adds defaultSendDailyUsagePing constant for shared default behavior. |
Review details
- Files reviewed: 4/4 changed files
- Comments generated: 2
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| let sendDailyUsagePingSettings = SendDataSetting( | ||
| prefs: profile.prefs, | ||
| prefKey: AppConstants.prefSendDailyUsagePing, | ||
| defaultValue: true, | ||
| defaultValue: AppConstants.defaultSendDailyUsagePing, | ||
| titleText: .SendDailyUsagePingSettingTitle, |
There was a problem hiding this comment.
Fixed in 114cd43: every non-studies row was writing its own pref to the global flag and the last one constructed won, so crash reports reset the Nimbus identifiers and technical data got silently overwritten. The call now happens once, at the technical data site.
|
|
||
| let result = TelemetryWrapper().shouldSendDailyUsagePing(prefs: prefs) | ||
|
|
||
| XCTAssertTrue(result, "Opting out of technical data must not disable the daily usage ping") |
There was a problem hiding this comment.
Keeping XCTAssertTrue here, since the point of the test is that the ping stays on when technical data is off, and comparing against the constant would keep it green even if the default flipped.
…t telemetry flag SendDataSetting.init pushed its own pref into Experiments.setTelemetrySetting for every non-studies instance, so the last row constructed won. Turning off crash reports reset the Nimbus telemetry identifiers, and turning off technical data was overwritten by the crash reports row defaulting to true. The call now happens once, at the technical data site.
| } | ||
| sendTechnicalDataSetting = sendTechnicalDataSettings | ||
|
|
||
| // Technical data is the only setting that may drive experiment participation |
There was a problem hiding this comment.
This isn't really right. updateExperimentParticipation() requires both studies and telemetry, and rollouts drives participation separately. Maybe this can say somethnig like "the only setting that drives the Nimbus telemetry setting."
📜 Tickets
Jira ticket
Github issue
💡 Description
Opting out of "Send technical and interaction data" on the Terms of Use card also turned off the Daily Usage Ping, a setting onboarding never shows.
initGleanfell back tosendUsageDatawhensendDailyUsagePingwas unset, which is exactly the state on a fresh install after the ToU handler writessendUsageData = false. Fallback removed: the ping reads only its own pref and defaults to on, fresh installs and upgrades alike per QA. The default now lives once, inAppConstants.defaultSendDailyUsagePing.Also reorders two lines in
TelemetryWrapperTests.setUpso dependencies bootstrap beforeExperiments; otherwise the class crashes when it runs first.Review note: users upgrading from v135 or earlier who opted out of technical data will now get the daily ping on.
🎥 Demos
Settings > Support, after opting out of technical data on the ToU card.
The Manage sheet the user actually sees during onboarding
Two toggles, and no daily usage ping control anywhere in the flow.
📝 Checklist