Skip to content
This repository has been archived by the owner on Nov 6, 2020. It is now read-only.

Real deleting accounts #3540

Merged
merged 10 commits into from
Nov 24, 2016
Merged

Real deleting accounts #3540

merged 10 commits into from
Nov 24, 2016

Conversation

gavofyork
Copy link
Contributor

@gavofyork gavofyork commented Nov 20, 2016

  • RPC
  • Test for RPC
  • parity.js binding
  • UI integration

@gavofyork gavofyork added A0-pleasereview 🤓 Pull request needs code review. M6-rpcapi 📣 RPC API. A3-inprogress ⏳ Pull request is in progress. No review needed at this stage. and removed A0-pleasereview 🤓 Pull request needs code review. labels Nov 20, 2016
@gavofyork
Copy link
Contributor Author

Last two should be down to @jacogr

@jacogr
Copy link
Contributor

jacogr commented Nov 21, 2016

@gavofyork
Copy link
Contributor Author

over to you, @jacogr .

@jacogr jacogr self-assigned this Nov 21, 2016
@jacogr
Copy link
Contributor

jacogr commented Nov 22, 2016

UI integration - https://youtu.be/4Ih7ITwbq-A

@gavofyork It would be good if we could have the same for the address_book.json interface. (Currently those are all marked as deleted, but not currently actually deleted - https://github.com/ethcore/parity/issues/3104)

Closes https://github.com/ethcore/parity/issues/3105

@jacogr jacogr added A0-pleasereview 🤓 Pull request needs code review. and removed A3-inprogress ⏳ Pull request is in progress. No review needed at this stage. labels Nov 22, 2016
@jacogr jacogr removed their assignment Nov 23, 2016
@jacogr jacogr added the M7-ui label Nov 23, 2016
@gavofyork gavofyork added A0-pleasereview 🤓 Pull request needs code review. and removed A0-pleasereview 🤓 Pull request needs code review. labels Nov 23, 2016
@gavofyork
Copy link
Contributor Author

will do the address book deletion in a separate PR.

Copy link
Collaborator

@tomusdrw tomusdrw left a comment

Choose a reason for hiding this comment

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

Minor grumbles

take_weak!(self.accounts)
.kill_account(&account, &password)
.map(|_| true)
.map_err(|e| errors::account("Could not fetch account info.", e))
Copy link
Collaborator

Choose a reason for hiding this comment

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

"Could not kill account?"

let res = tester.io.handle_request_sync(&request);
assert_eq!(res, Some(response.into()));

let request = r#"{"jsonrpc": "2.0", "method": "parity_accountsInfo", "params": [], "id": 1}"#;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Could be just a check in tester.accounts, no?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

where is tester.accounts?

onDeny={ this.closeDeleteDialog }
onConfirm={ this.onDeleteConfirmed }>
<div className={ styles.hero }>
Are you sure you want to remove permanently delete the following account?
Copy link
Collaborator

Choose a reason for hiding this comment

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

"remove permanently delete"

newError(new Error('Deletion failed.'));
}

this.closeDeleteDialog();
Copy link
Collaborator

Choose a reason for hiding this comment

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

Do we really want to close the dialog if there is an error?

.catch((error) => {
console.error('onDeleteConfirmed', error);
newError(new Error(`Deletion failed: ${error.message}`));
this.closeDeleteDialog();
Copy link
Collaborator

Choose a reason for hiding this comment

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

Do we really want to close the dialog if there is an error?

static propTypes = {
account: PropTypes.object.isRequired,
onClose: PropTypes.func,
newError: PropTypes.func
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why the functions are not required?

Consider static defaultProps = { onClose: () => {}, newError: () => () }

@tomusdrw tomusdrw added A5-grumble 🔥 Pull request has minor issues that must be addressed before merging. and removed A0-pleasereview 🤓 Pull request needs code review. labels Nov 23, 2016
@gavofyork
Copy link
Contributor Author

grumbles addressed on my part

@jacogr jacogr added A0-pleasereview 🤓 Pull request needs code review. and removed A5-grumble 🔥 Pull request has minor issues that must be addressed before merging. labels Nov 23, 2016
@gavofyork gavofyork added A8-looksgood 🦄 Pull request is reviewed well. and removed A0-pleasereview 🤓 Pull request needs code review. labels Nov 24, 2016
@gavofyork gavofyork merged commit 46e5a84 into master Nov 24, 2016
@gavofyork gavofyork deleted the delete-accounts branch November 24, 2016 16:16
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
A8-looksgood 🦄 Pull request is reviewed well. M6-rpcapi 📣 RPC API.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants