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

[PAY on 6/17] Uploading a video fails. #2712

Closed
parasharrajat opened this issue May 6, 2021 · 19 comments · Fixed by #3332
Closed

[PAY on 6/17] Uploading a video fails. #2712

parasharrajat opened this issue May 6, 2021 · 19 comments · Fixed by #3332
Assignees
Labels
Engineering Planning Changes still in the thought process Weekly KSv2

Comments

@parasharrajat
Copy link
Member

parasharrajat commented May 6, 2021

If you haven’t already, check out our contributing guidelines for onboarding and email contributors@expensify.com to request to join our Slack channel!


Expected Result:

  1. A error message should be thrown if video uploading fails.
  2. User should not be able to select Mkv videos as they are not supported from the backend.

Actual Result:

  1. Message is always in loading state.
    Error thrown
Uncaught (in promise) TypeError: can't access property "sequenceNumber", reportAction is undefined

Action Performed:

  1. Open Expensify.cash on web browser.
  2. Open any report.
  3. try sending an MKV video.

Workaround:

Covert the video to mp4 and then upload.

Platform:

Where is this issue occurring?

Web - ☑️
iOS
Android
Desktop App ☑️
Mobile Web ☑️

Version Number: V.1.0.38
Logs: https://stackoverflow.com/c/expensify/questions/4856
Notes/Photos/Videos: Any additional supporting documentation
Expensify/Expensify Issue URL:

View all open jobs on Upwork

@parasharrajat
Copy link
Member Author

parasharrajat commented May 6, 2021

Reason:

  1. Backend does not support MKV files and it throws an error like
{"code":666,"jsonCode":408,"type":"Expensify\\Libs\\Error\\ExpError","UUID":"...","message":"This file type is not supported. Please try again using a supported file type: jpg, jpeg, png, gif, pdf, html, txt, rtf, doc, docx, htm, tiff, tif, xml","title":"Upload Error: Unsupported file type","data":{"type":"video\/x-matroska; charset=binary"},"htmlMessage":"","requestID":"09e249e09d000028cd6f008000000001"}

Solution:

  1. We should handle the error gracefully here and show an error message to the user that the file is not supported and remove the unsent comment from the Report.
    https://github.com/Expensify/Expensify.cash/blob/53acb9ce968a9ea3007302e3debb8b2d9a345ebb/src/libs/actions/Report.js#L931

  2. We should update the AttachmentPicker so that it only allows the supported files using the accept attribute.
    https://github.com/Expensify/Expensify.cash/blob/53acb9ce968a9ea3007302e3debb8b2d9a345ebb/src/components/AttachmentPicker/index.js#L15-L20

Alerting the message is fine until we have growls.

@marcaaron
Copy link
Contributor

Thanks @parasharrajat, but I can't see any reason why we would not support this format 🤔
So we may have to fix it in the backend instead of blocking the user.

@parasharrajat
Copy link
Member Author

parasharrajat commented May 7, 2021

Ok. That is also fine.

But we should at least handle the errors as there could be many file formats and not all we will support. With respect to these, we can add accept attributes to control what we want to accept.

@mallenexpensify
Copy link
Contributor

What do we think is the best path forward here? Reword the OP to fixing the backend for all or specific file types like MKV video? (I have no idea how common MKV is).
My first, non-engineering, thought is 'how can we address and/or fix all file types instead of creating many one-off issues for any/everyone we see.

@parasharrajat
Copy link
Member Author

parasharrajat commented May 25, 2021

@mallenexpensify
In general, We can't handle all of the file extensions, we will have to throw en error with Growl if file is not supported from backend.

So my suggestion...

  1. We throw error on frontend when File is not supported from backend. This is not specific to Videos.
  2. We ask backend team to allow more common file types in video, audio, images.
  3. We put a check in the app so that only allowed files are visible on the File Chooser window.

@marcaaron marcaaron added the Planning Changes still in the thought process label May 26, 2021
@MelvinBot
Copy link

Triggered auto assignment to @stitesExpensify (Engineering), see https://stackoverflow.com/c/expensify/questions/4319 for more details.

@mallenexpensify
Copy link
Contributor

Thanks @parasharrajat , assigning an engineer for review

@stitesExpensify
Copy link
Contributor

I think that having a large catch-all for any non-supported file types is a great idea!

@mallenexpensify
Copy link
Contributor

@stitesExpensify Do we have any/everything needed here for a contributor to start work on the issue? If so can you add the External label. If not, can you type a short list of the deliverables?

@stitesExpensify
Copy link
Contributor

I think the deliverable is just:
When uploading attachments, if the upload fails show a growl that says "Upload Failed. File type may not be supported."

@parasharrajat are you seeing the same thing as me? For some reason the error I'm getting when uploading an MKV is a CORS error, which seems incorrect to me.... That's a weird error to catch here

@parasharrajat
Copy link
Member Author

So yeah. we get a jsoncode which indicate that file type is not supported this we can adjust the message . Such as Upload failed! {message here}.

@stitesExpensify
Copy link
Contributor

Hmm okay I'm not seeing that, i'm getting a CORS error for some reason. Weird.

Okay @mallenexpensify the deliverable in that case is:
When uploading attachments, if the upload fails due to an unsupported file type, show a growl that says "Upload Failed. <FileType> not supported."

@parasharrajat
Copy link
Member Author

One think I forget to mention is that what should happen with the report message that will be showing the loader when upload fails. Should not remove it?

@stitesExpensify
Copy link
Contributor

Ah yep you're totally right.

Okay round 3 😅

  • When uploading attachments, if the upload fails due to an unsupported file type, show a growl that says "Upload Failed. not supported."
  • There should not be an attachment loading in the chat
    2021-05-26_15-57-28

@mallenexpensify
Copy link
Contributor

Sorry for delay, Got this up on Upwork and invited @parasharrajat. Drop a comment here when you accept and I'll assign to you https://www.upwork.com/jobs/~0158e0f4c5a432c0ab

@parasharrajat
Copy link
Member Author

@mallenexpensify Done. Thanks.

@KauMah
Copy link

KauMah commented Jun 1, 2021

Just saw this posted on Upwork, did @parasharrajat already take issue on or are you guys still looking for help? If not,, the project seems pretty cool and I'd be excited to help.

@mallenexpensify
Copy link
Contributor

Hi @KauMah. I'm assigning this to @parasharrajat right now. You can view all of our jobs here https://www.upwork.com/ab/jobs/search/?q=Expensify%20React%20Native&sort=recency&user_location_match=2.
All the same jobs are also posted as issues in this Expensify/Expensify.cash repo and also to our #expensify-open-source Slack channel (which you can join by emailing contributors@expensify.com).

@mallenexpensify mallenexpensify changed the title Uploading a video fails. [PAY on 6/17] Uploading a video fails. Jun 11, 2021
@mallenexpensify
Copy link
Contributor

Paid w/ Bonus in Upwork for originally reporting the issue. Thanks @parasharrajat!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Engineering Planning Changes still in the thought process Weekly KSv2
Projects
None yet
Development

Successfully merging a pull request may close this issue.

7 participants