[HOLD for payment 2024-06-13] [$500] Login with edit description does not open edit description page

[m-natarajan]

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

---

Version Number: 1.3.79-3
Reproducible in staging?: Yes
Reproducible in production?: Yes
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
Expensify/Expensify Issue URL:
Issue reported by: @dhanashree-sawant
Slack conversation: https://expensify.slack.com/archives/C049HHMV9SM/p1696834330715939

Action Performed:

1. Open the app
2. Open request money report or raise one and open it
3. Click on description and copy the URL
4. Logout and paste the URL
5. Login and observe that app does not open description edit on after login

Expected Result:

After we login, app should open URL which we use before login

Actual Result:

After we login, app does not open edit description URL that we used before login

Workaround:

Unknown

Platforms:

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

• Android: Native
• Android: mWeb Chrome
• iOS: Native
• iOS: mWeb Safari
• MacOS: Chrome / Safari
• MacOS: Desktop

Screenshots/Videos

Android: Native

Android: mWeb Chrome

android.chrome.login.with.edit.description.does.not.open.edit.mp4

iOS: Native

iOS: mWeb Safari

ios.safari.login.with.edit.description.does.not.open.mov

MacOS: Chrome / Safari

mac.chrome.login.with.edit.description.does.not.open.mov

edit.description.mp4

MacOS: Desktop

View all open jobs on GitHub

Upwork Automation - Do Not Edit

• Upwork Job URL: https://www.upwork.com/jobs/~017838f4eb8f52a1be
• Upwork Job ID: 1711448302691934208
• Last Price Increase: 2023-10-30
• Automatic offers:
• abdulrahuman5196 | Reviewer | 27743863
• dukenv0307 | Contributor | 27743865
• dhanashree-sawant | Reporter | 27743868

Issue Owner

Current Issue Owner: @kevinksullivan

[melvin-bot]

Job added to Upwork: https://www.upwork.com/jobs/~017838f4eb8f52a1be

[melvin-bot]

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

[abdulrahuman5196]

@m-natarajan can you add Bug label so that a BZ is assigned here?

[abzokhattab]

Proposal

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

Logging with the edit description/merchant page does open these pages

What is the root cause of that problem?

in this use effect, we check if the canEdit value is false then dismiss the modal, in our case initially the report and the parent report are empty so the canEdit value resolves to false, which closes the modal

https://github.com/Expensify/App/blob/main/src/pages/EditRequestPage.js#L133-L140

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

Use the is isLoadingReportData element from oynx and set the default value to true then change the useffect to:

     isLoadingReportData: {
            key: ONYXKEYS.IS_LOADING_REPORT_DATA,
        },

    useEffect(() => {
        if (canEdit || isLoadingReportData) {
            return;
        }
        Navigation.isNavigationReady().then(() => {
            Navigation.dismissModal();
        });
    }, [canEdit, isLoadingReportData]);

Then show the loading component whenever the report is loading:

   if (isLoadingReportData) {
        return <FullScreenLoadingIndicator />;
    }

This way the loading indicator will be shown until the results are shown.

POC

Screen.Recording.2023-10-13.at.12.00.56.PM.mov

[saranshbalyan-1234]

Proposal

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

Login with edit description deep link does not open edit description page

What is the root cause of that problem?

App/src/pages/EditRequestPage.js

Lines 133 to 140 in c0fe505

	useEffect(() => {
	if (canEdit) {
	return;
	}
	Navigation.isNavigationReady().then(() => {
	Navigation.dismissModal();
	});
	}, [canEdit]);

we are dismissing modal if canEdit returns to false, by default as the data is loading, it return as false as the report object and parentreport object is empty

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

We should change the use effect to this, also add report id in the dependncy array to re run the use effect once the report id changes as with the above proposal this condition will always returns false as by default report obect or parent report objcet will be null and the below code will never be executed.

  useEffect(() => {
        if (canEdit || !report.reportID) ) {
            return;
        }
        Navigation.isNavigationReady().then(() => {
            Navigation.dismissModal();
        });
    }, [canEdit,report.reportID]);

What alternative solutions did you explore? (Optional)

N/A

[abzokhattab]

POC was added here

[b4s36t4]

#28645 this PR got merged today which does have some issues which is a blocker for re-producing this issue. We can put this on-hold for till that completed.

[dukenv0307]

Proposal

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

Logging with the edit description/merchant page does open these pages

