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

Fixes Brave Ads Purchase Intent segments should fall back to parent segments #6483

Merged
merged 1 commit into from
Aug 26, 2020
Merged

Fixes Brave Ads Purchase Intent segments should fall back to parent segments #6483

merged 1 commit into from
Aug 26, 2020

Conversation

tmancey
Copy link
Collaborator

@tmancey tmancey commented Aug 24, 2020 •
edited by btlechowski
Loading

Resolves brave/brave-browser#11410

Submitter Checklist:

Test Plan:

See brave/brave-browser#11410

  • Confirm parent-child purchase intent segments work
  • Confirm parent purchase intent segments work
  • Visit www.cnet.com and confirm intent signals are added to purchaseIntentSignalHistory
  • Search for amd in Google and confirm intent signals are added to purchaseIntentSignalHistory
  • Confirm ads are delivered as expected (including those related to purchase intent)

More detailed test plan in https://github.com/brave/internal/issues/717

Reviewer Checklist:

  • New files have MPL-2.0 license header.
  • Request a security/privacy review as needed.
  • Adequate test coverage exists to prevent regressions
  • Verify test plan is specified in PR before merging to source

After-merge Checklist:

  • The associated issue milestone is set to the smallest version that the
    changes has landed on.
  • All relevant documentation has been updated.

@tmancey tmancey self-assigned this Aug 24, 2020
@tmancey tmancey force-pushed the issues/11410 branch 2 times, most recently from ba31792 to 59b85af Compare August 25, 2020 00:19
moritzhaller
moritzhaller approved these changes Aug 25, 2020
View reviewed changes
Copy link
Contributor

@moritzhaller moritzhaller left a comment

Choose a reason for hiding this comment

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

LGTM!

yachtcaptain23
yachtcaptain23 reviewed Aug 25, 2020
View reviewed changes
vendor/bat-native-ads/src/bat/ads/internal/classification/classification_util.cc
parent_category = category;
}

