-
Notifications
You must be signed in to change notification settings - Fork 973
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This comment doesn't seem right. this function would return 'payments' not 'about:preferences#payments'.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good catch; fixed 😄
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
before we showed this notification only when !getSetting(settings.PAYMENTS_NOTIFICATIONS)
is that change intentional?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
maybe the above default: should be just break so this always runs?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
code looks good to me, I don't have positive balance, so can't test it fully. I agree with you that it would be nice to move notification into separate file
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 -i
to 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: