[AN-150] FC UI migration - Adds Clone method snapshot functionality #5162
Conversation
| @@ -289,6 +293,24 @@ export const WorkflowsContainer = (props: WorkflowContainerProps) => { | |||
| setPermissionsModalOpen, | |||
| refresh: loadSnapshot, | |||
| }), | |||
| showCloneModal && | |||
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
|
@LizBaldo @sam-schu any objections if I replace the new code added in this PR to refer to |
salonishah11
changed the title
|
@salonishah11 Given that we are completing AN-240 before the official feature preview release, I am fine either way, but if we updated the language for this PR now, we would need to decide now whether we want to update all internal naming referring to "snapshot" or just user-facing language. |
|
My two cents is to do the nam change in a different PR, just in case we need to revert 😬 |
|
Based on above comments, |
There was a problem hiding this comment.
Looks good overall! All suggestions are minor. One more thing to think about that is completely up to you: maybe we should move Clone to the top of the menu now that all the menu options are always displayed, since Clone is the only option enabled for non-owners?
| ); | ||
| expect(within(dialog).getByTestId('wdl editor')).toHaveDisplayValue(mockSnapshot.payload.toString()); | ||
| }); | ||
|
|
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.)
There was a problem hiding this comment.
This test should probably cover postMethodProvider and onSuccess, but SonarCloud shows that the real onSuccess code is never being run in tests.
There was a problem hiding this comment.
I have added the suggested test.
@sam-schu regarding this I see your point. But putting Clone Snapshot at top kind of makes the flow weird with ".. snapshot", ".. snapshot permissions" in between followed by ".. snapshot". How about we move it to the bottom? Another option is to order them like this? 🤔 |
|
Jira Ticket: https://broadworkbench.atlassian.net/browse/AN-150
Summary of changes:
What
CreateMethodProviderand its functions toPostMethodProviderso that it can be used for both create new method and clone method - both use the same POST /method API callWhy
Testing strategy
Visual Aids
Snapshot menu
Clone snapshot is available even when user is not snapshot owner
Clone snapshot modal
