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.51k stars 2.86k forks source link

[HOLD for payment 2021-12-27] $500 - solution + reporting bonus. Android/iOS - User is able to upload a video of >50mb - Reported by: @akshayasalvi #5918

Closed isagoico closed 2 years ago

isagoico commented 3 years 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!


Action Performed:

  1. Navigate to a conversation
  2. Upload a video that is over 50mb

Expected Result:

There should be a error message about the upload limit of 50mb

Actual Result:

User is able to upload the >50mb video

Workaround:

None needed.

Platform:

Where is this issue occurring?

Version Number: 1.1.8-2

Reproducible in staging?: Yes Reproducible in production?: Yes

Logs: https://stackoverflow.com/c/expensify/questions/4856

Notes/Photos/Videos: Any additional supporting documentation

https://user-images.githubusercontent.com/44479856/137604015-c2fbbed5-3fa5-41fb-8610-9e5e75accf0f.mp4

Expensify/Expensify Issue URL:

Issue reported by: @akshayasalvi Slack conversation: https://expensify.slack.com/archives/C01GTK53T8Q/p1634322848367700

View all open jobs on GitHub

MelvinBot commented 3 years ago

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

MelvinBot commented 3 years ago

Triggered auto assignment to @Beamanator (Demolition), see https://stackoverflow.com/c/expensify/questions/8099 for more details.

cead22 commented 3 years ago

It doesn't look like the user is actually able to upload the video successfully, but the reproduction steps are clear and we should show an error messages instead of endless spinners

Beamanator commented 3 years ago

N6-hold ended, and changing to Weekly instead of daily b/c it's not super high priority. Will look into it later this week.

Beamanator commented 2 years ago

Looking more into this later this week

Beamanator commented 2 years ago

Not finding much time to work on this so far this week πŸ™ˆ

akshayasalvi commented 2 years ago

@Beamanator I was retesting this and it looks like I closed this in another PR. (https://github.com/Expensify/App/pull/5900)

This commit fixed it. Earlier it basically failed to fetch the size for videos and allowed the user to upload.

Beamanator commented 2 years ago

Hmm can you test again on Android or iOS? I believe I reproduced the issue a week ago, which is even after the PR you mention was merged to production

Beamanator commented 2 years ago

I'll try again to reproduce this week in case it actually was fixed before

akshayasalvi commented 2 years ago

@Beamanator So I was able to reproduce this. Problem is when the files are picked from photo gallery we are not getting the size parameter. It turns out thats a limitation from react-native-image-picker as size works for photos only.

If I upload the video using add Document API then I do get fileSize even for videos (which is what I was trying earlier), and it worked fine with my last commit.

Here's my proposal to fix this: We use rn-fetch-blob to get the file size incase we don't get fileSize and size both. Tested the below function for iOS and I am guessing it should work with Android too.

// Inside pickAttachment

getDataForUpload(attachment).then((result) => {
            this.completeAttachmentSelection(result);
        });

// Update getDataForUpload
function getDataForUpload(fileData) {
    return new Promise((resolve, reject) => {
        const fileResult = {
            name: fileData.fileName || fileData.name || 'chat_attachment',
            type: fileData.type,
            uri: fileData.uri,
            size: fileData.fileSize || fileData.size,
        };
        if (_.has(fileData, 'fileSize') || _.has(fileData, 'size')) {
            return resolve(fileResult);
        }

        RNFetchBlob.fs.stat(fileData.uri.replace('file://', '')).then((stats) => {
            fileResult.size = stats.size;
            return resolve(fileResult);
        }).catch(reject);
    });
}
Beamanator commented 2 years ago

@akshayasalvi Thanks so much for looking into this! Since it looks like the issue is in the library, I'll mark this External

MelvinBot commented 2 years ago

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

Beamanator commented 2 years ago

@akshayasalvi just to be clear, if you're proposing to use rn-fetch-blob, you mean https://github.com/joltup/rn-fetch-blob right? It looks like that repo isn't maintained, so I would be pretty hesitant to use that one πŸ€”

akshayasalvi commented 2 years ago

@Beamanator I saw that but I only proposed after checking that it is a part of our dependencies already

Beamanator commented 2 years ago

@Beamanator I saw that but I only proposed after checking that it is a part of our dependencies already

Ooops I should have checked that, thanks for correcting me πŸ‘

Before I accept your proposal, would you mind testing in Android to make sure it works there too?

akshayasalvi commented 2 years ago

@Beamanator Can confirm this works on Android as well.

image

Beamanator commented 2 years ago

@akshayasalvi that's fantastic! One more thing I'm thinking about: if this is something we only want to do on native, maybe we should move it to /libs so we can easily have 1 function for web / desktop & 1 for native. What do you think?

akshayasalvi commented 2 years ago

Yeah sure. That makes sense.

Quick question; solution that I mentioned was for native only. We do have AttachmentPicker separate for native and web. This code will go in AttachmentPicker/index.native.js only. Still want to move to libs? Or what I mentioned is fine.

Beamanator commented 2 years ago

Aaahhhhh very good point, let's keep it in index.native.js πŸ‘ Thanks, you're hired!

MelvinBot commented 2 years ago

πŸ“£ @akshayasalvi You have been assigned to this job by @Beamanator! Please apply to this job in Upwork and let us know when we can expect a PR to be ready for review πŸ§‘β€πŸ’» Keep in mind: Code of Conduct | Contributing πŸ“–

parasharrajat commented 2 years ago

Aaahhhhh very good point, let's keep it in index.native.js +1 Thanks man, you're hired!

Correction :woman: @Beamanator

Beamanator commented 2 years ago

Haha I didn't ask for your (@parasharrajat) review b/c you weren't autoassigned yet by the Exported label - but please take a look at the proposal and let me know what you think when you have a chance!

parasharrajat commented 2 years ago

Not about that.

Thanks man

=> :woman:

About proposal, let's see it in review. I think proposal is :+1:

:ribbon: :eyes: :ribbon:

Beamanator commented 2 years ago

Oof πŸ™ˆ I'm so sorry @akshayasalvi !! Thanks for the catch @parasharrajat

laurenreidexpensify commented 2 years ago

https://www.upwork.com/jobs/~01793b87d36f476b80

@akshayasalvi job ready for you in Upwork

MelvinBot commented 2 years ago

Triggered auto assignment to @parasharrajat (Exported)

MelvinBot commented 2 years ago

Current assignee @Beamanator is eligible for the Exported assigner, not assigning anyone new.

FoolCoder commented 2 years ago

here is my proposal to fix this issue const uploadvideo = async () => {

try {
  const results = await DocumentPicker.pickMultiple({
    type: [DocumentPicker.types.allFiles],
  });

  results.map(q => {
    console.log(q.type);
    if (q.type === 'video/mp4' || q.type === 'video/mov' || q.type === 'video/avi' || q.type === 'video/mkv') {
      if (q.size >= 50000000) {
        setvideos((prev) => {
          return [
            ...prev, { uri: q.uri, name: q.name, type: q.type }
          ]
        })

      }
      else {
        alert('video file size should be greater than 50mb')
      }
    }
    else {
      alert('Please select  video')
    }
  })
  console.log('jjjjjj', results);

} catch (err) {
  if (DocumentPicker.isCancel(err)) {
    // User cancelled the picker, exit any dialogs or menus and move on
  } else {
    throw err;
  }
}

}

Beamanator commented 2 years ago

Thanks for the proposal @FoolCoder, but I've actually already assigned someone to this job (see this comment)

As for your proposal, please keep in mind that we currently don't use async / await in the app, and alert won't work too well since we're using React native - and there's some functions like setvideos that look like they're using the setState hook, we try not to use hooks whenever possible

MelvinBot commented 2 years ago

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

laurenreidexpensify commented 2 years ago

@stephanieelliott i'm OOO quite a bit over next week moving house, so handing this one off while I"m away thanks!

stephanieelliott commented 2 years ago

PR is on staging.

botify commented 2 years ago

The solution for this issue has been :rocket: deployed to production :rocket: in version 1.1.21-1 and is now subject to a 7-day regression period :calendar:. Here is the list of pull requests that resolve this issue:

If no regressions arise, payment will be issued on 2021-12-27. :confetti_ball:

laurenreidexpensify commented 2 years ago

paid $500 - solution ($250) + reporting bonus ($250)