(Fix) - #1605 QR Input address fix by Agupane · Pull Request #1612 · 5afe/safe-react

Agupane · 2020-11-16T18:30:58Z

Closes #1605
Closes #1196 by:

Adding types
Fixing how the. useEffect hook was being used for opening/closing the modal

github-actions · 2020-11-16T18:31:13Z

CLA Assistant Lite All Contributors have signed the CLA.

github-actions · 2020-11-16T18:36:54Z

ESLint Summary View Full Report

Annotations are provided inline on the Files Changed tab. You can also see all annotations that were generated on the annotations page.

Type	Occurrences	Fixable
Errors	0	0
Warnings	7	0
Ignored	0	N/A

Result: ✅ success
Annotations: 7 total

[warning] @typescript-eslint/explicit-module-boundary-types

Require explicit return and argument types on exported functions' and classes' public class methods

src/routes/open/components/SafeOwnersConfirmationsForm/index.tsx Line 47 - Missing return type on function.
src/routes/open/components/SafeOwnersConfirmationsForm/index.tsx Line 47 - Argument 'index' should be typed.
src/routes/open/components/SafeOwnersConfirmationsForm/index.tsx Line 47 - Argument 'notRemovedOwners' should be typed.
src/routes/open/components/SafeOwnersConfirmationsForm/index.tsx Line 47 - Argument 'values' should be typed.
src/routes/open/components/SafeOwnersConfirmationsForm/index.tsx Line 240 - Missing return type on function.
src/routes/open/components/SafeOwnersConfirmationsForm/index.tsx Line 240 - Argument 'controls' should be typed.
src/routes/open/components/SafeOwnersConfirmationsForm/index.tsx Line 240 - Object pattern argument should be typed.

^{Report generated by eslint-plus-action}

ghost · 2020-11-16T18:45:55Z

Travis automatic deployment:
https://pr1612--safereact.review.gnosisdev.com/volta/app

ghost · 2020-11-16T18:46:04Z

Travis automatic deployment:
https://pr1612--safereact.review.gnosisdev.com/rinkeby/app

fernandomg

#1196 's point 2. is not fixed. I still have the image selector popping up again, and the image is not being processed.

Maybe it would be better to let that fix for another PR and focus on the #1605 here.

src/components/ScanQRModal/index.tsx

ghost · 2020-11-17T13:53:16Z

Travis automatic deployment:
https://pr1612--safereact.review.gnosisdev.com/volta/app

ghost · 2020-11-17T13:53:47Z

Travis automatic deployment:
https://pr1612--safereact.review.gnosisdev.com/rinkeby/app

Fixes popping up again for wrong qr codes

ghost · 2020-11-17T14:58:47Z

Travis automatic deployment:
https://pr1612--safereact.review.gnosisdev.com/rinkeby/app

ghost · 2020-11-17T14:59:34Z

Travis automatic deployment:
https://pr1612--safereact.review.gnosisdev.com/volta/app

ghost · 2020-11-17T15:11:34Z

Travis automatic deployment:
https://pr1612--safereact.review.gnosisdev.com/volta/app

ghost · 2020-11-17T15:11:38Z

Travis automatic deployment:
https://pr1612--safereact.review.gnosisdev.com/rinkeby/app

Agupane · 2020-11-17T15:14:55Z

#1196 's point 2. is not fixed. I still have the image selector popping up again, and the image is not being processed.

Maybe it would be better to let that fix for another PR and focus on the #1605 here.

I tested it and seems that the problem is that the qr scanner cannot read the qr but instead of throwing an error it just open the modal again, I added an error text for those cases and remove the auto poping:

ghost · 2020-11-17T15:25:55Z

Travis automatic deployment:
https://pr1612--safereact.review.gnosisdev.com/rinkeby/app

ghost · 2020-11-17T15:27:01Z

Travis automatic deployment:
https://pr1612--safereact.review.gnosisdev.com/volta/app

nicosampler · 2020-11-17T19:43:15Z

src/components/ScanQRModal/index.tsx

      openImageDialog()
    }
-  }, [hasWebcam, openImageDialog])
+  }, [useWebcam, openImageDialog, fileUploadModalOpen, setFileUploadModalOpen, error])


I think this is not needed in dependencies setFileUploadModalOpen. But not sure.

It's not 100% needed but it's a bad react practice not adding all the things used on the useEffect

nicosampler · 2020-11-17T19:46:39Z

src/components/ScanQRModal/index.tsx

