Skip to content

/ brave-core

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

Revert rewards observer change #985

Merged
merged 2 commits into from Nov 29, 2018
Merged

Revert rewards observer change #985

merged 2 commits into from Nov 29, 2018

Conversation

@emerick
Copy link
Contributor

emerick commented Nov 29, 2018 •
edited

This reverts a recent change that moved OnGetPublisherActivityFromUrl from a private observer into a public observer. Although Android needs access to that function, we can't expose ledger types in the Rewards public API.

Associated with brave/brave-browser#2019

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
@emerick
Revert "Merge pull request #840 from brave/rewards-move-callback-to-p…
7c14ad5 
…ublic-observer"

This reverts commit f48cdbc, reversing
changes made to 1634a63.
@emerick emerick self-assigned this Nov 29, 2018
@emerick emerick requested review from bridiver and SergeyZhukovsky Nov 29, 2018
@emerick emerick force-pushed the revert-rewards-observer-change branch from 0e81b15 to 663d208 Nov 29, 2018
@emerick
Revert the function signature for OnWalletProperties
Loading status checks…
663d208 
The code we're reverting changed the function signature and subsequent
code started to use the new signature. This reverts those changes as
well.
@emerick
Copy link
Contributor Author

emerick commented Nov 29, 2018

@bsclifton Just pinging you as this touched the importer slightly (it reverts a function signature change for a function that the importer was relying on). It's pretty simple and split out into a different commit in this PR, just wanted you to be aware.
@bridiver
bridiver approved these changes Nov 29, 2018
View changes
@emerick emerick merged commit 30e7bbb into master Nov 29, 2018
1 check passed
1 check passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@emerick emerick deleted the revert-rewards-observer-change branch Nov 29, 2018
@emerick
Copy link
Contributor Author

emerick commented Nov 29, 2018 •
edited

master: 30e7bbb
0.58.x: 80253f2
@bsclifton
Copy link
Member

bsclifton commented Nov 29, 2018

@emerick ah- thanks for the heads up 😄 I ran into this in 0.57.x actually; here's a link to the PR where I fixed the error (basically by reverting the signature - since that didn't make its way back to 0.57.x)
https://github.com/brave/brave-core/pull/984/files

@emerick does this revert need to go into 0.58.x also? Because #779 was merged back to there
emerick added a commit that referenced this pull request Nov 29, 2018
@emerick
Merge pull request #985 from brave/revert-rewards-observer-change
Loading status checks…
80253f2 
Revert rewards observer change
@emerick emerick mentioned this pull request Nov 30, 2018
Merged
6 of 17 tasks complete
@bbondy bbondy added this to the 0.58.x - Release milestone Jan 14, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bridiver bridiver

@SergeyZhukovsky SergeyZhukovsky

Assignees

@emerick emerick

Labels
feature/rewards
Projects
None yet
Milestone
  0.58.x - Release
Linked issues

Successfully merging this pull request may close these issues.

None yet

5 participants
@emerick @bsclifton @bridiver @bbondy @NejcZdovc
You can’t perform that action at this time.