search

Expensify / App

Welcome to New Expensify: a complete re-imagination of financial collaboration, centered around chat. Help us build the next generation of Expensify by sharing feedback and contributing to the code.
https://new.expensify.com
MIT License
3.56k stars 2.9k forks source link

[HOLD for payment 2023-08-17] [$1000] Chat - Infinite loading after selecting flag option offline #23125

Closed lanitochka17 closed 1 year ago

lanitochka17 commented 1 year ago

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

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

Action Performed:

Precondition: user is logged in on two platforms with different accounts

  1. Open the app logged in with Account A
  2. On a different device, send a message from Account B to Account A
  3. Open the chat as Account A
  4. Turn off wifi or mobile data
  5. Tap the message and select "Reply in thread"
  6. Send a message in the thread
  7. Return to the report
  8. Tap the message and select "Flag as offensive"

Expected Result:

The reasons for flagging are displayed

Actual Result:

Infinite loading when selecting "Flag as offensive"

Workaround:

Unknown

Platforms:

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

Version Number: 1.3.42.18

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

Notes/Photos/Videos: Any additional supporting documentation

https://github.com/Expensify/App/assets/78819774/bfd625d0-0c20-4403-b2ab-0107e4888f31

Expensify/Expensify Issue URL:

Issue reported by: Applause - Internal Team

Slack conversation:

Upwork Automation - Do Not Edit
  • Upwork Job URL: https://www.upwork.com/jobs/~01d1f8e9ce942eb648
  • Upwork Job ID: 1681531600613777408
  • Last Price Increase: 2023-07-26
  • Automatic offers:
    • fedirjh | Reviewer | 25865636
    • dukenv0307 | Contributor | 25865637
melvin-bot[bot] commented 1 year ago

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

melvin-bot[bot] commented 1 year ago

Bug0 Triage Checklist (Main S/O)

Nodebrute commented 1 year ago

Proposal

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

Chat - Infinite loading after selecting flag option offline

What is the root cause of that problem?

When we open report isLoadingReportAction is true https://github.com/Expensify/App/blob/21d14c5135d186f2f3ba9dc0e8d53f071d43f80d/src/libs/actions/Report.js#L397

In flag comment this make this condition true hence the instead of page we have a loading indicator https://github.com/Expensify/App/blob/21d14c5135d186f2f3ba9dc0e8d53f071d43f80d/src/pages/FlagCommentPage.js#L160

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

Solution 1:

We can remove props.report.isLoadingReportActions from here https://github.com/Expensify/App/blob/21d14c5135d186f2f3ba9dc0e8d53f071d43f80d/src/pages/FlagCommentPage.js#L160

Or we can use && instead of ||

What alternative solutions did you explore? (Optional)

s-alves10 commented 1 year ago

Proposal

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

Selecting flag option shows infinite loading in offline mode

What is the root cause of that problem?

We're showing Loader here https://github.com/Expensify/App/blob/be7b6a09cde880cd96470e4e9e0c2f921499dd20/src/pages/FlagCommentPage.js#L150-L153

We set report.isLoadingReportActions to true when we open report and set isLoadingReportData when we reconnect app. In offline mode, openReport API isn't called but isLoadingReportActions is set optimistically. That's why we see the infinite loader.

This is from this PR

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

We have reportID and reportActionID(from the route). I think we should be able to flag comments as long as we have corresponding report and reportAction.

So I suggest to remove the above code completely(L150-L153)

This works as expected.

What alternative solutions did you explore? (Optional)

dukenv0307 commented 1 year ago

Proposal

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

Chat - Infinite loading after selecting flag option offline

What is the root cause of that problem?

Let's see the condition to display FullscreenLoadingIndicator https://github.com/Expensify/App/blob/5419d9ba7bb33347160ff20fc061e48d914eac6e/src/pages/FlagCommentPage.js#L160-L163

We are setting isLoadingReportActions: true in optimisticData of openReport API https://github.com/Expensify/App/blob/5419d9ba7bb33347160ff20fc061e48d914eac6e/src/libs/actions/Report.js#L397 but when offline the API is not sent so both successData and failureData are not updated. So, isLoadingReportActions still be true

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

https://github.com/Expensify/App/blob/5419d9ba7bb33347160ff20fc061e48d914eac6e/src/pages/FlagCommentPage.js#L160-L163 We need to show loadingPage here because in case the user access to FlagCommentPage by link, then there are 2 scenarios

  1. User has permission to flag a comment
  2. User has not permission to flag comments (in case the user wants to flag their own comment)

This is the reason why we need to show loadingPage while we wait for data and decide which scenario happen We only should show FullscreenLoadingIndicator if report is empty and isLoadingReportData is true or if reportActions is empty and isLoadingReportActions is true. Update like this code

const shouldShowLoading = (props.isLoadingReportData && isEmpty(props.report) || (isEmpty(props.reportActions) && props.report.isLoadingReportActions)

What alternative solutions did you explore? (Optional)

Access to FlagCommentPage by link only happens while the user is online so I suggest we only should show FullscreenLoadingIndicator while online, update like this code

const shouldShowLoading = (props.isLoadingReportData || props.report.isLoadingReportActions) && !props.network.isOffline
melvin-bot[bot] commented 1 year ago

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

melvin-bot[bot] commented 1 year ago

Current assignee @tjferriss is eligible for the External assigner, not assigning anyone new.

melvin-bot[bot] commented 1 year ago

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

eh2077 commented 1 year ago

Proposal

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

Infinite loading after selecting flag option offline.

What is the root cause of that problem?

The root cause of this issue is that

  1. We set optimistic isLoadingReportActions to true when open a report
  2. In offline mode, isLoadingReportActions of the report remains true, so following snippet will show the infinite loading

https://github.com/Expensify/App/blob/5419d9ba7bb33347160ff20fc061e48d914eac6e/src/pages/FlagCommentPage.js#L160-L163

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

The expected behaviours should be

  1. In offline mode, the flag options should be shown and is able to flag, which will update to backend after online.
  2. If report is loading, isLoadingReportData is true, we should show loading indicator
  3. If report or action to flag is empty, then we should show FullPageNotFoundView

To achieve it, we can

  1. Remove following snippet

    https://github.com/Expensify/App/blob/cc5526ba5ab448694e57d941a718dca2eca2174c/src/pages/FlagCommentPage.js#L160-L163

  2. Change this line

    https://github.com/Expensify/App/blob/cc5526ba5ab448694e57d941a718dca2eca2174c/src/pages/FlagCommentPage.js#L168

    to

    <FullPageNotFoundView shouldShow={_.isEmpty(props.report) || _.isEmpty(getActionToFlag()) || !ReportUtils.shouldShowFlagComment(getActionToFlag(), props.report)}>

  3. Add

    withReportOrNotFound,

    below this line

    https://github.com/Expensify/App/blob/cc5526ba5ab448694e57d941a718dca2eca2174c/src/pages/FlagCommentPage.js#L193

    and remove

    https://github.com/Expensify/App/blob/cc5526ba5ab448694e57d941a718dca2eca2174c/src/pages/FlagCommentPage.js#L202-L204

The usage of withReportOrNotFound and FullPageNotFoundView is similar to SplitBillDetailsPage.js

What alternative solutions did you explore? (Optional)

N/A

melvin-bot[bot] commented 1 year ago

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

fedirjh commented 1 year ago

Reviewing shortly.

rk111 commented 1 year ago

Proposal

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

Chat - Infinite loading after selecting flag option offline

What is the root cause of that problem?

FullscreenLoadingIndicator on the FlagCommentPage is not handling the scenario where the user is offline and the API call is not sent. Specifically, when the user is offline, the API call to update the report actions is not made, and as a result, the isLoadingReportActions flag remains true, causing the loading indicator to persist indefinitely.

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

Update the condition for shouldShowLoading to consider the scenario where the user is offline and the API call is not made. We should show the loadingPage when either of the following conditions is true: a. isLoadingReportData is true, and the report data is empty (props.isLoadingReportData && isEmpty(props.report)) b. isLoadingReportActions is true, and the reportActions data is empty (isEmpty(props.reportActions) && props.report.isLoadingReportActions)

To address the scenario where the user accesses the FlagCommentPage by link while online, show the FullscreenLoadingIndicator only when the user is online. Update the condition to include a check for the network status: (props.isLoadingReportData || props.report.isLoadingReportActions) && !props.network.isOffline

melvin-bot[bot] commented 1 year ago

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

tjferriss commented 1 year ago

Easy, MelvinBot. @fedirjh is reviewing proposals.

melvin-bot[bot] commented 1 year ago

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

fedirjh commented 1 year ago

@dukenv0307 It seems to be a regression from your PR, I have raised a question in the PR about the FullscreenLoadingIndicator, Can you please share any inputs on why it was implemented?

dukenv0307 commented 1 year ago

@fedirjh I replied in the PR. Can you check again.

dukenv0307 commented 1 year ago

@fedirjh The fix that I mentioned in the PR is my proposal of this issue. What do you think about it?

fedirjh commented 1 year ago

This will not work in offline mode when we do something to make the openReport API is called

@dukenv0307 This doesn’t looks like a valid use case.

dukenv0307 commented 1 year ago

@fedirjh In offline the API will be added in the queue, when openAPI is added in the queue optimisticData is merged in Onyx that makes isLoadingReportAction is true and then this issue occurs.

fedirjh commented 1 year ago

In offline the API will be added in the queue, when openAPI is added in the queue optimisticData is merged in Onyx that makes isLoadingReportAction is true and then this issue occurs.

Are we sure we are referring to the same thing? Regardless of the API call, when we are offline, we have persisted data in Onyx, we should check that data instead of awaiting new data.

This will not work in offline mode when we do something to make the openReport API is called

Can you list a valid use case? why it will not work in offline mode?

dukenv0307 commented 1 year ago

This will not work in offline mode when we do something to make the openReport API is called

@fedirjh
I mean the old check that I added in the PR will not work in offline when the openReport API is added in the queue and we should change the check like this 

const shouldShowLoading = (props.isLoadingReportData && isEmpty(props.report)) || (isEmpty(props.reportActions) && props.report.isLoadingReportActions)

we should check that data instead of awaiting new data.

We should keep the check props.isLoadingReportData and isEmpty(props.reportActions) because If the reportID or reportActionID is not found the page will load infinite because both report and reportAction data is empty.

fedirjh commented 1 year ago

We should keep the check props.isLoadingReportData and isEmpty(props.reportActions) because If the reportID or reportActionID is not found the page will load infinite because both report and reportAction data is empty.

We shouldn’t use the FullscreenLoadingIndicator, that was my first question , if we have data then we display the component else we display the not found view.

dukenv0307 commented 1 year ago

We shouldn’t use the FullscreenLoadingIndicator

@fedirjh We have a problem when we do not use FullscreenLoadingIndicator like this video. The page not found will display for first because the report and reportAction is loading from the API.

Screencast from 27-07-2023 20:27:06.webm

fedirjh commented 1 year ago

@dukenv0307 I think we have the WithReportOrNotFound HOC for that purpose ?

dukenv0307 commented 1 year ago

@fedirjh This HOC is not enough because we also need to wait reportAction data to display not found page or flag page correctly.

fedirjh commented 1 year ago

This HOC is not enough because we also need to wait reportAction data to display not found page or flag page correctly.

Isn’t the report and reportActions loaded in the same request ? Have we implemented the pagination yet ?

dukenv0307 commented 1 year ago

Report data will be not empty in API openApp and reportAction is loaded in openReport API

fedirjh commented 1 year ago

Thanks everyone for the proposals, it looks like we should proceed with @dukenv0307's proposal. We should display the FullscreenLoadingIndicator when there is no data in Onyx and we are loading the report.

🎀 👀 🎀 C+ reviewed

melvin-bot[bot] commented 1 year ago

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

eh2077 commented 1 year ago

@fedirjh Thanks for reviewing proposals. Can I have your feedback about my proposal.

I think it makes sense to display the not found page if report action is not yet in Onyx regarding the data flow guides. The not found page will be updated to flag page if openReport API return new report action data. The transition should be expected right?

If we access by deep link of the flag page, like http://localhost:8082/flag/8132130769751654/3782505051996183681 there're following cases to be considered

  1. Online or offline
  2. OpenApp or ReconnectApp API is called where the optimistic isLoadingReportData is set to true
  3. Report 8132130769751654 exists in Onyx or not
  4. Report Action 3782505051996183681 exists in Onyx or not
  5. ReportUtils.shouldShowFlagComment or not
fedirjh commented 1 year ago

Thanks for reviewing proposals. Can I have your feedback about my https://github.com/Expensify/App/issues/23125#issuecomment-1643567134.

@eh2077 Your proposal doesn’t cover the case when we don't have report action and we deep link, it will display 'not found' view for a brief moment before rendering the flag comment component and that's not expected.

The transition should be expected right?

Nope, it doesn’t make sense to display not found then found, it should be loading then found or not found. The expected transition is that we display a skeleton view until the data is loaded and given that we have not implemented that yet, we should display the loading indicator.

srikarparsi commented 1 year ago

Looks good to me as well, thanks @fedirjh and @dukenv0307!

melvin-bot[bot] commented 1 year ago

📣 @fedirjh 🎉 An offer has been automatically sent to your Upwork account for the Reviewer role 🎉 Thanks for contributing to the Expensify app!

Offer link Upwork job

melvin-bot[bot] commented 1 year ago

📣 @dukenv0307 🎉 An offer has been automatically sent to your Upwork account for the Contributor role 🎉 Thanks for contributing to the Expensify app!

Offer link Upwork job Please accept the offer and leave a comment on the Github issue letting us know when we can expect a PR to be ready for review 🧑‍💻 Keep in mind: Code of Conduct | Contributing 📖

dukenv0307 commented 1 year ago

@fedirjh The PR is ready for review.

fedirjh commented 1 year ago

Update: We are currently facing a similar issue that also impacts the split bill detail page #23568. In this PR, we plan to introduce a new HOC named withReportAndReportActionOrNotFound. This HOC will contain the same code as the one mentioned in this issue. To prevent redundant code, we will hold the PR until the new HOC is created, and then we will use it within the FlagCommentPage page.

melvin-bot[bot] commented 1 year ago

Based on my calculations, the pull request did not get merged within 3 working days of assignment. Please, check out my computations here:

On to the next one 🚀

dukenv0307 commented 1 year ago

@fedirjh @srikarparsi This PR is on hold from August 1 to August 4. So I think we are eligible for a bonus timeline

melvin-bot[bot] commented 1 year ago

Reviewing label has been removed, please complete the "BugZero Checklist".

melvin-bot[bot] commented 1 year ago

The solution for this issue has been :rocket: deployed to production :rocket: in version 1.3.52-5 and is now subject to a 7-day regression period :calendar:. Here is the list of pull requests that resolve this issue:

If no regressions arise, payment will be issued on 2023-08-17. :confetti_ball:

After the hold period is over and BZ checklist items are completed, please complete any of the applicable payments for this issue, and check them off once done.

For reference, here are some details about the assignees on this issue:

As a reminder, here are the bonuses/penalties that should be applied for any External issue:

melvin-bot[bot] commented 1 year ago

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:

tjferriss commented 1 year ago

@fedirjh and @dukenv0307 are both hired in Upworks: https://www.upwork.com/ab/applicants/1681531600613777408/hired. We're waiting on the regression period now.

@fedirjh can you get started on the checklist when you have a minute?

fedirjh commented 1 year ago

BugZero Checklist:

Regression Test Proposal

  1. From user A, send a new message to user B
  2. From user B, go offline
  3. Open a different report than user A
  4. Go back to the report with user A
  5. Tap the message and select "Flag as offensive"
  6. Verify that the flag page appears instead of loading infinitely

Do we agree 👍 or 👎

tjferriss commented 1 year ago

The payments have been sent via Upworks and QA issue created.

dukenv0307 commented 12 months ago

@tjferriss Can you please check my comment above https://github.com/Expensify/App/issues/23125#issuecomment-1669039819.

tjferriss commented 12 months ago

@dukenv0307 yes, that looks right. I've paid the bonus to both of you via Upworks.