From 5fc25fd6baecaf61f55c03bd3320e18021247b31 Mon Sep 17 00:00:00 2001 From: "J. Ryan Stinnett" Date: Mon, 3 Dec 2018 20:38:29 -0600 Subject: [PATCH 1/2] Only mark group as failed to load for summary Currently, any error in the `GroupStore`s several requests can cause the whole `GroupView` component to hide and be mark the group as failed to load. Since it is known that group members may fail to load in some cases, let's only show failed to load for the whole group when the summary fails. This also strengthens the `GroupView` test by ensuring we wait for multiple updates for checking results. Signed-off-by: J. Ryan Stinnett --- src/components/structures/GroupView.js | 17 ++++----- src/stores/GroupStore.js | 6 +--- test/components/structures/GroupView-test.js | 37 +++++++++++++++----- test/test-utils.js | 15 +++++--- 4 files changed, 50 insertions(+), 25 deletions(-) diff --git a/src/components/structures/GroupView.js b/src/components/structures/GroupView.js index 2c287c1b60..e2fd15aa89 100644 --- a/src/components/structures/GroupView.js +++ b/src/components/structures/GroupView.js @@ -470,7 +470,7 @@ export default React.createClass({ GroupStore.registerListener(groupId, this.onGroupStoreUpdated.bind(this, firstInit)); let willDoOnboarding = false; // XXX: This should be more fluxy - let's get the error from GroupStore .getError or something - GroupStore.on('error', (err, errorGroupId) => { + GroupStore.on('error', (err, errorGroupId, stateKey) => { if (this._unmounted || groupId !== errorGroupId) return; if (err.errcode === 'M_GUEST_ACCESS_FORBIDDEN' && !willDoOnboarding) { dis.dispatch({ @@ -483,11 +483,13 @@ export default React.createClass({ dis.dispatch({action: 'require_registration'}); willDoOnboarding = true; } - this.setState({ - summary: null, - error: err, - editing: false, - }); + if (stateKey === GroupStore.STATE_KEY.Summary) { + this.setState({ + summary: null, + error: err, + editing: false, + }); + } }); }, @@ -511,7 +513,6 @@ export default React.createClass({ isUserMember: GroupStore.getGroupMembers(this.props.groupId).some( (m) => m.userId === this._matrixClient.credentials.userId, ), - error: null, }); // XXX: This might not work but this.props.groupIsNew unused anyway if (this.props.groupIsNew && firstInit) { @@ -1157,7 +1158,7 @@ export default React.createClass({ if (this.state.summaryLoading && this.state.error === null || this.state.saving) { return ; - } else if (this.state.summary) { + } else if (this.state.summary && !this.state.error) { const summary = this.state.summary; let avatarNode; diff --git a/src/stores/GroupStore.js b/src/stores/GroupStore.js index bc2be37f51..4ac1e42e2e 100644 --- a/src/stores/GroupStore.js +++ b/src/stores/GroupStore.js @@ -122,10 +122,6 @@ class GroupStore extends EventEmitter { ); }, }; - - this.on('error', (err, groupId) => { - console.error(`GroupStore encountered error whilst fetching data for ${groupId}`, err); - }); } _fetchResource(stateKey, groupId) { @@ -148,7 +144,7 @@ class GroupStore extends EventEmitter { } console.error(`Failed to get resource ${stateKey} for ${groupId}`, err); - this.emit('error', err, groupId); + this.emit('error', err, groupId, stateKey); }).finally(() => { // Indicate finished request, allow for future fetches delete this._fetchResourcePromise[stateKey][groupId]; diff --git a/test/components/structures/GroupView-test.js b/test/components/structures/GroupView-test.js index 3b3510f26e..89632dcc48 100644 --- a/test/components/structures/GroupView-test.js +++ b/test/components/structures/GroupView-test.js @@ -164,7 +164,7 @@ describe('GroupView', function() { it('should indicate failure after failed /summary', function() { const groupView = ReactTestUtils.findRenderedComponentWithType(root, GroupView); - const prom = waitForUpdate(groupView).then(() => { + const prom = waitForUpdate(groupView, 4).then(() => { ReactTestUtils.findRenderedDOMComponentWithClass(root, 'mx_GroupView_error'); }); @@ -179,7 +179,7 @@ describe('GroupView', function() { 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(() => { + const prom = waitForUpdate(groupView, 4).then(() => { ReactTestUtils.findRenderedDOMComponentWithClass(root, 'mx_GroupView'); const avatar = ReactTestUtils.findRenderedComponentWithType(root, sdk.getComponent('avatars.GroupAvatar')); @@ -214,7 +214,7 @@ describe('GroupView', function() { it('should show a simple long description after successful /summary', function() { const groupView = ReactTestUtils.findRenderedComponentWithType(root, GroupView); - const prom = waitForUpdate(groupView).then(() => { + const prom = waitForUpdate(groupView, 4).then(() => { ReactTestUtils.findRenderedDOMComponentWithClass(root, 'mx_GroupView'); const longDesc = ReactTestUtils.findRenderedDOMComponentWithClass(root, 'mx_GroupView_groupDesc'); @@ -235,7 +235,7 @@ describe('GroupView', function() { 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 prom = waitForUpdate(groupView, 4).then(() => { const placeholder = ReactTestUtils .findRenderedDOMComponentWithClass(root, 'mx_GroupView_groupDesc_placeholder'); const placeholderElement = ReactDOM.findDOMNode(placeholder); @@ -255,7 +255,7 @@ describe('GroupView', function() { it('should show a complicated long description after successful /summary', function() { const groupView = ReactTestUtils.findRenderedComponentWithType(root, GroupView); - const prom = waitForUpdate(groupView).then(() => { + const prom = waitForUpdate(groupView, 4).then(() => { const longDesc = ReactTestUtils.findRenderedDOMComponentWithClass(root, 'mx_GroupView_groupDesc'); const longDescElement = ReactDOM.findDOMNode(longDesc); expect(longDescElement).toExist(); @@ -282,7 +282,7 @@ describe('GroupView', function() { it('should disallow images with non-mxc URLs', function() { const groupView = ReactTestUtils.findRenderedComponentWithType(root, GroupView); - const prom = waitForUpdate(groupView).then(() => { + const prom = waitForUpdate(groupView, 4).then(() => { const longDesc = ReactTestUtils.findRenderedDOMComponentWithClass(root, 'mx_GroupView_groupDesc'); const longDescElement = ReactDOM.findDOMNode(longDesc); expect(longDescElement).toExist(); @@ -305,7 +305,7 @@ describe('GroupView', function() { 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 prom = waitForUpdate(groupView, 4).then(() => { const roomDetailList = ReactTestUtils.findRenderedDOMComponentWithClass(root, 'mx_RoomDetailList'); const roomDetailListElement = ReactDOM.findDOMNode(roomDetailList); expect(roomDetailListElement).toExist(); @@ -322,7 +322,7 @@ describe('GroupView', function() { 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 prom = waitForUpdate(groupView, 4).then(() => { const roomDetailList = ReactTestUtils.findRenderedDOMComponentWithClass(root, 'mx_RoomDetailList'); const roomDetailListElement = ReactDOM.findDOMNode(roomDetailList); expect(roomDetailListElement).toExist(); @@ -355,4 +355,25 @@ describe('GroupView', function() { httpBackend.flush(undefined, undefined, 0); return prom; }); + + it('should show a summary even if /users fails', function() { + const groupView = ReactTestUtils.findRenderedComponentWithType(root, GroupView); + + // Only wait for 3 updates in this test since we don't change state for + // the /users error case. + const prom = waitForUpdate(groupView, 3).then(() => { + 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(500, {}); + 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; + }); }); diff --git a/test/test-utils.js b/test/test-utils.js index bc4d29210e..d5bcd9397a 100644 --- a/test/test-utils.js +++ b/test/test-utils.js @@ -310,19 +310,26 @@ export function wrapInMatrixClientContext(WrappedComponent) { /** * Call fn before calling componentDidUpdate on a react component instance, inst. * @param {React.Component} inst an instance of a React component. + * @param {integer} updates Number of updates to wait for. (Defaults to 1.) * @returns {Promise} promise that resolves when componentDidUpdate is called on * given component instance. */ -export function waitForUpdate(inst) { +export function waitForUpdate(inst, updates = 1) { return new Promise((resolve, reject) => { const cdu = inst.componentDidUpdate; + console.log(`Waiting for ${updates} update(s)`); + inst.componentDidUpdate = (prevProps, prevState, snapshot) => { - resolve(); + updates--; + console.log(`Got update, ${updates} remaining`); + + if (updates == 0) { + inst.componentDidUpdate = cdu; + resolve(); + } if (cdu) cdu(prevProps, prevState, snapshot); - - inst.componentDidUpdate = cdu; }; }); } From 22ff76e6b73e400f6966da0c59a1d1039164d60e Mon Sep 17 00:00:00 2001 From: "J. Ryan Stinnett" Date: Tue, 4 Dec 2018 16:42:50 -0600 Subject: [PATCH 2/2] Add error to UI when group member list fails to load Signed-off-by: J. Ryan Stinnett --- .../views/groups/GroupMemberList.js | 40 ++++- src/i18n/strings/en_EN.json | 1 + src/i18n/strings/en_US.json | 2 + .../views/groups/GroupMemberList-test.js | 149 ++++++++++++++++++ 4 files changed, 187 insertions(+), 5 deletions(-) create mode 100644 test/components/views/groups/GroupMemberList-test.js diff --git a/src/components/views/groups/GroupMemberList.js b/src/components/views/groups/GroupMemberList.js index 38c679a5b5..9b96fa33a9 100644 --- a/src/components/views/groups/GroupMemberList.js +++ b/src/components/views/groups/GroupMemberList.js @@ -32,7 +32,9 @@ export default React.createClass({ getInitialState: function() { return { members: null, + membersError: null, invitedMembers: null, + invitedMembersError: null, truncateAt: INITIAL_LOAD_NUM_MEMBERS, }; }, @@ -50,6 +52,19 @@ export default React.createClass({ GroupStore.registerListener(groupId, () => { this._fetchMembers(); }); + GroupStore.on('error', (err, errorGroupId, stateKey) => { + if (this._unmounted || groupId !== errorGroupId) return; + if (stateKey === GroupStore.STATE_KEY.GroupMembers) { + this.setState({ + membersError: err, + }); + } + if (stateKey === GroupStore.STATE_KEY.GroupInvitedMembers) { + this.setState({ + invitedMembersError: err, + }); + } + }); }, _fetchMembers: function() { @@ -83,7 +98,11 @@ export default React.createClass({ this.setState({ searchQuery: ev.target.value }); }, - makeGroupMemberTiles: function(query, memberList) { + makeGroupMemberTiles: function(query, memberList, memberListError) { + if (memberListError) { + return
{ _t("Failed to load group members") }
; + } + const GroupMemberTile = sdk.getComponent("groups.GroupMemberTile"); const TruncatedList = sdk.getComponent("elements.TruncatedList"); query = (query || "").toLowerCase(); @@ -153,15 +172,26 @@ export default React.createClass({ ); const joined = this.state.members ?
- { this.makeGroupMemberTiles(this.state.searchQuery, this.state.members) } + { + this.makeGroupMemberTiles( + this.state.searchQuery, + this.state.members, + this.state.membersError, + ) + }
:
; const invited = (this.state.invitedMembers && this.state.invitedMembers.length > 0) ?
-

{ _t("Invited") }

- { this.makeGroupMemberTiles(this.state.searchQuery, this.state.invitedMembers) } +

{_t("Invited")}

+ { + this.makeGroupMemberTiles( + this.state.searchQuery, + this.state.invitedMembers, + this.state.invitedMembersError, + ) + }
:
; - return (
{ inputBox } diff --git a/src/i18n/strings/en_EN.json b/src/i18n/strings/en_EN.json index f9980f5645..8e25f96b64 100644 --- a/src/i18n/strings/en_EN.json +++ b/src/i18n/strings/en_EN.json @@ -1107,6 +1107,7 @@ "Community %(groupId)s not found": "Community %(groupId)s not found", "This Home server does not support communities": "This Home server does not support communities", "Failed to load %(groupId)s": "Failed to load %(groupId)s", + "Failed to load group members": "Failed to load group members", "Couldn't load home page": "Couldn't load home page", "You are currently using Riot anonymously as a guest.": "You are currently using Riot anonymously as a guest.", "If you would like to create a Matrix account you can register now.": "If you would like to create a Matrix account you can register now.", diff --git a/src/i18n/strings/en_US.json b/src/i18n/strings/en_US.json index aa7140aaa8..b96a49eac7 100644 --- a/src/i18n/strings/en_US.json +++ b/src/i18n/strings/en_US.json @@ -134,6 +134,8 @@ "Failed to join room": "Failed to join room", "Failed to kick": "Failed to kick", "Failed to leave room": "Failed to leave room", + "Failed to load %(groupId)s": "Failed to load %(groupId)s", + "Failed to load group members": "Failed to load group members", "Failed to load timeline position": "Failed to load timeline position", "Failed to mute user": "Failed to mute user", "Failed to reject invite": "Failed to reject invite", diff --git a/test/components/views/groups/GroupMemberList-test.js b/test/components/views/groups/GroupMemberList-test.js new file mode 100644 index 0000000000..d71d0377d7 --- /dev/null +++ b/test/components/views/groups/GroupMemberList-test.js @@ -0,0 +1,149 @@ +/* +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 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 { waitForUpdate } = TestUtils; + +const GroupMemberList = sdk.getComponent("views.groups.GroupMemberList"); +const WrappedGroupMemberList = TestUtils.wrapInMatrixClientContext(GroupMemberList); + +describe("GroupMemberList", function() { + let root; + let rootElement; + let httpBackend; + let summaryResponse; + 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, + }; + + // Users response fields + const usersResponse = { + chunk: [ + { + user_id: "@test:matrix.org", + displayname: "Test", + avatar_url: "mxc://matrix.org/oUxxDyzQOHdVDMxgwFzyCWEe", + is_public: true, + is_privileged: true, + attestation: {}, + }, + ], + }; + + 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, + }; + + 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 group member list after successful /users", function() { + const groupMemberList = ReactTestUtils.findRenderedComponentWithType(root, GroupMemberList); + const prom = waitForUpdate(groupMemberList, 4).then(() => { + ReactTestUtils.findRenderedDOMComponentWithClass(root, "mx_MemberList"); + + const memberList = ReactTestUtils.findRenderedDOMComponentWithClass(root, "mx_MemberList_joined"); + const memberListElement = ReactDOM.findDOMNode(memberList); + expect(memberListElement).toExist(); + expect(memberListElement.innerText).toBe("Test"); + }); + + httpBackend.when("GET", "/groups/" + groupIdEncoded + "/summary").respond(200, summaryResponse); + httpBackend.when("GET", "/groups/" + groupIdEncoded + "/users").respond(200, usersResponse); + 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 error message after failed /users", function() { + const groupMemberList = ReactTestUtils.findRenderedComponentWithType(root, GroupMemberList); + const prom = waitForUpdate(groupMemberList, 4).then(() => { + ReactTestUtils.findRenderedDOMComponentWithClass(root, "mx_MemberList"); + + const memberList = ReactTestUtils.findRenderedDOMComponentWithClass(root, "mx_MemberList_joined"); + const memberListElement = ReactDOM.findDOMNode(memberList); + expect(memberListElement).toExist(); + expect(memberListElement.innerText).toBe("Failed to load group members"); + }); + + httpBackend.when("GET", "/groups/" + groupIdEncoded + "/summary").respond(200, summaryResponse); + httpBackend.when("GET", "/groups/" + groupIdEncoded + "/users").respond(500, {}); + 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; + }); +});