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

Hide dangerous operations behind configuration option #1025

Draft
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

csillag
Copy link
Contributor

@csillag csillag commented Sep 24, 2022

This PR adds the following things:

  1. Implement a settings dialog, accessible from the sidebar, currently with a single setting: the "danger mode". (For background, see here)
    dialog

  2. Updates the behavior of the modal dialogs, when the isDangerous flag is specified. The new behavior is as follows:

  • If an action is indicated as dangerous, but "danger mode" is off (the default), then the operation will be refused, and the user is referred to the settings.
  • If "danger mode" is enabled, then it's possible to execute the action, but there is still a strong warning message, and a mandatory waiting period.

modal

@github-actions
Copy link

github-actions bot commented Sep 24, 2022

🦙 MegaLinter status: ✅ SUCCESS

Descriptor Linter Files Fixed Errors Elapsed time
✅ EDITORCONFIG editorconfig-checker 20 0 0.04s
✅ JSON eslint-plugin-jsonc 1 0 0 0.87s
✅ JSON jsonlint 1 0 0.2s
✅ JSON prettier 1 0 0 0.51s
✅ JSON v8r 1 0 1.75s
✅ REPOSITORY checkov yes no 11.46s
✅ REPOSITORY git_diff yes no 0.0s
✅ TSX eslint 8 0 0 5.8s
✅ TYPESCRIPT eslint 9 0 0 4.51s

See errors details in artifact MegaLinter reports on CI Job page
Set VALIDATE_ALL_CODEBASE: true in mega-linter.yml to validate all sources, not only the diff

MegaLinter is graciously provided by OX Security

@kostko
Copy link
Member

kostko commented Sep 24, 2022

@codecov
Copy link

codecov bot commented Sep 24, 2022

Codecov Report

Merging #1025 (8f4da1e) into master (0e7cab6) will decrease coverage by 1.54%.
The diff coverage is 52.08%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1025      +/-   ##
==========================================
- Coverage   88.75%   87.20%   -1.55%     
==========================================
  Files         102      108       +6     
  Lines        1778     1852      +74     
  Branches      411      436      +25     
==========================================
+ Hits         1578     1615      +37     
- Misses        200      237      +37     
Flag Coverage Δ
cypress 59.55% <40.62%> (-1.03%) ⬇️
jest 77.81% <52.08%> (-1.05%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
src/app/components/Sidebar/index.tsx 88.75% <ø> (ø)
src/app/index.tsx 100.00% <ø> (ø)
src/store/reducers.ts 100.00% <ø> (ø)
src/app/components/SettingsDialog/index.tsx 12.50% <12.50%> (ø)
src/app/components/Modal/slice/index.ts 30.00% <30.00%> (ø)
src/app/components/SettingsDialog/slice/index.ts 42.85% <42.85%> (ø)
src/app/components/Modal/index.tsx 58.13% <57.14%> (-21.87%) ⬇️
src/app/components/SettingsButton/index.tsx 72.72% <72.72%> (ø)
src/app/components/AddEscrowForm/index.tsx 92.30% <100.00%> (ø)
src/app/components/Modal/slice/selectors.ts 100.00% <100.00%> (ø)
... and 5 more

@csillag
Copy link
Contributor Author

csillag commented Sep 24, 2022

Obviously, some tests need to be added, but first I wanted to get some feedback on the basics.

Copy link
Member

@lukaw3d lukaw3d left a comment

Choose a reason for hiding this comment

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

I love the UX flow

  • there's instructions on why confirm button is missing
  • opening settings as modal doesn't destroy the unfinished transaction
  • there's wait time on the button

src/app/components/SettingsDialog/index.tsx Outdated Show resolved Hide resolved
src/app/components/Modal/index.tsx Outdated Show resolved Hide resolved
src/app/components/Modal/index.tsx Outdated Show resolved Hide resolved
src/app/components/Modal/index.tsx Outdated Show resolved Hide resolved
@csillag
Copy link
Contributor Author

csillag commented Sep 27, 2022

@lukaw3d I have addressed all the feedback. Thanks.

@tjanez tjanez marked this pull request as draft September 27, 2022 14:06
@csillag csillag added the blocked Waiting for dependency to resolve label Sep 28, 2022
@csillag csillag force-pushed the csillag/danger-mode branch 2 times, most recently from 109cd65 to d1f488e Compare October 19, 2022 22:21
@csillag
Copy link
Contributor Author

csillag commented Oct 19, 2022

We have discussed the design with @donouwens. His main feedback was that it would be nice if the settings dialog could be opened directly from within the dangerous modal dialogs. However, @lukaw3d has explained that Grommet has an obscure issue with pop-ups being opened from pop-ups.

So I have created an implementation which supports opening the settings dialog, but in a way that first it closes the existing modal, then it brings up the settings dialog, and when we close that, it brings up the original modal dialog again.

I'm not fully satisfied with the implementation, because I feel that the logic and APIs for dealing with the modal are now so complex that they should be implemented in a redux store, via proper actions, instead of locally managed react hooks state, passed along in a react context.. I will refactor that after we have confirmed that this behavior is what we want from the UI/UX POV.

@csillag csillag removed the blocked Waiting for dependency to resolve label Oct 19, 2022
@lukaw3d
Copy link
Member

lukaw3d commented Oct 19, 2022

popup in popup issue reference: #863

csillag and others added 4 commits October 20, 2022 13:28
Also, in modals, improve handling of danger, and add delays.
 - The state is handled in Redux
 - No more ModalProvider
 - pop-up in pop-up is now supported (recursively)
@csillag
Copy link
Contributor Author

csillag commented Oct 20, 2022

I'm not fully satisfied with the implementation, because I feel that the logic and APIs for dealing with the modal are now so complex that they should be implemented in a redux store, via proper actions, instead of locally managed react hooks state, passed along in a react context..

I have refactored Modal so that

  • The state is stored in Redux
  • It now be opened recursively. (When opening a new one, we always close the previous one and stash it, to be re-opened later.)

I like it better now, however there is one drawback: the handleConfirm functions (which are part of the modal definition) are now also stored in the Redux store. Redux doesn't like non-serializable data in the store, so I had to add specific instructions to make it stop complaining.

@lukaw3d
Copy link
Member

lukaw3d commented Oct 24, 2022

one drawback: the handleConfirm functions (which are part of the modal definition) are now also stored in the Redux store. Redux doesn't like non-serializable data in the store, so I had to add specific instructions to make it stop complaining.

This will probably cause problems with redux-saga, webext-redux, redux-state-sync, or future libraries we may want. I don't think this simple settings shortcut is worth breaking redux's best practice

@lukaw3d
Copy link
Member

lukaw3d commented Oct 24, 2022

To reduce complexity I'd rather have any of:

  • remove settings shortcut
  • or ignore accessibility bug for this rare feature
  • or fix accessibility bug upstream

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants