App icon indicating copy to clipboard operation
App copied to clipboard

[$500] mWeb/Chrome - Distance-In offline, distance request details page loading infinitely.

Open lanitochka17 opened this issue 1 year ago • 12 comments

If you haven’t already, check out our contributing guidelines for onboarding and email [email protected] to request to join our Slack channel!


Version Number: 1.4.31-2 Reproducible in staging?: Y Reproducible in production?: Y 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: Applause - Internal Team Slack conversation:

Issue found when executing PR https://github.com/Expensify/App/pull/34604

Action Performed:

  1. Go to https://staging.new.expensify.com/
  2. Go offline
  3. Tap on Workspace chat which has distance request created
  4. Tap on distance request to open distance request details page

Expected Result:

In offline, distance request details page must be displayed without loading

Actual Result:

In offline, distance request details page loading infinitely

Workaround:

Unknown

Workaround:

Platforms:

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

  • [ ] Android: Native
  • [x] Android: mWeb Chrome
  • [ ] iOS: Native
  • [ ] iOS: mWeb Safari
  • [ ] MacOS: Chrome / Safari
  • [ ] MacOS: Desktop

Screenshots/Videos

Add any screenshot/video evidence

https://github.com/Expensify/App/assets/78819774/878bd4aa-e457-4bee-a6f2-43e1b429fe79

View all open jobs on GitHub

Upwork Automation - Do Not Edit
  • Upwork Job URL: https://www.upwork.com/jobs/~01ed39265b86a00161
  • Upwork Job ID: 1750223436772130816
  • Last Price Increase: 2024-01-31

lanitochka17 avatar Jan 24 '24 18:01 lanitochka17

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

melvin-bot[bot] avatar Jan 24 '24 18:01 melvin-bot[bot]

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

melvin-bot[bot] avatar Jan 24 '24 18:01 melvin-bot[bot]

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

melvin-bot[bot] avatar Jan 24 '24 18:01 melvin-bot[bot]

Bringing the proposal from here

Proposal

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

In offline, the distance request details page must be displayed without loading

What is the root cause of that problem?

As there is a purposeful reduction in data sent from BE in OpenApp, the report actions for a displayed report in LHN need not be present. This would bring about interesting scenarios in offline cases.

  1. If the user is offline at this moment, the report screen will keep showing the loading status for the chat report. But, when online, the chat report actions would be fetched via openReport if the user clicks on the report in LHN.
  2. Now, if the user goes offline after fetching the chat report, a similar loading status will be shown for the IOU report when clicked.
  3. The same situation will repeat for the transaction report as well.

When offline, is showing a loading status in each of the above three cases intentional?

If not intentional, the proposal below can be considered.

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

To solve the problem, we can ensure that the displayed reports in LHN are fetched along with their linked reports up to a single level depth.

More specifically, we can leverage optionListItems here and ensure that all these reports are fetched once at app launch. Additionally, we can look for childReportID in Report Actions of these reports which is a good indicator of linked reports, and fetch them. This will solve points (2) and (3) mentioned in the root cause. For solving (1), we need to discuss how to arrive at this as BE support may be required. However, when minimalistic report action is sent on openApp, we can additionally look for linkedReportID.

We can implement a side effect here which looks something like this to solve the problem:

const fetchedReportIDsRef = useRef([]);
useEffect(() => {
    const arrayChildReportIDs = [];
    optionListItems.forEach((reportID) => {
        const reportActions = allReportActions[`${ONYXKEYS.COLLECTION.REPORT_ACTIONS}${reportID}`];
        const childReportIDs = _.filter(_.pluck(reportActions, 'childReportID'),(childReportID) => childReportID);
        if(childReportIDs.length > 0) {
            arrayChildReportIDs.push(childReportIDs);
        } 
    });
    const allRecentReportIDs = _.union(optionListItems, ...arrayChildReportIDs);
    if(currentReportID) {
        allRecentReportIDs.push(currentReportID);
    }

    const unfetchedReportIDs = _.difference(allRecentReportIDs, fetchedReportIDsRef.current);
    unfetchedReportIDs.forEach((reportID) => {
        if(!reportID) {
            return;
        }
        Report.reconnect(reportID);
        fetchedReportIDsRef.current.push(reportID);
    });        
}, [currentReportID, optionListItems, chatReports, betas, policies, priorityMode, allReportActions]);  

Additionally, we also need to include childReportID in reportActionsSelector here

What alternative solutions did you explore? (Optional)

rojiphil avatar Jan 25 '24 05:01 rojiphil

As there is a purposeful reduction in data sent from BE in OpenApp, the report actions for a displayed report in LHN need not be present. This would bring about interesting scenarios in offline cases.

  1. If the user is offline at this moment, the report screen will keep showing the loading status for the chat report. But, when online, the chat report actions would be fetched via openReport if the user clicks on the report in LHN.
  2. Now, if the user goes offline after fetching the chat report, a similar loading status will be shown for the IOU report when clicked.
  3. The same situation will repeat for the transaction report as well.

