Support adding phone numbers in UserSettings #756

dbkr · 2017-03-16T15:02:00Z

Based off msisdn branch, so probably best to wait until after that's merged to review. [Now merged]

element-hq/element-web#1903

richvdh · 2017-03-21T21:59:09Z

src/AddThreepid.js

@@ -52,10 +53,34 @@ class AddThreepid {
    }

    /**
+     * Attempt to add a msisdn threepid. This will trigger a side-effect of
+     * sending a test message to the provided phone number.
+     * @param {string} emailAddress The email address to add


email -> phone

richvdh · 2017-03-21T22:00:32Z

src/AddThreepid.js

+    /**
+     * Takes a phone number verification code as entered by the user and validates
+     * it with the ID server, then if successful, adds the phone number.
+     * @return {Promise} Resolves if the email address was added. Rejects with an object


email address -> phone number

ironic given you'd fixed the email one :)

richvdh · 2017-03-21T22:03:01Z

src/components/structures/UserSettings.js

+        const ErrorDialog = sdk.getComponent("dialogs.ErrorDialog");
+        const QuestionDialog = sdk.getComponent("dialogs.QuestionDialog");
+
+        this._addThreepid = new AddThreepid();


hrm. What happens if someone tries to add a misisdn and an email at the same time? Might be possible if the requestToken call takes ages?

Refactored so this doesn't matter

richvdh · 2017-03-21T22:04:35Z

src/components/structures/UserSettings.js

+        const QuestionDialog = sdk.getComponent("dialogs.QuestionDialog");
+
+        this._addThreepid = new AddThreepid();
+        // we always phone numbers when registering, so let's do the


we always [bind] ?

richvdh · 2017-03-21T22:08:15Z

src/components/structures/UserSettings.js

+                </div>
+            );
+        }
+        if (this.state.msisdn_add_pending) {


how about factoriong the whole add-phone-number business out to a separate component? Apart from making this neater, it would solve the fight over this._addThreePid?

Done, although the CSS is a bit of a mess because it needs to be multiple parts of a table layout. Unsure how to fix this as all the solutions I can think of involve passing around the components or some callback functions, and I can't see a react-blessed way of doing that.

richvdh · 2017-03-21T22:09:30Z

looks like we still have a flaky test. Is that known?

dbkr · 2017-03-22T15:49:29Z

Also it might make sense to do similar things to the email adding bit, if this looks sensible. It seems the test is flaky - I was going to try & make it less flaky by using the waitForComponent stuff from react-sdk, although this involves cross-project testing code which we've not done before.

richvdh · 2017-03-22T15:53:58Z

src/components/views/settings/AddPhoneNumber.js

+import Modal from '../../../Modal';
+
+
+class AddPhoneNumber extends React.Component {


I asked this before, but... I thought we were sticking with react.createClass for now.

Point - I'll change it, especially as it makes WithMatrixClient much less awful

richvdh · 2017-03-22T15:55:06Z

src/components/views/settings/AddPhoneNumber.js

+
+    _addMsisdn() {
+        const ErrorDialog = sdk.getComponent("dialogs.ErrorDialog");
+        const QuestionDialog = sdk.getComponent("dialogs.QuestionDialog");


richvdh · 2017-03-22T15:55:45Z

src/components/views/settings/AddPhoneNumber.js

+            });
+        }).finally(() => {
+            this.setState({msisdn_add_pending: false});
+        }).done();;


richvdh · 2017-03-22T15:56:06Z

src/components/views/settings/AddPhoneNumber.js

+                description: msg,
+            });
+        }).finally(() => {
+            this.setState({msisdn_add_pending: false});


needs an unmounted guard

richvdh · 2017-03-22T15:57:29Z

src/components/views/settings/AddPhoneNumber.js

+        // we always bind phone numbers when registering, so let's do the
+        // same here.
+        this._addThreepid.addMsisdn(this.state.phoneCountry, this.state.phoneNumber, true).then((resp) => {
+            this._promptForMsisdnVerificationCode(resp.msisdn);


probably needs an unmounted guard?

richvdh · 2017-03-22T16:00:25Z

src/components/views/settings/AddPhoneNumber.js

+    _promptForMsisdnVerificationCode(msisdn, err) {
+        const TextInputDialog = sdk.getComponent("dialogs.TextInputDialog");
+        let msgElements = [
+            <div>A text message has been sent to +{msisdn}.


doesn't this need an index, to keep react happy?

ah yes, probably

richvdh · 2017-03-22T16:00:54Z

src/components/views/settings/AddPhoneNumber.js

+                    this._addThreepid = null;
+                    return;
+                }
+                this.setState({msisdn_add_pending: true});


probably needs an unmounted guard?

richvdh · 2017-03-22T16:01:03Z

src/components/views/settings/AddPhoneNumber.js

+                }).catch((err) => {
+                    this._promptForMsisdnVerificationCode(msisdn, err);
+                }).finally(() => {
+                    this.setState({msisdn_add_pending: false});


unmounted guard

richvdh · 2017-03-22T16:01:54Z

src/components/views/settings/AddPhoneNumber.js

+        const Loader = sdk.getComponent("elements.Spinner");
+        if (this.state.msisdn_add_pending) {
+            return <Loader />;
+        } else if (!this.props.matrixClient.isGuest()) {


return early if isGuest to lose a level of indentation?

Unmounted guards, extra semicolon, return early to lose indent level, add keys.

richvdh

nearly

richvdh · 2017-03-24T10:37:02Z

src/components/views/settings/AddPhoneNumber.js

-                </form>
-            );
+        } else if (this.props.matrixClient.isGuest()) {
+            return null;


I don't think you're allowed to return null from a render. Return an empty div or sth instead.

Ah, right you are.

richvdh · 2017-03-24T10:37:55Z

src/components/views/settings/AddPhoneNumber.js

+    },
+
+    getInitialState: function() {
+        return {


missing msisdn_add_pending

richvdh · 2017-03-24T10:38:49Z

src/components/structures/UserSettings.js

@@ -132,13 +133,15 @@ module.exports = React.createClass({
            threePids: [],
            phase: "UserSettings.LOADING", // LOADING, DISPLAY
            email_add_pending: false,
+            msisdn_add_pending: false,


dbkr added 2 commits March 16, 2017 14:56

Support adding phone numbers in UserSettings

af8c3ed

Merge remote-tracking branch 'origin/develop' into dbkr/add_msisdn

d5f7b39

dbkr assigned richvdh Mar 21, 2017

richvdh suggested changes Mar 21, 2017

View reviewed changes

richvdh assigned dbkr and unassigned richvdh Mar 21, 2017

dbkr added 3 commits March 22, 2017 12:00

Comment typos

6a37fc4

Factor out AddPhoneNumber to a separate component

4cd24d1

Make phone number form a bit more semantic

cca607d

dbkr assigned richvdh and unassigned dbkr Mar 22, 2017

richvdh suggested changes Mar 22, 2017

View reviewed changes

richvdh assigned dbkr and unassigned richvdh Mar 22, 2017

dbkr added 4 commits March 22, 2017 16:15

Convert to old style react class

e39979a

Unmounted guard

6b78440

More PR feedback

b58d8bf

Unmounted guards, extra semicolon, return early to lose indent level, add keys.

Another unmounted guard

d527214

dbkr assigned richvdh and unassigned dbkr Mar 22, 2017

richvdh suggested changes Mar 24, 2017

View reviewed changes

richvdh assigned dbkr and unassigned richvdh Mar 24, 2017

PR feedback fixes

5e3b991

dbkr assigned richvdh and unassigned dbkr Mar 24, 2017

richvdh approved these changes Mar 24, 2017

View reviewed changes

richvdh assigned dbkr Mar 24, 2017

richvdh removed their assignment Mar 24, 2017

dbkr merged commit a8d85ca into develop Mar 24, 2017

		import Modal from '../../../Modal';


		class AddPhoneNumber extends React.Component {

Support adding phone numbers in UserSettings #756

Support adding phone numbers in UserSettings #756

Conversation

dbkr commented Mar 16, 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

richvdh commented Mar 21, 2017

dbkr commented Mar 22, 2017

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

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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dbkr commented Mar 16, 2017 •

edited

Loading