From f04c347df7dad67cc698ca246f12188bfde71803 Mon Sep 17 00:00:00 2001 From: "J. Ryan Stinnett" Date: Wed, 11 Sep 2019 11:33:58 +0100 Subject: [PATCH 1/4] Lift 3PID state management up to Settings tab This pulls the 3PID state management in Settings up to a single location so that the Account and Discovery sections now work from a single list that updates immediately. Fixes https://github.com/vector-im/riot-web/issues/10519 --- src/boundThreepids.js | 5 +-- src/components/views/settings/SetIdServer.js | 4 +-- .../views/settings/account/EmailAddresses.js | 33 ++++++++--------- .../views/settings/account/PhoneNumbers.js | 33 ++++++++--------- .../settings/discovery/EmailAddresses.js | 22 +++--------- .../views/settings/discovery/PhoneNumbers.js | 22 +++--------- .../tabs/user/GeneralUserSettingsTab.js | 35 ++++++++++++++++--- 7 files changed, 77 insertions(+), 77 deletions(-) diff --git a/src/boundThreepids.js b/src/boundThreepids.js index 799728f801..90a5bd2a6f 100644 --- a/src/boundThreepids.js +++ b/src/boundThreepids.js @@ -16,7 +16,7 @@ limitations under the License. import IdentityAuthClient from './IdentityAuthClient'; -export async function getThreepidBindStatus(client, filterMedium) { +export async function getThreepidsWithBindStatus(client, filterMedium) { const userId = client.getUserId(); let { threepids } = await client.getThreePids(); @@ -24,7 +24,8 @@ export async function getThreepidBindStatus(client, filterMedium) { threepids = threepids.filter((a) => a.medium === filterMedium); } - if (threepids.length > 0) { + // Check bind status assuming we have an IS and terms are agreed + if (threepids.length > 0 && client.getIdentityServerUrl()) { // TODO: Handle terms agreement // See https://github.com/vector-im/riot-web/issues/10522 const authClient = new IdentityAuthClient(); diff --git a/src/components/views/settings/SetIdServer.js b/src/components/views/settings/SetIdServer.js index ae550725f1..9ef5fb295e 100644 --- a/src/components/views/settings/SetIdServer.js +++ b/src/components/views/settings/SetIdServer.js @@ -22,7 +22,7 @@ import sdk from '../../../index'; import MatrixClientPeg from "../../../MatrixClientPeg"; import Modal from '../../../Modal'; import dis from "../../../dispatcher"; -import { getThreepidBindStatus } from '../../../boundThreepids'; +import { getThreepidsWithBindStatus } from '../../../boundThreepids'; import IdentityAuthClient from "../../../IdentityAuthClient"; import {SERVICE_TYPES} from "matrix-js-sdk"; import {abbreviateUrl, unabbreviateUrl} from "../../../utils/UrlUtils"; @@ -249,7 +249,7 @@ export default class SetIdServer extends React.Component { }; async _showServerChangeWarning({ title, unboundMessage, button }) { - const threepids = await getThreepidBindStatus(MatrixClientPeg.get()); + const threepids = await getThreepidsWithBindStatus(MatrixClientPeg.get()); const boundThreepids = threepids.filter(tp => tp.bound); let message; diff --git a/src/components/views/settings/account/EmailAddresses.js b/src/components/views/settings/account/EmailAddresses.js index eb60d4a322..b7324eb272 100644 --- a/src/components/views/settings/account/EmailAddresses.js +++ b/src/components/views/settings/account/EmailAddresses.js @@ -23,8 +23,8 @@ import Field from "../../elements/Field"; import AccessibleButton from "../../elements/AccessibleButton"; import * as Email from "../../../../email"; import AddThreepid from "../../../../AddThreepid"; -const sdk = require('../../../../index'); -const Modal = require("../../../../Modal"); +import sdk from '../../../../index'; +import Modal from '../../../../Modal'; /* TODO: Improve the UX for everything in here. @@ -113,11 +113,15 @@ export class ExistingEmailAddress extends React.Component { } export default class EmailAddresses extends React.Component { - constructor() { - super(); + static propTypes = { + emails: PropTypes.array.isRequired, + onEmailsChange: PropTypes.func.isRequired, + } + + constructor(props) { + super(props); this.state = { - emails: [], verifying: false, addTask: null, continueDisabled: false, @@ -125,16 +129,9 @@ export default class EmailAddresses extends React.Component { }; } - componentWillMount(): void { - const client = MatrixClientPeg.get(); - - client.getThreePids().then((addresses) => { - this.setState({emails: addresses.threepids.filter((a) => a.medium === 'email')}); - }); - } - _onRemoved = (address) => { - this.setState({emails: this.state.emails.filter((e) => e !== address)}); + const emails = this.props.emails.filter((e) => e !== address); + this.props.onEmailsChange(emails); }; _onChangeNewEmailAddress = (e) => { @@ -184,12 +181,16 @@ export default class EmailAddresses extends React.Component { this.state.addTask.checkEmailLinkClicked().then(() => { const email = this.state.newEmailAddress; this.setState({ - emails: [...this.state.emails, {address: email, medium: "email"}], addTask: null, continueDisabled: false, verifying: false, newEmailAddress: "", }); + const emails = [ + ...this.props.emails, + { address: email, medium: "email" }, + ]; + this.props.onEmailsChange(emails); }).catch((err) => { this.setState({continueDisabled: false}); if (err.errcode !== 'M_THREEPID_AUTH_FAILED') { @@ -204,7 +205,7 @@ export default class EmailAddresses extends React.Component { }; render() { - const existingEmailElements = this.state.emails.map((e) => { + const existingEmailElements = this.props.emails.map((e) => { return ; }); diff --git a/src/components/views/settings/account/PhoneNumbers.js b/src/components/views/settings/account/PhoneNumbers.js index fbb5b7e561..8f91eb22cc 100644 --- a/src/components/views/settings/account/PhoneNumbers.js +++ b/src/components/views/settings/account/PhoneNumbers.js @@ -23,8 +23,8 @@ import Field from "../../elements/Field"; import AccessibleButton from "../../elements/AccessibleButton"; import AddThreepid from "../../../../AddThreepid"; import CountryDropdown from "../../auth/CountryDropdown"; -const sdk = require('../../../../index'); -const Modal = require("../../../../Modal"); +import sdk from '../../../../index'; +import Modal from '../../../../Modal'; /* TODO: Improve the UX for everything in here. @@ -108,11 +108,15 @@ export class ExistingPhoneNumber extends React.Component { } export default class PhoneNumbers extends React.Component { - constructor() { - super(); + static propTypes = { + msisdns: PropTypes.array.isRequired, + onMsisdnsChange: PropTypes.func.isRequired, + } + + constructor(props) { + super(props); this.state = { - msisdns: [], verifying: false, verifyError: false, verifyMsisdn: "", @@ -124,16 +128,9 @@ export default class PhoneNumbers extends React.Component { }; } - componentWillMount(): void { - const client = MatrixClientPeg.get(); - - client.getThreePids().then((addresses) => { - this.setState({msisdns: addresses.threepids.filter((a) => a.medium === 'msisdn')}); - }); - } - _onRemoved = (address) => { - this.setState({msisdns: this.state.msisdns.filter((e) => e !== address)}); + const msisdns = this.props.msisdns.filter((e) => e !== address); + this.props.onMsisdnsChange(msisdns); }; _onChangeNewPhoneNumber = (e) => { @@ -181,7 +178,6 @@ export default class PhoneNumbers extends React.Component { const token = this.state.newPhoneNumberCode; this.state.addTask.haveMsisdnToken(token).then(() => { this.setState({ - msisdns: [...this.state.msisdns, {address: this.state.verifyMsisdn, medium: "msisdn"}], addTask: null, continueDisabled: false, verifying: false, @@ -190,6 +186,11 @@ export default class PhoneNumbers extends React.Component { newPhoneNumber: "", newPhoneNumberCode: "", }); + const msisdns = [ + ...this.props.msisdns, + { address: this.state.verifyMsisdn, medium: "msisdn" }, + ]; + this.props.onMsisdnsChange(msisdns); }).catch((err) => { this.setState({continueDisabled: false}); if (err.errcode !== 'M_THREEPID_AUTH_FAILED') { @@ -210,7 +211,7 @@ export default class PhoneNumbers extends React.Component { }; render() { - const existingPhoneElements = this.state.msisdns.map((p) => { + const existingPhoneElements = this.props.msisdns.map((p) => { return ; }); diff --git a/src/components/views/settings/discovery/EmailAddresses.js b/src/components/views/settings/discovery/EmailAddresses.js index 4d18c1d355..c6ec826de6 100644 --- a/src/components/views/settings/discovery/EmailAddresses.js +++ b/src/components/views/settings/discovery/EmailAddresses.js @@ -23,7 +23,6 @@ import MatrixClientPeg from "../../../../MatrixClientPeg"; import sdk from '../../../../index'; import Modal from '../../../../Modal'; import AddThreepid from '../../../../AddThreepid'; -import { getThreepidBindStatus } from '../../../../boundThreepids'; /* TODO: Improve the UX for everything in here. @@ -187,27 +186,14 @@ export class EmailAddress extends React.Component { } export default class EmailAddresses extends React.Component { - constructor() { - super(); - - this.state = { - loaded: false, - emails: [], - }; - } - - async componentWillMount() { - const client = MatrixClientPeg.get(); - - const emails = await getThreepidBindStatus(client, 'email'); - - this.setState({ emails }); + static propTypes = { + emails: PropTypes.array.isRequired, } render() { let content; - if (this.state.emails.length > 0) { - content = this.state.emails.map((e) => { + if (this.props.emails.length > 0) { + content = this.props.emails.map((e) => { return ; }); } else { diff --git a/src/components/views/settings/discovery/PhoneNumbers.js b/src/components/views/settings/discovery/PhoneNumbers.js index fdebac5d22..6d5c8ad3b4 100644 --- a/src/components/views/settings/discovery/PhoneNumbers.js +++ b/src/components/views/settings/discovery/PhoneNumbers.js @@ -23,7 +23,6 @@ import MatrixClientPeg from "../../../../MatrixClientPeg"; import sdk from '../../../../index'; import Modal from '../../../../Modal'; import AddThreepid from '../../../../AddThreepid'; -import { getThreepidBindStatus } from '../../../../boundThreepids'; /* TODO: Improve the UX for everything in here. @@ -206,27 +205,14 @@ export class PhoneNumber extends React.Component { } export default class PhoneNumbers extends React.Component { - constructor() { - super(); - - this.state = { - loaded: false, - msisdns: [], - }; - } - - async componentWillMount() { - const client = MatrixClientPeg.get(); - - const msisdns = await getThreepidBindStatus(client, 'msisdn'); - - this.setState({ msisdns }); + static propTypes = { + msisdns: PropTypes.array.isRequired, } render() { let content; - if (this.state.msisdns.length > 0) { - content = this.state.msisdns.map((e) => { + if (this.props.msisdns.length > 0) { + content = this.props.msisdns.map((e) => { return ; }); } else { diff --git a/src/components/views/settings/tabs/user/GeneralUserSettingsTab.js b/src/components/views/settings/tabs/user/GeneralUserSettingsTab.js index 9c37730fc5..02f9cf3cd3 100644 --- a/src/components/views/settings/tabs/user/GeneralUserSettingsTab.js +++ b/src/components/views/settings/tabs/user/GeneralUserSettingsTab.js @@ -37,6 +37,7 @@ import {Service, startTermsFlow} from "../../../../../Terms"; import {SERVICE_TYPES} from "matrix-js-sdk"; import IdentityAuthClient from "../../../../../IdentityAuthClient"; import {abbreviateUrl} from "../../../../../utils/UrlUtils"; +import { getThreepidsWithBindStatus } from '../../../../../boundThreepids'; export default class GeneralUserSettingsTab extends React.Component { static propTypes = { @@ -58,17 +59,27 @@ export default class GeneralUserSettingsTab extends React.Component { // agreedUrls, // From the startTermsFlow callback // resolve, // Promise resolve function for startTermsFlow callback }, + emails: [], + msisdns: [], }; this.dispatcherRef = dis.register(this._onAction); } async componentWillMount() { - const serverRequiresIdServer = await MatrixClientPeg.get().doesServerRequireIdServerParam(); + const cli = MatrixClientPeg.get(); + + const serverRequiresIdServer = await cli.doesServerRequireIdServerParam(); this.setState({serverRequiresIdServer}); // Check to see if terms need accepting this._checkTerms(); + + // Need to get 3PIDs generally for Account section and possibly also for + // Discovery (assuming we have an IS and terms are agreed). + const threepids = await getThreepidsWithBindStatus(cli); + this.setState({ emails: threepids.filter((a) => a.medium === 'email') }); + this.setState({ msisdns: threepids.filter((a) => a.medium === 'msisdn') }); } componentWillUnmount() { @@ -82,6 +93,14 @@ export default class GeneralUserSettingsTab extends React.Component { } }; + _onEmailsChange = (emails) => { + this.setState({ emails }); + } + + _onMsisdnsChange = (msisdns) => { + this.setState({ msisdns }); + } + async _checkTerms() { if (!this.state.haveIdServer) { this.setState({idServerHasUnsignedTerms: false}); @@ -200,10 +219,16 @@ export default class GeneralUserSettingsTab extends React.Component { if (this.state.haveIdServer || this.state.serverRequiresIdServer === false) { threepidSection =
{_t("Email addresses")} - + {_t("Phone numbers")} - +
; } else if (this.state.serverRequiresIdServer === null) { threepidSection = ; @@ -279,10 +304,10 @@ export default class GeneralUserSettingsTab extends React.Component { const threepidSection = this.state.haveIdServer ?
{_t("Email addresses")} - + {_t("Phone numbers")} - +
: null; return ( From 0b7995dc11f06084540037201e3fad16214c26a0 Mon Sep 17 00:00:00 2001 From: "J. Ryan Stinnett" Date: Wed, 11 Sep 2019 13:36:10 +0100 Subject: [PATCH 2/4] Improve terms handling for 3PID state gathering This changes the 3PID state gathering (used in Settings) to ignore terms errors (no modals will be shown) on the assumption that other UX handles this case. --- src/IdentityAuthClient.js | 2 +- src/boundThreepids.js | 37 +++++++++++-------- .../tabs/user/GeneralUserSettingsTab.js | 2 +- 3 files changed, 23 insertions(+), 18 deletions(-) diff --git a/src/IdentityAuthClient.js b/src/IdentityAuthClient.js index 075ae93709..7cbad074bf 100644 --- a/src/IdentityAuthClient.js +++ b/src/IdentityAuthClient.js @@ -65,7 +65,7 @@ export default class IdentityAuthClient { } // Returns a promise that resolves to the access_token string from the IS - async getAccessToken(check=true) { + async getAccessToken({ check = true } = {}) { if (!this.authEnabled) { // The current IS doesn't support authentication return null; diff --git a/src/boundThreepids.js b/src/boundThreepids.js index 90a5bd2a6f..7f727d8e64 100644 --- a/src/boundThreepids.js +++ b/src/boundThreepids.js @@ -26,26 +26,31 @@ export async function getThreepidsWithBindStatus(client, filterMedium) { // Check bind status assuming we have an IS and terms are agreed if (threepids.length > 0 && client.getIdentityServerUrl()) { - // TODO: Handle terms agreement - // See https://github.com/vector-im/riot-web/issues/10522 - const authClient = new IdentityAuthClient(); - const identityAccessToken = await authClient.getAccessToken(); + try { + const authClient = new IdentityAuthClient(); + const identityAccessToken = await authClient.getAccessToken({ check: false }); - // Restructure for lookup query - const query = threepids.map(({ medium, address }) => [medium, address]); - const lookupResults = await client.bulkLookupThreePids(query, identityAccessToken); + // Restructure for lookup query + const query = threepids.map(({ medium, address }) => [medium, address]); + const lookupResults = await client.bulkLookupThreePids(query, identityAccessToken); - // Record which are already bound - for (const [medium, address, mxid] of lookupResults.threepids) { - if (mxid !== userId) { - continue; + // Record which are already bound + for (const [medium, address, mxid] of lookupResults.threepids) { + if (mxid !== userId) { + continue; + } + if (filterMedium && medium !== filterMedium) { + continue; + } + const threepid = threepids.find(e => e.medium === medium && e.address === address); + if (!threepid) continue; + threepid.bound = true; } - if (filterMedium && medium !== filterMedium) { - continue; + } catch (e) { + // Ignore terms errors here and assume other flows handle this + if (!(e.errcode === "M_TERMS_NOT_SIGNED")) { + throw e; } - const threepid = threepids.find(e => e.medium === medium && e.address === address); - if (!threepid) continue; - threepid.bound = true; } } diff --git a/src/components/views/settings/tabs/user/GeneralUserSettingsTab.js b/src/components/views/settings/tabs/user/GeneralUserSettingsTab.js index 02f9cf3cd3..f1ca314f13 100644 --- a/src/components/views/settings/tabs/user/GeneralUserSettingsTab.js +++ b/src/components/views/settings/tabs/user/GeneralUserSettingsTab.js @@ -110,7 +110,7 @@ export default class GeneralUserSettingsTab extends React.Component { // By starting the terms flow we get the logic for checking which terms the user has signed // for free. So we might as well use that for our own purposes. const authClient = new IdentityAuthClient(); - const idAccessToken = await authClient.getAccessToken(/*check=*/false); + const idAccessToken = await authClient.getAccessToken({ check: false }); startTermsFlow([new Service( SERVICE_TYPES.IS, MatrixClientPeg.get().getIdentityServerUrl(), From db33c138aa50d60a4c93e0c3eba08107e3fcf421 Mon Sep 17 00:00:00 2001 From: "J. Ryan Stinnett" Date: Wed, 11 Sep 2019 16:16:07 +0100 Subject: [PATCH 3/4] Update all 3PID state in Settings when IS changes This ensures we update all 3PID state (like bind status) whenever the IS changes. --- .../settings/discovery/EmailAddresses.js | 5 ++++ .../views/settings/discovery/PhoneNumbers.js | 5 ++++ .../tabs/user/GeneralUserSettingsTab.js | 24 ++++++++++++------- 3 files changed, 25 insertions(+), 9 deletions(-) diff --git a/src/components/views/settings/discovery/EmailAddresses.js b/src/components/views/settings/discovery/EmailAddresses.js index c6ec826de6..d6628f900a 100644 --- a/src/components/views/settings/discovery/EmailAddresses.js +++ b/src/components/views/settings/discovery/EmailAddresses.js @@ -58,6 +58,11 @@ export class EmailAddress extends React.Component { }; } + componentWillReceiveProps(nextProps) { + const { bound } = nextProps.email; + this.setState({ bound }); + } + async changeBinding({ bind, label, errorTitle }) { const ErrorDialog = sdk.getComponent("dialogs.ErrorDialog"); const { medium, address } = this.props.email; diff --git a/src/components/views/settings/discovery/PhoneNumbers.js b/src/components/views/settings/discovery/PhoneNumbers.js index 6d5c8ad3b4..99a90f23fb 100644 --- a/src/components/views/settings/discovery/PhoneNumbers.js +++ b/src/components/views/settings/discovery/PhoneNumbers.js @@ -50,6 +50,11 @@ export class PhoneNumber extends React.Component { }; } + componentWillReceiveProps(nextProps) { + const { bound } = nextProps.msisdn; + this.setState({ bound }); + } + async changeBinding({ bind, label, errorTitle }) { const ErrorDialog = sdk.getComponent("dialogs.ErrorDialog"); const { medium, address } = this.props.msisdn; diff --git a/src/components/views/settings/tabs/user/GeneralUserSettingsTab.js b/src/components/views/settings/tabs/user/GeneralUserSettingsTab.js index f1ca314f13..b378db707a 100644 --- a/src/components/views/settings/tabs/user/GeneralUserSettingsTab.js +++ b/src/components/views/settings/tabs/user/GeneralUserSettingsTab.js @@ -72,14 +72,7 @@ export default class GeneralUserSettingsTab extends React.Component { const serverRequiresIdServer = await cli.doesServerRequireIdServerParam(); this.setState({serverRequiresIdServer}); - // Check to see if terms need accepting - this._checkTerms(); - - // Need to get 3PIDs generally for Account section and possibly also for - // Discovery (assuming we have an IS and terms are agreed). - const threepids = await getThreepidsWithBindStatus(cli); - this.setState({ emails: threepids.filter((a) => a.medium === 'email') }); - this.setState({ msisdns: threepids.filter((a) => a.medium === 'msisdn') }); + this._getThreepidState(); } componentWillUnmount() { @@ -89,7 +82,7 @@ export default class GeneralUserSettingsTab extends React.Component { _onAction = (payload) => { if (payload.action === 'id_server_changed') { this.setState({haveIdServer: Boolean(MatrixClientPeg.get().getIdentityServerUrl())}); - this._checkTerms(); + this._getThreepidState(); } }; @@ -101,6 +94,19 @@ export default class GeneralUserSettingsTab extends React.Component { this.setState({ msisdns }); } + async _getThreepidState() { + const cli = MatrixClientPeg.get(); + + // Check to see if terms need accepting + this._checkTerms(); + + // Need to get 3PIDs generally for Account section and possibly also for + // Discovery (assuming we have an IS and terms are agreed). + const threepids = await getThreepidsWithBindStatus(cli); + this.setState({ emails: threepids.filter((a) => a.medium === 'email') }); + this.setState({ msisdns: threepids.filter((a) => a.medium === 'msisdn') }); + } + async _checkTerms() { if (!this.state.haveIdServer) { this.setState({idServerHasUnsignedTerms: false}); From c542ea555967f1f8a8bfcb4e4ffaf7c30b563a95 Mon Sep 17 00:00:00 2001 From: "J. Ryan Stinnett" Date: Wed, 11 Sep 2019 16:55:45 +0100 Subject: [PATCH 4/4] Force boolean value --- src/boundThreepids.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/boundThreepids.js b/src/boundThreepids.js index 7f727d8e64..3b32815913 100644 --- a/src/boundThreepids.js +++ b/src/boundThreepids.js @@ -25,7 +25,7 @@ export async function getThreepidsWithBindStatus(client, filterMedium) { } // Check bind status assuming we have an IS and terms are agreed - if (threepids.length > 0 && client.getIdentityServerUrl()) { + if (threepids.length > 0 && !!client.getIdentityServerUrl()) { try { const authClient = new IdentityAuthClient(); const identityAccessToken = await authClient.getAccessToken({ check: false });