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.43k stars 2.8k forks source link

[$250] Editing a message with an image attached before image loading completed never loads the attachments #42300

Closed m-natarajan closed 3 days ago

m-natarajan commented 4 months 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!

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?

Screenshots/Videos

Add any screenshot/video evidence image (1)

https://github.com/Expensify/App/assets/38435837/b1ddae28-108d-4fce-aa95-a68752a82ecd

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
melvin-bot[bot] commented 4 months ago

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.

melvin-bot[bot] commented 4 months ago

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

melvin-bot[bot] commented 4 months ago

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

hungvu193 commented 4 months ago

Waiting for proposals

beodw commented 4 months ago

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.

https://github.com/Expensify/App/assets/48998844/308dcf56-be57-4156-a406-7be2c3521438

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 commented 4 months ago

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

Cc @jliexpensify

dominictb commented 4 months ago

@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:

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 commented 4 months ago

@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 commented 4 months ago

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.

melvin-bot[bot] commented 3 months ago

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!

hungvu193 commented 3 months ago

Still holding

melvin-bot[bot] commented 1 month ago

@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 commented 1 month ago

@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

melvin-bot[bot] commented 1 week ago

📣 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] commented 4 days ago

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

hungvu193 commented 3 days ago

On it today.

hungvu193 commented 3 days ago

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

https://github.com/user-attachments/assets/889fcd52-c811-4f84-8cf4-84b6857f2c88

Feel free to close @twisterdotcom