-
Notifications
You must be signed in to change notification settings - Fork 3k
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
[HOLD for payment 2023-08-17] [$1000] Chat - Infinite loading after selecting flag option offline #23125
Comments
Triggered auto assignment to @tjferriss ( |
Bug0 Triage Checklist (Main S/O)
|
ProposalPlease 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 App/src/libs/actions/Report.js Line 397 in 21d14c5
In flag comment this make this condition true hence the instead of page we have a loading indicator App/src/pages/FlagCommentPage.js Line 160 in 21d14c5
What changes do you think we should make in order to solve the problem?Solution 1: We can remove App/src/pages/FlagCommentPage.js Line 160 in 21d14c5
Or we can use What alternative solutions did you explore? (Optional) |
ProposalPlease 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 App/src/pages/FlagCommentPage.js Lines 150 to 153 in be7b6a0
We set 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) |
ProposalPlease 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 App/src/pages/FlagCommentPage.js Lines 160 to 163 in 5419d9b
We are setting isLoadingReportActions: true in optimisticData of openReport API App/src/libs/actions/Report.js Line 397 in 5419d9b
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?App/src/pages/FlagCommentPage.js Lines 160 to 163 in 5419d9b
We need to show loadingPage here because in case the user access to FlagCommentPage by link, then there are 2 scenarios
This is the reason why we need to show loadingPage while we wait for data and decide which scenario happen
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
|
Job added to Upwork: https://www.upwork.com/jobs/~01d1f8e9ce942eb648 |
Current assignee @tjferriss is eligible for the External assigner, not assigning anyone new. |
Triggered auto assignment to Contributor-plus team member for initial proposal review - @fedirjh ( |
ProposalPlease 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
App/src/pages/FlagCommentPage.js Lines 160 to 163 in 5419d9b
What changes do you think we should make in order to solve the problem?The expected behaviours should be
To achieve it, we can
The usage of What alternative solutions did you explore? (Optional)N/A |
@tjferriss, @fedirjh Whoops! This issue is 2 days overdue. Let's get this updated quick! |
Reviewing shortly. |
ProposalPlease 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: 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: |
@tjferriss, @fedirjh Whoops! This issue is 2 days overdue. Let's get this updated quick! |
Easy, MelvinBot. @fedirjh is reviewing proposals. |
📣 It's been a week! Do we have any satisfactory proposals yet? Do we need to adjust the bounty for this issue? 💸 |
@dukenv0307 It seems to be a regression from your PR, I have raised a question in the PR about the |
@fedirjh I replied in the PR. Can you check again. |
@fedirjh The fix that I mentioned in the PR is my proposal of this issue. What do you think about it? |
@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.
Nope, it doesn’t make sense to display |
Looks good to me as well, thanks @fedirjh and @dukenv0307! |
📣 @fedirjh 🎉 An offer has been automatically sent to your Upwork account for the Reviewer role 🎉 Thanks for contributing to the Expensify app! |
📣 @dukenv0307 🎉 An offer has been automatically sent to your Upwork account for the Contributor role 🎉 Thanks for contributing to the Expensify app! Offer link |
@fedirjh The PR is ready for review. |
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 |
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 🚀 |
@fedirjh @srikarparsi This PR is on hold from August 1 to August 4. So I think we are eligible for a bonus timeline |
|
The solution for this issue has been 🚀 deployed to production 🚀 in version 1.3.52-5 and is now subject to a 7-day regression period 📆. Here is the list of pull requests that resolve this issue: If no regressions arise, payment will be issued on 2023-08-17. 🎊 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:
|
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:
|
@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? |
BugZero Checklist:
Regression Test Proposal
Do we agree 👍 or 👎 |
The payments have been sent via Upworks and QA issue created. |
@tjferriss Can you please check my comment above #23125 (comment). |
@dukenv0307 yes, that looks right. I've paid the bonus to both of you via Upworks. |
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 #22771
Action Performed:
Precondition: user is logged in on two platforms with different accounts
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
Bug6132837_IMG_0533.mp4
Expensify/Expensify Issue URL:
Issue reported by: Applause - Internal Team
Slack conversation:
Upwork Automation - Do Not Edit
The text was updated successfully, but these errors were encountered: