Skip to content

Commit

Permalink
Add logic to load focused group members
Browse files Browse the repository at this point in the history
  • Loading branch information
acelaya committed Jan 20, 2025
1 parent f2f720d commit 5b4b46c
Show file tree
Hide file tree
Showing 8 changed files with 268 additions and 6 deletions.
23 changes: 23 additions & 0 deletions src/sidebar/services/api.ts
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ import type {
RouteMap,
RouteMetadata,
Profile,
GroupMembers,
} from '../../types/api';
import { stripInternalProperties } from '../helpers/strip-internal-properties';
import type { SidebarStore } from '../store';
Expand Down Expand Up @@ -218,6 +219,17 @@ export class APIService {
member: {
delete: APICall<{ pubid: string; userid: string }>;
};
members: {
read: APICall<
{
pubid: string;
'page[number]'?: number;
'page[size]'?: number;
},
void,
GroupMembers
>;
};
read: APICall<{ id: string; expand: string[] }, void, Group>;
};
groups: {
Expand Down Expand Up @@ -287,6 +299,17 @@ export class APIService {
userid: string;
}>,
},
members: {
read: apiCall('group.members.read') as APICall<
{
pubid: string;
'page[number]'?: number;
'page[size]'?: number;
},
void,
GroupMembers
>,
},
read: apiCall('group.read') as APICall<
{ id: string; expand: string[] },
void,
Expand Down
61 changes: 60 additions & 1 deletion src/sidebar/services/groups.ts
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@ import shallowEqual from 'shallowequal';

// @ts-ignore - TS doesn't know about SVG files.
import { default as logo } from '../../images/icons/logo.svg';
import type { Group } from '../../types/api';
import type { Group, GroupMember, GroupMembers } from '../../types/api';
import type { SidebarSettings } from '../../types/config';
import type { Service } from '../../types/config';
import { serviceConfig } from '../config/service-config';
Expand All @@ -23,6 +23,8 @@ const DEFAULT_ORGANIZATION = {
logo: 'data:image/svg+xml;utf8,' + encodeURIComponent(logo),
};

const MEMBERS_PAGE_SIZE = 100;

/**
* For any group that does not have an associated organization, populate with
* the default Hypothesis organization.
Expand Down Expand Up @@ -440,6 +442,8 @@ export class GroupsService {
* Update the focused group. Update the store, then check to see if
* there are any new (unsaved) annotations—if so, update those annotations
* such that they are associated with the newly-focused group.
*
* Additionally, fetch focused group members and load them into the store.
*/
focus(groupId: string) {
const prevGroupId = this._store.focusedGroupId();
Expand All @@ -464,6 +468,15 @@ export class GroupsService {
// Persist this group as the last focused group default
this._store.setDefault('focusedGroup', newGroupId);
}

if (
// For now, group members are going to be used for at-mentions only, so we
// can skip loading them if the feature is not enabled
this._store.isFeatureEnabled('at_mentions') &&
this._store.getFocusedGroupMembers() === null
) {
this._loadFocusedGroupMembers(groupId).catch(e => console.error(e));
}
}

/**
Expand All @@ -478,4 +491,50 @@ export class GroupsService {
userid: 'me',
});
}

/**
* Fetch members for focused group from the API and load them into the store.
*/
private async _loadFocusedGroupMembers(groupId: string): Promise<void> {
// Open groups do not have members, so we can save the API calls
const members =
this._store.focusedGroup()?.type === 'open'
? []
: await this._fetchAllMembers(groupId);
this._store.loadFocusedGroupMembers(members);
}

private async _fetchAllMembers(groupId: string): Promise<GroupMember[]> {
// Fetch first page of members, to determine how many more pages there are
const firstPage = await this._fetchMembers(groupId);
const remainingMembers = firstPage.meta.page.total - MEMBERS_PAGE_SIZE;
let members = firstPage.data;

if (remainingMembers <= 0) {
return members;
}

const pages = Math.min(
Math.ceil(remainingMembers / MEMBERS_PAGE_SIZE) + 1,
// Do not try to load more than 10 pages, to avoid long loading times and
// hitting the server too much
10,
);
// Start at 1 to skip loading first page
for (let i = 1; i < pages; i++) {
// TODO Consider parallelizing requests
const groupMembers = await this._fetchMembers(groupId, i + 1);
members = members.concat(groupMembers.data);
}

return members;
}

private _fetchMembers(groupId: string, page = 1): Promise<GroupMembers> {
return this._api.group.members.read({
pubid: groupId,
'page[number]': page,
'page[size]': MEMBERS_PAGE_SIZE,
});
}
}
7 changes: 7 additions & 0 deletions src/sidebar/services/test/api-index.json
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,13 @@
"desc": "Remove the current user from a group."
}
},
"members": {
"read": {
"url": "https://example.com/api/groups/:pubid/members",
"method": "GET",
"desc": "Fetch a list of all members of a group"
}
},
"read": {
"url": "https://example.com/api/groups/:id",
"method": "GET",
Expand Down
20 changes: 20 additions & 0 deletions src/sidebar/services/test/api-test.js
Original file line number Diff line number Diff line change
Expand Up @@ -124,6 +124,26 @@ describe('APIService', () => {
return api.group.member.delete({ pubid: 'an-id', userid: 'me' });
});

it('gets group members', () => {
const groupMembers = {
meta: {
page: { total: 0 },
},
data: [],
};
expectCall(
'get',
`groups/an-id/members?${encodeURIComponent('page[number]')}=1&${encodeURIComponent('page[size]')}=100`,
200,
groupMembers,
);
return api.group.members.read({
pubid: 'an-id',
'page[number]': 1,
'page[size]': 100,
});
});

it('gets a group by provided group id', () => {
const group = { id: 'group-id', name: 'Group' };
expectCall('get', 'groups/group-id', 200, group);
Expand Down
59 changes: 56 additions & 3 deletions src/sidebar/services/test/groups-test.js
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
import { delay, waitFor } from '@hypothesis/frontend-testing';
import sinon from 'sinon';

import { fakeReduxStore } from '../../test/fake-redux-store';
import { GroupsService, $imports } from '../groups';
Expand Down Expand Up @@ -78,9 +79,7 @@ describe('GroupsService', () => {
allGroups() {
return this.getState().groups.groups;
},
focusedGroup() {
return this.getState().groups.focusedGroup;
},
focusedGroup: sinon.stub().returns(null),
defaultContentFrame() {
return this.getState().frames[0];
},
Expand All @@ -89,6 +88,9 @@ describe('GroupsService', () => {
clearDirectLinkedGroupFetchFailed: sinon.stub(),
profile: sinon.stub().returns({ userid: null }),
route: sinon.stub().returns('sidebar'),
isFeatureEnabled: sinon.stub().returns(false),
getFocusedGroupMembers: sinon.stub().returns(null),
loadFocusedGroupMembers: sinon.stub(),
},
);
fakeApi = {
Expand All @@ -100,6 +102,9 @@ describe('GroupsService', () => {
member: {
delete: sinon.stub().returns(Promise.resolve()),
},
members: {
read: sinon.stub().resolves({}),
},
read: sinon.stub().returns(Promise.resolve(new Error('404 Error'))),
},
groups: {
Expand Down Expand Up @@ -205,6 +210,54 @@ describe('GroupsService', () => {

assert.notCalled(fakeStore.setDefault);
});

context('when at-mentions are enabled', () => {
beforeEach(() => {
fakeStore.isFeatureEnabled.withArgs('at_mentions').returns(true);
});

it('does not try to load group members when loaded group is open', async () => {
fakeStore.focusedGroup.returns({ type: 'open' });

const svc = createService();
svc.focus('newgroup');

// Wait, as loadFocusedGroupMembers gets called asynchronously
await waitFor(() => fakeStore.loadFocusedGroupMembers.calledWith([]));
assert.notCalled(fakeApi.group.members.read);
});

[
{ totalMembers: 48, expectedApiCalls: 1 },
{ totalMembers: 100, expectedApiCalls: 1 },
{ totalMembers: 125, expectedApiCalls: 2 },
{ totalMembers: 236, expectedApiCalls: 3 },
{ totalMembers: 650, expectedApiCalls: 7 },
{ totalMembers: 936, expectedApiCalls: 10 },

// We'll never load more than 10 pages
{ totalMembers: 1_000, expectedApiCalls: 10 },
{ totalMembers: 1_200, expectedApiCalls: 10 },
{ totalMembers: 10_000, expectedApiCalls: 10 },
].forEach(({ totalMembers, expectedApiCalls }) => {
it('calls members API as many times as needed, until all members are loaded', async () => {
fakeStore.focusedGroup.returns({ type: 'restricted' });
fakeApi.group.members.read.resolves({
meta: {
page: { total: totalMembers },
},
data: [],
});

const svc = createService();
svc.focus('newgroup');

// Wait, as loadFocusedGroupMembers gets called asynchronously
await waitFor(() => fakeStore.loadFocusedGroupMembers.called);
assert.callCount(fakeApi.group.members.read, expectedApiCalls);
});
});
});
});

describe('#load', () => {
Expand Down
40 changes: 38 additions & 2 deletions src/sidebar/store/modules/groups.ts
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
import { createSelector } from 'reselect';

import type { Group } from '../../../types/api';
import type { Group, GroupMember } from '../../../types/api';
import { createStoreModule, makeAction } from '../create-store';
import { sessionModule } from './session';
import type { State as SessionState } from './session';
Expand All @@ -18,12 +18,16 @@ export type State = {
groups: Group[];
/** ID of currently selected group. */
focusedGroupId: string | null;

/** Members of currently selected group */
focusedGroupMembers: GroupMember[] | null;
};

const initialState: State = {
filteredGroupIds: null,
groups: [],
focusedGroupId: null,
focusedGroupMembers: null,
};

const reducers = {
Expand Down Expand Up @@ -62,7 +66,7 @@ const reducers = {
);
return {};
}
return { focusedGroupId: action.id };
return { focusedGroupId: action.id, focusedGroupMembers: null };
},

LOAD_GROUPS(state: State, action: { groups: Group[] }) {
Expand All @@ -87,11 +91,24 @@ const reducers = {
};
},

LOAD_FOCUSED_GROUP_MEMBERS(
state: State,
action: { focusedGroupMembers: GroupMember[] },
) {
if (!state.focusedGroupId) {
throw new Error('A group needs to be focused before loading its members');
}

const { focusedGroupMembers } = action;
return { focusedGroupMembers };
},

CLEAR_GROUPS() {
return {
filteredGroupIds: null,
focusedGroupId: null,
groups: [],
focusedGroupMembers: null,
};
},
};
Expand Down Expand Up @@ -121,6 +138,23 @@ function loadGroups(groups: Group[]) {
return makeAction(reducers, 'LOAD_GROUPS', { groups });
}

/**
* Update focused group members.
*/
function loadFocusedGroupMembers(focusedGroupMembers: GroupMember[]) {
return makeAction(reducers, 'LOAD_FOCUSED_GROUP_MEMBERS', {
focusedGroupMembers,
});
}

/**
* Return list of members for focused group.
* Null is returned if members are being loaded or a group is not focused.
*/
function getFocusedGroupMembers(state: State): GroupMember[] | null {
return state.focusedGroupMembers;
}

/**
* Return the currently focused group.
*/
Expand Down Expand Up @@ -229,6 +263,7 @@ export const groupsModule = createStoreModule(initialState, {
filterGroups,
focusGroup,
loadGroups,
loadFocusedGroupMembers,
clearGroups,
},
selectors: {
Expand All @@ -237,6 +272,7 @@ export const groupsModule = createStoreModule(initialState, {
filteredGroupIds,
focusedGroup,
focusedGroupId,
getFocusedGroupMembers,
getFeaturedGroups,
getGroup,
getInScopeGroups,
Expand Down
Loading

0 comments on commit 5b4b46c

Please sign in to comment.