(Bugfix) - #1246 Addressbook entries removed when reloading page by Agupane · Pull Request #1300 · 5afe/safe-react

Agupane · 2020-08-28T13:34:48Z

Closes #1246
Also closes #558

Description

Add types, removes duplicated code
Simplifies the addressBook state, previously was like: {addressBook:{addressBook:[]}}, now it just: {addressBook: []}
The error was caused basically because we were saving an empty addressBook even if the addressBook was empty

Restructure addressbook store type

…to 1246-addressbook-entries-removed

github-actions · 2020-08-28T13:35:04Z

CLA Assistant Lite All Contributors have signed the CLA.

github-actions · 2020-08-28T13:40:23Z

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	10	0
Ignored	0	N/A

Result: ✅ success
Annotations: 10 total

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

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

src/logic/addressBook/store/middleware/addressBookMiddleware.ts Line 13 - Argument 'store' should be typed.
src/logic/addressBook/store/middleware/addressBookMiddleware.ts Line 13 - Argument 'next' should be typed.
src/logic/addressBook/store/middleware/addressBookMiddleware.ts Line 13 - Missing return type on function.
src/logic/addressBook/store/middleware/addressBookMiddleware.ts Line 13 - Argument 'action' should be typed.
src/logic/safe/store/middleware/safeStorage.ts Line 72 - Argument 'store' should be typed.
src/logic/safe/store/middleware/safeStorage.ts Line 72 - Argument 'next' should be typed.
src/logic/safe/store/middleware/safeStorage.ts Line 72 - Missing return type on function.
src/logic/safe/store/middleware/safeStorage.ts Line 72 - Argument 'action' should be typed.
src/routes/safe/components/Balances/SendModal/screens/SendCollectible/index.tsx Line 46 - Object pattern argument should be typed.
src/routes/safe/components/Balances/SendModal/screens/SendFunds/index.tsx Line 51 - Object pattern argument should be typed.

^{Report generated by eslint-plus-action}

ghost · 2020-08-28T13:47:33Z

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

mmv08 · 2020-08-28T14:12:09Z

Simplifies the addressBook state, previously was like: {addressBook:{addressBook:[]}}, now it just: {addressBook: []}

What happens to users who have old structure in their storage?

Agupane · 2020-08-28T14:21:46Z

Simplifies the addressBook state, previously was like: {addressBook:{addressBook:[]}}, now it just: {addressBook: []}

What happens to users who have old structure in their storage?

That's not a problem, I updated how we are managing the state on redux, not how we are storing it on localStorage, you can do the test using this preview branch and another one if you like

…to 1246-addressbook-entries-removed

francovenica · 2020-08-31T14:29:38Z

The address book starts empty (normal behavior), and I started to add names to it and they were saved correctly.
But after refreshing the page a couple of times the list suddenly filled itself with all the owner of that safe, but without their name, probably because they are called all "Owner#N" and there is a condition to hide those names and the "UNKNOWN" ones as well

Chances are that this are 2 different issues, one is the address book being filled on its own and the other is not showing the name for being a name by default

Steps:
Enter in the address book. If a safe was never load in the PR the address book will be empty
Create an entry in the address book
Go to send funds and use the recipient field to search for the entry created.
Close the form and go back to the address book
If you refresh the page, it loads all the owners of the safe as shown in the snapshot

https://pr1300--safereact.review.gnosisdev.com/app/#/safes/0x9913B9180C20C6b0F21B6480c84422F6ebc4B808/address-book

src/logic/addressBook/store/actions/saveAndUpdateAddressBook.ts

fernandomg

Aside from what Franco pointed out, another use case is that it fails to persist the already stored names across Safes. That is:

