Skip to content

Commit

Permalink
Disable controls for a member when saving changes
Browse files Browse the repository at this point in the history
Disable the role-change select and "remove member" button when an API request is
in-flight to change or remove the user. This both prevents conflicting actions
(eg. changing role while removing) and provides a subtle saving indicator.
  • Loading branch information
robertknight committed Nov 29, 2024
1 parent a4865c4 commit e4600ff
Show file tree
Hide file tree
Showing 2 changed files with 63 additions and 11 deletions.
42 changes: 31 additions & 11 deletions h/static/scripts/group-forms/components/EditGroupMembersForm.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -25,9 +25,18 @@ type TableColumn<Row> = {
type MemberRow = {
username: string;
userid: string;

/** Whether to show the button to remove this member from the group? */
showDeleteAction: boolean;

/** Current role for this user. */
role: Role;

/** Roles that can be assigned to this member. */
availableRoles: Role[];

/** True if an operation is currently being performed against this member. */
busy: boolean;
};

/**
Expand Down Expand Up @@ -57,6 +66,7 @@ function memberToRow(member: GroupMember, currentUserid: string): MemberRow {
member.actions.includes('delete') && member.userid !== currentUserid,
role,
availableRoles,
busy: false,
};
}

Expand Down Expand Up @@ -102,6 +112,8 @@ async function setMemberRoles(
type RoleSelectProps = {
username: string;

disabled?: boolean;

/** The current role of the member. */
current: Role;

Expand All @@ -114,6 +126,7 @@ type RoleSelectProps = {

function RoleSelect({
username,
disabled = false,
current,
available,
onChange,
Expand All @@ -124,6 +137,7 @@ function RoleSelect({
onChange={onChange}
buttonContent={roleStrings[current]}
data-testid={`role-${username}`}
disabled={disabled}
>
{available.map(role => (
<Select.Option key={role} value={role}>
Expand Down Expand Up @@ -183,6 +197,15 @@ export default function EditGroupMembersForm({

const [pendingRemoval, setPendingRemoval] = useState<string | null>(null);

const updateMember = (userid: string, update: Partial<MemberRow>) => {
setMembers(
members =>
members?.map(m => {
return m.userid === userid ? { ...m, ...update } : m;
}) ?? null,
);
};

const removeUserFromGroup = async (username: string) => {
// istanbul ignore next
if (!members || !config.api.removeGroupMember) {
Expand All @@ -195,27 +218,21 @@ export default function EditGroupMembersForm({
}
setPendingRemoval(null);

updateMember(member.userid, { busy: true });

try {
await removeMember(config.api.removeGroupMember, member.userid);
setMembers(members =>
members ? members.filter(m => m.userid !== member.userid) : null,
);
} catch (err) {
updateMember(member.userid, { busy: false });
setErrorMessage(err.message);
}
};

const updateMember = (userid: string, update: Partial<MemberRow>) => {
setMembers(
members =>
members?.map(m => {
return m.userid === userid ? { ...m, ...update } : m;
}) ?? null,
);
};

const changeRole = async (member: MemberRow, role: Role) => {
updateMember(member.userid, { role });
updateMember(member.userid, { role, busy: true });
try {
const updatedMember = await setMemberRoles(
config.api.editGroupMember!,
Expand All @@ -227,7 +244,7 @@ export default function EditGroupMembersForm({
updateMember(member.userid, memberToRow(updatedMember, currentUserid));
} catch (err) {
const prevRole = member.role;
updateMember(member.userid, { role: prevRole });
updateMember(member.userid, { role: prevRole, busy: false });
setErrorMessage(err.message);
}
};
Expand Down Expand Up @@ -258,11 +275,14 @@ export default function EditGroupMembersForm({
current={user.role}
available={user.availableRoles}
onChange={role => changeRole(user, role)}
disabled={user.busy}
/>
);
case 'showDeleteAction':
return user.showDeleteAction ? (
<IconButton
classes={user.busy ? 'opacity-50' : ''}
disabled={user.busy}
icon={TrashIcon}
title="Remove member"
data-testid={`remove-${user.username}`}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -146,6 +146,18 @@ describe('EditGroupMembersForm', () => {
return wrapper.find(`span[data-testid="role-${username}"]`);
};

/**
* Return true if controls to edit a given member are all disabled, or there
* are no controls for the member.
*/
const controlsDisabled = (wrapper, username) => {
const removeButton = getRemoveUserButton(wrapper, username);
const roleSelect = getRoleSelect(wrapper, username);
return [removeButton, roleSelect].every(
control => !control.exists() || control.prop('disabled'),
);
};

it('fetches and displays members', async () => {
const wrapper = createForm();
assert.calledWith(
Expand Down Expand Up @@ -226,6 +238,9 @@ describe('EditGroupMembersForm', () => {
wrapper.update();
assert.isFalse(wrapper.exists('WarningDialog'));

// Controls should be disabled while API call is in flight.
assert.isTrue(controlsDisabled(wrapper, 'bob'));

// Once the user is removed, their row should disappear.
await waitFor(() => {
wrapper.update();
Expand Down Expand Up @@ -254,6 +269,10 @@ describe('EditGroupMembersForm', () => {
await waitForError(wrapper);
const error = wrapper.find('ErrorNotice');
assert.equal(error.prop('message'), 'User not found');

// Controls should be re-enabled after saving fails.
assert.include(getRenderedUsers(wrapper), 'bob');
assert.isFalse(controlsDisabled(wrapper, 'bob'));
});

it('displays current and available user roles', async () => {
Expand Down Expand Up @@ -308,6 +327,18 @@ describe('EditGroupMembersForm', () => {
},
}),
);

// Controls should be disabled while saving.
assert.isTrue(controlsDisabled(wrapper, 'bob'));

await waitFor(() => {
wrapper.update();
return !controlsDisabled(wrapper, 'bob');
});

// New role should be preserved once save completes.
bobRole = getRoleSelect(wrapper, 'bob');
assert.equal(bobRole.prop('value'), 'moderator');
});

it('displays error if changing role fails', async () => {
Expand Down Expand Up @@ -340,5 +371,6 @@ describe('EditGroupMembersForm', () => {
wrapper.update();
bobRole = getRoleSelect(wrapper, 'bob');
assert.equal(bobRole.prop('value'), originalRole);
assert.isFalse(controlsDisabled(wrapper, 'bob'));
});
});

0 comments on commit e4600ff

Please sign in to comment.