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

Export acc js #4973

Merged
merged 47 commits into from Apr 26, 2017
Merged

Export acc js #4973

merged 47 commits into from Apr 26, 2017

Conversation

CraigglesO
Copy link
Contributor

Export accounts as json files
JS from #4967

some typos and need to finish adding in the api. Super close.

alt-img

@parity-cla-bot
Copy link

It looks like @CraigglesO hasn'signed our Contributor License Agreement, yet.

The purpose of a CLA is to ensure that the guardian of a project's outputs has the necessary ownership or grants of rights over all contributions to allow them to distribute under the chosen licence.
Wikipedia

You can read and sign our full Contributor License Agreement at the following URL: https://cla.ethcore.io

Once you've signed, plesae reply to this thread with [clabot:check] to prove it.

Many thanks,

Parity Technologies CLA Bot

@CraigglesO CraigglesO added A3-inprogress ⏳ Pull request is in progress. No review needed at this stage. M7-ui labels Mar 21, 2017
@CraigglesO
Copy link
Contributor Author

[clabot:check]

@parity-cla-bot
Copy link

It looks like @CraigglesO signed our Contributor License Agreement. 👍

Many thanks,

Parity Technologies CLA Bot

@CraigglesO
Copy link
Contributor Author

Done.

Ice Ice baby ;)

fail:
alt
success:
alt-2

@CraigglesO CraigglesO added A0-pleasereview 🤓 Pull request needs code review. and removed A3-inprogress ⏳ Pull request is in progress. No review needed at this stage. labels Mar 21, 2017
@jacogr jacogr added the A1-onice 🌨 Pull request is reviewed well, but should not yet be merged. label Mar 21, 2017
@jacogr
Copy link
Contributor

jacogr commented Mar 21, 2017

Marked onice for merging until #4967 merged/available.

@ngotchac
Copy link
Contributor

Closes #4824

@jacogr
Copy link
Contributor

jacogr commented Mar 22, 2017

As discussed - We need this on the Account page as well, i.e. going into account, being a able to export from there.

jacogr
jacogr reviewed Mar 22, 2017
View reviewed changes
js/src/modals/ExportAccount/ExportAccount.js Outdated
accounts: PropTypes.object.isRequired,
balances: PropTypes.object.isRequired,
newError: PropTypes.func.isRequired,
personalAccountsInfo: PropTypes.func.isRequired,
Copy link
Contributor

Choose a reason for hiding this comment

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

Could we keep this as accountsInfo (just consistent with what we have everywhere else)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

pesonalAccountsInfo? It's a redux action pulled in connect->dispatch. Lol, it does look like I added it in though.

Copy link
Contributor

Choose a reason for hiding this comment

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

We are not using this anywhere? (Should be removed)

jacogr
jacogr reviewed Mar 22, 2017
View reviewed changes
js/src/modals/ExportAccount/ExportAccount.js Outdated

static ExportStore = ExportStore;

componentDidMount () {
Copy link
Contributor

Choose a reason for hiding this comment

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

Remove if not used.

jacogr
jacogr reviewed Mar 22, 2017
View reviewed changes
js/src/modals/ExportAccount/ExportAccount.js Outdated
onClose: PropTypes.func.isRequired
};

static ExportStore = ExportStore;
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there a specific reason this is not on the instance? Basically doing it twice will (on the second run), have the same selection/values as on the first.

