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

[AN-150] FC UI migration - Adds Clone method snapshot functionality #5162

Open
wants to merge 19 commits into
base: dev
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 13 commits
Commits
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
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
import { Methods, MethodsAjaxContract } from 'src/libs/ajax/methods/Methods';
import { MethodResponse } from 'src/libs/ajax/methods/methods-models';
import { createMethodProvider } from 'src/libs/ajax/methods/providers/CreateMethodProvider';
import { postMethodProvider } from 'src/libs/ajax/methods/providers/PostMethodProvider';
import { asMockedFn, partial } from 'src/testing/test-utils';

jest.mock('src/libs/ajax/methods/Methods');
Expand Down Expand Up @@ -29,15 +29,15 @@ const mockMethodResponse: MethodResponse = {
url: 'http://agora.dsde-dev.broadinstitute.org/api/v1/methods/sschu/response-test/1',
};

describe('create method provider', () => {
it('handles create call', async () => {
describe('post method provider', () => {
it('handles post call', async () => {
// Arrange
const methodsMock = mockMethodsNeeds();
asMockedFn(methodsMock.postMethod).mockResolvedValue(mockMethodResponse);
const signal = new window.AbortController().signal;

// Act
const result = await createMethodProvider.create(
const result = await postMethodProvider.postMethod(
'input-namespace',
'input-name',
'workflow input {}',
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -2,8 +2,8 @@ import { AbortOption } from '@terra-ui-packages/data-client-core';
import { Methods } from 'src/libs/ajax/methods/Methods';
import { MethodResponse } from 'src/libs/ajax/methods/methods-models';

export interface CreateMethodProvider {
create: (
export interface PostMethodProvider {
postMethod: (
namespace: string,
name: string,
wdl: string,
Expand All @@ -14,8 +14,8 @@ export interface CreateMethodProvider {
) => Promise<MethodResponse>;
}

export const createMethodProvider: CreateMethodProvider = {
create: async (
export const postMethodProvider: PostMethodProvider = {
postMethod: async (
namespace: string,
name: string,
wdl: string,
Expand Down
6 changes: 3 additions & 3 deletions src/pages/methods/WorkflowList.test.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@ import { Ajax, AjaxContract } from 'src/libs/ajax';
import { MethodsAjaxContract } from 'src/libs/ajax/methods/Methods';
import { MethodResponse } from 'src/libs/ajax/methods/methods-models';
import { MethodDefinition } from 'src/libs/ajax/methods/methods-models';
import { createMethodProvider } from 'src/libs/ajax/methods/providers/CreateMethodProvider';
import { postMethodProvider } from 'src/libs/ajax/methods/providers/PostMethodProvider';
import * as Nav from 'src/libs/nav';
import { getLink } from 'src/libs/nav';
import { notify } from 'src/libs/notifications';
Expand Down Expand Up @@ -950,7 +950,7 @@ describe('create workflow modal', () => {
// Arrange
asMockedFn(Ajax).mockImplementation(() => mockAjax([]) as AjaxContract);

jest.spyOn(createMethodProvider, 'create').mockResolvedValue(mockCreateMethodResponse);
jest.spyOn(postMethodProvider, 'postMethod').mockResolvedValue(mockCreateMethodResponse);

const user: UserEvent = userEvent.setup();

Expand All @@ -973,7 +973,7 @@ describe('create workflow modal', () => {
await user.click(screen.getByRole('button', { name: 'Upload' }));

// Assert
expect(createMethodProvider.create).toHaveBeenCalled();
expect(postMethodProvider.postMethod).toHaveBeenCalled();

expect(Nav.goToPath).toHaveBeenCalledWith('workflow-dashboard', {
name: 'response-name',
Expand Down
4 changes: 2 additions & 2 deletions src/pages/methods/WorkflowList.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,7 @@ import { FlexTable, HeaderCell, Paginator, Sortable, TooltipCell } from 'src/com
import { TopBar } from 'src/components/TopBar';
import { Ajax } from 'src/libs/ajax';
import { MethodDefinition } from 'src/libs/ajax/methods/methods-models';
import { createMethodProvider } from 'src/libs/ajax/methods/providers/CreateMethodProvider';
import { postMethodProvider } from 'src/libs/ajax/methods/providers/PostMethodProvider';
import * as Nav from 'src/libs/nav';
import { notify } from 'src/libs/notifications';
import { useCancellation, useOnMount } from 'src/libs/react-utils';
Expand Down Expand Up @@ -291,7 +291,7 @@ export const WorkflowList = (props: WorkflowListProps) => {
<WorkflowModal
title='Create New Method'
buttonActionName='Upload'
createMethodProvider={createMethodProvider}
postMethodProvider={postMethodProvider}
onSuccess={navigateToWorkflow}
onDismiss={() => setCreateWorkflowModalOpen(false)}
/>
Expand Down
81 changes: 79 additions & 2 deletions src/pages/methods/workflow-details/WorkflowWrapper.test.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,14 @@ jest.mock('src/libs/notifications');

type NavExports = typeof import('src/libs/nav');

type WDLEditorExports = typeof import('src/workflows/methods/WDLEditor');
jest.mock('src/workflows/methods/WDLEditor', (): WDLEditorExports => {
const mockWDLEditorModule = jest.requireActual('src/workflows/methods/WDLEditor.mock');
return {
WDLEditor: mockWDLEditorModule.MockWDLEditor,
};
});

jest.mock(
'src/libs/nav',
(): NavExports => ({
Expand All @@ -37,9 +45,9 @@ const mockSnapshot: Snapshot = {
managers: ['hello@world.org'],
name: 'testname',
createDate: '2024-09-04T15:37:57Z',
documentation: '',
documentation: 'mock documentation',
entityType: 'Workflow',
snapshotComment: '',
snapshotComment: 'mock snapshot comment',
snapshotId: 1,
namespace: 'testnamespace',
payload:
Expand Down Expand Up @@ -407,6 +415,75 @@ describe('workflows container', () => {
expect(goToPath).toHaveBeenCalledWith('workflows');
});

it('renders the clone snapshot modal when the corresponding button is pressed', async () => {
// Arrange
mockAjax();

// set the user's email
jest.spyOn(userStore, 'get').mockImplementation(jest.fn().mockReturnValue(mockUserState('hello@world.org')));

const user: UserEvent = userEvent.setup();

// Act
await act(async () => {
render(
<WorkflowsContainer
namespace={mockSnapshot.namespace}
name={mockSnapshot.name}
snapshotId={`${mockSnapshot.snapshotId}`}
tabName='dashboard'
/>
);
});

await user.click(screen.getByRole('button', { name: 'Snapshot action menu' }));
await user.click(screen.getByRole('button', { name: 'Clone snapshot' }));

const dialog = screen.getByRole('dialog', { name: /clone snapshot/i });

// Assert
expect(dialog).toBeInTheDocument();
expect(within(dialog).getByRole('textbox', { name: 'Namespace *' })).toHaveDisplayValue('');
expect(within(dialog).getByRole('textbox', { name: 'Name *' })).toHaveDisplayValue('testname_copy');
expect(within(dialog).getByRole('textbox', { name: 'Documentation' })).toHaveDisplayValue('mock documentation');
expect(within(dialog).getByRole('textbox', { name: 'Synopsis (80 characters max)' })).toHaveDisplayValue('');
expect(within(dialog).getByRole('textbox', { name: 'Snapshot comment' })).toHaveDisplayValue(
'mock snapshot comment'
);
expect(within(dialog).getByTestId('wdl editor')).toHaveDisplayValue(mockSnapshot.payload.toString());
});

Copy link
Contributor

Choose a reason for hiding this comment

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

Could you please add a test similar to WorkflowList.test.tsx > create workflow modal > uploads a new workflow and navigates to its workflow details page to check that the right values were given for the postMethodProvider and onSuccess props of the clone modal?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Do you think that test case would be covered by this test? This test checks that the postMethodProvider has been called with correct values on submit.

Copy link
Contributor

Choose a reason for hiding this comment

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

That test is for the modal component itself, and as far as I can tell doesn't really test anything not already tested by the existing tests in that file (which makes sense since the only changes to the modal component were naming changes). The test I am suggesting would be for the WorkflowWrapper, to make sure that the right functions are called by the clone modal specifically as used in the workflow wrapper. (For onSuccess, for example: for the create workflow modal, the tests for WorkflowModal check that whatever function was given for onSuccess will be called, and the tests for WorkflowList check that specifically Nav.goToPath is called by the actual modal used on the workflow list page.)

Copy link
Contributor

Choose a reason for hiding this comment

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

This test should probably cover postMethodProvider and onSuccess, but SonarCloud shows that the real onSuccess code is never being run in tests.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have added the suggested test.

it('hides the clone snapshot modal when it is dismissed', async () => {
// Arrange
mockAjax();

// set the user's email
jest.spyOn(userStore, 'get').mockImplementation(jest.fn().mockReturnValue(mockUserState('hello@world.org')));

const user: UserEvent = userEvent.setup();

// Act
await act(async () => {
render(
<WorkflowsContainer
namespace={mockSnapshot.namespace}
name={mockSnapshot.name}
snapshotId={`${mockSnapshot.snapshotId}`}
tabName='dashboard'
/>
);
});

await user.click(screen.getByRole('button', { name: 'Snapshot action menu' }));
await user.click(screen.getByRole('button', { name: 'Clone snapshot' }));
await user.click(screen.getByRole('button', { name: 'Cancel' }));

// Assert
const dialog = screen.queryByRole('dialog', { name: /clone snapshot/i });

expect(dialog).not.toBeInTheDocument();
});

it('hides the delete snapshot modal and displays a loading spinner when the deletion is confirmed', async () => {
// Arrange
mockAjax({
Expand Down
29 changes: 29 additions & 0 deletions src/pages/methods/workflow-details/WorkflowWrapper.ts
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@ import { TabBar } from 'src/components/tabBars';
import { TopBar } from 'src/components/TopBar';
import { Ajax } from 'src/libs/ajax';
import { Snapshot } from 'src/libs/ajax/methods/methods-models';
import { postMethodProvider } from 'src/libs/ajax/methods/providers/PostMethodProvider';
import { makeExportWorkflowFromMethodsRepoProvider } from 'src/libs/ajax/workspaces/providers/ExportWorkflowToWorkspaceProvider';
import { ErrorCallback, withErrorReporting } from 'src/libs/error';
import * as Nav from 'src/libs/nav';
Expand All @@ -19,6 +20,7 @@ import * as Utils from 'src/libs/utils';
import { withBusyState } from 'src/libs/utils';
import DeleteSnapshotModal from 'src/workflows/methods/modals/DeleteSnapshotModal';
import { PermissionsModal } from 'src/workflows/methods/modals/PermissionsModal';
import { WorkflowModal } from 'src/workflows/methods/modals/WorkflowModal';
import SnapshotActionMenu from 'src/workflows/methods/SnapshotActionMenu';
import ExportWorkflowModal from 'src/workflows/modals/ExportWorkflowModal';
import { isGoogleWorkspace, WorkspaceInfo, WorkspaceWrapper } from 'src/workspaces/utils';
Expand Down Expand Up @@ -122,6 +124,7 @@ export const WorkflowsContainer = (props: WorkflowContainerProps) => {
const [snapshotNotFound, setSnapshotNotFound] = useState<boolean>(false);
const [exportingWorkflow, setExportingWorkflow] = useState<boolean>(false);
const [showDeleteModal, setShowDeleteModal] = useState<boolean>(false);
const [showCloneModal, setShowCloneModal] = useState<boolean>(false);
const [busy, setBusy] = useState<boolean>(false);
const [permissionsModalOpen, setPermissionsModalOpen] = useState<boolean>(false);

Expand Down Expand Up @@ -239,6 +242,7 @@ export const WorkflowsContainer = (props: WorkflowContainerProps) => {
isSnapshotOwner,
onEditPermissions: () => setPermissionsModalOpen(true),
onDelete: () => setShowDeleteModal(true),
onClone: () => setShowCloneModal(true),
}),
]),
]
Expand Down Expand Up @@ -289,6 +293,31 @@ export const WorkflowsContainer = (props: WorkflowContainerProps) => {
setPermissionsModalOpen,
refresh: loadSnapshot,
}),
showCloneModal &&
Copy link
Collaborator

Choose a reason for hiding this comment

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

One thing to maybe discuss during the UX review today, is that the button shows clone snapshot, but then it opens a clone method modal 🤔
It does not seem like there is a difference between a snapshot or a method, is there?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That is a good point. It is actually cloning the snapshot (of that method) that is selected from the dropdown. I think it might make more sense to call everything Clone snapshot instead. I am going to change that and based on the outcome of UX discussion it can be changed again later.

h(WorkflowModal, {
title: 'Clone snapshot',
defaultName: name.concat('_copy'),
defaultWdl: snapshot?.payload,
salonishah11 marked this conversation as resolved.
Show resolved Hide resolved
defaultDocumentation: snapshot?.documentation,
defaultSynopsis: snapshot?.synopsis,
defaultSnapshotComment: snapshot?.snapshotComment,
salonishah11 marked this conversation as resolved.
Show resolved Hide resolved
buttonActionName: 'Clone snapshot',
salonishah11 marked this conversation as resolved.
Show resolved Hide resolved
postMethodProvider,
onSuccess: (namespace: string, name: string, snapshotId: number) => {
// when the user has owner permissions on the original method, there is an interesting situation where
// if the user types in the same namespace and name for the cloned method as the original method,
// instead of creating a new method Agora will create a new snapshot of the original method.
// Hence, to ensure the data is correct in the UI we reset the cached snapshot list store and then load the page.
// (Note: this behaviour is same as in Firecloud UI)
snapshotsListStore.reset();
Nav.goToPath('workflow-dashboard', {
namespace,
name,
snapshotId,
});
},
onDismiss: () => setShowCloneModal(false),
}),
busy && spinnerOverlay,
snapshotNotFound && h(NotFoundMessage, { subject: 'snapshot' }),
snapshot && div({ style: { flex: 1, display: 'flex', flexDirection: 'column' } }, [children]),
Expand Down
Loading
Loading