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

🪟 🐛 Fix loading state of "Launch" button in connection table #22002

Merged
merged 10 commits into from
Jan 31, 2023
6 changes: 2 additions & 4 deletions airbyte-webapp/src/components/EntityTable/ConnectionTable.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -22,10 +22,9 @@ interface IProps {
data: ITableDataItem[];
entity: "source" | "destination" | "connection";
onClickRow?: (data: ITableDataItem) => void;
onSync: (id: string) => void;
}

const ConnectionTable: React.FC<IProps> = ({ data, entity, onClickRow, onSync }) => {
const ConnectionTable: React.FC<IProps> = ({ data, entity, onClickRow }) => {
const navigate = useNavigate();
const query = useQuery<{ sortBy?: string; order?: SortOrderEnum }>();
const allowAutoDetectSchema = useFeature(FeatureItem.AllowAutoDetectSchema);
Expand Down Expand Up @@ -165,7 +164,6 @@ const ConnectionTable: React.FC<IProps> = ({ data, entity, onClickRow, onSync })
id={row.original.connectionId}
isSyncing={row.original.isSyncing}
isManual={row.original.scheduleType === ConnectionScheduleType.manual}
onSync={onSync}
hasBreakingChange={allowAutoDetectSchema && row.original.schemaChange === SchemaChange.breaking}
allowSync={allowSync}
/>
Expand All @@ -178,7 +176,7 @@ const ConnectionTable: React.FC<IProps> = ({ data, entity, onClickRow, onSync })
Cell: ({ cell }: CellProps<ITableDataItem>) => <ConnectionSettingsCell id={cell.value} />,
},
],
[sortBy, sortOrder, entity, onSortClick, onSync, allowSync, allowAutoDetectSchema]
[sortBy, sortOrder, entity, onSortClick, allowSync, allowAutoDetectSchema]
);

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

import { StatusCell } from "./StatusCell";

jest.mock("hooks/services/useConnectionHook", () => ({
useConnectionList: jest.fn(() => ({
connections: [],
})),
useEnableConnection: jest.fn(() => ({
mutateAsync: jest.fn(),
})),
useSyncConnection: jest.fn(() => ({
mutateAsync: jest.fn(),
})),
}));

const mockId = "mock-id";

