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] Editing a message with an image attached before image loading completed never loads the attachments #42300

Closed
1 of 6 tasks
m-natarajan opened this issue May 16, 2024 · 17 comments
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 Not a priority

Comments

@m-natarajan
Copy link

m-natarajan commented May 16, 2024

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.74-4
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: @jliexpensify
Slack conversation: https://expensify.slack.com/archives/C049HHMV9SM/p1715827355076719

Action Performed:

  1. Write a message, then upload an image
  2. Immediately click Edit (as image is still uploading)
  3. Add more letters to the message and save

Expected Result:

Image attachment is loaded

Actual Results:

Image never uploads, and instead shows uploading attachment

Workaround:

unknown

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
image (1)

Recording.74.mp4

View all open jobs on GitHub

Upwork Automation - Do Not Edit
  • Upwork Job URL: https://www.upwork.com/jobs/~01737e481166a622be
  • Upwork Job ID: 1791515161526546432
  • Last Price Increase: 2024-09-26
Issue OwnerCurrent Issue Owner: @hungvu193
@m-natarajan m-natarajan added Daily KSv2 Bug Something is broken. Auto assigns a BugZero manager. labels May 16, 2024
Copy link

melvin-bot bot commented May 16, 2024

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

@twisterdotcom twisterdotcom added the External Added to denote the issue can be worked on by a contributor label May 17, 2024
@melvin-bot melvin-bot bot changed the title Editing a message with an image attached before image loading completed never loads the attachments [$250] Editing a message with an image attached before image loading completed never loads the attachments May 17, 2024
Copy link

melvin-bot bot commented May 17, 2024

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

@melvin-bot melvin-bot bot added the Help Wanted Apply this label when an issue is open to proposals by contributors label May 17, 2024
Copy link

melvin-bot bot commented May 17, 2024

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

@hungvu193
Copy link
Contributor

Waiting for proposals

@beodw
Copy link

beodw commented May 18, 2024

Proposal

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

Editing a message with an image attached before image loading completed never loads the attachments

What is the root cause of that problem?

In the network tab we can see that the AddTextAndAttachement endpoint returns 200 but after the UpdateComment is called the returned data to merge with onyx does not contain the html with . This seems to be a backend thing. The call to UpdateComment seems to erase the uploaded file if it is not done uploading.

Screen.Recording.2024-05-18.at.14.30.03.mp4

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

There are a few options:

  1. We can decide to temporarily prevent editing a comment when the file is uploading but this goes against the offline first principle of the app.
  2. Another thing we can do is to make the network call to UploadComment only occurs after the file has finished uploading to the backend and the attachment has rendered while preemptively updating the UI with the new comment. @hungvu193

What alternative solutions did you explore? (Optional)

This could alternatively be prevented on the backend.

@hungvu193
Copy link
Contributor

hungvu193 commented May 18, 2024

Thanks for your proposal @beodw
I personally think this should be handled in the BE.

Cc @jliexpensify

@dominictb
Copy link
Contributor

@hungvu193 This cannot be fixed in the back-end because the back-end won't be able to know if the Uploading attachment... actually represents an "uploading attachment" or if the user manually changes the text to Uploading attachment...

Proposal

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

  1. Write a message, then upload an image
  2. Immediately click Edit (as image is still uploading)
  3. Add more letters to the message and save
    -> Image never uploads, and instead shows uploading attachment

What is the root cause of that problem?

When we edit the message, we will send

Text
Uploading attachment...

as HTML to the back-end, so the back-end assumes that we want to change the text as such and updates the comment to

Text
Uploading attachment...

Without considering that the Uploading attachment... is an uploading attachment and should be replaced by the image HTML

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

When we receives the response from the back-end of AddTextAndAttachment command, after applying the successData:

  • Extract the image HTML part that's present in the request response
  • Look for UpdateComment commands that are still in the request queue
  • If there's the Uploading attachment... text at the end of the message data of those UpdateComment commands, replace them with the image HTML in the first step
  • Optionally update the report action message data in Onyx to replace Uploading attachment... with image HTML too

Another optional improvement is we can make the Uploading attachment... more specific, like Uploading Filename.jpg attachment... so that we can uniquely identify the file name from the image HTML and replace the correct Uploading Filename.jpg attachment... text with it.

The above changes will make sure the sequence of commands we send to the back-end in this case is the same as when the user edits the comment after the image is completely uploaded and shown (in this case the UpdateComment command sent will have the

Text
<img ...>

content)

What alternative solutions did you explore? (Optional)

NA

@beodw
Copy link

beodw commented May 18, 2024

@dominictb when we are uploading an attachment the form payload has a binary file included which is one way the backend may differentiate it from updating the command to update a request. @hungvu193 thoughts on this?

@hungvu193
Copy link
Contributor

There's an on going discussion here, I'm gonna wait for it.
For me, BE will know exactly when attachment was uploaded and in the meantime if there was any UpdateComment was called, we should return attachment + text instead. I also prefer the way Slack handle this case (only edit the text, leave attachment alone), but yeah let wait for this discussion.

@twisterdotcom twisterdotcom changed the title [$250] Editing a message with an image attached before image loading completed never loads the attachments [HOLD on #41952] [$250] Editing a message with an image attached before image loading completed never loads the attachments May 20, 2024
@twisterdotcom twisterdotcom added Weekly KSv2 and removed Daily KSv2 labels May 20, 2024
@melvin-bot melvin-bot bot removed the Weekly KSv2 label Jun 12, 2024
Copy link

melvin-bot bot commented Jun 12, 2024

This issue has not been updated in over 15 days. @twisterdotcom, @hungvu193 eroding to Monthly issue.

P.S. Is everyone reading this sure this is really a near-term priority? Be brave: if you disagree, go ahead and close it out. If someone disagrees, they'll reopen it, and if they don't: one less thing to do!

@melvin-bot melvin-bot bot added the Monthly KSv2 label Jun 12, 2024
@hungvu193
Copy link
Contributor

Still holding

Copy link

melvin-bot bot commented Aug 26, 2024

@twisterdotcom, @hungvu193, this Monthly task hasn't been acted upon in 6 weeks; closing.

If you disagree, feel encouraged to reopen it -- but pick your least important issue to close instead.

@dominictb
Copy link
Contributor

@twisterdotcom The discussion this issue was held on was done a long time ago, could you help reopen the issue and add Daily label?

Thanks

@twisterdotcom twisterdotcom reopened this Sep 25, 2024
@twisterdotcom twisterdotcom added Daily KSv2 and removed Monthly KSv2 labels Sep 25, 2024
@twisterdotcom twisterdotcom changed the title [HOLD on #41952] [$250] Editing a message with an image attached before image loading completed never loads the attachments [$250] Editing a message with an image attached before image loading completed never loads the attachments Sep 25, 2024
Copy link

melvin-bot bot commented Sep 26, 2024

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

Copy link

melvin-bot bot commented Sep 30, 2024

@twisterdotcom, @hungvu193 Huh... This is 4 days overdue. Who can take care of this?

@melvin-bot melvin-bot bot added the Overdue label Sep 30, 2024
@hungvu193
Copy link
Contributor

On it today.

@melvin-bot melvin-bot bot removed the Overdue label Oct 1, 2024
@hungvu193
Copy link
Contributor

With new UI, I don't think we can reproduce this issue:

Screen.Recording.2024-10-01.at.11.09.47.mov

Feel free to close @twisterdotcom

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 Not a priority
Projects
No open projects
Status: No status
Development

No branches or pull requests

5 participants