Added attachment size check for videos on native #6588

akshayasalvi · 2021-12-06T13:17:37Z

Details

Fixed Issues

$ #5918

Tests

Tested video uploads on all platforms
Tested video upload via document picker

QA Steps

Go to any chat and click on Add Attachment
Choose a video greater than the limit (50mb).
Ensure that it throws an error on the attachment size

Tested On

Screenshots

Web

Mobile Web

Desktop

iOS

Android

Beamanator

Looks great, tests well on native devices! 👍 Just one quick question

src/components/AttachmentPicker/index.native.js

Beamanator · 2021-12-07T12:50:33Z

Also interested in your review @parasharrajat when you have time 👍

parasharrajat · 2021-12-07T13:02:28Z

I will do that today. It's on my list.

parasharrajat

There are a few suggestions otherwise looks good.

src/components/AttachmentPicker/index.native.js

parasharrajat

NAB: But https://github.com/Expensify/App/pull/6588/files#R244 completeAttachmentSelection is very confusing.
This function should be just initialized on the constructor as noop.

this.completeAttachmentSelection = () => {}

parasharrajat

NAB: another very confusing code is isValidSize in AttachementModal.js

App/src/components/AttachmentModal.js

Lines 116 to 118 in 04d28af

    
           isValidSize(file) { 
        
               return !file || lodashGet(file, 'size', 0) < CONST.API_MAX_ATTACHMENT_SIZE; 
        
           }

.

it returns true when file is not present.

akshayasalvi · 2021-12-08T16:35:37Z

@parasharrajat @Beamanator I've updated PR as per comments.

I am not sure about isValidSize(file) and hence I haven't changed that.

parasharrajat

Looks better but one more suggestion before I will test it on platforms.

src/components/AttachmentPicker/index.native.js

parasharrajat · 2021-12-08T16:47:57Z

I am not sure about isValidSize(file) and hence I haven't changed that.

OK. let's leave it then. In theory, the file will never be null for this function.

src/components/AttachmentPicker/index.native.js

parasharrajat

The code looks good now. Thanks for the changes. cc: @Beamanator

NAB: The only thing I found confusing is that the #6588 (files) completeAttachmentSelection is very confusing.
We should remove this function definition. It is never used. It also uses an unused state variable result which is not consumed anywhere. In action, completeAttachmentSelection is replaced on runtime at L256. I leave this up to you @Beamanator to decide.

    /**
      * Opens the attachment modal
      *
      * @param {function} onPicked A callback that will be called with the selected attachment
      */
    open(onPicked) {
        this.completeAttachmentSelection = onPicked;
        this.setState({isVisible: true});
    }

Beamanator · 2021-12-09T10:08:15Z

src/components/AttachmentPicker/index.native.js

