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

Support Ads history and feedback loop #2792

Merged
merged 5 commits into from
Aug 9, 2019
Merged

Support Ads history and feedback loop #2792

merged 5 commits into from
Aug 9, 2019

Conversation

emerick
Copy link
Contributor

@emerick emerick commented Jun 25, 2019 •
edited by NejcZdovc
Loading

Fixes brave/brave-browser#4047

UI PR: brave/brave-ui#465

Submitter Checklist:

Test Plan:

  • Create a new profile
  • Launch Brave and view several ads
  • Visit brave://rewards and click 7-day Ads History link in Ads section
  • No more than 7 days worth of ads should ever appear in this list
  • Click thumbs down on an ad and ensure that:
    • The ad appears in filtered_ads section of client.json file in user profile directory
    • No additional ads in this creative set appear
  • Click thumbs up on the same ad and ensure that:
    • The ad is removed from the filtered_ads section of client.json
    • This ad and other ads in this creative set should again be eligible for viewing
  • Click heart icon for a category and ensure that:
    • All entries with that category are hearted
  • Click circle-and-slash (prohibit) icon on the same ad and ensure that:
    • All entries with that category are "prohibited"
    • This category appears in filtered_categories section of client.json
  • Save and Mark as Inappropriate (under ... icon) work similarly:
    • Clicking them should reflect new status
    • Entry should appear in saved_ads or flagged_ads as appropriate
    • Flagged ads should not be shown, same as thumbs down ads
    • Saving an ad should create a new "toolbar" at the top of the page, allowing you to see the saved ads

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.

@emerick emerick self-assigned this Jun 25, 2019
@emerick emerick force-pushed the ads-history branch 11 times, most recently from 6014339 to 564242c Compare July 2, 2019 01:55
tmancey
tmancey reviewed Jul 2, 2019
View reviewed changes
browser/ui/webui/brave_rewards_ui.cc Outdated Show resolved Hide resolved
browser/ui/webui/brave_rewards_ui.cc Outdated
#include "base/memory/weak_ptr.h"
#include "base/time/time.h"
#include "base/i18n/time_formatting.h"
#include "bat/ads/ad_history_detail.h"
Copy link
Collaborator

Choose a reason for hiding this comment

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

All Ads code should be wrapped in guards

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Will just remove this as part of refactoring the WebUI.

browser/ui/BUILD.gn Outdated Show resolved Hide resolved
components/brave_ads/browser/ads_service.h Show resolved Hide resolved
components/brave_ads/browser/ads_service.h Show resolved Hide resolved
vendor/bat-native-ads/src/bat/ads/internal/ads_impl.cc Outdated Show resolved Hide resolved
vendor/bat-native-ads/src/bat/ads/internal/ads_impl.cc Show resolved Hide resolved
vendor/bat-native-ads/src/bat/ads/internal/ads_impl.cc Outdated Show resolved Hide resolved
vendor/bat-native-ads/src/bat/ads/internal/ads_impl.cc Outdated Show resolved Hide resolved
vendor/bat-native-ads/src/bat/ads/internal/ads_impl.cc Outdated Show resolved Hide resolved
@emerick emerick force-pushed the ads-history branch 3 times, most recently from aa936c0 to 4d5030c Compare July 5, 2019 20:13
kylehickinson
kylehickinson previously approved these changes Aug 6, 2019
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 changes looks good

@emerick emerick force-pushed the ads-history branch 3 times, most recently from 139af2b to dc18fff Compare August 7, 2019 13:27
@emerick emerick force-pushed the ads-history branch 3 times, most recently from 79f3039 to fc10e05 Compare August 9, 2019 16:54
@tmancey tmancey self-requested a review August 9, 2019 17:41
tmancey
tmancey previously approved these changes Aug 9, 2019
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++

@emerick
Copy link
Contributor Author

emerick commented Aug 9, 2019 •
edited
Loading

@kylehickinson Mind re-approving, had to make a small (non-iOS) change? Thanks!

kylehickinson
kylehickinson previously approved these changes Aug 9, 2019
View reviewed changes
Copy link
Collaborator

@kylehickinson kylehickinson left a comment •
edited
Loading

Choose a reason for hiding this comment

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

I'm not needed for approval in this PR because no iOS code is changed, but sure :) I stand corrected, this is a different PR lol

@NejcZdovc
Copy link
Contributor

@emerick heads up about CI (https://staging.ci.brave.com/job/brave-browser-build-pr/job/ads-history/15/execution/node/510/log/)

@emerick emerick dismissed stale reviews from kylehickinson and tmancey via c928e2d August 9, 2019 19:49
tmancey
tmancey approved these changes Aug 9, 2019
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++

@emerick emerick merged commit e86910e into master Aug 9, 2019
@emerick emerick deleted the ads-history branch August 9, 2019 22:30
@tmancey tmancey added this to the 0.70.x - Dev milestone Aug 14, 2019
@LaurenWags LaurenWags mentioned this pull request Sep 27, 2019
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jasonrsadler jasonrsadler jasonrsadler left review comments

@kylehickinson kylehickinson kylehickinson approved these changes

@tmancey tmancey tmancey approved these changes

@NejcZdovc NejcZdovc Awaiting requested review from NejcZdovc

@jenn-rhim jenn-rhim Awaiting requested review from jenn-rhim

Assignees

@emerick emerick

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

Successfully merging this pull request may close these issues.

Brave Ads: display 7-day ads history and feedback options for viewed ads
6 participants
@emerick @NejcZdovc @kylehickinson @tmancey @jenn-rhim @jasonrsadler