feat(preferences): 2FA activation dialog with mocked state #605
Conversation
gorjan5sk
commented
Jul 22, 2021
gorjan5sk
commented
- Implemented two factor authentication activation process in preferences
- Uses hard-coded values for interface, can be completed successfully without providing secret key and auth code
gorjan5sk
force-pushed
the
feature/2fa-dialog-flow
branch
from
4583127 to
0d3bfe5
Compare
gorjan5sk
force-pushed
the
feature/2fa-dialog-flow
branch
from
0d3bfe5 to
18fb0d1
Compare
app/assets/javascripts/preferences/panes/two-factor-auth/AuthAppInfoPopup.tsx
Outdated
Show resolved
Hide resolved
app/assets/javascripts/preferences/panes/two-factor-auth/AuthAppInfoPopup.tsx
Outdated
Show resolved
Hide resolved
app/assets/javascripts/preferences/panes/two-factor-auth/TwoFactorAuthView.tsx
Outdated
Show resolved
Hide resolved
| </Text> | ||
| </div> | ||
| <Switch | ||
| checked={auth.enabled !== false} |
There was a problem hiding this comment.
Can auth.enabled be anything different from a boolean?
There was a problem hiding this comment.
Yes. And yes, it doesn't look good :) I'm in the process of changing that.
There was a problem hiding this comment.
It should be better now. Commit 1f75df8.
app/assets/javascripts/preferences/panes/two-factor-auth/TwoFactorAuthView.tsx
Outdated
Show resolved
Hide resolved
app/assets/javascripts/preferences/panes/two-factor-auth/TwoFactorAuthView.tsx
Outdated
Show resolved
Hide resolved
| @@ -0,0 +1,169 @@ | |||
| import { action, makeAutoObservable, observable, untracked } from 'mobx'; | |||
There was a problem hiding this comment.
These models are implemented using Mobx, any specific reason to keep them outside of the shared store?
There was a problem hiding this comment.
Yes, the single responsibility principle. I don't want to clutter the shared state with all possible states the Preferences and 2FA can be in.
Also, mobx observables don't need to be grouped in a single store. However, the reason it's not connected to it at all ATM is because the following PR is the one for integration with the actual activation.
There was a problem hiding this comment.
You’d just add an extra domain store to the root store, so it would still be isolated enough that it wouldn’t be cluttered. The advantage of having all stores grouped in a root one is they can share references and interact with each other if they need to.
There was a problem hiding this comment.
I understand that advantage, but by exposing the whole state to any of the other components we are opening up to many bugs. If we share all the states it'd be pretty easy any component from anywhere to introduce an unexpected functionality. Additionally the outsider would have to handle all the possible states of the store that it uses, and it doesn't need to do that.
The simpler way, less bug-prone, would be to introduce a channel(method) towards the state that we want to access, and let the state handle the reception properly, in the exact place where we manage it! 😄
There was a problem hiding this comment.
I’m not sure I agree tbh, I feel isolating each domain object from each other completely will make it difficult to share state between them in the future. So far I’ve found multiple domain stores grouped in a root store is a nice balance between having several stores each with their own responsibility but them being able to interact. I’d agree with this approach only if these models won’t need to have any interaction with the other stores, is this the case here?
There was a problem hiding this comment.
I’m not sure I agree tbh, I feel isolating each domain object from each other completely will make it difficult to share state between them in the future.
Not difficult at all. In fact, it's easier to reason about than handling all the combinations of a shared state in a variety of components. As I mentioned before:
- If we want to trigger some event in the local state, we just implement a method which is called through an official channel instead of mutating the values directly.
- If we want to make some properties readable by other components then we expose only those properties.
The alternative of a global shared state allows any component to change any state for any screen and that is a source of unpredictability. If multiple components alter and access the same property, there is no way of predicting what order the mutations will happen in, nor there is a way to guarantee that the changes will happen at the right moment.
There was a problem hiding this comment.
It might even seem that it's more code, but it's not. Because we create an official channel to mutate the specific state, the outsider components need only call it, whereas if we mutate directly from those outsider components then they have to handle every intermediary state until the correct one is reached.
Not to mention that properly testing a several small states is much easier than testing one large shared state.
There was a problem hiding this comment.
@gorjan5sk I'll give this approach a try, I think it'll make more sense to me when I start applying it 🙂
There was a problem hiding this comment.
Cool! :) I suspect (but not sure) we'll see it in action in my next PR and we'll see how well it works in the current codebase.
| navigator?.clipboard?.writeText(secretKey); | ||
| }} | ||
| /> | ||
| ); |
There was a problem hiding this comment.
We could create a separate higher order component that returns receives icon and click handler and returns corresponding <IconButton /> component.
There was a problem hiding this comment.
But, isn't that what IconButton already does?
There was a problem hiding this comment.
I was referring to code duplication related to IconButton. For example, we have defined copy variable in 3 different places and only onClick handler is different there, so we could add abstraction over it. However, after looking into IconButton and its children, it seems introducing a new component will really not be so useful here.
