[$500] Dev: Web - Console error occurs when clicking on a deleted or paid request money category URL

[kbecciv]

If you haven’t already, check out our contributing guidelines for onboarding and email contributors@expensify.com to request to join our Slack channel!

---

Action Performed:

1. Go to the workspace expense room chat.
2. Navigate to "Request money" > "Manual"
3. Enter an amount and click on "Next"
4. Select a category and send the request.
5. Go to the sent request money detail page.
6. Click on the Category and copy the URL, then send it.
7. Delete the request money.
8. Click on the sent URL for the deleted request money.

Expected Result:

No console error should appear when clicking on the URL for a deleted or paid request money category

Actual Result:

A console error occurs when clicking on a deleted or paid request money category URL

Workaround:

Unknown

Platforms:

Which of our officially supported platforms is this issue occurring on?

• Android / native
• Android / Chrome
• iOS / native
• iOS / Safari
• MacOS / Chrome / Safari
• MacOS / Desktop

Version Number: Dev 1.3.74-2
Reproducible in staging?: n
Reproducible in production?: n
If this was caught during regression testing, add the test name, ID and link from TestRail:
Email or phone of affected tester (no customers):
Logs: https://stackoverflow.com/c/expensify/questions/4856
Notes/Photos/Videos: Any additional supporting documentation

screen-recording-2023-09-26-at-122150-pm_MBRPYdoK.mp4

Expensify/Expensify Issue URL:
Issue reported by: @ayazhussain79
Slack conversation: https://expensify.slack.com/archives/C049HHMV9SM/p1695713591802959

View all open jobs on GitHub

Upwork Automation - Do Not Edit

• Upwork Job URL: https://www.upwork.com/jobs/~01d64fca889407111a
• Upwork Job ID: 1707045828508962816
• Last Price Increase: 2023-09-27

[melvin-bot]

Job added to Upwork: https://www.upwork.com/jobs/~01d64fca889407111a

[melvin-bot]

Triggered auto assignment to Contributor-plus team member for initial proposal review - @fedirjh (External)

[hungvu193]

Proposal

Please re-state the problem that we are trying to solve in this issue.

Dev: Web - Console error occurs when clicking on a deleted or paid request money category URL

What is the root cause of that problem?

We're checking for update here to dismiss the modal when a request is deleted or paid.

App/src/pages/EditRequestPage.js

Lines 109 to 114 in cf9ad20

	if (canEdit) {
	return;
	}
	Navigation.isNavigationReady().then(() => {
	Navigation.dismissModal();
	});

However in the meantime while navigate is ready, we still can render other pages before our Navigation.dismissModal(); is called.

However it took 300ms for this timeout to be called, but the view is already unmounted due to our Navigation.dismissModal();

App/src/components/OptionsSelector/BaseOptionsSelector.js

Lines 71 to 75 in cf9ad20

	if (this.props.isFocused && this.props.autoFocus && this.textInput) {
	setTimeout(() => {
	this.textInput.focus();
	}, CONST.ANIMATED_TRANSITION);
	}

What changes do you think we should make in order to solve the problem?

1. We should create a timeoutID for our BaseOptionSelector here:
   

   App/src/components/OptionsSelector/BaseOptionsSelector.js

   Lines 71 to 75 in cf9ad20

   	if (this.props.isFocused && this.props.autoFocus && this.textInput) {
   	setTimeout(() => {
   	this.textInput.focus();
   	}, CONST.ANIMATED_TRANSITION);
   	}

And clear it in our componentWillUnmount function.

We can also do similar thing with this block of code

2. Remove the useEffect that used to dismiss the modal. Add a new variable called isCurrentRequestSettled to check if the current request was already settled, if yes we should redirect user in request page.

const isCurrentRequestSettled = isSettled && (isAdmin || isRequestor);

    useEffect(() => {
        if (!isCurrentRequestSettled) { 
            return;
        }
        
        Navigation.isNavigationReady().then(() => {
            Navigation.dismissModal(report.reportID);
        });

    }, [isCurrentRequestSettled]);

3. Update the condition of rendering other screen, use need to have canEdit true to edit the page, otherwise FullPageNotFound will be displayed.

    if (canEdit && fieldToEdit === CONST.EDIT_REQUEST_FIELD.DESCRIPTION) {
        return (
            <EditRequestDescriptionPage />
        );
    }

Or we can just early return the FullPageNotFoundView page with the shouldShow={!canEdit}.

What alternative solutions did you explore? (Optional)

In our EditRequestPage, we should add an early return, instead of return FullPageNotFound at the end, we can bring it first if we couldn't edit the route:

    if (!canEdit) {
        return <FullPageNotFoundView shouldShow />;
    }

or we just return null if we can't edit the route:

    if (!canEdit) {
        return null;
    }

[saranshbalyan-1234]

Proposal

Please re-state the problem that we are trying to solve in this issue.

Getting error while editing category of deleted request.

What is the root cause of that problem?

App/src/components/OptionRow.js

Line 218 in cf9ad20

<View style={[styles.flexRow, styles.alignItemsCenter]}>

Here this.props.option.icons is undefined which causes the error.

What changes do you think we should make in order to solve the problem?

We can have default value as 0 if Here this.props.option.icons is undefined

What alternative solutions did you explore? (Optional)

N/A

[Youhy]

Proposal

Please re-state the problem that we are trying to solve in this issue.

Getting error while editing category of deleted request.

What is the root cause of that problem?

this.textInput is a ref which is not initialised by the time when the function is called, or component already unmounted when the timeOut is executed

App/src/components/OptionsSelector/BaseOptionsSelector.js

Lines 71 to 75 in [cf9ad20](https://github.com/Expensify/App/commit/cf9ad20a4545ec01b16f88f9c1a53a7488ca1b44)
 if (this.props.isFocused && this.props.autoFocus && this.textInput) { 
     setTimeout(() => { 
         this.textInput.focus(); 
     }, CONST.ANIMATED_TRANSITION); 
 }

What changes do you think we should make in order to solve the problem?

Ref cycle should be initialised properly and should not rely on timeouts assuming that element will be accessible by the time timeout is fulfilled. The timeout execution depends on the amount of asynchronous code running in EventLoop already.

What alternative solutions did you explore? (Optional)

Early return in EditRequestPage instead of returning FullPageNotFound at the end can help also, but this is a hack to bypass the issue instead of fixing it

[melvin-bot]

📣 @Youhy! 📣
Hey, it seems we don’t have your contributor details yet! You'll only have to do this once, and this is how we'll hire you on Upwork.
Please follow these steps:

1. Make sure you've read and understood the contributing guidelines.
2. Get the email address used to login to your Expensify account. If you don't already have an Expensify account, create one here. If you have multiple accounts (e.g. one for testing), please use your main account email.
3. Get the link to your Upwork profile. It's necessary because we only pay via Upwork. You can access it by logging in, and then clicking on your name. It'll look like this. If you don't already have an account, sign up for one here.
4. Copy the format below and paste it in a comment on this issue. Replace the placeholder text with your actual details.
   
   Format:

Contributor details
Your Expensify account email: <REPLACE EMAIL HERE>
Upwork Profile Link: <REPLACE LINK HERE>

[Youhy]

Contributor details
Your Expensify account email: youhy@tut.by
Upwork Profile Link: https://www.upwork.com/freelancers/~01d867cf0154cbd9ba

[melvin-bot]

✅ Contributor details stored successfully. Thank you for contributing to Expensify!

[fedirjh]

Thanks @hungvu193 for the proposal, it seems that we have the clear logic for the timeout in the BaseOptionsSelector, it looks this was missed during #25564

App/src/components/OptionsSelector/BaseOptionsSelector.js

Lines 135 to 137 in cf9ad20

	if (this.focusTimeout) {
	clearTimeout(this.focusTimeout);
	}

Happy to move forward with @hungvu193's proposal as it fixes the root cause.

🎀 👀 🎀 C+ reviewed

---

Thanks, @Youhy for the proposal, clearing the timeout is the ideal solution for this case.

[melvin-bot]

Triggered auto assignment to @iwiznia, see https://stackoverflow.com/c/expensify/questions/7972 for more details.

[fedirjh]

cc @iwiznia Seems like we haven’t assigned a BZ member for this bug.

[iwiznia]

A couple questions:

1. How would the timeoutID work exactly?
2. Looking at the error seems that the variable is null and thus why it is failing, can't we just check that this.textInput is not null?
3. What happens after you click the link with the proposed fix? Does it not do anything at all? Because I think it should show the page not found page, but I don't think it will?

[hungvu193]

A couple questions:

1. How would the timeoutID work exactly?

2. Looking at the error seems that the variable is null and thus why it is failing, can't we just check that this.textInput is not null?

3. What happens after you click the link with the proposed fix? Does it not do anything at all? Because I think it should show the page not found page, but I don't think it will?

1. setTimeout returned a positive int and we can pass it to clearTimeout to cancel the timeout (we actually did it before, but recently when we added new setTimeout we forgot to clean it)
2. I don't think checking this.textInput is a good idea since it didn't solved the root cause, the problem here is that we were trying to run a function from unmounted component.
3. After proposing the fix it will do nothing (as we dismissNavigation in our useEffect), but we can improve that.

[fedirjh]

How would the timeoutID work exactly?

It will simply be a reference that holds the timeout ID. We will use this reference to clear the timeout when the component unmounts. We already have the clear logic in this line

this.focusTimeout = setTimeout(...)

Looking at the error seems that the variable is null and thus why it is failing, can't we just check that this.textInput is not null?

Yes, that is expected. We didn't clear the timeout, and it will be executed after the component unmounts. Consequently, the input reference will be null at that time. So, it's ideal to clear the timeout rather than adding a safety check.

but I don't think it will?

Yes, that is currently the behavior. Should we consider updating it to display a "not found" view?

[iwiznia]

Thanks for clarifying! And yes, I think having a link that does absolutely nothing is as broken as the current behavior from a user perspective.

[hungvu193]

So do we need to show the Not found page in this case?

[melvin-bot]

Triggered auto assignment to @greg-schroeder (Bug), see https://stackoverflow.com/c/expensify/questions/14418 for more details.

[melvin-bot]

Bug0 Triage Checklist (Main S/O)

• This "bug" occurs on a supported platform (ensure Platforms in OP are ✅)
• This bug is not a duplicate report (check E/App issues and #expensify-bugs)
  • If it is, comment with a link to the original report, close the issue and add any novel details to the original issue instead
• This bug is reproducible using the reproduction steps in the OP. S/O
  • If the reproduction steps are clear and you're unable to reproduce the bug, check with the reporter and QA first, then close the issue.
  • If the reproduction steps aren't clear and you determine the correct steps, please update the OP.
• This issue is filled out as thoroughly and clearly as possible
  • Pay special attention to the title, results, platforms where the bug occurs, and if the bug happens on staging/production.
• I have reviewed and subscribed to the linked Slack conversation to ensure Slack/Github stay in sync

[iwiznia]

I think we should, @greg-schroeder what do you think?

[greg-schroeder]

We should definitely not have a link that does nothing - that's just the same buggy user experienced repackaged. I think we should show the not found page for sure

[greg-schroeder]

Seems the slack discussion ended with:

"So if the user that submitted the request visits the edit page of a settled request, they would land (or get redirected) to the request page with all fields disabled"

[hungvu193]

Cool. Update my proposal to match the expected result:

• If request is settled, redirect user to request page.
• If user don't have edit permission, FullPageNotFound will be displayed.

Result

Screen.Recording.2023-10-17.at.22.22.12.mov

[melvin-bot]

@iwiznia @greg-schroeder @fedirjh this issue is now 3 weeks old. There is one more week left before this issue breaks WAQ and will need to go internal. What needs to happen to get a PR in review this week? Please create a thread in #expensify-open-source to discuss. Thanks!

[fedirjh]

Let's hold until #29101 is merged.

[hungvu193]

Looks like #29101 is now merged

[hungvu193]

now merchant and description can be updated when a request is paid, is this expected?

[greg-schroeder]

What do you think @fedirjh?

[fedirjh]

now merchant and description can be updated when a request is paid, is this expected?

This is the expected behavior:

1. IOU: No edits allowed after request is settled.
2. Expense Reports:
   • Members (requester): No edits allowed after request is settled.
   • Admins: Can edit any field except : amount, currency, date, distance and receipt, after the request is settled

---

What do you think

@greg-schroeder Let's remove the hold. I am not sure what we will be fixing in this issue. Another PR fixed the original issue that was reported. So I think we should just close this issue.

[hungvu193]

When clicking edit date deeplink still nothing happens. Isn't that we're fixing in this issue?

[fedirjh]

When clicking edit date deeplink still nothing happens. Isn't that we're fixing in this issue?

@hungvu193 As per this thread: https://expensify.slack.com/archives/C01GTK53T8Q/p1697137352110949?thread_ts=1697134708.508379&cid=C01GTK53T8Q , I think that's expected behavior.

[greg-schroeder]

Removed hold, sorry for delay I missed this one

[greg-schroeder]

I think we should just close this after reviewing more. Please comment if you disagree.