New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Get all transactions when exporting account #485

Merged
merged 9 commits into from Dec 30, 2017

Conversation

Projects
None yet
4 participants
@Nasicus
Contributor

Nasicus commented Dec 23, 2017

I implemented it so that it always gets 50 transactions per call (which I think is the max?).
When the export account button is clicked a modal dialog is shown, which stays open until either a successful or a fail result is returned.
If there occurs an exception in the middle of getting all transactions (e.g. 1500 were already received), then a file is still exported, but contains only these transactions.

Note that this takes VERY long on big accounts!

I tried this for Jarunik:

  • He has around 45'000 transactions
  • It took 8 minutes
  • It took around 900 API calls

I'm not sure if we should now show a selection to the user of how many transactions he wants to export (eg. 50, 100, 500, 1000, All) or even a selection for which time span the transaction should be exported (but I don't think the API offers this possibility yet? )

Well let me know what you guys think :)

Addresses: #483

@Nasicus

This comment has been minimized.

Show comment
Hide comment
@Nasicus

Nasicus Dec 24, 2017

Contributor

I wasn't satisfied with what I did.

So I implemented the dialog where the user can choose how many transactions to export.
Because I think we need this anyway.
The whole thing looks now like this:

demo

Question:

  • Is there no way to get the total number of transactions (in the ark-explorer there it's included when you get the account)
Contributor

Nasicus commented Dec 24, 2017

I wasn't satisfied with what I did.

So I implemented the dialog where the user can choose how many transactions to export.
Because I think we need this anyway.
The whole thing looks now like this:

demo

Question:

  • Is there no way to get the total number of transactions (in the ark-explorer there it's included when you get the account)
@zillionn

This comment has been minimized.

Show comment
Hide comment
@zillionn

zillionn Dec 24, 2017

Contributor

I believe the better approach is to select start and end date.

Contributor

zillionn commented Dec 24, 2017

I believe the better approach is to select start and end date.

@Nasicus

This comment has been minimized.

Show comment
Hide comment
@Nasicus

Nasicus Dec 24, 2017

Contributor

@zillionn
True. However I don't think the API supports that yet.

Let's see what the devs say :)

Edit:
Added unit tests for getAllTransactions

Contributor

Nasicus commented Dec 24, 2017

@zillionn
True. However I don't think the API supports that yet.

Let's see what the devs say :)

Edit:
Added unit tests for getAllTransactions

@alexbarnsley

This comment has been minimized.

Show comment
Hide comment
@alexbarnsley

alexbarnsley Dec 28, 2017

Member

I do think the date range would be the better approach. Since transactions are ordered pretty much in date order, you could keep getting pages (like you are at the minute) until you reach a specific date. I know it's a bit more of an intensive task, but that way you can guarantee exporting gives you all the transactions you need

Member

alexbarnsley commented Dec 28, 2017

I do think the date range would be the better approach. Since transactions are ordered pretty much in date order, you could keep getting pages (like you are at the minute) until you reach a specific date. I know it's a bit more of an intensive task, but that way you can guarantee exporting gives you all the transactions you need

Nasicus added some commits Dec 23, 2017

refactor export account functionality
- make it possible for the user to choose how many transactions to include
- refactor logic into own controller
@Nasicus

This comment has been minimized.

Show comment
Hide comment
@Nasicus

Nasicus Dec 29, 2017

Contributor

@alexbarnsley
Done... honestly it was much harder than I thought a little "in memory" filtering would be xD

When reviewing please pay special attention to:

  • getAllTransactions
  • getRangedTransactions

I wrote unit tests for both of these methods, however it would still be good if someone else could verify the logic of them!

Some screenshot on how it looks now:

image

image

Edit: fixed typo in screenshot

Contributor

Nasicus commented Dec 29, 2017

@alexbarnsley
Done... honestly it was much harder than I thought a little "in memory" filtering would be xD

When reviewing please pay special attention to:

  • getAllTransactions
  • getRangedTransactions

I wrote unit tests for both of these methods, however it would still be good if someone else could verify the logic of them!

Some screenshot on how it looks now:

image

image

Edit: fixed typo in screenshot

@alexbarnsley

This comment has been minimized.

Show comment
Hide comment
@alexbarnsley

alexbarnsley Dec 29, 2017

Member

Looks amazing! I'm getting the below issue though. When I open the date picker, it moves and hides the first digit of the date. The below screenshot should have "11/29/2017" in it but is only showing "1/29/2017"

image

Other than that, I love it and once that small UI tweak is made, I shall approve!

Member

alexbarnsley commented Dec 29, 2017

Looks amazing! I'm getting the below issue though. When I open the date picker, it moves and hides the first digit of the date. The below screenshot should have "11/29/2017" in it but is only showing "1/29/2017"

image

Other than that, I love it and once that small UI tweak is made, I shall approve!

@j-a-m-l j-a-m-l self-requested a review Dec 29, 2017

@j-a-m-l

j-a-m-l requested changes Dec 29, 2017 edited

I think that is necessary to notify the user that the process has finished:
I've exported an account and my impression, as a normal user, is that, since I don't see any message, the process could have failed silently. Adding a message with a "OK" button or similar would be enough, probably.

One small bug (from the previous work), is that the balance isn't correct: 9900000000 instead of 99.00000000.

Very good job, anyway.

@Nasicus

This comment has been minimized.

Show comment
Hide comment
@Nasicus

Nasicus Dec 29, 2017

Contributor

@alexbarnsley
Hmm I think it's a angular material bug, but I'll try to fix it (I noticed it's only when the wiindow is small, I can probably override left css attribute)

@j-a-m-l
Good point - I even thought myself of this. However I ... hmm let's be honest, was to lazy to do it :>
Noticed that too, thought that was by design (it's arktoshi instead of ark)

Contributor

Nasicus commented Dec 29, 2017

@alexbarnsley
Hmm I think it's a angular material bug, but I'll try to fix it (I noticed it's only when the wiindow is small, I can probably override left css attribute)

@j-a-m-l
Good point - I even thought myself of this. However I ... hmm let's be honest, was to lazy to do it :>
Noticed that too, thought that was by design (it's arktoshi instead of ark)

@Nasicus

This comment has been minimized.

Show comment
Hide comment
@Nasicus

Nasicus Dec 29, 2017

Contributor

Done with the 3 things !
It now looks like this when it's finished:

image

I would have preferred to leave the "progress bar", but show it as complete (which is possible) and add a "success mark" in the middle of it. However I don't think the progress circle can do that, so I let it be for now.

Contributor

Nasicus commented Dec 29, 2017

Done with the 3 things !
It now looks like this when it's finished:

image

I would have preferred to leave the "progress bar", but show it as complete (which is possible) and add a "success mark" in the middle of it. However I don't think the progress circle can do that, so I let it be for now.

@j-a-m-l

Awesome job, but some additional changes:

  • Add a cancel button or a way to dismiss the last modal instead of being force to download it.
  • Instead of exporting the smartbridge as undefined, keep it empty.

I haven't been able to reproduce the date-selector problem.

@Nasicus

This comment has been minimized.

Show comment
Hide comment
@Nasicus

Nasicus Dec 30, 2017

Contributor

@j-a-m-l
You probably cannot reproduce it because I fixed it :P Well anyway it happened when the dev tools were open and the windows was maybe like 1000px wide.

I moved the button to the button row and I remebered that I can use md-icon and now it loooks imho waaay better when it's finished. Also removed the now "redudant" success message: Because a picture is worth a thousands words :trollface:

image

Contributor

Nasicus commented Dec 30, 2017

@j-a-m-l
You probably cannot reproduce it because I fixed it :P Well anyway it happened when the dev tools were open and the windows was maybe like 1000px wide.

I moved the button to the button row and I remebered that I can use md-icon and now it loooks imho waaay better when it's finished. Also removed the now "redudant" success message: Because a picture is worth a thousands words :trollface:

image

@j-a-m-l j-a-m-l merged commit dcacf8a into ArkEcosystem:master Dec 30, 2017

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@j-a-m-l

This comment has been minimized.

Show comment
Hide comment
@j-a-m-l

j-a-m-l Dec 30, 2017

Member

+10 instead of + 5: instead of just fixing the behaviour you've improved it a lot and added tests

Member

j-a-m-l commented Dec 30, 2017

+10 instead of + 5: instead of just fixing the behaviour you've improved it a lot and added tests

@Nasicus Nasicus deleted the Nasicus:feat/account-export-all-transactions branch Dec 30, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment