[Pay 29/4] [$250] HIGH: [Polish] Combine image attachment with the unsent composed message

[quinthar]

Problem:

Today when you upload an image while a message is drafted in the compose window, it:

1. Sends whatever is drafted
2. Sends a 2nd message containing only the image

This means that even if the two are conceptually related (ie, the comment is describing the image), it will be forked into separate reactions, and have separate threads.

Solution

Rather than sending images as a totally separate message, combine the message and the attachment into a single message, such that it gets the same reactions and are part of the same thread.

Issue Owner

Current Issue Owner: @bfitzexpensify

[roryabraham]

Personally I think this issue kind of "beats around the bush" – I would love to see our efforts spent towards building full support for inline images in messages, GitHub-style, with image previews inlined in react-native-live-markdown.

[jeremy-croff]

Personally I think this issue kind of "beats around the bush" – I would love to see our efforts spent towards building full support for inline images in messages, GitHub-style, with image previews inlined in react-native-live-markdown.

Is this really how our customers would use it?
Slack for example does not have inline images in messages, cause it's just chat. This allows the full image to be previewed that's attached without it being cumbersome for the content your typing.

Jira and GitHub have inline messages, where the image is converted with the upload path because it's used for steps and detailed information, it's like a whole mini text editor. I think those use cases are more prosumer, rather than consumer.

[roryabraham]

My opinion is that the inline image support offered by GitHub and Jira is superior in every way to the way images are handled by Slack, WhatsApp, iMessage, etc...

Those platforms don't make it any easier to add images to a message. The only advantage over GitHub for example is that you don't have to switch to the Preview tab to see how the image looks in your final message. But that limitation won't apply in New Expensify.

I believe that the inline image experience I'm proposing would be better than any of the other platforms we've listed here and as such would be a differentiating factor in our chat platform.

[quinthar]

I agree with @roryabraham -- this is just a step in that direction. A more complete plan is here: https://expensify.slack.com/archives/C01GTK53T8Q/p1708849229702429?thread_ts=1707270017.261459&cid=C01GTK53T8Q

[quinthar]

I think we should start with this to fix our current image support, but I agree we should add true inline image support -- and ultimately, large object support as well. But that's a bigger project that probably warrants a design doc, this is more low hanging fruit.

[tgolen]

I confirmed a few things via Slack, and it sounds like this is the detailed path to move forward on this issue:

• When a comment is made that has both an attachment and a text comment
• Upload the file as normal
• Retrieve it's URL
• Format the URL like the HTML that results from the Markdown code from HIGH: [Polish] Add support for inline image Markdown #37246
• Add that HTML at the end of the text comment instead of creating a separate report action

[quinthar]

#37246 is on staging; I'm taking this off hold! @tgolen can you give an ETA for when you can take it up?

[tgolen]

I'll bump this up to daily and will add an ETA once I finish catching up on my other issues today.

[tgolen]

Daily Update

• I haven't made any progress on this today

Next Steps

• Write a PR
• Get the PR reviewed then deployed

ETA

• PR created tomorrow, March 25
• PR merged the day after on Wednesday, March 26
• PR deployed to production on Monday, April 1

[tgolen]

I ran into a small snag with this today once I realized that the backend changes will require corresponding frontend changes to the optimistic data. I posted in slack about this to discuss a possible solution.

[tgolen]

Daily Update

• I created a draft Web-E PR that combines the attachments and text into a single message
• I created a draft App PR that updates the optimistic data
• I started a convo in Slack to discuss how to deploy this without breaking anything

Next Steps

• @quinthar Could you please look at the slack convo and see if the proposed solution is acceptable?
• Once that thread is resolved I will implement the new changes in the existing PRs and submit them for review

ETA

• Web-E change merged by Thursday, March 28
• App changed merged by Tuesday, April 2

[tgolen]

The slack discussion was resolved and this will be the rollout plan to ensure nothing breaks:

Background context:
There are two API commands on the backend:

• AddComment - Used for creating text-only comments
• AddAttachment - Used for creating attachment comments and accepts an optional text param

Problem:
The backend changes to combine text and attachments into a single message also require corresponding frontend changes to support the proper creation of optimistic data. Without both frontend and backend changes being deployed at the same time, attachments will be broken until both changes are deployed (which could be several days or weeks).

Solution:
Create a third API command called AddAttachmentAndText which will implement the new combined text+attachment message, and leave the other two API commands alone. Once the new API command has been deployed on the server, then the frontend can be updated to handle the new optimistic data format and switch from calling AddAttachment to AddAttachmentAndText.

[tgolen]

Daily Update

• The Web PR is out of draft and in full review
• The App PR is on hold

Next Steps

• Merge/deploy the Web-E PR
• I will test the App PR on all platforms, then remove the HOLD once the Web-E PR has been deployed

ETA

• Web-E PR merged by Tuesday, April 2
• App PR merged by Friday, April 5

[tgolen]

Daily Update

• The Web PR is still waiting to be reviewed
• The App PR is on hold and has had full platform testing done to it so that it's ready for review

Next Steps

• Merge/deploy the Web-E PR
• Un-HOLD the App PR and get it reviewed/merged

ETA

• Same

[tgolen]

Daily Update

• The Web-E PR was reviewed
• The App PR is still on HOLD

Next Steps

• @tgolen respond to the review from @neil-marcellini and get the PR approved and merged
• Un-HOLD the App PR and get it reviewed/merged

ETA

• Web-E PR merged by Wednesday, April 3 (updated)
• App PR merged by Tuesday, April 9 (updated)

[tgolen]

Daily Update

• The Web-E PR has been updated and is entering a second round of reviews
• The App PR is still on HOLD

Next Steps

• @tgolen get the Web-E PR approved and merged
• Un-HOLD the App PR and get it reviewed/merged

ETA

• Web-E PR merged by Wednesday, April 3 (updated)
• App PR merged by Tuesday, April 9 (updated)

[tgolen]

Daily Update

• The Web-E PR has one approval and is in final review
• The App PR is still on HOLD

Next Steps

• @Julesssss approve and merge the Web-E PR
• @tgolen Un-HOLD the App PR and get it reviewed/merged

ETA

• Web-E PR merged by Thursday, April 4 (updated)
• App PR merged by Tuesday, April 9

[tgolen]

Daily Update

• The Web-E PR has been merged
• The App PR is now off HOLD

Next Steps

• @tgolen get App PR reviewed and merged

ETA

• App PR merged by Tuesday, April 9

Note

I'm going to be OOO tomorrow and Monday so the ETA might get off a little if there are actions required while I am gone

[rayane-djouah]

Reviewing the PR as C+, please assign me to the issue and add Bug and External labels for C+ payment. Thank you!

[melvin-bot]

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

[tgolen]

@bfitzexpensify can you please ensure @rayane-djouah gets paid for a C+ review of the PR?

[bfitzexpensify]

PR was assigned for review (and the review began) before the price for a PR review was updated, so payment for @rayane-djouah is $500. Offer sent via Upwork.

Looks like the PR is merged but not yet deployed. I've subscribed and will update the payment date here.

[melvin-bot]

⚠️ Looks like this issue was linked to a Deploy Blocker here

If you are the assigned CME please investigate whether the linked PR caused a regression and leave a comment with the results.

If a regression has occurred and you are the assigned CM follow the instructions here.

If this regression could have been avoided please consider also proposing a recommendation to the PR checklist so that we can avoid it in the future.

[melvin-bot]

@tgolen, @bfitzexpensify, @rayane-djouah Uh oh! This issue is overdue by 2 days. Don't forget to update your issues!

[bfitzexpensify]

Deployed today, updated title.

[melvin-bot]

⚠️ Could not update price automatically because there is no linked Upwork Job ID. The BZ team member will need to update the price manually in Upwork.

[quinthar]

Should this be tagged Awaiting Payment? Why wasn't it automatically?

[melvin-bot]

@tgolen, @bfitzexpensify, @rayane-djouah Whoops! This issue is 2 days overdue. Let's get this updated quick!

[bfitzexpensify]

All done here.