-
-
Notifications
You must be signed in to change notification settings - Fork 833
Remove "Current Password" input if mx_pass exists #881
Remove "Current Password" input if mx_pass exists #881
Conversation
If the user is PWLU, do not show "Current Password" field in ChangePassword and when setting a new password, use the cached password.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure about passing the password down from MatrixChat - we don't generally pass most things down all the way from the top, instead just leaving the components that care about them to fish them out of the store (whatever 'the store' is: in this case localStorage). I would just do the getItem and remoteItem in UserSettings (or a separate logic class if it's complex enough). This would also save you doing the dispatch (which is not really what dispatches are for). Also in general we're trying to get as much stuff as possible out of MatrixChat rather than give it more jobs.
IRL we discussed a much nicer way of handling app-specific state - flux stores! We're going to use this opportunity to make an example of a flux store that hides the mx_pass state. |
This wraps session-related state into a basic flux store. The localStorage item 'mx_pass' is the only thing managed by this store for now but it could easily be extended to track other items (like the teamToken which is passed around through props a lot)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks reasonable. The dispatcher now feels like it has two separate purposes which I dislike: triggering high level UI changes and sending low-level data to the stores. I guess this is really a symptom of me having used the flux dispatcher for something other than the classic flux use case.
To me it feels a bit like the low-level store stuff should just be a straight function call to the store, although I appreciate why flux does it through the dispatcher.
How do people feel about this? Is it that bad? We could even just have two different dispatchers for different purposes.
@richvdh - since this is a reasonable seismic shift in the design, would like input from you.
@@ -896,6 +897,12 @@ module.exports = React.createClass({ | |||
}); | |||
}, | |||
|
|||
_setStateFromSessionStore() { | |||
this.setState({ | |||
userHasGeneratedPassword: Boolean(this._sessionStore.getCachedPassword()), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This doesn't actually seem to be used anywhere?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's used in LoggedInView
... I guess I should pass that through explicitly by I then question why ...this.state
is given to the props of LoggedInView
from MatrixChat
because of confusion like this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the ...this.state
is a hangover from when LoggedInView and MatrixChat were one - it was a way to make sure I wasn't missing any important bits of state. Please make any new bits of state explicit, and if you see any others which aren't being passed through explicitly, fix them. We should probably get rid of ...this.state
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agreed! I've since moved userHasGeneratedPassword
to LoggedInView
anyway.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agreed! I've since moved userHasGeneratedPassword
to LoggedInView
anyway.
src/stores/SessionStore.js
Outdated
|
||
// Export singleton getter | ||
let singletonSessionStore = null; | ||
export default function getSessionStore() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's a bit weird that we export a getter function from this which is different to how dispatcher / MatrixClientPeg etc do it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh I didn't check how they did it. I'll make more like them.
src/stores/SessionStore.js
Outdated
* store that listens for actions and updates its state accordingly, informing any | ||
* listeners (views) of state changes via the 'update' event. | ||
*/ | ||
function SessionStore() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can this not be an ES6 class?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure, but I thought we were moving away from those. Unless we're only moving away from them for React components?
|
src/stores/SessionStore.js
Outdated
* store that listens for actions and updates its state accordingly, informing any | ||
* listeners (views) of state changes via the 'update' event. | ||
*/ | ||
class SessionStore extends EventEmitter { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
shouldn't this extend flux.Store if it's a flux store?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't see why not!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
|
||
let singletonSessionStore = null; | ||
if (!singletonSessionStore) { | ||
singletonSessionStore = new SessionStore(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is there a reason this needs to be a singleton rather than (say) one per MatrixChat or even one per LoggedInView?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If it wasn't a singleton we'd have to pass the store down the tree, running into one of the problems this store aims to solve: passing props through components that shouldn't care/passing too many props. I found a nice reddit comment that summarises this in the form of a rebuttal against ... props
: https://www.reddit.com/r/reactjs/comments/4v3mcb/passing_down_too_many_props_to_child_components/d5v8yb7/
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
well, you can pass it down via the react context rather than the properties. But maybe it doesn't matter for now anyway.
_update() { | ||
// Persist state to localStorage | ||
if (this._state.cachedPassword) { | ||
localStorage.setItem('mx_pass', this._state.cachedPassword); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
hum. Having accesses to localStorage spread out across the app makes me sad - write access in particular. One of the goals of Lifecycle
was to try and collect this sort of thing together.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yep, in practice localStorage
is used throughout the app directly and this doesn't gel with a React-based thing. In an ideal world any changes to a localStorage
thing should first go through a flux store so that the changes can be propagated nicely and the React component in question should have no idea that localStorage
is being used. Lifecycle
could be LifecycleStore
that stores and persists lifecycle-related state.
TBF the change to use SessionStore
is a massive improvement on accessing mx_pass
directly from MatrixChat
etc.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes, true. So perhaps the right answer is to move all the credential-stashing that Lifecycle is doing into SessionStore?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So perhaps the right answer is to move all the credential-stashing that Lifecycle is doing into SessionStore?
it seems like that would work quite well?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure but maybe we can leave that for another PR?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sure. Unless you plan to do one immediately, could you put a comment in Lifecycle
to this effect, so we can remember which direction we were going in?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@richvdh said:
sure. Unless you plan to do one immediately, could you put a comment in Lifecycle to this effect, so we can remember which direction we were going in?
And I can indeed put a TODO in Lifecycle
@@ -249,6 +250,10 @@ module.exports = React.createClass({ | |||
register_hs_url: paramHs, | |||
}); | |||
} | |||
|
|||
this._sessionStore = sessionStore; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
so, is there a reason this needs to be here rather than LoggedInView?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(and even if there is, isn't the point of a flux store that we can just pass the store object down to lower-level controllers (eg LoggedInView) rather than having a mahoosive state
in the top-level component?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually, no this was a side-effect of MatrixChat being the thing originally managing mx_pass.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
Todo:
|
MatrixChat didn't actually use the sessionStore, so this is one less prop to pass.
@@ -80,6 +77,10 @@ export default React.createClass({ | |||
this._scrollStateMap = {}; | |||
|
|||
document.addEventListener('keydown', this._onKeyDown); | |||
|
|||
this._sessionStore = sessionStore; | |||
this._sessionStore.addListener(this._setStateFromSessionStore); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
do we not need to remove this on unmount?
}; | ||
}, | ||
|
||
componentWillMount: function() { | ||
this._sessionStore = sessionStore; | ||
this._sessionStore.addListener(this._setStateFromSessionStore); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
remove on unmount?
@@ -139,8 +139,7 @@ module.exports = React.createClass({ | |||
register_is_url: null, | |||
register_id_sid: null, | |||
|
|||
// Initially, use localStorage as source of truth | |||
userHasGeneratedPassword: localStorage && localStorage.getItem('mx_pass'), | |||
userHasGeneratedPassword: false, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
so is this (and the reference to it in render
) now redundant?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yep, whoops
Until matrix-org/matrix-react-sdk#881, ChangePassword will not know about the cached password (so it won't hide "Current Password" yet). There's also a bit of work left - informing the SessionStore that the password has changed (marked with a TODO)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
just a few silly questions left
}, | ||
|
||
componentWillUnmount: function() { | ||
document.removeEventListener('keydown', this._onKeyDown); | ||
if (this._removeSSListener) { | ||
this._removeSSListener(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
does this actually work right? I can imagine it getting confused about what this
is within the remove
function.
If it works I guess it's fair enough, though personally I would just store the token and call remove
on it here explicitly.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would just store the token and call remove on it here explicitly.
WFM
|
||
componentWillUnmount: function() { | ||
if (this._removeSSListener) { | ||
this._removeSSListener(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ditto
_update() { | ||
// Persist state to localStorage | ||
if (this._state.cachedPassword) { | ||
localStorage.setItem('mx_pass', this._state.cachedPassword); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sure. Unless you plan to do one immediately, could you put a comment in Lifecycle
to this effect, so we can remember which direction we were going in?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm
If the user is PWLU, do not show "Current Password" field in ChangePassword and when setting a new password, use the cached password.