Skip to content

Commit

Permalink
🪟 🎉 Auto-detect schema changes UX flow (#19226)
Browse files Browse the repository at this point in the history
* Show breaking/non-breaking changes detected when auto detect schema featur is enabled

* Fix spacing issues in StatusMainInfo component to more closely match design

* Extract SchemaChanges detected cell from StatusMainInfo to a component
Add checks when the connection has been refreshed
Don't render when the connection is read-only.

* Navigate to replication settings when clicking on review changes button

* Disable connection status when breaking changes are detected
Add useConnectionTableData hook to take advantage of hooks and replace usages of getConnectionTableData
Clean up imports with relative paths

* Cleanup isSchemaChangesFeatureEnabled flags

* Revert some changes to getConnectionsTable
Update how the StatusCell disables based on  schema changes
Update getConnectionTable to provide the schemaChange value

* Clean up review action var naming

* Show reset confirmation modal when trying to refresh the schema after editing a form
Update FormChangeTracker service hooks to return hasFormChanges

* Fix linting issues in SchemaChangesDetected component

* Add StatusCell tests to verify has breaking changes

* Add tests for StatusMainInfo for auto-detect schema changes

* Add mock source and destination definition
* Extract useSchemaChange out of SchemaChangesDetected file for better testing

* Add SchemaDetectedChanges unit test

* Simplify mocking in StatusMainInfo test

* Add manual sync test cases to StatusCell

* Update schemaChange when the catalog is updated
  • Loading branch information
edmundito committed Dec 2, 2022
1 parent 4c55840 commit b72cd71
Show file tree
Hide file tree
Showing 25 changed files with 823 additions and 59 deletions.
6 changes: 4 additions & 2 deletions airbyte-webapp/src/components/EntityTable/ConnectionTable.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@ import { CellProps } from "react-table";

import { Table, SortableTableHeader } from "components/ui/Table";

import { ConnectionScheduleType } from "core/request/AirbyteClient";
import { ConnectionScheduleType, SchemaChange } from "core/request/AirbyteClient";
import { FeatureItem, useFeature } from "hooks/services/Feature";
import { useQuery } from "hooks/useQuery";

Expand All @@ -28,6 +28,7 @@ interface IProps {
const ConnectionTable: React.FC<IProps> = ({ data, entity, onClickRow, onSync }) => {
const navigate = useNavigate();
const query = useQuery<{ sortBy?: string; order?: SortOrderEnum }>();
const isSchemaChangesFeatureEnabled = process.env.REACT_APP_AUTO_DETECT_SCHEMA_CHANGES === "true";
const allowSync = useFeature(FeatureItem.AllowSync);

const sortBy = query.sortBy || "entityName";
Expand Down Expand Up @@ -161,6 +162,7 @@ const ConnectionTable: React.FC<IProps> = ({ data, entity, onClickRow, onSync })
isSyncing={row.original.isSyncing}
isManual={row.original.scheduleType === ConnectionScheduleType.manual}
onSync={onSync}
hasBreakingChange={isSchemaChangesFeatureEnabled && row.original.schemaChange === SchemaChange.breaking}
allowSync={allowSync}
/>
),
Expand All @@ -172,7 +174,7 @@ const ConnectionTable: React.FC<IProps> = ({ data, entity, onClickRow, onSync })
Cell: ({ cell }: CellProps<ITableDataItem>) => <ConnectionSettingsCell id={cell.value} />,
},
],
[allowSync, entity, onSync, onSortClick, sortBy, sortOrder]
[sortBy, sortOrder, entity, onSortClick, onSync, allowSync, isSchemaChangesFeatureEnabled]
);

return <Table columns={columns} data={sortingData} onClickRow={onClickRow} erroredRows />;
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,53 @@
import { render } from "@testing-library/react";
import { TestWrapper } from "test-utils/testutils";

import StatusCell from "./StatusCell";

const mockId = "mock-id";

jest.doMock("hooks/services/useConnectionHook", () => ({
useEnableConnection: () => ({
mutateAsync: jest.fn(),
isLoading: false,
}),
}));