+      console.error('Error uploading file', error)
+      setError(`The QR could not be read`)
+    }
+    if (!useWebcam) {


is this validation needed? seems like if useWebcam is null or false, it will never reach this line.

Yes because if it's false means that the user opened the modal for uploading the file and if that line was reached that means that there was not an error but the qr could not be readed (it returns null or successData) and we need to display a feedback in that case

nicosampler

LGTM.
I just left a few comments!

ghost · 2020-11-17T20:49:53Z

Travis automatic deployment:
https://pr1612--safereact.review.gnosisdev.com/rinkeby/app

ghost · 2020-11-17T20:50:18Z

Travis automatic deployment:
https://pr1612--safereact.review.gnosisdev.com/volta/app

francovenica · 2020-11-17T21:47:59Z

Tested here:
https://pr1612--safereact.review.gnosisdev.com/rinkeby/app/#/safes/0x9913B9180C20C6b0F21B6480c84422F6ebc4B808/balances

The recipient field not being filled with the contract's address was fixed
The file selector window opening over and over doesn't happen anymore
If a wrong QR image is being uploaded a message indicating such error is displayed
Checked that the address validation still works (loading a MM account as a Safe won't work for example)

It looks good to me

ghost · 2020-11-18T14:36:06Z

Travis automatic deployment:
https://pr1612--safereact.review.gnosisdev.com/volta/app

ghost · 2020-11-18T14:37:38Z

Travis automatic deployment:
https://pr1612--safereact.review.gnosisdev.com/rinkeby/app

ghost · 2020-11-19T12:41:21Z

Travis automatic deployment:
https://pr1612--safereact.review.gnosisdev.com/rinkeby/app

ghost · 2020-11-19T12:45:31Z

Travis automatic deployment:
https://pr1612--safereact.review.gnosisdev.com/volta/app

Agupane added 3 commits November 16, 2020 13:08

Add types

42d69ca

Fix missing address in input field

871a88e

Fix reopening file upload modal once closed

ca2bdca

Agupane self-assigned this Nov 16, 2020

Agupane requested review from dasanra and fernandomg November 16, 2020 18:31

Agupane requested a review from francovenica November 16, 2020 18:31

fernandomg reviewed Nov 16, 2020

View reviewed changes

src/components/ScanQRModal/index.tsx Outdated Show resolved Hide resolved

Error first callback onFileUploadHandlerClose

30a9542

Agupane added 2 commits November 17, 2020 11:39

Adds error text for qrs that can't be read

1425f11

Fixes popping up again for wrong qr codes

Fix opening modal

64ec1b4

Merge branch 'development' into fix/1605-qr-input-address-fix

1b576c5

Agupane requested a review from fernandomg November 17, 2020 15:15

nicosampler reviewed Nov 17, 2020

View reviewed changes

Merge branch 'development' into fix/1605-qr-input-address-fix

1dff834

nicosampler approved these changes Nov 17, 2020

View reviewed changes

francovenica approved these changes Nov 17, 2020

View reviewed changes

fernandomg approved these changes Nov 18, 2020

View reviewed changes

Merge branch 'development' into fix/1605-qr-input-address-fix

73594b6

Merge branch 'development' into fix/1605-qr-input-address-fix

fd8ceca

dasanra approved these changes Nov 19, 2020

View reviewed changes

Agupane merged commit e482d32 into development Nov 19, 2020

Agupane deleted the fix/1605-qr-input-address-fix branch November 19, 2020 15:22

github-actions bot locked and limited conversation to collaborators Nov 19, 2020

Conversation

Agupane commented Nov 16, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

github-actions bot commented Nov 16, 2020

Uh oh!

github-actions bot commented Nov 16, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

ESLint Summary View Full Report

[warning] @typescript-eslint/explicit-module-boundary-types

Uh oh!

ghost commented Nov 16, 2020

Uh oh!

ghost commented Nov 16, 2020

Uh oh!

fernandomg left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

ghost commented Nov 17, 2020

Uh oh!

ghost commented Nov 17, 2020

Uh oh!

ghost commented Nov 17, 2020

Uh oh!

ghost commented Nov 17, 2020

Uh oh!

ghost commented Nov 17, 2020

Uh oh!

ghost commented Nov 17, 2020

Uh oh!

Agupane commented Nov 17, 2020

Uh oh!

ghost commented Nov 17, 2020

Uh oh!

ghost commented Nov 17, 2020

Uh oh!

nicosampler Nov 17, 2020

Choose a reason for hiding this comment

Uh oh!

Agupane Nov 17, 2020

Choose a reason for hiding this comment

Uh oh!

nicosampler Nov 17, 2020

Choose a reason for hiding this comment

Uh oh!

Agupane Nov 17, 2020

Choose a reason for hiding this comment

Uh oh!

nicosampler left a comment

Choose a reason for hiding this comment

Uh oh!

ghost commented Nov 17, 2020

Uh oh!

ghost commented Nov 17, 2020

Uh oh!

francovenica commented Nov 17, 2020

Uh oh!

ghost commented Nov 18, 2020

Uh oh!

ghost commented Nov 18, 2020

Uh oh!

ghost commented Nov 19, 2020

Uh oh!

ghost commented Nov 19, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

Agupane commented Nov 16, 2020 •

edited

Loading

github-actions bot commented Nov 16, 2020 •

edited

Loading