validation/microdeposits: add a config for withdrawing from savings accounts #551
Conversation
Codecov Report
@@ Coverage Diff @@
## master #551 +/- ##
==========================================
- Coverage 54.02% 53.21% -0.82%
==========================================
Files 102 102
Lines 3815 3845 +30
==========================================
- Hits 2061 2046 -15
- Misses 1384 1433 +49
+ Partials 370 366 -4
Continue to review full report at Codecov.
|
adamdecaf
force-pushed
the
config-withdraw-savings-account
branch
2 times, most recently
from
af1711d to
8f871b7
Compare
| @@ -57,6 +59,11 @@ func createMicroDeposits( | |||
| micro.TransferIDs = append(micro.TransferIDs, xfer.TransferID) | |||
| } | |||
|
|
|||
| // skip the debit for Savings accounts when configured to do so | |||
| if skipDebit(cfg.Withdraw, dest.Account) { | |||
There was a problem hiding this comment.
I suggest removing skipDebit and being explicit:
if !cfg.Withdraw.FromSavingsAccount && dest.Account.Type != customers.Savings {
return micro, nil
}To figure out what skipDebit does I had to look into its implementation, and customers.Savings is baked into the implementation.
I'd suggest adding the helper method once another use case (e.g. Checking) pops up and you need to reuse the logic.
There was a problem hiding this comment.
I'm hoping my bool logic is correct here, SKIP the debit from Savings if both match:
- Not configured to withdraw from savings,
FromSavingsAccountis false - account type is not
Savings
There was a problem hiding this comment.
I would rework this part as you suggest @vxio. We already discussed it with @adamdecaf while ago as it was not clear for me as well. after our change it seems still unclear.
In code we should read it as this:
if (we should not withdraw from savings account && current account is savings)
what are the options to name this "we should not withdraw from savings account"?
- config.Withdraw.NotFromSavings
- config.Withdraw.SkipForSavings
- ...?
There was a problem hiding this comment.
config.Withdraw.FromSavingsAccount is ok since it defaults to false like @adamdecaf mentioned.
Something like config.Withdraw.IsSavingsActive could also work.
There was a problem hiding this comment.
What if we renamed skipDebit to skipDebitFromSavings?
There was a problem hiding this comment.
@adamdecaf maybe we can rename config option into skipDebitFromSavings to simplify if statement to this:
if cfg.Withdraw.SkipDebitFromSavings && dest.Account.Type == customers.Savings {
...
}There was a problem hiding this comment.
The default behavior needs to skip debits from savings accounts, so skipDebitFromSavings bool would have a default of false - that wouldn't skip debits and so I'm opposed to that naming.
adamdecaf
force-pushed
the
master
branch
2 times, most recently
from
fb477cf to
effcefb
Compare
Fixes: #541