Skip to content
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.

Sign up for GitHub

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

[Form Refactor] ACHContractStep #13501

Merged
merged 33 commits into from
Feb 1, 2023
Merged

[Form Refactor] ACHContractStep #13501

Changes from 8 commits
Commits
Show all changes
33 commits
Select commit Hold shift + click to select a range
05d3bb2
Replace ReimbursementAccountForm with Form
grgia Oct 19, 2022
4b089ce
Create ACHContractForm Onyx key and use it
grgia Oct 19, 2022
797e578
Clean up inputs
grgia Oct 19, 2022
780212a
Merge branch 'main' into georgia-ACH-form
grgia Nov 16, 2022
bee453f
Merge branch 'main' into georgia-ACH-form
grgia Nov 29, 2022
6abfb7e
New commit
grgia Dec 10, 2022
cb0fff7
Merge branch 'georgia-ACH-form' of github.com:Expensify/App into geor…
grgia Dec 10, 2022
292e3f3
Use corrext ONYX Form key, Use ReimbursementAccountUtils.getDefaultSt…
grgia Dec 12, 2022
57c61d3
Use dynamic keys for IdentityForm
grgia Dec 13, 2022
b816f5b
Remove consoles, reformat data sent on submit
grgia Dec 13, 2022
382b75a
Merge branch 'main' into georgia-ACH-form
grgia Dec 15, 2022
a379d39
Clean up code and remove unused ReimbursementAccountUtils
grgia Dec 15, 2022
3ad4f46
Set to empty array if hasOtherBeneficialOwners is checked
grgia Dec 15, 2022
fbe7e15
Fix bug where 4 forms are submitted even if requester owns 25%
grgia Dec 15, 2022
a36e8a8
Fix bug where checking the beneifial owners check doesn't open an ide…
grgia Dec 15, 2022
6a32616
Fix > 18 years old check
grgia Dec 16, 2022
49098db
Update comment to be more descriptive
grgia Dec 16, 2022
3f5c1cc
Fix long line
grgia Dec 16, 2022
ea35dc3
Merge branch 'main' into georgia-ACH-form
grgia Dec 16, 2022
b67bd24
Rename ownerID variable to ownerKey, add JSDocs and comments
grgia Dec 16, 2022
efed1c3
Use for loop for requiredFulfilled() calls
grgia Dec 19, 2022
8d167d8
Use Str.guid to generate keys, add section to FORMS.md
grgia Dec 22, 2022
ccb589b
Fix param type, use variable name instead of value for clarity
grgia Dec 23, 2022
d5085c6
Replace '.' with '_' for key to not be confused with dot notation
grgia Dec 23, 2022
45c97c1
Merge branch 'main' into georgia-ACH-form
grgia Jan 9, 2023
853a4f9
pass props from forms
grgia Jan 13, 2023
d4a95a0
Log change
grgia Jan 26, 2023
d27dc93
Use default values / draft values for initial render of form
grgia Jan 26, 2023
aee6c38
Merge branch 'main' into georgia-ACH-form
grgia Jan 31, 2023
a63e322
fix JS console errors
grgia Jan 31, 2023
5937ed9
Remove unused imports
grgia Jan 31, 2023
0df7ec4
use props instead of this.props
grgia Jan 31, 2023
27a6f27
Move proptype to single line
grgia Jan 31, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
162 changes: 89 additions & 73 deletions src/pages/ReimbursementAccount/ACHContractStep.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,7 @@ import ONYXKEYS from '../../ONYXKEYS';
import compose from '../../libs/compose';
import * as ReimbursementAccountUtils from '../../libs/ReimbursementAccountUtils';
import reimbursementAccountPropTypes from './reimbursementAccountPropTypes';
import ReimbursementAccountForm from './ReimbursementAccountForm';
import Form from '../../components/Form';

