Remove "Current Password" input if mx_pass exists #881

lukebarnard1 · 2017-05-11T16:49:55Z

If the user is PWLU, do not show "Current Password" field in ChangePassword and when setting a new password, use the cached password.

dbkr

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.

lukebarnard1 · 2017-05-12T09:21:03Z

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)

dbkr

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.

dbkr · 2017-05-12T13:13:17Z

src/components/structures/MatrixChat.js

@@ -896,6 +897,12 @@ module.exports = React.createClass({
        });
    },

+    _setStateFromSessionStore() {
+        this.setState({
+            userHasGeneratedPassword: Boolean(this._sessionStore.getCachedPassword()),


This doesn't actually seem to be used anywhere?

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.

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.

Agreed! I've since moved userHasGeneratedPassword to LoggedInView anyway.

dbkr · 2017-05-12T13:32:52Z

src/stores/SessionStore.js

+
+// Export singleton getter
+let singletonSessionStore = null;
+export default function getSessionStore() {


It's a bit weird that we export a getter function from this which is different to how dispatcher / MatrixClientPeg etc do it.

Oh I didn't check how they did it. I'll make more like them.

dbkr · 2017-05-12T13:33:15Z

src/stores/SessionStore.js

+ * store that listens for actions and updates its state accordingly, informing any
+ * listeners (views) of state changes via the 'update' event.
+ */
+function SessionStore() {


Can this not be an ES6 class?

Sure, but I thought we were moving away from those. Unless we're only moving away from them for React components?

lukebarnard1 · 2017-05-12T14:45:25Z

Explicitly pass through userHas..
Use ES6 class for SessionStore
Use peg-y singleton

richvdh · 2017-05-15T09:10:34Z

src/stores/SessionStore.js

+ * 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 {


shouldn't this extend flux.Store if it's a flux store?

I don't see why not!

richvdh · 2017-05-15T09:24:05Z

src/stores/SessionStore.js

+
+let singletonSessionStore = null;
+if (!singletonSessionStore) {
+    singletonSessionStore = new SessionStore();


is there a reason this needs to be a singleton rather than (say) one per MatrixChat or even one per LoggedInView?

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/

well, you can pass it down via the react context rather than the properties. But maybe it doesn't matter for now anyway.

richvdh · 2017-05-15T09:28:32Z

src/stores/SessionStore.js

+    _update() {
+        // Persist state to localStorage
+        if (this._state.cachedPassword) {
+            localStorage.setItem('mx_pass', this._state.cachedPassword);


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.

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.

yes, true. So perhaps the right answer is to move all the credential-stashing that Lifecycle is doing into SessionStore?

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?

Sure but maybe we can leave that for another PR?

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?

@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

richvdh · 2017-05-15T10:20:35Z

src/components/structures/MatrixChat.js

@@ -249,6 +250,10 @@ module.exports = React.createClass({
                register_hs_url: paramHs,
            });
        }
+
+        this._sessionStore = sessionStore;


so, is there a reason this needs to be here rather than LoggedInView?

(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?

Actually, no this was a side-effect of MatrixChat being the thing originally managing mx_pass.

lukebarnard1 · 2017-05-15T13:08:40Z

Todo:

SessionStore should extend flux.Store
Reference to sessionStore to be moved from MatrixChat to LoggedInView

MatrixChat didn't actually use the sessionStore, so this is one less prop to pass.

richvdh · 2017-05-15T14:57:29Z

src/components/structures/LoggedInView.js

@@ -80,6 +77,10 @@ export default React.createClass({
        this._scrollStateMap = {};

        document.addEventListener('keydown', this._onKeyDown);
+
+        this._sessionStore = sessionStore;
+        this._sessionStore.addListener(this._setStateFromSessionStore);


do we not need to remove this on unmount?

richvdh · 2017-05-15T14:59:04Z

src/components/views/settings/ChangePassword.js

        };
    },

+    componentWillMount: function() {
+        this._sessionStore = sessionStore;
+        this._sessionStore.addListener(this._setStateFromSessionStore);


remove on unmount?

richvdh · 2017-05-15T15:00:25Z

src/components/structures/MatrixChat.js

@@ -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,


so is this (and the reference to it in render) now redundant?

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)

richvdh

just a few silly questions left

richvdh · 2017-05-16T09:29:43Z

src/components/structures/LoggedInView.js

    },

    componentWillUnmount: function() {
        document.removeEventListener('keydown', this._onKeyDown);
+        if (this._removeSSListener) {
+            this._removeSSListener();


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.

I would just store the token and call remove on it here explicitly.

WFM

richvdh · 2017-05-16T09:31:10Z

src/components/views/settings/ChangePassword.js

+
+    componentWillUnmount: function() {
+        if (this._removeSSListener) {
+            this._removeSSListener();


richvdh · 2017-05-16T09:32:16Z

src/stores/SessionStore.js

+    _update() {
+        // Persist state to localStorage
+        if (this._state.cachedPassword) {
+            localStorage.setItem('mx_pass', this._state.cachedPassword);


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?

richvdh

lgtm

Remove "Current Password" input if mx_pass exists

8725ef3

If the user is PWLU, do not show "Current Password" field in ChangePassword and when setting a new password, use the cached password.

lukebarnard1 assigned dbkr May 11, 2017

lukebarnard1 mentioned this pull request May 11, 2017

Don't prompt PWLUs for the password they don't know element-hq/element-web#3748

Closed

dbkr suggested changes May 11, 2017

View reviewed changes

dbkr assigned lukebarnard1 and unassigned dbkr May 11, 2017

Implement SessionStore

1176573

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)

dbkr reviewed May 12, 2017

View reviewed changes

dbkr assigned richvdh and unassigned lukebarnard1 May 12, 2017

Luke Barnard added 3 commits May 12, 2017 15:47

Explicitly pass thru userHasGeneratedPassword

5c8187d

Use same singleton impl as MatrixClientPeg for SessionStore

6ffe7ef

ES6 SessionStore

536724e

lukebarnard1 assigned dbkr May 12, 2017

Remove useless comment

2b4c87a

lukebarnard1 mentioned this pull request May 12, 2017

Invite the welcome user after registration if configured #882

Merged

richvdh reviewed May 15, 2017

View reviewed changes

richvdh assigned lukebarnard1 and unassigned richvdh May 15, 2017

Luke Barnard added 2 commits May 15, 2017 14:52

SessionStore extends flux.Store

da3cb0e

Move sessionStore ref from MatrixChat to LoggedInView

f73cf77

MatrixChat didn't actually use the sessionStore, so this is one less prop to pass.

lukebarnard1 assigned richvdh and unassigned lukebarnard1 May 15, 2017

richvdh reviewed May 15, 2017

View reviewed changes

richvdh assigned lukebarnard1 and unassigned richvdh May 15, 2017

Luke Barnard added 2 commits May 15, 2017 17:03

Remove redundant state

eb0041d

Remove SessionStore listener on unmount

269fd51

lukebarnard1 assigned richvdh and unassigned lukebarnard1 May 15, 2017

lukebarnard1 mentioned this pull request May 16, 2017

Implement dialog to set password element-hq/element-web#3921

Merged

richvdh reviewed May 16, 2017

View reviewed changes

Luke Barnard added 2 commits May 16, 2017 11:52

Reference store token, call .remove on it on unmount

eb36e97

Add comment to Lifecycle

633c6b3

richvdh approved these changes May 16, 2017

View reviewed changes

richvdh removed their assignment May 16, 2017

lukebarnard1 merged commit c635037 into new-guest-access May 16, 2017

Remove "Current Password" input if mx_pass exists #881

Remove "Current Password" input if mx_pass exists #881

Conversation

lukebarnard1 commented May 11, 2017

dbkr left a comment

Choose a reason for hiding this comment

lukebarnard1 commented May 12, 2017

dbkr left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lukebarnard1 May 12, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lukebarnard1 commented May 12, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lukebarnard1 May 15, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lukebarnard1 commented May 15, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

richvdh left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

richvdh left a comment

Choose a reason for hiding this comment

lukebarnard1 May 12, 2017 •

edited

Loading

lukebarnard1 commented May 12, 2017 •

edited

Loading

lukebarnard1 May 15, 2017 •

edited

Loading

lukebarnard1 commented May 15, 2017 •

edited

Loading