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.

Sign up for GitHub

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

Jump to bottom

rewire SaveRecurringTip through ledger #2962

Merged
merged 3 commits into from
Jul 27, 2019
Merged

rewire SaveRecurringTip through ledger #2962

merged 3 commits into from
Jul 27, 2019

Conversation

cg505
Copy link
Contributor

@cg505 cg505 commented Jul 19, 2019 •
edited by NejcZdovc
Loading

Resolves brave/brave-browser#5152.

@kylehickinson These iOS changes build, but I didn't actually build them into a full build. If it's straightforward to make sure this doesn't break anything, that would be good to test.

Submitter Checklist:

Test Plan:

  1. Enable rewards and go to a verified publisher.
  2. Add a montly tip for the publisher.
  3. Verify that you see the montly contribution in brave://rewards.
  4. To back the the publisher page and open the wallet panel. Change the tip amount.
  5. Verify that the new tip amount is show in brave://rewards.

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.

@cg505 cg505 added this to the 0.69.x - Nightly milestone Jul 19, 2019
@cg505 cg505 self-assigned this Jul 19, 2019
kylehickinson
kylehickinson requested changes Jul 19, 2019
View reviewed changes
vendor/brave-ios/Ledger/BATBraveLedger.mm Outdated
@@ -466,19 +466,20 @@ - (void)listRecurringTips:(void (NS_NOESCAPE ^)(NSArray<BATPublisherInfo *> *))c

- (void)addRecurringTipToPublisherWithId:(NSString *)publisherId amount:(double)amount
Copy link
Collaborator

@kylehickinson kylehickinson Jul 19, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

We should add a completion block here that is called in the SaveRecurringTip callback. suggested declaration:

- (void)addRecurringTipToPublisherWithId:(NSString *)publisherId amount:(double)amount completion:(void (^)(BOOL success))completion

Needs to updated in header as well