jacogr
jacogr reviewed Mar 22, 2017
View reviewed changes
js/src/modals/ExportAccount/ExportAccount.js Outdated
.then((content) => {
const text = JSON.stringify(content, null, 4);
const blob = new Blob([ text ], { type: 'application/json' });
const filename = accounts[account].name;
Copy link
Contributor

Choose a reason for hiding this comment

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

We should stick to the UUID naming as per Parity for the sake of consistency.

jacogr
jacogr reviewed Mar 22, 2017
View reviewed changes
js/src/modals/ExportAccount/ExportAccount.js Outdated
const { parity } = this.context.api;
const { accounts, newError } = this.props;

parity.exportAccount(account, password)
Copy link
Contributor

Choose a reason for hiding this comment

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

Would move the interaction to the store. (As well as onExport, i.e. all logic in the actual store as opposed to the component)

jacogr
jacogr reviewed Mar 22, 2017
View reviewed changes
js/src/ui/AccountCard/accountCard.js Outdated
@@ -32,19 +34,22 @@ export default class AccountCard extends Component {
className: PropTypes.string,
disableAddressClick: PropTypes.bool,
onClick: PropTypes.func,
onFocus: PropTypes.func
onFocus: PropTypes.func,
password: PropTypes.bool,
Copy link
Contributor

Choose a reason for hiding this comment

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

showPassword? (On first read-through it looked like passing the actual password)

jacogr
jacogr reviewed Mar 22, 2017
View reviewed changes
js/src/ui/AccountCard/accountCard.js Outdated
@@ -88,6 +93,7 @@ export default class AccountCard extends Component {
className={ styles.balance }
showOnlyEth
/>
{ (password) ? this.renderPassword() : null }
Copy link
Contributor

Choose a reason for hiding this comment

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

@ngotchac Happy with the approach or would passing children from the renderer be more appropriate? (Not unhappy, just concerned that this may blow up as we add more stuff - and currently the password is a single-instance use as well)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I was thinking about this too. Should really just do children, as I could see in the future using accountCards in this way.

@jacogr
Copy link
Contributor

jacogr commented Mar 22, 2017

Some grumbles around the store and moving the actual RPCs there, in addition one question for @ngotchac around the password, may possibly be better to have a more generic this.props.children renderer instead.

@jacogr jacogr added A5-grumble 🔥 Pull request has minor issues that must be addressed before merging. and removed A0-pleasereview 🤓 Pull request needs code review. labels Mar 22, 2017
@ngotchac ngotchac removed the A0-pleasereview 🤓 Pull request needs code review. label Apr 5, 2017
@CraigglesO CraigglesO added A0-pleasereview 🤓 Pull request needs code review. and removed A4-gotissues 💥 Pull request is reviewed and has significant issues which must be addressed. labels Apr 10, 2017
@bjornwgnr bjornwgnr mentioned this pull request Apr 12, 2017
Closed
jacogr
jacogr reviewed Apr 19, 2017
View reviewed changes
js/src/modals/ExportAccount/exportStore.js
const { passwordHint } = meta;

this._newError({
message: `[${err.code}] Account "${name}" - Incorrect password. (Password Hint: ${passwordHint})`
Copy link
Contributor

Choose a reason for hiding this comment

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

Should rather have this string in the view or make it and explicit FormattedMessage here

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Dunno. None of this code works anymore with the latest updates.

  1. _pollStatus TypeError: Cannot convert undefined or null to object
  2. Unable to update callback for subscriptionId 2 TypeError: Cannot convert undefined or null to object
  3. Warning: Failed prop type: The prop 'balances' is marked as required in 'ExportAccount', but its value is 'undefined'.

So. It's a little more complicated then just a newError.

Although. I tried FormattedMessage and the newError ui does not handle it. Nor multiple errors.

https://github.com/paritytech/parity/blob/master/js/src/ui/Errors/errors.js#L59-L77

@jacogr jacogr added A8-looksgood 🦄 Pull request is reviewed well. A5-grumble 🔥 Pull request has minor issues that must be addressed before merging. and removed A0-pleasereview 🤓 Pull request needs code review. A8-looksgood 🦄 Pull request is reviewed well. labels Apr 19, 2017
@jacogr
Copy link
Contributor

jacogr commented Apr 19, 2017

LGTM, just one message to move.

@CraigglesO
Copy link
Contributor Author

CraigglesO commented Apr 24, 2017 •
edited

All errors are fixed. As discussed on riot, I don't know how to fix newError, it's an action that takes an object and the errors ui component requires message to be a string....

If you want me to change the way errors is built, can I do that in a separate PR and than just update this one?

@CraigglesO CraigglesO added A0-pleasereview 🤓 Pull request needs code review. and removed A5-grumble 🔥 Pull request has minor issues that must be addressed before merging. labels Apr 25, 2017
@jacogr jacogr added A8-looksgood 🦄 Pull request is reviewed well. and removed A0-pleasereview 🤓 Pull request needs code review. labels Apr 26, 2017
@jacogr jacogr merged commit cf904b6 into master Apr 26, 2017
@jacogr jacogr deleted the export-acc-js branch April 26, 2017 09:34
@ngotchac ngotchac mentioned this pull request May 5, 2017
Closed
@5chdn 5chdn added the B0-patch label May 18, 2017
@5chdn 5chdn mentioned this pull request Jul 10, 2017
Closed
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@jacogr jacogr jacogr left review comments

@ngotchac ngotchac ngotchac left review comments

Assignees
No one assigned
Labels
A8-looksgood 🦄 Pull request is reviewed well.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
7 participants
@CraigglesO @parity-cla-bot @jacogr @ngotchac @gavofyork @5chdn @tomusdrw