fix navigation for different assignees in confirmation flow #56732

twilight2294 · 2025-02-12T06:35:05Z

Explanation of Change

This PR fixes the navigation logic for company cards page, whenever a card is assigned from the workspace members profile page but we select a different member on the confirmation screen, we now redirect to the company cards page than workspace member details page.

Fixed Issues

$ #56009
PROPOSAL: #56009 (comment)

Tests

Same as QA

Verify that no errors appear in the JS console

Offline tests

Cannot test offline

QA Steps

Precondition: workspace with several members, with enabled Company cards. Some company cards are added.

Go to ws > Members
Click on member with already assigned company card (if no, assign card to member)
Click on 'New card'
Proceed to the 4th step 'Let’s double check that everything looks right.'
Click on Cardholder field
Select another ws member > Click 'Assign card'

Verify that: User lands to Company cards page, rather than the workspace member profile he was previously on (as you assigned the card to someone else)

Verify that no errors appear in the JS console

PR Author Checklist

Screenshots/Videos

Android: Native

Android: mWeb Chrome

iOS: Native

iOS: mWeb Safari

MacOS: Chrome / Safari

MacOS: Desktop

melvin-bot · 2025-02-12T06:35:50Z

@s77rt Please copy/paste the Reviewer Checklist from here into a new comment on this PR and complete it. If you have the K2 extension, you can simply click: [this button]

twilight2294 · 2025-02-12T07:00:43Z

@s77rt , I can only test with mocking the data, can you test with some real data please ?

s77rt · 2025-02-12T10:56:21Z

src/pages/workspace/companyCards/assignCard/ConfirmationStep.tsx

+    const parts = backTo?.split('/');
+    const membersIndex = parts?.indexOf('members') ?? -1;
+    const workspaceMemberAccountID = parts?.[membersIndex + 1] ?? '';


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?

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

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

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)

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

@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

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

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 ?

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

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

s77rt · 2025-02-12T10:56:52Z

Also please merge main to take into account the new navigation refactor

twilight2294 · 2025-02-14T09:44:36Z

@s77rt done with merging, I'll update the requested change today

twilight2294 · 2025-02-18T07:49:34Z

src/pages/workspace/companyCards/assignCard/ConfirmationStep.tsx

+    const workspaceMemberAccountID = getPersonalDetailByEmail(firstValidEmail ?? '')?.accountID?.toString() ?? '';
+    const cardholderAccountID = getPersonalDetailByEmail(data?.email ?? '')?.accountID?.toString() ?? '';


@s77rt last question, I think we should directly compare numerical values here instead of converting it to a string, it saves extra compute, what do you think ?

…o issue/56009

src/pages/workspace/companyCards/assignCard/AssignCardFeedPage.tsx

twilight2294 · 2025-02-18T08:59:29Z

src/pages/workspace/companyCards/assignCard/AssignCardFeedPage.tsx

