Skip to content
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

Rewrites the electron app in vue.js #371

Merged
merged 113 commits into from Dec 14, 2019
Merged

Rewrites the electron app in vue.js #371

merged 113 commits into from Dec 14, 2019

Conversation

@kelsos
Copy link
Collaborator

kelsos commented Jun 12, 2019

Opening the PR to keep track of the work done on the vue - rewrite of the app.

Remaining tasks for the PR

  • Add first login dialog
  • Add Update message
  • Add premium dialog
    - [ ] Fix the e2e tests(after merge)
  • Fix packaging
    • Add packaging targets
    • Upgrade package scripts
    • Modify CI deployment
  • Add context actions to OTC Trades
  • Check if OTC is up to date
  • Rebase on master
  • Rewrite cointracking import functionality
  • ETH Account table should be expandable (show the account tokens)
  • Messages (Upgrade dialog) should not appear on errors

Additional tasks

After our last talk and based on your feedback I am adding some extra points
of things needed to finalize this PR:

  • Include package.sh changes from #552
  • Rebase on master. Implement check for analytics
  • Evaluate conditional expand on ETH Accounts
  • Find a better way to emphasize already setup account.
  • Fix currency change after rebase
  • Show exchange balances on the dashboard
  • Fix OTC edit/delete functionality
  • Greyscale Exchange icons
  • Fix premium support
  • Check and make sure Tax Reporting works properly and shows valid progress

Extra:

  • Rewrite Statistics

Closes #354

@codecov-io

This comment has been minimized.

Copy link

codecov-io commented Aug 7, 2019

Codecov Report

❗️ No coverage uploaded for pull request base (develop@7858aac). Click here to learn what that means.
The diff coverage is 0%.

Impacted file tree graph

@@            Coverage Diff             @@
##             develop     #371   +/-   ##
==========================================
  Coverage           ?   77.63%           
==========================================
  Files              ?       53           
  Lines              ?     5143           
  Branches           ?      751           
==========================================
  Hits               ?     3993           
  Misses             ?      948           
  Partials           ?      202
Impacted Files Coverage Δ
rotkehlchen/premium/premium.py 61.86% <0%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 7858aac...a64c52a. Read the comment docs.

@LefterisJP LefterisJP force-pushed the rotki:master branch from c55a912 to 6f9d300 Oct 22, 2019
Copy link
Contributor

LefterisJP left a comment

Hey @kelsos

First of all huge thanks for putting so much work on this. You rock!

So I am doing an initial review of the functionality here by testing it with live data of a heavy user. I am not going to comment on things that work fine.

What I see that definitely needs fixing:

  1. The profit currency on the top right was not read from the user settings. It's showing USD while the user I tried has it set to EUR.
  2. At start in the console tools I got this:

internal/process/warning.js:18 (node:47933) [DEP0005] DeprecationWarning: Buffer() is deprecated due to security and usability issues. Please use the Buffer.alloc(), Buffer.allocUnsafe(), or Buffer.from() methods instead.

  1. The All Balances table in the dashboard did not get updated as each async query completed. It had the only synchronous call as its initial polulated data (the fiat balances) and only once the blockchain balance query completed which for this user was super slow (6+mins) only then did all balances update.

  2. After looking at it further I realized that the balances of the exchanges are not included in the dashboard of the all balances. This needs to be fixed and restored to the previous functionality.

  3. OTC Trades table: There is no way to (i) edit (ii) remove (iii) show details of a trade in the table anymore like it used to be in the past. This functionality needs to be restored.
    A sidenote here. This will probably be renamed to external trades ... and the API (coming with #526) will be the same for querying all types of trades and will just filter the location (external in this case).

  4. All the exchange icons on the dashboard have colour. In the previous version they all had a css grey gradient. The reason was that different colors of all logos start being a bit too much.

  5. Trying to manually set the currency by editing the dropdown menu on the top right does not work.

  6. User settings: The user has a premium API key and going to the user settings does not seem to have picked this up.

2019-10-29-223603_965x276_scrot

  1. User Settings: The user has API keys for all exchanges. Somehow Bitmex is missing from the icons. Also the icons are not nicely arranged in a grid like before.
    2019-10-29-223648_988x468_scrot

  2. User Settings: The Fiat balances has a NaN for the usd value equivalent of the fiat amount.

2019-10-29-223802_949x435_scrot

  1. User Settings: Same as above for blockchain balances per asset
    2019-10-29-224057_502x644_scrot

  2. User Settings: Same as above for bitcoin balances per account
    2019-10-29-224117_536x208_scrot

  3. User Settings: Same as above for ETH balances per account
    2019-10-29-224112_518x130_scrot

  4. In the tax report: The progress bar seems to be off. Perhaps the periodic data are not read correctly?
    2019-10-29-225537_662x664_scrot

  5. In the tax report: After generation is finished the overview has only NaN:
    2019-10-29-230241_921x555_scrot

  6. In the tax report: Something is off. The resulting activities are only 36 .. while it should have been in the thousands..
    2019-10-29-230258_429x60_scrot

  7. Premium statistics does not work/premium subscription is not detected when going to check out the statistics.

@kelsos

This comment has been minimized.

Copy link
Collaborator Author

kelsos commented Oct 30, 2019

So I have an idea why the currency set/NAN happens if you check the header it says value when it should say EUR value and since I am using the current currency for the calculation thus the NaN.

The most curious thing is why it works locally. I remember seeing a change in the setCurrency method when I rebased on top of master yesterday so that should not be a hard fix.

OTC trades I am already aware it broke when I upgraded to the latest Vuetify, I already have it as a todo.

About the grid for the exchange icons should is the problem only with cointracking or is it something different?

On the progress bar, I had similar negative progress issues with the calculations on the retail, I have to re-evaluate.

on 3 the blockchain balances update together right? so the other missing balances are practically the exchange balances that are currently not displayed.

I will have to set it up and go again through the premium flow.

Statistics won't work at the moment due to the fact that they depend on the implementation details of the live version. I think I mentioned it before. But the last part of the rewrite after everything works is to also update the statistics.

Thanks for the feedback, I will finish the tests and then start fixing the current issues.

@LefterisJP

This comment has been minimized.

Copy link
Contributor

LefterisJP commented Oct 30, 2019

About the grid for the exchange icons should is the problem only with cointracking or is it something different?

Cointracking? Cointracking is not an exchange. No this is only that in the previous version it was a grid. So three exchanges first row, three exchanges last row.

On the progress bar, I had similar negative progress issues with the calculations on the retail, I have to re-evaluate.

There used to be but I fixed it with a PR. So it either still is or somehow with the rewrite it relapsed.

on 3 the blockchain balances update together right? so the other missing balances are practically the exchange balances that are currently not displayed.

Yes. But they have been queried since for example I can see them if I click on any exchange's icons.

@kelsos

This comment has been minimized.

Copy link
Collaborator Author

kelsos commented Oct 30, 2019

@LefterisJP yes I meant coinbase, my mistake I was merging the coin tracking info import yesterday, thus the mistake

I will check master to be sure. Maybe when I implemented the progress the fix was not merged yet.

Yes, the problem is that I had no clue that exchange data were displayed there, so 3-4 is the same issue I just don't show them at all. It should be easy to modify the getter to include them

@LefterisJP

This comment has been minimized.

Copy link
Contributor

LefterisJP commented Nov 9, 2019

So I had another look as requested and will write out what I found.

Need to do pip install -e.

For some reason the electron app does not discover the python process without pip install -e .. I do not think this was the case before but I would have to check.

Multiple balances of same asset in "All balances"

For some reason in the "all balances" table in the dashboard there are multiple entries for different assets. Perhaps since it does not sum up the locations (e.g. exchanges, blockchain, banks)?

2019-11-09-114829_966x229_scrot
2019-11-09-114944_793x177_scrot

Missing Grey out/Green out of exchange premium key if it's already registered.

In the current version if an exchange is already registered (also if premium is working) the respective api key/secret fields are green/greyed out so that the user knows he can't input a new key unless he presses the remove button.

2019-11-09-115041_956x476_scrot

Missing a way to provide tokens location per ETH account

2019-11-09-115755_897x187_scrot

In the original UI you could see how many/which tokens each account holds. It is indeed subtoptimal since for each token there was a new column in that table and as such became too big and needed scrolling. Perhaps as you suggested in our chat add it as hidden info per account that can be shown? You could have another column Number of tokens owned by account or something like that. The name needs to be shorter I guess. And there show a number of how many tokens the account owns. So if the account has GNO, RDN and OMG the number will be 3. And then when you press that number show you details about these tokens and expand the entry.

Did not pick up the main currency from settings

The main currency of the account should have been EUR when starting but the UI showed USD when it started. So for some reason it did not pick up the user's saved currency from the settings.

UI element has no padding

This is rather minor but the UI element of the start date does not seem to have enough distance from the "Main currency selection"

2019-11-09-120918_931x155_scrot

Editing a date should start from date that is being edited

I am trying to edit a date for a trade and wanted to make it 1 day later. But when clicking the UI element it starts from the current date. It should start from the date that is being edited.

2019-11-09-121124_620x463_scrot

Editing/deleting external trades does not work

2019-11-09-121326_482x183_scrot
2019-11-09-121354_471x181_scrot

Perhaps something wrong with a rebase? There were some changes there.

Data import margin/padding

Super minor ... but it looks ugly at the moment. There is no padding/margin to separate from the left and top menus.

2019-11-09-121542_1177x524_scrot

Tax report warning

The new progress bar looks much better. I think it would be nice to have a message there too warning the user that this will probably take a lot of time if he has done many trades and/or if the period is long. One thing to keep in mind here is that the report will go through all trades/actions no matter when the user set the start period since it needs to know about all actions that happened.

2019-11-09-121750_1094x497_scrot

Missing is_virtual marker from trades report

At some point there is some trades of selling BCH for BTC. That creates also a virtual trade of buying the equivalent BTC amount with EUR but there is nothing in the virtual column.

2019-11-09-123030_1134x256_scrot

@LefterisJP

This comment has been minimized.

Copy link
Contributor

LefterisJP commented Nov 12, 2019

Looking at the new stuff I guess:

The ui of the api keys still does not really show that it's not inputtable. I indeed can't click on an exchange that is already registered but it's not really visible that it's greyed out. Something a bit more obvious would be nice. Does not need to be as much highlighted as it is in the current version.

2019-11-12-235751_894x215_scrot

I like the way the tokens show for the accounts now, but perhaps it should be a bit more obvious that they can be expanded. Not sure how. We should think on it.

One note though. The accounts that have no tokens should not be expandable. The arrow should be missing for them.

I also saw this warning in the dev tools:


vue.runtime.esm.js:619 [Vue warn]: Property or method "floatingPrecision" is not defined on the instance but referenced during render. Make sure that this property is reactive, either in the data option, or for class-based components, by initializing the property. See: https://vuejs.org/v2/guide/reactivity.html#Declaring-Reactive-Properties.

found in

---> <AccountAssetBalances> at src/components/settings/AccountAssetBalances.vue
       <VSimpleTable>
         <VData>
           <VDataTable>
             <AccountBalances> at src/components/settings/AccountBalances.vue
               <VCard>
                 <BlockchainBalances> at src/components/settings/BlockchainBalances.vue
                   <User> at src/views/settings/User.vue
                     <VContent>
                       <VApp>
                         <App> at src/App.vue
                           <Root>

The rest of the concerns of the last comment seem to be addressed.

@kelsos

This comment has been minimized.

Copy link
Collaborator Author

kelsos commented Nov 18, 2019

@LefterisJP Could you have a look at the packaging and how it works for you?
Build on macOS, Windows and Linux and see how the builds work.

To be safe rename/delete the node_modules folder to make sure that the zeromq module is not picked from the absolute path.

The build targets are dmg for macOS, a portable executable for Windows and a tar/AppImage for linux.

@LefterisJP LefterisJP force-pushed the rotki:master branch 3 times, most recently from 3b27dc4 to 74a9d11 Dec 1, 2019
@kelsos kelsos changed the title [WIP] Rewrites the electron app in vue.js Rewrites the electron app in vue.js Dec 8, 2019
@kelsos kelsos marked this pull request as ready for review Dec 8, 2019
@kelsos

This comment has been minimized.

Copy link
Collaborator Author

kelsos commented Dec 8, 2019

@LefterisJP everything should be ready for testing and reviewing.
The only thing left to be done is to modify the deployment to use the new files.

How do you want to proceed from here?

@LefterisJP

This comment has been minimized.

Copy link
Contributor

LefterisJP commented Dec 9, 2019

So testing along with the premium stuff like statistics is ready? If so I need to rebuild the ranch and test it.

@LefterisJP

This comment has been minimized.

Copy link
Contributor

LefterisJP commented Dec 10, 2019

So from the premium statistics the only thing that I see is the order is wrong.

It should be:

  1. Total netvalue
  2. Netvalue by asset
  3. Distribution by asset
  4. Distribution by location.

Also the start date dropdown is kind of annoying as it insists to be provided time. If I start typing I want the calendar view to not hide the typing. And If I type "01/01/2016" and press Enter it should work and not tell me I have to enter the hour too.

@kelsos

This comment has been minimized.

Copy link
Collaborator Author

kelsos commented Dec 10, 2019

@LefterisJP Can you open that as an issue to the repo of the statistics? I will try to fix them tonight.

@LefterisJP

This comment has been minimized.

Copy link
Contributor

LefterisJP commented Dec 11, 2019

After merging the latest premium components and continuing testing I noticed this warning in the console. It is emitted directly at the start of the app:

(node:22992) [DEP0005] DeprecationWarning: Buffer() is deprecated due to security and usability issues. Please use the Buffer.alloc(), Buffer.allocUnsafe(), or Buffer.from() methods instead.

Also still as before letting the app query all blockchain balances seems to over-write the balances of an asset found in the exchanges. For example the test user had X BTC in all exchanges and Y in the bitcoin blockchain. When the app firs quries exchanges the BTC balance shows as X and then later once the blockchain balance query came in he had Y instead of X+Y

Same for other assets, for example kraken EUR balances being overwritten by banks EUR balances, instead of getting added.

Finally for the premium statistics I would like some default values for the charts as they were before.

  1. The netvalue graph should have the entire range of data as its default value.
  2. The Asset amount and value over time graph does not need a default value but can be as (1) for the first asset on the list.
@LefterisJP LefterisJP changed the base branch from master to develop Dec 12, 2019
@LefterisJP

This comment has been minimized.

Copy link
Contributor

LefterisJP commented Dec 12, 2019

Hey @kelsos as discussed the default branch for development of big features has been changed to develop. I updated the PR to reflect this. Make sure you are aware of this when you do anything with it.

@LefterisJP

This comment has been minimized.

Copy link
Contributor

LefterisJP commented Dec 13, 2019

Hey @kelsos so I think that it's mostly working and we will need to iron out any other bugs we find. But at this point this should be merged to develop so that the other big change can also be rebased on top of it. Do you agree?

Can you add a changelog entry also? Since you are the author of the change choose the words, but try to focus on modern/sleek UI which is also easily customizable.

@kelsos

This comment has been minimized.

Copy link
Collaborator Author

kelsos commented Dec 13, 2019

@LefterisJP sure I will add it and we can merge on develop then

kelsos added 7 commits May 8, 2019
kelsos and others added 27 commits Nov 10, 2019
Copy link
Contributor

LefterisJP left a comment

LGTM. As discussed packaging should be worked on a separate PR.

@LefterisJP LefterisJP merged commit 6339a22 into rotki:develop Dec 14, 2019
3 checks passed
3 checks passed
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
license/cla Contributor License Agreement is signed.
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.