Add support for payout status #14063

emerick · 2022-07-06T20:03:51Z

Submitter Checklist:

I confirm that no security/privacy review is needed, or that I have requested one
There is a ticket for my issue
Used Github auto-closing keywords in the PR description above
Wrote a good PR/commit description
Squashed any review feedback or "fixup" commits before merge, so that history is a record of what happened in the repo, not your PR
Added appropriate labels (QA/Yes or QA/No; release-notes/include or release-notes/exclude; OS/...) to the associated issue
Checked the PR locally: npm run test -- brave_browser_tests, npm run test -- brave_unit_tests, npm run lint, npm run gn_check, npm run tslint
Ran git rebase master (if needed)

Reviewer Checklist:

A security review is not needed, or a link to one is included in the PR description
New files have MPL-2.0 license header
Adequate test coverage exists to prevent regressions
Major classes, functions and non-trivial code blocks are well-commented
Changes in component dependencies are properly reflected in gn
Code follows the style guide
Test plan is specified in PR before merging

After-merge Checklist:

Test Plan:

Testing is similar to brave/brave-browser#18602 with the additional caveat that payout status is now controlled via a server endpoint which returns one of the following values for a wallet provider or unverified account:

off (no payment message will be shown at all)
processing (blue "Rewards are on the way" banner is shown)
complete (green "Your Rewards have arrived" banner is shown)

Scenario 1: Payment pending messaging

Given that user has ad earnings for the previous month
And the current day of month is before the payout day
When the user opens the panel, the NTP, or the rewards page
Then the user should see the "payment pending" message.

Scenario 1: Payment processing messaging

Given that user has ad earnings for the previous month
And the user has not received an Ad grant this month
And the server indicates that the payout status is "processing" for this provider
When the user opens the panel, the NTP, or the rewards page
Then the user should see the "payment processing" message.

Scenario 3: Payment arrived messaging

Given that user has ad earnings for the previous month
And the user has received an Ad grant this month
And the server indicates that the payout status is "complete" for this provider
When the user opens the panel, the NTP, or the rewards page
Then the user should see the "payment arrived" message.

brave-builds · 2022-07-06T21:31:45Z

A Storybook has been deployed to preview UI for the latest push

brave-builds · 2022-07-06T22:19:27Z

A Storybook has been deployed to preview UI for the latest push

brave-builds · 2022-07-07T00:58:47Z

A Storybook has been deployed to preview UI for the latest push

zenparsing

Looks great - mostly just need to look at the any things.

zenparsing · 2022-07-08T13:26:32Z

vendor/bat-native-ledger/src/bat/ledger/internal/endpoint/api/get_parameters/get_parameters.cc