@@ -27,10 +27,17 @@ function AssignCardFeedPage({route, policy}: AssignCardFeedPageProps) {
    const backTo = route.params?.backTo;
    const policyID = policy?.id;
    const [isActingAsDelegate] = useOnyx(ONYXKEYS.ACCOUNT, {selector: (account) => !!account?.delegatedAccess?.delegate});
+    const [firstValidEmail, setFirstValidEmail] = useState<string | null>(null);
+    useEffect(() => {
+        if (firstValidEmail || !assignCard?.data?.email) {


@s77rt should we disable this error ?, in useState i can initialise the value to empty string and then silence the error here, what do you think ?

src/pages/workspace/companyCards/assignCard/ConfirmationStep.tsx

s77rt · 2025-02-21T18:21:46Z

src/pages/workspace/companyCards/assignCard/AssignCardFeedPage.tsx

                <ConfirmationStep
                    policyID={policyID}
                    backTo={backTo}
+                    workspaceMemberAccountID={workspaceMemberAccountID}
                />


I missed that we pass route params here. In that case we shouldn't use the current approach as we are still passing through AssignCardFeedPage. Instead we should store the first valid assignee email and compare it with the current one and if they are not the same let's not pass the backTo param. Something like this:

const firstAssigneeEmail = useRef(issueNewCard?.data.assigneeEmail); if (!firstAssigneeEmail.current) { firstAssigneeEmail.current = issueNewCard?.data.assigneeEmail; } const shouldUseBackToParam = !firstAssigneeEmail.current || firstAssigneeEmail.current === issueNewCard?.data.assigneeEmail;

@s77rt I tried this one and Confirmation page re-renders each time clearing the firstAssigneeEmail ref, we had to pass it via AssignCardFeedPage only for the value of the ref to persist, you can take a look at 867522c commit of the previous implementation.

Its late for me now, but i will again try to take a look at this tomorrow morning, thanks

That code should go in AssignCardFeedPage

Alright!, I am OoO for today, I will update this PR tomorrow morning, thanks for the patience

fix navigation for different assignees in confirmation flow

1dd72aa

twilight2294 marked this pull request as ready for review February 12, 2025 06:35

twilight2294 requested a review from a team as a code owner February 12, 2025 06:35

melvin-bot bot requested a review from s77rt February 12, 2025 06:35

melvin-bot bot removed the request for review from a team February 12, 2025 06:35

fix failing test

ba7bcf0

s77rt suggested changes Feb 12, 2025

View reviewed changes

Merge branch 'Expensify:main' into issue/56009

7eb930a

twilight2294 added 4 commits February 16, 2025 17:40

Merge branch 'Expensify:main' into issue/56009

9da4a6b

Merge branch 'Expensify:main' into issue/56009

cf36251

use a useRef to store the first value of the card assignee email

60a1c89

make use of useState

4722cfd

twilight2294 commented Feb 18, 2025

View reviewed changes

twilight2294 added 3 commits February 18, 2025 08:44

move useRef usage to AssignCardFeedPage

867522c

Merge branch 'issue/56009' of https://github.com/twilight2294/App int…

f5a57e5

…o issue/56009

fix named imports ESLint

9a7fa81

twilight2294 commented Feb 18, 2025

View reviewed changes

src/pages/workspace/companyCards/assignCard/AssignCardFeedPage.tsx Outdated Show resolved Hide resolved

remove unnecessary console log

8f34ad6

twilight2294 commented Feb 18, 2025

View reviewed changes

twilight2294 added 5 commits February 20, 2025 14:28

Merge branch 'Expensify:main' into issue/56009

3e49e20

Merge branch 'Expensify:main' into issue/56009

85638f4

Revert all changes after cf36251 , excluding merge commits

a177e5c

update route to add new param workspaceMemberAccountID

baad25d

fix eslint

57f24c1

twilight2294 requested a review from s77rt February 21, 2025 06:11

fix lint

f328608

twilight2294 commented Feb 21, 2025

View reviewed changes

src/pages/workspace/companyCards/assignCard/ConfirmationStep.tsx Outdated Show resolved Hide resolved

gracefully exit if value if undefined

e15accf

s77rt suggested changes Feb 21, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix navigation for different assignees in confirmation flow #56732

fix navigation for different assignees in confirmation flow #56732

twilight2294 commented Feb 12, 2025 •

edited

Loading

melvin-bot bot commented Feb 12, 2025

twilight2294 commented Feb 12, 2025

s77rt Feb 12, 2025

twilight2294 Feb 14, 2025

twilight2294 Feb 16, 2025

s77rt Feb 16, 2025

twilight2294 Feb 16, 2025

twilight2294 Feb 20, 2025

s77rt Feb 20, 2025

twilight2294 Feb 20, 2025

s77rt Feb 20, 2025

twilight2294 Feb 21, 2025 •

edited

Loading

s77rt commented Feb 12, 2025

twilight2294 commented Feb 14, 2025

twilight2294 Feb 18, 2025

twilight2294 Feb 18, 2025

s77rt Feb 21, 2025

twilight2294 Feb 21, 2025

s77rt Feb 21, 2025

twilight2294 Feb 22, 2025

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

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

	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}`;
	}

		const workspaceMemberAccountID = getPersonalDetailByEmail(firstValidEmail ?? '')?.accountID?.toString() ?? '';
		const cardholderAccountID = getPersonalDetailByEmail(data?.email ?? '')?.accountID?.toString() ?? '';

fix navigation for different assignees in confirmation flow #56732

Are you sure you want to change the base?

fix navigation for different assignees in confirmation flow #56732

Conversation

twilight2294 commented Feb 12, 2025 • edited Loading

Explanation of Change

Fixed Issues

Tests

Offline tests

QA Steps

PR Author Checklist

Screenshots/Videos

melvin-bot bot commented Feb 12, 2025

twilight2294 commented Feb 12, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

twilight2294 Feb 21, 2025 • edited Loading

Choose a reason for hiding this comment

s77rt commented Feb 12, 2025

twilight2294 commented Feb 14, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

twilight2294 commented Feb 12, 2025 •

edited

Loading

twilight2294 Feb 21, 2025 •

edited

Loading