const propTypes = {
/** Name of the company */
Expand All @@ -36,55 +36,75 @@ const propTypes = {
class ACHContractStep extends React.Component {
constructor(props) {
super(props);
this.validate = this.validate.bind(this);

this.addBeneficialOwner = this.addBeneficialOwner.bind(this);
this.submit = this.submit.bind(this);

this.state = {
ownsMoreThan25Percent: ReimbursementAccountUtils.getDefaultStateForField(props, 'ownsMoreThan25Percent', false),
hasOtherBeneficialOwners: ReimbursementAccountUtils.getDefaultStateForField(props, 'hasOtherBeneficialOwners', false),
acceptTermsAndConditions: ReimbursementAccountUtils.getDefaultStateForField(props, 'acceptTermsAndConditions', false),
certifyTrueInformation: ReimbursementAccountUtils.getDefaultStateForField(props, 'certifyTrueInformation', false),
beneficialOwners: ReimbursementAccountUtils.getDefaultStateForField(props, 'beneficialOwners', []),
grgia marked this conversation as resolved.
Show resolved Hide resolved
};

// These fields need to be filled out in order to submit the form (doesn't include IdentityForm fields)
this.requiredFields = [
'acceptTermsAndConditions',
'certifyTrueInformation',
];

// Map a field to the key of the error's translation
this.errorTranslationKeys = {
acceptTermsAndConditions: 'common.error.acceptedTerms',
certifyTrueInformation: 'beneficialOwnersStep.error.certify',
};

this.getErrors = () => ReimbursementAccountUtils.getErrors(this.props);
luacmartins marked this conversation as resolved.
Show resolved Hide resolved
this.clearError = inputKey => ReimbursementAccountUtils.clearError(this.props, inputKey);
this.clearErrors = inputKeys => ReimbursementAccountUtils.clearErrors(this.props, inputKeys);
this.getErrorText = inputKey => ReimbursementAccountUtils.getErrorText(this.props, this.errorTranslationKeys, inputKey);
}

/**
* @returns {Boolean}
* @param {Object} values
luacmartins marked this conversation as resolved.
Show resolved Hide resolved
* @returns {Object}
*/
validate() {
let beneficialOwnersErrors = [];
if (this.state.hasOtherBeneficialOwners) {
beneficialOwnersErrors = _.map(this.state.beneficialOwners, ValidationUtils.validateIdentity);
}

validate(values) {
const errors = {};
_.each(this.requiredFields, (inputKey) => {
if (ValidationUtils.isRequiredFulfilled(this.state[inputKey])) {
return;

_.each(values.beneficialOwners, (beneficialOwner, index) => {
if (!ValidationUtils.isRequiredFulfilled(beneficialOwner.firstName)) {
errors[`beneficialOwner${index}`] = this.props.translate('bankAccount.error.firstName');
}

if (!ValidationUtils.isRequiredFulfilled(beneficialOwner.lastName)) {
errors[`beneficialOwner${index}`] = this.props.translate('bankAccount.error.lastName');
}

if (!ValidationUtils.isRequiredFulfilled(beneficialOwner.dob)) {
errors[`beneficialOwner${index}`] = this.props.translate('bankAccount.error.dob');
}

if (values.dob && !ValidationUtils.meetsAgeRequirements(values.dob)) {
errors[`beneficialOwner${index}`] = this.props.translate('bankAccount.error.age');
}

errors[inputKey] = true;
if (!ValidationUtils.isRequiredFulfilled(values.ssnLast4) || !ValidationUtils.isValidSSNLastFour(values.ssnLast4)) {
errors[`beneficialOwner${index}`] = this.props.translate('bankAccount.error.ssnLast4');
}

if (!ValidationUtils.isRequiredFulfilled(beneficialOwner.beneficialOwnerAddressStreet)) {
errors[`beneficialOwner${index}`] = this.props.translate('bankAccount.error.address');
}

if (values.beneficialOwnerAddressStreet && !ValidationUtils.isValidAddress(beneficialOwner.beneficialOwnerAddressStreet)) {
errors[`beneficialOwner${index}`] = this.props.translate('bankAccount.error.addressStreet');
}

if (!ValidationUtils.isRequiredFulfilled(beneficialOwner.beneficialOwnerAddressCity)) {
errors[`beneficialOwner${index}`] = this.props.translate('bankAccount.error.addressCity');
}

if (!ValidationUtils.isRequiredFulfilled(beneficialOwner.beneficialOwnerAddressState)) {
errors[`beneficialOwner${index}`] = this.props.translate('bankAccount.error.addressState');
}

if (!ValidationUtils.isRequiredFulfilled(beneficialOwner.beneficialOwnerAddressZipCode) || !ValidationUtils.isValidZipCode(values.beneficialOwnerAddressZipCode)) {
errors[`beneficialOwner${index}`] = this.props.translate('bankAccount.error.zipCode');
}
});
BankAccounts.setBankAccountFormValidationErrors({...errors, beneficialOwnersErrors});
return _.every(beneficialOwnersErrors, _.isEmpty) && _.isEmpty(errors);

if (!ValidationUtils.isRequiredFulfilled(values.acceptTermsAndConditions)) {
errors.acceptTermsAndConditions = this.props.translate('common.error.acceptedTerms');
}

if (!ValidationUtils.isRequiredFulfilled(values.certifyTrueInformation)) {
errors.certifyTrueInformation = this.props.translate('beneficialOwnersStep.error.certify');
}
return errors;
}

removeBeneficialOwner(beneficialOwner) {
Expand Down Expand Up @@ -138,8 +158,8 @@ class ACHContractStep extends React.Component {
this.clearErrors(_.map(inputKeys, inputKey => `beneficialOwnersErrors.${ownerIndex}.${inputKey}`));
}

submit() {
if (!this.validate()) {
submit(values) {
grgia marked this conversation as resolved.
Show resolved Hide resolved
if (!this.validate(values)) {
return;
}

Expand All @@ -164,7 +184,6 @@ class ACHContractStep extends React.Component {
BankAccounts.updateReimbursementAccountDraft(newState);
grgia marked this conversation as resolved.
Show resolved Hide resolved
return newState;
});
this.clearError(fieldName);
}

render() {
Expand All @@ -182,46 +201,37 @@ class ACHContractStep extends React.Component {
guidesCallTaskID={CONST.GUIDES_CALL_TASK_IDS.WORKSPACE_BANK_ACCOUNT}
shouldShowBackButton
/>
<ReimbursementAccountForm
reimbursementAccount={this.props.reimbursementAccount}
<Form
formID={ONYXKEYS.FORMS.REIMBURSEMENT_ACCOUNT_FORM}
validate={this.validate}
onSubmit={this.submit}
submitButtonText={this.props.translate('common.saveAndContinue')}
style={[styles.mh5, styles.flexGrow1]}
>
<Text style={[styles.mb5]}>
<Text>{this.props.translate('beneficialOwnersStep.checkAllThatApply')}</Text>
</Text>
<CheckboxWithLabel
inputID="ownsMoreThan25Percent"
style={[styles.mb2]}
isChecked={this.state.ownsMoreThan25Percent}
onInputChange={() => this.toggleCheckbox('ownsMoreThan25Percent')}
LabelComponent={() => (
<Text>
{this.props.translate('beneficialOwnersStep.iOwnMoreThan25Percent')}
<Text style={[styles.textStrong]}>{this.props.companyName}</Text>
</Text>
)}
shouldSaveDraft
/>
<CheckboxWithLabel
inputID="hasOtherBeneficialOwners"
style={[styles.mb2]}
isChecked={this.state.hasOtherBeneficialOwners}
onInputChange={() => {
this.setState((prevState) => {
const hasOtherBeneficialOwners = !prevState.hasOtherBeneficialOwners;
const newState = {
hasOtherBeneficialOwners,
beneficialOwners: hasOtherBeneficialOwners && _.isEmpty(prevState.beneficialOwners)
? [{}]
: prevState.beneficialOwners,
};
BankAccounts.updateReimbursementAccountDraft(newState);
return newState;
});
}}
grgia marked this conversation as resolved.
Show resolved Hide resolved
LabelComponent={() => (
<Text>
{this.props.translate('beneficialOwnersStep.someoneOwnsMoreThan25Percent')}
<Text style={[styles.textStrong]}>{this.props.companyName}</Text>
</Text>
)}
shouldSaveDraft
/>
{this.state.hasOtherBeneficialOwners && (
<View style={[styles.mb2]}>
Expand All @@ -233,18 +243,27 @@ class ACHContractStep extends React.Component {
<IdentityForm
translate={this.props.translate}
style={[styles.mb2]}
onFieldChange={values => this.clearErrorAndSetBeneficialOwnerValues(index, values)}
values={{
firstName: owner.firstName || '',
lastName: owner.lastName || '',
street: owner.street || '',
city: owner.city || '',
state: owner.state || '',
zipCode: owner.zipCode || '',
dob: owner.dob || '',
ssnLast4: owner.ssnLast4 || '',
defaultValues={{
firstName: ReimbursementAccountUtils.getDefaultStateForField(this.props, 'firstName'),
lastName: ReimbursementAccountUtils.getDefaultStateForField(this.props, 'lastName'),
street: ReimbursementAccountUtils.getDefaultStateForField(this.props, 'beneficialOwnerAddressStreet'),
city: ReimbursementAccountUtils.getDefaultStateForField(this.props, 'beneficialOwnerAddressCity'),
state: ReimbursementAccountUtils.getDefaultStateForField(this.props, 'beneficialOwnerAddressState'),
zipCode: ReimbursementAccountUtils.getDefaultStateForField(this.props, 'beneficialOwnerAddressZipCode'),
dob: ReimbursementAccountUtils.getDefaultStateForField(this.props, 'dob'),
ssnLast4: ReimbursementAccountUtils.getDefaultStateForField(this.props, 'ssnLast4'),
}}
inputKeys={{
firstName: 'firstName',
lastName: 'lastName',
dob: 'dob',
ssnLast4: 'ssnLast4',
street: 'beneficialOwnerAddressStreet',
city: 'beneficialOwnerAddressCity',
state: 'beneficialOwnerAddressState',
zipCode: 'beneficialOwnerAddressZipCode',
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These keys should be dynamic

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll look into a solution for these since we might need to make changes to the Form component itself

Copy link
Contributor

@luacmartins luacmartins Dec 12, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@grgia I took a look at dynamically adding/removing IdentityForm and the solution below should work:

  1. Store beneficialOwners in local state like we do now. However, we'll only store an ID for the IdentityForm instead of the full form input keys. No need to change anything here, we'll do so in the other methods that use state.
  2. Update addBeneficialOwner to store this ID in beneficialOwners and set the Form draft values accordingly. Something like:
addBeneficialOwner() {
    this.setState((prevState) => {
        const beneficialOwners = [...prevState.beneficialOwners, NumberUtils.rand64()];

        // We set 'beneficialOwners' to null first because we don't have a way yet to replace a specific property without merging it.
        // We don't use the debounced function because we want to make both function calls.
        FormActions.setDraftValues(ONYXKEYS.REIMBURSEMENT_ACCOUNT, {beneficialOwners: null});
        FormActions.setDraftValues(ONYXKEYS.REIMBURSEMENT_ACCOUNT, {beneficialOwners});

        return {beneficialOwners};
    });
}
  1. Update removeBeneficialOwner to take the ID of the IdentityForm we are removing and update the Form draft accordingly. Like so:
removeBeneficialOwner(beneficialOwner) {
    this.setState((prevState) => {
        const beneficialOwners = _.without(prevState.beneficialOwners, beneficialOwner);

        // We set 'beneficialOwners' to null first because we don't have a way yet to replace a specific property without merging it.
        // We don't use the debounced function because we want to make both function calls.
        FormActions.setDraftValues(ONYXKEYS.REIMBURSEMENT_ACCOUNT, {beneficialOwners: null});
        FormActions.setDraftValues(ONYXKEYS.REIMBURSEMENT_ACCOUNT, {beneficialOwners});

        return {beneficialOwners};
    });
}
  1. In the render method, map over the beneficialOwners state, which now stores IDs, and pass a key in the format beneficialOwner.${id}.firstName to both defaultValues and inputKeys. Something like:
defaultValues={{
    firstName: ReimbursementAccountUtils.getDefaultStateForField(this.props, `beneficialOwner.${id}.firstName`),
    ...
    }}
inputKeys={{
    firstName: `beneficialOwner.${id}.firstName`,
    ...
}}
  1. Add onValueChange={value => this.setState({hasOtherBeneficialOwners: value})} to the hasOtherBeneficialOwners checkbox.
  2. Update the call to this.removeBeneficialOwner to pass the ID of the IdentityForm we are removing.
  3. Update the validate function to use the new keys.
_.each(this.state.beneficialOwners, (id) => {
    if (!ValidationUtils.isRequiredFulfilled(values[`beneficialOwner.${id}.firstName`])) {
        errors[`beneficialOwner.${id}.firstName`] = this.props.translate('bankAccount.error.firstName');
    }
    ...
  1. The main issue with this approach is that it doesn't update the state in Form, so it could still pass data stored for deleted IdentityForms (if the user removed a beneficial owner as they are editing the form). The only drawback that I can think of is that we will have to filter these values in the submit function so we don't send them with the API request. I think this is a fine trade off considering that it makes the rest of the implementation easier.

Let me know what you think of this approach!

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This method looks like it's working well! I wrote out my plan for testing, so I still need to QA edge cases

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sounds good! I'll take another look today! Thanks for working on this.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK I am still a little confused, but haven't yet given this a proper investigation to produce any alternatives. Something does not sit right with me about the random id thing. I feel like I would have remembered this from the Design Doc (maybe it didn't come up). It has the feeling of something we might want to bring up in Slack, decide together the best way to do it, and then update everyone on the process (maybe modify the very awesome FORMS.md doc 😉).

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think this came up in the doc (at least I don't recall it either). IMO the random id solution solves this issue quite well, but I'm open to discussing this further in Slack if others prefer that.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd love to see this get to the finish line this week, but I'm happy to hold this for a discussion and move forward with whatever is decided (final testing for the current method or implementing a new method). I agree that the current method works well, and also that it would be worth updating everyone on the process and why it's done this way in FORMS.md

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

so, sounds like we agree that it's a good thing to discuss and that you are offering to lead a discussion on it? Or no? 😄

It might feel like a small battle - but I am concerned with this solution because it feels like something I would never remember how to do or know the reason why a random ID is used. I am barely understanding the explanation about why it is needed. I'd say that's a sign we haven't solved it in the best way possible (maybe I'm wrong). But if it has to be difficult to understand then at least we can document it better.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Started here!

}}
errors={lodashGet(this.getErrors(), `beneficialOwnersErrors[${index}]`, {})}
shouldSaveDraft
/>
{this.state.beneficialOwners.length > 1 && (
<TextLink onPress={() => this.removeBeneficialOwner(owner)}>
Expand All @@ -265,9 +284,8 @@ class ACHContractStep extends React.Component {
{this.props.translate('beneficialOwnersStep.agreement')}
</Text>
<CheckboxWithLabel
inputID="acceptTermsAndConditions"
style={[styles.mt4]}
isChecked={this.state.acceptTermsAndConditions}
onInputChange={() => this.toggleCheckbox('acceptTermsAndConditions')}
LabelComponent={() => (
<View style={[styles.flexRow]}>
<Text>{this.props.translate('common.iAcceptThe')}</Text>
Expand All @@ -276,19 +294,17 @@ class ACHContractStep extends React.Component {
</TextLink>
</View>
)}
errorText={this.getErrorText('acceptTermsAndConditions')}
hasError={this.getErrors().acceptTermsAndConditions}
shouldSaveDraft
/>
<CheckboxWithLabel
inputID="certifyTrueInformation"
style={[styles.mt4]}
isChecked={this.state.certifyTrueInformation}
onInputChange={() => this.toggleCheckbox('certifyTrueInformation')}
LabelComponent={() => (
<Text>{this.props.translate('beneficialOwnersStep.certifyTrueAndAccurate')}</Text>
)}
errorText={this.getErrorText('certifyTrueInformation')}
shouldSaveDraft
/>
</ReimbursementAccountForm>
</Form>
</>
);
}
Expand Down