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

Adds functionality to monthly contribution dropdown (Panel) #957

Merged
merged 1 commit into from
Dec 3, 2018

Conversation

ryanml
Copy link
Contributor

@ryanml ryanml commented Nov 27, 2018

Fixes: brave/brave-browser#2245

The brunt of the work for this was adding extension functions to support:

Adding a recurring contribution
Removing a recurring contribution
Retrieving recurring contributions

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:

  1. Enable Brave Rewards
  2. Navigate to a site such as brave.com
  3. Bring up the wallet panel.
  4. Change monthly contribution amount to '5.0'
  5. Close and re-open panel
  6. Confirm that the value '5.0' persists
  7. Confirm that the change is reflected in the donation table

The above six steps should be tried in different ways with different values. Ex: Ensuring that the data persists across tabs with the same site open

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

@@ -1502,6 +1508,10 @@ void RewardsServiceImpl::OnTipsUpdatedData(const ledger::PublisherInfoList list)
}
}

void RewardsServiceImpl::AddRecurringPayment(const std::string& publisher_key, double new_amount) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we just use SaveRecurringDonation directly? So that we don't need to add new function

Copy link
Contributor

@jasonrsadler jasonrsadler Nov 30, 2018

Choose a reason for hiding this comment

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

@NejcZdovc SaveRecurringDonation is marked private and wouldn't be accessible to brave_rewards_api and tests

@ryanml ryanml force-pushed the monthly-amount-dropdown branch 4 times, most recently from 89f5191 to 1873817 Compare November 28, 2018 02:01
.release());

std::unique_ptr<extensions::Event> event(new extensions::Event(
extensions::events::BRAVE_START,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Using BRAVE_START constant per @bridiver

onAmountChange={this.onContributionAmountChange.bind(this, publisher.publisher_key)}
onIncludeInAuto={this.switchAutoContribute}
/>
<div style={{ height: '48px' }}></div>
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 a place holder that fills the space that would be taken up by the "Enable tips on action" component. This will be reworked once that feature is supported.

@@ -306,6 +307,7 @@ test("brave_browser_tests") {
":brave_browser_tests_deps",
":browser_tests_runner",
"//testing/gmock",
"//brave/vendor/bat-native-ledger",
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Adding because it was a dependency on unit tests only, not browser tests.

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 comments

@ryanml ryanml requested a review from bridiver November 28, 2018 22:58
@ryanml ryanml dismissed bridiver’s stale review November 28, 2018 23:00

Using ContentSiteList now

@ryanml ryanml mentioned this pull request Nov 29, 2018
18 tasks
@ryanml ryanml force-pushed the monthly-amount-dropdown branch from be4e3bc to 0ba41c0 Compare November 29, 2018 05:33
@jasonrsadler
Copy link
Contributor

@ryanml please rebase

@ryanml ryanml force-pushed the monthly-amount-dropdown branch from 0ba41c0 to fb19e25 Compare November 30, 2018 02:19
@ryanml
Copy link
Contributor Author

ryanml commented Nov 30, 2018

@jasonrsadler rebased

@jasonrsadler
Copy link
Contributor

Currently getting build failure:

In file included from ../../brave/components/brave_rewards/browser/extension_rewards_service_observer.cc:5:
In file included from ../../brave/components/brave_rewards/browser/extension_rewards_service_observer.h:11:
../../brave/components/brave_rewards/browser/rewards_service_observer.h:56:7: error: use of undeclaredidentifier 'ledger'
      ledger::PublisherInfo* info,
      ^
In file included from ../../brave/components/brave_rewards/browser/extension_rewards_service_observer.cc:5:
../../brave/components/brave_rewards/browser/extension_rewards_service_observer.h:38:8: error: 'brave_rewards::ExtensionRewardsServiceObserver::OnGetPublisherActivityFromUrl' hides overloaded virtual function [-Werror,-Woverloaded-virtual]
  void OnGetPublisherActivityFromUrl(
       ^
../../brave/components/brave_rewards/browser/rewards_service_observer.h:53:16: note: hidden overloadedvirtual function 'brave_rewards::RewardsServiceObserver::OnGetPublisherActivityFromUrl' declared here:type mismatch at 3rd parameter ('int *' vs 'std::unique_ptr<ledger::PublisherInfo>')
  virtual void OnGetPublisherActivityFromUrl(

@emerick
Copy link
Contributor

emerick commented Nov 30, 2018

@jasonrsadler I think you may need #985, which reverts a change that mistakenly added a ledger type to the Rewards public API.

@jasonrsadler
Copy link
Contributor

Thanks @emerick
That seems to be the case as my master doesn't build either.
I'll resync and get back to this @ryanml

@jasonrsadler
Copy link
Contributor

This might require another rebase to get building

@ryanml ryanml force-pushed the monthly-amount-dropdown branch from fb19e25 to 18f492a Compare December 3, 2018 03:41
@ryanml
Copy link
Contributor Author

ryanml commented Dec 3, 2018

@jasonrsadler rebased

@ryanml ryanml force-pushed the monthly-amount-dropdown branch from 18f492a to a4959e8 Compare December 3, 2018 05:34
Copy link
Contributor

@jasonrsadler jasonrsadler left a comment

Choose a reason for hiding this comment

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

Amount picklist is clipped at borders.
Let me know if this is handled elsewhere:
screen shot 2018-12-03 at 3 01 41 pm

@ryanml
Copy link
Contributor Author

ryanml commented Dec 3, 2018

@jasonrsadler dropdown clipping is part of a larger issue, limited I believe by a chrome extension panel's capabilities. Cc: @petemill

@petemill
Copy link
Member

petemill commented Dec 3, 2018

@ryanml if we're going to use this custom html to simulate a dropdown, then we'll need to change it to avoid the viewport / window edge.

@ryanml
Copy link
Contributor Author

ryanml commented Dec 3, 2018

@petemill my mistake, I remember that was the intended approach now, which is captured as part of this issue: brave/brave-ui#126

So this problem will be addressed separately, I need to work on the above issue soon.

@ryanml ryanml dismissed jasonrsadler’s stale review December 3, 2018 20:56

clipping will be addessed separately

@ryanml ryanml requested a review from jasonrsadler December 3, 2018 20:56
@jasonrsadler jasonrsadler merged commit dcc6e31 into brave:master Dec 3, 2018
@jasonrsadler
Copy link
Contributor

master (0.60.x): dcc6e31

@bbondy
Copy link
Member

bbondy commented Dec 4, 2018

Reverted from master here due to failing unit tests:
f77acbe

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Monthly Contribution Dropdown in Wallet Panel should retain value
7 participants