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.

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

[AJ-1277] select and delete rows #5090

Closed
wants to merge 24 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
24 commits
Select commit Hold shift + click to select a range
4ea75a6
WIP
mspector Sep 11, 2024
2f29d0f
Merge branch 'dev' into AJ-1277_select-and-delete-rows
mspector Sep 11, 2024
c6be257
wire up deleteRecordsV1 API
mspector Sep 11, 2024
ece9d2d
remove old comments
mspector Sep 18, 2024
f101dfe
fix WDSContent tests
mspector Sep 18, 2024
0253f4e
Merge branch 'dev' into AJ-1277_select-and-delete-rows
mspector Sep 18, 2024
95b8fed
RecordDeleter.js
mspector Sep 18, 2024
55fc585
build errors
mspector Sep 18, 2024
9f23736
type errors
mspector Sep 19, 2024
f9e609f
Merge branch 'dev' into AJ-1277_select-and-delete-rows
mspector Sep 19, 2024
64bfba1
unit test for checkboxes
mspector Sep 19, 2024
4fb0a71
rework comment
mspector Sep 19, 2024
512c57f
Merge branch 'dev' into AJ-1277_select-and-delete-rows
mspector Sep 19, 2024
405e9be
Merge branch 'dev' into AJ-1277_select-and-delete-rows
mspector Sep 20, 2024
267dc5a
Merge branch 'dev' into AJ-1277_select-and-delete-rows
mspector Sep 20, 2024
c433dd9
Update src/libs/ajax/WorkspaceDataService.ts
mspector Sep 25, 2024
799adf1
Update src/workspace-data/data-table/wds/RecordDeleter.js
mspector Sep 25, 2024
731d503
Update src/workspace-data/data-table/wds/RecordDeleter.js
mspector Sep 25, 2024
151869f
Merge branch 'dev' into AJ-1277_select-and-delete-rows
mspector Sep 25, 2024
0dab7b7
build errors from suggestions
mspector Sep 25, 2024
d379ddd
Merge branch 'dev' into AJ-1277_select-and-delete-rows
mspector Sep 25, 2024
e3b019b
gate features behind capabilities
mspector Sep 29, 2024
93a95e3
render edit button with supportsRowSelection
mspector Sep 30, 2024
c9d731e
Merge branch 'dev' into AJ-1277_select-and-delete-rows
mspector Sep 30, 2024
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 12 additions & 0 deletions src/libs/ajax/WorkspaceDataService.ts
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@ import _ from 'lodash/fp';
import { authOpts } from 'src/auth/auth-session';
import { fetchWDS } from 'src/libs/ajax/ajax-common';
import {
DeleteRecordsRequest,
RecordQueryResponse,
RecordResponseBody,
RecordTypeSchema,
Expand Down Expand Up @@ -197,6 +198,17 @@ export const WorkspaceData = (signal) => ({
resultJson.records = _.map(_.unset('attributes.sys_name'), resultJson.records);
return resultJson;
},
deleteRecords: async (
root: string,
collectionId: string,
recordType: string,
parameters: DeleteRecordsRequest
): Promise<any> => {
await fetchWDS(root)(
`records/v1/${collectionId}/${recordType}/delete`,
_.mergeAll([authOpts(), jsonBody(parameters), { signal, method: 'POST' }])
);
},
describeAllRecordTypes: async (root: string, instanceId: string): Promise<any> => {
const res = await fetchWDS(root)(`${instanceId}/types/v0.2`, _.mergeAll([authOpts(), { signal, method: 'GET' }]));
return _.map(
Expand Down
8 changes: 7 additions & 1 deletion src/libs/ajax/data-table-providers/WdsDataTableProvider.ts
Original file line number Diff line number Diff line change
Expand Up @@ -41,6 +41,12 @@ export interface SearchRequest {
sortAttribute?: string;
}

export interface DeleteRecordsRequest {
record_ids?: string[];
excluded_record_ids?: string[];
delete_all?: boolean;
}

export type RecordAttributes = Record<string, unknown>; // truly "unknown" here; the backend Java representation is Map<String, Object>

export interface RecordResponse {
Expand Down Expand Up @@ -177,7 +183,7 @@ export class WdsDataTableProvider implements DataTableProvider {
supportsExport: false,
supportsPointCorrection: false,
supportsFiltering: false,
supportsRowSelection: false,
supportsRowSelection: this.isCapabilityEnabled('apiV1.deleteRecords'), // deletion is the only row-based edit action available right now
supportsPerColumnDatatype: true,
};
}
Expand Down
101 changes: 101 additions & 0 deletions src/workspace-data/data-table/wds/RecordDeleter.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,101 @@
import _ from 'lodash/fp';
import { useState } from 'react';
import { b, div, h } from 'react-hyperscript-helpers';
import { absoluteSpinnerOverlay, DeleteConfirmationModal } from 'src/components/common';
import { Ajax } from 'src/libs/ajax';
import colors from 'src/libs/colors';
import { reportError } from 'src/libs/error';
import * as Utils from 'src/libs/utils';

export const RecordDeleter = ({ onDismiss, onSuccess, dataProvider, collectionId, selectedRecords, runningSubmissionsCount }) => {
Copy link
Contributor

Choose a reason for hiding this comment

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

Any particular reason why this is .js instead of .ts? I'd love for new files to be .ts so we don't have to convert them later!

const [additionalDeletions, setAdditionalDeletions] = useState([]);
const [deleting, setDeleting] = useState(false);

const selectedKeys = _.keys(selectedRecords);

const doDelete = async () => {
const recordsToDelete = _.flow(
_.map(({ name: entityName, entityType }) => ({ entityName, entityType })),
(records) => _.concat(additionalDeletions, records)
)(selectedRecords);

const recordTypes = _.uniq(_.map(({ entityType }) => entityType, selectedRecords));
if (recordTypes.length > 1) {
await reportError('Something went wrong; more than one recordType is represented in the selection. This should not happen.');
}
const recordType = recordTypes[0];
setDeleting(true);

mspector marked this conversation as resolved.
Show resolved Hide resolved
try {
await Ajax().WorkspaceData.deleteRecords(dataProvider.proxyUrl, collectionId, recordType, {
record_ids: recordsToDelete,
});
onSuccess();
} catch (error) {
if (error.status !== 409) {
await reportError('Error deleting data entries', error);
return onDismiss();
}

// Handle 409 error by filtering additional deletions that need to be deleted first
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this in case of references? Or why would they need to be deleted first?

setAdditionalDeletions(await filterAdditionalDeletions(error, recordsToDelete));
setDeleting(false);
}
};

const filterAdditionalDeletions = async (error, recordsToDelete) => {
const errorEntities = await error.json();

return _.filter(
errorEntities,
(errorEntity) =>
!_.some(
recordsToDelete,
(selectedEntity) => selectedEntity.entityType === errorEntity.entityType && selectedEntity.entityName === errorEntity.entityName
)
);
};

const moreToDelete = !!additionalDeletions.length;

const total = selectedKeys.length + additionalDeletions.length;
Copy link
Contributor

Choose a reason for hiding this comment

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

Should this additionalDeletions also be checked with !!?

return h(
DeleteConfirmationModal,
{
objectType: 'data',
title: `Delete ${total} ${total > 1 ? 'entries' : 'entry'}`,
onConfirm: doDelete,
onDismiss,
},
[
runningSubmissionsCount > 0 &&
b({ style: { display: 'block', margin: '1rem 0' } }, [
`WARNING: ${runningSubmissionsCount} workflows are currently running in this workspace. ` +
'Deleting the following entries could cause workflows using them to fail.',
]),
moreToDelete &&
b({ style: { display: 'block', margin: '1rem 0' } }, [
'In order to delete the selected data entries, the following entries that reference them must also be deleted.',
]),
// Size the scroll container to cut off the last row to hint that there's more content to be scrolled into view
// Row height calculation is font size * line height + padding + border
div(
{ style: { maxHeight: 'calc((1em * 1.15 + 1.2rem + 1px) * 10.5)', overflowY: 'auto', margin: '0 -1.25rem' } },
_.map(
([i, entity]) =>
div(
{
style: {
borderTop: i === 0 && runningSubmissionsCount === 0 ? undefined : `1px solid ${colors.light()}`,
padding: '0.6rem 1.25rem',
},
},
moreToDelete ? `${entity.entityName} (${entity.entityType})` : entity
),
Utils.toIndexPairs(moreToDelete ? additionalDeletions : selectedKeys)
)
),
deleting && absoluteSpinnerOverlay,
]
);
};
28 changes: 27 additions & 1 deletion src/workspace-data/data-table/wds/WDSContent.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -54,6 +54,9 @@ const defaultProps: WDSContentProps = {
// attributes are required to avoid an error while destructuring from 'workspace-column-defaults'
attributes: {},
},
workspaceSubmissionStats: {
runningSubmissionsCount: 0,
},
},
recordType: marbleSchema.name,
wdsSchema: [marbleSchema],
Expand All @@ -76,7 +79,7 @@ const defaultFeatures: DataTableFeatures = {
supportsExport: false,
supportsPointCorrection: false,
supportsFiltering: false,
supportsRowSelection: false,
supportsRowSelection: true,
supportsPerColumnDatatype: true,
};

Expand Down Expand Up @@ -243,4 +246,27 @@ describe('WDSContent', () => {
expect(editableValues.length).toEqual(3);
});
});

describe('select rows', () => {
it('', async () => {
// Arrange
const { props } = setup({
...defaultSetupOptions,
props: { ...defaultProps, editable: true },
features: {
...defaultFeatures,
},
});

// Act
await act(() => {
render(h(WDSContent, props));
});

// Assert
// there should be 4 checkboxes for 3 values: one for each value, plus one to select all rows.
const checkboxes = await screen.findAllByRole('checkbox');
expect(checkboxes.length).toEqual(4);
});
});
});
77 changes: 72 additions & 5 deletions src/workspace-data/data-table/wds/WDSContent.ts
Original file line number Diff line number Diff line change
@@ -1,11 +1,20 @@
import _ from 'lodash/fp';
import { Fragment, useState } from 'react';
import { h } from 'react-hyperscript-helpers';
import { div, h } from 'react-hyperscript-helpers';
import { ButtonSecondary } from 'src/components/common';
import { icon } from 'src/components/icons';
import { MenuButton } from 'src/components/MenuButton';
import { MenuTrigger } from 'src/components/PopupTrigger';
import { Ajax } from 'src/libs/ajax';
import { DataTableProvider } from 'src/libs/ajax/data-table-providers/DataTableProvider';
import { RecordTypeSchema, wdsToEntityServiceMetadata } from 'src/libs/ajax/data-table-providers/WdsDataTableProvider';
import colors from 'src/libs/colors';
import Events, { extractWorkspaceDetails } from 'src/libs/events';
import { isGoogleWorkspace, WorkspaceWrapper as Workspace } from 'src/workspaces/utils';
import * as WorkspaceUtils from 'src/workspaces/utils';

