-
Notifications
You must be signed in to change notification settings - Fork 2.8k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Trim the value when going back on connect bank account #28305
Changes from 27 commits
183e3fe
99477df
04e7887
161abe5
15ef37f
883db65
10f72f8
0fa5251
4532a36
3c4be81
936289c
88fc114
25c2366
6caa796
270eddf
12f5f05
a58a10e
f2b02f1
6139ff2
96bf07d
0de5e86
b4bfd76
1c1dd14
401e05d
cc35473
0314480
abb6b20
bec41b8
4e75345
c0e74b9
0f48962
8d692b0
26e9552
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -57,10 +57,10 @@ function clearOnfidoToken() { | |
|
||
/** | ||
* Helper method to build the Onyx data required during setup of a Verified Business Bank Account | ||
* | ||
* @param {String | undefined} currentStep The step that we need to update the data from BE to draft value | ||
* @returns {Object} | ||
*/ | ||
function getVBBADataForOnyx() { | ||
function getVBBADataForOnyx(currentStep = undefined) { | ||
return { | ||
optimisticData: [ | ||
{ | ||
|
@@ -79,6 +79,7 @@ function getVBBADataForOnyx() { | |
value: { | ||
isLoading: false, | ||
errors: null, | ||
stepToUpdateToDraft: currentStep, | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. So why is this called There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We already have a field named There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Right... maybe There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Updated. |
||
}, | ||
}, | ||
], | ||
|
@@ -222,7 +223,7 @@ function deletePaymentBankAccount(bankAccountID) { | |
* @param {Boolean} [params.isOnfidoSetupComplete] | ||
*/ | ||
function updatePersonalInformationForBankAccount(params) { | ||
API.write('UpdatePersonalInformationForBankAccount', params, getVBBADataForOnyx()); | ||
API.write('UpdatePersonalInformationForBankAccount', params, getVBBADataForOnyx(CONST.BANK_ACCOUNT.STEP.REQUESTOR)); | ||
} | ||
|
||
/** | ||
|
@@ -339,7 +340,7 @@ function openReimbursementAccountPage(stepToOpen, subStep, localCurrentStep) { | |
* @param {String} policyID | ||
*/ | ||
function updateCompanyInformationForBankAccount(bankAccount, policyID) { | ||
API.write('UpdateCompanyInformationForBankAccount', {...bankAccount, policyID}, getVBBADataForOnyx()); | ||
API.write('UpdateCompanyInformationForBankAccount', {...bankAccount, policyID}, getVBBADataForOnyx(CONST.BANK_ACCOUNT.STEP.COMPANY)); | ||
} | ||
|
||
/** | ||
|
@@ -376,7 +377,7 @@ function connectBankAccountManually(bankAccountID, accountNumber, routingNumber, | |
routingNumber, | ||
plaidMask, | ||
}, | ||
getVBBADataForOnyx(), | ||
getVBBADataForOnyx(CONST.BANK_ACCOUNT.STEP.BANK_ACCOUNT), | ||
); | ||
} | ||
|
||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -157,6 +157,12 @@ class ReimbursementAccountPage extends React.Component { | |
return; | ||
} | ||
|
||
// Update the data that is returned from back-end to draft value | ||
const stepToUpdateToDraft = this.props.reimbursementAccount.stepToUpdateToDraft; | ||
if (stepToUpdateToDraft) { | ||
BankAccounts.updateReimbursementAccountDraft(this.getBankAccountFields(this.getFieldsOfCurrentStep(stepToUpdateToDraft))); | ||
} | ||
|
||
const currentStepRouteParam = this.getStepToOpenFromRouteParams(); | ||
if (currentStepRouteParam === currentStep) { | ||
// The route is showing the correct step, no need to update the route param or clear errors. | ||
|
@@ -177,6 +183,42 @@ class ReimbursementAccountPage extends React.Component { | |
Navigation.navigate(ROUTES.BANK_ACCOUNT_WITH_STEP_TO_OPEN.getRoute(this.getRouteForCurrentStep(currentStep), policyId, backTo)); | ||
} | ||
|
||
getFieldsOfCurrentStep(currentStep) { | ||
dukenv0307 marked this conversation as resolved.
Show resolved
Hide resolved
|
||
switch (currentStep) { | ||
case CONST.BANK_ACCOUNT.STEP.BANK_ACCOUNT: | ||
return ['routingNumber', 'accountNumber', 'bankName', 'plaidAccountID', 'plaidAccessToken', 'isSavings']; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It feels like we could be repeating ourselves a bit here, as if someone wanted to add/remove a field they would have to update this list too. Is there any way we could reference the fields in one place? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @Ollyws Here are the list values of the step and this is only called after api update success. If we want to update the change field to draft we will need to compare draft value and There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @dukenv0307 What I mean is, in RequestorStep for example we already have a list of input IDs could we perhaps use this list instead of creating a new list here, and implement something similar for the other steps? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The list here is the list that I copied in each step. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Should we create a const for the list and re-use this. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I just updated it. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @dukenv0307 Thanks but I was specifically referring to this list of input IDs, which we could then perhaps get the values of using There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @Ollyws I see that we only use this in the special form like There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ok well if we're not going to implement that then I don't really see the need for a const, just to be used for the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Updated. |
||
case CONST.BANK_ACCOUNT.STEP.COMPANY: | ||
return [ | ||
'companyName', | ||
'addressStreet', | ||
'addressZipCode', | ||
'addressCity', | ||
'addressState', | ||
'companyPhone', | ||
'website', | ||
'companyTaxID', | ||
'incorporationType', | ||
'incorporationDate', | ||
'incorporationState', | ||
]; | ||
case CONST.BANK_ACCOUNT.STEP.REQUESTOR: | ||
return ['firstName', 'lastName', 'dob', 'ssnLast4', 'requestorAddressStreet', 'requestorAddressCity', 'requestorAddressState', 'requestorAddressZipCode']; | ||
default: | ||
return []; | ||
} | ||
} | ||
|
||
/** | ||
* @param {Array} fieldNames | ||
* | ||
* @returns {*} | ||
*/ | ||
getBankAccountFields(fieldNames) { | ||
return { | ||
..._.pick(lodashGet(this.props.reimbursementAccount, 'achData'), ...fieldNames), | ||
}; | ||
} | ||
|
||
/* | ||
* Calculates the state used to show the "Continue with setup" view. If a bank account setup is already in progress and | ||
* no specific further step was passed in the url we'll show the workspace bank account reset modal if the user wishes to start over | ||
|
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.
Is this what it means? If yes, let's update it to use the full word, I don't find it necessary to save a few characters. And if yes, I honestly don't really understand the sentence, perhaps you can try rephrasing it?
Also, you did not clarify what happens if you pass
undefined
?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 think something like
The name of the step for which we will update the draft value when data has been sucessfully submitted to the backend.
would be clearer.Undefined here is just for the default value
currentStep = undefined
as far as I'm aware, in which case the draft will not be updated as this condition will be undefined.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.
Agreed, I do understand this better. Also, can we say "API" instead of "backend", since that's what we use everywhere?