From 5b4b46cbe7b70d92897243b776294b224f56708f Mon Sep 17 00:00:00 2001 From: Alejandro Celaya Date: Wed, 15 Jan 2025 16:00:14 +0100 Subject: [PATCH] Add logic to load focused group members --- src/sidebar/services/api.ts | 23 +++++++ src/sidebar/services/groups.ts | 61 ++++++++++++++++++- src/sidebar/services/test/api-index.json | 7 +++ src/sidebar/services/test/api-test.js | 20 ++++++ src/sidebar/services/test/groups-test.js | 59 +++++++++++++++++- src/sidebar/store/modules/groups.ts | 40 +++++++++++- src/sidebar/store/modules/test/groups-test.js | 44 +++++++++++++ src/types/api.ts | 20 ++++++ 8 files changed, 268 insertions(+), 6 deletions(-) diff --git a/src/sidebar/services/api.ts b/src/sidebar/services/api.ts index 7d3cc97bd0d..59a049f2892 100644 --- a/src/sidebar/services/api.ts +++ b/src/sidebar/services/api.ts @@ -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'; @@ -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: { @@ -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, diff --git a/src/sidebar/services/groups.ts b/src/sidebar/services/groups.ts index bedfb667b55..fdfb13c400d 100644 --- a/src/sidebar/services/groups.ts +++ b/src/sidebar/services/groups.ts @@ -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'; @@ -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. @@ -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(); @@ -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)); + } } /** @@ -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 { + // 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 { + // 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 { + return this._api.group.members.read({ + pubid: groupId, + 'page[number]': page, + 'page[size]': MEMBERS_PAGE_SIZE, + }); + } } diff --git a/src/sidebar/services/test/api-index.json b/src/sidebar/services/test/api-index.json index 5ee05011be7..ad642817c38 100644 --- a/src/sidebar/services/test/api-index.json +++ b/src/sidebar/services/test/api-index.json @@ -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", diff --git a/src/sidebar/services/test/api-test.js b/src/sidebar/services/test/api-test.js index 06b47208cff..6e26e3432ec 100644 --- a/src/sidebar/services/test/api-test.js +++ b/src/sidebar/services/test/api-test.js @@ -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); diff --git a/src/sidebar/services/test/groups-test.js b/src/sidebar/services/test/groups-test.js index 87641f0f1cd..1539d5fbf99 100644 --- a/src/sidebar/services/test/groups-test.js +++ b/src/sidebar/services/test/groups-test.js @@ -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'; @@ -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]; }, @@ -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 = { @@ -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: { @@ -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', () => { diff --git a/src/sidebar/store/modules/groups.ts b/src/sidebar/store/modules/groups.ts index 8b1083d68d9..70e43faeedd 100644 --- a/src/sidebar/store/modules/groups.ts +++ b/src/sidebar/store/modules/groups.ts @@ -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'; @@ -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 = { @@ -62,7 +66,7 @@ const reducers = { ); return {}; } - return { focusedGroupId: action.id }; + return { focusedGroupId: action.id, focusedGroupMembers: null }; }, LOAD_GROUPS(state: State, action: { groups: Group[] }) { @@ -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, }; }, }; @@ -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. */ @@ -229,6 +263,7 @@ export const groupsModule = createStoreModule(initialState, { filterGroups, focusGroup, loadGroups, + loadFocusedGroupMembers, clearGroups, }, selectors: { @@ -237,6 +272,7 @@ export const groupsModule = createStoreModule(initialState, { filteredGroupIds, focusedGroup, focusedGroupId, + getFocusedGroupMembers, getFeaturedGroups, getGroup, getInScopeGroups, diff --git a/src/sidebar/store/modules/test/groups-test.js b/src/sidebar/store/modules/test/groups-test.js index 62775c023f0..ba13dc9b82e 100644 --- a/src/sidebar/store/modules/test/groups-test.js +++ b/src/sidebar/store/modules/test/groups-test.js @@ -134,6 +134,19 @@ describe('sidebar/store/modules/groups', () => { assert.notCalled(console.error); }); + it('unsets the focused group members if valid', () => { + store.loadGroups([publicGroup, privateGroup]); + + // We need to initially focus a group so that we can set its members + store.focusGroup(publicGroup.id); + store.loadFocusedGroupMembers([]); + assert.isNotNull(store.getState().groups.focusedGroupMembers); + + // Once we switch to focus another group, members are unset + store.focusGroup(privateGroup.id); + assert.isNull(store.getState().groups.focusedGroupMembers); + }); + it('does not update focused group if not valid', () => { store.loadGroups([publicGroup]); @@ -167,6 +180,23 @@ describe('sidebar/store/modules/groups', () => { }); }); + describe('loadFocusedGroupMembers', () => { + it('throws if trying to set group members before focusing a group', () => { + assert.throws( + () => store.loadFocusedGroupMembers([]), + 'A group needs to be focused before loading its members', + ); + }); + + it('sets group members', () => { + store.loadGroups([privateGroup]); + store.focusGroup(privateGroup.id); + store.loadFocusedGroupMembers([]); + + assert.deepEqual(store.getState().groups.focusedGroupMembers, []); + }); + }); + describe('clearGroups', () => { it('clears the list of groups', () => { store.loadGroups([publicGroup]); @@ -246,6 +276,20 @@ describe('sidebar/store/modules/groups', () => { }); }); + describe('getFocusedGroupMembers', () => { + it('returns `null` if no group members have been loaded', () => { + assert.equal(store.getFocusedGroupMembers(), null); + }); + + it('returns list of members if they have been loaded', () => { + store.loadGroups([privateGroup]); + store.focusGroup(privateGroup.id); + store.loadFocusedGroupMembers([]); + + assert.deepEqual(store.getFocusedGroupMembers(), []); + }); + }); + describe('getFeaturedGroups', () => { [ { diff --git a/src/types/api.ts b/src/types/api.ts index 60f7895f554..70d5b6cce47 100644 --- a/src/types/api.ts +++ b/src/types/api.ts @@ -294,6 +294,26 @@ export type Group = { */ export type GroupIdentifier = NonNullable; +export type GroupMember = { + authority: string; + userid: string; + username: string; + display_name: string | null; + roles: string[]; + actions: string[]; + created: string; + updated: string; +}; + +export type GroupMembers = { + meta: { + page: { + total: number; + }; + }; + data: GroupMember[]; +}; + /** * Query parameters for an `/api/search` API call. *