[DDW-881] Enhance logic for updating menus #2838

szymonmaslowski · 2022-01-25T02:32:13Z

This PR:

fixes behaviour of the wallet settings item of the menu:
- enables the option if the user is on the one of wallet pages
- disables (greyed with deactivated hotkey) the option if the user:
  - is on the one of wallet pages but has some dialog opened
  - is already on the wallet settings page
- hides (and deactivates the hotkey) the option if the user:
  - is not on the one of the wallet pages
integrates AssetSettingDialog with the UiDIalogs mobx store
unifies IPC communication regarding menu rebuild to a single channel
removes implicit IPC channel.send() calls from react components and stores methods
creates new feature responsible for watching for all data changes requiring menu rebuild and triggering it

⚠️ This PR touches also the implementation of menu changes that happen after accepting terms of use (enabling "About Daedalus", "Redeem ITN rewards", "Settings" and "Daedalus Diagnostics") and changing language. For those cases the behaviour was not changed, so everything should work as previously. For those cases changes are all about the refactoring.

Todos

Screenshots

Testing Checklist

Slack QA thread

Test scenarios

Scenario 1 - Validate the menu is updated after switching languages

Given that Daedalus is running
When I access to the "Settings" screen
And switch the current language
Then I can see that the items on the menu are also displayed in the selected language

Scenario 2 - Validate the wallet settings option while wallet is selected

Given that Daedalus has at least one wallet
And I am on any tab from a wallet (except settings)
And no dialog is open
When I check the menu "Daedalus"
Then I can see the option "Wallet settings" is visible and enabled
And upon clicking on it using the hotkeys, I'm redirected to the "Settings" tab if the wallet

Scenario 3 - Validate the wallet settings option while wallet is selected and a dialog is open

Given that Daedalus has at least one wallet
And I am on any tab from a wallet (except settings)
And any dialog or overlay is open (Send ADA, Wallet Address, Add tokens, Tokens settings, Change password, Verify recovery phrase, Wallet public/multi-signature key, Delete wallet, About screen,Diagnostic screen, Redeem ITN rewards)
When I check the menu "Daedalus"
Then I can see the option "Wallet settings" is visible and disabled
And upon clicking on it or using the hotkeys, nothing happens

Scenario 4 - Validate the wallet settings option while "wallet settings" is visible

Given that Daedalus has at least one wallet
And I am on the settings tab from a wallet 
When I check the menu "Daedalus"
Then I can see the option "Wallet settings" is visible and disabled
And upon clicking on it or using the hotkeys, nothing happens

Scenario 5 - Validate the wallet settings option when there are no wallets

Given that Daedalus has no wallet
And I am on the "Add wallet" screen
When I check the menu "Daedalus"
Then I can see the option "Wallet settings" is not visible

Scenario 6 - Validate the wallet settings option when no wallet is selected

Given that Daedalus has at least one wallet
And I am not on any tab from a wallet (not selected)
When I check the menu "Daedalus"
Then I can see the option "Wallet settings" is not visible

Scenario 7 - Validate options before initial setup

Given that I have just installed Daedalus
When I run it for the first time
And validate the menu item before the initial setup
Then I can see the following options are disabled:

- Daedalus - About Daedalus
- Daedalus - Redeem ITN rewards
- Daedalus - Settings
- Help - Daedalus diagnostics

Review Checklist

Basics

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

Update Slack QA thread by marking it with a green checkmark

…nu on changes from various stores

…dedicated feature

source/renderer/app/features/menu-updater/makeReactionCallback.js

alexander-rukin · 2022-01-25T14:38:57Z

this dialog should have setting option disabledd too

…en on already on that page

szymonmaslowski · 2022-01-27T17:21:05Z

this dialog should have setting option disabledd too

This case is covered now.

gabriela-ponce · 2022-01-27T19:27:36Z

@szymonmaslowski Here are a couple of scenarios where the "wallet settings" option is still visible and enabled:

Transactions tab - Export CSV dialog - Check this screenshot.
Send tab - Add tokens dialog- Check this screenshot.

szymonmaslowski · 2022-01-28T11:51:55Z

source/renderer/app/api/utils/localStorage.js

-  getAssetSettingsDialogWasOpened = (): Promise<boolean> =>
-    LocalStorageApi.get(keys.ASSET_SETTINGS_DIALOG_WAS_OPENED, false);
-
-  setAssetSettingsDialogWasOpened = (): Promise<void> =>
-    LocalStorageApi.set(keys.ASSET_SETTINGS_DIALOG_WAS_OPENED, true);
-
-  unsetAssetSettingsDialogWasOpened = (): Promise<void> =>
-    LocalStorageApi.unset(keys.ASSET_SETTINGS_DIALOG_WAS_OPENED);
-


I removed those three methods since they look like an overhead. We should not use persistent storage to store the information about whether the dialog is open since it is just a runtime information. Except the usage in the AssetsStore which was just checking whether the dialog was open I didn't found any other usages so they were safe to remove.

szymonmaslowski · 2022-01-28T15:49:03Z

@szymonmaslowski Here are a couple of scenarios where the "wallet settings" option is still visible and enabled:

Transactions tab - Export CSV dialog - Check this screenshot.

Send tab - Add tokens dialog- Check this screenshot.

Thanks @gabriela-ponce for rising those.
Regarding the Add tokens dialog - already working on a fix for it it.
Regarding the Export CSV I didn't consider it a dialog, but you are right. I have consulted with @alexander-rukin on that and we agreed it is worth to fix if it is relatively small amount of effort so I will check how is it implemented and if that will be easy to fix I will do that.

…store

…ostic dialog and all of Redeem Rewards dialogs

…tainer

…ove to WalletSendForm

…erge

…daedalus into feature/ddw-881

renanvalentin · 2022-02-02T12:42:52Z

source/renderer/app/containers/MenuUpdater/useMenuUpdater.spec.js

@@ -38,7 +40,15 @@ const makeRebuildApplicationMenu = ({
  send = () => {},
 } = {}): RebuildApplicationMenu => ({ send }: any);

-const defaultArgs: MakeReactionCallbackArgs = {
+const renderComponent = (args) => {
+  const Component = () => {


Another suggestion is that you could test the MenuUpdater to have a better integration test. Currently you are only testing the internal implementation.

function renderComponent(spy) { return <> <MenuUpdaterProvider appStore={{}} profileStore={{}} router={{}} stakingStore={{}} uiDialogsStore={{}}> <MenuUpdater onUpdate={({isNavigationEnabled, walletSettingsState}) => spy({isNavigationEnabled, walletSettingsState})} /> </MenuUpdaterProvider> }

The MenuUpdater component itself is not important as its role is only to provide required data using the mobx api. The sole logic is contained in the useMenuUpdater. Testing the MenuUpdater would require from me to involve mobx to testing which IMO is not worth for value it would give. I think we don't need to test things like connecting the component to mobx store

gabriela-ponce · 2022-02-02T19:35:22Z

@szymonmaslowski The scenarios were updated according to the new requirements you mentioned. And the latest fixes look good, great job 🚀

I found this issue on build 20711 (doesn't reproduce on develop).

Can't open the dialog "Native token settings". Check this video.

szymonmaslowski · 2022-02-03T21:36:29Z

@szymonmaslowski The scenarios were updated according to the new requirements you mentioned. And the latest fixes look good, great job 🚀

I found this issue on build 20711 (doesn't reproduce on develop).

Can't open the dialog "Native token settings". Check this video.

The change responsible for opening that dialog should have been missed during merging. It is reverted already and should be working fine now. Thanks @gabriela-ponce.

gabriela-ponce

Good job 🚀

renanvalentin

Great job! 🚀

…ion" This reverts commit 54ff345.

Co-authored-by: Michael Chappell <michael.chappell@iohk.io>

szymonmaslowski added 4 commits January 24, 2022 14:29

[DDW-881] Implement global Reaction responsible for rebuilding app me…

f5026be

…nu on changes from various stores

[DDW-881] Move the side effect of watching for manu to rebuld to the …

db14b17

…dedicated feature

[DDW-881] Fix lint and codestyle issues

5b7ef84

Merge remote-tracking branch 'origin/develop' into feature/ddw-881

b713dce

szymonmaslowski added bug feature ⏳release-vNext labels Jan 25, 2022

szymonmaslowski requested review from alexander-rukin and a team January 25, 2022 02:32

szymonmaslowski self-assigned this Jan 25, 2022

szymonmaslowski removed the feature label Jan 25, 2022

[DDW-881] Update changelog

1029883

szymonmaslowski commented Jan 25, 2022

View reviewed changes

source/renderer/app/features/menu-updater/makeReactionCallback.js Outdated Show resolved Hide resolved

szymonmaslowski changed the title ~~[DDW-881] A feature responsible for rebuilding app menu on changes from various stores~~ [DDW-881] Enhance logic for updating menus Jan 25, 2022

szymonmaslowski added the WIP label Jan 26, 2022

szymonmaslowski added 3 commits January 26, 2022 10:30

[DDW-881] Make wallet settings menu option be visible but disabled wh…

cc73406

…en on already on that page

[DDW-881] Integrate Asset Settings Dialog with the ui dialogs mobx store

1b7acc0

[DDW-881] Minor code improvements

62d86f3

szymonmaslowski removed the WIP label Jan 27, 2022

szymonmaslowski commented Jan 28, 2022

View reviewed changes

szymonmaslowski added the WIP label Jan 31, 2022

szymonmaslowski added 3 commits January 31, 2022 09:26

[DDW-881] Plug in the WalletTokenPicker dialog to the uiDialogs mobx …

c1a3b14

…store

[DDW-881] Disable wallet settings menu option for About dialog, Diagn…

20e6cff

…ostic dialog and all of Redeem Rewards dialogs

[DDW-881] Hide closing the Picket dialog functionality inside the con…

2b9d727

…tainer

szymonmaslowski force-pushed the feature/ddw-881 branch from 3a070aa to 2b9d727 Compare February 1, 2022 07:43

mchappell and others added 7 commits February 2, 2022 10:11

[DDW-881] lockfile fixed

1e0819e

[DDW-881] Use dialog related actions and store methods passed from ab…

ef5c709

…ove to WalletSendForm

Merge branch 'develop' into feature/ddw-881

b5dd487

[DDW-881] removed previously deleted files conflicting with develop m…

ef05075

…erge

[DDW-881] fixed lockfile

840e978

Merge branch 'feature/ddw-881' of https://github.com/input-output-hk/…

3cf0227

…daedalus into feature/ddw-881

[DDW-881] updated wallet send page

cf3bfd0

renanvalentin reviewed Feb 2, 2022

View reviewed changes

szymonmaslowski added 4 commits February 3, 2022 11:15

[DDW-881] Apply again reverted changes of opening AssetSettingsDialog

d8f0ad5

[DDW-881] Fix WalletSendForm tests

4f4950a

[DDW-881] Fix one of useMenuUpdater tests

77e90cd

[DDW-881] Fix ts issue of MenuUpdater

3a8ff36

szymonmaslowski requested a review from renanvalentin February 3, 2022 21:38

szymonmaslowski added 2 commits February 3, 2022 22:39

Merge remote-tracking branch 'origin/develop' into feature/ddw-881

649c731

[DDW-881] Reimplement WalletSettingsStateEnum to TS native enum

370d9d1

gabriela-ponce approved these changes Feb 4, 2022

View reviewed changes

renanvalentin approved these changes Feb 7, 2022

View reviewed changes

[DDW-881] Memoize part of the useMenuUpdater hook implementation

54ff345

szymonmaslowski force-pushed the feature/ddw-881 branch from 64014eb to 54ff345 Compare February 8, 2022 11:42

szymonmaslowski added 2 commits February 8, 2022 13:19

Revert "[DDW-881] Memoize part of the useMenuUpdater hook implementat…

a818d53

…ion" This reverts commit 54ff345.

Merge remote-tracking branch 'origin/develop' into feature/ddw-881

c7bafab

danielmain merged commit 8abd7b3 into develop Feb 8, 2022

danielmain deleted the feature/ddw-881 branch February 8, 2022 16:59

danielmain added release-4.9.0-FC1 Daedalus Flight and removed ⏳release-vNext labels Feb 10, 2022

renanvalentin pushed a commit that referenced this pull request Mar 7, 2022

[DDW-881] Enhance logic for updating menus (#2838)

260f0b5

Co-authored-by: Michael Chappell <michael.chappell@iohk.io>

renanvalentin pushed a commit that referenced this pull request Mar 7, 2022

[DDW-881] Enhance logic for updating menus (#2838)

35ef207

Co-authored-by: Michael Chappell <michael.chappell@iohk.io>

renanvalentin pushed a commit that referenced this pull request Mar 7, 2022

[DDW-881] Enhance logic for updating menus (#2838)

a055f62

Co-authored-by: Michael Chappell <michael.chappell@iohk.io>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[DDW-881] Enhance logic for updating menus #2838

[DDW-881] Enhance logic for updating menus #2838

szymonmaslowski commented Jan 25, 2022 •

edited by gabriela-ponce

alexander-rukin commented Jan 25, 2022

szymonmaslowski commented Jan 27, 2022

gabriela-ponce commented Jan 27, 2022

szymonmaslowski Jan 28, 2022

szymonmaslowski commented Jan 28, 2022

renanvalentin Feb 2, 2022

szymonmaslowski Feb 3, 2022

gabriela-ponce commented Feb 2, 2022

szymonmaslowski commented Feb 3, 2022

gabriela-ponce left a comment

renanvalentin left a comment

[DDW-881] Enhance logic for updating menus #2838

[DDW-881] Enhance logic for updating menus #2838

Conversation

szymonmaslowski commented Jan 25, 2022 • edited by gabriela-ponce

Todos

Screenshots

Testing Checklist

Test scenarios

Review Checklist

Basics

Code Quality

Testing

After Review

alexander-rukin commented Jan 25, 2022

szymonmaslowski commented Jan 27, 2022

gabriela-ponce commented Jan 27, 2022

szymonmaslowski Jan 28, 2022

Choose a reason for hiding this comment

szymonmaslowski commented Jan 28, 2022

renanvalentin Feb 2, 2022

Choose a reason for hiding this comment

szymonmaslowski Feb 3, 2022

Choose a reason for hiding this comment

gabriela-ponce commented Feb 2, 2022

szymonmaslowski commented Feb 3, 2022

gabriela-ponce left a comment

Choose a reason for hiding this comment

renanvalentin left a comment

Choose a reason for hiding this comment

szymonmaslowski commented Jan 25, 2022 •

edited by gabriela-ponce