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.

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

Ads first launch notification timer no longer fires after notification dismissal #2371

Merged
merged 1 commit into from
May 7, 2019

Conversation

ryanml
Copy link
Contributor

@ryanml ryanml commented May 6, 2019

Fixes: brave/brave-browser#4180

A preference was added to mark whether the ads notification has gone away. The pref, kBraveAdsHasDeletedFirstLaunchNotification will be set to true if the user dismisses the notification, engages with the notification, enables ads, or if the notification goes away on its own after its 7 day timeout. This is used as the crutch to determine whether we need to initialize the timeout timer or not.

Submitter Checklist:

Test Plan:

As there are no behavioral indicators that the deletion timer is still firing after the notification goes away, this will have to be confirmed via logging.

  1. Create a profile from an older version of Brave where Ads was disabled (0.62.x), enable Rewards, copy profile to development directory.
  2. Start Brave from this branch.
  3. Dismiss the "Ads Have Arrived!" notification.
  4. Restart the browser
  5. Confirm via logging that the timer to trigger OnFirstLaunchNotificationTimedOut does not start up.

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.

@ryanml ryanml added this to the 0.66.x - Nightly milestone May 6, 2019
@ryanml ryanml requested review from tmancey and NejcZdovc May 6, 2019 04:01
@ryanml ryanml self-assigned this May 6, 2019
@ryanml ryanml changed the title Ads first launch notification timer no longer fires on startup after notification goes away Ads first launch notification timer no longer fires on startup after notification dismissal May 6, 2019
@ryanml ryanml changed the title Ads first launch notification timer no longer fires on startup after notification dismissal Ads first launch notification timer no longer fires after notification dismissal May 6, 2019
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++

@ryanml
Copy link
Contributor Author

ryanml commented May 7, 2019

Note: failure related to AWS connection

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

AdsServiceImpl should not create a new timer after notification dismissal
3 participants