+    return type::Result::LEDGER_ERROR;
+  }
+
+  for (const auto [key, value] : *payout_status_dict) {


Is this copying key and value? Is that intentional?

Yeah, it is copying them due to this (new I think?) error in clang:

../../brave/vendor/bat-native-ledger/src/bat/ledger/internal/endpoint/api/get_parameters/get_parameters.cc(139,20): error: loop variable '[key, value]' binds to a temporary value produced by a range of type 'const base::Value::Dict' [-Werror,-Wrange-loop-bind-reference] for (const auto& [key, value] : *payout_status_dict) { ^ ../../brave/vendor/bat-native-ledger/src/bat/ledger/internal/endpoint/api/get_parameters/get_parameters.cc(139,8): note: use non-reference type 'base::detail::const_dict_iterator::reference' (aka 'pair<const basic_string<char, char_traits<char>, allocator<char>> &, const base::Value &>') for (const auto& [key, value] : *payout_status_dict) { ^~~~~~~~~~~~~~~~~~~~~~~~~~ 1 error generated.

I'll use an iterator instead to avoid the copy.

I see, the dict iterator constructs a pair when dereferenced, whose lifetime gets extended by the destructured binding, and we have a compiler setting on to complain about that.

Does for (const auto& pair : *payout_status_dict) work?

Or maybe it needs to be for (const auto pair : *payout_status_dict).

Also curious if for (const auto&& [key, value] : *payout_status_dict) works :)

Yes that does work, perfect!

Ha, that's super weird. That's because base::detail::const_dict_iterator is a proxy iterator, and Clang warns you that you're extending the lifetime of a std::pair<const std::string&, const base::Value&> temporary. Not sure why it doesn't complain about const auto&&, as you're still extending the lifetime... let's remove this const, though, since it doesn't convey too much semantic meaning here. 🙂
Also, you can use emplace<>():

for (auto&& [key, value] : *payout_status_dict) { if (value.is_string()) { parameters->payout_status.emplace(key, value.GetString()); } }

Thanks, I always forget about emplace!

Regarding the && - it's a little cryptic, but I suppose it means that we're extending lifetimes explicitly instead of implicitly, which is better?

Clang complains about lifetime extension, so it should emit the same warning regardless of whether you're extending the lifetime via a const lvalue reference, or an rvalue reference. As far as extending lifetime via &&, this is more like implicit, since auto&& doesn't necessarily have to be an rvalue reference, it depends on type deduction (although in this specific case, it is). const auto&, however, is an lvalue reference-to-const (and not a universal/forwarding reference), hence it's more explicit.

@emerick @zenparsing FYI: #14176 (comment)

components/brave_rewards/resources/shared/lib/provider_payout_status.ts

common/extensions/api/brave_rewards.json

components/definitions/newTab.d.ts

components/definitions/rewards.d.ts

brave-builds · 2022-07-11T13:36:58Z

A Storybook has been deployed to preview UI for the latest push

browser/extensions/api/brave_rewards_api.cc

browser/ui/webui/brave_rewards_page_ui.cc

vendor/bat-native-ledger/src/bat/ledger/internal/state/state.cc

components/brave_rewards/resources/shared/lib/provider_payout_status.ts

zenparsing · 2022-07-11T14:57:15Z

components/brave_rewards/resources/shared/components/payment_status_view.tsx

@@ -126,7 +128,7 @@ export function PaymentStatusView (props: Props) {
    return null
  }

-  if (props.earningsReceived) {
+  if (props.earningsReceived && props.providerPayoutStatus === 'complete') {


It seems like we'll still show the processing messaging if the status is "processing", even if props.earningsReceived is true (meaning that the user has accepted the grant). Could that mean that we still show "processing" even after the user claims the grant?

Also, we still use the 3 day window for determining whether to show this messaging at all. Do we still want that?

Fixed up the processing issue, good catch.

Regarding the 3-day window, I did intentionally leave that as-is but maybe that wasn't a good idea. @Miyayes do you have any thoughts on that?

Conclusion:

Remove 3-day window and just display banner based on status from /parameters endpoint

Only exception is for props.earningsReceived, whereby if it is true, no banner will show (regardless of what is received from /parameters) until the following month

Spoke with @Miyayes privately and we decided to remove the 3-day window and just rely on the endpoint.

brave-builds · 2022-07-11T15:24:23Z

A Storybook has been deployed to preview UI for the latest push

brave-builds · 2022-07-11T17:01:23Z

A Storybook has been deployed to preview UI for the latest push

brave-builds · 2022-07-11T20:44:35Z

A Storybook has been deployed to preview UI for the latest push

brave-builds · 2022-07-11T21:43:30Z

A Storybook has been deployed to preview UI for the latest push

emerick · 2022-07-11T22:51:10Z

Windows-based CI runs failed in S3 upload step; other than that, all CI platforms passed.

Miyayes · 2022-08-08T15:45:53Z

Test Plan:

Check for crashes on older versions of Brave

Older versions of Brave also hit the /parameters endpoint. However, they don't necessarily contain logic to handle responses that contain "payoutStatus"-related values. As a result, we should make sure that older versions of Brave do not crash or have problems simply because some new values are coming back from /parameters.

Check 1.42.x does not error out or crash
Check 1.41.x does not error out or crash
Check 1.40.x does not error out or crash

Check new payout banner logic

The response from /parameters endpoint is the same for everybody. The values inside "payoutStatus" are set on the server side.

Example response:

"payoutStatus": {                                                                                                                      
    "unverified": "off",                                                                                                                 
    "uphold": "processing",                                                                                                              
    "gemini": "off",                                                                                                                     
    "bitflyer": "off"
  }

If user is Uphold, then the browser logic only cares about the value from "uphold":"value"
If user is unverified, the browser logic only cares about the value from "unverified":"value"
If user is Gemini, then the browser logic only cares about value from “"gemini":"value"
If user is bitFlyer, then the browser logic only cares about value from "bitflyer":"value"

When value is "off"

No payout banner should be shown.

When value is "processing"

The following banner should be shown:

When value is "complete"

The following banner should be shown:

Note: If the payout countdown banner ("your payout will arrive in x days") banner is present, it will take precedence over payoutStatus banner. For example, if payout date == 7th and it is the 4th, it will say "your payout will arrive in 3 days", and not show any payoutStatus banner.

emerick self-assigned this Jul 6, 2022

emerick added the CI/run-draft label Jul 6, 2022

github-actions bot added the CI/storybook-url Deploy storybook and provide a unique URL for each build label Jul 6, 2022

emerick force-pushed the emerick-rewards-payout-status branch 2 times, most recently from bd5cff4 to f6c926f Compare July 6, 2022 21:00

emerick force-pushed the emerick-rewards-payout-status branch from f6c926f to 6df470f Compare July 6, 2022 21:50

emerick force-pushed the emerick-rewards-payout-status branch from 6df470f to a4da516 Compare July 7, 2022 00:28

emerick marked this pull request as ready for review July 7, 2022 17:09

emerick requested a review from a team as a code owner July 7, 2022 17:09

emerick removed the CI/run-draft label Jul 7, 2022

zenparsing reviewed Jul 8, 2022

View reviewed changes

emerick force-pushed the emerick-rewards-payout-status branch 2 times, most recently from 42cdbb1 to 1bcb3e3 Compare July 11, 2022 12:59

emerick requested a review from zenparsing July 11, 2022 13:04

szilardszaloki reviewed Jul 11, 2022

View reviewed changes

browser/extensions/api/brave_rewards_api.cc Outdated Show resolved Hide resolved

szilardszaloki reviewed Jul 11, 2022

View reviewed changes

browser/ui/webui/brave_rewards_page_ui.cc Outdated Show resolved Hide resolved

emerick force-pushed the emerick-rewards-payout-status branch from 1bcb3e3 to 5f5c6f5 Compare July 11, 2022 14:17

emerick requested a review from szilardszaloki July 11, 2022 14:17

emerick force-pushed the emerick-rewards-payout-status branch from 5f5c6f5 to f7809e8 Compare July 11, 2022 14:36

szilardszaloki reviewed Jul 11, 2022

View reviewed changes

vendor/bat-native-ledger/src/bat/ledger/internal/state/state.cc Outdated Show resolved Hide resolved

szilardszaloki reviewed Jul 11, 2022

View reviewed changes

vendor/bat-native-ledger/src/bat/ledger/internal/state/state.cc Outdated Show resolved Hide resolved

szilardszaloki approved these changes Jul 11, 2022

View reviewed changes

emerick force-pushed the emerick-rewards-payout-status branch from f7809e8 to b52a882 Compare July 11, 2022 14:54

zenparsing reviewed Jul 11, 2022

View reviewed changes

emerick force-pushed the emerick-rewards-payout-status branch from b52a882 to 95bd02a Compare July 11, 2022 16:33

zenparsing approved these changes Jul 11, 2022

View reviewed changes

emerick force-pushed the emerick-rewards-payout-status branch from 95bd02a to 3262beb Compare July 11, 2022 18:51

emerick force-pushed the emerick-rewards-payout-status branch from 3262beb to f983339 Compare July 11, 2022 21:09

Adding support for payout status

a4cf4c4

emerick force-pushed the emerick-rewards-payout-status branch from f983339 to a4cf4c4 Compare July 11, 2022 21:13

emerick merged commit d4d8d96 into master Jul 11, 2022

emerick deleted the emerick-rewards-payout-status branch July 11, 2022 22:51

emerick added this to the 1.43.x - Nightly milestone Jul 11, 2022

This was referenced Sep 7, 2022

Use responsive Rewards page on Android #14604

Merged

Fix payment status link in WebUI Rewards panel #15018

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for payout status #14063

Add support for payout status #14063

emerick commented Jul 6, 2022 •

edited

Loading

brave-builds commented Jul 6, 2022

brave-builds commented Jul 6, 2022

brave-builds commented Jul 7, 2022

zenparsing left a comment

zenparsing Jul 8, 2022

emerick Jul 8, 2022

zenparsing Jul 8, 2022 •

edited

Loading

zenparsing Jul 8, 2022

emerick Jul 8, 2022

szilardszaloki Jul 11, 2022 •

edited

Loading

emerick Jul 11, 2022

zenparsing Jul 11, 2022

szilardszaloki Jul 11, 2022 •

edited

Loading

szilardszaloki Jul 21, 2022

brave-builds commented Jul 11, 2022

zenparsing Jul 11, 2022

emerick Jul 11, 2022

Miyayes Jul 11, 2022 •

edited

Loading

emerick Jul 11, 2022

brave-builds commented Jul 11, 2022

brave-builds commented Jul 11, 2022

brave-builds commented Jul 11, 2022

brave-builds commented Jul 11, 2022

emerick commented Jul 11, 2022

Miyayes commented Aug 8, 2022 •

edited

Loading

Add support for payout status #14063

Add support for payout status #14063

Conversation

emerick commented Jul 6, 2022 • edited Loading

Submitter Checklist:

Reviewer Checklist:

After-merge Checklist:

Test Plan:

Scenario 1: Payment pending messaging

Scenario 1: Payment processing messaging

Scenario 3: Payment arrived messaging

brave-builds commented Jul 6, 2022

brave-builds commented Jul 6, 2022

brave-builds commented Jul 7, 2022

zenparsing left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zenparsing Jul 8, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

szilardszaloki Jul 11, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

szilardszaloki Jul 11, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

brave-builds commented Jul 11, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Miyayes Jul 11, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

brave-builds commented Jul 11, 2022

brave-builds commented Jul 11, 2022

brave-builds commented Jul 11, 2022

brave-builds commented Jul 11, 2022

emerick commented Jul 11, 2022

Miyayes commented Aug 8, 2022 • edited Loading

Test Plan:

Check for crashes on older versions of Brave

Check new payout banner logic

emerick commented Jul 6, 2022 •

edited

Loading

zenparsing Jul 8, 2022 •

edited

Loading

szilardszaloki Jul 11, 2022 •

edited

Loading

szilardszaloki Jul 11, 2022 •

edited

Loading

Miyayes Jul 11, 2022 •

edited

Loading

Miyayes commented Aug 8, 2022 •

edited

Loading