Skip to content

Commit 839df45

Browse files
rcreasircreasi
authored
fix access group deletion (#757)
The delete request sent the group id as a body instead of in the URL, so it never reached the group. Fixing that exposed how long the request takes while Breadbox re-checks dataset access, so the groups manager now shows a progress modal while deletes run, and reports failures.
1 parent ca704d5 commit 839df45

6 files changed

Lines changed: 179 additions & 84 deletions

File tree

‎frontend/packages/@depmap/api/src/breadboxAPI/index.ts‎

Lines changed: 14 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -32,14 +32,24 @@ export const breadboxAPI = {
3232

3333
type Api = typeof breadboxAPI;
3434

35-
(Object.keys(breadboxAPI) as Array<keyof Api>).forEach((name) => {
36-
const originalFn = breadboxAPI[name];
35+
// Each method gets swapped out for a wrapper that decorates failures with a
36+
// call-site stack. TypeScript can't check that slot by slot (writing to
37+
// `breadboxAPI[someUnionOfKeys]` demands a function satisfying *every* method
38+
// signature at once), so the types are erased for the duration of the loop.
39+
// The wrapper preserves each method's arguments and return value verbatim, so
40+
// the declared `Api` types still hold for callers.
41+
const untypedApi = (breadboxAPI as unknown) as Record<
42+
string,
43+
(...args: unknown[]) => Promise<unknown>
44+
>;
3745

38-
breadboxAPI[name] = async (...args: Parameters<typeof originalFn>) => {
46+
Object.keys(untypedApi).forEach((name) => {
47+
const originalFn = untypedApi[name];
48+
49+
untypedApi[name] = async (...args: unknown[]) => {
3950
const callSiteError = new Error(`breadboxAPI method "${name}" failed`);
4051

4152
try {
42-
// @ts-expect-error 2556
4353
return await originalFn(...args);
4454
} catch (error) {
4555
const lines = callSiteError.stack?.split("\n") || [];

‎frontend/packages/@depmap/api/src/breadboxAPI/resources/groups.ts‎

Lines changed: 1 addition & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -11,8 +11,7 @@ export function postGroup(groupArgs: GroupArgs) {
1111
}
1212

1313
export function deleteGroup(id: string) {
14-
// TODO: Figure out return type.
15-
return deleteJson<any>("/groups", id);
14+
return deleteJson<{ message: string }>(uri`/groups/${id}`);
1615
}
1716

1817
export function postGroupEntry(

‎frontend/packages/@depmap/groups-manager/src/components/AddGroupEntryForm.tsx‎

Lines changed: 65 additions & 64 deletions
Original file line numberDiff line numberDiff line change
@@ -46,6 +46,13 @@ interface EmailEntriesInput {
4646
readonly emailEntries: string[];
4747
}
4848

49+
// Email addresses can be separated by whitespace or commas.
50+
const parseEmails = (value: string) =>
51+
value.split(/[\s,]+/).filter((email) => email !== "");
52+
53+
const isValidEmail = (email: string) =>
54+
/^[^\s@]+@[^\s@.]+(\.[^\s@.]+)+$/.test(email);
55+
4956
function AddGroupEntryForm({
5057
group,
5158
addGroupEntries,
@@ -102,27 +109,37 @@ function AddGroupEntryForm({
102109
value: label,
103110
});
104111

105-
const handleKeyDown = (e: React.KeyboardEvent<HTMLElement>) => {
106-
if (!emailEntriesOptions.inputValue) return;
107-
const valuesArray = emailEntriesOptions.inputValue
108-
.split(",")
109-
.map((option) => option.trim());
110-
const valueOptions = valuesArray.map((val) => {
111-
return createOption(val);
112+
// Turns whatever has been typed so far into tags.
113+
const commitInputValue = () => {
114+
const emails = parseEmails(emailEntriesOptions.inputValue);
115+
116+
setEmailEntriesOptions({
117+
inputValue: "",
118+
valueOptions: [
119+
...emailEntriesOptions.valueOptions,
120+
...emails.map(createOption),
121+
],
122+
emailEntries: [...emailEntriesOptions.emailEntries, ...emails],
112123
});
113-
switch (e.key) {
114-
case "Enter":
115-
case "Tab":
116-
setEmailEntriesOptions({
117-
inputValue: "",
118-
valueOptions: [...emailEntriesOptions.valueOptions, ...valueOptions],
119-
emailEntries: [...emailEntriesOptions.emailEntries, ...valuesArray],
120-
});
121-
e.preventDefault();
122-
break;
123-
default:
124-
console.log("Unexpected keu pressed");
124+
};
125+
126+
const handleKeyDown = (e: React.KeyboardEvent<HTMLElement>) => {
127+
if (e.key !== "Enter" && e.key !== "Tab" && e.key !== " ") {
128+
return;
129+
}
130+
131+
// A space is never part of an email address, so swallow it even when
132+
// there's nothing to commit yet.
133+
if (e.key === " ") {
134+
e.preventDefault();
125135
}
136+
137+
if (parseEmails(emailEntriesOptions.inputValue).length === 0) {
138+
return;
139+
}
140+
141+
e.preventDefault();
142+
commitInputValue();
126143
};
127144

128145
const AccessTypeSelector = () => {
@@ -152,36 +169,20 @@ function AddGroupEntryForm({
152169
addedGroupEntries: string[],
153170
newGroupEntries: GroupEntry[]
154171
) => {
155-
if (addedGroupEntries.length === emailEntriesOptions.emailEntries.length) {
156-
setEmailEntriesOptions({
157-
inputValue: "",
158-
valueOptions: [],
159-
emailEntries: [],
160-
});
161-
} else {
162-
// Remove email entries already added
163-
const indexesToRemove = [];
164-
for (let i = 0; i < addedGroupEntries.length; i += 1) {
165-
const idxToRemove = emailEntriesOptions.emailEntries.indexOf(
166-
addedGroupEntries[i]
167-
);
168-
if (idxToRemove > -1) {
169-
indexesToRemove.push(idxToRemove);
170-
}
171-
}
172-
173-
const emailEntriesToBeAdded = [...emailEntriesOptions.emailEntries];
174-
indexesToRemove.map((idx) => emailEntriesToBeAdded.splice(idx, 1));
172+
// Keep whatever wasn't added, so a partial failure leaves the offending
173+
// addresses in the input for the user to correct.
174+
setEmailEntriesOptions((prev) => {
175+
const remaining = [
176+
...prev.emailEntries,
177+
...parseEmails(prev.inputValue),
178+
].filter((email) => !addedGroupEntries.includes(email));
175179

176-
const valueOptionsToRemain = emailEntriesToBeAdded.map((emailEntry) => {
177-
return createOption(emailEntry);
178-
});
179-
setEmailEntriesOptions({
180+
return {
180181
inputValue: "",
181-
valueOptions: valueOptionsToRemain,
182-
emailEntries: emailEntriesToBeAdded,
183-
});
184-
}
182+
valueOptions: remaining.map(createOption),
183+
emailEntries: remaining,
184+
};
185+
});
185186
setGroupEntryTableData(newGroupEntries);
186187
};
187188

@@ -193,7 +194,16 @@ function AddGroupEntryForm({
193194
setGroupEntryTableData(newGroupEntries);
194195
};
195196

196-
/* TODO: Add validater for email address and check if owner */
197+
/* TODO: check if owner */
198+
199+
// Uncommitted input counts towards the Add button, so a single address
200+
// doesn't have to be turned into a tag first. It only counts once it looks
201+
// like an email address, otherwise a half-typed one would enable Add.
202+
const pendingEmails = parseEmails(emailEntriesOptions.inputValue);
203+
const emailsToAdd =
204+
pendingEmails.length > 0 && pendingEmails.every(isValidEmail)
205+
? [...emailEntriesOptions.emailEntries, ...pendingEmails]
206+
: emailEntriesOptions.emailEntries;
197207

198208
return (
199209
<>
@@ -211,33 +221,24 @@ function AddGroupEntryForm({
211221
inputValue={emailEntriesOptions.inputValue}
212222
value={emailEntriesOptions.valueOptions}
213223
onInputChange={handleEmailEntriesInputChange}
214-
onChange={() => {
215-
if (Array.isArray(emailEntriesOptions.valueOptions)) {
216-
throw new Error(
217-
"Unexpected type passed to ReactSelect onChange handler"
218-
);
219-
}
220-
return handleEmailEntriesChange;
221-
}}
224+
onChange={handleEmailEntriesChange}
222225
onKeyDown={handleKeyDown}
223-
placeholder="Type email or comma-separated email addresses and press 'Enter' or 'Tab'"
226+
placeholder="Type one or more email addresses, separated by spaces or commas"
224227
/>
225228
<HelpBlock>{groupEntryErrors?.addGroupEntryError}</HelpBlock>
226229
</FormGroup>
227230
</Col>
228231
<Col xs={6} md={4}>
229232
<Button
230-
disabled={emailEntriesOptions.emailEntries.length === 0}
233+
disabled={emailsToAdd.length === 0}
231234
onClick={() => {
232-
const newGroupEntryArgs: GroupEntryArgs[] = [];
233-
emailEntriesOptions.emailEntries.forEach((emailEntry) => {
234-
const params: GroupEntryArgs = {
235+
const newGroupEntryArgs: GroupEntryArgs[] = emailsToAdd.map(
236+
(emailEntry) => ({
235237
email: emailEntry,
236238
access_type: AccessType.read,
237239
exact_match: true,
238-
};
239-
newGroupEntryArgs.push(params);
240-
});
240+
})
241+
);
241242
addGroupEntries(
242243
group.id,
243244
newGroupEntryArgs,

‎frontend/packages/@depmap/groups-manager/src/components/GroupAddDelete.tsx‎

Lines changed: 5 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -15,13 +15,15 @@ interface GroupAddDeleteProps {
1515
onAdd: (groupArgs: GroupArgs) => void;
1616
onDelete: (groupIdsSet: Set<string>) => void;
1717
errorMessage?: string | null;
18+
isDeleting?: boolean;
1819
}
1920

2021
function GroupAddDelete({
2122
selectedGroupIds,
2223
onAdd,
2324
onDelete,
2425
errorMessage = null,
26+
isDeleting = false,
2527
}: GroupAddDeleteProps) {
2628
const [groupName, setGroupName] = useState<string>("");
2729

@@ -57,17 +59,17 @@ function GroupAddDelete({
5759
onAdd({ name: groupName });
5860
setGroupName("");
5961
}}
60-
disabled={groupName.length === 0}
62+
disabled={groupName.length === 0 || isDeleting}
6163
>
6264
Add new group
6365
</Button>
6466
<Button
6567
style={{ marginLeft: 10 }}
6668
bsStyle="danger"
6769
onClick={() => onDelete(selectedGroupIds)}
68-
disabled={selectedGroupIds.size === 0}
70+
disabled={selectedGroupIds.size === 0 || isDeleting}
6971
>
70-
Delete selected
72+
{isDeleting ? "Deleting…" : "Delete selected"}
7173
</Button>
7274
</Row>
7375
</Col>

‎frontend/packages/@depmap/groups-manager/src/components/GroupsPage.tsx‎

Lines changed: 83 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -9,8 +9,9 @@ import {
99
GroupTableData,
1010
} from "@depmap/types";
1111

12-
import { FormModal, Spinner } from "@depmap/common-components";
12+
import { FormModal, Spinner, showInfoModal } from "@depmap/common-components";
1313
import Button from "react-bootstrap/lib/Button";
14+
import Modal from "react-bootstrap/lib/Modal";
1415

1516
import styles from "../styles/styles.scss";
1617
import WideTable from "@depmap/wide-table";
@@ -21,7 +22,7 @@ import AddGroupEntryForm from "./AddGroupEntryForm";
2122
export interface GroupsPageProps {
2223
getGroups: () => Promise<Group[]>;
2324
addGroup: (groupArgs: GroupArgs) => Promise<Group>;
24-
deleteGroup: (group_id: string) => void;
25+
deleteGroup: (group_id: string) => Promise<unknown>;
2526
addGroupEntry: (
2627
groupId: string,
2728
groupEntryArgs: GroupEntryArgs
@@ -67,6 +68,10 @@ export default function GroupsPage(props: GroupsPageProps) {
6768
null
6869
);
6970
const [addGroupError, setAddGroupError] = useState<string | null>(null);
71+
const [deleteProgress, setDeleteProgress] = useState<{
72+
completed: number;
73+
total: number;
74+
} | null>(null);
7075
const [groupEntryErrors, setGroupEntryErrors] = useState<{
7176
addGroupEntryError: string | null;
7277
updateGroupEntryError: string | null;
@@ -121,18 +126,57 @@ export default function GroupsPage(props: GroupsPageProps) {
121126
};
122127

123128
const deleteButtonAction = async (groupIdsSet: Set<string>) => {
129+
const groupIds = [...groupIdsSet];
130+
const deletedIds = new Set<string>();
131+
let errorMessage: string | null = null;
132+
133+
setDeleteProgress({ completed: 0, total: groupIds.length });
134+
124135
try {
125-
groupIdsSet.forEach(async (groupId) => {
126-
await deleteGroup(groupId);
127-
setGroups((groups || []).filter((group) => group.id !== groupId));
128-
});
136+
/* eslint-disable no-await-in-loop */
137+
for (let i = 0; i < groupIds.length; i += 1) {
138+
await deleteGroup(groupIds[i]);
139+
deletedIds.add(groupIds[i]);
140+
setDeleteProgress({
141+
completed: deletedIds.size,
142+
total: groupIds.length,
143+
});
144+
}
145+
/* eslint-enable no-await-in-loop */
129146
} catch (e) {
130147
console.error(e);
131-
if (e instanceof ErrorTypeError) {
132-
setAddGroupError(e.message);
133-
} else {
134-
setAddGroupError("An unknown error occurred!");
135-
}
148+
errorMessage =
149+
e instanceof ErrorTypeError ? e.message : "An unknown error occurred!";
150+
} finally {
151+
// Drop whatever was successfully deleted, even if a later delete failed.
152+
setGroups((prevGroups) =>
153+
(prevGroups || []).filter((group) => !deletedIds.has(group.id))
154+
);
155+
setSelectedGroupIds(
156+
(prevSelected) =>
157+
new Set([...prevSelected].filter((id) => !deletedIds.has(id)))
158+
);
159+
setDeleteProgress(null);
160+
}
161+
162+
if (errorMessage) {
163+
showInfoModal({
164+
title:
165+
groupIds.length === 1
166+
? "Error deleting group"
167+
: "Error deleting groups",
168+
content: (
169+
<>
170+
<p>{errorMessage}</p>
171+
{groupIds.length > 1 ? (
172+
<p>
173+
{deletedIds.size} of {groupIds.length} selected groups were
174+
deleted before the error occurred.
175+
</p>
176+
) : null}
177+
</>
178+
),
179+
});
136180
}
137181
};
138182

@@ -280,7 +324,35 @@ export default function GroupsPage(props: GroupsPageProps) {
280324
onAdd={addButtonAction}
281325
onDelete={deleteButtonAction}
282326
errorMessage={addGroupError}
327+
isDeleting={deleteProgress !== null}
283328
/>
329+
{deleteProgress ? (
330+
<Modal show backdrop="static" keyboard={false} onHide={() => {}}>
331+
<Modal.Header>
332+
<Modal.Title>
333+
{deleteProgress.total === 1
334+
? "Deleting group"
335+
: `Deleting ${deleteProgress.total} groups`}
336+
</Modal.Title>
337+
</Modal.Header>
338+
<Modal.Body>
339+
<p>
340+
This can take several minutes, because every dataset has to be
341+
checked for access changes. Please don’t close this page.
342+
</p>
343+
{deleteProgress.total > 1 ? (
344+
<p>
345+
<b>
346+
Deleted {deleteProgress.completed} of {deleteProgress.total}.
347+
</b>
348+
</p>
349+
) : null}
350+
<div className={styles.deletingSpinner}>
351+
<Spinner position="static" />
352+
</div>
353+
</Modal.Body>
354+
</Modal>
355+
) : null}
284356
{groupToEditEntries && groupEntryForm ? (
285357
<FormModal
286358
bsSize="large"

0 commit comments

Comments
 (0)