[AN-247] Edit method functionality #5175
Conversation
| allConfigs: async () => { | ||
| const res = await fetchAgora(`methods/${namespace}/${name}/configurations`, _.merge(authOpts(), { signal })); | ||
| return res.json(); | ||
| }, | ||
|
|
There was a problem hiding this comment.
This wasn't used anywhere so I removed it.
|
Sonarcloud reported one below issue. I am not sure how strict we want to enforce it because reducing the number of arguments would entail creating a new type for passing arguments. Plus Terra UI does have other functions with more than allowed 7 arguments. But I would like to reviewers opinion on this.
|
|
@salonishah11 Regarding the SonarCloud issue, I have no problem with this function's having 9 arguments. All of its arguments are necessary for making the Ajax request, and if the caller had to construct the payload object themselves at the call site so that it could be passed as one argument to the provider function, that would defeat part of the purpose of the provider abstraction layer. |
| // Act | ||
| await act(async () => { | ||
| render( | ||
| <EditWorkflowModal |
There was a problem hiding this comment.
Can we also check that the title and snapshot ID are rendered, and non-empty defaults for the synopsis and snapshot comment?
There was a problem hiding this comment.
I have added the check for title, snapshot ID and non-empty synopsis.
Your comment made me realize that snapshot comment in any action (create new method, clone method or edit method) doesn't have a default value because
- it doesn't make sense for previous snapshot's comment to be automatically applied to new snapshot
- makes user deliberately write new comment if they want to
- replicates FC UI's current behavior
Hence I have removed defaultSnapshotComment from props and associated files, tests.
| expect(screen.getByRole('textbox', { name: 'Snapshot comment' })).toHaveDisplayValue(''); | ||
| }); | ||
|
|
||
| it('successfully creates a new snapshot with inputted information', async () => { |
There was a problem hiding this comment.
It wouldn't hurt to add a test that makes sure that by default redactPreviousSnapshot is set to false in the provider function call, given the consequences if it were always true regardless of whether the checkbox was checked.
There was a problem hiding this comment.
Good point. I have added that test.
|
Jira Ticket: https://broadworkbench.atlassian.net/browse/AN-247
Summary of changes:
What
Adds "Edit method" functionality
Why
FC UI migration
TODO
Testing strategy
Action menu
Edit modal