Load an existing Safe and set some names for the owners
Create a new Safe, with one owner (this is irrelevant, I guess, but it's what I did)
Switch to the previously loaded Safe
Switch back to the newly created Safe, go to address book tab (you'll see all the previously loaded Safe owners, but without a name being listed)
Obviously, if you set a name in the newly created Safe, it will overwrite the name for the previously loaded one.

Agupane · 2020-09-01T14:32:59Z

As fas as I investigated, most of this issues are related to a race condition between the load of the safe and the load of the address book, we are currently syncing the owners of each of the safes and there are times in which the safe is loaded before the addressBook. I think that the best solution should be the implementation of this refactor in order to have an unique source of truth and avoid this sync of owners between safes. What do you think? @lukasschor

…to 801-address-book-refactor # Conflicts: # src/logic/addressBook/store/actions/addAddressBook.js # src/logic/addressBook/store/actions/addAddressBookEntry.js # src/logic/addressBook/store/actions/addOrUpdateAddressBookEntry.js # src/logic/addressBook/store/actions/loadAddressBookFromStorage.js # src/logic/addressBook/store/actions/removeAddressBookEntry.js # src/logic/addressBook/store/actions/updateAddressBookEntry.js # src/logic/addressBook/store/reducer/addressBook.ts # src/logic/addressBook/store/selectors/index.js # src/routes/safe/components/AddressBook/index.tsx # src/routes/safe/components/Settings/ManageOwners/AddOwnerModal/index.tsx # src/routes/safe/components/Settings/ManageOwners/EditOwnerModal/index.tsx # src/routes/safe/components/Settings/ManageOwners/ReplaceOwnerModal/index.tsx # src/routes/safe/container/Hooks/useLoadSafe.jsx # src/routes/safe/container/index.jsx # src/routes/safe/store/middleware/safeStorage.js

Remove unused saveAndUpdateAddressBook action

Add types Removes unused addAddressBook action

Let the user remove owners users without adding them again each time the safe loads

…ons on addAddressBookEntry

Agupane · 2020-09-15T15:29:10Z

I'm not sure if the current behavior.

I opened this locally, with already stored addresses in the AddressBook, nothing was listed. So I assume this is a destructive change?

After cleaning the storage, no address was preloaded. As I loaded the Safe using the URL (not the Load Storage form), owners weren't loaded either.

This last item makes the thing a bit hard to handle.

I'm not being told what should I do to have the owners loaded.

I added a migration function together with tests to avoid the destruction of the old addressBook, please check it out

ghost · 2020-09-15T15:38:57Z

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

…to 1246-addressbook-entries-removed

ghost · 2020-09-16T13:47:22Z

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

src/logic/addressBook/store/actions/addAddressBookEntry.ts

src/logic/addressBook/utils/index.ts

ghost · 2020-09-18T12:56:07Z

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

ghost · 2020-09-18T15:04:11Z

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

…' into 1246-addressbook-entries-removed

ghost · 2020-09-18T15:20:09Z

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

ghost · 2020-09-18T15:28:03Z

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

ghost · 2020-09-21T14:38:34Z

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

francovenica · 2020-09-21T17:51:06Z

The issues reported in #1246 and #558 were solved

There is an inconsistency how address book is working regarding the unknown names, and addresses being added automatically sometimes. This is going to be discussion on when and how implement it.

But since the 2 main issues are solved here I'll approve the PR

ghost · 2020-09-21T19:11:29Z

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

Agupane added 8 commits August 26, 2020 11:24

Fix addressbook types

b02199b

Restructure addressbook store type

Add more safe types

aac0bbb

Merge branch 'development' of https://github.com/gnosis/safe-react in…

51d2263

…to 1246-addressbook-entries-removed

Fix imports

383f72b

Removes .toJS() usage

5f1ce43

Fix condition for saving addressBook

f7cdd5c

Types & remove send button from addressbook if user not an owner

15bf738

Merge branch 'development' of https://github.com/gnosis/safe-react in…

fd15391

…to 1246-addressbook-entries-removed

Agupane self-assigned this Aug 28, 2020

Agupane requested review from francovenica and mmv08 August 28, 2020 13:52

Merge branch 'development' of https://github.com/gnosis/safe-react in…

a74db59

…to 1246-addressbook-entries-removed

fernandomg approved these changes Aug 31, 2020

View reviewed changes

fernandomg self-requested a review August 31, 2020 18:14

mmv08 reviewed Sep 1, 2020

View reviewed changes

src/logic/addressBook/store/actions/saveAndUpdateAddressBook.ts Outdated Show resolved Hide resolved

fernandomg suggested changes Sep 1, 2020

View reviewed changes

Agupane added 7 commits September 4, 2020 08:36

Add types for addressBook actions

62aec5b

Remove unused saveAndUpdateAddressBook action

Refactor addressBook: make it global and removes immutableJS

c1503bb

Add types Removes unused addAddressBook action

Remove todo

d1efe8c

Fix edit and remove entries style when user is not owner

7985638

Adds and updates safe name in addressBook

1065d8f

Adds checkIfOwnerWasDeletedFromAddressBook

2539623

Let the user remove owners users without adding them again each time the safe loads

Agupane added 3 commits September 15, 2020 11:20

Replaces shouldAvoidUpdatesNotifications with addAddressBookEntryOpti…

6d5809c

…ons on addAddressBookEntry

Update tests

51dc123

Unify return on getOwnersWithNameFromAddressBook

c082155

Agupane requested a review from fernandomg September 15, 2020 15:29

Merge branch 'development' of https://github.com/gnosis/safe-react in…

799779b

…to 1246-addressbook-entries-removed

fernandomg suggested changes Sep 17, 2020

View reviewed changes

src/logic/addressBook/store/actions/addAddressBookEntry.ts Show resolved Hide resolved

src/logic/addressBook/utils/index.ts Outdated Show resolved Hide resolved

src/logic/addressBook/utils/index.ts Outdated Show resolved Hide resolved

Agupane added 2 commits September 18, 2020 09:41

Reword shouldAvoidUpdatesNotifications

38d92a1

Replaces adbk with addressBook

72365c7

fernandomg approved these changes Sep 18, 2020

View reviewed changes

Fix condition

aa407dc

Merge branch 'development' into 1246-addressbook-entries-removed

b6c8964

dasanra approved these changes Sep 18, 2020

View reviewed changes

Agupane added 2 commits September 18, 2020 12:13

Fix typos

9ba03d8

Merge remote-tracking branch 'origin/1246-addressbook-entries-removed…

e8405ca

…' into 1246-addressbook-entries-removed

Fix typo

bd01de5

francovenica approved these changes Sep 21, 2020

View reviewed changes

Merge branch 'development' into 1246-addressbook-entries-removed

49cd752

Agupane merged commit 59dc1f7 into development Sep 22, 2020

Agupane deleted the 1246-addressbook-entries-removed branch September 22, 2020 12:31

github-actions bot locked and limited conversation to collaborators Sep 22, 2020

Conversation

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

Uh oh!

Description

Uh oh!

github-actions bot commented Aug 28, 2020

Uh oh!

github-actions bot commented Aug 28, 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 Aug 28, 2020

Uh oh!

mmv08 commented Aug 28, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Agupane commented Aug 28, 2020

Uh oh!

francovenica commented Aug 31, 2020

Uh oh!

Uh oh!

fernandomg left a comment

Choose a reason for hiding this comment

Uh oh!

Agupane commented Sep 1, 2020

Uh oh!

Agupane commented Sep 15, 2020

Uh oh!

ghost commented Sep 15, 2020

Uh oh!

ghost commented Sep 16, 2020

Uh oh!

Uh oh!

Uh oh!

Uh oh!

ghost commented Sep 18, 2020

Uh oh!

ghost commented Sep 18, 2020

Uh oh!

ghost commented Sep 18, 2020

Uh oh!

ghost commented Sep 18, 2020

Uh oh!

ghost commented Sep 21, 2020

Uh oh!

francovenica commented Sep 21, 2020

Uh oh!

ghost commented Sep 21, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

Agupane commented Aug 28, 2020 •

edited

Loading

github-actions bot commented Aug 28, 2020 •

edited

Loading

mmv08 commented Aug 28, 2020 •

edited

Loading