When offline, is showing a loading status in each of the above three cases intentional?

@sophiepintoraetz @parasharrajat Can you please help clarify the above query as mentioned in my proposal? Also, what would be the expected behaviour here?

rojiphil avatar Jan 25 '24 10:01 rojiphil

I will take a look. Currently away from laptop.

parasharrajat avatar Jan 25 '24 13:01 parasharrajat

Still waiting on @parasharrajat

sophiepintoraetz avatar Jan 30 '24 00:01 sophiepintoraetz

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

melvin-bot[bot] avatar Jan 31 '24 16:01 melvin-bot[bot]

I will checking this today,

parasharrajat avatar Jan 31 '24 17:01 parasharrajat

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

melvin-bot[bot] avatar Feb 01 '24 23:02 melvin-bot[bot]

@johncschuster, @parasharrajat Uh oh! This issue is overdue by 2 days. Don't forget to update your issues!

melvin-bot[bot] avatar Feb 05 '24 15:02 melvin-bot[bot]

@parasharrajat, did you get a chance to look at the above proposal?

johncschuster avatar Feb 05 '24 23:02 johncschuster

@johncschuster @parasharrajat this issue was created 2 weeks ago. Are we close to approving a proposal? If not, what's blocking us from getting this issue assigned? Don't hesitate to create a thread in #expensify-open-source to align faster in real time. Thanks!

melvin-bot[bot] avatar Feb 07 '24 15:02 melvin-bot[bot]

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

melvin-bot[bot] avatar Feb 07 '24 16:02 melvin-bot[bot]

@mananjadhav is taking over C+ review while Rajat is away

johncschuster avatar Feb 08 '24 21:02 johncschuster

Just took over going through the issue history, and the linked issue discussion where the proposal was originally posted.

mananjadhav avatar Feb 08 '24 21:02 mananjadhav

@mananjadhav, @johncschuster Uh oh! This issue is overdue by 2 days. Don't forget to update your issues!

melvin-bot[bot] avatar Feb 12 '24 15:02 melvin-bot[bot]

@lanitochka17 @rojiphil Can you confirm if this is reproducible in all platforms? I can see only Android mWeb marked? If it's specific platform, then I am wondering how can backend have impact on one platform only?

mananjadhav avatar Feb 14 '24 03:02 mananjadhav

@mananjadhav @johncschuster 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!

melvin-bot[bot] avatar Feb 14 '24 15:02 melvin-bot[bot]

Issue is not reproducible on the latest build 1.4.41-4

https://github.com/Expensify/App/assets/78819774/aacb03b8-1913-457d-b4df-f6f417e2e7c7

lanitochka17 avatar Feb 14 '24 17:02 lanitochka17

I've applied the retest-weekly label. If we find this behavior is no longer present on the next retest, I think we can close it.

johncschuster avatar Feb 16 '24 23:02 johncschuster

@mananjadhav @johncschuster this issue is now 4 weeks old and preventing us from maintaining WAQ, can you:

  • Decide whether any proposals currently meet our guidelines and can be approved as-is today
  • If no proposals meet that standard, please take this issue internal and treat it as one of your highest priorities
  • If you have any questions, don't hesitate to start a discussion in #expensify-open-source

Thanks!

melvin-bot[bot] avatar Feb 21 '24 15:02 melvin-bot[bot]

Current assignee @mananjadhav is eligible for the Internal assigner, not assigning anyone new.

melvin-bot[bot] avatar Feb 21 '24 15:02 melvin-bot[bot]

@mananjadhav, @johncschuster Eep! 4 days overdue now. Issues have feelings too...

melvin-bot[bot] avatar Feb 26 '24 15:02 melvin-bot[bot]

This issue has the retest label applied. Coming from this process, it looks like Applause will re-test by Friday and comment with their results on Monday.

johncschuster avatar Feb 26 '24 22:02 johncschuster

That said, this should have been tested this previous Friday and the results should be posted today-ish. @Expensify/applause, did this issue get retested in the last batch? If not, can you please add it? Thank you!

johncschuster avatar Feb 26 '24 22:02 johncschuster

@johncschuster Updating results right now. Give me a couple minutes. Working on it

mvtglobally avatar Feb 27 '24 14:02 mvtglobally

Issue is reproducible during KI retests.

mvtglobally avatar Feb 27 '24 16:02 mvtglobally

I took a look and I was able to reproduce the bug as described by the test steps. Are we sure that it's specific to distance requests? Does it happen for manual or scan requests with the same steps?

Please assign me once it's time to review a proposal.

neil-marcellini avatar Mar 01 '24 03:03 neil-marcellini

@mananjadhav, @johncschuster Uh oh! This issue is overdue by 2 days. Don't forget to update your issues!

melvin-bot[bot] avatar Mar 01 '24 15:03 melvin-bot[bot]