if (std::find(parent_categories.begin(), parent_categories.end(),
Copy link
Contributor

@yachtcaptain23 yachtcaptain23 Aug 25, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

We can take this over DM, but how many categories are there?

Copy link
Collaborator Author

@tmancey tmancey Aug 25, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

there can be up to 3 page classification winning categories and 3 purchase intent winning categories. Therefore, a maximum of 6 categories

@tmancey tmancey force-pushed the issues/11410 branch 10 times, most recently from 5a164c8 to e89f03e Compare August 25, 2020 17:01
@tmancey
Copy link
Collaborator Author

tmancey commented Aug 26, 2020

CI failed due to known browser tests

@tmancey tmancey merged commit 5a2d086 into master Aug 26, 2020
@tmancey tmancey deleted the issues/11410 branch August 26, 2020 08:44
@tmancey tmancey added this to the 1.15.x - Nightly milestone Aug 26, 2020
@tmancey tmancey mentioned this pull request Aug 26, 2020
Merged
4 tasks
@btlechowski
Copy link

btlechowski commented Sep 3, 2020 •
edited
Loading

LGTM to uplift

Verification passed on

Brave 1.15.29 Chromium: 85.0.4183.83 (Official Build) nightly (64-bit)
Revision 94abc2237ae0c9a4cb5f035431c8adfb94324633-refs/branch-heads/4183@{#1658}
OS Ubuntu 18.04 LTS

Used test plan from https://github.com/brave/internal/issues/717 and the test plan from the description

Verified Parent segment derived from parent only purchase intent

[19871:19871:0903/051704.493437:VERBOSE1:ads_impl.cc(750)] Serving ad from categories:
[19871:19871:0903/051704.493545:VERBOSE1:ads_impl.cc(752)]   gamer-pc purists
[19871:19871:0903/051704.493612:VERBOSE1:ads_impl.cc(752)]   gamer-mainstream enthusiasts
[19871:19871:0903/051704.493678:VERBOSE1:ads_impl.cc(752)]   gamer
[19871:19871:0903/051704.495496:VERBOSE2:eligible_ads_pacing_filter.cc(31)] 1 eligible ads after pacing
[19871:19871:0903/051704.495631:VERBOSE2:eligible_ads_priority_filter.cc(81)] 1 eligible ads with a priority of 1
[19871:19871:0903/051704.495688:VERBOSE1:ads_impl.cc(863)] Found 1 eligible ads
[19871:19871:0903/051704.496506:VERBOSE1:ads_impl.cc(1118)] Ad notification shown:
  uuid: e3caec0f-4c11-42ad-8895-771f684c7aae
  parentUuid: d64e223f-de7e-4232-9e76-3c8c2cbe3373
  creativeInstanceId: 6a66e391-9a1d-4349-8a04-64751edf692b
  creativeSetId: 0aa3505d-9829-4c34-911a-7e0ecb691483
  category: gamer
  title: gamer
  body: gamer
  targetUrl: https://brave.com

Verified Parent-child segment derived from parent-child purchase intent

[22029:22029:0903/054737.902744:VERBOSE1:ads_impl.cc(750)] Serving ad from categories:
[22029:22029:0903/054737.902817:VERBOSE1:ads_impl.cc(752)]   gamer-pc purists
[22029:22029:0903/054737.902840:VERBOSE1:ads_impl.cc(752)]   gamer-mainstream enthusiasts
[22029:22029:0903/054737.902876:VERBOSE1:ads_impl.cc(752)]   gamer-alpha influencers
[22029:22029:0903/054737.918070:VERBOSE2:eligible_ads_pacing_filter.cc(31)] 1 eligible ads after pacing
[22029:22029:0903/054737.918256:VERBOSE2:eligible_ads_priority_filter.cc(81)] 1 eligible ads with a priority of 1
[22029:22029:0903/054737.918322:VERBOSE1:ads_impl.cc(863)] Found 1 eligible ads
[22029:22029:0903/054737.918999:VERBOSE1:ads_impl.cc(1118)] Ad notification shown:
  uuid: 251e7a4d-deab-47b1-8cee-4b1ddbc67f91
  parentUuid: 33419606-c092-46a1-8a41-538bc75ddea4
  creativeInstanceId: 6a66e391-9a1d-4349-8a04-64751edf692b
  creativeSetId: 0aa3505d-9829-4c34-911a-7e0ecb691483
  category: gamer-mainstream enthusiasts
  title: gamer-mainstream enthusiasts
  body: gamer-mainstream enthusiasts
  targetUrl: https://brave.com

Verified Parent segment derived from parent-child purchase intent

[20996:20996:0903/050344.887864:VERBOSE1:ads_impl.cc(750)] Serving ad from categories:
[20996:20996:0903/050344.887991:VERBOSE1:ads_impl.cc(752)]   gamer-pc purists
[20996:20996:0903/050344.888072:VERBOSE1:ads_impl.cc(752)]   gamer-mainstream enthusiasts
[20996:20996:0903/050344.888141:VERBOSE1:ads_impl.cc(752)]   gamer-alpha influencers
[20996:20996:0903/050344.888867:VERBOSE1:ads_impl.cc(768)] No eligible ads found in categories:
[20996:20996:0903/050344.888966:VERBOSE1:ads_impl.cc(770)]   gamer-pc purists
[20996:20996:0903/050344.889050:VERBOSE1:ads_impl.cc(770)]   gamer-mainstream enthusiasts
[20996:20996:0903/050344.889117:VERBOSE1:ads_impl.cc(770)]   gamer-alpha influencers
[20996:20996:0903/050344.889181:VERBOSE1:ads_impl.cc(786)] Serving ad from parent categories:
[20996:20996:0903/050344.889285:VERBOSE1:ads_impl.cc(788)]   gamer
[20996:20996:0903/050344.890518:VERBOSE2:eligible_ads_pacing_filter.cc(31)] 1 eligible ads after pacing
[20996:20996:0903/050344.890593:VERBOSE2:eligible_ads_priority_filter.cc(81)] 1 eligible ads with a priority of 1
[20996:20996:0903/050344.890660:VERBOSE1:ads_impl.cc(863)] Found 1 eligible ads
[20996:20996:0903/050344.891318:VERBOSE1:ads_impl.cc(1118)] Ad notification shown:
  uuid: 0c12a534-1711-4602-97e9-3ab525a256f1
  parentUuid: 4a08dcf4-3370-4e6e-9062-56b9aae8c5ec
  creativeInstanceId: 6a66e391-9a1d-4349-8a04-64751edf692b
  creativeSetId: 0aa3505d-9829-4c34-911a-7e0ecb691483
  category: gamer
  title: gamer
  body: gamer
  targetUrl: https://brave.com

Verified Show other ad after parent only purchase intent ad

[23275:23275:0903/063553.976841:VERBOSE1:ads_impl.cc(768)] No eligible ads found in categories:
[23275:23275:0903/063553.976925:VERBOSE1:ads_impl.cc(770)]   technology & computing-software
[23275:23275:0903/063553.976986:VERBOSE1:ads_impl.cc(770)]   personal finance-personal finance
[23275:23275:0903/063553.978444:VERBOSE1:ads_impl.cc(770)]   food & drink-vegetarian
[23275:23275:0903/063553.978535:VERBOSE1:ads_impl.cc(770)]   gamer-pc purists
[23275:23275:0903/063553.978615:VERBOSE1:ads_impl.cc(770)]   gamer-mainstream enthusiasts
[23275:23275:0903/063553.978717:VERBOSE1:ads_impl.cc(770)]   gamer
[23275:23275:0903/063553.978784:VERBOSE1:ads_impl.cc(786)] Serving ad from parent categories:
[23275:23275:0903/063553.978827:VERBOSE1:ads_impl.cc(788)]   technology & computing
[23275:23275:0903/063553.978884:VERBOSE1:ads_impl.cc(788)]   personal finance
[23275:23275:0903/063553.978926:VERBOSE1:ads_impl.cc(788)]   food & drink
[23275:23275:0903/063553.978982:VERBOSE1:ads_impl.cc(788)]   gamer
[23275:23275:0903/063553.985932:VERBOSE2:ads_impl.cc(970)] creativeInstanceId 6a66e391-9a1d-4349-8a04-64751edf692b has exceeded the frequency capping for perHour
[23275:23275:0903/063553.986049:VERBOSE2:eligible_ads_pacing_filter.cc(31)] 1 eligible ads after pacing
[23275:23275:0903/063553.986113:VERBOSE2:eligible_ads_priority_filter.cc(81)] 1 eligible ads with a priority of 1
[23275:23275:0903/063553.986195:VERBOSE1:ads_impl.cc(863)] Found 1 eligible ads
[23275:23275:0903/063553.986996:VERBOSE1:ads_impl.cc(1118)] Ad notification shown:
  uuid: 371a537d-ba8d-4dbe-99bf-83337c6b4a68
  parentUuid: 5e1b92ed-470f-49bb-af3e-7474a7b70702
  creativeInstanceId: d9dc0028-8f86-4c92-b25d-981bc1bdb903
  creativeSetId: dea8df38-27b6-4b53-a699-77629934db80
  category: technology & computing
  title: Technology & Computing
  body: Technology & Computing

Verified Show other ad after parent-child purchase intent ad

[23967:23967:0903/064728.831679:VERBOSE1:ads_impl.cc(750)] Serving ad from categories:
[23967:23967:0903/064728.831881:VERBOSE1:ads_impl.cc(752)]   technology & computing-software
[23967:23967:0903/064728.831940:VERBOSE1:ads_impl.cc(752)]   personal finance-personal finance
[23967:23967:0903/064728.831963:VERBOSE1:ads_impl.cc(752)]   food & drink-vegetarian
[23967:23967:0903/064728.831997:VERBOSE1:ads_impl.cc(752)]   gamer-pc purists
[23967:23967:0903/064728.832017:VERBOSE1:ads_impl.cc(752)]   gamer-mainstream enthusiasts
[23967:23967:0903/064728.832157:VERBOSE1:ads_impl.cc(752)]   gamer-alpha influencers
[23967:23967:0903/064728.833097:VERBOSE1:ads_impl.cc(768)] No eligible ads found in categories:
[23967:23967:0903/064728.833139:VERBOSE1:ads_impl.cc(770)]   technology & computing-software
[23967:23967:0903/064728.833260:VERBOSE1:ads_impl.cc(770)]   personal finance-personal finance
[23967:23967:0903/064728.833294:VERBOSE1:ads_impl.cc(770)]   food & drink-vegetarian
[23967:23967:0903/064728.833404:VERBOSE1:ads_impl.cc(770)]   gamer-pc purists
[23967:23967:0903/064728.833439:VERBOSE1:ads_impl.cc(770)]   gamer-mainstream enthusiasts
[23967:23967:0903/064728.833563:VERBOSE1:ads_impl.cc(770)]   gamer-alpha influencers
[23967:23967:0903/064728.833596:VERBOSE1:ads_impl.cc(786)] Serving ad from parent categories:
[23967:23967:0903/064728.833732:VERBOSE1:ads_impl.cc(788)]   technology & computing
[23967:23967:0903/064728.833868:VERBOSE1:ads_impl.cc(788)]   personal finance
[23967:23967:0903/064728.833986:VERBOSE1:ads_impl.cc(788)]   food & drink
[23967:23967:0903/064728.834022:VERBOSE1:ads_impl.cc(788)]   gamer
[23967:23967:0903/064728.847117:VERBOSE2:eligible_ads_pacing_filter.cc(31)] 1 eligible ads after pacing
[23967:23967:0903/064728.847183:VERBOSE2:eligible_ads_priority_filter.cc(81)] 1 eligible ads with a priority of 1
[23967:23967:0903/064728.847232:VERBOSE1:ads_impl.cc(863)] Found 1 eligible ads
[23967:23967:0903/064728.848129:VERBOSE1:ads_impl.cc(1118)] Ad notification shown:
  uuid: 6f3ecb4a-f938-4bd2-b03b-e5c740486521
  parentUuid: d88d7ffd-5e9d-4fd1-8a9e-4417062dffb2
  creativeInstanceId: d9dc0028-8f86-4c92-b25d-981bc1bdb903
  creativeSetId: dea8df38-27b6-4b53-a699-77629934db80
  category: technology & computing
  title: Technology & Computing
  body: Technology & Computing

Verified visiting www.cnet.com and confirmed intent signals are added to purchaseIntentSignalHistory

"purchaseIntentSignalHistory":{"gamer-alpha influencers":[{"timestamp_in_seconds":1599108582,"weight":1}]}

Verified searching for amd in Google and confirm intent signals are added to purchaseIntentSignalHistory

"purchaseIntentSignalHistory":{"gamer-alpha influencers":[{"timestamp_in_seconds":1599108912,"weight":1},{"timestamp_in_seconds":1599108914,"weight":1}],"gamer-mainstream enthusiasts":[{"timestamp_in_seconds":1599108912,"weight":1},{"timestamp_in_seconds":1599108914,"weight":1}],"gamer-pc purists":[{"timestamp_in_seconds":1599108912,"weight":1},{"timestamp_in_seconds":1599108914,"weight":1}]}

@btlechowski btlechowski mentioned this pull request Sep 8, 2020
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@yachtcaptain23 yachtcaptain23 yachtcaptain23 approved these changes

@moritzhaller moritzhaller moritzhaller approved these changes

Assignees

@tmancey tmancey

Labels
bug feature/ads
Projects
None yet
Milestone
1.15.x - Release #1
Development

Successfully merging this pull request may close these issues.

[Desktop] Purchase Intent parent only segments should not block page classification parent segment Brave ads
4 participants
@tmancey @btlechowski @moritzhaller @yachtcaptain23