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.

Sign up for GitHub

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

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

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
1633b34
chore(ui): refactoring tasks to use oats rather than client, need to …
asalem1 Dec 27, 2019
0cd9414
chore(ui): refactored tasks to use oats rather than client library
asalem1 Dec 30, 2019
46154a0
fix(ui): updated parameter to resolve lint error
asalem1 Dec 30, 2019
a2b0438
fix(ui): fixed e2e tests, updated errorMessage func to handle message…
asalem1 Dec 30, 2019
488a2e2
fix(ui): updated PR to align with requested changes
asalem1 Dec 31, 2019
7e34126
chore(ui): finished type casting functions for greater clarity
asalem1 Dec 31, 2019
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
13 changes: 13 additions & 0 deletions ui/src/labels/utils/index.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
import _ from 'lodash'
import {Label} from 'src/types'

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

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

return errorMessage.join(', ')
}

const DEFAULT_LABEL_COLOR = '#326BBA'

export const addLabelDefaults = (l: Label): 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
View file
Open in desktop
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
View file
Open in desktop
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,32 @@ 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'

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

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

export const setTaskOption = (taskOption: {
key: TaskOptionKeys
value: string
Expand Down Expand Up @@ -240,7 +253,13 @@ 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)
}

let tasks = resp.data.tasks as Task[]
tasks = tasks.map(task => addDefaults(task))
Copy link
Contributor

Choose a reason for hiding this comment

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

consider typing the addDefaults function as taking an ITask and converting it to Task ITask => Task (I've explained more below where it is defined) and changing these two lines to const tasks resp.data.tasks.map(task=> addDefaults(task)). We should try to avoid type casting (using as) as much as we can so we can use all the wonderful types that the client gives us! and it's so close to being perfect! This way we also avoid using an unecessary reassignment to tasks and changing it from one type to another. consts are good. only let when absolutely necessary.


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 as Task)
Copy link
Contributor

Choose a reason for hiding this comment

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

we don't need as here if we correct the types on addDefaults function.

Copy link
Contributor

Choose a reason for hiding this comment

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

ditto for all uses of as from here on.. :)


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 as Task)

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 as Task)

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 as Task)
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 as Task)
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 as Task)
const taskTemplate = taskToTemplate(task)

dispatch(setExportTemplate(RemoteDataState.Done, taskTemplate))
Expand Down
4 changes: 2 additions & 2 deletions ui/src/tasks/components/RunLogRow.tsx
View file
Open in desktop
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
View file
Open in desktop
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
View file
Open in desktop
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