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

[DDW-492] Add initial loading state to UTXO screen #2265

Merged
merged 12 commits into from Dec 14, 2020

Conversation

DominikGuzei
Copy link
Member

@DominikGuzei DominikGuzei commented Dec 4, 2020

This PR adds an initial loading state for the UTXO page:

Screenshots

Screenshot 2020-12-21 at 12 56 45

Screenshot 2020-12-21 at 12 56 52


Testing Checklist

  • Slack QA thread
  • The UTXO page should show a loading spinner and not an "empty wallet" message for wallets that have UTXO but are still loading.

Test cases

Scenario 1: Wallet with UTXO, select first time
Given I am on Daedalus
And Daedalus is fully synced
And I have a fully synced wallet with UTXO
When I select the wallet for the first time
And navigate to ‘Wallet UTXO distribution’
Then I will briefly see the text ‘Wallet UTXO distribution’
And I will briefly see the spinner
And the spinner disappears
And I see the text “The wallet contains xxx ADA on x UTXOs ….”
And I see the UTXO graph distribution

Scenario 2: Wallet with no UTXO
Given I am on Daedalus
And Daedalus is fully synced
And I have a fully synced wallet with 0 ADA
When I select the wallet
And navigate to ‘Wallet UTXO distribution’
Then I will see the text ‘Wallet UTXO distribution’
And I will briefly see the spinner
And the spinner disappears
And underneath I will see the text “This wallet is empty….”

Scenario 3: Wallet with UTXO, select the second time
Given I am on Daedalus
And Daedalus is fully synced
And I have a fully synced wallet with UTXO
And I have already checked this wallet for UTXO distribution
When I select the wallet for the second time
And navigate to ‘Wallet UTXO distribution’
Then I will see the text ‘Wallet UTXO distribution’
And I will briefly see the spinner
And the spinner disappears
And I see the text “The wallet contains xxx ADA on x UTXOs ….”
And I see the UTXO graph distribution

Scenario 4: Select another wallet with UTXO, after checking UTXO of the first wallet
Given I am on Daedalus
And Daedalus is fully synced
And I have 2 fully synced wallets with UTXO
And both wallets have never been selected to check UTXO distribution
And I have checked the UTXO distribution of the first wallet
When I select the second wallet
And navigate to ‘Wallet UTXO distribution’
Then I will briefly see the text ‘Wallet UTXO distribution’
And I will briefly see the spinner
And the spinner disappears
And I see the text “The wallet contains xxx ADA on x UTXOs ….”
And I see the UTXO graph distribution

Testing Summary

For build 15559

Test cases Results Note
Scenario 1 Pass Recording for Scenario 1
Scenario 2 Pass Recording for Scenario 2
Scenario 3 Pass Recording for Scenario 3
Scenario 4 Pass Recording for Scenario 4

Review Checklist

Basics

  • PR has been assigned and has appropriate labels (feature/bug/chore, release-x.x.x)
  • PR is updated to the most recent version of the target branch (and there are no conflicts)
  • PR has a good description that summarizes all changes
  • PR has default-sized Daedalus window screenshots or animated GIFs of important UI changes:
    • In English
    • In Japanese
  • CHANGELOG entry has been added to the top of the appropriate section (Features, Fixes, Chores) and is linked to the correct PR on GitHub
  • Automated tests: All acceptance and unit tests are passing (yarn test)
  • Manual tests (minimum tests should cover newly added feature/fix): App works correctly in development build (yarn dev)
  • Manual tests (minimum tests should cover newly added feature/fix): App works correctly in production build (yarn package / CI builds)
  • There are no flow errors or warnings (yarn flow:test)
  • There are no lint errors or warnings (yarn lint)
  • There are no prettier errors or warnings (yarn prettier:check)
  • There are no missing translations (running yarn manage:translations produces no changes)
  • Text changes are proofread and approved (Jane Wild / Amy Reeve)
  • Japanese text changes are proofread and approved (Junko Oda)
  • UI changes look good in all themes (Alexander Rukin)
  • Storybook works and no stories are broken (yarn storybook)
  • In case of dependency changes yarn.lock file is updated

Code Quality

  • Important parts of the code are properly commented and documented
  • Code is properly typed with flow
  • React components are split-up enough to avoid unnecessary re-renderings
  • Any code that only works in main process is neatly separated from components

Testing

  • New feature/change is covered by acceptance tests
  • New feature/change is manually tested and approved by QA team
  • All existing acceptance tests are still up-to-date
  • New feature/change is covered by Daedalus Testing scenario
  • All existing Daedalus Testing scenarios are still up-to-date

After Review

  • Merge the PR
  • Delete the source branch
  • Move the ticket to done column on the YouTrack board
  • Update Slack QA thread by marking it with a green checkmark

@DominikGuzei DominikGuzei self-assigned this Dec 4, 2020
@DominikGuzei DominikGuzei requested a review from a team December 4, 2020 18:57
@miorsufianiohk
Copy link

miorsufianiohk commented Dec 7, 2020

Hi @DominikGuzei Here is the finding for build 15520.

New issue
1 - I have taken a video of a fully synced wallet and checking the UTXO distribution for the first time. I could still see "The wallet is empty.." message and no spinner. Please refer to video at 00:06 second. Is this expected? I am under the impression that when the wallet has UTXO, the message "The wallet is empty..." will not be displayed at all. Appreciate your confirmation please @DominikGuzei See video

Non issue on this PR
1 - No spinner was observed during loading stage on UTXO page. See video (Dominik to create a new card to fix this)

@DominikGuzei DominikGuzei removed the WIP label Dec 7, 2020
@DominikGuzei
Copy link
Member Author

@mioriohk thanks for the video - you were right, the logic was missing one check 👏
I pushed the fix and it's ready for a final review 🎉

@nikolaglumac nikolaglumac changed the title [DDW-492] Add initial loading state to utxo page [DDW-492] Add initial loading state to UTXO page Dec 8, 2020
@nikolaglumac nikolaglumac changed the title [DDW-492] Add initial loading state to UTXO page [DDW-492] Add initial loading state to UTXO screen Dec 8, 2020
@miorsufianiohk
Copy link

miorsufianiohk commented Dec 8, 2020

Hi @DominikGuzei. Here is the report for build 15543.
Note: I have created Test scenarios under 'Test Cases' in this section

Test cases Results Note
Scenario 1 Fail I am still able to see the message "The wallet is empty..". See video build 15543 at 00:05s. However the behaviour was correct on build 15533 See video build 15533
Scenario 2 Pass

@@ -39,6 +39,9 @@ export default class WalletSettingsPage extends Component<Props> {

return (
<WalletUtxo
isLoadingInitialUtxoData={
walletSettings.getWalletUtxosRequest.isExecutingFirstTime
Copy link
Contributor

Choose a reason for hiding this comment

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

@DominikGuzei shouldn't we make sure we check if this request has been executed for the first time for the active wallet? We fetch UTXO distribution data for each wallet and I am afraid we will have this working only for the first one (or if the user has only one wallet)...
If I remember correctly, it should be possible to pass on walletId to check if the request has been ever executed for the given wallet?

Copy link
Member Author

Choose a reason for hiding this comment

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

Great catch 👍

Copy link
Member Author

Choose a reason for hiding this comment

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

@nikolaglumac my latest commit fixes this by resetting the utxo request when leaving the utxo page. This also means that it will show the spinner also anytime you go back to the utxo page within the same wallet. But it's the easiest solution and I think ok since the spinner is rarely showing up anyway - what do you think?

@DominikGuzei DominikGuzei removed the WIP label Dec 9, 2020
@DominikGuzei
Copy link
Member Author

@mioriohk ready for testing again 👍

@DominikGuzei
Copy link
Member Author

@mioriohk sorry, I could reproduce the issue from your video and also fixed these edge cases (these have been caused because the loading spinner was only shown when the utxo request has already been made - now it's also shown before the request has been made).

Ready for final test 🎉

@miorsufianiohk
Copy link

Hi @DominikGuzei. Here is the report for build 15559.
All looks good to me

Test cases Results Note
Scenario 1 Pass Recording for Scenario 1
Scenario 2 Pass Recording for Scenario 2
Scenario 3 Pass Recording for Scenario 3
Scenario 4 Pass Recording for Scenario 4

@nikolaglumac
Copy link
Contributor

@mioriohk would you mind approving this PR if it is all good from your side? 🙏

Copy link

@miorsufianiohk miorsufianiohk left a comment

Choose a reason for hiding this comment

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

Looks good to me, Well done @DominikGuzei 👍

Copy link
Contributor

@nikolaglumac nikolaglumac left a comment

Choose a reason for hiding this comment

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

Great work @DominikGuzei 👌

@nikolaglumac nikolaglumac merged commit 824f9a1 into develop Dec 14, 2020
@iohk-bors iohk-bors bot deleted the fix/ddw-492-wallet-utxo-screen-delay branch December 14, 2020 18:33
@nikolaglumac nikolaglumac added release-3.1.0 Daedalus Mainnet and removed ⏳release-vNext labels Jan 4, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug release-3.1.0 Daedalus Mainnet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants