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

[$500] Thread – Draft message disappears after page view finishes loading. #31705

Closed
3 of 6 tasks
kbecciv opened this issue Nov 22, 2023 · 53 comments
Closed
3 of 6 tasks
Assignees
Labels
Bug Something is broken. Auto assigns a BugZero manager. Daily KSv2 Engineering Internal Requires API changes or must be handled by Expensify staff retest-weekly Apply this label if you want this issue tested on a Weekly basis by Applause

Comments

@kbecciv
Copy link

kbecciv commented Nov 22, 2023

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.4.2.0
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:

Action Performed:

  1. Go to https://staging.new.expensify.com/
  2. Log in with any account that has access to a workspace
  3. Go to global create > request money > choose a workspace > confirm
  4. Click on the report preview component in the workspace chat to access the report view
  5. Right click on the request preview component and select "Reply in thread"
  6. Start typing quickly before the skeleton UI finishes loading the request view
  7. Once loaded, observe the message disappears.

Expected Result:

Draft message should remain in the compose box.

Actual Result:

Draft message disappears after the request page view loads.

Workaround:

Wait until it loads before typing.

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

Add any screenshot/video evidence

Bug6287214_1700660919011.Draft_of_message_disappears.mp4

View all open jobs on GitHub

Upwork Automation - Do Not Edit
  • Upwork Job URL: https://www.upwork.com/jobs/~019569b231b07825eb
  • Upwork Job ID: 1727331482736373760
  • Last Price Increase: 2023-11-22
@kbecciv kbecciv added External Added to denote the issue can be worked on by a contributor Daily KSv2 Bug Something is broken. Auto assigns a BugZero manager. labels Nov 22, 2023
@melvin-bot melvin-bot bot changed the title Thread – Draft of message disappears after Thread page upload. [$500] Thread – Draft of message disappears after Thread page upload. Nov 22, 2023
Copy link

melvin-bot bot commented Nov 22, 2023

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

Copy link

melvin-bot bot commented Nov 22, 2023

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

@melvin-bot melvin-bot bot added the Help Wanted Apply this label when an issue is open to proposals by contributors label Nov 22, 2023
Copy link

melvin-bot bot commented Nov 22, 2023

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

Copy link

melvin-bot bot commented Nov 22, 2023

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

@tienifr
Copy link
Contributor

tienifr commented Nov 22, 2023

Proposal

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

When the isReportReadyForDisplay is false, we show the ReportFooter with the default report ({reportId: '0'}). When users start to type, it's saved into reportDraftComment_0

<ReportFooter isReportReadyForDisplay={false} />

so when isReportReadyForDisplay is true, we get the draft value from reportDraftComment_{id} so it's empty

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

in ReportScreen, when isReportReadyForDisplay is false, we still have the optimistic reportId in route, we can get it and pass to ReportFooter

<ReportFooter isReportReadyForDisplay={false} draftReportID={getReportID(route)}/>

Then use it in ComposerWithSuggestions when reportID is '0'

@esh-g
Copy link
Contributor

esh-g commented Nov 22, 2023

Proposal

Please re-state the problem

Thread – Draft of message disappears after Thread page upload.

What is root cause?

There are two issue here:

  1. The IOU child report is not being generated optimistically unlike threads for a comment.
  2. The composer allows setting the draft even though the reportID is 0.

