Skip to content

feat(nimbus): Advanced targeting for users who have not accepted ToU yet, have not disabled ads, and are on Mac or Win #12596

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
Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

feat(nimbus): Advanced targeting for users who have not accepted ToU yet, have not disabled ads, and are on Mac or Win #12596

wants to merge 2 commits into from
+115 −0

Conversation

lmegviar
Copy link
Contributor

@lmegviar lmegviar commented May 6, 2025 •
edited
Loading

Because

  • We want to target users who have not accepted the Terms of Use yet, have not disabled ads, and are on Windows or Mac for an upcoming experiment

This commit

  • Adds advanced targeting described above (see experiment brief advanced targeting section for more context)

Fixes #12594

@lmegviar lmegviar force-pushed the tos-targeting branch 2 times, most recently from 71d6610 to f0db0e3 Compare May 6, 2025 23:40
@jaredlockhart
Copy link
Collaborator

@lmegviar Yep this looks right to me, you'll have to split that long line and then run make code_format or we can run it for you when you're ready 👍

@cpeterso
Copy link

cpeterso commented May 7, 2025

@lmegviar : Will we need to update this advanced targeting when the ToU prefs are renamed in https://bugzilla.mozilla.org/show_bug.cgi?id=1964544? Can we make this advanced targeting future proof, checking both the old datareporting.policy.dataSubmissionPolicyAcceptedVersion and new termsofuse.* prefs?

@freshstrangemusic freshstrangemusic changed the title feat(nimbus): Advanced targeting for users who have not accepted ToU … feat(nimbus): Advanced targeting for users who have not accepted ToU yet, and are on Mac or Win May 8, 2025
@lmegviar lmegviar changed the title feat(nimbus): Advanced targeting for users who have not accepted ToU yet, and are on Mac or Win feat(nimbus): Advanced targeting for users who have not accepted ToU yet, have not disabled ads, and are on Mac or Win May 9, 2025
@lmegviar
Copy link
Contributor Author

lmegviar commented May 9, 2025

@lmegviar : Will we need to update this advanced targeting when the ToU prefs are renamed in https://bugzilla.mozilla.org/show_bug.cgi?id=1964544? Can we make this advanced targeting future proof, checking both the old datareporting.policy.dataSubmissionPolicyAcceptedVersion and new termsofuse.* prefs?

Updated to also include the new termsofuse pref

@lmegviar lmegviar marked this pull request as ready for review May 9, 2025 15:19
@lmegviar
Copy link
Contributor Author

lmegviar commented May 9, 2025

@lmegviar Yep this looks right to me, you'll have to split that long line and then run make code_format or we can run it for you when you're ready 👍

Hi @jaredlockhart - is the long line the description of the targeting expression itself?

@lmegviar lmegviar force-pushed the tos-targeting branch 5 times, most recently from 6dd030f to 767a41e Compare May 12, 2025 15:15
@lmegviar
Copy link
Contributor Author

@jaredlockhart I gave splitting up the description and expression a go, but am having some issues with make code_format locally. Thanks again.

@lmegviar lmegviar marked this pull request as draft May 12, 2025 19:58
@lmegviar
Copy link
Contributor Author

Taking back to draft state as there are pending updates from product. I'll finalize the patch when we have the full requirements.

@lmegviar lmegviar marked this pull request as ready for review May 14, 2025 13:10
@lmegviar
Copy link
Contributor Author

Thanks for your patience, we confirmed the requirements this morning and I've updated the patch. This should be ready for review.

@lmegviar
Copy link
Contributor Author

@dmose to review for OMC team before landing

lmegviar
lmegviar commented May 21, 2025
View reviewed changes
experimenter/experimenter/targeting/constants.py Outdated
&&
'datareporting.policy.dataSubmissionPolicyAcceptedVersion'|preferenceValue >= 2
&&
'datareporting.policy.dataSubmissionPolicyNotifiedTime'|preferenceValue >= "1747843698000"
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Date of TOS being enabled by default with release of Fx139

@lmegviar lmegviar marked this pull request as draft May 22, 2025 13:03
@lmegviar
Copy link
Contributor Author

We have some new product requirements, I'll update today and take back out of draft when those are in place.

@lmegviar
Copy link
Contributor Author

@mikeconley could you add yourself as a reviewer on this one? After discussion with product, we'd appreciate having your feedback on this as well.

@lmegviar lmegviar force-pushed the tos-targeting branch 2 times, most recently from 15f4bb4 to a526efb Compare May 22, 2025 19:52
dmose
dmose requested changes May 23, 2025
View reviewed changes
Copy link
Member

@dmose dmose left a comment

Choose a reason for hiding this comment

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

The splitting you've done so far is already a big improvement in readability. I have a few tweaks and a proposal to make it even easier to gain confidence in the sub-expressions and overall expression by making it easier avoid & spot errors.

experimenter/experimenter/targeting/constants.py Outdated
# The following indicate whether the user accepted the terms of use in one of our initial phases of experimentation/rollout
ACCEPTED_TOU_IN_NIMBUS_EXPERIMENT = "'datareporting.policy.dataSubmissionPolicyAcceptedVersion'|preferenceValue == 3"
# Accepted in 100% on-train rollout in 139 release (after May 27, 6am PT timestamp), or in earlier partial rollout indicated by presence of on-train rollout population value
ACCEPTED_TOU_IN_ON_TRAIN_ROLLOUT = "(('browser.preonboarding.enrolledInOnTrainRollout'|preferenceValue && 'datareporting.policy.dataSubmissionPolicyAcceptedVersion'|preferenceValue == 2) && ('datareporting.policy.dataSubmissionPolicyNotifiedTime'|preferenceValue >= "1748350800000" || 'browser.preonboarding.onTrainRolloutPopulation'|preferenceValue))"
Copy link
Member

@dmose dmose May 23, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

The current thing fails to parse because of the "174..." inside the larger quoted string. Switching the inner " symbols to \" should fix this.

mikeconley
mikeconley reviewed May 23, 2025
View reviewed changes
experimenter/experimenter/targeting/constants.py Outdated
Comment on lines 50 to 51
TOPSITES_DISABLED = "(!'browser.newtabpage.activity-stream.feeds.topsites'|preferenceValue || !'browser.newtabpage.activity-stream.showSponsoredTopSites'|preferenceValue)"
STORIES_DISABLED = "(!'browser.newtabpage.activity-stream.feeds.section.topstories'|preferenceValue || !'browser.newtabpage.activity-stream.showSponsored'|preferenceValue)"
Copy link
Contributor

@mikeconley mikeconley May 23, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Kind of a nit - to make this clearer, I wonder if this should be SPONSORED_TOPSITES_DISABLED and SPONSORED_STORIES_DISABLED, as that's the only thing guaranteed by this being true. Top Sites is not guaranteed to be disabled if, for example, only browser.newtabpage.activity-stream.showSponsoredTopSites' is false.

Same deal with Stories being disabled if only browser.newtabpage.activity-stream.showSponsored is set to false.

mikeconley
mikeconley reviewed May 23, 2025
View reviewed changes
Copy link
Contributor

@mikeconley mikeconley left a comment

Choose a reason for hiding this comment

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

Beyond the one thing I noticed about the labelling of TOPSITES_DISABLED and STORIES_DISABLED, I think this makes sense to me. Thanks!

@lmegviar lmegviar force-pushed the tos-targeting branch 3 times, most recently from 3183964 to 36d7d25 Compare May 23, 2025 15:45
@lmegviar lmegviar marked this pull request as ready for review May 23, 2025 15:46
@lmegviar
Copy link
Contributor Author

Thanks @dmose and @mikeconley - updated.

dmose
dmose reviewed May 23, 2025
View reviewed changes
Copy link
Member

@dmose dmose left a comment

Choose a reason for hiding this comment

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

I notice that none of the targeting in the experiment mentions datareporting.policy.dataSubmissionPolicyBypassNotification. Don't we want to exclude people who have that set to true from at least enrollment if not also infobar display?

experimenter/experimenter/targeting/constants.py
{SEARCH_SUGGESTIONS_DISABLED}
)
)
"""
Copy link
Member

Choose a reason for hiding this comment

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

@mikeconley Do we want to also handle browser.newtabpage.enabled just to be on the safe side? It is in the codebase a fair bit: https://searchfox.org/mozilla-central/search?q=browser.newtabpage.enabled&path=&case=false&regexp=false. Of course, maybe that's all just dead code.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, I think that's a good idea. I just tested it, and it doesn't appear that that state gets reflected in newtabSettings. :/ So we should check that pref too.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Something like this?

Copy link
Member

Choose a reason for hiding this comment

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

Looks good to me. r=dmose

dmose
dmose requested changes May 23, 2025
View reviewed changes
Copy link
Member

@dmose dmose left a comment

Choose a reason for hiding this comment

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

OK, I think we're almost there. Once the various comments here and in Slack/the doc are addressed, I think we'll be ready to go.

@lmegviar lmegviar force-pushed the tos-targeting branch 2 times, most recently from 589c57b to d6f3058 Compare May 23, 2025 17:59
dmose
dmose approved these changes May 23, 2025
View reviewed changes
Copy link
Member

@dmose dmose left a comment

Choose a reason for hiding this comment

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

Looks good to me; thanks for all the hard work involved in both making this clear and making it accurate!

@lmegviar
Copy link
Contributor Author

@jaredlockhart I think we're ready for code formatting and merge whenever you're ready. FYI, I'll be heading out on PTO around 5pm EST today and won't be back until June 2. @dmose can take over during that time.

@lmegviar
Copy link
Contributor Author

@dmose just want to point out I had an error in the partial and full on train rollout variables - I've corrected those in the patch.

@dmose
Copy link
Member

dmose commented May 23, 2025

OK, just looked over those; thanks for the fixes!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mikeconley mikeconley mikeconley left review comments

@dmose dmose dmose approved these changes

@jaredlockhart jaredlockhart Awaiting requested review from jaredlockhart jaredlockhart is a code owner

@yashikakhurana yashikakhurana Awaiting requested review from yashikakhurana yashikakhurana is a code owner

@freshstrangemusic freshstrangemusic Awaiting requested review from freshstrangemusic freshstrangemusic is a code owner

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Add advanced targeting to support the existing user infobar baseline experiment
5 participants
@lmegviar @jaredlockhart @cpeterso @dmose @mikeconley