Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 6 commits into from
Dec 31, 2019
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 14 additions & 0 deletions ui/src/labels/utils/index.ts
Original file line number Diff line number Diff line change
@@ -1,4 +1,6 @@
import _ from 'lodash'
import {Label as GenLabel} from 'src/client'
import {Label} from 'src/types'

import {HEX_CODE_CHAR_LENGTH, PRESET_LABEL_COLORS} from 'src/labels/constants/'

Expand Down Expand Up @@ -54,3 +56,15 @@ export const validateHexCode = (colorHex: string): string | null => {

return errorMessage.join(', ')
}

const DEFAULT_LABEL_COLOR = '#326BBA'

export const addLabelDefaults = (l: GenLabel): Label => ({
...l,
properties: {
...l.properties,
// add default color hex if missing
color: (l.properties || {}).color || DEFAULT_LABEL_COLOR,
description: (l.properties || {}).description || '',
},
})
4 changes: 2 additions & 2 deletions ui/src/shared/utils/mocks/resourceToTemplate.ts
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
import {Task, Dashboard, View, Label, TaskStatus} from 'src/types'
import {Task, Dashboard, View, Label} from 'src/types'
import {IVariable as Variable} from '@influxdata/influx'

export const myDashboard: Dashboard = {
Expand Down Expand Up @@ -99,7 +99,7 @@ export const myfavetask: Task = {
offset: '1m0s',
org: 'org',
orgID: '037b084ec8ebc000',
status: TaskStatus.Active,
status: 'active',
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

❤️

}

export const myVariable: Variable = {
Expand Down
156 changes: 120 additions & 36 deletions ui/src/tasks/actions/index.ts
Original file line number Diff line number Diff line change
@@ -1,10 +1,7 @@
// Libraries
import {push, goBack} from 'react-router-redux'
import _ from 'lodash'

// APIs
import {LogEvent, ITask as Task} from '@influxdata/influx'
import {client} from 'src/utils/api'
import {notify} from 'src/shared/actions/notifications'
import {
taskNotCreated,
Expand All @@ -19,23 +16,33 @@ import {
taskCloneFailed,
taskRunSuccess,
taskGetFailed,
} from 'src/shared/copy/notifications'
import {
importTaskFailed,
importTaskSucceeded,
} from 'src/shared/copy/notifications'
import {createTaskFromTemplate as createTaskFromTemplateAJAX} from 'src/templates/api'

import {addLabelDefaults} from 'src/labels/utils'
import {
deleteTask as apiDeleteTask,
deleteTasksLabel,
getTask as apiGetTask,
getTasks as apiGetTasks,
getTasksRuns as apiGetTasksRuns,
getTasksRunsLogs as apiGetTasksRunsLogs,
patchTask as apiPatchTask,
postTask as apiPostTask,
postTasksLabel as apiPostTasksLabel,
postTasksRun as apiPostTasksRun,
} from 'src/client'
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

// Actions
import {setExportTemplate} from 'src/templates/actions'

// Constants
import * as copy from 'src/shared/copy/notifications'

// Types
import {AppState, Label, TaskTemplate} from 'src/types'
import {AppState, Label, TaskTemplate, LogEvent, Run, Task} from 'src/types'
import {RemoteDataState} from '@influxdata/clockface'
import {Run} from 'src/tasks/components/TaskRunsPage'
import {Task as ITask} from 'src/client'

// Utils
import {getErrorMessage} from 'src/utils/api'
Expand Down Expand Up @@ -162,6 +169,13 @@ export interface UpdateTask {
}
}

export const addDefaults = (task: ITask): Task => {
return {
...task,
labels: (task.labels || []).map(addLabelDefaults),
}
}

export const setTaskOption = (taskOption: {
key: TaskOptionKeys
value: string
Expand Down Expand Up @@ -240,7 +254,12 @@ export const getTasks = () => async (
orgs: {org},
} = getState()

const tasks = await client.tasks.getAll(org.id)
const resp = await apiGetTasks({query: {orgID: org.id}})
if (resp.status !== 200) {
throw new Error(resp.data.message)
}

const tasks = resp.data.tasks.map(task => addDefaults(task))

dispatch(setTasks(tasks))
dispatch(setTasksStatus(RemoteDataState.Done))
Expand All @@ -252,12 +271,18 @@ export const getTasks = () => async (
}
}

export const addTaskLabelsAsync = (taskID: string, labels: Label[]) => async (
export const addTaskLabelAsync = (taskID: string, label: Label) => async (
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this makes me really happy

dispatch
): Promise<void> => {
try {
await client.tasks.addLabels(taskID, labels.map(l => l.id))
const task = await client.tasks.get(taskID)
await apiPostTasksLabel({taskID, data: {labelID: label.id}})
const resp = await apiGetTask({taskID})

if (resp.status !== 200) {
throw new Error(resp.data.message)
}

const task = addDefaults(resp.data)

dispatch(updateTask(task))
} catch (error) {
Expand All @@ -266,13 +291,17 @@ export const addTaskLabelsAsync = (taskID: string, labels: Label[]) => async (
}
}

export const removeTaskLabelsAsync = (
taskID: string,
labels: Label[]
) => async (dispatch): Promise<void> => {
export const removeTaskLabelAsync = (taskID: string, label: Label) => async (
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

dispatch
): Promise<void> => {
try {
await client.tasks.removeLabels(taskID, labels.map(l => l.id))
const task = await client.tasks.get(taskID)
await deleteTasksLabel({taskID, labelID: label.id})
const resp = await apiGetTask({taskID})
if (resp.status !== 200) {
throw new Error(resp.data.message)
}

const task = addDefaults(resp.data)

dispatch(updateTask(task))
} catch (error) {
Expand All @@ -283,7 +312,14 @@ export const removeTaskLabelsAsync = (

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

if (resp.status !== 200) {
throw new Error(resp.data.message)
}

dispatch(getTasks())
dispatch(notify(taskUpdateSuccess()))
Expand All @@ -294,9 +330,16 @@ export const updateTaskStatus = (task: Task) => async dispatch => {
}
}

export const updateTaskName = (task: Task) => async dispatch => {
export const updateTaskName = (
name: string,
taskID: string
) => async dispatch => {
try {
await client.tasks.update(task.id, task)
const resp = await apiPatchTask({taskID, data: {name}})

if (resp.status !== 200) {
throw new Error(resp.data.message)
}

dispatch(getTasks())
dispatch(notify(taskUpdateSuccess()))
Expand All @@ -309,7 +352,11 @@ export const updateTaskName = (task: Task) => async dispatch => {

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

if (resp.status !== 204) {
throw new Error(resp.data.message)
}

dispatch(getTasks())
dispatch(notify(taskDeleteSuccess()))
Expand All @@ -322,7 +369,19 @@ export const deleteTask = (task: Task) => async dispatch => {

export const cloneTask = (task: Task, _) => async dispatch => {
try {
await client.tasks.clone(task.id)
const resp = await apiGetTask({taskID: task.id})

if (resp.status !== 200) {
throw new Error(resp.data.message)
}

const postData = addDefaults(resp.data)

const newTask = await apiPostTask({data: postData})

if (newTask.status !== 201) {
throw new Error(newTask.data.message)
}

dispatch(notify(taskCloneSuccess(task.name)))
dispatch(getTasks())
Expand All @@ -342,7 +401,12 @@ export const selectTaskByID = (id: string) => async (
dispatch
): Promise<void> => {
try {
const task = await client.tasks.get(id)
const resp = await apiGetTask({taskID: id})
if (resp.status !== 200) {
throw new Error(resp.data.message)
}

const task = addDefaults(resp.data)
dispatch(setCurrentTask(task))
} catch (e) {
console.error(e)
Expand All @@ -356,7 +420,12 @@ export const setAllTaskOptionsByID = (taskID: string) => async (
dispatch
): Promise<void> => {
try {
const task = await client.tasks.get(taskID)
const resp = await apiGetTask({taskID})
if (resp.status !== 200) {
throw new Error(resp.data.message)
}

const task = addDefaults(resp.data)
dispatch(setAllTaskOptions(task))
} catch (e) {
console.error(e)
Expand Down Expand Up @@ -415,7 +484,11 @@ export const updateScript = () => async (dispatch, getState: GetStateFunc) => {
updatedTask.every = null
}

await client.tasks.update(task.id, updatedTask)
const resp = await apiPatchTask({taskID: task.id, data: updatedTask})

if (resp.status !== 200) {
throw new Error(resp.data.message)
}

dispatch(goToTasks())
dispatch(setCurrentTask(null))
Expand All @@ -437,7 +510,10 @@ export const saveNewScript = (script: string, preamble: string) => async (
const {
orgs: {org},
} = getState()
await client.tasks.createByOrgID(org.id, fluxScript, null)
const resp = await apiPostTask({data: {orgID: org.id, flux: fluxScript}})
if (resp.status !== 201) {
throw new Error(resp.data.message)
}

dispatch(setNewScript(''))
dispatch(clearTask())
Expand All @@ -459,13 +535,14 @@ export const saveNewScript = (script: string, preamble: string) => async (
export const getRuns = (taskID: string) => async (dispatch): Promise<void> => {
try {
dispatch(setRuns([], RemoteDataState.Loading))
dispatch(selectTaskByID(taskID))
const resp = await apiGetTasksRuns({taskID})

const [runs] = await Promise.all([
client.tasks.getRunsByTaskID(taskID),
dispatch(selectTaskByID(taskID)),
])
if (resp.status !== 200) {
throw new Error(resp.data.message)
}

const runsWithDuration = runs.map(run => {
const runsWithDuration = resp.data.runs.map(run => {
const finished = new Date(run.finishedAt)
const started = new Date(run.startedAt)

Expand All @@ -486,7 +563,7 @@ export const getRuns = (taskID: string) => async (dispatch): Promise<void> => {

export const runTask = (taskID: string) => async dispatch => {
try {
await client.tasks.startRunByTaskID(taskID)
await apiPostTasksRun({taskID})
dispatch(notify(taskRunSuccess()))
} catch (error) {
const message = getErrorMessage(error)
Expand All @@ -499,8 +576,11 @@ export const getLogs = (taskID: string, runID: string) => async (
dispatch
): Promise<void> => {
try {
const logs = await client.tasks.getLogEventsByRunID(taskID, runID)
dispatch(setLogs(logs))
const resp = await apiGetTasksRunsLogs({taskID, runID})
if (resp.status !== 200) {
throw new Error(resp.data.message)
}
dispatch(setLogs(resp.data.events))
} catch (error) {
console.error(error)
dispatch(setLogs([]))
Expand All @@ -512,7 +592,11 @@ export const convertToTemplate = (taskID: string) => async (
): Promise<void> => {
try {
dispatch(setExportTemplate(RemoteDataState.Loading))
const task = await client.tasks.get(taskID)
const resp = await apiGetTask({taskID})
if (resp.status !== 200) {
throw new Error(resp.data.message)
}
const task = addDefaults(resp.data)
const taskTemplate = taskToTemplate(task)

dispatch(setExportTemplate(RemoteDataState.Done, taskTemplate))
Expand Down
4 changes: 2 additions & 2 deletions ui/src/tasks/components/RunLogRow.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@ import moment from 'moment'
import {IndexList} from '@influxdata/clockface'

// Types
import {LogEvent} from '@influxdata/influx'
import {LogEvent} from 'src/types'
import {DEFAULT_TIME_FORMAT} from 'src/shared/constants'

interface Props {
Expand Down Expand Up @@ -36,7 +36,7 @@ class RunLogRow extends PureComponent<Props> {
)
}

private dateTimeString(dt: Date): string {
private dateTimeString(dt: string): string {
if (!dt) {
return ''
}
Expand Down
2 changes: 1 addition & 1 deletion ui/src/tasks/components/RunLogsList.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@ import RunLogRow from 'src/tasks/components/RunLogRow'
import FancyScrollbar from 'src/shared/components/fancy_scrollbar/FancyScrollbar'

// Types
import {LogEvent} from '@influxdata/influx'
import {LogEvent} from 'src/types'

interface Props {
onDismissOverlay: () => void
Expand Down
4 changes: 2 additions & 2 deletions ui/src/tasks/components/TaskCard.test.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -21,8 +21,8 @@ const setup = (override = {}) => {
onRunTask: jest.fn(),
onFilterChange: jest.fn(),
onUpdate: jest.fn(),
onAddTaskLabels: jest.fn(),
onRemoveTaskLabels: jest.fn(),
onAddTaskLabel: jest.fn(),
onRemoveTaskLabel: jest.fn(),
onCreateLabel: jest.fn(),
labels: [], // all labels
...override,
Expand Down
Loading