diff --git a/client/client.ts b/client/client.ts index f34a86b46..d2a3f5e3a 100644 --- a/client/client.ts +++ b/client/client.ts @@ -222,7 +222,6 @@ export class Client { async tablePatch(props: { path: string - toPath?: string history?: types.IHistory resource?: types.IResource }) { diff --git a/client/components/Application/Dialog.tsx b/client/components/Application/Dialog.tsx index 23d22a148..3e8578978 100644 --- a/client/components/Application/Dialog.tsx +++ b/client/components/Application/Dialog.tsx @@ -9,6 +9,7 @@ import { FileUploadDialog } from './Dialogs/FileUpload' import OpenLocationDialog from './Dialogs/OpenLocation' import PublishDialog from './Dialogs/Publish' import RenameFileDialog from './Dialogs/RenameFile' +import { SaveChangesDialog } from './Dialogs/SaveChanges' import UnsavedChangesDialog from './Dialogs/UnsavedChanges' import WelcomeBannerDialog from './Dialogs/WelcomeBanner' @@ -37,4 +38,5 @@ const DIALOGS = { fileUpload: FileUploadDialog, openLocation: OpenLocationDialog, renameFile: RenameFileDialog, + saveChanges: SaveChangesDialog, } as const diff --git a/client/components/Application/Dialogs/SaveChanges/SaveChanges.store.ts b/client/components/Application/Dialogs/SaveChanges/SaveChanges.store.ts new file mode 100644 index 000000000..8c0b4fdb6 --- /dev/null +++ b/client/components/Application/Dialogs/SaveChanges/SaveChanges.store.ts @@ -0,0 +1,63 @@ +import { client } from '@client/client' +import * as helpers from '@client/helpers' +import * as appStore from '@client/store' + +// We use component level state because dialog state +// needs to be shared between multiple components +// but it is not needed in the global state +class State { + progress?: IProgress +} + +type IProgress = { + type: 'loading' | 'error' + title?: string + message?: string + blocking?: boolean +} + +export const { state, useState } = helpers.createState('SaveChangesDialog', new State()) + +export async function saveChanges() { + const { grid } = appStore.getRefs() + const { path, resource, table } = appStore.getState() + if (!path || !grid || !table) return + + appStore.openDialog('saveChanges') + state.progress = { + type: 'loading', + title: 'Saving the updated table', + message: 'If the file is large, this may take some time...', + blocking: true, + } + + const appState = appStore.getState() + const isTableUpdated = appStore.getIsTableUpdated(appState) + const isResourceUpdated = appState.isResourceUpdated + + const result = await client.tablePatch({ + path, + history: isTableUpdated ? table.history : undefined, + resource: isResourceUpdated ? resource : undefined, + }) + + if (result instanceof client.Error) { + state.progress = { + type: 'error', + title: 'Error saving changes', + message: result.detail, + } + } + + await appStore.onFileUpdated([path]) + grid.reload() + + state.progress = undefined + closeDialog() +} + +export function closeDialog() { + if (!state.progress?.blocking) { + appStore.closeDialog() + } +} diff --git a/client/components/Application/Dialogs/SaveChanges/SaveChanges.tsx b/client/components/Application/Dialogs/SaveChanges/SaveChanges.tsx new file mode 100644 index 000000000..31d9c81dc --- /dev/null +++ b/client/components/Application/Dialogs/SaveChanges/SaveChanges.tsx @@ -0,0 +1,47 @@ +import NoButtonDialog from '@client/components/Parts/Dialogs/NoButton' +import Box from '@mui/material/Box' +import LinearProgress from '@mui/material/LinearProgress' +import Stack from '@mui/material/Stack' +import { startCase } from 'lodash' +import * as store from './SaveChanges.store' + +export function SaveChangesDialog() { + return ( + + + + ) +} + +// TODO: move to common components +function ProgressIndicator() { + const { progress } = store.useState() + + if (!progress) { + return null + } + + if (progress.type === 'error') { + return {progress.message} + } + + return ( + + {progress.title || startCase(progress.type)}... + + {progress.message} + + ) +} diff --git a/client/components/Application/Dialogs/SaveChanges/index.ts b/client/components/Application/Dialogs/SaveChanges/index.ts new file mode 100644 index 000000000..dbffc1cfd --- /dev/null +++ b/client/components/Application/Dialogs/SaveChanges/index.ts @@ -0,0 +1,2 @@ +export { SaveChangesDialog } from './SaveChanges' +export * as saveChangesDialog from './SaveChanges.store' diff --git a/client/components/Controllers/Table/Editor.tsx b/client/components/Controllers/Table/Editor.tsx index 6d05b99f0..d3f8bdbf1 100644 --- a/client/components/Controllers/Table/Editor.tsx +++ b/client/components/Controllers/Table/Editor.tsx @@ -1,10 +1,11 @@ -import TableEditor from '../../Editors/Table' +import { saveChangesDialog } from '@client/components/Application/Dialogs/SaveChanges' +import * as store from '@client/store' +import * as types from '@client/types' import { ClickAwayListener } from '@mui/base' import Box from '@mui/material/Box' -import * as React from 'react' import { useKeyPress } from 'ahooks' -import * as store from '@client/store' -import * as types from '@client/types' +import * as React from 'react' +import TableEditor from '../../Editors/Table' export default function Editor() { const schema = store.useStore((state) => state.record?.resource.schema) @@ -40,7 +41,7 @@ export default function Editor() { }) useKeyPress(['ctrl+s', 'meta+s'], () => { - store.saveTable() + saveChangesDialog.saveChanges() }) // Ensure that when the user interact with other parts on the application diff --git a/client/components/Controllers/Table/Menu.tsx b/client/components/Controllers/Table/Menu.tsx index 6904a2cbb..2e588d036 100644 --- a/client/components/Controllers/Table/Menu.tsx +++ b/client/components/Controllers/Table/Menu.tsx @@ -1,7 +1,8 @@ -import * as menu from '../../Parts/Bars/Menu' +import { saveChangesDialog } from '@client/components/Application/Dialogs/SaveChanges' import * as store from '@client/store' import Box from '@mui/material/Box' import * as action from '../../Parts/Bars/Action' +import * as menu from '../../Parts/Bars/Menu' export default function Menu() { const panel = store.useStore((state) => state.panel) @@ -56,7 +57,10 @@ export default function Menu() { disabled={isTableUpdated} onClick={() => store.openDialog('publish')} /> - + diff --git a/client/components/Parts/Dialogs/NoButton.tsx b/client/components/Parts/Dialogs/NoButton.tsx new file mode 100644 index 000000000..0130e4847 --- /dev/null +++ b/client/components/Parts/Dialogs/NoButton.tsx @@ -0,0 +1,48 @@ +import CloseIcon from '@mui/icons-material/Close' +import Dialog, { DialogProps } from '@mui/material/Dialog' +import DialogContent from '@mui/material/DialogContent' +import DialogTitle from '@mui/material/DialogTitle' +import IconButton from '@mui/material/IconButton' + +export default function NoButtonDialog( + props: DialogProps & { title: string; onClose: () => void } +) { + const handleClose = () => { + props.onClose() + } + + return ( + + theme.palette.grey[500], + }} + > + + + theme.palette.OKFNGray100.main, + }} + > + {props.title} + + {props.children} + + ) +} diff --git a/client/store/actions/file.ts b/client/store/actions/file.ts index 21fc755a7..6353e85ad 100644 --- a/client/store/actions/file.ts +++ b/client/store/actions/file.ts @@ -1,4 +1,5 @@ import { client } from '@client/client' +import { saveChangesDialog } from '@client/components/Application/Dialogs/SaveChanges' import * as helpers from '@client/helpers' import * as settings from '@client/settings' import { cloneDeep } from 'lodash' @@ -7,7 +8,7 @@ import * as store from '../store' import { openDialog } from './dialog' import { emitEvent } from './event' import { loadSource } from './source' -import { closeTable, getIsTableUpdated, openTable, revertTable, saveTable } from './table' +import { closeTable, getIsTableUpdated, openTable, revertTable } from './table' import { closeText, getIsTextUpdated, openText, revertText, saveText } from './text' export async function loadFiles(throwError?: boolean) { @@ -141,7 +142,7 @@ export async function saveFile() { invariant(record) if (record.type === 'table') { - await saveTable() + await saveChangesDialog.saveChanges() } else if (record.type === 'text') { await saveText() } diff --git a/client/store/actions/table.ts b/client/store/actions/table.ts index 276beba79..acb167b29 100644 --- a/client/store/actions/table.ts +++ b/client/store/actions/table.ts @@ -1,14 +1,13 @@ import { client } from '@client/client' -import invariant from 'tiny-invariant' -import { mapValues, isNull } from 'lodash' -import { onFileCreated, onFileUpdated } from './file' -import { cloneDeep } from 'lodash' -import { revertResource } from './resource' -import { getRefs } from './refs' import * as helpers from '@client/helpers' import * as settings from '@client/settings' import * as types from '@client/types' +import { cloneDeep, isNull, mapValues } from 'lodash' +import invariant from 'tiny-invariant' import * as store from '../store' +import { onFileUpdated } from './file' +import { getRefs } from './refs' +import { revertResource } from './resource' export async function openTable() { const { path, record } = store.getState() @@ -36,26 +35,6 @@ export async function closeTable() { }) } -export async function forkTable(toPath: string) { - const { path, table, resource } = store.getState() - if (!path || !table) return - - const result = await client.tablePatch({ - path, - toPath, - history: table.history, - resource, - }) - - if (result instanceof client.Error) { - return store.setState('fork-table-error', (state) => { - state.error = result - }) - } - - await onFileCreated([result.path]) -} - export async function publishTable(control: types.IControl) { const { record } = store.getState() @@ -93,31 +72,6 @@ export async function revertTable() { revertResource() } -export async function saveTable() { - const { grid } = getRefs() - const { path, resource, table } = store.getState() - if (!path || !grid || !table) return - - const state = store.getState() - const isTableUpdated = getIsTableUpdated(state) - const isResourceUpdated = state.isResourceUpdated - - const result = await client.tablePatch({ - path, - history: isTableUpdated ? table.history : undefined, - resource: isResourceUpdated ? resource : undefined, - }) - - if (result instanceof client.Error) { - return store.setState('save-table-error', (state) => { - state.error = result - }) - } - - await onFileUpdated([path]) - grid.reload() -} - export function setTableSelection(selection: types.ITableSelection) { store.setState('set-table-selection', (state) => { state.table!.selection = selection diff --git a/client/store/index.ts b/client/store/index.ts index 0bc3636be..f18da8bce 100644 --- a/client/store/index.ts +++ b/client/store/index.ts @@ -1,3 +1,4 @@ export * from './actions' +export { getRefs } from './actions/refs' export * from './state' export { getState, useStore } from './store' diff --git a/client/store/state.ts b/client/store/state.ts index 3033b079d..38f4f68a2 100644 --- a/client/store/state.ts +++ b/client/store/state.ts @@ -184,6 +184,7 @@ export type IDialog = | 'fileUpload' | 'openLocation' | 'renameFile' + | 'saveChanges' export type IPanel = 'metadata' | 'report' | 'changes' | 'source' diff --git a/server/endpoints/table/patch.py b/server/endpoints/table/patch.py index cfd835637..83a352b2e 100644 --- a/server/endpoints/table/patch.py +++ b/server/endpoints/table/patch.py @@ -1,10 +1,10 @@ from __future__ import annotations -from typing import Optional +from typing import List, Optional import sqlalchemy as sa from fastapi import Request -from frictionless import FrictionlessException, Schema +from frictionless import Schema from frictionless.formats.sql import SqlControl from frictionless.resources import TableResource from pydantic import BaseModel @@ -16,7 +16,6 @@ class Props(BaseModel, extra="forbid"): path: str - toPath: Optional[str] = None history: Optional[models.History] = None resource: Optional[types.IDescriptor] = None @@ -31,80 +30,107 @@ def endpoint(request: Request, props: Props) -> Result: def action(project: Project, props: Props) -> Result: + from ... import endpoints + fs = project.filesystem db = project.database - # Forbid overwriting - if props.toPath and helpers.test_file(project, path=props.toPath): - raise FrictionlessException("file already exists") - # Patch record record = helpers.patch_record( project, path=props.path, - toPath=props.toPath, resource=props.resource, ) - # Copy table - if props.toPath: - fromRecord = helpers.read_record_or_raise(project, path=props.path) - with db.engine.begin() as conn: - query = f'CREATE TABLE "{record.name}" AS SELECT * FROM "{fromRecord.name}"' - conn.execute(sa.text(query)) - db.metadata.reflect(conn) + # This data will help us to reconstruct the error report + # https://github.com/okfn/opendataeditor/issues/614 + prev_report = db.read_artifact(name=record.name, type="report") or {} + updated_cells: List[models.Cell] = [] # Patch table if props.history: table = db.metadata.tables[record.name] schema = Schema.from_descriptor(record.resource.get("schema", {})) + # Collect updated cells + for change in props.history.changes: + if change.type == "cell-update": + # TODO: it's better to sync types of "cell-update" and "multiple-cells-update" + updated_cells.append(models.Cell(**change.model_dump())) + elif change.type == "multiple-cells-update": + for cell in change.cells: + updated_cells.append(cell) + # Patch database table - with db.engine.begin() as conn: - for change in props.history.changes: - if change.type == "cell-update": - # Skip virtual extra cells - if change.fieldName.startswith("_"): - continue - - # Prepare value - value = change.value - if schema.has_field(change.fieldName): - field = schema.get_field(change.fieldName) - value, _ = field.read_cell(value) - - # Write value - conn.execute( - sa.update(table) - .where(table.c._rowNumber == change.rowNumber) - .values(**{change.fieldName: value}) - ) - elif change.type == "multiple-cells-update": - for cell in change.cells: - # Skip virtual extra cells - if cell.fieldName.startswith("_"): - continue - - # Prepare value - value = cell.value - if schema.has_field(cell.fieldName): - field = schema.get_field(cell.fieldName) - value, _ = field.read_cell(value) - # Write value - conn.execute( - sa.update(table) - .where(table.c._rowNumber == cell.rowNumber) - .values(**{cell.fieldName: value}) - ) + with db.engine.begin() as trx: + for cell in updated_cells: + # Skip virtual extra cells + if cell.fieldName.startswith("_"): + continue + + # Prepare value + value = cell.value + if schema.has_field(cell.fieldName): + field = schema.get_field(cell.fieldName) + value, _ = field.read_cell(cell.value) + + # Write value + trx.execute( + sa.update(table) + .where(table.c._rowNumber == cell.rowNumber) + .values(**{cell.fieldName: value}) + ) # Export database table - target = fs.get_fullpath(props.toPath or props.path) + target = fs.get_fullpath(props.path) control = SqlControl(table=record.name, with_metadata=True) resource = TableResource(path=db.database_url, control=control) resource.write_table(path=str(target)) # Reset record - if not props.toPath: - helpers.reset_record(project, path=props.path) + helpers.reset_record(project, path=props.path) + + # Index record + endpoints.file.index.action( + project, props=endpoints.file.index.Props(path=props.path) + ) + + # Reconstruct previous errors + # Currently, we only recover errors if no metadata is changed + # otherwise it's basically not possible to reconstruct errors + # The data/metadata separation needs to be properly implemeted as per: + # https://github.com/okfn/opendataeditor/issues/494 + if updated_cells and not props.resource: + next_report = db.read_artifact(name=record.name, type="report") or {} + not_fixed_errors_count = 0 + + prev_task = prev_report["tasks"][0] + next_task = next_report["tasks"][0] + + for error in prev_task["errors"]: + type = error["type"] + if type not in ["type-error"]: + continue + + rowNumber = error["rowNumber"] + fieldName = error["fieldName"] + + fixed = False + for cell in updated_cells: + # The error was fixed so we don't need to recover it + if cell.rowNumber == rowNumber and cell.fieldName == fieldName: + fixed = True + break + + if not fixed: + not_fixed_errors_count += 1 + next_task["errors"].append(error) # type: ignore + + if not_fixed_errors_count: + next_task["valid"] = False + next_task["stats"]["errors"] += not_fixed_errors_count + next_report["valid"] = False + next_report["stats"]["errors"] += not_fixed_errors_count + db.write_artifact(name=record.name, type="report", descriptor=next_report) return Result(path=record.path)