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.

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

[$250] Chat - When navigating to the IOU details link, the 1:1 chat opens #57589

Open
3 of 8 tasks
IuliiaHerets opened this issue Feb 28, 2025 · 16 comments
Open
3 of 8 tasks
Assignees
Labels
Bug Something is broken. Auto assigns a BugZero manager. Daily KSv2 External Added to denote the issue can be worked on by a contributor Help Wanted Apply this label when an issue is open to proposals by contributors Overdue

Comments

@IuliiaHerets
Copy link

IuliiaHerets commented Feb 28, 2025

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: v9.1.7-1
Reproducible in staging?: Yes
Reproducible in production?: Yes
Issue reported by: Applause Internal Team
Device used: Mac 15.2/Safari, iPhone 13/Safari
App Component: Other

Action Performed:

  1. Navigate to https://staging.new.expensify.com
  2. As account A, create an IOU with account B
  3. Navigate to the IOU details page and copy the URL
  4. Open a new private window and paste the link
  5. Log in to account B

Expected Result:

The user B should see the IOU details page after navigating to the link

Actual Result:

When navigating to the IOU details link, the 1:1 chat opens instead of the IOU details page

Workaround:

Unknown

Platforms:

  • Android: Standalone
  • Android: HybridApp
  • Android: mWeb Chrome
  • iOS: Standalone
  • iOS: HybridApp
  • iOS: mWeb Safari
  • MacOS: Chrome / Safari
  • MacOS: Desktop

Screenshots/Videos

Bug6757088_1740707607732.Recording__687.mp4

View all open jobs on GitHub

Upwork Automation - Do Not Edit
  • Upwork Job URL: https://www.upwork.com/jobs/~021895633114810053155
  • Upwork Job ID: 1895633114810053155
  • Last Price Increase: 2025-03-08
Issue OwnerCurrent Issue Owner: @eVoloshchak
@IuliiaHerets IuliiaHerets added Bug Something is broken. Auto assigns a BugZero manager. Daily KSv2 labels Feb 28, 2025
Copy link

melvin-bot bot commented Feb 28, 2025

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

@mallenexpensify mallenexpensify added Bug Something is broken. Auto assigns a BugZero manager. and removed Bug Something is broken. Auto assigns a BugZero manager. labels Feb 28, 2025
Copy link

melvin-bot bot commented Feb 28, 2025

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

@mallenexpensify
Copy link
Contributor

@sakluger please take over as BZ, thx

@sakluger sakluger added the External Added to denote the issue can be worked on by a contributor label Mar 1, 2025
@melvin-bot melvin-bot bot changed the title Chat - When navigating to the IOU details link, the 1:1 chat opens [$250] Chat - When navigating to the IOU details link, the 1:1 chat opens Mar 1, 2025
Copy link

melvin-bot bot commented Mar 1, 2025

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

@melvin-bot melvin-bot bot added the Help Wanted Apply this label when an issue is open to proposals by contributors label Mar 1, 2025
@sakluger sakluger moved this to Bugs and Follow Up Issues in [#whatsnext] #expense Mar 1, 2025
Copy link

melvin-bot bot commented Mar 1, 2025

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

@linhvovan29546
Copy link
Contributor

linhvovan29546 commented Mar 1, 2025

🚨 Edited by proposal-police: This proposal was edited at 2025-03-01 13:47:31 UTC.

Proposal

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

Chat - When navigating to the IOU details link, the 1:1 chat opens

What is the root cause of that problem?

When opening a deeplink, we only check for the report ID and do not consider IOU reports. As a result, when opening a link from the sign-in flow, the app navigates to the report details instead of the IOU details.

App/src/libs/actions/Report.ts

Lines 2940 to 2951 in ec99650

const report = allReports?.[`${ONYXKEYS.COLLECTION.REPORT}${reportID}`];
// If the report does not exist, navigate to the last accessed report or Concierge chat
if (!report) {
const lastAccessedReportID = findLastAccessedReport(false, shouldOpenOnAdminRoom(), undefined, reportID)?.reportID;
if (lastAccessedReportID) {
const lastAccessedReportRoute = ROUTES.REPORT_WITH_ID.getRoute(lastAccessedReportID);
Navigation.navigate(lastAccessedReportRoute);
return;
}
navigateToConciergeChat(false, () => true);
return;
}

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

We can check for iouReportID as well before navigating. The fix could look like this:

App/src/libs/actions/Report.ts

Lines 2942 to 2951 in ec99650

if (!report) {
const lastAccessedReportID = findLastAccessedReport(false, shouldOpenOnAdminRoom(), undefined, reportID)?.reportID;
if (lastAccessedReportID) {
const lastAccessedReportRoute = ROUTES.REPORT_WITH_ID.getRoute(lastAccessedReportID);
Navigation.navigate(lastAccessedReportRoute);
return;
}
navigateToConciergeChat(false, () => true);
return;
}

if (!report) {
    if (reportID) {
        const matchingIOUReport = Object.values(allReports).find(
            (report) => report?.iouReportID === reportID
        );

        if (matchingIOUReport) {
            Navigation.navigate(route as Route);
            return;
        }
    }
    // Continue with existing fallback logic...
}

What specific scenarios should we cover in automated tests to prevent reintroducing this issue in the future?

We should write a unit test to verify that navigation to IOU detail

What alternative solutions did you explore? (Optional)

We can add a new parameter to findLastAccessedReport to search for reports matching iouReportID. If findLastAccessedReport?.iouReportID === reportID, then we should navigate to the IOU details.

@daledah
Copy link
Contributor

daledah commented Mar 2, 2025

🚨 Edited by proposal-police: This proposal was edited at 2025-03-04 04:09:00 UTC.

Proposal

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

When navigating to the IOU details link, the 1:1 chat opens instead of the IOU details page

What is the root cause of that problem?

After signing in, our logic attempts to open a report:

App/src/libs/actions/Report.ts

Lines 2939 to 2953 in ec99650

// Check if the report exists in the collection
const report = allReports?.[`${ONYXKEYS.COLLECTION.REPORT}${reportID}`];
// If the report does not exist, navigate to the last accessed report or Concierge chat
if (!report) {
const lastAccessedReportID = findLastAccessedReport(false, shouldOpenOnAdminRoom(), undefined, reportID)?.reportID;
if (lastAccessedReportID) {
const lastAccessedReportRoute = ROUTES.REPORT_WITH_ID.getRoute(lastAccessedReportID);
Navigation.navigate(lastAccessedReportRoute);
return;
}
navigateToConciergeChat(false, () => true);
return;
}
Navigation.navigate(route as Route);

First, we check whether the report specified in the route exists. If it does, we open it directly. Otherwise, we attempt to retrieve a fallback report using the findLastAccessedReport function.

In this bug, the route’s report was considered nonexistent because its data originates from:

const report = allReports?.[`${ONYXKEYS.COLLECTION.REPORT}${reportID}`];

At this point, the allReports data does not include the thread report (IOU report) because BE cannot return all report data at once, causing the logic to fall back to opening the last accessed report instead, so the incorrect report is opened.

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

After the user signs in, we first attempt to fetch the report data from the route. If the retrieved data corresponds to a valid report, we should open it instead of falling back to the last accessed report.

To achieve this, we can modify the logic at:

const connection = Onyx.connect({

By adding a function to retrieve the report data from the route:

            let accessibleReport = {};
            openReport(reportID, '', [], undefined, '0');
            const reportConnection = Onyx.connect({
                key: `${ONYXKEYS.COLLECTION.REPORT}${reportID}`,
                waitForCollectionCallback: true,
                callback: (report) => {
                    if (report.reportID) {
                        accessibleReport = report;
                        Onyx.disconnect(reportConnection);
                    }
                },
            });

Then, update the condition at:

if (!report) {

                            if (!report && !accessibleReport?.reportID) {

This ensures that we properly check for report data before defaulting to the last accessed report.

What specific scenarios should we cover in automated tests to prevent reintroducing this issue in the future?

N / A

What alternative solutions did you explore? (Optional)

As mentioned in the RCA, this issue occurs because the openApp call at the following location:

is unable to fetch all report data at once.

To address this, I propose modifying the solution to send specific reportIDs via the openApp API to ensure the required reports are fetched. We have already implemented a similar approach for policies:

return getPolicyParamsForOpenOrReconnect().then((policyParams: PolicyParamsForOpenOrReconnect) => {

Since we cannot retrieve all policies in a single request, we send policyIDs to the backend, allowing it to return the necessary policy data for initialization. A similar strategy can be applied here to resolve the issue.

Update:

                App.openApp(getReportIDFromLink(initialURL));

with the initialURL comes from:

    const {initialURL} = useContext(InitialURLContext);

Then update this function:

function openApp() {

function openApp(reportID = '') {

so we can send the reportID to BE.

@eVoloshchak
Copy link
Contributor

Tested both solutions with the latest main, both of them do not work for me. @linhvovan29546, @daledah, could you please double check if this is still fixed for you? Are you using regular accounts or new/old ones?

@daledah
Copy link
Contributor

daledah commented Mar 3, 2025

@eVoloshchak I just reviewed my code changes and noticed a typo in the accessibleReport variable. I've corrected it—could you check again and verify if it works now?

I tested it from my side and it works

@eVoloshchak
Copy link
Contributor

I just reviewed my code changes and noticed a typo in the accessibleReport variable

Yeah, I noticed that too, I've tested with this fix already applied
This still opens an incorrect page for me (correct conversation, but it opens the group iou instead of the specific report)

Screen.Recording.2025-03-03.at.20.47.29.mov

@daledah
Copy link
Contributor

daledah commented Mar 4, 2025

@eVoloshchak Thanks! After testing, I noticed that my main solution works inconsistently, depending on network conditions.

To ensure a more reliable fix, I propose an alternative solution that properly addresses the issue.

Proposal updated

@eVoloshchak
Copy link
Contributor

@daledah, thanks, updated proposal looks good to me!

To address this, I propose modifying the solution to send specific reportIDs via the openApp API to ensure the required reports are fetched

I think we should proceed with alternative solution (if it requires no BE fix)

🎀👀🎀 C+ reviewed!

Copy link

melvin-bot bot commented Mar 5, 2025

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

@roryabraham
Copy link
Contributor

It used to be the case that we would send all data that was needed for OpenApp, but I think at some point it (unsurprisingly) became too much data, so we started just ... not sending everything. Overall it seems to me that change was made in haste, without fully considering the systems we would need for the front-end to detect missing data and fetch it as needed. It seems like the solution would require backend changes to support a reportID param for OpenApp, and it does seem like a kind of one-off/hacky solution. But a better solution essentially boils down to report pagination, which is a very complicated (but long overdue) project we can't tackle in the scope of this issue.

So I guess for now, the one-off solution works. 👍🏼

I need to look into the backend code and see what changes are needed 👀

@melvin-bot melvin-bot bot added the Overdue label Mar 8, 2025
Copy link

melvin-bot bot commented Mar 8, 2025

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

@sakluger
Copy link
Contributor

@roryabraham do you think we should move forward with the FE solution now, or wait until we've fixed the BE?

If we should move forward now with FE, then we can assign @daledah. If you think we should wait, then I will change the priority to weekly.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug Something is broken. Auto assigns a BugZero manager. Daily KSv2 External Added to denote the issue can be worked on by a contributor Help Wanted Apply this label when an issue is open to proposals by contributors Overdue
Projects
Status: Bugs and Follow Up Issues
Development

No branches or pull requests

8 participants