jest.doMock("hooks/services/useConnectionHook", () => ({
Expand All @@ -14,35 +26,52 @@ jest.doMock("hooks/services/useConnectionHook", () => ({

describe("<StatusCell />", () => {
it("renders switch when connection has schedule", () => {
const { getByTestId } = render(<StatusCell id={mockId} onSync={jest.fn()} allowSync enabled />, {
wrapper: TestWrapper,
});
const { getByTestId } = render(
<TestSuspenseBoundary>
<StatusCell id={mockId} allowSync enabled />
</TestSuspenseBoundary>,
{
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,
});
it("renders button when connection does not have schedule", async () => {
const { getByTestId } = render(
<TestSuspenseBoundary>
<StatusCell id={mockId} allowSync enabled isManual />
</TestSuspenseBoundary>,
{
wrapper: TestWrapper,
}
);

expect(getByTestId("manual-sync-button")).toBeEnabled();
await waitFor(() => 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,
});
const { getByTestId } = render(
<TestSuspenseBoundary>
<StatusCell id={mockId} allowSync hasBreakingChange />
</TestSuspenseBoundary>,
{
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 />,
<TestSuspenseBoundary>
<StatusCell id={mockId} allowSync hasBreakingChange enabled isManual />
</TestSuspenseBoundary>,
{
wrapper: TestWrapper,
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,6 @@ interface StatusCellProps {
isSyncing?: boolean;
isManual?: boolean;
id: string;
onSync: (id: string) => void;
schemaChange?: SchemaChange;
}

Expand All @@ -23,7 +22,6 @@ export const StatusCell: React.FC<StatusCellProps> = ({
isManual,
id,
isSyncing,
onSync,
allowSync,
schemaChange,
hasBreakingChange,
Expand All @@ -37,7 +35,6 @@ export const StatusCell: React.FC<StatusCellProps> = ({
id={id}
isSyncing={isSyncing}
isManual={isManual}
onSync={onSync}
hasBreakingChange={hasBreakingChange}
allowSync={allowSync}
/>
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
@use "scss/variables";

.inProgressLabel {
height: variables.$button-height-xs;
display: flex;
align-items: center;
}
Original file line number Diff line number Diff line change
@@ -1,12 +1,12 @@
import React from "react";
import { FormattedMessage } from "react-intl";
import { useAsyncFn } from "react-use";
import styled from "styled-components";

import { Button } from "components/ui/Button";
import { Switch } from "components/ui/Switch";

import { useEnableConnection } from "hooks/services/useConnectionHook";
import { useConnectionList, useEnableConnection, useSyncConnection } from "hooks/services/useConnectionHook";

import styles from "./StatusCellControl.module.scss";

interface StatusCellControlProps {
allowSync?: boolean;
Expand All @@ -15,30 +15,28 @@ interface StatusCellControlProps {
isSyncing?: boolean;
isManual?: boolean;
id: string;
onSync: (id: string) => void;
}

const ProgressMessage = styled.div`
padding: 7px 0;
`;

export const StatusCellControl: React.FC<StatusCellControlProps> = ({
enabled,
isManual,
id,
isSyncing,
onSync,
allowSync,
hasBreakingChange,
}) => {
const { connections } = useConnectionList();
const { mutateAsync: enableConnection, isLoading } = useEnableConnection();
const [{ loading }, OnLaunch] = useAsyncFn(
async (event: React.SyntheticEvent) => {
event.stopPropagation();
onSync(id);
},
[id]
);
const { mutateAsync: syncConnection, isLoading: isSyncStarting } = useSyncConnection();

const onRunManualSync = (event: React.SyntheticEvent) => {
event.stopPropagation();

const connection = connections.find((c) => c.connectionId === id);
if (connection) {
syncConnection(connection);
}
};
Comment on lines +32 to +39
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This function was basically constructed in three places before (SourceConnectionTable, DestinationConnectionTable and ConnectionTable) and drilled down as prop called onSync. Instead we can just create the function here and remove all that intermediate code.


if (!isManual) {
const onSwitchChange = async (event: React.SyntheticEvent) => {
Expand Down Expand Up @@ -69,21 +67,20 @@ export const StatusCellControl: React.FC<StatusCellControlProps> = ({

if (isSyncing) {
return (
<ProgressMessage>
<FormattedMessage id="tables.progress" />
</ProgressMessage>
<div className={styles.inProgressLabel}>
<FormattedMessage id="connection.syncInProgress" />
</div>
);
}

return (
<Button
size="xs"
onClick={OnLaunch}
isLoading={loading}
disabled={!allowSync || !enabled || hasBreakingChange}
onClick={onRunManualSync}
isLoading={isSyncStarting}
disabled={!allowSync || !enabled || hasBreakingChange || isSyncStarting}
data-testid="manual-sync-button"
>
<FormattedMessage id="tables.launch" />
<FormattedMessage id="connection.startSync" />
</Button>
);
};
16 changes: 0 additions & 16 deletions airbyte-webapp/src/components/EntityTable/hooks.tsx

This file was deleted.

Original file line number Diff line number Diff line change
@@ -1,8 +1,6 @@
@use "scss/variables";
@use "scss/colors";

$buttonHeight: 36px;

.container {
padding: variables.$spacing-lg;
height: 100%;
Expand All @@ -19,7 +17,7 @@ $buttonHeight: 36px;

.streamSelector {
flex: 0 0 auto;
height: $buttonHeight;
height: variables.$button-height-sm;
}

.selectAndTestContainer {
Expand Down
Original file line number Diff line number Diff line change
@@ -1,8 +1,7 @@
import React, { useCallback } from "react";
import React from "react";
import { useNavigate } from "react-router-dom";

import { ConnectionTable } from "components/EntityTable";
import useSyncActions from "components/EntityTable/hooks";
import { ITableDataItem } from "components/EntityTable/types";
import { getConnectionTableData } from "components/EntityTable/utils";

Expand All @@ -17,25 +16,14 @@ interface IProps {

export const DestinationConnectionTable: React.FC<IProps> = ({ connections }) => {
const navigate = useNavigate();
const { syncManualConnection } = useSyncActions();

const data = getConnectionTableData(connections, "destination");

const onSync = useCallback(
async (connectionId: string) => {
const connection = connections.find((item) => item.connectionId === connectionId);
if (connection) {
await syncManualConnection(connection);
}
},
[connections, syncManualConnection]
);

const clickRow = (source: ITableDataItem) => navigate(`../../../${RoutePaths.Connections}/${source.connectionId}`);

return (
<div className={styles.content}>
<ConnectionTable data={data} onClickRow={clickRow} entity="destination" onSync={onSync} />
<ConnectionTable data={data} onClickRow={clickRow} entity="destination" />
</div>
);
};
6 changes: 3 additions & 3 deletions airbyte-webapp/src/components/ui/Button/Button.module.scss
Original file line number Diff line number Diff line change
Expand Up @@ -106,7 +106,7 @@
}

&.sizeXS {
height: 32px;
height: variables.$button-height-xs;
font-size: 12px;
line-height: 15px;
padding: 10px;
Expand All @@ -117,7 +117,7 @@
}

&.sizeS {
height: 36px;
height: variables.$button-height-sm;
font-size: 12px;
line-height: 15px;
padding: 10px 14px;
Expand All @@ -128,7 +128,7 @@
}

&.sizeL {
height: 44px;
height: variables.$button-height-lg;
font-size: 16px;
line-height: 19px;
padding: 10px 14px;
Expand Down
Loading