Ledger notifications refactor and add new BTC=>BAT notification #11265
Conversation
js/about/preferences.js
Outdated
| * Parses a query string like: | ||
| * about:preferences#payments?ledgerBackupOverlayVisible | ||
| * and returns the part: | ||
| * `about:preferences#payments` |
There was a problem hiding this comment.
This comment doesn't seem right. this function would return 'payments' not 'about:preferences#payments'.
Codecov Report
@@ Coverage Diff @@
## master #11265 +/- ##
==========================================
+ Coverage 51.43% 52.12% +0.69%
==========================================
Files 263 263
Lines 24541 24617 +76
Branches 3928 3940 +12
==========================================
+ Hits 12623 12832 +209
+ Misses 11918 11785 -133
|
app/browser/api/ledger.js
Outdated
| if (getSetting(settings.PAYMENTS_ENABLED)) { | ||
| notifications.showEnabledNotifications(state) | ||
| } else { | ||
| notifications.showDisabledNotifications(state) |
There was a problem hiding this comment.
before we showed this notification only when !getSetting(settings.PAYMENTS_NOTIFICATIONS) is that change intentional?
There was a problem hiding this comment.
logic shouldn't have changed at all (just moved). If PAYMENT_NOTIFICATIONS is set, none of the notifications should be shown
There was a problem hiding this comment.
I guess I'm thinking about getSetting(settings.PAYMENTS_NOTIFICATIONS) is false and getSetting(settings.PAYMENTS_ENABLED) is false, before it would show.
There was a problem hiding this comment.
Great catch- I rolled back to the original behavior (so that there is no change from previous code)
| return | ||
| } | ||
|
|
||
| appActions.hideNotification(message) |
There was a problem hiding this comment.
maybe the above default: should be just break so this always runs?
There was a problem hiding this comment.
I considered that... but since other messages may be coming through (since this fires on ipc.on(messages.NOTIFICATION_RESPONSE) I didn't want to change the behavior
Fixes #11021 Presents this one-time "Converted BTC to BAT" alert: - if payments are enabled - user has a positive balance - this is an existing profile (new profiles will have firstRunTimestamp matching btcToBatTimestamp) - notification has not already been shown yet Includes a "learn more" link pointing to: https://brave.com/faq-payments/#brave-payments Has a possible security risk- the deep link allows URL hash to modify state (limited to boolean state variables on about:preferences) Auditors: @NejcZdovc, @diracdeltas Test Plan: `npm run unittest -- --grep="ledger api unit tests"` `npm run unittest -- --grep="ledgerReducer unit tests"` `npm run unittest -- --grep="Preferences component"`
|
@bsclifton This alert bar location looks wrong. The screen shot above shows the per-tab location and this should be at the app level. |
|
tracked here: #11256 |
|
also: #8934 |
Ledger notifications refactor and add new BTC=>BAT notification
Ledger notifications refactor and add new BTC=>BAT notification
Fixes #11021
Presents this one-time "Converted BTC to BAT" alert:
Includes a "learn more" link pointing to:
https://brave.com/faq-payments/#brave-payments
Has a possible security risk- the deep link allows URL hash to modify state (limited to boolean state variables on about:preferences)
Auditors: @NejcZdovc, @diracdeltas
Submitter Checklist:
git rebase -ito squash commits (if needed).Automated Test Plan:
npm run unittest -- --grep="ledger api unit tests"npm run unittest -- --grep="ledgerReducer unit tests"npm run unittest -- --grep="Preferences component"Reviewer Checklist:
Merge Checklist: