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 navigation for different assignees in confirmation flow #56732

Open
wants to merge 18 commits into
base: main
Choose a base branch
from
+32 −14
Open

fix navigation for different assignees in confirmation flow #56732

Changes from 2 commits
Commits
Show all changes
18 commits
Select commit Hold shift + click to select a range
1dd72aa
fix navigation for different assignees in confirmation flow
twilight2294 Feb 12, 2025
ba7bcf0
fix failing test
twilight2294 Feb 12, 2025
7eb930a
Merge branch 'Expensify:main' into issue/56009
twilight2294 Feb 14, 2025
9da4a6b
Merge branch 'Expensify:main' into issue/56009
twilight2294 Feb 16, 2025
cf36251
Merge branch 'Expensify:main' into issue/56009
twilight2294 Feb 18, 2025
60a1c89
use a `useRef` to store the first value of the card assignee email
twilight2294 Feb 18, 2025
4722cfd
make use of `useState`
twilight2294 Feb 18, 2025
867522c
move `useRef` usage to `AssignCardFeedPage`
twilight2294 Feb 18, 2025
f5a57e5
Merge branch 'issue/56009' of https://github.com/twilight2294/App int…
twilight2294 Feb 18, 2025
9a7fa81
fix named imports `ESLint`
twilight2294 Feb 18, 2025
8f34ad6
remove unnecessary console log
twilight2294 Feb 18, 2025
3e49e20
Merge branch 'Expensify:main' into issue/56009
twilight2294 Feb 20, 2025
85638f4
Merge branch 'Expensify:main' into issue/56009
twilight2294 Feb 21, 2025
a177e5c
Revert all changes after cf362518984492f51112009286e966b775319433 , e…
twilight2294 Feb 21, 2025
baad25d
update route to add new param `workspaceMemberAccountID`
twilight2294 Feb 21, 2025
57f24c1
fix eslint
twilight2294 Feb 21, 2025
f328608
fix lint
twilight2294 Feb 21, 2025
e15accf
gracefully exit if value if undefined
twilight2294 Feb 21, 2025
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
31 changes: 20 additions & 11 deletions src/pages/workspace/companyCards/assignCard/ConfirmationStep.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,10 +9,10 @@ import Text from '@components/Text';
import useLocalize from '@hooks/useLocalize';
import useNetwork from '@hooks/useNetwork';
import useThemeStyles from '@hooks/useThemeStyles';
import * as CardUtils from '@libs/CardUtils';
import * as PersonalDetailsUtils from '@libs/PersonalDetailsUtils';
import {assignWorkspaceCompanyCard, clearAssignCardStepAndData, setAssignCardStepAndData} from '@libs/actions/CompanyCards';
import {maskCardNumber} from '@libs/CardUtils';
import {getPersonalDetailByEmail} from '@libs/PersonalDetailsUtils';
import Navigation from '@navigation/Navigation';
import * as CompanyCards from '@userActions/CompanyCards';
import CONST from '@src/CONST';
import ONYXKEYS from '@src/ONYXKEYS';
import ROUTES from '@src/ROUTES';
Expand All @@ -35,23 +35,32 @@ function ConfirmationStep({policyID, backTo}: ConfirmationStepProps) {
const [assignCard] = useOnyx(ONYXKEYS.ASSIGN_CARD);

const data = assignCard?.data;
const cardholderName = PersonalDetailsUtils.getPersonalDetailByEmail(data?.email ?? '')?.displayName ?? '';

const cardholderName = getPersonalDetailByEmail(data?.email ?? '')?.displayName ?? '';
const parts = backTo?.split('/');
const membersIndex = parts?.indexOf('members') ?? -1;
const workspaceMemberAccountID = parts?.[membersIndex + 1] ?? '';
Copy link
Contributor

Choose a reason for hiding this comment

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

This is a hacky solution. We don't want to assume that the string members is always followed by. the account id. Can we instead extract the account id from assignCard?.data first value?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

okay, I will try that today and update back, thanks for the feedback

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Can we instead extract the account id from assignCard?.data first value?

@s77rt we get the assignCard?.data email value for cardholderAccountID, already, and it will keep changing everytime the assingee will change, so i guess our option is to extract the account ID from the URL itself, I will still think through this one, but let me know if you have any idea around this

Copy link
Contributor

Choose a reason for hiding this comment

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

I meant to use the first valid value from assignCard?.data. You can try use useRef or usePrevious hooks.

PS: Only store the first valid value i.e. check that assignedData is not undefined (e.g. onyx data is not loaded yet)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yeah, usePrevious was the first thing that came in my mind, thanks for the input, I will try implementing it and report back!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@s77rt how about we add a new prop to AssignCardData :

App/src/types/onyx/AssignCard.ts

Lines 8 to 10 in 6d5c89f

type AssignCardData = {
/** The email address of the assignee */
email: string;

As workspaceMemberEmail, so everytime in WorkspaceMemberNewCardPage, when we set initial data below:

App/src/pages/workspace/members/WorkspaceMemberNewCardPage.tsx

Lines 73 to 76 in 6d5c89f

} else {
const data: Partial<AssignCardData> = {
email: memberLogin,
bankName: selectedFeed,

We set this value and later compare this value with cardholder email ? , what do you think of this approach

Copy link
Contributor

Choose a reason for hiding this comment

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

Let's not do that as that info is not part of the card data

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Can we instead pass an extra param to the WORKSPACE_EXPENSIFY_CARD_ISSUE_NEW route and use it for comparison with the selected account id

the routes will only take backTo as an extra param by using getUrlWithBackToParam:

App/src/ROUTES.ts

Lines 17 to 20 in edef703

function getUrlWithBackToParam<TUrl extends string>(url: TUrl, backTo?: string, shouldEncodeURIComponent = true): `${TUrl}` {
const backToParam = backTo ? (`${url.includes('?') ? '&' : '?'}backTo=${shouldEncodeURIComponent ? encodeURIComponent(backTo) : backTo}` as const) : '';
return `${url}${backToParam}` as `${TUrl}`;
}

So i guess for this one we would need to build a new similar function which would work for the company cards redirection route, currently it used getUrlWithBackToParam, should i proceed with this approach ?

Copy link
Contributor

Choose a reason for hiding this comment

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

No need for a new function. You can add the param before calling getUrlWithBackToParam. Please refer to SETTINGS_2FA or SETTINGS_EXIT_SURVEY_RESPONSE

Copy link
Contributor Author

@twilight2294 twilight2294 Feb 21, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Oh sorry i misunderstood what you meant here, yeah, it is a lot more reliable to follow the approach you suggested, updating the code now

const cardholderAccountID = getPersonalDetailByEmail(data?.email ?? '')?.accountID.toString() ?? '';
const submit = () => {
if (!policyID) {
return;
}
CompanyCards.assignWorkspaceCompanyCard(policyID, data);
Navigation.navigate(backTo ?? ROUTES.WORKSPACE_COMPANY_CARDS.getRoute(policyID));
CompanyCards.clearAssignCardStepAndData();
assignWorkspaceCompanyCard(policyID, data);

if (backTo) {
Navigation.navigate(workspaceMemberAccountID === cardholderAccountID ? backTo : ROUTES.WORKSPACE_COMPANY_CARDS.getRoute(policyID));
} else {
Navigation.navigate(ROUTES.WORKSPACE_COMPANY_CARDS.getRoute(policyID));
}

clearAssignCardStepAndData();
};

const editStep = (step: AssignCardStep) => {
CompanyCards.setAssignCardStepAndData({currentStep: step, isEditing: true});
setAssignCardStepAndData({currentStep: step, isEditing: true});
};

const handleBackButtonPress = () => {
CompanyCards.setAssignCardStepAndData({currentStep: CONST.COMPANY_CARD.STEP.TRANSACTION_START_DATE});
setAssignCardStepAndData({currentStep: CONST.COMPANY_CARD.STEP.TRANSACTION_START_DATE});
};

return (
Expand All @@ -77,7 +86,7 @@ function ConfirmationStep({policyID, backTo}: ConfirmationStepProps) {
/>
<MenuItemWithTopDescription
description={translate('workspace.companyCards.card')}
title={CardUtils.maskCardNumber(data?.cardNumber ?? '', data?.bankName)}
title={maskCardNumber(data?.cardNumber ?? '', data?.bankName)}
shouldShowRightIcon
onPress={() => editStep(CONST.COMPANY_CARD.STEP.CARD)}
/>
Expand Down
Loading