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

[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 opened this issue Oct 16, 2021 · 34 comments
Assignees
Labels
Awaiting Payment Auto-added when associated PR is deployed to production Engineering External Added to denote the issue can be worked on by a contributor Weekly KSv2

Comments

@isagoico
Copy link

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?

  • iOS
  • Android

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

android-bug_5S3TO7ec.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
Copy link

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

@MelvinBot
Copy link

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

@cead22
Copy link
Contributor

cead22 commented Oct 19, 2021

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 Beamanator added Weekly KSv2 and removed Daily KSv2 labels Oct 19, 2021
@MelvinBot MelvinBot removed the Overdue label Oct 19, 2021
@Beamanator
Copy link
Contributor

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
Copy link
Contributor

Looking more into this later this week

@Beamanator
Copy link
Contributor

Not finding much time to work on this so far this week 🙈

@MelvinBot MelvinBot removed the Overdue label Nov 9, 2021
@akshayasalvi
Copy link
Contributor

akshayasalvi commented Nov 12, 2021

@Beamanator I was retesting this and it looks like I closed this in another PR. (#5900)

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

@Beamanator
Copy link
Contributor

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
Copy link
Contributor

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

@MelvinBot MelvinBot removed the Overdue label Nov 29, 2021
@akshayasalvi
Copy link
Contributor

@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);
    });
}



@laurenreidexpensify laurenreidexpensify changed the title Android/iOS - User is able to upload a video of >50mb - Reported by: @akshayasalvi $500 - solution + reporting bonus. Android/iOS - User is able to upload a video of >50mb - Reported by: @akshayasalvi Nov 30, 2021
@botify botify removed the Daily KSv2 label Nov 30, 2021
@MelvinBot MelvinBot added the Weekly KSv2 label Nov 30, 2021
@MelvinBot
Copy link

Triggered auto assignment to @parasharrajat (Exported)

@MelvinBot MelvinBot added the Help Wanted Apply this label when an issue is open to proposals by contributors label Nov 30, 2021
@MelvinBot
Copy link

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

@FoolCoder
Copy link

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
Copy link
Contributor

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

@Beamanator Beamanator removed the Help Wanted Apply this label when an issue is open to proposals by contributors label Dec 1, 2021
@laurenreidexpensify laurenreidexpensify removed their assignment Dec 7, 2021
@laurenreidexpensify laurenreidexpensify added External Added to denote the issue can be worked on by a contributor and removed External Added to denote the issue can be worked on by a contributor labels Dec 7, 2021
@MelvinBot
Copy link

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

@MelvinBot MelvinBot added Daily KSv2 and removed Weekly KSv2 labels Dec 7, 2021
@laurenreidexpensify
Copy link
Contributor

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

@laurenreidexpensify laurenreidexpensify added Weekly KSv2 and removed Daily KSv2 labels Dec 7, 2021
@stephanieelliott
Copy link
Contributor

PR is on staging.

@botify botify added Weekly KSv2 Awaiting Payment Auto-added when associated PR is deployed to production and removed Weekly KSv2 labels Dec 20, 2021
@botify botify changed the title $500 - solution + reporting bonus. Android/iOS - User is able to upload a video of >50mb - Reported by: @akshayasalvi [HOLD for payment 2021-12-27] $500 - solution + reporting bonus. Android/iOS - User is able to upload a video of >50mb - Reported by: @akshayasalvi Dec 20, 2021
@botify
Copy link

botify commented Dec 20, 2021

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

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

@laurenreidexpensify
Copy link
Contributor

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Awaiting Payment Auto-added when associated PR is deployed to production Engineering External Added to denote the issue can be worked on by a contributor Weekly KSv2
Projects
None yet
Development

No branches or pull requests

10 participants