Skip to content
This repository has been archived by the owner on May 10, 2024. It is now read-only.

Always run brave ads if ShouldAlwaysRunBraveAdsService feature is true #8699

Closed
wants to merge 1 commit into from
Closed

Always run brave ads if ShouldAlwaysRunBraveAdsService feature is true #8699

wants to merge 1 commit into from

Conversation

aseren
Copy link
Collaborator

@aseren aseren commented Jan 24, 2024 •
edited
Loading

Summary of Changes

PR has been moved to brave-core repo brave/brave-core#22008

Added ability to always run brave ads if ShouldAlwaysRunBraveAdsService feature is true.
Corresponding brave-core change: brave/brave-core#21736

This pull request fixes #8696

Submitter Checklist:

  • Unit Tests are updated to cover new or changed functionality
  • User-facing strings use NSLocalizableString()
  • New or updated UI has been tested across:
    • Light & dark mode
    • Different size classes (iPhone, landscape, iPad)
    • Different dynamic type sizes

Test Plan:

ShouldAlwaysRunBraveAdsService feature DISABLED

Test case 1:

  • Fresh install
  • Launch browser
    • EXPECTED RESULT: Ads service is not started

Test case 2:

  • Fresh install
  • Launch browser
  • Join Brave Rewards
    • EXPECTED RESULT: Ads service is started
  • Quit browser
  • Launch browser
    • EXPECTED RESULT: Ads service is started
  • Disable Brave Rewards
    • EXPECTED RESULT: Ads service is stopped
  • Quit browser
  • Launch browser
    • EXPECTED RESULT: Ads service is not started

Test case 3:

  • Fresh install
  • Launch browser
  • Join Brave News
    • EXPECTED RESULT: Ads service is started
  • Quit browser
  • Launch browser
    • EXPECTED RESULT: Ads service is started
  • Disable Brave News
    • EXPECTED RESULT: Ads service is stopped
  • Quit browser
  • Launch browser
    • EXPECTED RESULT: Ads service is not started

—

ShouldAlwaysRunBraveAdsService feature ENABLED

Test case 1:

  • Fresh install
    • EXPECTED RESULT: Ads service is started
  • Launch browser
  • Quit browser
  • Launch browser
    • EXPECTED RESULT: Ads service is started

Test case 2:

  • Fresh install
  • Launch browser
    • EXPECTED RESULT: Ads service is started
  • Join Brave Rewards
    • EXPECTED RESULT: Ads service should not be restarted
  • Quit browser
  • Launch browser
    • EXPECTED RESULT: Ads service is started
  • Disable Brave Rewards
    • EXPECTED RESULT: Ads service should not stop
  • Quit browser
  • Launch browser
    • EXPECTED RESULT: Ads service is started

Test case 3:

  • Fresh install
  • Launch browser
    • EXPECTED RESULT: Ads service is started
  • Join Brave News
    • EXPECTED RESULT: Ads service should not be restarted
  • Quit browser
  • Launch browser
    • EXPECTED RESULT: Ads service is started
  • Disable Brave News
    • EXPECTED RESULT: Ads service should not stop
  • Quit browser
  • Launch browser
    • EXPECTED RESULT: Ads service is started

Screenshots:

Reviewer Checklist:

  • Issues include necessary QA labels:
    • QA/(Yes|No)
    • bug / enhancement
  • Necessary security reviews have taken place.
  • Adequate unit test coverage exists to prevent regressions.
  • Adequate test plan exists for QA to validate (if applicable).
  • Issue and pull request is assigned to a milestone (should happen at merge time).

@aseren aseren requested a review from tmancey January 24, 2024 20:09
@aseren aseren requested a review from a team as a code owner January 24, 2024 20:09
This was referenced Jan 25, 2024
Merged
Closed
tmancey
tmancey approved these changes Jan 26, 2024
View reviewed changes
Copy link
Collaborator

@tmancey tmancey left a comment

Choose a reason for hiding this comment

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

LGTM++ (As long as we have sanity tested all the test cases we wrote)

@aseren
Copy link
Collaborator Author

aseren commented Jan 26, 2024

Tests are failing because brave/brave-core#21736 is not merged yet

@tmancey tmancey changed the title Always run brave ads if ShouldAlwaysRunBraveAdsService feature is true Always run brave ads if ShouldAlwaysRunBraveAdsService feature is true [DO NOT MERGE] Jan 26, 2024
@tmancey tmancey changed the title Always run brave ads if ShouldAlwaysRunBraveAdsService feature is true [DO NOT MERGE] Always run brave ads if ShouldAlwaysRunBraveAdsService feature is true Jan 26, 2024
@iccub iccub added this to the 1.62 milestone Jan 30, 2024
@tmancey
Copy link
Collaborator

tmancey commented Jan 31, 2024

@iccub this issue is unrelated to the New Tab Page position changes and can ride the trains. This only needs to be merged in 1.64.x. cc @kylehickinson @aseren

@aseren aseren removed this from the 1.62 milestone Feb 1, 2024
@aseren aseren mentioned this pull request Feb 12, 2024
Merged
24 tasks
@aseren
Copy link
Collaborator Author

aseren commented Feb 12, 2024

PR has been moved to brave-core repo brave/brave-core#22008

@aseren aseren closed this Feb 12, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@Brandon-T Brandon-T Brandon-T approved these changes

@tmancey tmancey tmancey approved these changes

@kylehickinson kylehickinson Awaiting requested review from kylehickinson

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

Successfully merging this pull request may close these issues.

[ads] Always trigger new tab page ad events
4 participants
@aseren @tmancey @Brandon-T @iccub