Skip to content
This repository was archived by the owner on Nov 10, 2023. It is now read-only.

(Fix) Duplicated address validation#1699

Merged
dasanra merged 12 commits intodevelopmentfrom
fix/1673-validate-address-duplicates
Dec 10, 2020
Merged

(Fix) Duplicated address validation#1699
dasanra merged 12 commits intodevelopmentfrom
fix/1673-validate-address-duplicates

Conversation

@fernandomg
Copy link
Contributor

This PR closes #1673, by:

  • removing the memoization for the addressBookAddressesListSelector (see 52315cd)

The rest was me trying to understand what was going on and trying to fix the AddressBook issue. After fixing the selector issue, I checked the Owner's related bugs and all were fixed as well.


In regard to the createSelector, I tried using createSelectorCreator, with different strategies (using lodash.memoize, defaultMemoize, with different setups) with no luck.

I prefer having these calls memoized, but couldn't make it work for the time being.

as memoization in this scenario is not working as expected and list is not refreshed
- `selectedEntry` being `null` made the code harder to follow
-
@github-actions
Copy link

github-actions bot commented Dec 4, 2020

CLA Assistant Lite All Contributors have signed the CLA.

@fernandomg fernandomg changed the title (Fix) Address duplicated validation (Fix) Duplicated address validation Dec 4, 2020
@github-actions
Copy link

github-actions bot commented Dec 4, 2020

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 0 0
Ignored 1 N/A
  • Result: ✅ success
  • Annotations: 0 total

Report generated by eslint-plus-action

@francovenica
Copy link
Contributor

Travis is giving troubles so I had to bring the branch to my local env and test there

Tried to add a duplicated address into the address book and when adding/replacing an owner and the input is now recognizing the duplication

Looks good to me

image.png

@ghost
Copy link

ghost commented Dec 4, 2020

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

@ghost
Copy link

ghost commented Dec 4, 2020

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

@ghost
Copy link

ghost commented Dec 4, 2020

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

@ghost
Copy link

ghost commented Dec 5, 2020

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

@ghost
Copy link

ghost commented Dec 9, 2020

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

@ghost
Copy link

ghost commented Dec 9, 2020

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

1 similar comment
@ghost
Copy link

ghost commented Dec 9, 2020

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

@ghost
Copy link

ghost commented Dec 10, 2020

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


return undefined
export const uniqueAddress = (addresses: string[] | List<string> = []) => (address?: string): string | undefined => {
const addressExists = addresses.some(sameAddress.bind(null, address))
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I feel I have some readability issues here. I would do it in one more line but more explicit what is happening here.

const addressExists = addresses.some((addressFromList) => {
  return sameAddress(addressFromList, address)
})

What do you think?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

😆 Yes, sure.

To be honest, I came looking for this comment.

I must have been nostalgic when I did

@fernandomg fernandomg requested a review from dasanra December 10, 2020 14:10
@ghost
Copy link

ghost commented Dec 10, 2020

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

@ghost
Copy link

ghost commented Dec 10, 2020

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

@dasanra dasanra merged commit 71f1ea7 into development Dec 10, 2020
@dasanra dasanra deleted the fix/1673-validate-address-duplicates branch December 10, 2020 15:07
@github-actions github-actions bot locked and limited conversation to collaborators Dec 10, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Validate duplicated addresses in the address book and settings

4 participants