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

Publisher updates flow #1457

Merged
merged 6 commits into from
Feb 1, 2019
Merged

Publisher updates flow #1457

merged 6 commits into from
Feb 1, 2019

Conversation

NejcZdovc
Copy link
Contributor

@NejcZdovc NejcZdovc commented Jan 25, 2019 •
edited
Loading

Resolves brave/brave-browser#2973
Resolves brave/brave-browser#3015
Resolves brave/brave-browser#3135

Submitter Checklist:

  • Submitted a ticket for my issue if one did not already exist.
  • Used Github auto-closing keywords in the commit message.
  • Added/updated tests for this change (for new code or code which already has tests).
  • Verified that these changes build without errors on
    • Windows
    • macOS
    • Linux
  • Verified that these changes pass automated tests (npm test brave_unit_tests && npm test brave_browser_tests) on
    • Windows
    • macOS
    • Linux
  • Ran git rebase master (if needed).
  • Ran git rebase -i to squash commits (if needed).
  • Tagged reviewers and labelled the pull request as needed.
  • Request a security/privacy review as needed.
  • Add appropriate QA labels (QA/Yes or QA/No) to include the closed issue in milestone

Test Plan:

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

@NejcZdovc NejcZdovc added this to the 0.62.x - Nightly milestone Jan 25, 2019
@NejcZdovc NejcZdovc self-assigned this Jan 25, 2019
@NejcZdovc NejcZdovc force-pushed the publisher-updates branch 3 times, most recently from 60f5db1 to 7c30622 Compare January 25, 2019 18:44
bridiver
bridiver reviewed Jan 25, 2019
View reviewed changes
vendor/bat-native-ledger/src/ledger_impl.cc Outdated
void LedgerImpl::SetPublisherInfo(std::unique_ptr<ledger::PublisherInfo> info,
ledger::PublisherInfoCallback callback) {
uint64_t window_id) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

if we get rid of the callback, how does the caller find out about errors?

bridiver
bridiver reviewed Jan 25, 2019
View reviewed changes
components/brave_rewards/browser/rewards_service_impl.cc
void RewardsServiceImpl::OnNormalizedPublisherListSaved(
std::unique_ptr<ledger::PublisherInfoList> list) {
if (!list) {
LOG(ERROR) << "Problem saving normalized publishers "
Copy link
Collaborator

Choose a reason for hiding this comment

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

we can't just log this error and ignore it here, SaveNormalizedPublisherList needs a callback

Copy link
Contributor Author

@NejcZdovc NejcZdovc Jan 28, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

sure, we can add callback to go back to native ledger. What should we do in native ledger with this callback?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

what we were doing with save publisher actions was redirecting callback to dummy one

bridiver
bridiver previously requested changes Jan 25, 2019
View reviewed changes
Copy link
Collaborator

@bridiver bridiver left a comment

Choose a reason for hiding this comment

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

see below

@NejcZdovc NejcZdovc force-pushed the publisher-updates branch 2 times, most recently from c1a0513 to 2f200e0 Compare January 28, 2019 08:11
This was referenced Jan 28, 2019
Closed
Closed
NejcZdovc added a commit that referenced this pull request Jan 29, 2019
@NejcZdovc
Adds new flow
600a294 
Resolves #1457
@NejcZdovc NejcZdovc force-pushed the publisher-updates branch 2 times, most recently from cef247a to dc0dfd7 Compare January 29, 2019 08:25
ryanml
ryanml reviewed Jan 30, 2019
View reviewed changes
Copy link
Contributor

@ryanml ryanml left a comment

Choose a reason for hiding this comment

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

Pertaining to:

brave/brave-browser#3015
brave/brave-browser#3135

Both issues were addressed. A general smoke test of Rewards (adding sites to a-c, removal and restoration) produced no unexpected behavior or errors. This is now just awaiting a final look from brian j and a rebase. Good work!

@NejcZdovc
Copy link
Contributor Author

rebased, cc @bridiver for a final review

@NejcZdovc NejcZdovc merged commit 782aab8 into master Feb 1, 2019
@NejcZdovc NejcZdovc deleted the publisher-updates branch February 1, 2019 04:59
@NejcZdovc NejcZdovc mentioned this pull request Feb 1, 2019
Merged
NejcZdovc added a commit that referenced this pull request Feb 1, 2019
@NejcZdovc
Merge pull request #1457 from brave/publisher-updates
e066ec1 
Publisher updates flow
@NejcZdovc NejcZdovc mentioned this pull request Feb 6, 2019
Closed
jasonrsadler
jasonrsadler reviewed Apr 7, 2019
View reviewed changes
...brave_rewards/resources/extension/brave_rewards/background/reducers/rewards_panel_reducer.ts
@@ -82,7 +82,11 @@ export const rewardsPanelReducer = (state: RewardsExtension.State | undefined, a
const id = getWindowId(tab.windowId)
const publishers: Record<string, RewardsExtension.Publisher> = state.publishers
const publisher = publishers[id]
chrome.braveRewards.getPublisherData(tab.windowId, tab.url, tab.favIconUrl || '', payload.publisherBlob || '')

if ((payload.onlyDiff && publisher && publisher.tabUrl !== tab.url) || !payload.onlyDiff) {
Copy link
Contributor

Choose a reason for hiding this comment

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

This seems to be causing problems with getting media publishers in panel

Copy link
Contributor Author

Choose a reason for hiding this comment

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

this is working as expected, fix for the panel not working #2178

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

@ryanml ryanml ryanml approved these changes

@bridiver bridiver Awaiting requested review from bridiver

Assignees

@NejcZdovc NejcZdovc

Labels
feature/rewards
Projects
None yet
Milestone
0.62.x - Release
4 participants
@NejcZdovc @bridiver @ryanml @jasonrsadler