Fix/tasks edit #15549
Fix/tasks edit #15549
Conversation
ghost
requested review from
ebb-tide and
hoorayimhelping
and removed request for
a team
ui/src/tasks/reducers/tasks.test.ts
Outdated
| @@ -5,7 +5,8 @@ import tasksReducer, { | |||
| import {setTaskOption} from 'src/tasks/actions' | |||
| import {TaskSchedule} from 'src/utils/taskOptionsToFluxScript' | |||
|
|
|||
| describe('tasksReducer', () => { | |||
| // skipping this test since the cron and interval values should not be reset when toggling between schedule tasks | |||
| describe.skip('tasksReducer', () => { | |||
There was a problem hiding this comment.
if this asserts something that is no longer the case, or it represents outdated assumptions, i'd say just remove the whole assertion. i think keeping it around will be more confusing than helpful. i appreciate the hesitation to delete an entire file someone else wrote though :)
if we're not sure what this is supposed to do, let's get some clarification from russ.
what do you think?
There was a problem hiding this comment.
I appreciate the sympathy - definitely didn't wanna go around deleting files and taking names my second week, but it doesn't seem applicable. The only reason why I'd want to keep it would be to have a basis for fleshing out tests on the reducer later on. I'll go ahead and fix the tests so that they're consistent with the new logic of not overwriting the previous values
… the issue where task form data didn't persist when toggling between schedule task options
…rrent reducer functionality and updated the parameter name for consistency
asalem1
force-pushed
the
fix/tasks-edit
branch
from
787ed97
to
579dbda
Compare
ui/src/tasks/reducers/tasks.test.ts
Outdated
| @@ -5,11 +5,13 @@ import tasksReducer, { | |||
| import {setTaskOption} from 'src/tasks/actions' | |||
| import {TaskSchedule} from 'src/utils/taskOptionsToFluxScript' | |||
|
|
|||
| // skipping this test since the cron and interval values should not be reset when toggling between schedule tasks | |||
Closes #15534
Problem
The page would crash whenever an existing task was selected to edit. In addition, data on the task form would not populate or persist whenever a task was selected to edit / was toggled between task
Solution
The app was originally failing because of a race condition where the
currentTaskthat was needed to call thesetAllTaskOptionswas only being populate after the componentDidMount functionselectTaskByIDwas triggered. Since they were both being triggered in the CDM lifecycle, thesetAllTaskOptionswould never actually populate the data. To solve this, I created a newsetAllTaskOptionsByIDto get the task data based on theid.In doing so, this fixed our issue of data persistence in the
TaskForm. To resolve the issue of data persistence when toggling betweenschedule taskssimply required a modification to the following reducer:https://github.com/influxdata/influxdb/blob/master/ui/src/tasks/reducers/index.ts#L94
It seems like the original intent of this conditional was to prevent data persistence from mucking up the task (i.e. storing interval data and cron data may have set the data for both), but these fears seem unfounded since the data is set correctly without any unintended consequences.