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

Add configurable default tip amounts #3848

Merged
merged 3 commits into from
Dec 5, 2019

Conversation

jhoneycutt
Copy link

@jhoneycutt jhoneycutt commented Oct 31, 2019

Submitter Checklist:

Resolves brave/brave-browser#6011

Test Plan:

Test case

  1. Launch on staging
  2. Enable Rewards and claim grant
  3. Visit a verified publisher with publisher-defined default tip amounts
  4. Open tip panel and choose "Send a tip..."
  5. Ensure that publisher's defaults are shown in the site banner

Test case

  1. Launch on staging
  2. Enable Rewards and claim grant
  3. Visit a publisher without publisher-defined default tip amounts
  4. Open tip panel and choose "Send a tip..."
  5. Ensure that wallet's defaults of [1, 10, 50] BAT are shown in the site banner

Test case

  1. Launch on staging
  2. Enable Rewards and claim grant
  3. Visit a publisher with publisher-defined default tip amounts
  4. Open tip panel and choose "Set..." to set a monthly contribution
  5. Ensure that publisher's defaults are shown in the site banner

Test case

  1. Launch on staging
  2. Enable Rewards and claim grant
  3. Visit a publisher without publisher-defined default tip amounts
  4. Open tip panel and choose "Set..." to set a monthly contribution
  5. Ensure that wallet's defaults of [1, 10, 50] BAT are shown in the site banner

Test case

  1. Launch on staging
  2. Enable Rewards and claim grant
  3. Visit a publisher with publisher-defined default tip amounts
  4. Open tip panel and choose "Set..." to set a monthly contribution
  5. Set monthly contribution to any value and click "Set Contribution"
  6. Open tip panel again
  7. Ensure that "Monthly Contribution" shows the value from step 4
  8. Click the value to change it
  9. Ensure that publisher's defaults are shown in the menu

Test case

  1. Launch on staging
  2. Enable Rewards and claim grant
  3. Visit a publisher without publisher-defined default tip amounts
  4. Open tip panel and choose "Set..." to set a monthly contribution
  5. Set contribution to 1.0 BAT and click "Set Contribution"
  6. Open tip panel again
  7. Ensure that "Monthly Contribution" shows "1.0 BAT"
  8. Click the value to change it
  9. Ensure that wallet's defaults of [1, 10, 50] BAT are shown in the menu

Test case

  1. Launch on staging
  2. Enable Rewards and claim grant
  3. Visit a publisher with publisher-defined default tip amounts
  4. Open tip panel and choose "Set..." to set a monthly contribution
  5. Set monthly contribution to any value and click "Set Contribution"
  6. Close the browser tab
  7. Modify values in server_publisher_amounts table in publisher_info_db to change the value set in step 4 to another value. For example, if the publisher has defaults of [5, 10, 20], and you selected 5 BAT in step 4, change the value in the table from 5 to 6 BAT
  8. Open a new tab with the same publisher
  9. Open tip panel again
  10. Ensure that "Monthly Contribution" shows the value from step 3
  11. Click the value to change it
  12. Ensure that publisher's new defaults are shown in the menu (e.g. [6, 10, 20])

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.

@jhoneycutt jhoneycutt added this to the 0.73.x - Nightly milestone Oct 31, 2019
@jhoneycutt jhoneycutt requested a review from a team October 31, 2019 22:35
@jhoneycutt jhoneycutt self-assigned this Oct 31, 2019
@jhoneycutt jhoneycutt force-pushed the issue-6011-configurable-default-tip-amounts branch 2 times, most recently from 24c13d0 to 2a099dd Compare November 1, 2019 17:20
@jhoneycutt
Copy link
Author

Failed pipeline steps are the network audit test, which appears to be filed as brave/brave-browser#6189.

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.

Can you confirm that we don't need a migration path on upgrade for new wallet properties? (For the client side, in case the now non-optional default tip choices wallet property is not immediately present)

@jhoneycutt jhoneycutt force-pushed the issue-6011-configurable-default-tip-amounts branch from 2a099dd to 0d0ab56 Compare November 4, 2019 19:06
@jhoneycutt
Copy link
Author

Can you confirm that we don't need a migration path on upgrade for new wallet properties? (For the client side, in case the now non-optional default tip choices wallet property is not immediately present)

We do not, and I've just added a unit test for this.

content::WebContents* site_banner) {
auto options = content::EvalJs(
site_banner,
R"(
Copy link
Collaborator

Choose a reason for hiding this comment

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

prefix(optional) R "delimiter( raw_characters )delimiter" (6) (since C++11)

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.

Preselected amount is not working. So when you open tip banner, middle selections should be preselected. In this PR this doesn't happen anymore. Same thing needs to happen on monthly tip.

Expected:
image

Actual:
image

@jhoneycutt jhoneycutt force-pushed the issue-6011-configurable-default-tip-amounts branch from 0d0ab56 to 52d40bd Compare November 12, 2019 06:26
@jhoneycutt jhoneycutt requested a review from NejcZdovc November 12, 2019 06:28
@jhoneycutt
Copy link
Author

Issue with default not being pre-selected noticed by @NejcZdovc was fixed.

@jhoneycutt jhoneycutt force-pushed the issue-6011-configurable-default-tip-amounts branch 4 times, most recently from 18d8d3a to f0893ef Compare November 15, 2019 03:22
@mandar-brave
Copy link

@jhoneycutt @NejcZdovc can we unblock the merge and get in to Master?

@jhoneycutt jhoneycutt force-pushed the issue-6011-configurable-default-tip-amounts branch 2 times, most recently from 309fb6a to 0261f35 Compare November 19, 2019 04:43
@NejcZdovc NejcZdovc force-pushed the issue-6011-configurable-default-tip-amounts branch from 0261f35 to 2e8da9a Compare December 2, 2019 10:15
@NejcZdovc NejcZdovc force-pushed the issue-6011-configurable-default-tip-amounts branch 2 times, most recently from e4dee37 to f1bcf3e Compare December 4, 2019 06:20
@NejcZdovc NejcZdovc force-pushed the issue-6011-configurable-default-tip-amounts branch from f1bcf3e to a7054be Compare December 4, 2019 16:57
@NejcZdovc NejcZdovc modified the milestones: 1.3.x - Dev, 1.4.x - Nightly Dec 5, 2019
@NejcZdovc NejcZdovc merged commit 5ca8cfc into master Dec 5, 2019
@NejcZdovc NejcZdovc deleted the issue-6011-configurable-default-tip-amounts branch December 5, 2019 07:49
brave-builds pushed a commit that referenced this pull request Dec 18, 2019
@kjozwiak
Copy link
Member

kjozwiak commented Jan 8, 2020

Went through the STR/Cases outlined under #3848 (comment) under the following build:

Brave 1.5.15 Chromium: 79.0.3945.88 (Official Build) nightly (64-bit)
Revision c2a58a36b9411c80829b4b154bfcab97e581f1f3-refs/branch-heads/3945@{#954}
OS macOS Version 10.15.2 (Build 19C57)

Looks like everything is working as expected 👍 Going to start the uplift process for #4248.

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.

Configurable default tip amounts
7 participants