describe("<StatusCell />", () => {
it("renders switch when connection has schedule", () => {
const { getByTestId } = render(<StatusCell id={mockId} onSync={jest.fn()} allowSync enabled />, {
wrapper: TestWrapper,
});

const switchElement = getByTestId("enable-connection-switch");

expect(switchElement).toBeEnabled();
expect(switchElement).toBeChecked();
});

it("renders button when connection does not have schedule", () => {
const { getByTestId } = render(<StatusCell id={mockId} onSync={jest.fn()} allowSync enabled isManual />, {
wrapper: TestWrapper,
});

expect(getByTestId("manual-sync-button")).toBeEnabled();
});

it("disables switch when hasBreakingChange is true", () => {
const { getByTestId } = render(<StatusCell id={mockId} onSync={jest.fn()} allowSync hasBreakingChange />, {
wrapper: TestWrapper,
});

expect(getByTestId("enable-connection-switch")).toBeDisabled();
});

it("disables manual sync button when hasBreakingChange is true", () => {
const { getByTestId } = render(
<StatusCell id={mockId} onSync={jest.fn()} allowSync hasBreakingChange enabled isManual />,
{
wrapper: TestWrapper,
}
);

expect(getByTestId("manual-sync-button")).toBeDisabled();
});
});
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@ import { useEnableConnection } from "hooks/services/useConnectionHook";

