Conversation
| handleFormChange(name, value) { | ||
| const formState = this.state.formState; | ||
| const formState = Utils.deepClone(this.state.formState); | ||
| formState[name] = value; |
There was a problem hiding this comment.
This was mutating the existing state, which is bad and was causing problems.
|
LGTM |
| if (_.isEqual(this.state.formState, getDefaultFormState(this.props))) { | ||
| this.setState({formState: getDefaultFormState(newProps)}); | ||
| } | ||
| } |
There was a problem hiding this comment.
can you explain what this method does?
There was a problem hiding this comment.
What it does: If the new props set a new default form state, and the form hasn't been changed off of the default, this will update the current value of the form to the new default value.
Why it's needed: The default value of the command line arguments isn't always present when the modal is initially rendered on page load. When the run now button is pressed, the last task run (or the task to be rerun) is loaded, then the default command line args are taken from that task. The value of the field then needs to be updated with these command line arguments.
There was a problem hiding this comment.
I'm curious why the command line args aren't always present. How is that data coming in?
There was a problem hiding this comment.
The command line args are taken from:
- The task being rerun, if you are rerunning a task.
- Else the last task that was completed on that request.
Right now, to find the command line arguments, it's necessary to load the task's information. That's fine on the request detail page, as the task is already loaded (the command line args are part of the data retrieved while loading the task history table).
But on the requests page we don't load the task detail. At some point we need to load the detail of the last task run, but it doesn't make sense to do that before rendering (there could be thousands of requests, and we don't want to send an API call for each of them). So instead that call is only sent when a user clicks the run now button.
|
LGTM aside from my comments |
4 participants
Implements the following changes: