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

Automatically limit unwanted Brave Ads delivery #5031

Merged
merged 1 commit into from
Aug 11, 2020
Merged

Conversation

tmancey
Copy link
Collaborator

@tmancey tmancey commented Mar 24, 2020

Resolves brave/brave-browser#6253

Submitter Checklist:

Test Plan:

See brave/brave-browser#6253

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.

@@ -11,6 +11,7 @@ @interface BATAdNotification ()
@property (nonatomic, copy) NSString *parentUuid;
@property (nonatomic, copy) NSString *creativeInstanceID;
@property (nonatomic, copy) NSString *creativeSetID;
@property (nonatomic, copy) NSString *campaignID;
Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't see anything in particular (aside from the change to the ads_service_impl at the top), but is this new property supposed to be used in the client in a specific way?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Also missing the read-only property definition for this in BATAdsNotification.h

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This new property is not used in the client as of yet

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Fixed

Copy link
Collaborator

@kylehickinson kylehickinson left a comment

Choose a reason for hiding this comment

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

iOS change LGTM 🙂

@tmancey tmancey force-pushed the issues/6253 branch 3 times, most recently from 11cb6ad to 67d7025 Compare April 1, 2020 22:59
@tmancey tmancey added this to the 1.9.x - Nightly milestone Apr 3, 2020
@tmancey tmancey force-pushed the issues/6253 branch 3 times, most recently from 24a229b to 4b2ee90 Compare April 7, 2020 12:49
@tmancey tmancey requested review from pes10k and removed request for masparrow and pes10k April 8, 2020 21:51
@tmancey tmancey force-pushed the issues/6253 branch 7 times, most recently from 20d5af8 to e7b9e43 Compare April 12, 2020 00:40
@tmancey tmancey force-pushed the issues/6253 branch 4 times, most recently from bad3dda to 801eec3 Compare May 27, 2020 08:49
@tmancey tmancey added CI/skip-android Do not run CI builds for Android CI/skip-ios Do not run CI builds for iOS CI/skip-linux labels May 29, 2020
@tmancey
Copy link
Collaborator Author

tmancey commented May 29, 2020

CI failed on macOS, restarting macOS

@tmancey tmancey force-pushed the issues/6253 branch 2 times, most recently from ba42680 to 8b08be2 Compare June 4, 2020 09:21
@tmancey tmancey removed CI/skip-android Do not run CI builds for Android CI/skip-ios Do not run CI builds for iOS CI/skip-linux labels Jun 17, 2020
@NejcZdovc NejcZdovc removed their request for review June 17, 2020 07:34
@tmancey tmancey force-pushed the issues/6253 branch 3 times, most recently from 3f7c055 to 16b50c3 Compare August 10, 2020 12:59
@tmancey tmancey added CI/skip-android Do not run CI builds for Android CI/skip-ios Do not run CI builds for iOS labels Aug 10, 2020
@tmancey
Copy link
Collaborator Author

tmancey commented Aug 11, 2020

CI failed on Windows for known test, restarting macOS. macOS failing due to known issue at brave/brave-browser#6515

@tmancey tmancey merged commit 53d8740 into master Aug 11, 2020
@tmancey tmancey deleted the issues/6253 branch August 11, 2020 10:31
@tmancey tmancey added this to the 1.14.x - Nightly milestone Aug 11, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CI/skip-android Do not run CI builds for Android CI/skip-ios Do not run CI builds for iOS feature/ads
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Automatically limit unwanted Brave Ads delivery
2 participants