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

Fix company's website step is skipped #46940

Merged
merged 7 commits into from
Aug 8, 2024
Merged

Fix company's website step is skipped #46940

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
f525a3e
fix website step is skipped
bernhardoj Aug 7, 2024
9e530a0
only checks for undefined
bernhardoj Aug 7, 2024
297c05a
reset website to undefined
bernhardoj Aug 7, 2024
0628d37
linter
bernhardoj Aug 7, 2024
60a5bdd
remove unnecessary checks
bernhardoj Aug 7, 2024
bf68bbb
Merge branch 'main' into fix/45283-company-step-is-skipped
bernhardoj Aug 8, 2024
e4f0616
use ?? back
bernhardoj Aug 8, 2024
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
5 changes: 2 additions & 3 deletions src/libs/actions/ReimbursementAccount/resetFreePlanBankAccount.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,6 @@ import type {OnyxCollection, OnyxEntry} from 'react-native-onyx';
import Onyx from 'react-native-onyx';
import * as API from '@libs/API';
import {WRITE_COMMANDS} from '@libs/API/types';
import {getDefaultCompanyWebsite} from '@libs/BankAccountUtils';
import CONST from '@src/CONST';
import ONYXKEYS from '@src/ONYXKEYS';
import INPUT_IDS from '@src/types/form/ReimbursementAccountForm';
Expand All @@ -18,7 +17,7 @@ Onyx.connect({
/**
* Reset user's reimbursement account. This will delete the bank account.
*/
function resetFreePlanBankAccount(bankAccountID: number | undefined, session: OnyxEntry<OnyxTypes.Session>, policyID: string, user: OnyxEntry<OnyxTypes.User>) {
function resetFreePlanBankAccount(bankAccountID: number | undefined, session: OnyxEntry<OnyxTypes.Session>, policyID: string) {
if (!bankAccountID) {
throw new Error('Missing bankAccountID when attempting to reset free plan bank account');
}
Expand Down Expand Up @@ -98,7 +97,7 @@ function resetFreePlanBankAccount(bankAccountID: number | undefined, session: On
[INPUT_IDS.BUSINESS_INFO_STEP.STATE]: '',
[INPUT_IDS.BUSINESS_INFO_STEP.ZIP_CODE]: '',
[INPUT_IDS.BUSINESS_INFO_STEP.COMPANY_PHONE]: '',
[INPUT_IDS.BUSINESS_INFO_STEP.COMPANY_WEBSITE]: getDefaultCompanyWebsite(session, user),
[INPUT_IDS.BUSINESS_INFO_STEP.COMPANY_WEBSITE]: undefined,
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
[INPUT_IDS.BUSINESS_INFO_STEP.COMPANY_WEBSITE]: undefined,
[INPUT_IDS.BUSINESS_INFO_STEP.COMPANY_WEBSITE]: '',

NAB: The defaultCompanyWebsite has changed from ?? to || so we can use empty string for that case

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The value reset here is for the form draft, so it does have no effect on defaultCompanyWebsite. If we use an empty string, then the default value won't be applied.

App/src/components/Form/FormProvider.tsx

Lines 252 to 255 in 53eee53

} else if (inputValues[inputID] === undefined) {
// We want to initialize the input value if it's undefined
inputValues[inputID] = inputProps.defaultValue ?? getInitialValueByType(inputProps.valueType);
}

I updated the defaultCompanyWebsite from ?? to || because reimbursementAccount?.achData?.website is a string, so I think it's safer to use ||. Do you think we should just revert that?

Copy link
Contributor

Choose a reason for hiding this comment

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

@bernhardoj I got it, we can revert change the defaultCompanyWebsite from ?? to || and keep this change

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

Copy link
Contributor

Choose a reason for hiding this comment

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

NAB: Would null be better to remove the key from Onyx?

[INPUT_IDS.BUSINESS_INFO_STEP.COMPANY_TAX_ID]: '',
[INPUT_IDS.BUSINESS_INFO_STEP.INCORPORATION_TYPE]: '',
[INPUT_IDS.BUSINESS_INFO_STEP.INCORPORATION_DATE]: '',
Expand Down
7 changes: 1 addition & 6 deletions src/pages/ReimbursementAccount/BusinessInfo/substeps/WebsiteBusiness.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
import React, {useCallback, useEffect, useMemo} from 'react';
import React, {useCallback, useMemo} from 'react';
import type {OnyxEntry} from 'react-native-onyx';
import {withOnyx} from 'react-native-onyx';
import FormProvider from '@components/Form/FormProvider';
Expand All @@ -12,7 +12,6 @@ import type {SubStepProps} from '@hooks/useSubStep/types';
import useThemeStyles from '@hooks/useThemeStyles';
import {getDefaultCompanyWebsite} from '@libs/BankAccountUtils';
import * as ValidationUtils from '@libs/ValidationUtils';
import * as BankAccounts from '@userActions/BankAccounts';
import CONST from '@src/CONST';
import ONYXKEYS from '@src/ONYXKEYS';
import INPUT_IDS from '@src/types/form/ReimbursementAccountForm';
Expand Down Expand Up @@ -59,10 +58,6 @@ function WebsiteBusiness({reimbursementAccount, user, session, onNext, isEditing
shouldSaveDraft: isEditing,
});

useEffect(() => {
BankAccounts.addBusinessWebsiteForDraft(defaultCompanyWebsite);
}, [defaultCompanyWebsite]);

return (
<FormProvider
formID={ONYXKEYS.FORMS.REIMBURSEMENT_ACCOUNT_FORM}
Expand Down
3 changes: 2 additions & 1 deletion src/pages/ReimbursementAccount/utils/getInitialSubstepForBusinessInfo.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
import * as ValidationUtils from '@libs/ValidationUtils';
import INPUT_IDS from '@src/types/form/ReimbursementAccountForm';
import type {CompanyStepProps} from '@src/types/form/ReimbursementAccountForm';

Expand All @@ -15,7 +16,7 @@ function getInitialSubstepForBusinessInfo(data: CompanyStepProps): number {
return 1;
}

if (data[businessInfoStepKeys.COMPANY_WEBSITE] === '') {
if (!ValidationUtils.isValidWebsite(data[businessInfoStepKeys.COMPANY_WEBSITE])) {
return 2;
}

Expand Down
10 changes: 2 additions & 8 deletions src/pages/workspace/WorkspaceResetBankAccountModal.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,17 +13,14 @@ import type * as OnyxTypes from '@src/types/onyx';
type WorkspaceResetBankAccountModalOnyxProps = {
/** Session info for the currently logged in user. */
session: OnyxEntry<OnyxTypes.Session>;

/** The user's data */
user: OnyxEntry<OnyxTypes.User>;
};

type WorkspaceResetBankAccountModalProps = WorkspaceResetBankAccountModalOnyxProps & {
/** Reimbursement account data */
reimbursementAccount: OnyxEntry<OnyxTypes.ReimbursementAccount>;
};

function WorkspaceResetBankAccountModal({reimbursementAccount, session, user}: WorkspaceResetBankAccountModalProps) {
function WorkspaceResetBankAccountModal({reimbursementAccount, session}: WorkspaceResetBankAccountModalProps) {
const styles = useThemeStyles();
const {translate} = useLocalize();
const achData = reimbursementAccount?.achData;
Expand All @@ -49,7 +46,7 @@ function WorkspaceResetBankAccountModal({reimbursementAccount, session, user}: W
}
danger
onCancel={BankAccounts.cancelResetFreePlanBankAccount}
onConfirm={() => BankAccounts.resetFreePlanBankAccount(bankAccountID, session, achData?.policyID ?? '-1', user)}
onConfirm={() => BankAccounts.resetFreePlanBankAccount(bankAccountID, session, achData?.policyID ?? '-1')}
Comment on lines -52 to +49
Copy link
Contributor

Choose a reason for hiding this comment

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

Could you please explain this change?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The user is used before for the default website.

App/src/libs/actions/ReimbursementAccount/resetFreePlanBankAccount.ts

Line 101 in 7b18ac5

[INPUT_IDS.BUSINESS_INFO_STEP.COMPANY_WEBSITE]: getDefaultCompanyWebsite(session, user),

shouldShowCancelButton
isVisible
/>
Expand All @@ -62,7 +59,4 @@ export default withOnyx<WorkspaceResetBankAccountModalProps, WorkspaceResetBankA
session: {
key: ONYXKEYS.SESSION,
},
user: {
key: ONYXKEYS.USER,
},
})(WorkspaceResetBankAccountModal);
Loading