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

Create BAT notification service to deliver BAT-related notifications #581

Merged
merged 12 commits into from
Oct 16, 2018
Merged

Create BAT notification service to deliver BAT-related notifications #581

merged 12 commits into from
Oct 16, 2018

Conversation

emerick
Copy link
Contributor

@emerick emerick commented Oct 8, 2018 •
edited by NejcZdovc
Loading

Fixes brave/brave-browser#922

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:

  • clean profile
  • enable rewards
  • click claim on grant
  • click on BAT icon in the url bar
  • make sure that you see notification about grant
  • clicking on X should remove this notifiation

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 Oct 8, 2018
@emerick emerick requested a review from NejcZdovc October 8, 2018 19:36
emerick
emerick commented Oct 8, 2018
View reviewed changes
components/brave_rewards/browser/rewards_service_impl.cc Outdated
@@ -473,6 +478,8 @@ void RewardsServiceImpl::OnWalletProperties(ledger::Result result,
void RewardsServiceImpl::OnGrant(ledger::Result result,
const ledger::Grant& grant) {
TriggerOnGrant(result, grant);
bat_notifications_service_->AddNotification(
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This seemed to be the only place to add a notification like this so far, but let me know.

Copy link
Contributor

Choose a reason for hiding this comment

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

yes this is the only place for now

emerick
emerick commented Oct 8, 2018
View reviewed changes
common/extensions/api/bat_notifications.json Outdated
]
}
],
"functions": [
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Wasn't sure if all of these functions would be necessary? But added them in case, let me know.

@emerick emerick force-pushed the bat-notifications-service branch from 0e786b3 to ba86196 Compare October 9, 2018 22:48
@NejcZdovc NejcZdovc mentioned this pull request Oct 10, 2018
Closed
@NejcZdovc
Copy link
Contributor

@emerick can you please rename all files and functions from bat* to rewards*. For example bat_notifications_service_factory to rewards_notifications_service_factory. Thank you

@emerick emerick force-pushed the bat-notifications-service branch 8 times, most recently from 71ebea5 to 901583f Compare October 12, 2018 18:19
@emerick emerick changed the title WIP: Create BAT notification service to deliver BAT-related notifications Create BAT notification service to deliver BAT-related notifications Oct 12, 2018
@NejcZdovc NejcZdovc force-pushed the bat-notifications-service branch from 5516b4c to 545c115 Compare October 14, 2018 20:05
@emerick emerick force-pushed the bat-notifications-service branch from 25a486e to d97a6d2 Compare October 15, 2018 01:17
@NejcZdovc NejcZdovc force-pushed the bat-notifications-service branch from d97a6d2 to e3ba710 Compare October 15, 2018 12:37
NejcZdovc
NejcZdovc reviewed Oct 16, 2018
View reviewed changes
components/brave_rewards/browser/rewards_notifications_service.h Outdated
};

struct RewardsNotification {
RewardsNotification()
Copy link
Contributor

@NejcZdovc NejcZdovc Oct 16, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

we need to add args to RewardsNotification sturct as well, so that we save args as well to disk

@NejcZdovc NejcZdovc force-pushed the bat-notifications-service branch from e4f33f6 to 7504ea0 Compare October 16, 2018 10:22
jasonrsadler
jasonrsadler reviewed Oct 16, 2018
View reviewed changes
components/brave_rewards/extension/brave_rewards/_locales/en/messages.json
}
}
}
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: Add CRLF

package.json Outdated
@@ -94,7 +94,7 @@
"babel-preset-react": "^6.24.1",
"babel-preset-react-optimize": "^1.0.1",
"babel-preset-stage-0": "^6.24.1",
"brave-ui": "^0.26.0",
"brave-ui": "^0.27.0",
Copy link
Contributor

Choose a reason for hiding this comment

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

We're getting 28.x for free. This can be removed.

@jasonrsadler
Copy link
Contributor

@NejcZdovc please rebase

@NejcZdovc NejcZdovc force-pushed the bat-notifications-service branch from 399ee54 to 5cab204 Compare October 16, 2018 10:39
@NejcZdovc NejcZdovc merged commit 5af4edc into master Oct 16, 2018
@NejcZdovc NejcZdovc deleted the bat-notifications-service branch October 16, 2018 12:33
NejcZdovc added a commit that referenced this pull request Oct 16, 2018
@NejcZdovc
Merge pull request #581 from brave/bat-notifications-service
60af59a 
Create BAT notification service to deliver BAT-related notifications
@NejcZdovc
Copy link
Contributor

NejcZdovc commented Oct 16, 2018 •
edited
Loading

master 5af4edc
0.56.x 60af59a
0.55.x 8e079f2

NejcZdovc added a commit that referenced this pull request Oct 16, 2018
@NejcZdovc
Merge pull request #581 from brave/bat-notifications-service
8e079f2 
Create BAT notification service to deliver BAT-related notifications
@bridiver
Copy link
Collaborator

@emerick I think the title of this issue was misleading for the implementation Create BAT notification service - this really doesn't make sense as separate keyed service. I think this should be a RewardsServiceObserver that is owned by RewardsService and be access through RewardsServiceFactory::GetInstance()->GetForProfile(profile)->notification_service();

@emerick
Copy link
Contributor Author

emerick commented Oct 17, 2018

Yeah, agreed @bridiver. @bbondy and I discussed that the keyed service approach turned out to be overkill, but he suggested to go with it for now to make the release and then revisit as follow-on work. @bbondy want me to create a new issue for this

@bbondy
Copy link
Member

bbondy commented Oct 17, 2018

yep sounds good. Label with refactor label.

@emerick
Copy link
Contributor Author

emerick commented Oct 17, 2018

Sounds good, created brave/brave-browser#1679 to track this work.

@NejcZdovc NejcZdovc mentioned this pull request Oct 30, 2018
Merged
18 tasks
fmarier pushed a commit that referenced this pull request Oct 29, 2019
@mbacchi
Merge pull request #581 from brave/rename_linux_unstable
293bb72 
Rename unstable to dev in linux build output
@NejcZdovc NejcZdovc modified the milestones: Closed, 0.65.x - Release Mar 6, 2020
petemill pushed a commit that referenced this pull request Jul 27, 2020
@mbacchi
Merge pull request #581 from brave/rename_linux_unstable
bc63409 
Rename unstable to dev in linux build output
petemill pushed a commit that referenced this pull request Jul 28, 2020
@mbacchi
Merge pull request #581 from brave/rename_linux_unstable
badd61f 
Rename unstable to dev in linux build output
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jasonrsadler jasonrsadler jasonrsadler left review comments

@NejcZdovc NejcZdovc NejcZdovc approved these changes

Assignees

@emerick emerick

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

Successfully merging this pull request may close these issues.

Notification service
5 participants
@emerick @NejcZdovc @jasonrsadler @bridiver @bbondy