What is the root cause of that problem?

The problem is here, we'll close the modal as soon as canEdit is false. Initially after logged in when the report is not loaded yet, the data for calculation of canEdit is not available so it's false, leading to the Edit modal closing.

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

Checking if report is empty or reportID exists like suggested above will not work since if the request was deleted/we try to navigate to an invalid request to edit, it will also not dismiss the Edit modal.

We should connect to isLoadingReportData in Onyx, and if it's loading or the transaction is loading, and canEdit is false, we'll not dismiss the edit modal. And if the data is loading, we should show the FullScreenLoadingIndicator.
This can be updated to

const isTransactionLoadingRoute = lodashGet(transaction, 'comment.isLoading', false);
const isTransactionLoading = lodashGet(transaction, 'isLoading', false);

const isDataLoading = isLoadingReportData || isTransactionLoading || isTransactionLoadingRoute || isEmpty(transaction);
    
useEffect(() => {
        // Do not dismiss the modal, when a current user can edit this property of the money request.
        if (isDataLoading || ReportUtils.canEditFieldOfMoneyRequest(parentReportAction, parentReport.reportID, fieldToEdit)) {
            return;
        }

        // Dismiss the modal when a current user cannot edit a money request.
        Navigation.isNavigationReady().then(() => {
            Navigation.dismissModal();
        });
    }, [isDataLoading, parentReportAction, parentReport.reportID, fieldToEdit]);
    
if (isDataLoading) {
    return <FullScreenLoadingIndicator />
}

We need to connect isLoadingReportData from Onyx on that screen and set the default value of it to true as well.

What alternative solutions did you explore? (Optional)

IMO we should check both report loading and transaction loading for maximum safety, but if we want to omit the report loading check, we can try that along with rigorous testing.

[melvin-bot]

Triggered auto assignment to @kevinksullivan (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

[abdulrahuman5196]

@abzokhattab @saranshbalyan-1234 @b4s36t4 @dukenv0307
I think all the above solutions are only partial fixes for the issue. If we do any of the above, it will open the description page but could have issues if data isn't loaded (@dukenv0307's proposal mentioned to show after loading data but still incomplete IMO)

We should fix this how TasK description works. Like it shows loading indicator until data is loaded then proceeds with showing the screen.

Screen.Recording.2023-10-13.at.1.07.23.PM.mov

[saranshbalyan-1234]

@abdulrahuman5196 how you want it be handled, any suggestions?

[b4s36t4]

Proposal

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

Login with edit description does not open edit description page

What is the root cause of that problem?

There's no report data available after login, causing the modal's rendering condition to be falsy which makes modal to close automatically.

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

Proposal Update

As per @abdulrahuman5196 suggestion following the same methodologies would prevent such issues further.

We should wrap the the component with HOC we already have withReportOrNotFound which gives us the ability to check the availability of report weather the report is available or not.

With wrapping withReportOrNotFound will only work with report data, but this all happens with-in seconds at times transaction data won't be available which causes the modal to close or show empty description.

To resolve this following are the options.

1. Create a wrapper that does the same withReportOrNotFound maybe like withTransactionOrNotFound

2. Show loader while the transaction is loading.

Code for option-2

const isTransactionLoading = _.isEmpty(transaction) || lodashGet(transaction, ['comment', 'isLoading'], false);

useEffect(() => {
    if (isTransactionLoading) {
        return;
    }
    // Do not dismiss the modal, when a current user can edit this property of the money request.
    if (ReportUtils.canEditFieldOfMoneyRequest(parentReportAction, parentReport.reportID, fieldToEdit)) {
        return;
    }

    // Dismiss the modal when a current user cannot edit a money request.
    Navigation.isNavigationReady().then(() => {
        Navigation.dismissModal();
    });
}, [parentReportAction, parentReport.reportID, fieldToEdit, isTransactionLoading]);

• Create a new variable to detect the transaction data availability

• Update existing useEffect to return when transaction is loading

I have verified the following three scenarios using my proposal

• Login with valid URL - Should Open
• Login with Invalid URL - NotFoundPage (modal & reportScreen)
• Login with completed Request - Should close.

What alternative solutions did you explore? (Optional)

NA

[abzokhattab]

Proposal is updated @abdulrahuman5196 .. now the loading indactor is shown while the page is loading , please check the POC

[melvin-bot]

@kevinksullivan, @abdulrahuman5196 Whoops! This issue is 2 days overdue. Let's get this updated quick!

[abdulrahuman5196]

@b4s36t4 I tried your 3 rd option

We can make the param threadReportID as reportID just like we do with other reports

It shows loading and proper open. But it doesn't show the existing description and always shows up empty?

I assume it should be the case for other options as well. We should fix that as well IMO .

[b4s36t4]

Yes, there are some other changes might require to complete things correctly. Would you want me to update my proposal to cover all things using the third Option?

[melvin-bot]

📣 It's been a week! Do we have any satisfactory proposals yet? Do we need to adjust the bounty for this issue? 💸

[dukenv0307]

If we do any of the above, it will open the description page but could have issues if data isn't loaded (@dukenv0307's proposal mentioned to show after loading data but still incomplete IMO)

@abdulrahuman5196 sorry, what do you mean by "could have issues", can you give a specific case where it doesn't work as expected.

Thank you!

[melvin-bot]

BugZero Checklist: The PR fixing this issue has been merged! The following checklist (instructions) will need to be completed before the issue can be closed:

• [@abdulrahuman5196 / @dukenv0307] The PR that introduced the bug has been identified. Link to the PR:
• [@abdulrahuman5196 / @dukenv0307] The offending PR has been commented on, pointing out the bug it caused and why, so the author and reviewers can learn from the mistake. Link to comment:
• [@abdulrahuman5196 / @dukenv0307] A discussion in #expensify-bugs has been started about whether any other steps should be taken (e.g. updating the PR review checklist) in order to catch this type of bug sooner. Link to discussion:
• [@abdulrahuman5196 / @dukenv0307] Determine if we should create a regression test for this bug.
• [@abdulrahuman5196 / @dukenv0307] If we decide to create a regression test for the bug, please propose the regression test steps to ensure the same bug will not reach production again.
• [@kevinksullivan] Link the GH issue for creating/updating the regression test once above steps have been agreed upon:

[kevinksullivan]

@abdulrahuman5196 @dukenv0307 can you please finish the steps above?

[kevinksullivan]

Created a new job since the previous one expired, and sent offers to @dukenv0307 and @abdulrahuman5196

https://www.upwork.com/jobs/~017a3e1b0beac19165

[melvin-bot]

Triggered auto assignment to @slafortune (Bug), see https://stackoverflow.com/c/expensify/questions/14418 for more details. Please add this bug to a GH project, as outlined in the SO.

[kevinksullivan]

hey @slafortune ! I am assigning another BZ member here to wrap this up as I am OOO the rest of the week. Waiting on:

1. The checklist to be complete
2. @dukenv0307 and @abdulrahuman5196 to accept the upwork job, and then we can pay it

Thanks!

[kevinksullivan]

@abdulrahuman5196 @dukenv0307 can you complete the checklist please?

[kevinksullivan]

Payments made, just waiting on the checklist.

[dukenv0307]

@abdulrahuman5196 as the C+ need to complete the checklist

[dhanashree-sawant]

Hi @kevinksullivan, am I eligible for reporting bonus?

[kevinksullivan]

Bump @abdulrahuman5196

[kevinksullivan]

@dhanashree-sawant since this was prior to this announcement I will pay out reporting bonus. Please let me know when you accept this job

https://www.upwork.com/jobs/~0190db0218ebfa737d

[dhanashree-sawant]

Thanks @kevinksullivan, I have accepted the offer.

[kevinksullivan]

Paid @dhanashree-sawant

[robertjchen]

Looks like we're just waiting on the checklist here?

[abdulrahuman5196]

The PR that introduced the bug has been identified. Link to the PR:
The offending PR has been commented on, pointing out the bug it caused and why, so the author and reviewers can learn from the mistake. Link to comment:
A discussion in #expensify-bugs has been started about whether any other steps should be taken (e.g. updating the PR review checklist) in order to catch this type of bug sooner. Link to discussion:

Doesn't seem to be a regression

Determine if we should create a regression test for this bug.

Yes.

If we decide to create a regression test for the bug, please propose the regression test steps to ensure the same bug will not reach production again.

1. Open a request money report or raise one and open it
2. Click on the description or merchant and copy the URL
3. Logout and paste the URL
4. Login and observe that the app opens the correct description or merchant modal

@kevinksullivan

[robertjchen]

I think we can close this one out?