-
Notifications
You must be signed in to change notification settings - Fork 1.2k
Add os_version to active subscription pixel #7406
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
base: develop
Are you sure you want to change the base?
Conversation
lmac012
left a comment
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.
LGTM, just a couple of comments.
| } | ||
| ] | ||
| }, | ||
| "m_privacy-pro_app_subscription_active": { |
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 always fired as a "daily" pixel, and as such, it has the _d suffix. This is not accounted for in this definition and will most likely cause validation errors. You could add "daily_count_short" to "suffixes", but this is never fired as a "count" type of pixel, so it's probably better to just include the type suffix in the name:
| "m_privacy-pro_app_subscription_active": { | |
| "m_privacy-pro_app_subscription_active_d": { |
| { | ||
| "key": "petal", | ||
| "description": "Signals the pixel processing pipeline", | ||
| "type": "boolean" | ||
| } |
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'm not sure about the best practices here, but shouldn't we add "enum": ["true"] to indicate that this is the only possible value for the petal param? cc @nshuba
Regardless, I think it would make sense to add this parameter to params_dictionary.json, since it's not specific to this pixel and will likely get reused on other pixels as well.

Task/Issue URL: https://app.asana.com/1/137249556945/project/488551667048375/task/1212426814527389?focus=true
Description
Added os_version and petal to the pixel.
Removed atb from the pixel.
Added entry for pixel registry.
https://app.asana.com/1/137249556945/project/69071770703008/task/1212452022245359?focus=true
Steps to test this PR
QA-optional
Note
Adds
os_versionandpetaltom_privacy-pro_app_subscription_activeand stops includingATBfor it, with registry entry and sender wiring.m_privacy-pro_app_subscription_active):os_version(fromAppBuildConfig.sdkInt) andpetal(hardcoded "true").ATB; keepAPP_VERSIONonly inincludedParameters.PixelDefinitions/pixels/subscription_pixels.json5with new params.SubscriptionPixelSender.reportSubscriptionActiveto sendos_versionandpetal.SubscriptionPixelParameter.OS_VERSIONandPETALconstants.Written by Cursor Bugbot for commit 0257c72. This will update automatically on new commits. Configure here.