interface IProps {
allowSync?: boolean;
hasBreakingChange?: boolean;
enabled?: boolean;
isSyncing?: boolean;
isManual?: boolean;
Expand All @@ -21,7 +22,7 @@ const ProgressMessage = styled.div`
padding: 7px 0;
`;

const StatusCell: React.FC<IProps> = ({ enabled, isManual, id, isSyncing, onSync, allowSync }) => {
const StatusCell: React.FC<IProps> = ({ enabled, isManual, id, isSyncing, onSync, allowSync, hasBreakingChange }) => {
const { mutateAsync: enableConnection, isLoading } = useEnableConnection();

const [{ loading }, OnLaunch] = useAsyncFn(async (event: React.SyntheticEvent) => {
Expand All @@ -45,7 +46,13 @@ const StatusCell: React.FC<IProps> = ({ enabled, isManual, id, isSyncing, onSync
onClick={(event: React.SyntheticEvent) => event.stopPropagation()}
onKeyPress={(event: React.SyntheticEvent) => event.stopPropagation()}
>
<Switch checked={enabled} onChange={onSwitchChange} disabled={!allowSync} loading={isLoading} />
<Switch
checked={enabled}
onChange={onSwitchChange}
disabled={!allowSync || hasBreakingChange}
loading={isLoading}
data-testid="enable-connection-switch"
/>
</div>
);
}
Expand All @@ -59,7 +66,13 @@ const StatusCell: React.FC<IProps> = ({ enabled, isManual, id, isSyncing, onSync
}

return (
<Button size="xs" onClick={OnLaunch} isLoading={loading} disabled={!allowSync || !enabled}>
<Button
size="xs"
onClick={OnLaunch}
isLoading={loading}
disabled={!allowSync || !enabled || hasBreakingChange}
data-testid="manual-sync-button"
>
<FormattedMessage id="tables.launch" />
</Button>
);
Expand Down
3 changes: 2 additions & 1 deletion airbyte-webapp/src/components/EntityTable/types.ts
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
import { ConnectionScheduleData, ConnectionScheduleType } from "../../core/request/AirbyteClient";
import { ConnectionScheduleData, ConnectionScheduleType, SchemaChange } from "../../core/request/AirbyteClient";

interface EntityTableDataItem {
entityId: string;
Expand Down Expand Up @@ -26,6 +26,7 @@ interface ITableDataItem {
lastSync?: number | null;
scheduleData?: ConnectionScheduleData;
scheduleType?: ConnectionScheduleType;
schemaChange: SchemaChange;
lastSyncStatus: string | null;
connectorIcon?: string;
entityIcon?: string;
Expand Down
4 changes: 3 additions & 1 deletion airbyte-webapp/src/components/EntityTable/utils.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,8 @@ import {
SourceDefinitionRead,
SourceRead,
WebBackendConnectionListItem,
} from "../../core/request/AirbyteClient";
} from "core/request/AirbyteClient";

import { EntityTableDataItem, ITableDataItem, Status as ConnectionSyncStatus } from "./types";

const getConnectorTypeName = (connectorSpec: DestinationRead | SourceRead) => {
Expand Down Expand Up @@ -94,6 +95,7 @@ export const getConnectionTableData = (
: getConnectorTypeName(connection[connectType]),
lastSync: connection.latestSyncJobCreatedAt,
enabled: connection.status === ConnectionStatus.active,
schemaChange: connection.schemaChange,
scheduleData: connection.scheduleData,
scheduleType: connection.scheduleType,
status: connection.status,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -6,9 +6,9 @@ import useSyncActions from "components/EntityTable/hooks";
import { ITableDataItem } from "components/EntityTable/types";
import { getConnectionTableData } from "components/EntityTable/utils";

import { WebBackendConnectionListItem } from "core/request/AirbyteClient";
import { RoutePaths } from "pages/routePaths";

import { WebBackendConnectionListItem } from "../../../core/request/AirbyteClient";
import styles from "./DestinationConnectionTable.module.scss";

interface IProps {
Expand Down
20 changes: 20 additions & 0 deletions airbyte-webapp/src/hooks/connection/useSchemaChanges.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,20 @@
import { useMemo } from "react";

import { SchemaChange } from "core/request/AirbyteClient";

export const useSchemaChanges = (schemaChange: SchemaChange) => {
const isSchemaChangesFeatureEnabled = process.env.REACT_APP_AUTO_DETECT_SCHEMA_CHANGES === "true";

return useMemo(() => {
const hasSchemaChanges = isSchemaChangesFeatureEnabled && schemaChange !== SchemaChange.no_change;
const hasBreakingSchemaChange = hasSchemaChanges && schemaChange === SchemaChange.breaking;
const hasNonBreakingSchemaChange = hasSchemaChanges && schemaChange === SchemaChange.non_breaking;

return {
schemaChange,
hasSchemaChanges,
hasBreakingSchemaChange,
hasNonBreakingSchemaChange,
};
}, [isSchemaChangesFeatureEnabled, schemaChange]);
};
Original file line number Diff line number Diff line change
Expand Up @@ -68,11 +68,19 @@ const useConnectionEdit = ({ connectionId }: ConnectionEditProps): ConnectionEdi
const connectionPatch = pick(updatedConnection, updatedKeys);
const wasSyncCatalogUpdated = !!connectionPatch.syncCatalog;

// Ensure that the catalog diff cleared and that the schemaChange status has been updated
const syncCatalogUpdates: Partial<WebBackendConnectionRead> | undefined = wasSyncCatalogUpdated
? {
catalogDiff: undefined,
schemaChange: updatedConnection.schemaChange,
}
: undefined;

// Mutate the current connection state only with the values that were updated
setConnection((connection) => ({
...connection,
...connectionPatch,
catalogDiff: wasSyncCatalogUpdated ? undefined : connection.catalogDiff,
...syncCatalogUpdates,
}));

if (wasSyncCatalogUpdated) {
Expand Down
Original file line number Diff line number Diff line change
@@ -1,14 +1,14 @@
import type { Transition } from "history";

import React, { useCallback, useMemo } from "react";
import React, { useCallback } from "react";

import { useBlocker } from "hooks/router/useBlocker";

import { useConfirmationModalService } from "../ConfirmationModal";
import { useChangedFormsById } from "./hooks";
import { useFormChangeTrackerService } from "./hooks";

export const FormChangeTrackerService: React.FC<React.PropsWithChildren<unknown>> = ({ children }) => {
const [changedFormsById, setChangedFormsById] = useChangedFormsById();
const { hasFormChanges, clearAllFormChanges } = useFormChangeTrackerService();
const { openConfirmationModal, closeConfirmationModal } = useConfirmationModalService();

const blocker = useCallback(
Expand All @@ -18,21 +18,16 @@ export const FormChangeTrackerService: React.FC<React.PropsWithChildren<unknown>
text: "form.discardChangesConfirmation",
submitButtonText: "form.discardChanges",
onSubmit: () => {
setChangedFormsById({});
clearAllFormChanges();
closeConfirmationModal();
tx.retry();
},
});
},
[closeConfirmationModal, openConfirmationModal, setChangedFormsById]
[clearAllFormChanges, closeConfirmationModal, openConfirmationModal]
);

const formsChanged = useMemo(
() => Object.values(changedFormsById ?? {}).some((formChanged) => formChanged),
[changedFormsById]
);

useBlocker(blocker, formsChanged);
useBlocker(blocker, hasFormChanges);

return <>{children}</>;
};
37 changes: 35 additions & 2 deletions airbyte-webapp/src/hooks/services/FormChangeTracker/hooks.test.ts
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
import { renderHook } from "@testing-library/react-hooks";
import { act, renderHook } from "@testing-library/react-hooks";

import { useUniqueFormId } from "./hooks";
import { useUniqueFormId, useFormChangeTrackerService, useChangedFormsById } from "./hooks";

describe("#useUniqueFormId", () => {
it("should use what is passed into it", () => {
Expand All @@ -17,3 +17,36 @@ describe("#useUniqueFormId", () => {
expect(current).toMatch(/form_/);
});
});

describe("#useFormChangeTrackerService", () => {
afterEach(() => {
const { result } = renderHook(() => useChangedFormsById());
act(() => {
const [, setChangedFormsById] = result.current;
setChangedFormsById({});
});
});

it("hasFormChanges returns true when there are form changes", () => {
const { result } = renderHook(() => useFormChangeTrackerService());

act(() => {
result.current.trackFormChange("a", false);
result.current.trackFormChange("b", true);
result.current.trackFormChange("c", false);
});

expect(result.current.hasFormChanges).toBe(true);
});

it("hasFormChanges returns false when there are no form changes", () => {
const { result } = renderHook(() => useFormChangeTrackerService());

act(() => {
result.current.trackFormChange("a", false);
result.current.trackFormChange("c", false);
});

expect(result.current.hasFormChanges).toBe(false);
});
});
6 changes: 6 additions & 0 deletions airbyte-webapp/src/hooks/services/FormChangeTracker/hooks.ts
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,11 @@ export const useUniqueFormId = (formId?: string) => useMemo(() => formId ?? uniq
export const useFormChangeTrackerService = (): FormChangeTrackerServiceApi => {
const [changedFormsById, setChangedFormsById] = useChangedFormsById();

const hasFormChanges = useMemo<boolean>(
() => Object.values(changedFormsById ?? {}).some((changed) => !!changed),
[changedFormsById]
);

const clearAllFormChanges = useCallback(() => {
setChangedFormsById({});
}, [setChangedFormsById]);
Expand All @@ -32,6 +37,7 @@ export const useFormChangeTrackerService = (): FormChangeTrackerServiceApi => {
);

return {
hasFormChanges,
trackFormChange,
clearFormChange,
clearAllFormChanges,
Expand Down
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
export interface FormChangeTrackerServiceApi {
hasFormChanges: boolean;
trackFormChange: (id: string, changed: boolean) => void;
clearFormChange: (id: string) => void;
clearAllFormChanges: () => void;
Expand Down
4 changes: 4 additions & 0 deletions airbyte-webapp/src/locales/en.json
Original file line number Diff line number Diff line change
Expand Up @@ -371,6 +371,10 @@
"connection.linkedJobNotFound": "Job not found",
"connection.returnToSyncHistory": "Return to Sync History",

"connection.schemaChange.breaking": "Breaking schema updates detected.",
"connection.schemaChange.nonBreaking": "Non-breaking schema updates detected.",
"connection.schemaChange.reviewAction": "Review changes",

"form.frequency": "Replication frequency*",
"form.cronExpression": "Cron expression*",
"form.cronExpression.placeholder": "Cron expression",
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@ import useSyncActions from "components/EntityTable/hooks";
import { ITableDataItem } from "components/EntityTable/types";
import { getConnectionTableData } from "components/EntityTable/utils";

import { WebBackendConnectionListItem } from "../../../../../core/request/AirbyteClient";
import { WebBackendConnectionListItem } from "core/request/AirbyteClient";

interface IProps {
connections: WebBackendConnectionListItem[];
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,19 @@
@use "scss/colors";
@use "scss/variables";

.container {
display: flex;
align-items: center;
gap: variables.$spacing-md;
}

.label {
min-width: 60px;
font-size: 14px;
line-height: 19px;
font-weight: 500;
color: colors.$grey-300;
display: inline-block;
text-align: left;
cursor: pointer;
}
Loading

0 comments on commit b72cd71

Please sign in to comment.