Monthly statement can now be viewed from Rewards page #2733

jasonrsadler · 2019-06-18T21:07:57Z

Submitter Checklist:

Test Plan:

Start Brave and enable Rewards.
Add funds to wallet and perform contributions of all types.
Accept Ads and UGP Grants (if available)
open Rewards page and click 'View monthly statement for more details'
View and verify summary, transactions, monthly contributions, auto-contributions, and one-time tips
--
After merge it may be necessary to bring in a prepared profile that includes previous months. Required files need to be matching publisher_info_db, ledger_state, and publisher_state

Ensure that monthly dropdown operates correctly.

Note

Currently this PR allows for the current month to be displayed so long as wallet has had activity. Before merge, it is expected that only previous months from the current will be displayed.

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.

Additional Notes

Was attempting rebase and 403'd

jasonrsadler · 2019-07-13T20:18:56Z

++ @kylehickinson for iOS

emerick · 2019-07-15T14:12:34Z

browser/ui/webui/brave_rewards_ui.cc

+  args->GetString(1, &year);
+  int32_t month_conv;
+  uint32_t year_conv;
+  base::StringToInt(month, &month_conv);


Could we send these as integers from JavaScript?

emerick · 2019-07-15T14:15:13Z

components/brave_rewards/browser/rewards_service.h

@@ -240,6 +243,10 @@ class RewardsService : public KeyedService {
      const std::string& type,
      const std::map<std::string, std::string>& args,
      GetShareURLCallback callback) = 0;
+  virtual void GetMonthlyStatements(
+      int32_t month,


Is there a reason that month is signed and year is unsigned?

Yes, month maps to ledger::MONTH which can be -1 for ANY.

OK, gotcha.

emerick · 2019-07-15T14:18:55Z

components/brave_rewards/browser/rewards_service_browsertest.cc

+        ".getElementsByTagName('tbody')[0].childNodes.length",
+        content::EXECUTE_SCRIPT_DEFAULT_OPTIONS,
+        content::ISOLATED_WORLD_ID_CONTENT_END);
+    LOG(ERROR) << "=====table rows: " << acTableCountResult.ExtractInt();


Is this a leftover debug statement or intentional?

emerick · 2019-07-15T14:23:42Z

components/brave_rewards/resources/ui/components/pageWallet.tsx

+    for (let ix = 0; ix < monthsAvailable.length; ix++) {
+      const date = new Date(parseInt(monthsAvailable[ix].split('_')[0], 10), parseInt(monthsAvailable[ix].split('_')[1], 10) - 1)
+      const selectionStringValue = date.toLocaleString(navigator.language, { month: 'long' }) ;
+      (ix + 1) === monthsAvailable.length ? monthsJson += `\"${monthsAvailable[ix]}\" : \"${selectionStringValue} ${date.getFullYear()}\"` : monthsJson += `\"${monthsAvailable[ix]}\" : \"${selectionStringValue} ${date.getFullYear()}\",`


Nitpick: I think this would be clearer as an if-else, just given the length of this line

emerick · 2019-07-15T14:28:39Z

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

+      transaction->probi = report->grants;
+      transaction->category = 3;  // placeholder for grants on statement
+      base::Time parsed_time;
+      std::string time_str =


I've been using base::StringPrintf for this kind of string formatting, just FYI.

Thanks. This does work better. changed.

I'm a bit confused by this. Why do we have BalanceReport and also BalanceReportInfo and then converting again to TransactionStatementInfo?

@bridiver removed

emerick · 2019-07-15T19:19:00Z

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

+      base::Time parsed_time;
+      std::string time_str =
+          base::StringPrintf("%s/1/%s",
+            std::to_string(month).c_str(),


Probably simpler to use the %u format specifier instead of %s, then you don't need to convert to string.

bridiver · 2019-07-15T19:33:32Z

vendor/bat-native-ledger/include/bat/ledger/public/interfaces/ledger.mojom

+  int32 category;
+};
+
+struct MonthlyStatements {


I think we should avoid having plural types. We can have a MonthlyStatement type and pass an array of them, but that array does not require a new type.

bridiver · 2019-07-15T19:33:51Z

vendor/bat-native-ledger/include/bat/ledger/public/interfaces/ledger.mojom

+  array<TransactionStatementInfo> transactions;
+};
+
+struct AllTransactions {


same as above, I'm not sure this type is necessary at all

bridiver · 2019-07-15T19:36:51Z

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

@@ -1644,4 +1649,161 @@ void LedgerImpl::FetchBalance(ledger::FetchBalanceCallback callback) {
  bat_wallet_->FetchBalance(callback);
 }

+void LedgerImpl::OnGetAllTransactions(


trying to put all these different types together into a single query seems really overly complicated to me. I think we either need a new type with its own database query or do multiple queries to Ledger to get the different data types

These have been split out.

bridiver · 2019-07-15T19:41:23Z

vendor/bat-native-ledger/src/bat/ledger/internal/ledger_impl.h

+      ledger::ACTIVITY_MONTH month,
+      uint32_t year,
+      double current_balance,
+      ledger::MonthlyStatementCallback callback);


not sure if MonthlyStatement is something that ledger should care about. It's just returning some range of Transactions. It also seems a bit strange to me that the method is called GetAllTransactions/OnGetAllTransactions when it isn't getting all the transactions. I'm not really sure what the method should be called though because I'm not following exactly what we are returning since AllTransactions holds different types of data

bridiver · 2019-07-16T17:08:57Z

vendor/bat-native-ledger/include/bat/ledger/public/interfaces/ledger.mojom

  double value;
  uint64 date;
+  uint32 category;


this is much better than a blank field for recurrent tips

bridiver · 2019-07-16T17:20:21Z

components/brave_rewards/browser/rewards_service_impl.cc

+  ledger::PublisherInfoList list;
+
+  if (!backend) {
+    return all_transactions;


how do we tell the difference between an empty list and an error?

bridiver · 2019-07-18T15:12:44Z

vendor/bat-native-ledger/include/bat/ledger/ledger.h

@@ -279,6 +281,11 @@ class LEDGER_EXPORT Ledger {
      const ledger::MonthlyStatementCallback& callback,
      ledger::ACTIVITY_MONTH month,
      uint32_t year) = 0;
+
+  virtual void GetOneTimeTipsStatements(


I think we should just add an optional month and year to the existing GetOneTimeTips method

bridiver · 2019-07-18T15:52:41Z

browser/ui/webui/brave_rewards_ui.cc

-  int32_t month = args->GetList()[0].GetInt();
-  int32_t year = args->GetList()[1].GetInt();
-  rewards_service_->GetMonthlyStatements(
+  int32_t month = args->GetList()[1].GetInt();


I know this was already like this, but we should avoid args->GetList()[] in favor of args->GetInteger(index, &month)

oh... actually I guess not

// DEPRECATED, use GetList()::operator[]::GetInt() instead. huh, wouldn't have thought that

jasonrsadler added feature/rewards CI/skip-android Do not run CI builds for Android CI/skip-linux CI/skip-ios Do not run CI builds for iOS labels Jun 18, 2019

jasonrsadler added this to the 0.68.x - Nightly milestone Jun 18, 2019

jasonrsadler self-assigned this Jun 18, 2019

jasonrsadler force-pushed the monthly_statement branch 2 times, most recently from d710dca to a3a272d Compare June 21, 2019 15:10

jasonrsadler added CI/skip Do not run CI builds (except noplatform) CI/skip-macos-x64 Do not run CI builds for macOS x64 labels Jun 21, 2019

jasonrsadler force-pushed the monthly_statement branch 2 times, most recently from 439da23 to 6ca927e Compare June 26, 2019 15:08

jasonrsadler force-pushed the monthly_statement branch 2 times, most recently from 762db26 to 9afe16b Compare July 2, 2019 21:24

NejcZdovc modified the milestones: 0.68.x - Dev, 0.69.x - Nightly Jul 8, 2019

jasonrsadler requested review from bridiver, kylehickinson and mihaiplesa as code owners July 8, 2019 16:19

jasonrsadler force-pushed the monthly_statement branch from edde833 to 63acf9e Compare July 9, 2019 11:11

jasonrsadler removed request for bridiver, kylehickinson and mihaiplesa July 9, 2019 11:11

jasonrsadler force-pushed the monthly_statement branch 4 times, most recently from 6cff332 to 700c2b0 Compare July 10, 2019 14:21

jasonrsadler removed CI/skip Do not run CI builds (except noplatform) CI/skip-macos-x64 Do not run CI builds for macOS x64 labels Jul 10, 2019

jasonrsadler force-pushed the monthly_statement branch 3 times, most recently from f07aa97 to 68bddc8 Compare July 13, 2019 20:17

jasonrsadler assigned kylehickinson Jul 13, 2019

jasonrsadler requested a review from kylehickinson July 13, 2019 20:18

jasonrsadler unassigned kylehickinson Jul 13, 2019

jasonrsadler force-pushed the monthly_statement branch 2 times, most recently from 4a1a87b to 073be5c Compare July 15, 2019 10:53

emerick reviewed Jul 15, 2019

View reviewed changes

jasonrsadler force-pushed the monthly_statement branch from 073be5c to 60a11b6 Compare July 15, 2019 15:25

jasonrsadler requested a review from emerick July 15, 2019 15:26

jasonrsadler force-pushed the monthly_statement branch 2 times, most recently from ab6d716 to 7ab14da Compare July 15, 2019 17:35

emerick reviewed Jul 15, 2019

View reviewed changes

bridiver reviewed Jul 15, 2019

View reviewed changes

bridiver reviewed Jul 16, 2019

View reviewed changes

jasonrsadler added the CI/skip Do not run CI builds (except noplatform) label Jul 17, 2019

jasonrsadler force-pushed the monthly_statement branch 2 times, most recently from 1680d5c to 17c2382 Compare July 18, 2019 02:08

bridiver reviewed Jul 18, 2019

View reviewed changes

jasonrsadler closed this Jul 22, 2019

jasonrsadler force-pushed the monthly_statement branch from 7fd60a4 to e3f81b0 Compare July 22, 2019 17:07

kylehickinson mentioned this pull request Oct 15, 2019

Add support for monthly statements brave/brave-ios#1669

Closed

bsclifton deleted the monthly_statement branch May 12, 2020 16:39

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Monthly statement can now be viewed from Rewards page #2733

Monthly statement can now be viewed from Rewards page #2733

jasonrsadler commented Jun 18, 2019 •

edited

Loading

jasonrsadler commented Jul 13, 2019

emerick Jul 15, 2019

jasonrsadler Jul 15, 2019

emerick Jul 15, 2019

jasonrsadler Jul 15, 2019

emerick Jul 15, 2019

emerick Jul 15, 2019

jasonrsadler Jul 15, 2019

emerick Jul 15, 2019

jasonrsadler Jul 15, 2019

emerick Jul 15, 2019

jasonrsadler Jul 15, 2019

bridiver Jul 15, 2019

jasonrsadler Jul 22, 2019

emerick Jul 15, 2019

jasonrsadler Jul 22, 2019

bridiver Jul 15, 2019

bridiver Jul 15, 2019

bridiver Jul 15, 2019

jasonrsadler Jul 22, 2019

bridiver Jul 15, 2019

bridiver Jul 16, 2019

bridiver Jul 16, 2019

bridiver Jul 18, 2019

bridiver Jul 18, 2019

bridiver Jul 18, 2019

bridiver Jul 18, 2019

Monthly statement can now be viewed from Rewards page #2733

Monthly statement can now be viewed from Rewards page #2733

Conversation

jasonrsadler commented Jun 18, 2019 • edited Loading

Submitter Checklist:

Test Plan:

Note

Reviewer Checklist:

After-merge Checklist:

Additional Notes

jasonrsadler commented Jul 13, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jasonrsadler commented Jun 18, 2019 •

edited

Loading