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

Refactor Brave Ads in preparation for merging Publisher Ads #4685

Merged
merged 1 commit into from
Feb 20, 2020
Merged

Refactor Brave Ads in preparation for merging Publisher Ads #4685

merged 1 commit into from
Feb 20, 2020

Conversation

tmancey
Copy link
Collaborator

@tmancey tmancey commented Feb 19, 2020 •
edited
Loading

Resolves brave/brave-browser#8294
Requires #4698
Requires #4699
Requires #4704

Submitter Checklist:

Test Plan:

  • Confirm ad notifications are shown and the user is rewarded for viewing an ad

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 Feb 19, 2020
@tmancey tmancey force-pushed the issues/8294 branch 2 times, most recently from 4eab6b5 to a93e298 Compare February 19, 2020 04:07
@tmancey tmancey marked this pull request as ready for review February 19, 2020 04:10
@tmancey
Copy link
Collaborator Author

tmancey commented Feb 19, 2020 •
edited
Loading

Cleaned up data structures, Renamed instances of Ad to AdNotification ready for PublisherAds. Renamed uuid to creative_instance_id then renamed id to uuid to match catalog/server-side. Removed const where not needed. Added const where needed. Refactored enum to enum class.

@tmancey tmancey added this to the 1.6.x - Nightly milestone Feb 19, 2020
@NejcZdovc
Copy link
Contributor

@tmancey CI is failing on build. https://ci.brave.com/job/brave-browser-build-pr/job/issues%252F8294/2/execution/node/481/log/

@tmancey
Copy link
Collaborator Author

tmancey commented Feb 19, 2020

@tmancey CI is failing on build. https://ci.brave.com/job/brave-browser-build-pr/job/issues%252F8294/2/execution/node/481/log/

Fixed, thanks

@tmancey tmancey force-pushed the issues/8294 branch 4 times, most recently from c91e873 to b6a51fa Compare February 19, 2020 08:13
moritzhaller
moritzhaller reviewed Feb 19, 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.

skimmed wrt naming esp. around conversions, client and bundle state. lgtm

@tmancey tmancey force-pushed the issues/8294 branch 2 times, most recently from 0ac12ae to bc730f3 Compare February 19, 2020 10:42
@tmancey
Copy link
Collaborator Author

tmancey commented Feb 19, 2020

Unrelated CI TestSuite Security failure

masparrow
masparrow approved these changes Feb 19, 2020
View reviewed changes
Copy link
Contributor

@masparrow masparrow left a comment

Choose a reason for hiding this comment

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

LGTM

vendor/bat-native-ads/include/bat/ads/ad_content.h Show resolved Hide resolved
vendor/bat-native-ads/include/bat/ads/ads_history.h Show resolved Hide resolved
vendor/bat-native-ads/src/bat/ads/internal/bundle.cc Show resolved Hide resolved
vendor/bat-native-ads/src/bat/ads/internal/catalog_state.cc Show resolved Hide resolved
vendor/bat-native-ads/src/bat/ads/internal/error_helper.cc Show resolved Hide resolved
vendor/bat-native-ledger/src/bat/ledger/internal/ledger_impl.cc Show resolved Hide resolved
components/brave_ads/browser/bundle_state_database.cc Show resolved Hide resolved
@tmancey tmancey force-pushed the issues/8294 branch 6 times, most recently from 4073399 to d1bafa7 Compare February 19, 2020 16:59
kylehickinson
kylehickinson approved these changes Feb 19, 2020
View reviewed changes
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 LGTM 👍

@tmancey
Copy link
Collaborator Author

tmancey commented Feb 19, 2020

@NejcZdovc Other changes were to rename variables and functions to be inline with Publisher Ads

@NejcZdovc
Copy link
Contributor

CI failed on android, everything else passed. Restarting android

@NejcZdovc NejcZdovc added CI/skip-ios Do not run CI builds for iOS CI/skip-linux CI/skip-macos-x64 Do not run CI builds for macOS x64 labels Feb 20, 2020
NejcZdovc
NejcZdovc approved these changes Feb 20, 2020
View reviewed changes
Copy link
Contributor

@NejcZdovc NejcZdovc left a comment

Choose a reason for hiding this comment

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

rewards code looks good

@tmancey tmancey merged commit b145d94 into master Feb 20, 2020
@tmancey tmancey deleted the issues/8294 branch February 20, 2020 16:31
@tmancey tmancey removed CI/skip-ios Do not run CI builds for iOS CI/skip-linux CI/skip-macos-x64 Do not run CI builds for macOS x64 PR/Pending Review labels Feb 20, 2020
@tmancey tmancey modified the milestones: 1.6.x - Dev, 1.7.x - Nightly Feb 20, 2020
This was referenced Mar 30, 2020
Closed
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@gdregalo gdregalo gdregalo approved these changes

@masparrow masparrow masparrow approved these changes

@NejcZdovc NejcZdovc NejcZdovc approved these changes

@moritzhaller moritzhaller moritzhaller approved these changes

@kylehickinson kylehickinson kylehickinson approved these changes

@emerick emerick Awaiting requested review from emerick

Assignees

@tmancey tmancey

Labels
feature/ads
Projects
None yet
Milestone
1.7.x - Release
Development

Successfully merging this pull request may close these issues.

Refactor Brave Ads in preparation for merging Publisher Ads
6 participants
@tmancey @NejcZdovc @kylehickinson @moritzhaller @masparrow @gdregalo