chore(ui): refactored tasks to use oats rather than client library #16353

asalem1 · 2019-12-30T16:17:02Z

Problem

The client library is deprecated and no longer being supported, need to transition api routes based on the oats generatedRoutes

Solution

Revised the client api calls that directly correlate to tasks to use the generatedRoutes. Added defaults for labels where applicable in order to refactor this in stages and prevent TS errors

ui/src/types/logEvent.ts

asalem1

No defaults were added to the tasks whenever the task was not returned / set onto the client.

asalem1 · 2019-12-30T16:18:49Z

ui/src/tasks/actions/index.ts

@@ -283,7 +308,7 @@ export const removeTaskLabelsAsync = (

 export const updateTaskStatus = (task: Task) => async dispatch => {
  try {
-    await client.tasks.updateStatus(task.id, task.status)
+    await patchTask({taskID: task.id, data: {status: task.status}})


Since this only updates the status (and does not pass along any information relating the cron, every specifics), I did not run a check to see whether or not the cron / every props needed to be updated

We should check response status and throw an error if response status is not 200.

Also notice that this function now works very differently than it used to in the other client. (which is fine if it works, but just need to be aware!) The one in the old client would make a get req. to task, and send all of the parameters whether or not they were updated. Make sure you are testing functionality! :)

asalem1 · 2019-12-30T16:18:57Z

ui/src/tasks/actions/index.ts

@@ -296,7 +321,7 @@ export const updateTaskStatus = (task: Task) => async dispatch => {

 export const updateTaskName = (task: Task) => async dispatch => {
  try {
-    await client.tasks.update(task.id, task)
+    await patchTask({taskID: task.id, data: task})


Since this only updates the name (and does not pass along any information relating the cron, every specifics), I did not run a check to see whether or not the cron / every props needed to be updated

asalem1 · 2019-12-30T16:20:33Z

ui/src/tasks/actions/index.ts

@@ -415,7 +462,7 @@ export const updateScript = () => async (dispatch, getState: GetStateFunc) => {
      updatedTask.every = null
    }

-    await client.tasks.update(task.id, updatedTask)
+    await patchTask({taskID: task.id, data: updatedTask})


The UI is already explicitly setting cron and every to null values if the other value is set:

https://github.com/influxdata/influxdb/pull/16353/files#diff-3084610c031fd81910381dcc8507c9fcR457-R463

asalem1 · 2019-12-30T16:21:25Z

ui/src/tasks/actions/index.ts

@@ -252,12 +267,18 @@ export const getTasks = () => async (
  }
 }

-export const addTaskLabelsAsync = (taskID: string, labels: Label[]) => async (
+export const addTaskLabelAsync = (taskID: string, label: Label) => async (


renamed this to accurately reflect its functionality since only one label is ever passed

this makes me really happy

asalem1 · 2019-12-30T16:21:30Z

ui/src/tasks/actions/index.ts

-  taskID: string,
-  labels: Label[]
-) => async (dispatch): Promise<void> => {
+export const removeTaskLabelAsync = (taskID: string, label: Label) => async (


renamed this to accurately reflect its functionality since only one label is ever passed

ebb-tide

I've made a bunch of comments, the most important is the one about view task runs button not working will review again tomorrow! :)

ebb-tide · 2019-12-30T17:06:21Z

ui/src/tasks/actions/index.ts

  importTaskFailed,
  importTaskSucceeded,
 } from 'src/shared/copy/notifications'
 import {createTaskFromTemplate as createTaskFromTemplateAJAX} from 'src/templates/api'
-
+import {addDefaults} from 'src/templates/utils/'


I would expect to find this function in ui/src/tasks/actions/index.ts. just cause it is so closely tied with the API calls being made in this function. it could also possibly go in src/tasks/utils but it is not related to templates.

ebb-tide · 2019-12-30T17:07:52Z

ui/src/tasks/actions/index.ts

@@ -1,10 +1,9 @@
 // Libraries
 import {push, goBack} from 'react-router-redux'
 import _ from 'lodash'
-
+// Type
+import {LogEvent, Task} from 'src/types'


There is also a Types section lower in this file- could we combine them?

ebb-tide · 2019-12-30T17:20:29Z

ui/src/tasks/actions/index.ts

+  postTask,
+  postTasksLabel,
+  postTasksRun,
+} from 'src/client'


hrrm.. so in other /actions/ files (such as in ui/src/alerting/actions/notifications/endpoints.ts) we have
import * as api from 'src/client' at the top of the file, and then each function call is api.getNotificationEndpoints I personally like that pattern, since it communicates immediately that "this is an api function". I think it's an acceptable alternative to alias functions like you are doing with some here: deleteTask as apiDeleteTask. Uniformity in one file is good (ie. if you're going to alias one function, it would be better to alias all the functions), it would be even better to have uniformity in the app of how we use the client. by using the api.FunctionName pattern everywhere.

I hear what you're saying and had originally set out to use the same manner of importing and using the client routes, however @drdelambre brought up the point of being explicit and I felt it to be a good convention to follow:

#16325 (comment)

At the end of the day, this is a matter of preference, and while having an identifier like api before the function call name is a bit more indicative of the functionality we're performing, I don't think that importing the entire file to use one function from that file should serve that purpose. I am happy to alias the functions to their apiFunctionName though since that would help provide greater clarity on what the purpose of the functions are

ok apiFunctionName it is then, yeah- opinions may differ :)

ui/src/tasks/actions/index.ts

ebb-tide · 2019-12-30T17:30:10Z

ui/src/types/tasks.ts

  lastRunError?: string
  lastRunStatus?: 'failed' | 'success' | 'canceled'


We can remove these two lines since lastRunError and lastRunStatus are already in ITask

ebb-tide · 2019-12-31T01:28:48Z

ui/src/tasks/actions/index.ts

-    await client.tasks.clone(task.id)
+    const resp = await apiGetTask({taskID: task.id})
+
+    if (resp.status !== 200 || resp.data.orgID === undefined) {


whats the reason for this orgID check?

That's a great question. No idea anymore. Gonna remove it

ebb-tide · 2019-12-31T01:57:25Z

ui/src/tasks/actions/index.ts

@@ -296,7 +321,7 @@ export const updateTaskStatus = (task: Task) => async dispatch => {

 export const updateTaskName = (task: Task) => async dispatch => {


this is such a weird function! I know you didn't write it and you don't have to change it, but it would be so much cleaner if it were a function of (id, name) and only updated name and nothing else?

Right, I totally agree! Especially since we are literally doing just that where it's being passed in:

https://github.com/influxdata/influxdb/blob/master/ui/src/tasks/components/TaskCard.tsx#L155-L158

refactoring it now

ebb-tide · 2019-12-31T01:58:18Z

ui/src/tasks/actions/index.ts

@@ -309,7 +334,7 @@ export const updateTaskName = (task: Task) => async dispatch => {

 export const deleteTask = (task: Task) => async dispatch => {
  try {
-    await client.tasks.delete(task.id)
+    await apiDeleteTask({taskID: task.id})


need to check for error

ebb-tide · 2019-12-31T02:14:16Z

ui/src/tasks/actions/index.ts

+
+    let [runs] = await Promise.all([dispatch(selectTaskByID(taskID))])
+
+    runs = runs.concat(resp.data)


selectTaskByID doesn't return anything, so runs is always undefined. and concat on undefined errors.
try dispatch selectTaskByID above awaiting getTasksRuns.
The view task runs button on tasks does not work after this change. We don't have enough tests, and maybe that's the problem, but if we don't have tests, we can't move this fast. :/ Could you please check every place where each function you are changing is being used in the app? Let's try to get this PR through with no regressions! :)

I truthfully hadn't even noticed that. Thanks for pointing that out!

ebb-tide · 2019-12-31T02:16:05Z

ui/src/tasks/components/TaskRunsPage.tsx

-import {RemoteDataState} from 'src/types'
-import {Run as APIRun, Task} from '@influxdata/influx'
+import {AppState, RemoteDataState, Task} from 'src/types'
+import {Run as APIRun} from '@influxdata/influx'


Would it be possible to eliminate relying on @influxdata/influx for Run types as well?

Definitely.

…finish tasks actions

…s returned from the api

ebb-tide

woot- just make sure you click around test everything that has to do with tasks to ensure everything works and then merge!

ebb-tide · 2019-12-31T19:53:57Z

ui/src/shared/utils/mocks/resourceToTemplate.ts

@@ -99,7 +99,7 @@ export const myfavetask: Task = {
  offset: '1m0s',
  org: 'org',
  orgID: '037b084ec8ebc000',
-  status: TaskStatus.Active,
+  status: 'active',


ebb-tide · 2019-12-31T20:06:30Z

ui/src/tasks/components/TaskCard.tsx

-    if (task.status === TaskStatus.Active) {
-      task.status = TaskStatus.Inactive
+    if (task.status === 'active') {
+      task.status = 'inactive'


ebb-tide · 2019-12-31T20:06:40Z

ui/src/tasks/components/TaskRunsList.tsx

@@ -8,7 +8,7 @@ import TaskRunsRow from 'src/tasks/components/TaskRunsRow'

 // Types
 import {Sort, ComponentSize} from '@influxdata/clockface'
-import {Run} from 'src/tasks/components/TaskRunsPage'
+import {Run} from 'src/types'


…16353) chore(ui): refactoring tasks to use oats rather than client, need to finish tasks actions

asalem1 requested review from ebb-tide and a team December 30, 2019 16:17

chnn mentioned this pull request Dec 30, 2019

Move API client generation into influxdb #14482

Open

19 tasks

ebb-tide reviewed Dec 30, 2019

View reviewed changes

ui/src/types/logEvent.ts Outdated Show resolved Hide resolved

asalem1 commented Dec 30, 2019

View reviewed changes

ghost requested review from ebb-tide and TCL735 and removed request for a team December 30, 2019 16:34

ebb-tide suggested changes Dec 31, 2019

View reviewed changes

asalem1 requested a review from ebb-tide December 31, 2019 15:32

asalem1 added 5 commits December 31, 2019 07:44

chore(ui): refactoring tasks to use oats rather than client, need to …

1633b34

…finish tasks actions

chore(ui): refactored tasks to use oats rather than client library

0cd9414

fix(ui): updated parameter to resolve lint error

46154a0

fix(ui): fixed e2e tests, updated errorMessage func to handle message…

a2b0438

…s returned from the api

fix(ui): updated PR to align with requested changes

488a2e2

asalem1 force-pushed the task-client-chore branch from af800b4 to 488a2e2 Compare December 31, 2019 15:44

chore(ui): finished type casting functions for greater clarity

7e34126

ebb-tide approved these changes Dec 31, 2019

View reviewed changes

asalem1 merged commit 18d2bf6 into master Dec 31, 2019

asalem1 deleted the task-client-chore branch December 31, 2019 20:35

alexpaxton pushed a commit that referenced this pull request Jan 9, 2020

chore(ui): refactored tasks to use oats rather than client library (#…

7a81952

…16353) chore(ui): refactoring tasks to use oats rather than client, need to finish tasks actions

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

chore(ui): refactored tasks to use oats rather than client library #16353

chore(ui): refactored tasks to use oats rather than client library #16353

asalem1 commented Dec 30, 2019

asalem1 left a comment

asalem1 Dec 30, 2019

ebb-tide Dec 31, 2019

ebb-tide Dec 31, 2019

asalem1 Dec 30, 2019

asalem1 Dec 30, 2019

asalem1 Dec 30, 2019

ebb-tide Dec 31, 2019

asalem1 Dec 30, 2019

ebb-tide left a comment

ebb-tide Dec 30, 2019

ebb-tide Dec 30, 2019

ebb-tide Dec 30, 2019

asalem1 Dec 31, 2019

ebb-tide Dec 31, 2019

ebb-tide Dec 30, 2019

ebb-tide Dec 31, 2019

asalem1 Dec 31, 2019

ebb-tide Dec 31, 2019

asalem1 Dec 31, 2019 •

edited

Loading

ebb-tide Dec 31, 2019

ebb-tide Dec 31, 2019 •

edited

Loading

asalem1 Dec 31, 2019

ebb-tide Dec 31, 2019

asalem1 Dec 31, 2019

ebb-tide left a comment

ebb-tide Dec 31, 2019

ebb-tide Dec 31, 2019

ebb-tide Dec 31, 2019

		lastRunError?: string
		lastRunStatus?: 'failed' \| 'success' \| 'canceled'

		@@ -296,7 +321,7 @@ export const updateTaskStatus = (task: Task) => async dispatch => {

		export const updateTaskName = (task: Task) => async dispatch => {


		let [runs] = await Promise.all([dispatch(selectTaskByID(taskID))])

		runs = runs.concat(resp.data)

chore(ui): refactored tasks to use oats rather than client library #16353

chore(ui): refactored tasks to use oats rather than client library #16353

Conversation

asalem1 commented Dec 30, 2019

Problem

Solution

asalem1 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ebb-tide left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

asalem1 Dec 31, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ebb-tide Dec 31, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ebb-tide left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

asalem1 Dec 31, 2019 •

edited

Loading

ebb-tide Dec 31, 2019 •

edited

Loading