import DataTable from '../shared/DataTable';
import { RecordDeleter } from './RecordDeleter';

export interface WDSContentProps {
workspace: Workspace;
Expand All @@ -26,11 +35,49 @@ export const WDSContent = ({
}: WDSContentProps) => {
const googleProject = isGoogleWorkspace(workspace) ? workspace.workspace.googleProject : undefined;
// State
const [refreshKey] = useState(0);
const [refreshKey, setRefreshKey] = useState(0);
const [selectedRecords, setSelectedRecords] = useState({});
const [deletingRecords, setDeletingRecords] = useState(false);

// Render
const [entityMetadata, setEntityMetadata] = useState(() => wdsToEntityServiceMetadata(wdsSchema));

const recordsSelected = !_.isEmpty(selectedRecords);

// This is a (mostly) copy/paste from the EntitiesContent component.
// Maintainers of the future should consider abstracting it into its own component or shared function.
const renderEditMenu = () => {
return h(
MenuTrigger,
{
side: 'bottom',
closeOnClick: true,
content: h(Fragment, [
h(
MenuButton,
{
disabled: !recordsSelected,
tooltip: !recordsSelected && 'Select rows to delete in the table',
onClick: () => setDeletingRecords(true),
},
['Delete selected rows']
),
]),
},
[
h(
ButtonSecondary,
{
tooltip: 'Edit data',
...WorkspaceUtils.getWorkspaceEditControlProps(workspace as WorkspaceUtils.WorkspaceAccessInfo),
style: { marginRight: '1.5rem' },
},
[icon('edit', { style: { marginRight: '0.5rem' } }), 'Edit']
),
]
);
};

// dataProvider contains the proxyUrl for an instance of WDS
return h(Fragment, [
h(DataTable, {
Expand All @@ -45,19 +92,39 @@ export const WDSContent = ({
workspace,
snapshotName: undefined,
selectionModel: {
selected: [],
setSelected: () => [],
selected: selectedRecords,
setSelected: setSelectedRecords,
},
setEntityMetadata,
childrenBefore: undefined,
enableSearch: false,
controlPanelStyle: {
background: colors.light(1),
borderBottom: `1px solid ${colors.grey(0.4)}`,
},
border: false,
loadMetadata,
childrenBefore: () =>
div(
{ style: { display: 'flex', alignItems: 'center', flex: 'none' } },
dataProvider.features.supportsRowSelection ? [renderEditMenu()] : []
),
}),
deletingRecords &&
h(RecordDeleter, {
onDismiss: () => setDeletingRecords(false),
onSuccess: () => {
setDeletingRecords(false);
setSelectedRecords({});
setRefreshKey(_.add(1));
Ajax().Metrics.captureEvent(Events.workspaceDataDelete, extractWorkspaceDetails(workspace.workspace));
Copy link
Contributor

Choose a reason for hiding this comment

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

I like that we are capturing the deletion event

loadMetadata();
},
dataProvider,
collectionId: workspace.workspace.workspaceId,
selectedRecords,
selectedRecordType: recordType,
runningSubmissionsCount: workspace?.workspaceSubmissionStats?.runningSubmissionsCount,
}),
]);
};

Expand Down
Loading