Skip to content

/ brave-core

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

Send pending rewards notifications after registering listener #1436

Merged
merged 1 commit into from Jan 25, 2019
Merged

Send pending rewards notifications after registering listener #1436

merged 1 commit into from Jan 25, 2019

Conversation

@emerick
Copy link
Contributor

emerick commented Jan 24, 2019 •
edited

Fixes brave/brave-browser#3078

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:

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
@emerick emerick self-assigned this Jan 24, 2019
@emerick emerick requested review from bridiver and ryanml Jan 24, 2019
@bridiver
bridiver reviewed Jan 24, 2019
View changes
components/brave_rewards/browser/rewards_notification_service_impl.cc Outdated
@@ -34,6 +34,9 @@ RewardsNotificationServiceImpl::RewardsNotificationServiceImpl(Profile* profile)
std::make_unique<ExtensionRewardsNotificationServiceObserver>(
profile);
AddObserver(extension_rewards_notification_service_observer_.get());
for (auto& notification : rewards_notifications_) {
TriggerOnNotificationAdded(notification.second);

This comment has been minimized.

Sign in to view
Copy link
@bridiver

bridiver Jan 24, 2019

Collaborator

I don't think we want to re-trigger for all observers, that might cause duplicates, but either way this isn't the right place to do this. We should be doing it in the AddObserver method. This should apply to all observers, not just the extension_rewards_notification_service_observer

This comment has been minimized.

Sign in to view
Copy link
@emerick

emerick Jan 24, 2019

Author Contributor

OK, I see now. The AddObserver method doesn't currently have access to the pending notifications, so I'll have to plumb those through. I'm thinking to just add a getter on the notification service, unless you have a better idea there.
@emerick emerick force-pushed the send-pending-rewards-notifications branch from ea1e072 to 6ecaf27 Jan 24, 2019
@bridiver
bridiver reviewed Jan 24, 2019
View changes
components/brave_rewards/browser/rewards_notification_service_impl.cc Outdated
@@ -110,6 +110,13 @@ void RewardsNotificationServiceImpl::GetAllNotifications() {
OnGetAllNotifications(rewards_notifications_list);
}

void RewardsNotificationServiceImpl::GetAllNotifications(

This comment has been minimized.

Sign in to view
Copy link
@bridiver

bridiver Jan 24, 2019

Collaborator

you should use * instead of & when initializing a parameter like this for clarity, but it doesn't seem necessary either way. Why wouldn't we just return a const RewardsNotificationList for rewards_notifications_? Also the method itself should be const

This comment has been minimized.

Sign in to view
Copy link
@bridiver

bridiver Jan 24, 2019

Collaborator

actually why do we need this method at all? We really don't need the _impl.cc class here anyway, but just put rewards_notifications_ in rewards_notification_service.h
@emerick emerick force-pushed the send-pending-rewards-notifications branch from 6ecaf27 to 7e0bbba Jan 24, 2019
@emerick
Send pending rewards notifications after registering listener
Loading status checks…
2fa44ba
@emerick emerick force-pushed the send-pending-rewards-notifications branch from 7e0bbba to 2fa44ba Jan 24, 2019
@bridiver
bridiver approved these changes Jan 25, 2019
View changes
@bridiver bridiver merged commit afd51a3 into master Jan 25, 2019
2 checks passed
2 checks passed
continuous-integration/jenkins/pr-head This commit looks good
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@bridiver bridiver deleted the send-pending-rewards-notifications branch Jan 25, 2019
@NejcZdovc NejcZdovc added this to the 0.62.x - Nightly milestone Jan 25, 2019
@NejcZdovc NejcZdovc removed the bug label Jan 25, 2019
This was referenced Jan 28, 2019
Merged
Merged
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bridiver bridiver

@ryanml ryanml

Assignees

@emerick emerick

Labels
feature/rewards
Projects
None yet
Milestone
  0.62.x - Release
Linked issues

Successfully merging this pull request may close these issues.

Send any pending rewards notifications after registering listener
3 participants
@emerick @bridiver @NejcZdovc
You can’t perform that action at this time.