Reset old reconcileStamp when re-enabling Payments #4314
Conversation
| if (reason === 'changeSettingPaymentsEnabled') { | ||
| let timeUntilReconcile = client.timeUntilReconcile() | ||
| if (typeof timeUntilReconcile === 'number' && timeUntilReconcile < 0) { | ||
| let futureReconcileStamp = underscore.now() + 30 * msecs.day |
There was a problem hiding this comment.
it is better to call setTimeUntilReconcile with a null timestamp and let the ledger-client package figure out the next time.
| let timeUntilReconcile = client.timeUntilReconcile() | ||
| if (typeof timeUntilReconcile === 'number' && timeUntilReconcile < 0) { | ||
| let futureReconcileStamp = underscore.now() + 30 * msecs.day | ||
| client.setTimeUntilReconcile(futureReconcileStamp, callback) |
There was a problem hiding this comment.
you shouldn't use callback here, since that's used for client.sync ... look at how the call to setBraveryProperties does it!
There was a problem hiding this comment.
thanks, i've updated it
| if (action.key === settings.PAYMENTS_CONTRIBUTION_AMOUNT) return setPaymentInfo(action.value) | ||
| switch (action.key) { | ||
| case settings.PAYMENTS_ENABLED: | ||
| return initialize(action.value, 'changeSettingPaymentsEnabled') |
There was a problem hiding this comment.
probably better to get rid of return and add a break line -- that would be consistent with everything else in the function.
| case settings.PAYMENTS_CONTRIBUTION_AMOUNT: | ||
| setPaymentInfo(action.value) | ||
| break | ||
| default: |
There was a problem hiding this comment.
thank you for adding the default case!
| try { | ||
| client = ledgerClient(state.personaId, | ||
| underscore.extend(state.options, { roundtrip: roundtrip }, clientOptions), state) | ||
| client = ledgerClient( |
There was a problem hiding this comment.
why did the indentation change?
i agree that line 446 is indented too much, that must have been a slip of the finger. i thought it would be:
client = ledgerClient(state.personaId,
underscore.extend(state.options, { roundtrip: roundtrip }, clientOptions), state)
There was a problem hiding this comment.
I think it's clearest to indent the 3 parameters equally
There was a problem hiding this comment.
except that this is the only example of that style of indentation in the file...
perhaps we can compromise by putting the first parameter right after the "(", and the ")" right after the final parameter?
Fix #4058 Auditors: @mrose17 Test Plan: 1. Open Brave 2. Enable Payments then disable 2. Close Brave 3. Edit {userData}/ledger-state.json `reconcileStamp` to be way in the past (change 14.. to 13..) 4. Open Brave 5. Enable Payments 6. Note that contribution date is today + 30 days, rather than way in the past
ayumi
force-pushed
the
fix/ledger-reset-old-reconcileStamp
branch
from
43c4d5d
to
8f3d16f
Compare
|
mrose said it could be merged now, thanks. |
Fix #4058
Auditors: @mrose17
Test Plan:
reconcileStampto be way in the past (change 14.. to 13..)