This repository has been archived by the owner on Dec 11, 2019. It is now read-only.
Limit 24 hour payment notification to once per reconcile period #5296
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
bsclifton
reviewed
Oct 31, 2016
| @@ -445,6 +445,7 @@ WindowStore | |||
| ledgerInfo: { | |||
| creating: boolean, // wallet is being created | |||
| created: boolean, // wallet is created | |||
| reconcilePeriod: number, // duration of each reconciliation period in days | |||
There was a problem hiding this comment.
Totally a nitpick- would it be better to say frequency? like the reconcileFrequency would be every 30 days. When I see period or duration, it's not immediately clear
Fix #4481 Auditors: @bsclifton Test Plan: 1. Trigger the "Payment in 24 hours, please review" notification. - Update reconcileStamp to <24 hours from now - Have sufficient funds; OR disable the sufficient funds conditional by adding false to ledger.js L1485 (showEnabledNotifications()) - Change startup notification delay in ledger.js L484 from 15m to 5s 2. Open Brave and observe 24h review notification 3. Close Brave and reopen. Notification should not reappear. (Next 24h notification timestamp is set in Application Support/brave-development/session-store-1)
ayumi
force-pushed
the
fix/calm-ledger-notifications
branch
from
c4d20f6
to
addfd70
Compare
bsclifton
reviewed
Oct 31, 2016
| @@ -445,6 +445,7 @@ WindowStore | |||
| ledgerInfo: { | |||
| creating: boolean, // wallet is being created | |||
| created: boolean, // wallet is created | |||
| reconcileFrequency: number, // duration between each reconciliation in days | |||
bsclifton
reviewed
Oct 31, 2016
| @@ -119,6 +119,9 @@ module.exports = { | |||
| 'bookmarks.toolbar.showOnlyFavicon': false, | |||
| 'payments.enabled': false, | |||
| 'payments.notifications': false, | |||
| // "Out of money, pls add" / "In 24h we'll pay publishers [Review]" | |||
| // After shown, set timestamp to next reconcile time - 1 day. | |||
| 'payments.notification-reconcile-soon-timestamp': null, | |||
| 'payments.notificationTryPaymentsDismissed': false, // True if you dismiss the message or enable Payments | |||
There was a problem hiding this comment.
This might be more of a @bradleyrichter question... but I wonder if this should this be covering both cases?
Let's say you're within 24 hours of reconciling and you only have the ~5 dollars in your account. You'd dismiss the nag window but then you're out of budget after reconciliation happens. This is a case where you might want to get nagged again sometime before the next reconciliation period (especially since getting funds in might take > 24 hours)
(if action needed, can be done in a follow up 😄)
|
can i get @ayumi or @bsclifton to merge this... |
|
Will merge in a sec... just manually testing it now... |
|
Looks great! 👍 |
This was referenced Nov 2, 2016
ayumi
added a commit
that referenced
this pull request
Nov 7, 2016
Because you should always be able to dismiss a notification. Auditors: @bsclifton Test Plan: (Similar to plan for #5296) 1. Trigger the "Payment in 24 hours, please review" notification. a. Update reconcileStamp to <24 hours from now b. Have sufficient funds; OR disable the sufficient funds conditional by adding false to ledger.js L1485 (showEnabledNotifications()) c. Change startup notification delay in ledger.js L484 from 15m to 5s 2. Open Brave and observe 24h review notification 3. Close Brave and reopen. Notification should not reappear. (Next 24h notification timestamp is set in Application Support/brave-development/session-store-1)
bsclifton
pushed a commit
that referenced
this pull request
Nov 8, 2016
Because you should always be able to dismiss a notification. Auditors: @bsclifton Test Plan: (Similar to plan for #5296) 1. Trigger the "Payment in 24 hours, please review" notification. a. Update reconcileStamp to <24 hours from now b. Have sufficient funds; OR disable the sufficient funds conditional by adding false to ledger.js L1485 (showEnabledNotifications()) c. Change startup notification delay in ledger.js L484 from 15m to 5s 2. Open Brave and observe 24h review notification 3. Close Brave and reopen. Notification should not reappear. (Next 24h notification timestamp is set in Application Support/brave-development/session-store-1)
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Fix #4481
Auditors: @bsclifton
Test Plan:
(Next 24h notification timestamp is set in Application Support/brave-development/session-store-1)