From 88624c8548723b1bb494711d725af98a94ba8d6b Mon Sep 17 00:00:00 2001 From: Luke Barnard Date: Thu, 12 Apr 2018 17:16:18 +0100 Subject: [PATCH 01/11] Upgrade to mocha 5.05 primarily for use of `describe.only` --- package.json | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/package.json b/package.json index 77338b4874..de0179c870 100644 --- a/package.json +++ b/package.json @@ -126,7 +126,7 @@ "karma-summary-reporter": "^1.3.3", "karma-webpack": "^1.7.0", "matrix-react-test-utils": "^0.1.1", - "mocha": "^2.4.5", + "mocha": "^5.0.5", "parallelshell": "^3.0.2", "react-addons-test-utils": "^15.4.0", "require-json": "0.0.1", From bec81d82d2dba1964d4eb11a54271c12995e7491 Mon Sep 17 00:00:00 2001 From: David Baker Date: Fri, 27 Apr 2018 12:57:01 +0100 Subject: [PATCH 02/11] Update version of hoek --- package-lock.json | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/package-lock.json b/package-lock.json index 6dd02674be..23ffa68bef 100644 --- a/package-lock.json +++ b/package-lock.json @@ -3719,9 +3719,9 @@ "integrity": "sha1-uKnFSTISqTkvAiK2SclhFJfr+4g=" }, "hoek": { - "version": "4.2.0", - "resolved": "https://registry.npmjs.org/hoek/-/hoek-4.2.0.tgz", - "integrity": "sha512-v0XCLxICi9nPfYrS9RL8HbYnXi9obYAeLbSP00BmnZwCK9+Ih9WOjoZ8YoHCoav2csqn4FOz4Orldsy2dmDwmQ==" + "version": "4.2.1", + "resolved": "https://registry.npmjs.org/hoek/-/hoek-4.2.1.tgz", + "integrity": "sha512-QLg82fGkfnJ/4iy1xZ81/9SIJiq1NGFUMGs6ParyjBZr6jW2Ufj/snDqTHixNlHdPNwN2RLVD0Pi3igeK9+JfA==" }, "home-or-tmp": { "version": "2.0.0", From a1c442422438866fda9371c2128c5dcca63fd5ef Mon Sep 17 00:00:00 2001 From: Luke Barnard Date: Fri, 27 Apr 2018 14:28:24 +0100 Subject: [PATCH 03/11] Add tests for GroupView --- package.json | 1 + test/components/structures/GroupView-test.js | 378 +++++++++++++++++++ test/test-utils.js | 1 + 3 files changed, 380 insertions(+) create mode 100644 test/components/structures/GroupView-test.js diff --git a/package.json b/package.json index de0179c870..1f979369e3 100644 --- a/package.json +++ b/package.json @@ -125,6 +125,7 @@ "karma-spec-reporter": "^0.0.31", "karma-summary-reporter": "^1.3.3", "karma-webpack": "^1.7.0", + "matrix-mock-request": "^1.2.1", "matrix-react-test-utils": "^0.1.1", "mocha": "^5.0.5", "parallelshell": "^3.0.2", diff --git a/test/components/structures/GroupView-test.js b/test/components/structures/GroupView-test.js new file mode 100644 index 0000000000..2df0599f89 --- /dev/null +++ b/test/components/structures/GroupView-test.js @@ -0,0 +1,378 @@ +/* +Copyright 2018 New Vector Ltd. + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + +import React from 'react'; +import ReactDOM from 'react-dom'; +import ReactTestUtils from 'react-dom/test-utils'; +import expect from 'expect'; +import Promise from 'bluebird'; + +import MockHttpBackend from 'matrix-mock-request'; +import MatrixClientPeg from '../../../src/MatrixClientPeg'; +import sdk from 'matrix-react-sdk'; +import Matrix from 'matrix-js-sdk'; + +import * as TestUtils from 'test-utils'; + +const GroupView = sdk.getComponent('structures.GroupView'); +const WrappedGroupView = TestUtils.wrapInMatrixClientContext(GroupView); + +const Spinner = sdk.getComponent('elements.Spinner'); + +/** + * Call fn before calling componentDidUpdate on a react component instance, inst. + * @param {React.Component} inst an instance of a React component. + * @returns {Promise} promise that resolves when componentDidUpdate is called on + * given component instance. + */ +function waitForUpdate(inst) { + return new Promise((resolve, reject) => { + const cdu = inst.componentDidUpdate; + + inst.componentDidUpdate = (prevProps, prevState, snapshot) => { + resolve(); + + if (cdu) cdu(prevProps, prevState, snapshot); + + inst.componentDidUpdate = cdu; + }; + }); +} + +describe.only('GroupView', function() { + let root; + let rootElement; + let httpBackend; + let summaryResponse; + let summaryResponseWithComplicatedLongDesc; + let summaryResponseWithNoLongDesc; + let summaryResponseWithBadImg; + let groupId; + let groupIdEncoded; + + // Summary response fields + const user = { + is_privileged: true, // can edit the group + is_public: true, // appear as a member to non-members + is_publicised: true, // display flair + }; + const usersSection = { + roles: {}, + total_user_count_estimate: 0, + users: [], + }; + const roomsSection = { + categories: {}, + rooms: [], + total_room_count_estimate: 0, + }; + + beforeEach(function() { + TestUtils.beforeEach(this); + + httpBackend = new MockHttpBackend(); + + Matrix.request(httpBackend.requestFn); + + MatrixClientPeg.get = () => Matrix.createClient({ + baseUrl: 'https://my.home.server', + userId: '@me:here', + accessToken: '123456789', + }); + + summaryResponse = { + profile: { + avatar_url: "mxc://someavatarurl", + is_openly_joinable: true, + is_public: true, + long_description: "This is a LONG description.", + name: "The name of a community", + short_description: "This is a community", + }, + user, + users_section: usersSection, + rooms_section: roomsSection, + }; + summaryResponseWithNoLongDesc = { + profile: { + avatar_url: "mxc://someavatarurl", + is_openly_joinable: true, + is_public: true, + long_description: null, + name: "The name of a community", + short_description: "This is a community", + }, + user, + users_section: usersSection, + rooms_section: roomsSection, + }; + summaryResponseWithComplicatedLongDesc = { + profile: { + avatar_url: "mxc://someavatarurl", + is_openly_joinable: true, + is_public: true, + long_description: ` +

This is a more complicated group page

+

With paragraphs

+
    +
  • And lists!
    • +
  • With list items.
    • +
+

And also images:

`, + name: "The name of a community", + short_description: "This is a community", + }, + user, + users_section: usersSection, + rooms_section: roomsSection, + }; + + summaryResponseWithBadImg = { + profile: { + avatar_url: "mxc://someavatarurl", + is_openly_joinable: true, + is_public: true, + long_description: '

Evil image:

', + name: "The name of a community", + short_description: "This is a community", + }, + user, + users_section: usersSection, + rooms_section: roomsSection, + }; + + groupId = "+" + Math.random().toString(16).slice(2) + ':domain'; + groupIdEncoded = encodeURIComponent(groupId); + + rootElement = document.createElement('div'); + root = ReactDOM.render(, rootElement); + }); + + afterEach(function() { + ReactDOM.unmountComponentAtNode(rootElement); + }); + + it('should show a spinner when first displayed', function() { + ReactTestUtils.findRenderedComponentWithType(root, Spinner); + + // If we don't respond here, the rate limiting done to ensure a maximum of + // 3 concurrent network requests for GroupStore will block subsequent requests + // in other tests. + // + // This is a good case for doing the rate limiting somewhere other than the module + // scope of GroupStore.js + httpBackend.when('GET', '/groups/' + groupIdEncoded + '/summary').respond(200, summaryResponse); + httpBackend.when('GET', '/groups/' + groupIdEncoded + '/users').respond(200, { chunk: [] }); + httpBackend.when('GET', '/groups/' + groupIdEncoded + '/invited_users').respond(200, { chunk: [] }); + httpBackend.when('GET', '/groups/' + groupIdEncoded + '/rooms').respond(200, { chunk: [] }); + + return httpBackend.flush(undefined, undefined, 0); + }); + + it('should indicate failure after failed /summary', function() { + const groupView = ReactTestUtils.findRenderedComponentWithType(root, GroupView); + const prom = waitForUpdate(groupView).then(() => { + ReactTestUtils.findRenderedDOMComponentWithClass(root, 'mx_GroupView_error'); + }); + + httpBackend.when('GET', '/groups/' + groupIdEncoded + '/summary').respond(500, {}); + httpBackend.when('GET', '/groups/' + groupIdEncoded + '/users').respond(200, { chunk: [] }); + httpBackend.when('GET', '/groups/' + groupIdEncoded + '/invited_users').respond(200, { chunk: [] }); + httpBackend.when('GET', '/groups/' + groupIdEncoded + '/rooms').respond(200, { chunk: [] }); + + httpBackend.flush(undefined, undefined, 0); + return prom; + }); + + it('should show a group avatar, name, id and short description after successful /summary', function() { + const groupView = ReactTestUtils.findRenderedComponentWithType(root, GroupView); + const prom = waitForUpdate(groupView).then(() => { + ReactTestUtils.findRenderedDOMComponentWithClass(root, 'mx_GroupView'); + + const avatar = ReactTestUtils.findRenderedComponentWithType(root, sdk.getComponent('avatars.GroupAvatar')); + const img = ReactTestUtils.findRenderedDOMComponentWithTag(avatar, 'img'); + const avatarImgElement = ReactDOM.findDOMNode(img); + expect(avatarImgElement).toExist(); + expect(avatarImgElement.src).toInclude( + 'https://my.home.server/_matrix/media/v1/thumbnail/' + + 'someavatarurl?width=48&height=48&method=crop', + ); + + const name = ReactTestUtils.findRenderedDOMComponentWithClass(root, 'mx_GroupView_header_name'); + const nameElement = ReactDOM.findDOMNode(name); + expect(nameElement).toExist(); + expect(nameElement.innerText).toInclude('The name of a community'); + expect(nameElement.innerText).toInclude(groupId); + + const shortDesc = ReactTestUtils.findRenderedDOMComponentWithClass(root, 'mx_GroupView_header_shortDesc'); + const shortDescElement = ReactDOM.findDOMNode(shortDesc); + expect(shortDescElement).toExist(); + expect(shortDescElement.innerText).toBe('This is a community'); + }); + + httpBackend.when('GET', '/groups/' + groupIdEncoded + '/summary').respond(200, summaryResponse); + httpBackend.when('GET', '/groups/' + groupIdEncoded + '/users').respond(200, { chunk: [] }); + httpBackend.when('GET', '/groups/' + groupIdEncoded + '/invited_users').respond(200, { chunk: [] }); + httpBackend.when('GET', '/groups/' + groupIdEncoded + '/rooms').respond(200, { chunk: [] }); + + httpBackend.flush(undefined, undefined, 0); + return prom; + }); + + it('should show a simple long description after successful /summary', function() { + const groupView = ReactTestUtils.findRenderedComponentWithType(root, GroupView); + const prom = waitForUpdate(groupView).then(() => { + ReactTestUtils.findRenderedDOMComponentWithClass(root, 'mx_GroupView'); + + const longDesc = ReactTestUtils.findRenderedDOMComponentWithClass(root, 'mx_GroupView_groupDesc'); + const longDescElement = ReactDOM.findDOMNode(longDesc); + expect(longDescElement).toExist(); + expect(longDescElement.innerText).toBe('This is a LONG description.'); + expect(longDescElement.innerHTML).toBe('
This is a LONG description.
'); + }); + + httpBackend.when('GET', '/groups/' + groupIdEncoded + '/summary').respond(200, summaryResponse); + httpBackend.when('GET', '/groups/' + groupIdEncoded + '/users').respond(200, { chunk: [] }); + httpBackend.when('GET', '/groups/' + groupIdEncoded + '/invited_users').respond(200, { chunk: [] }); + httpBackend.when('GET', '/groups/' + groupIdEncoded + '/rooms').respond(200, { chunk: [] }); + + httpBackend.flush(undefined, undefined, 0); + return prom; + }); + + it('should show a placeholder if a long description is not set', function() { + const groupView = ReactTestUtils.findRenderedComponentWithType(root, GroupView); + const prom = waitForUpdate(groupView).then(() => { + const placeholder = ReactTestUtils + .findRenderedDOMComponentWithClass(root, 'mx_GroupView_groupDesc_placeholder'); + const placeholderElement = ReactDOM.findDOMNode(placeholder); + expect(placeholderElement).toExist(); + }); + + httpBackend + .when('GET', '/groups/' + groupIdEncoded + '/summary') + .respond(200, summaryResponseWithNoLongDesc); + httpBackend.when('GET', '/groups/' + groupIdEncoded + '/users').respond(200, { chunk: [] }); + httpBackend.when('GET', '/groups/' + groupIdEncoded + '/invited_users').respond(200, { chunk: [] }); + httpBackend.when('GET', '/groups/' + groupIdEncoded + '/rooms').respond(200, { chunk: [] }); + + httpBackend.flush(undefined, undefined, 0); + return prom; + }); + + it('should show a complicated long description after successful /summary', function() { + const groupView = ReactTestUtils.findRenderedComponentWithType(root, GroupView); + const prom = waitForUpdate(groupView).then(() => { + const longDesc = ReactTestUtils.findRenderedDOMComponentWithClass(root, 'mx_GroupView_groupDesc'); + const longDescElement = ReactDOM.findDOMNode(longDesc); + expect(longDescElement).toExist(); + + expect(longDescElement.innerHTML).toInclude('

This is a more complicated group page

'); + expect(longDescElement.innerHTML).toInclude('

With paragraphs

'); + expect(longDescElement.innerHTML).toInclude('
    '); + expect(longDescElement.innerHTML).toInclude('
  • And lists!
    • '); + + const imgSrc = "https://my.home.server/_matrix/media/v1/thumbnail/someimageurl?width=800&height=600"; + expect(longDescElement.innerHTML).toInclude(''); + }); + + httpBackend + .when('GET', '/groups/' + groupIdEncoded + '/summary') + .respond(200, summaryResponseWithComplicatedLongDesc); + httpBackend.when('GET', '/groups/' + groupIdEncoded + '/users').respond(200, { chunk: [] }); + httpBackend.when('GET', '/groups/' + groupIdEncoded + '/invited_users').respond(200, { chunk: [] }); + httpBackend.when('GET', '/groups/' + groupIdEncoded + '/rooms').respond(200, { chunk: [] }); + + httpBackend.flush(undefined, undefined, 0); + return prom; + }); + + it('should disallow images with non-mxc URLs', function() { + const groupView = ReactTestUtils.findRenderedComponentWithType(root, GroupView); + const prom = waitForUpdate(groupView).then(() => { + const longDesc = ReactTestUtils.findRenderedDOMComponentWithClass(root, 'mx_GroupView_groupDesc'); + const longDescElement = ReactDOM.findDOMNode(longDesc); + expect(longDescElement).toExist(); + + // If this fails, the URL could be in an img `src`, which is what we care about but + // there's no harm in keeping this simple and checking the entire HTML string. + expect(longDescElement.innerHTML).toExclude('evilimageurl'); + }); + + httpBackend + .when('GET', '/groups/' + groupIdEncoded + '/summary') + .respond(200, summaryResponseWithBadImg); + httpBackend.when('GET', '/groups/' + groupIdEncoded + '/users').respond(200, { chunk: [] }); + httpBackend.when('GET', '/groups/' + groupIdEncoded + '/invited_users').respond(200, { chunk: [] }); + httpBackend.when('GET', '/groups/' + groupIdEncoded + '/rooms').respond(200, { chunk: [] }); + + httpBackend.flush(undefined, undefined, 0); + return prom; + }); + + it('should show a RoomDetailList after a successful /summary & /rooms (no rooms returned)', function() { + const groupView = ReactTestUtils.findRenderedComponentWithType(root, GroupView); + const prom = waitForUpdate(groupView).then(() => { + const roomDetailList = ReactTestUtils.findRenderedDOMComponentWithClass(root, 'mx_RoomDetailList'); + const roomDetailListElement = ReactDOM.findDOMNode(roomDetailList); + expect(roomDetailListElement).toExist(); + }); + + httpBackend.when('GET', '/groups/' + groupIdEncoded + '/summary').respond(200, summaryResponse); + httpBackend.when('GET', '/groups/' + groupIdEncoded + '/users').respond(200, { chunk: [] }); + httpBackend.when('GET', '/groups/' + groupIdEncoded + '/invited_users').respond(200, { chunk: [] }); + httpBackend.when('GET', '/groups/' + groupIdEncoded + '/rooms').respond(200, { chunk: [] }); + + httpBackend.flush(undefined, undefined, 0); + return prom; + }); + + it('should show a RoomDetailList after a successful /summary & /rooms (with a single room)', function() { + const groupView = ReactTestUtils.findRenderedComponentWithType(root, GroupView); + const prom = waitForUpdate(groupView).then(() => { + const roomDetailList = ReactTestUtils.findRenderedDOMComponentWithClass(root, 'mx_RoomDetailList'); + const roomDetailListElement = ReactDOM.findDOMNode(roomDetailList); + expect(roomDetailListElement).toExist(); + + const roomDetailListRoomName = ReactTestUtils.findRenderedDOMComponentWithClass( + root, + 'mx_RoomDirectory_name', + ); + const roomDetailListRoomNameElement = ReactDOM.findDOMNode(roomDetailListRoomName); + + expect(roomDetailListRoomNameElement).toExist(); + expect(roomDetailListRoomNameElement.innerText).toEqual('Some room name'); + }); + + httpBackend.when('GET', '/groups/' + groupIdEncoded + '/summary').respond(200, summaryResponse); + httpBackend.when('GET', '/groups/' + groupIdEncoded + '/users').respond(200, { chunk: [] }); + httpBackend.when('GET', '/groups/' + groupIdEncoded + '/invited_users').respond(200, { chunk: [] }); + httpBackend.when('GET', '/groups/' + groupIdEncoded + '/rooms').respond(200, { chunk: [{ + avatar_url: "mxc://someroomavatarurl", + canonical_alias: "#somealias:domain", + guest_can_join: true, + is_public: true, + name: "Some room name", + num_joined_members: 123, + room_id: "!someroomid", + topic: "some topic", + world_readable: true, + }] }); + + httpBackend.flush(undefined, undefined, 0); + return prom; + }); +}); diff --git a/test/test-utils.js b/test/test-utils.js index b593761bd4..d2c685b371 100644 --- a/test/test-utils.js +++ b/test/test-utils.js @@ -92,6 +92,7 @@ export function createTestClient() { content: {}, }); }, + mxcUrlToHttp: (mxc) => 'http://this.is.a.url/', setAccountData: sinon.stub(), sendTyping: sinon.stub().returns(Promise.resolve({})), sendTextMessage: () => Promise.resolve({}), From 7915d97ed70cf5cab3f0b83d807db228aa978f10 Mon Sep 17 00:00:00 2001 From: Luke Barnard Date: Fri, 27 Apr 2018 14:56:48 +0100 Subject: [PATCH 04/11] Also run other tests --- test/components/structures/GroupView-test.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/components/structures/GroupView-test.js b/test/components/structures/GroupView-test.js index 2df0599f89..71df26da46 100644 --- a/test/components/structures/GroupView-test.js +++ b/test/components/structures/GroupView-test.js @@ -52,7 +52,7 @@ function waitForUpdate(inst) { }); } -describe.only('GroupView', function() { +describe('GroupView', function() { let root; let rootElement; let httpBackend; From fed74646b0eb8b6fa5cc55fff92a613810714e64 Mon Sep 17 00:00:00 2001 From: David Baker Date: Fri, 27 Apr 2018 17:49:53 +0100 Subject: [PATCH 05/11] Rewrite to use async / await --- src/Lifecycle.js | 84 +++++++++++++++++++++++------------------------- 1 file changed, 41 insertions(+), 43 deletions(-) diff --git a/src/Lifecycle.js b/src/Lifecycle.js index a22a5aeebd..7378e982ef 100644 --- a/src/Lifecycle.js +++ b/src/Lifecycle.js @@ -65,14 +65,14 @@ import sdk from './index'; * Resolves to `true` if we ended up starting a session, or `false` if we * failed. */ -export function loadSession(opts) { - let enableGuest = opts.enableGuest || false; - const guestHsUrl = opts.guestHsUrl; - const guestIsUrl = opts.guestIsUrl; - const fragmentQueryParams = opts.fragmentQueryParams || {}; - const defaultDeviceDisplayName = opts.defaultDeviceDisplayName; +export async function loadSession(opts) { + try { + let enableGuest = opts.enableGuest || false; + const guestHsUrl = opts.guestHsUrl; + const guestIsUrl = opts.guestIsUrl; + const fragmentQueryParams = opts.fragmentQueryParams || {}; + const defaultDeviceDisplayName = opts.defaultDeviceDisplayName; - return Promise.resolve().then(() => { if (!guestHsUrl) { console.warn("Cannot enable guest access: can't determine HS URL to use"); enableGuest = false; @@ -91,8 +91,7 @@ export function loadSession(opts) { guest: true, }, true).then(() => true); } - return _restoreFromLocalStorage(); - }).then((success) => { + const success = await _restoreFromLocalStorage(); if (success) { return true; } @@ -103,9 +102,9 @@ export function loadSession(opts) { // fall back to login screen return false; - }).catch((e) => { + } catch (e) { return _handleLoadSessionFailure(e); - }); + } } /** @@ -199,40 +198,39 @@ function _registerAsGuest(hsUrl, isUrl, defaultDeviceDisplayName) { // The plan is to gradually move the localStorage access done here into // SessionStore to avoid bugs where the view becomes out-of-sync with // localStorage (e.g. teamToken, isGuest etc.) -function _restoreFromLocalStorage() { - return Promise.resolve().then(() => { - if (!localStorage) { - return Promise.resolve(false); - } - const hsUrl = localStorage.getItem("mx_hs_url"); - const isUrl = localStorage.getItem("mx_is_url") || 'https://matrix.org'; - const accessToken = localStorage.getItem("mx_access_token"); - const userId = localStorage.getItem("mx_user_id"); - const deviceId = localStorage.getItem("mx_device_id"); +async function _restoreFromLocalStorage() { + if (!localStorage) { + return false; + } + const hsUrl = localStorage.getItem("mx_hs_url"); + const isUrl = localStorage.getItem("mx_is_url") || 'https://matrix.org'; + const accessToken = localStorage.getItem("mx_access_token"); + const userId = localStorage.getItem("mx_user_id"); + const deviceId = localStorage.getItem("mx_device_id"); - let isGuest; - if (localStorage.getItem("mx_is_guest") !== null) { - isGuest = localStorage.getItem("mx_is_guest") === "true"; - } else { - // legacy key name - isGuest = localStorage.getItem("matrix-is-guest") === "true"; - } + let isGuest; + if (localStorage.getItem("mx_is_guest") !== null) { + isGuest = localStorage.getItem("mx_is_guest") === "true"; + } else { + // legacy key name + isGuest = localStorage.getItem("matrix-is-guest") === "true"; + } - if (accessToken && userId && hsUrl) { - console.log(`Restoring session for ${userId}`); - return _doSetLoggedIn({ - userId: userId, - deviceId: deviceId, - accessToken: accessToken, - homeserverUrl: hsUrl, - identityServerUrl: isUrl, - guest: isGuest, - }, false).then(() => true); - } else { - console.log("No previous session found."); - return Promise.resolve(false); - } - }); + if (accessToken && userId && hsUrl) { + console.log(`Restoring session for ${userId}`); + await _doSetLoggedIn({ + userId: userId, + deviceId: deviceId, + accessToken: accessToken, + homeserverUrl: hsUrl, + identityServerUrl: isUrl, + guest: isGuest, + }, false); + return true; + } else { + console.log("No previous session found."); + return false; + } } function _handleLoadSessionFailure(e) { From 0c309c88add295798689fbab993d326bd1549cf0 Mon Sep 17 00:00:00 2001 From: David Baker Date: Fri, 27 Apr 2018 17:52:25 +0100 Subject: [PATCH 06/11] Bluebird has no need for your .done() --- src/components/structures/MatrixChat.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/components/structures/MatrixChat.js b/src/components/structures/MatrixChat.js index cdeb99ef53..8df46d2f7c 100644 --- a/src/components/structures/MatrixChat.js +++ b/src/components/structures/MatrixChat.js @@ -360,7 +360,7 @@ export default React.createClass({ // Note we don't catch errors from this: we catch everything within // loadSession as there's logic there to ask the user if they want // to try logging out. - }).done(); + }); }, componentWillUnmount: function() { From d3c368e19fe97d8e429fcb8cef0762638ff1f89a Mon Sep 17 00:00:00 2001 From: David Baker Date: Fri, 27 Apr 2018 17:53:11 +0100 Subject: [PATCH 07/11] typo --- src/components/views/dialogs/BaseDialog.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/components/views/dialogs/BaseDialog.js b/src/components/views/dialogs/BaseDialog.js index 7959e7cb10..54623e6510 100644 --- a/src/components/views/dialogs/BaseDialog.js +++ b/src/components/views/dialogs/BaseDialog.js @@ -36,7 +36,7 @@ export default React.createClass({ propTypes: { // onFinished callback to call when Escape is pressed - // Take a boolean which is true is the dialog was dismissed + // Take a boolean which is true if the dialog was dismissed // with a positive / confirm action or false if it was // cancelled (from BaseDialog, this is always false). onFinished: PropTypes.func.isRequired, From 873993a7cae2ceae8a3207bb9cf6cc10dbf78a38 Mon Sep 17 00:00:00 2001 From: David Baker Date: Fri, 27 Apr 2018 17:56:33 +0100 Subject: [PATCH 08/11] Clarify, hopefully --- src/components/views/dialogs/BaseDialog.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/components/views/dialogs/BaseDialog.js b/src/components/views/dialogs/BaseDialog.js index 54623e6510..51d9180885 100644 --- a/src/components/views/dialogs/BaseDialog.js +++ b/src/components/views/dialogs/BaseDialog.js @@ -38,7 +38,7 @@ export default React.createClass({ // onFinished callback to call when Escape is pressed // Take a boolean which is true if the dialog was dismissed // with a positive / confirm action or false if it was - // cancelled (from BaseDialog, this is always false). + // cancelled (BaseDialog itself only calls this with false). onFinished: PropTypes.func.isRequired, // called when a key is pressed From fc136607f107a24351bd44893a370413453dbaea Mon Sep 17 00:00:00 2001 From: David Baker Date: Fri, 27 Apr 2018 12:38:49 +0100 Subject: [PATCH 09/11] UI fixes in SessionRestoreErrorDialog * Make the 'delete my data' button not the default * Make it red * Give it a confirmation dialog * Remove the 'cancel' button: what does it mean to cancel an error? In this case, it tried again and almost certainly got the same error. * Remove the top-right 'x' and don't cancel on esc for the same reason. * Move 'send bug report' to a button rather than a 'click here' link * Add a 'refresh' button which, even if it's no more likely to work, will at least look like it's doing something (it's mostly so if you don't have a bug report endpoint, there's still a button other than the one that deletes all your data). --- res/css/_common.scss | 1 + src/components/views/dialogs/BaseDialog.js | 20 +++++- .../dialogs/SessionRestoreErrorDialog.js | 70 +++++++++++-------- .../views/elements/DialogButtons.js | 18 +++-- src/i18n/strings/en_EN.json | 13 ++-- 5 files changed, 79 insertions(+), 43 deletions(-) diff --git a/res/css/_common.scss b/res/css/_common.scss index e81c228430..c4cda6821e 100644 --- a/res/css/_common.scss +++ b/res/css/_common.scss @@ -250,6 +250,7 @@ textarea { .mx_Dialog button.danger, .mx_Dialog input[type="submit"].danger { background-color: $warning-color; border: solid 1px $warning-color; + color: $accent-fg-color; } .mx_Dialog button:disabled, .mx_Dialog input[type="submit"]:disabled { diff --git a/src/components/views/dialogs/BaseDialog.js b/src/components/views/dialogs/BaseDialog.js index 51d9180885..2f2d6fe8cb 100644 --- a/src/components/views/dialogs/BaseDialog.js +++ b/src/components/views/dialogs/BaseDialog.js @@ -1,5 +1,6 @@ /* Copyright 2017 Vector Creations Ltd +Copyright 2018 New Vector Ltd Licensed under the Apache License, Version 2.0 (the "License"); you may not use this file except in compliance with the License. @@ -41,6 +42,13 @@ export default React.createClass({ // cancelled (BaseDialog itself only calls this with false). onFinished: PropTypes.func.isRequired, + // Whether the dialog should have a 'close' button that will + // cause the dialog to be cancelled. This should only be set + // to true if there is nothing the app can sensibly do if the + // dialog is cancelled, eg. "We can't restore your session and + // the app cannot work". + hasCancel: PropTypes.bool, + // called when a key is pressed onKeyDown: PropTypes.func, @@ -59,6 +67,12 @@ export default React.createClass({ contentId: React.PropTypes.string, }, + getDefaultProps: function() { + return { + hasCancel: true, + }; + }, + childContextTypes: { matrixClient: PropTypes.instanceOf(MatrixClient), }, @@ -77,7 +91,7 @@ export default React.createClass({ if (this.props.onKeyDown) { this.props.onKeyDown(e); } - if (e.keyCode === KeyCode.ESCAPE) { + if (this.props.hasCancel && e.keyCode === KeyCode.ESCAPE) { e.stopPropagation(); e.preventDefault(); this.props.onFinished(false); @@ -104,11 +118,11 @@ export default React.createClass({ // AT users can skip its presentation. aria-describedby={this.props.contentId} > - - + : null }
    { this.props.title }
    diff --git a/src/components/views/dialogs/SessionRestoreErrorDialog.js b/src/components/views/dialogs/SessionRestoreErrorDialog.js index f101381ebf..2e152df4b0 100644 --- a/src/components/views/dialogs/SessionRestoreErrorDialog.js +++ b/src/components/views/dialogs/SessionRestoreErrorDialog.js @@ -31,63 +31,73 @@ export default React.createClass({ onFinished: PropTypes.func.isRequired, }, - componentDidMount: function() { - if (this.refs.bugreportLink) { - this.refs.bugreportLink.focus(); - } - }, - _sendBugReport: function() { const BugReportDialog = sdk.getComponent("dialogs.BugReportDialog"); Modal.createTrackedDialog('Session Restore Error', 'Send Bug Report Dialog', BugReportDialog, {}); }, - _onContinueClick: function() { - this.props.onFinished(true); + _onClearStorageClick: function() { + const QuestionDialog = sdk.getComponent("dialogs.QuestionDialog"); + Modal.createTrackedDialog('Session Restore Confirm Logout', '', QuestionDialog, { + title: _t("Sign out"), + description: +
    { _t("Log out and remove encryption keys?") }
    , + button: _t("Sign out"), + danger: true, + onFinished: this.props.onFinished, + }); }, - _onCancelClick: function() { - this.props.onFinished(false); + _onRefreshClick: function() { + // Is this likely to help? Probably not, but giving only one button + // that clears your storage seems awful. + window.location.reload(true); }, render: function() { const BaseDialog = sdk.getComponent('views.dialogs.BaseDialog'); const DialogButtons = sdk.getComponent('views.elements.DialogButtons'); - let bugreport; + let dialogButtons; if (SdkConfig.get().bug_report_endpoint_url) { - bugreport = ( -

    - { _t( - "Otherwise, click here to send a bug report.", - {}, - { 'a': (sub) => { sub } }, - ) } -

    - ); + dialogButtons = + + + } else { + dialogButtons = + + } - const shouldFocusContinueButton =!(bugreport==true); return (
    -

    { _t("We encountered an error trying to restore your previous session. If " + - "you continue, you will need to log in again, and encrypted chat " + - "history will be unreadable.") }

    +

    { _t("We encountered an error trying to restore your previous session.") }

    { _t("If you have previously used a more recent version of Riot, your session " + "may be incompatible with this version. Close this window and return " + "to the more recent version.") }

    - { bugreport } +

    { _t("Clearing your browser's storage may fix the problem, but will sign you " + + "out and cause any encrypted chat history to become unreadable.") }

    - + { dialogButtons }     ); }, diff --git a/src/components/views/elements/DialogButtons.js b/src/components/views/elements/DialogButtons.js index 537f906a74..73e47cbde2 100644 --- a/src/components/views/elements/DialogButtons.js +++ b/src/components/views/elements/DialogButtons.js @@ -1,5 +1,6 @@ /* Copyright 2017 Aidan Gauland +Copyright 2018 New Vector Ltd. Licensed under the Apache License, Version 2.0 (the "License"); you may not use this file except in compliance with the License. @@ -14,8 +15,6 @@ See the License for the specific language governing permissions and limitations under the License. */ -"use strict"; - import React from "react"; import PropTypes from "prop-types"; import { _t } from '../../../languageHandler'; @@ -33,12 +32,21 @@ module.exports = React.createClass({ // onClick handler for the primary button. onPrimaryButtonClick: PropTypes.func.isRequired, + // should there be a cancel button? default: true + hasCancel: PropTypes.bool, + // onClick handler for the cancel button. - onCancel: PropTypes.func.isRequired, + onCancel: PropTypes.func, focus: PropTypes.bool, }, + getDefaultProps: function() { + return { + hasCancel: true, + } + }, + _onCancelClick: function() { this.props.onCancel(); }, @@ -57,9 +65,9 @@ module.exports = React.createClass({ { this.props.primaryButton } { this.props.children } - + : null } ); }, diff --git a/src/i18n/strings/en_EN.json b/src/i18n/strings/en_EN.json index 33f18e47a4..3c8dd5b66f 100644 --- a/src/i18n/strings/en_EN.json +++ b/src/i18n/strings/en_EN.json @@ -103,7 +103,6 @@ "You need to be logged in.": "You need to be logged in.", "You need to be able to invite users to do that.": "You need to be able to invite users to do that.", "Unable to create widget.": "Unable to create widget.", - "Popout widget": "Popout widget", "Missing roomId.": "Missing roomId.", "Failed to send request.": "Failed to send request.", "This room is not recognised.": "This room is not recognised.", @@ -655,6 +654,7 @@ "Delete widget": "Delete widget", "Revoke widget access": "Revoke widget access", "Minimize apps": "Minimize apps", + "Popout widget": "Popout widget", "Picture": "Picture", "Edit": "Edit", "Create new room": "Create new room", @@ -807,11 +807,15 @@ "Ignore request": "Ignore request", "Loading device info...": "Loading device info...", "Encryption key request": "Encryption key request", - "Otherwise, click here to send a bug report.": "Otherwise, click here to send a bug report.", + "Sign out": "Sign out", + "Log out and remove encryption keys?": "Log out and remove encryption keys?", + "Send Logs": "Send Logs", + "Clear Storage and Sign Out": "Clear Storage and Sign Out", + "Refresh": "Refresh", "Unable to restore session": "Unable to restore session", - "We encountered an error trying to restore your previous session. If you continue, you will need to log in again, and encrypted chat history will be unreadable.": "We encountered an error trying to restore your previous session. If you continue, you will need to log in again, and encrypted chat history will be unreadable.", + "We encountered an error trying to restore your previous session.": "We encountered an error trying to restore your previous session.", "If you have previously used a more recent version of Riot, your session may be incompatible with this version. Close this window and return to the more recent version.": "If you have previously used a more recent version of Riot, your session may be incompatible with this version. Close this window and return to the more recent version.", - "Continue anyway": "Continue anyway", + "Clearing your browser's storage may fix the problem, but will sign you out and cause any encrypted chat history to become unreadable.": "Clearing your browser's storage may fix the problem, but will sign you out and cause any encrypted chat history to become unreadable.", "Invalid Email Address": "Invalid Email Address", "This doesn't appear to be a valid email address": "This doesn't appear to be a valid email address", "Verification Pending": "Verification Pending", @@ -1015,7 +1019,6 @@ "Status.im theme": "Status.im theme", "Can't load user settings": "Can't load user settings", "Server may be unavailable or overloaded": "Server may be unavailable or overloaded", - "Sign out": "Sign out", "For security, logging out will delete any end-to-end encryption keys from this browser. If you want to be able to decrypt your conversation history from future Riot sessions, please export your room keys for safe-keeping.": "For security, logging out will delete any end-to-end encryption keys from this browser. If you want to be able to decrypt your conversation history from future Riot sessions, please export your room keys for safe-keeping.", "Success": "Success", "Your password was successfully changed. You will not receive push notifications on other devices until you log back in to them": "Your password was successfully changed. You will not receive push notifications on other devices until you log back in to them", From 37cb8abf13008362875f64fd853e5e17d9b83594 Mon Sep 17 00:00:00 2001 From: David Baker Date: Fri, 27 Apr 2018 15:19:08 +0100 Subject: [PATCH 10/11] Fix UX issues with bug report dialog * Make it use BaseDialog / DialogButtons (also gives it has a top-right 'x' & escape to cancel works) * Stop misusing the 'danger' CSS class on the buttons. There is nothing dangerous about submitting logs. * Continued campaign against 'Click here' links. Fixes https://github.com/vector-im/riot-web/issues/6622 --- src/components/structures/UserSettings.js | 4 +- .../views/dialogs/BugReportDialog.js | 42 +++++++------------ .../views/elements/DialogButtons.js | 14 +++++-- src/i18n/strings/en_EN.json | 2 +- 4 files changed, 30 insertions(+), 32 deletions(-) diff --git a/src/components/structures/UserSettings.js b/src/components/structures/UserSettings.js index 7948f4fb5d..44e2f93d63 100644 --- a/src/components/structures/UserSettings.js +++ b/src/components/structures/UserSettings.js @@ -1,7 +1,7 @@ /* Copyright 2015, 2016 OpenMarket Ltd Copyright 2017 Vector Creations Ltd -Copyright 2017 New Vector Ltd +Copyright 2017, 2018 New Vector Ltd Licensed under the Apache License, Version 2.0 (the "License"); you may not use this file except in compliance with the License. @@ -804,7 +804,7 @@ module.exports = React.createClass({ "other users. They do not contain messages.", ) }

    - diff --git a/src/components/views/dialogs/BugReportDialog.js b/src/components/views/dialogs/BugReportDialog.js index 2025b6fc81..83cdb1f07f 100644 --- a/src/components/views/dialogs/BugReportDialog.js +++ b/src/components/views/dialogs/BugReportDialog.js @@ -1,5 +1,6 @@ /* Copyright 2017 OpenMarket Ltd +Copyright 2018 New Vector Ltd Licensed under the Apache License, Version 2.0 (the "License"); you may not use this file except in compliance with the License. @@ -105,6 +106,8 @@ export default class BugReportDialog extends React.Component { render() { const Loader = sdk.getComponent("elements.Spinner"); + const BaseDialog = sdk.getComponent('views.dialogs.BaseDialog'); + const DialogButtons = sdk.getComponent('views.elements.DialogButtons'); let error = null; if (this.state.err) { @@ -113,13 +116,6 @@ export default class BugReportDialog extends React.Component { ; } - let cancelButton = null; - if (!this.state.busy) { - cancelButton = ; - } - let progress = null; if (this.state.busy) { progress = ( @@ -131,11 +127,11 @@ export default class BugReportDialog extends React.Component { } return ( -
    -
    - { _t("Submit debug logs") } -
    -
    + +

    { _t( "Debug logs contain application usage data including your " + @@ -146,7 +142,7 @@ export default class BugReportDialog extends React.Component {

    { _t( - "Click here to create a GitHub issue.", + "Riot bugs are tracked on GitHub: create a GitHub issue.", {}, { a: (sub) => -

    -
    + +     ); } } diff --git a/src/components/views/elements/DialogButtons.js b/src/components/views/elements/DialogButtons.js index 73e47cbde2..c8c90ddc47 100644 --- a/src/components/views/elements/DialogButtons.js +++ b/src/components/views/elements/DialogButtons.js @@ -39,11 +39,14 @@ module.exports = React.createClass({ onCancel: PropTypes.func, focus: PropTypes.bool, + + disabled: PropTypes.bool, }, getDefaultProps: function() { return { hasCancel: true, + disabled: false, } }, @@ -56,18 +59,23 @@ module.exports = React.createClass({ if (this.props.primaryButtonClass) { primaryButtonClassName += " " + this.props.primaryButtonClass; } + let cancelButton; + if (this.props.hasCancel) { + cancelButton = ; + } return (
    { this.props.children } - { this.props.hasCancel ? : null } + { cancelButton }
    ); }, diff --git a/src/i18n/strings/en_EN.json b/src/i18n/strings/en_EN.json index 3c8dd5b66f..daf0ce605d 100644 --- a/src/i18n/strings/en_EN.json +++ b/src/i18n/strings/en_EN.json @@ -745,7 +745,7 @@ "Failed to send logs: ": "Failed to send logs: ", "Submit debug logs": "Submit debug logs", "Debug logs contain application usage data including your username, the IDs or aliases of the rooms or groups you have visited and the usernames of other users. They do not contain messages.": "Debug logs contain application usage data including your username, the IDs or aliases of the rooms or groups you have visited and the usernames of other users. They do not contain messages.", - "    Click here to create a GitHub issue.": "Click here to create a GitHub issue.", + "Riot bugs are tracked on GitHub: create a GitHub issue.": "Riot bugs are tracked on GitHub: create a GitHub issue.", "GitHub issue link:": "GitHub issue link:", "Notes:": "Notes:", "Send logs": "Send logs", From a9b6db3f2ecbd2a9f27d3d2b9f033a95bf303709 Mon Sep 17 00:00:00 2001 From: David Baker Date: Fri, 27 Apr 2018 15:56:28 +0100 Subject: [PATCH 11/11] Lint --- src/components/views/dialogs/SessionRestoreErrorDialog.js | 4 ++-- src/components/views/elements/DialogButtons.js | 2 +- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/src/components/views/dialogs/SessionRestoreErrorDialog.js b/src/components/views/dialogs/SessionRestoreErrorDialog.js index 2e152df4b0..779e54cfba 100644 --- a/src/components/views/dialogs/SessionRestoreErrorDialog.js +++ b/src/components/views/dialogs/SessionRestoreErrorDialog.js @@ -68,7 +68,7 @@ export default React.createClass({ - + ; } else { dialogButtons = { _t("Clear Storage and Sign Out") } - + ; } return ( diff --git a/src/components/views/elements/DialogButtons.js b/src/components/views/elements/DialogButtons.js index c8c90ddc47..267127568d 100644 --- a/src/components/views/elements/DialogButtons.js +++ b/src/components/views/elements/DialogButtons.js @@ -47,7 +47,7 @@ module.exports = React.createClass({ return { hasCancel: true, disabled: false, - } + }; }, _onCancelClick: function() {