From 5a2c777052de2dd606f107a5952371c8448766fe Mon Sep 17 00:00:00 2001 From: fernandomg Date: Fri, 27 Nov 2020 18:56:41 -0300 Subject: [PATCH 1/5] fix types --- src/components/Stepper/index.tsx | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/components/Stepper/index.tsx b/src/components/Stepper/index.tsx index 0147677fbf..07ee84979f 100644 --- a/src/components/Stepper/index.tsx +++ b/src/components/Stepper/index.tsx @@ -26,7 +26,7 @@ export interface StepperPageFormProps { } interface StepperPageProps { - validate?: (...args: unknown[]) => undefined | string[] | Promise> + validate?: (...args: unknown[]) => undefined | Record | Promise> component: ( ...args: unknown[] ) => (controls: React.ReactElement, formProps: StepperPageFormProps) => React.ReactElement From 4a621ae3ee31a52ffc1c3cc9d9457483ada60540 Mon Sep 17 00:00:00 2001 From: fernandomg Date: Fri, 27 Nov 2020 19:02:24 -0300 Subject: [PATCH 2/5] add form level validation for OwnersForm - also fixed `calculateValuesAfterRemoving` function that removed an owner's row by clicking on the trash icon --- src/routes/open/components/Layout.tsx | 4 +- .../SafeOwnersConfirmationsForm/index.tsx | 75 ++++++++++++++----- 2 files changed, 59 insertions(+), 20 deletions(-) diff --git a/src/routes/open/components/Layout.tsx b/src/routes/open/components/Layout.tsx index 78e5a43fea..45385f05e4 100644 --- a/src/routes/open/components/Layout.tsx +++ b/src/routes/open/components/Layout.tsx @@ -9,7 +9,7 @@ import Row from 'src/components/layout/Row' import { instantiateSafeContracts } from 'src/logic/contracts/safeContracts' import { Review } from 'src/routes/open/components/ReviewInformation' import SafeNameField from 'src/routes/open/components/SafeNameForm' -import { SafeOwnersPage } from 'src/routes/open/components/SafeOwnersConfirmationsForm' +import { SafeOwnersPage, validateOwnersForm } from 'src/routes/open/components/SafeOwnersConfirmationsForm' import { FIELD_CONFIRMATIONS, FIELD_CREATION_PROXY_SALT, @@ -133,7 +133,7 @@ export const Layout = (props: LayoutProps): React.ReactElement => { testId="create-safe-form" > - + diff --git a/src/routes/open/components/SafeOwnersConfirmationsForm/index.tsx b/src/routes/open/components/SafeOwnersConfirmationsForm/index.tsx index 76494f54c5..a14698421b 100644 --- a/src/routes/open/components/SafeOwnersConfirmationsForm/index.tsx +++ b/src/routes/open/components/SafeOwnersConfirmationsForm/index.tsx @@ -4,7 +4,6 @@ import { makeStyles } from '@material-ui/core/styles' import CheckCircle from '@material-ui/icons/CheckCircle' import * as React from 'react' import { styles } from './style' -import { getAddressValidator } from './validators' import QRIcon from 'src/assets/icons/qrcode.svg' import trash from 'src/assets/icons/trash.svg' @@ -21,6 +20,7 @@ import { noErrorsOn, required, minMaxLength, + ADDRESS_REPEATED_ERROR, } from 'src/components/forms/validator' import Block from 'src/components/layout/Block' import Button from 'src/components/layout/Button' @@ -44,30 +44,70 @@ const { useState } = React export const ADD_OWNER_BUTTON = '+ Add another owner' -export const calculateValuesAfterRemoving = (index, notRemovedOwners, values) => { - const initialValues = { ...values } +/** + * Validates the whole OwnersForm, specially checks for non-repeated addresses + * + * If finds a repeated address, marks it as invalid + * @param {Object} values + * @return Object + */ +export const validateOwnersForm = (values: Record): Record => { + const { errors } = Object.keys(values).reduce( + (result, key) => { + if (/owner\d+Address/.test(key)) { + const address = values[key].toLowerCase() - const numOwnersAfterRemoving = notRemovedOwners - 1 + if (result.addresses.includes(address)) { + result.errors[key] = ADDRESS_REPEATED_ERROR + } - for (let i = index; i < numOwnersAfterRemoving; i += 1) { - initialValues[getOwnerNameBy(i)] = values[getOwnerNameBy(i + 1)] - initialValues[getOwnerAddressBy(i)] = values[getOwnerAddressBy(i + 1)] - } + result.addresses.push(address) + } + return result + }, + { addresses: [] as string[], errors: {} }, + ) + return errors +} - if (+values[FIELD_CONFIRMATIONS] === notRemovedOwners) { - initialValues[FIELD_CONFIRMATIONS] = numOwnersAfterRemoving.toString() - } +export const calculateValuesAfterRemoving = (index: number, values: Record): Record => + Object.keys(values) + .sort() + .reduce((newValues, key) => { + const ownerRelatedField = /owner(\d+)(Name|Address)/ - delete initialValues[getOwnerNameBy(index)] - delete initialValues[getOwnerAddressBy(index)] + if (!ownerRelatedField.test(key)) { + // no owner-related field + newValues[key] = values[key] + return newValues + } - return initialValues -} + const ownerToRemove = new RegExp(`owner${index}(Name|Address)`) + + if (ownerToRemove.test(key)) { + // skip, doing anything with the removed field + return newValues + } + + // we only have the owner-related fields to work with + // we must reduce the index value for those owners that come after the deleted owner row + const [, ownerOrder, ownerField] = key.match(ownerRelatedField) as RegExpMatchArray + + if (Number(ownerOrder) > index) { + // reduce by one the order of the owner + newValues[`owner${Number(ownerOrder) - 1}${ownerField}`] = values[key] + } else { + // previous owners to the deleted row + newValues[key] = values[key] + } + + return newValues + }, {} as Record) const useStyles = makeStyles(styles) const SafeOwnersForm = (props): React.ReactElement => { - const { errors, form, otherAccounts, values } = props + const { errors, form, values } = props const classes = useStyles() const validOwners = getNumOwnersFrom(values) @@ -87,7 +127,7 @@ const SafeOwnersForm = (props): React.ReactElement => { } const onRemoveRow = (index) => () => { - const initialValues = calculateValuesAfterRemoving(index, numOwners, values) + const initialValues = calculateValuesAfterRemoving(index, values) form.reset(initialValues) setNumOwners(numOwners - 1) @@ -171,7 +211,6 @@ const SafeOwnersForm = (props): React.ReactElement => { name={addressName} placeholder="Owner Address*" text="Owner Address" - validators={[getAddressValidator(otherAccounts, index)]} testId={`create-safe-address-field-${index}`} /> From 3604dfcdc8b16557a9521690b4110ced4696bdff Mon Sep 17 00:00:00 2001 From: fernandomg Date: Tue, 1 Dec 2020 10:32:02 -0300 Subject: [PATCH 3/5] add tests for `calculateValuesAfterRemoving` function --- .../calculateValuesAfterRemoving.test.ts | 51 +++++++++++++++++++ 1 file changed, 51 insertions(+) create mode 100644 src/routes/open/components/SafeOwnersConfirmationsForm/tests/calculateValuesAfterRemoving.test.ts diff --git a/src/routes/open/components/SafeOwnersConfirmationsForm/tests/calculateValuesAfterRemoving.test.ts b/src/routes/open/components/SafeOwnersConfirmationsForm/tests/calculateValuesAfterRemoving.test.ts new file mode 100644 index 0000000000..6ce50ec36e --- /dev/null +++ b/src/routes/open/components/SafeOwnersConfirmationsForm/tests/calculateValuesAfterRemoving.test.ts @@ -0,0 +1,51 @@ +import { calculateValuesAfterRemoving } from 'src/routes/open/components/SafeOwnersConfirmationsForm/index' + +describe('calculateValuesAfterRemoving', () => { + it(`should properly remove the last owner row`, () => { + // Given + const formContent = { + name: 'My Safe', + owner0Name: 'Owner 0', + owner0Address: '0x90F8bf6A479f320ead074411a4B0e7944Ea8c9C1', + owner1Name: 'Owner 1', + owner1Address: '0xFFcf8FDEE72ac11b5c542428B35EEF5769C409f0', + } + + // When + const newFormContent = calculateValuesAfterRemoving(1, formContent) + + // Then + expect(newFormContent).toStrictEqual({ + name: 'My Safe', + owner0Name: 'Owner 0', + owner0Address: '0x90F8bf6A479f320ead074411a4B0e7944Ea8c9C1', + }) + }) + + it(`should properly remove an owner and recalculate fields indices`, () => { + // Given + const formContent = { + name: 'My Safe', + owner0Name: 'Owner 0', + owner0Address: '0x90F8bf6A479f320ead074411a4B0e7944Ea8c9C1', + owner1Name: 'Owner 1', + owner1Address: '0xFFcf8FDEE72ac11b5c542428B35EEF5769C409f0', + owner2Name: 'Owner 2', + owner2Address: '0x22d491Bde2303f2f43325b2108D26f1eAbA1e32b', + } + + + // When + const newFormContent = calculateValuesAfterRemoving(1, formContent) + + // Then + expect(newFormContent).toStrictEqual({ + name: 'My Safe', + owner0Name: 'Owner 0', + owner0Address: '0x90F8bf6A479f320ead074411a4B0e7944Ea8c9C1', + owner1Name: 'Owner 2', + owner1Address: '0x22d491Bde2303f2f43325b2108D26f1eAbA1e32b', + } + ) + }) +}) From 954c9422c1d26eea9e00c4ebd0a0e4841d0be4f2 Mon Sep 17 00:00:00 2001 From: fernandomg Date: Tue, 1 Dec 2020 11:21:20 -0300 Subject: [PATCH 4/5] reformat with prettier --- .../tests/calculateValuesAfterRemoving.test.ts | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/routes/open/components/SafeOwnersConfirmationsForm/tests/calculateValuesAfterRemoving.test.ts b/src/routes/open/components/SafeOwnersConfirmationsForm/tests/calculateValuesAfterRemoving.test.ts index 6ce50ec36e..e039ce9853 100644 --- a/src/routes/open/components/SafeOwnersConfirmationsForm/tests/calculateValuesAfterRemoving.test.ts +++ b/src/routes/open/components/SafeOwnersConfirmationsForm/tests/calculateValuesAfterRemoving.test.ts @@ -1,4 +1,4 @@ -import { calculateValuesAfterRemoving } from 'src/routes/open/components/SafeOwnersConfirmationsForm/index' +import { calculateValuesAfterRemoving } from 'src/routes/open/components/SafeOwnersConfirmationsForm' describe('calculateValuesAfterRemoving', () => { it(`should properly remove the last owner row`, () => { @@ -45,7 +45,7 @@ describe('calculateValuesAfterRemoving', () => { owner0Address: '0x90F8bf6A479f320ead074411a4B0e7944Ea8c9C1', owner1Name: 'Owner 2', owner1Address: '0x22d491Bde2303f2f43325b2108D26f1eAbA1e32b', - } + }, ) }) }) From fdc843e590b7724dcfcb8181654a2fbf307dc426 Mon Sep 17 00:00:00 2001 From: fernandomg Date: Fri, 4 Dec 2020 19:27:00 -0300 Subject: [PATCH 5/5] prettier:fix --- .../tests/calculateValuesAfterRemoving.test.ts | 14 ++++++-------- 1 file changed, 6 insertions(+), 8 deletions(-) diff --git a/src/routes/open/components/SafeOwnersConfirmationsForm/tests/calculateValuesAfterRemoving.test.ts b/src/routes/open/components/SafeOwnersConfirmationsForm/tests/calculateValuesAfterRemoving.test.ts index e039ce9853..a305388c03 100644 --- a/src/routes/open/components/SafeOwnersConfirmationsForm/tests/calculateValuesAfterRemoving.test.ts +++ b/src/routes/open/components/SafeOwnersConfirmationsForm/tests/calculateValuesAfterRemoving.test.ts @@ -34,18 +34,16 @@ describe('calculateValuesAfterRemoving', () => { owner2Address: '0x22d491Bde2303f2f43325b2108D26f1eAbA1e32b', } - // When const newFormContent = calculateValuesAfterRemoving(1, formContent) // Then expect(newFormContent).toStrictEqual({ - name: 'My Safe', - owner0Name: 'Owner 0', - owner0Address: '0x90F8bf6A479f320ead074411a4B0e7944Ea8c9C1', - owner1Name: 'Owner 2', - owner1Address: '0x22d491Bde2303f2f43325b2108D26f1eAbA1e32b', - }, - ) + name: 'My Safe', + owner0Name: 'Owner 0', + owner0Address: '0x90F8bf6A479f320ead074411a4B0e7944Ea8c9C1', + owner1Name: 'Owner 2', + owner1Address: '0x22d491Bde2303f2f43325b2108D26f1eAbA1e32b', + }) }) })