Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Allow group summary to load when /users fails #2326

Merged
merged 2 commits into from Dec 5, 2018
Merged

Allow group summary to load when /users fails #2326

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
5fc25fd
Only mark group as failed to load for summary
jryans Dec 4, 2018
22ff76e
Add error to UI when group member list fails to load
jryans Dec 4, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
17 changes: 9 additions & 8 deletions src/components/structures/GroupView.js
View file
Open in desktop
Expand Up @@ -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({
Expand All @@ -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,
});
}
});
},

Expand All @@ -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) {
Expand Down Expand Up @@ -1157,7 +1158,7 @@ export default React.createClass({

if (this.state.summaryLoading && this.state.error === null || this.state.saving) {
return <Spinner />;
} else if (this.state.summary) {
} else if (this.state.summary && !this.state.error) {
const summary = this.state.summary;

let avatarNode;
Expand Down
40 changes: 35 additions & 5 deletions src/components/views/groups/GroupMemberList.js
View file
Open in desktop
Expand Up @@ -32,7 +32,9 @@ export default React.createClass({
getInitialState: function() {
return {
members: null,
membersError: null,
invitedMembers: null,
invitedMembersError: null,
truncateAt: INITIAL_LOAD_NUM_MEMBERS,
};
},
Expand All @@ -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() {
Expand Down Expand Up @@ -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 <div className="warning">{ _t("Failed to load group members") }</div>;
}

const GroupMemberTile = sdk.getComponent("groups.GroupMemberTile");
const TruncatedList = sdk.getComponent("elements.TruncatedList");
query = (query || "").toLowerCase();
Expand Down Expand Up @@ -153,15 +172,26 @@ export default React.createClass({
);

const joined = this.state.members ? <div className="mx_MemberList_joined">
{ this.makeGroupMemberTiles(this.state.searchQuery, this.state.members) }
{
this.makeGroupMemberTiles(
this.state.searchQuery,
this.state.members,
this.state.membersError,
)
}
</div> : <div />;

const invited = (this.state.invitedMembers && this.state.invitedMembers.length > 0) ?
<div className="mx_MemberList_invited">
<h2>{ _t("Invited") }</h2>
{ this.makeGroupMemberTiles(this.state.searchQuery, this.state.invitedMembers) }
<h2>{_t("Invited")}</h2>
{
this.makeGroupMemberTiles(
this.state.searchQuery,
this.state.invitedMembers,
this.state.invitedMembersError,
)
}
</div> : <div />;

return (
<div className="mx_MemberList">
{ inputBox }
Expand Down
1 change: 1 addition & 0 deletions src/i18n/strings/en_EN.json
View file
Open in desktop
Expand Up @@ -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 <a>register</a> now.": "If you would like to create a Matrix account you can <a>register</a> now.",
Expand Down
2 changes: 2 additions & 0 deletions src/i18n/strings/en_US.json
View file
Open in desktop
Expand Up @@ -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",
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ftr, you don't need to add entries to this file: en_US falls back to en_EN automatically (even if there is a US english translation, it's generally better to add it in weblate otherwise you'll get conflicts in weblate).

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah okay, thanks!

"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",
Expand Down
6 changes: 1 addition & 5 deletions src/stores/GroupStore.js
View file
Open in desktop
Expand Up @@ -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) {
Expand All @@ -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];
Expand Down
37 changes: 29 additions & 8 deletions test/components/structures/GroupView-test.js
View file
Open in desktop
Expand Up @@ -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');
});

Expand All @@ -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'));
Expand Down Expand Up @@ -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');
Expand All @@ -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);
Expand All @@ -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();
Expand All @@ -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();
Expand All @@ -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();
Expand All @@ -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();
Expand Down Expand Up @@ -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;
});
});