Unlike normal comments, IOU actions have a childReportID that is set by the backend when the action is created (we don't generate it optimistically). This means that if we create a request online and then go offline and click on the request, we are greeted with an infinite loading screen

Screen.Recording.2023-11-22.at.8.56.34.PM.mov

What changes should be made to fix this?

Options for first issue

Option 1
The backend should not generate the child report for the request as it is not generated on the front-end and the backend should not return the childReportID field so that it can be generated optimistically in the front-end when required.

Option 2
Instead of generating the child report on the backend, we can add the childReportID on the front-end itself and build the child report optimistically for every money request in the IOU.getMoneyRequestInformation() method itself.


Solution for second issue

We can pass the report to the ReportFooter even when the report is not loaded here from the route itself:

<ReportFooter isReportReadyForDisplay={false} />

<ReportFooter isReportReadyForDisplay={false} report={{reportID: getReportID(route)}} />

This prevents the need for adding any additional prop as well.

@trjExpensify trjExpensify changed the title [$500] Thread – Draft of message disappears after Thread page upload. [$500] Thread – Draft message disappears after page view finishes loading. Nov 23, 2023
@trjExpensify
Copy link
Contributor

Interesting! I can reproduce this reliably!

@melvin-bot melvin-bot bot added the Overdue label Nov 27, 2023
@trjExpensify
Copy link
Contributor

Awaiting your input @thesahindia!

@melvin-bot melvin-bot bot removed the Overdue label Nov 27, 2023
@thesahindia
Copy link
Member

@esh-g's proposal looks good to me!

🎀 👀 🎀 C+ reviewed

Copy link

melvin-bot bot commented Nov 28, 2023

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

@esh-g
Copy link
Contributor

esh-g commented Nov 28, 2023

@thesahindia Which option in the proposal should we go with for the first issue?

Copy link

melvin-bot bot commented Dec 1, 2023

@puneetlath, @trjExpensify, @thesahindia Whoops! This issue is 2 days overdue. Let's get this updated quick!

@trjExpensify
Copy link
Contributor

Bump @thesahindia on the question!

@melvin-bot melvin-bot bot removed the Overdue label Dec 4, 2023
@thesahindia
Copy link
Member

@thesahindia Which option in the proposal should we go with for the first issue?

I think option 1.

cc: @puneetlath for thoughts.

Copy link

melvin-bot bot commented Dec 6, 2023

@puneetlath @trjExpensify @thesahindia 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 melvin-bot bot added the Overdue label Dec 8, 2023
@trjExpensify
Copy link
Contributor

Yup! I think it's #vip-vsb this one as threads related, so popping it there while we look into it again.

Can anyone reproduce? I tried on prod and I was able to get a slow enough load of an expense thread to repro it, but on staging I haven't been able to run into a slow enough load to be able to. CC: @esh-g

@esh-g
Copy link
Contributor

esh-g commented Mar 5, 2024

@trjExpensify The problem still is present. It just happens if you have really slow internet or you are offline and then go online.

Screen.Recording.2024-03-05.at.5.55.36.PM.mov

@esh-g
Copy link
Contributor

esh-g commented Mar 5, 2024

Proposal

What is the root cause of the problem?

The root cause is that when creating a money request offline, we generate an optimistic transaction report but when the money request is actually created after going online, the optimistic report is no longer relevant as the report that backend returns has an entirely new id and basically the optimistic report never gets created on the backend, thus we see the composer disappear when we go online as it generates an Auth OpenReport error.

Screen.Recording.2024-03-05.at.6.04.32.PM.mov

I think this was done as a compromise to have an optimistic report to show until it was created on the backend. But this leads to issues like messages sent on the optimistic report being deleted as well.

What changes should be made to fix this?

We should disable the composer for the optimistic transaction report because if the optimistic report never exists on the backend, it should never be able to be written to by the user. Therefore, we can hide the composer if there is a pending create action on the money report the same way we do if there is a pending delete action here:

App/src/libs/ReportUtils.ts

Lines 4518 to 4521 in 9411a08

// If the Money Request report is marked for deletion, let us prevent any further write action.
if (isMoneyRequestReportPendingDeletion(report)) {
return false;
}

Screenshot 2024-03-05 at 6 11 49 PM

This would disable the composer for the optimistic report just like it does when we go online so that the user isn't sending messages to a report that will never have any existence on the backend.

@melvin-bot melvin-bot bot added Daily KSv2 and removed Weekly KSv2 labels Mar 5, 2024
@esh-g
Copy link
Contributor

esh-g commented Mar 5, 2024

The problem I think has changed a little from it's root cause... So I think a new solution as proposed above is needed.
cc @thesahindia

@trjExpensify
Copy link
Contributor

Oh wow, so the composer disappears completely as well?

We should disable the composer for the optimistic transaction report because if the optimistic report never exists on the backend, it should never be able to be written to by the user.

I don't really like the sound of that. To the user, they should be able to comment on reports when offline with no difference.

@cead22
Copy link
Contributor

cead22 commented Mar 8, 2024

The childReportID issue should get fixed by this PR #37232. Once that's fixed, will we still be saving drafts on reportID 0?

@melvin-bot melvin-bot bot added the Overdue label Mar 11, 2024
@trjExpensify
Copy link
Contributor

Ah, at that point the draft message nor the composer would disappear? 🤔 Should we wait for that PR to be done and retest this then?

@melvin-bot melvin-bot bot added Overdue and removed Overdue labels Mar 11, 2024
@trjExpensify
Copy link
Contributor

Hit prod 5 hours ago. @esh-g we're ready for a re-test.

@melvin-bot melvin-bot bot added Overdue and removed Overdue labels Mar 14, 2024
@trjExpensify
Copy link
Contributor

Bump, @esh-g. If you think this issue is still present, do let us know! In the meantime, I'm going to add the retest weekly label for Applause.

@melvin-bot melvin-bot bot removed the Overdue label Mar 18, 2024
@trjExpensify trjExpensify added Overdue retest-weekly Apply this label if you want this issue tested on a Weekly basis by Applause and removed Overdue labels Mar 18, 2024
@melvin-bot melvin-bot bot added the Overdue label Mar 25, 2024
@trjExpensify
Copy link
Contributor

Awaiting the Applause retest.

@melvin-bot melvin-bot bot removed the Overdue label Mar 25, 2024
@mvtglobally
Copy link

Issue not reproducible during KI retests. (First week)

@melvin-bot melvin-bot bot added the Overdue label Mar 28, 2024
@puneetlath
Copy link
Contributor

Sounds like this is no longer happening so I'm going to close. Feel free to let me know if I'm mistaken!

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 Engineering Internal Requires API changes or must be handled by Expensify staff retest-weekly Apply this label if you want this issue tested on a Weekly basis by Applause
Projects
No open projects
Status: CRITICAL
Development

No branches or pull requests

8 participants