@@ -111,13 +121,15 @@ class AttachmentPicker extends Component {

        this.close = this.close.bind(this);
        this.pickAttachment = this.pickAttachment.bind(this);
+        this.completeAttachmentSelection = () => {};


I'd say we should not add this for now, but make a separate issue to clean up completeAttachmentSelection - as you said, it looks pretty horrible in this file, but that's not the point of this PR

@akshayasalvi Could you please remove this line based on the request above?

src/components/AttachmentPicker/index.native.js

Added Promise for jsdoc Co-authored-by: Alex Beaman <[email protected]>

…p into attachment-size-err

akshayasalvi · 2021-12-09T13:19:59Z

@parasharrajat @Beamanator PR updated with the changes. Thanks for the feedback and helping improve the code.

parasharrajat

I think you missed one pointer.

parasharrajat · 2021-12-09T13:56:03Z

src/components/AttachmentPicker/index.native.js

@@ -111,13 +121,15 @@ class AttachmentPicker extends Component {

        this.close = this.close.bind(this);
        this.pickAttachment = this.pickAttachment.bind(this);
+        this.completeAttachmentSelection = () => {};


@akshayasalvi Could you please remove this line based on the request above?

akshayasalvi · 2021-12-09T15:15:11Z

@parasharrajat Done

parasharrajat

LGTM.

cc: @Beamanator

Beamanator

Awesome 👍 Nice work @akshayasalvi and thanks for the helpful reviews @parasharrajat 👍

OSBotify · 2021-12-10T15:21:16Z

✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release.

OSBotify · 2021-12-10T22:00:44Z

🚀 Deployed to staging by @Beamanator in version: 1.1.19-5 🚀

platform	result
🤖 android 🤖	success ✅
🖥 desktop 🖥	success ✅
🍎 iOS 🍎	success ✅
🕸 web 🕸	success ✅

OSBotify · 2021-12-20T12:09:28Z

🚀 Deployed to production by @Julesssss in version: 1.1.21-1 🚀

platform	result
🤖 android 🤖	failure ❌
🖥 desktop 🖥	success ✅
🍎 iOS 🍎	success ✅
🕸 web 🕸	success ✅

Added attachment size check for videos on native

dcf8ec6

akshayasalvi requested a review from a team as a code owner December 6, 2021 13:17

MelvinBot requested review from Beamanator and parasharrajat and removed request for a team December 6, 2021 13:17

Beamanator reviewed Dec 7, 2021

View reviewed changes

src/components/AttachmentPicker/index.native.js Outdated Show resolved Hide resolved

thrown the received error for picker

a500f43

Beamanator assigned akshayasalvi Dec 7, 2021

parasharrajat suggested changes Dec 7, 2021

View reviewed changes

akshayasalvi added 2 commits December 8, 2021 21:29

Changed promise structure

75386dc

Added noop for completeAttachmentSelection

c675c6f

akshayasalvi requested a review from parasharrajat December 8, 2021 16:35

parasharrajat suggested changes Dec 8, 2021

View reviewed changes

src/components/AttachmentPicker/index.native.js Outdated Show resolved Hide resolved

Return Promise

d1c2e4c

parasharrajat reviewed Dec 9, 2021

View reviewed changes

src/components/AttachmentPicker/index.native.js Outdated Show resolved Hide resolved

parasharrajat previously approved these changes Dec 9, 2021

View reviewed changes

Beamanator self-requested a review December 9, 2021 10:03

Beamanator reviewed Dec 9, 2021

View reviewed changes

src/components/AttachmentPicker/index.native.js Outdated Show resolved Hide resolved

akshayasalvi dismissed parasharrajat’s stale review via c26ae54 December 9, 2021 11:36

akshayasalvi and others added 3 commits December 9, 2021 17:06

Update src/components/AttachmentPicker/index.native.js

c26ae54

Added Promise for jsdoc Co-authored-by: Alex Beaman <[email protected]>

Added catch for error block

4ba96a8

Merge branch 'attachment-size-err' of github-personal:akshayasalvi/Ap…

49e3362

…p into attachment-size-err

parasharrajat suggested changes Dec 9, 2021

View reviewed changes

Removed the function noop

a5d18d8

parasharrajat approved these changes Dec 9, 2021

View reviewed changes

Beamanator approved these changes Dec 10, 2021

View reviewed changes

Beamanator merged commit a35db1e into Expensify:main Dec 10, 2021

OSBotify mentioned this pull request Dec 10, 2021

Deploy Checklist: New Expensify 2021-12-10 #6703

Closed

38 tasks

botify mentioned this pull request Dec 20, 2021

[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

	isValidSize(file) {
	return !file \|\| lodashGet(file, 'size', 0) < CONST.API_MAX_ATTACHMENT_SIZE;
	}

Added attachment size check for videos on native #6588

Added attachment size check for videos on native #6588

Uh oh!

Conversation

akshayasalvi commented Dec 6, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Details

Fixed Issues

Tests

QA Steps

Tested On

Screenshots

Web

Mobile Web

Desktop

iOS

Android

Uh oh!

Beamanator left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Beamanator commented Dec 7, 2021

Uh oh!

parasharrajat commented Dec 7, 2021

Uh oh!

parasharrajat left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

parasharrajat left a comment

Choose a reason for hiding this comment

Uh oh!

parasharrajat left a comment

Choose a reason for hiding this comment

Uh oh!

akshayasalvi commented Dec 8, 2021

Uh oh!

parasharrajat left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

parasharrajat commented Dec 8, 2021

Uh oh!

Uh oh!

parasharrajat left a comment

Choose a reason for hiding this comment

Uh oh!

Beamanator Dec 9, 2021

Choose a reason for hiding this comment

Uh oh!

parasharrajat Dec 9, 2021

Choose a reason for hiding this comment

Uh oh!

Uh oh!

akshayasalvi commented Dec 9, 2021

Uh oh!

parasharrajat left a comment

Choose a reason for hiding this comment

Uh oh!

parasharrajat Dec 9, 2021

Choose a reason for hiding this comment

Uh oh!

akshayasalvi commented Dec 9, 2021

Uh oh!

parasharrajat left a comment

Choose a reason for hiding this comment

Uh oh!

Beamanator left a comment

Choose a reason for hiding this comment

Uh oh!

OSBotify commented Dec 10, 2021

Uh oh!

OSBotify commented Dec 10, 2021

Uh oh!

OSBotify commented Dec 20, 2021

Uh oh!

akshayasalvi commented Dec 6, 2021 •

edited

Loading