-
Notifications
You must be signed in to change notification settings - Fork 3k
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
Add FXIOS-10902 Firefox iOS send data deletion request when dau ping is toggled off #24263
Add FXIOS-10902 Firefox iOS send data deletion request when dau ping is toggled off #24263
Conversation
… is toggled off
… is toggled off
Client.app: Coverage: 32.34
Generated by 🚫 Danger Swift against a1ca3fa |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One suggestion to clarify the reason
for the ping, no need to block on re-review from me.
The ping was submitted between Glean init and Glean shutdown. | ||
After init but before shutdown the upload of usage reporting data changed | ||
from enabled to disabled. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The ping was submitted between Glean init and Glean shutdown. | |
After init but before shutdown the upload of usage reporting data changed | |
from enabled to disabled. | |
The ping was submitted due to the usage-reporting data preference changing from | |
enabled to disabled. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I said that before on, I think, the desktop implementation: due to this being a include_info_sections=false
ping we won't see any reasons anyway. They are stripped from the final payload.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note that desktop did land with the reason defined. So for documentation reasons we can probably keep it here too (it's not set in the above submit
call anyway)
This pull request has conflicts when rebasing. Could you fix it @razvanlitianu? 🙏 |
The ping was submitted between Glean init and Glean shutdown. | ||
After init but before shutdown the upload of usage reporting data changed | ||
from enabled to disabled. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I said that before on, I think, the desktop implementation: due to this being a include_info_sections=false
ping we won't see any reasons anyway. They are stripped from the final payload.
@@ -236,6 +237,7 @@ class AppSettingsTableViewController: SettingsTableViewController, | |||
if value { | |||
self?.gleanLifecycleObserver.startObserving() | |||
} else { | |||
GleanMetrics.Pings.shared.usageDeletionRequest.submit() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a follows_collection_enabled=false
ping, thus it needs to be specifically enabled, otherwise it won't ever be submitted.
Because it also should carry the usage_profile_id it needs to be enabled before that metric is set so that we actually store that ID for it.
@@ -104,6 +104,9 @@ class TelemetryWrapper: TelemetryWrapperProtocol, FeatureFlaggable { | |||
let uuid = UUID(uuidString: uuidString) { | |||
GleanMetrics.LegacyIds.clientId.set(uuid) | |||
} | |||
|
|||
GleanMetrics.Pings.shared.usageDeletionRequest.setEnabled(enabled: true) | |||
GleanMetrics.Pings.shared.onboardingOptOut.setEnabled(enabled: true) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@badboy Do you think this is good spot, before init profile id? I assume we have to do the same for onboarding opt out since it has the same property.
I also wonder, do we ever need to disable them?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No disabling needed. It's only explicitly submitted anyway and we can't hit that codepath unless it's the actual disabling by a user through the UI.
📜 Tickets
Jira ticket
💡 Description
Add data deletion request following Android and Desktop implementation.
📝 Checklist
You have to check all boxes before merging
@Mergifyio backport release/v120
)