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

feat(ui): added labels to buckets #16855

Merged
merged 16 commits into from
Feb 13, 2020
Merged
Show file tree
Hide file tree
Changes from 15 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
3 changes: 3 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
@@ -1,6 +1,9 @@
## v2.0.0-beta.4 [unreleased]

### Features

1. [16855](https://github.com/influxdata/influxdb/pull/16855): Added labels to buckets

### Bug Fixes

## v2.0.0-beta.3 [2020-02-11]
Expand Down
52 changes: 32 additions & 20 deletions ui/cypress/e2e/buckets.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,20 @@ describe('Buckets', () => {
})

cy.getByTestID(`bucket--card--name ${newBucket}`).should('exist')

// Add a label
cy.getByTestID(`bucket-card ${newBucket}`).within(() => {
cy.getByTestID('inline-labels--add').click()
})

const labelName = 'l1'
cy.getByTestID('inline-labels--popover--contents').type(labelName)
cy.getByTestID('inline-labels--create-new').click()
cy.getByTestID('create-label-form--submit').click()

// Delete the label
cy.getByTestID(`label--pill--delete ${labelName}`).click({force: true})
cy.getByTestID('inline-labels--empty').should('exist')
})

it("can update a bucket's retention rules", () => {
Expand Down Expand Up @@ -60,28 +74,26 @@ describe('Buckets', () => {

describe('Searching and Sorting', () => {
it('can sort by name and retention', () => {
cy.getByTestID('name-sorter').click()
cy.getByTestID('bucket-card')
.first()
.contains('defbuck')

cy.getByTestID('name-sorter').click()
cy.getByTestID('bucket-card')
.first()
.contains('_monitoring')

cy.getByTestID('retention-sorter').click()
cy.getByTestID('bucket-card')
.first()
.contains('_tasks')

cy.getByTestID('retention-sorter').click()
cy.getByTestID('bucket-card')
.first()
.contains('defbuck')
const buckets = ['defbuck', '_tasks', '_monitoring']
cy.getByTestID('name-sorter')
.click()
.then(() => {
cy.get('.cf-resource-name').each((val, index) => {
Copy link
Contributor

Choose a reason for hiding this comment

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

still have to change this (and change bucket + name back to bucket on the testid in the component)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

why is that? We changed it for greater specificity to access a specific bucket card.

Copy link
Contributor

Choose a reason for hiding this comment

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

so use that specificity in the test by generating ids to look up (instead of evaluating the text value). i think we're still straddling two solutions right now and just need to pick one and push it through.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@drdelambre i'm not quite sure I understand your solution, would you mind expanding upon that with an example of what you had in mind?

Copy link
Contributor

Choose a reason for hiding this comment

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

yeah. so you have the name of the resource in both the test id and in the text content of the dom node you are trying to pinpoint. So you have two sources of truth, and two directions to go to unify them:

  1. revert the test id specificity in the component, and test that the value within the component (with expect(val.text()).to.include(buckets[index])) is equal to the resource name. that keeps things scoped to buckets (good) but also ties the representation of the key to the value of the key (bad).
  2. don't test for the value of the node, but instead look for the test id that meets the key you are looking for( cy.getByTestId(`bucket ${cool name}`) ). this scopes it to the bucket (good) as well as decouples the representation from the state (good)

seems like you're leaning towards #2. either one is fine, it's just that generic resource selector has got to go.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

but wouldn't the second solution defeat the purpose of the test? If we're trying to test the sorting mechanism to present a specific order, targeting a bucket by the name would only validate that the bucket exists. It wouldn't represent the order with which that bucket exists in the context of the page

expect(val.text()).to.include(buckets[index])
})
})

cy.getByTestID('name-sorter')
.click()
.then(() => {
const asc_buckets = buckets.slice().sort()
cy.get('.cf-resource-name').each((val, index) => {
expect(val.text()).to.include(asc_buckets[index])
})
})

cy.getByTestID('search-widget').type('tasks')
cy.getByTestID('bucket-card').should('have.length', 1)
cy.get('.cf-resource-card').should('have.length', 1)
})
})

Expand Down
130 changes: 108 additions & 22 deletions ui/src/buckets/actions/thunks.ts
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,15 @@ import * as api from 'src/client'
import {bucketSchema, arrayOfBuckets} from 'src/schemas'

// Types
import {RemoteDataState, GetState, Bucket, BucketEntities} from 'src/types'
import {
AppState,
RemoteDataState,
GetState,
GenBucket,
Bucket,
BucketEntities,
Label,
} from 'src/types'

// Utils
import {getErrorMessage} from 'src/utils/api'
Expand All @@ -35,7 +43,10 @@ import {
bucketUpdateSuccess,
bucketRenameSuccess,
bucketRenameFailed,
addBucketLabelFailed,
removeBucketLabelFailed,
} from 'src/shared/copy/notifications'
import {getLabels} from 'src/resources/selectors'

type Action = BucketAction | NotifyAction

Expand All @@ -59,8 +70,8 @@ export const getBuckets = () => async (
)

dispatch(setBuckets(RemoteDataState.Done, buckets))
} catch (e) {
console.error(e)
} catch (error) {
console.error(error)
dispatch(setBuckets(RemoteDataState.Error))
dispatch(notify(getBucketsFailed()))
}
Expand Down Expand Up @@ -93,13 +104,17 @@ export const createBucket = (bucket: Bucket) => async (
}
}

export const updateBucket = (updatedBucket: Bucket) => async (
dispatch: Dispatch<Action>
export const updateBucket = (bucket: Bucket) => async (
dispatch: Dispatch<Action>,
getState: GetState
) => {
try {
const state = getState()
const data = denormalizeBucket(state, bucket)

const resp = await api.patchBucket({
bucketID: updatedBucket.id,
data: updatedBucket,
bucketID: bucket.id,
data,
})

if (resp.status !== 200) {
Expand All @@ -112,22 +127,25 @@ export const updateBucket = (updatedBucket: Bucket) => async (
)

dispatch(editBucket(newBucket))
dispatch(notify(bucketUpdateSuccess(updatedBucket.name)))
} catch (e) {
console.error(e)
const message = getErrorMessage(e)
dispatch(notify(bucketUpdateSuccess(bucket.name)))
} catch (error) {
console.error(error)
const message = getErrorMessage(error)
dispatch(notify(bucketUpdateFailed(message)))
}
}

export const renameBucket = (
originalName: string,
updatedBucket: Bucket
) => async (dispatch: Dispatch<Action>) => {
export const renameBucket = (originalName: string, bucket: Bucket) => async (
dispatch: Dispatch<Action>,
getState: GetState
) => {
try {
const state = getState()
const data = denormalizeBucket(state, bucket)

const resp = await api.patchBucket({
bucketID: updatedBucket.id,
data: updatedBucket,
bucketID: bucket.id,
data,
})

if (resp.status !== 200) {
Expand All @@ -140,9 +158,9 @@ export const renameBucket = (
)

dispatch(editBucket(newBucket))
dispatch(notify(bucketRenameSuccess(updatedBucket.name)))
} catch (e) {
console.error(e)
dispatch(notify(bucketRenameSuccess(bucket.name)))
} catch (error) {
console.error(error)
dispatch(notify(bucketRenameFailed(originalName)))
}
}
Expand All @@ -159,8 +177,76 @@ export const deleteBucket = (id: string, name: string) => async (

dispatch(removeBucket(id))
dispatch(checkBucketLimits())
} catch (e) {
console.error(e)
} catch (error) {
console.error(error)
dispatch(notify(bucketDeleteFailed(name)))
}
}

export const addBucketLabel = (bucketID: string, label: Label) => async (
dispatch: Dispatch<Action>
): Promise<void> => {
try {
const postResp = await api.postBucketsLabel({
bucketID,
data: {labelID: label.id},
})

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

const resp = await api.getBucket({bucketID})

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

const newBucket = normalize<Bucket, BucketEntities, string>(
resp.data,
bucketSchema
)

dispatch(editBucket(newBucket))
} catch (error) {
console.error(error)
dispatch(notify(addBucketLabelFailed()))
}
}

export const deleteBucketLabel = (bucketID: string, label: Label) => async (
dispatch: Dispatch<Action>
): Promise<void> => {
try {
const deleteResp = await api.deleteBucketsLabel({
bucketID,
labelID: label.id,
})
if (deleteResp.status !== 204) {
throw new Error(deleteResp.data.message)
}

const resp = await api.getBucket({bucketID})
if (resp.status !== 200) {
throw new Error(resp.data.message)
}

const newBucket = normalize<Bucket, BucketEntities, string>(
resp.data,
bucketSchema
)

dispatch(editBucket(newBucket))
} catch (error) {
console.error(error)
dispatch(notify(removeBucketLabelFailed()))
}
}

const denormalizeBucket = (state: AppState, bucket: Bucket): GenBucket => {
const labels = getLabels(state, bucket.labels)
return {
...bucket,
labels,
}
}
48 changes: 42 additions & 6 deletions ui/src/buckets/components/BucketCard.tsx
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
// Libraries
import React, {PureComponent} from 'react'
import {withRouter, WithRouterProps} from 'react-router'
import {connect} from 'react-redux'
import _ from 'lodash'

// Components
Expand All @@ -13,19 +14,28 @@ import {
} from '@influxdata/clockface'
import BucketContextMenu from 'src/buckets/components/BucketContextMenu'
import BucketAddDataButton from 'src/buckets/components/BucketAddDataButton'
import InlineLabels from 'src/shared/components/inlineLabels/InlineLabels'
import {FeatureFlag} from 'src/shared/utils/featureFlag'

// Constants
import {isSystemBucket} from 'src/buckets/constants/index'

// Types
import {Bucket} from 'src/types'
import {Bucket, Label} from 'src/types'
import {DataLoaderType} from 'src/types/dataLoaders'

// Actions
import {addBucketLabel, deleteBucketLabel} from 'src/buckets/actions/thunks'

export interface PrettyBucket extends Bucket {
ruleString: string
}

interface DispatchProps {
onAddBucketLabel: typeof addBucketLabel
onDeleteBucketLabel: typeof deleteBucketLabel
}

interface Props {
bucket: PrettyBucket
onEditBucket: (b: PrettyBucket) => void
Expand All @@ -36,12 +46,12 @@ interface Props {
onFilterChange: (searchTerm: string) => void
}

class BucketRow extends PureComponent<Props & WithRouterProps> {
class BucketRow extends PureComponent<Props & WithRouterProps & DispatchProps> {
public render() {
const {bucket, onDeleteBucket} = this.props
return (
<ResourceCard
testID="bucket-card"
testID={`bucket-card ${bucket.name}`}
contextMenu={
!isSystemBucket(bucket.name) && (
<BucketContextMenu
Expand Down Expand Up @@ -96,14 +106,20 @@ class BucketRow extends PureComponent<Props & WithRouterProps> {
}

private get actionButtons(): JSX.Element {
const {bucket} = this.props
const {bucket, onFilterChange} = this.props
if (bucket.type === 'user') {
return (
<FlexBox
direction={FlexDirection.Row}
margin={ComponentSize.Small}
style={{marginTop: '4px'}}
>
<InlineLabels
selectedLabelIDs={bucket.labels}
onFilterChange={onFilterChange}
onAddLabel={this.handleAddLabel}
onRemoveLabel={this.handleRemoveLabel}
/>
<BucketAddDataButton
onAddCollector={this.handleAddCollector}
onAddLineProtocol={this.handleAddLineProtocol}
Expand All @@ -118,7 +134,7 @@ class BucketRow extends PureComponent<Props & WithRouterProps> {
<FeatureFlag name="deleteWithPredicate">
<Button
text="Delete Data By Filter"
testID="bucket-delete-task"
testID="bucket-delete-bucket"
size={ComponentSize.ExtraSmall}
onClick={this.handleDeleteData}
/>
Expand All @@ -128,6 +144,18 @@ class BucketRow extends PureComponent<Props & WithRouterProps> {
}
}

private handleAddLabel = (label: Label) => {
const {bucket, onAddBucketLabel} = this.props

onAddBucketLabel(bucket.id, label)
}

private handleRemoveLabel = (label: Label) => {
const {bucket, onDeleteBucketLabel} = this.props

onDeleteBucketLabel(bucket.id, label)
}

private handleDeleteData = () => {
const {onDeleteData, bucket} = this.props

Expand Down Expand Up @@ -185,4 +213,12 @@ class BucketRow extends PureComponent<Props & WithRouterProps> {
}
}

export default withRouter<Props>(BucketRow)
const mdtp: DispatchProps = {
onAddBucketLabel: addBucketLabel,
onDeleteBucketLabel: deleteBucketLabel,
}

export default connect<{}, DispatchProps>(
null,
mdtp
)(withRouter<Props>(BucketRow))
2 changes: 1 addition & 1 deletion ui/src/buckets/components/BucketsTab.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -186,7 +186,7 @@ class BucketsTab extends PureComponent<Props, State> {
}

private handleUpdateBucket = (updatedBucket: PrettyBucket) => {
this.props.updateBucket(updatedBucket as Bucket)
this.props.updateBucket(updatedBucket)
}

private handleDeleteBucket = ({id, name}: PrettyBucket) => {
Expand Down
Loading