[MEDIUM] [Image][$1000] Improve NewDot image uploading experience, add large file support

[chiragsalian]

If you haven’t already, check out our contributing guidelines for onboarding and email [email protected] to request to join our Slack channel!

---

Action Performed:

1. Upload an image in App

Expected Result:

The image should be grayed (opacity 0.6 i.e., chatItemUnsentMessage) until its uploaded and once its uploaded it should not show the white thumbnail with loader

and instead continue showing the local thumbnail or show the final image immediately.

Actual Result:

1. The local image is being uploaded and we see the local thumbnail,
   
2. The local image has finished uploading and now we attempt to render it which first shows up a white thumbnail with loader
   
3. The render completes and we see the uploaded image
   

Workaround:

Can the user still use Expensify without this being fixed? Have you informed them of the workaround?

Platform:

• Web
• iOS
• Android
• Desktop App
• Mobile Web

Version Number: 1.1.76
Reproducible in staging?: Y
Reproducible in production?: Y
Slack conversation: https://expensify.slack.com/archives/C01GTK53T8Q/p1654262165566879

View all open jobs on GitHub

cc @michaelhaxhiu, @cead22, @kidroca.

Issue Owner

Current Issue Owner: @deetergp

[melvin-bot]

Triggered auto assignment to @MitchExpensify (External), see https://stackoverflow.com/c/expensify/questions/8582 for more details.

[MitchExpensify]

Upwork Job

[melvin-bot]

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

[melvin-bot]

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

[parasharrajat]

Good change. I agree.

[JosueEchandiaAsto]

Proposal

Opacity can be added to the preview image, but loading later. It would be better to keep it, because it is the loading that all the images have when a url is received and it is being loaded.

This spinner while loading the image is better to keep it.

Sin.titulo.mp4

Add a style:

App/src/styles/styles.js

Lines 326 to 328 in 72ed0e5

	opacityImageLoading: {
	opacity: 0.6,
	},

App/src/components/HTMLEngineProvider/HTMLRenderers/ImageRenderer.js

Lines 52 to 58 in 72ed0e5

	<ThumbnailImage
	previewSourceURL={previewSource}
	style={[styles.webViewStyles.tagStyles.img, styles.opacityImageLoading]}
	isAuthTokenRequired={isAttachment}
	imageWidth={imageWidth}
	imageHeight={imageHeight}
	/>

Result

Simulator.Screen.Recording.-.iPhone.13.-.2022-06-15.at.05.37.50.mp4

[parasharrajat]

@JosueEchandiaAsto Thanks for the proposal. But as you are a new contributor and have already been assigned two 2 tasks, you will have to wait before applying to new tasks.

Contribution policy: https://github.com/Expensify/App/blob/main/CONTRIBUTING.md#payment-for-contributions

New contributors are limited to working on one job at a time, however experienced contributors may work on numerous jobs simultaneously.

[JosueEchandiaAsto]

@parasharrajat Both tasks have already been completed and have been merged. Do I need to complete any additional steps in those tasks?

[parasharrajat]

Oh, I see. I will review your proposal shortly.

[parasharrajat]

@JosueEchandiaAsto Your proposal does not meet the requirements.

It would be better to keep it, because it is the loading that all the images have when a url is received and it is being loaded.This spinner while loading the image is better to keep it.

Why should we keep the loader?

Idea is to show the same thumbnail when the image is loading even after the server URL is received. When the image is fully loaded from the URL show the final image.

[deetergp]

Still no news from @kidroca. You still out there?

[deetergp]

Still no word from @kidroca. Do we need to open this back up to other contributors?

[trjExpensify]

Do we have a clear picture of what we need done in the front-end PR for that? In the meantime, I've reached out. 👍

[kidroca]

Hi team, I apologize for my extended absence and lack of communication. I'm back now and reviewing the current state of the ticket. I'll provide an update within 24 hours, after I've caught up with the latest changes

[kidroca]

Status Update

I've synchronized my PR with latest changes from main.

• Fix Console Error When Uploading PDF Files #47184 (comment)

• I've tried including data-attachment-id to the markup we send when creating attachments, but re-inspecting the code I've realised this markup is only being used in the optimistic data.

• We can generate attachmentID and send it as part of the parameters for API.write (details below).

• next: I'll re-test the fix in the PR and add screenshots so it's ready for review.

We could do the following:

1. When we create the attachment report action (on the client)
2. We generate an attachmentID via NumberUtils.rand64()
3. We add a parameter like parameters.attachmentData to the "AddAttachment" API, where we store attachment related information
4. When the backend creates the action report action and markup it can read the attachment-id from parameters.attachmentData.attachmentID

Or instead of parameters.attachmentData we can introduce, parameters.attachments and move any attachment information like file there. This way when we support multiple attachments they can be listed in parameters.attachments as [{file: ..., attachmentID: ...}, {...}, ...]

---

My original understanding was that "AddAttachment" API worked with the html markup generated on the client, so the original proposal was to add data-attachment-id to the markup generated here:

App/src/libs/ReportUtils.ts

Lines 4234 to 4248 in 30d508b

	function getUploadingAttachmentHtml(file?: FileObject): string {
	if (!file || typeof file.uri !== 'string') {
	return '';
	}
	const dataAttributes = [
	`${CONST.ATTACHMENT_OPTIMISTIC_SOURCE_ATTRIBUTE}="${file.uri}"`,
	`${CONST.ATTACHMENT_SOURCE_ATTRIBUTE}="${file.uri}"`,
	`${CONST.ATTACHMENT_ORIGINAL_FILENAME_ATTRIBUTE}="${file.name}"`,
	'width' in file && `${CONST.ATTACHMENT_THUMBNAIL_WIDTH_ATTRIBUTE}="${file.width}"`,
	'height' in file && `${CONST.ATTACHMENT_THUMBNAIL_HEIGHT_ATTRIBUTE}="${file.height}"`,
	]
	.filter((x) => !!x)
	.join(' ');

But inspecting the Network call I see no markup is being sent, or at least the <img> part is being omitted, double checked the code at Report.addActions and it seems that indeed we send only reportCommentText

[deetergp]

Hey gang, I'm back from OOO and will try to get to this today.

[parasharrajat]

@deetergp What do you think about this #9402 (comment)? do we need these changes on backend?

[deetergp]

The issue here is that on the back end, we get the file information from PHP's built-in $_FILES global which has a pre-defined set of parameters. I'm a little uncertain about how we'd get the attachmentID to track with the files 🤔

It's possible we could pass a separate parameters.attachments array then loop through the $_FILES array and try to associate the additional metadata passed in the attachments array with it.

[parasharrajat]

Do we need multiple attachments for this implementation? If not, then I would say that we just add another parameter to the API for attachment-id and then on backend we can map that with the file coming from file array.

IMO, AddAttachment API only supports sending one file at a time so we link the attachment-id with the file being sent. $_FILES will only contain one file with the current implementation so you can pick the first file and map the id to it.

---

Another way

1. we can use indexing to achieve linking.
2. We can send the attachment-id as array and map them based on index to the $_FILES array values.
3. This way we can be prepared for multiple file handling.

[deetergp]

We don't currently support multiple attachments and I don't know of any immediate plans to do so though it seems likely we will want to at some point. But for now, I think @parasharrajat makes a good point, let's just send the single attachment-id along the file in $_FILES and link them on the back end. I'll get a PR going.

[parasharrajat]

I don't think we can send anything other than files in $_FILES array. But we should be able to send the attachment-id as a new key:value in request. That should also work.

@kidroca Can you please make these changes and let us know if you need anything differently. Let's get this merged asap.

[deetergp]

Just so I am crystal clear on what we need on the back end here: The end result we are looking for here is that that we will add data-attachment-id=<the ID passed from NewDot> to the image tag in the HTML we save to the message field on the ADDCOMMENT report action. Is that correct?

[parasharrajat]

@kidroca Can you please chime in here and explain what you need?

[parasharrajat]

Correct. IMO, what we need is that when backend generates the HTML for image in Report action, we have to add the attachment id as attribute data-attachment-id to the tag passed previously.

[kidroca]

Just so I am crystal clear on what we need on the back end here: The end result we are looking for here is that that we will add data-attachment-id=<the ID passed from NewDot> to the image tag in the HTML we save to the message field on the ADDCOMMENT report action. Is that correct?

Yes

It's possible we could pass a separate parameters.attachments array then loop through the $_FILES array and try to associate the additional metadata passed in the attachments array with it.

Yes, both these lists should have the same order when sent

I originally thought that we send file metadata as JSON, but it seems we only send binary data for the file, so any metadata like the attachment id should be added under a separate key

Since we currently support only a single attachment let's add a key like attachmentMeta or attachmentData to the payload for AddAttachment and AddTextAndAttachment

Or if we like to keep it super simple we can just add the key attachmentID to these actions

[kidroca]

Just so I am crystal clear on what we need on the back end here: The end result we are looking for here is that that we will add data-attachment-id=<the ID passed from NewDot> to the image tag in the HTML we save to the message field on the ADDCOMMENT report action. Is that correct?

Actually we need to add this to all attachments not just <img>, but <video> and <a> for attachments as well

[deetergp]

Or if we like to keep it super simple we can just add the key attachmentID to these actions

Let's keep it simple for now. I have a back end PR updated and out for review.

[deetergp]

The PR to add this is now on staging.

[parasharrajat]

Let's roll @kidroca

[trjExpensify]

Great stuff, that's now on prod! @kidroca can you give us an idea of when you can progress the front-end PR? Thanks!

[deetergp]

Bump @kidroca