vendor/brave-ios/Ledger/BATBraveLedger.mm Outdated
info->value = amount;
info->date = [[NSDate date] timeIntervalSince1970];
ledger->SaveRecurringTip(std::move(info), ^(ledger::Result result){
if (result != ledger::Result::LEDGER_OK) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

completion block should be called above this line after adding it (seen in previous comment):

dispatch_async(dispatch_get_main_queue(), ^{
  completion(result == ledger::Result::LEDGER_OK)
});

Copy link
Collaborator

Choose a reason for hiding this comment

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

edit: you don't need the dispatch_async to main queue, the database insertion callbacks are done on main queue already

@NejcZdovc NejcZdovc force-pushed the save-recurring-tip-refactor branch from 16c2127 to 69bd936 Compare July 22, 2019 07:41
NejcZdovc
NejcZdovc previously approved these changes Jul 22, 2019
View reviewed changes
@cg505 cg505 force-pushed the save-recurring-tip-refactor branch 2 times, most recently from d63795c to 41cf16d Compare July 23, 2019 17:54
kylehickinson
kylehickinson previously approved these changes Jul 23, 2019
View reviewed changes
Copy link
Collaborator

@kylehickinson kylehickinson left a comment

Choose a reason for hiding this comment

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

ios++

@cg505 cg505 requested a review from NejcZdovc July 23, 2019 20:52
@cg505
Copy link
Contributor Author

cg505 commented Jul 23, 2019

tests pass, some CI issues only

@cg505 cg505 modified the milestones: 0.69.x - Dev, 0.70.x - Nightly Jul 23, 2019
@cg505 cg505 force-pushed the save-recurring-tip-refactor branch from 398da29 to 49c6098 Compare July 24, 2019 19:11
@cg505 cg505 requested a review from ryanml July 24, 2019 22:01
ryanml
ryanml previously approved these changes Jul 25, 2019
View reviewed changes
Copy link
Contributor

@ryanml ryanml left a comment •
edited
Loading

Choose a reason for hiding this comment

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

lgtm cc: @NejcZdovc

kylehickinson
kylehickinson previously approved these changes Jul 25, 2019
View reviewed changes
NejcZdovc
NejcZdovc reviewed Jul 25, 2019
View reviewed changes
components/brave_rewards/browser/rewards_service_impl.cc Outdated

void RewardsServiceImpl::OnRecurringTipSaved(
ledger::SaveRecurringTipCallback callback,
bool success) {
for (auto& observer : observers_) {
Copy link
Contributor

Choose a reason for hiding this comment

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

this observers should be in OnSaveRecurringTipUI

NejcZdovc
NejcZdovc reviewed Jul 25, 2019
View reviewed changes
components/brave_rewards/browser/rewards_service_impl.h Outdated
const int amount) override;
void OnSaveRecurringTipUI(
SaveRecurringTipCallback callback,
int32_t result);
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: const

NejcZdovc
NejcZdovc reviewed Jul 25, 2019
View reviewed changes
components/brave_rewards/browser/rewards_service_impl.h Outdated
void OnRecurringTipSaved(bool success);
void OnRecurringTipSaved(
ledger::SaveRecurringTipCallback callback,
bool success);
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: const

@cg505 cg505 dismissed stale reviews from kylehickinson and ryanml via ce7ae91 July 25, 2019 22:31
@cg505 cg505 force-pushed the save-recurring-tip-refactor branch 2 times, most recently from ce7ae91 to d80499f Compare July 26, 2019 00:17
@cg505 cg505 force-pushed the save-recurring-tip-refactor branch from d80499f to c3b54cf Compare July 26, 2019 21:34
@cg505 cg505 added CI/skip-android Do not run CI builds for Android CI/skip-ios Do not run CI builds for iOS CI/skip-linux CI/skip-macos-x64 Do not run CI builds for macOS x64 and removed CI/skip-android Do not run CI builds for Android CI/skip-ios Do not run CI builds for iOS CI/skip-linux CI/skip-macos-x64 Do not run CI builds for macOS x64 labels Jul 26, 2019
@cg505
Copy link
Contributor Author

cg505 commented Jul 26, 2019 •
edited
Loading

CI passes everything but Windows here: https://staging.ci.brave.com/job/brave-browser-build-pr/job/save-recurring-tip-refactor/9//flowGraphTable/ (windows failed due to a CI bug)
Triggerred another windows-only CI

kylehickinson
kylehickinson approved these changes Jul 26, 2019
View reviewed changes
Copy link
Collaborator

@kylehickinson kylehickinson left a comment

Choose a reason for hiding this comment

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

iOS++

@cg505 cg505 added CI/skip-android Do not run CI builds for Android CI/skip-ios Do not run CI builds for iOS CI/skip-linux CI/skip-macos-x64 Do not run CI builds for macOS x64 labels Jul 26, 2019
@NejcZdovc NejcZdovc removed CI/skip-android Do not run CI builds for Android CI/skip-ios Do not run CI builds for iOS CI/skip-linux CI/skip-macos-x64 Do not run CI builds for macOS x64 labels Jul 27, 2019
@NejcZdovc NejcZdovc merged commit 36f6230 into master Jul 27, 2019
@NejcZdovc NejcZdovc deleted the save-recurring-tip-refactor branch July 27, 2019 06:55
@NejcZdovc NejcZdovc mentioned this pull request Jul 27, 2019
Closed
@cg505
Copy link
Contributor Author

cg505 commented Jul 28, 2019

Windows CI pass for reference: https://staging.ci.brave.com/job/brave-browser-build-pr/job/save-recurring-tip-refactor/11//flowGraphTable/

@kylehickinson kylehickinson mentioned this pull request Jul 30, 2019
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@kylehickinson kylehickinson kylehickinson approved these changes

@NejcZdovc NejcZdovc NejcZdovc approved these changes

@ryanml ryanml ryanml left review comments

Assignees

@cg505 cg505

Labels
feature/rewards
Projects
None yet
Milestone
0.69.x - Release
Development

Successfully merging this pull request may close these issues.

Wire SaveRecurringTip through ledger
4 participants
@cg505 @kylehickinson @ryanml @NejcZdovc