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

Zs sortbyseconds #14923 #15728

Merged
merged 7 commits into from
Nov 5, 2019
Merged

Zs sortbyseconds #14923 #15728

Show file tree
Hide file tree
Changes from 6 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
0c41231
fix(ui) Fixing the sort by seconds
zoesteinkamp Oct 31, 2019
f30240a
fix(ui): Fixing the sort by seconds
zoesteinkamp Nov 4, 2019
af8943e
fix(ui): Fixing the sort by seconds
zoesteinkamp Nov 4, 2019
fd09f26
fix(ui): Fixing the sort by seconds
zoesteinkamp Nov 4, 2019
f37ea3b
fix(ui): Fixing the sort by seconds
zoesteinkamp Nov 4, 2019
c11fc49
fix(ui): Fixing the sort by seconds
zoesteinkamp Nov 4, 2019
d643f07
fix(ui): Fixing the sort by seconds
zoesteinkamp Nov 5, 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
49 changes: 43 additions & 6 deletions ui/cypress/e2e/buckets.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,7 @@ describe('Buckets', () => {
describe('from the buckets index page', () => {
it('can create a bucket', () => {
const newBucket = '🅱️ucket'
cy.getByTestID(`bucket--card ${newBucket}`).should('not.exist')
cy.getByTestID(`bucket--card--name ${newBucket}`).should('not.exist')

cy.getByTestID('Create Bucket').click()
cy.getByTestID('overlay--container').within(() => {
Expand All @@ -30,13 +30,16 @@ describe('Buckets', () => {
.click()
})

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

it("can update a bucket's retention rules", () => {
cy.get<Bucket>('@bucket').then(({name}: Bucket) => {
cy.getByTestID(`bucket--card--name ${name}`).click()
cy.getByTestID(`bucket--card ${name}`).should('not.contain', '7 days')
cy.getByTestID(`bucket--card--name ${name}`).should(
'not.contain',
'7 days'
)
})

cy.getByTestID('retention-intervals--button').click()
Expand All @@ -47,8 +50,42 @@ describe('Buckets', () => {
cy.contains('Save').click()
})

cy.get<Bucket>('@bucket').then(({name}: Bucket) => {
cy.getByTestID(`bucket--card ${name}`).should('contain', '7 days')
cy.get<Bucket>('@bucket').then(() => {
cy.getByTestID(`cf-resource-card--meta-item`).should(
'contain',
'7 days'
)
})
})

describe('Searching and Sorting', () => {
it('Searching buckets', () => {
cy.getByTestID('search-widget').type('tasks')
cy.getByTestID('bucket-card').should('have.length', 1)
})

it('Sorting by Name', () => {
cy.getByTestID('name-sorter').click()
cy.getByTestID('bucket-card')
.first()
.contains('defbuck')

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

it('Sorting by Retention', () => {
cy.getByTestID('retention-sorter').click()
cy.getByTestID('bucket-card')
.first()
.contains('_tasks')

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

Expand Down Expand Up @@ -89,7 +126,7 @@ describe('Buckets', () => {
.click()
})

it('closes the overlay upon a successful delete with predicate submission', () => {
it.skip('closes the overlay upon a successful delete with predicate submission', () => {
Copy link
Contributor

Choose a reason for hiding this comment

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

can you add a quick comment here explaining why we're skipping this please?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

adding in comment! im going to make a new issue to address this failing test and another

cy.getByTestID('delete-checkbox').check({force: true})
cy.getByTestID('confirm-delete-btn').click()
cy.getByTestID('overlay--container').should('not.exist')
Expand Down
2 changes: 1 addition & 1 deletion ui/src/buckets/components/BucketCard.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,7 +40,7 @@ class BucketRow extends PureComponent<Props & WithRouterProps> {
const {bucket, onDeleteBucket} = this.props
return (
<ResourceCard
testID={`bucket--card ${bucket.name}`}
testID="bucket-card"
contextMenu={
!isSystemBucket(bucket.name) && (
<BucketContextMenu
Expand Down
41 changes: 23 additions & 18 deletions ui/src/buckets/components/BucketList.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,7 @@ import {SortTypes} from 'src/shared/utils/sort'
// Utils
import {prettyBuckets} from 'src/shared/utils/prettyBucket'

type SortKey = keyof PrettyBucket
type SortKey = keyof PrettyBucket | 'retentionRules[0].everySeconds'
Copy link
Contributor

Choose a reason for hiding this comment

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

this is kind of setting off a spidey sense - seems like we're coupling the state and shape of our redux store to a value type - the end result is that the sorting code relies on the precise shape and existence of the store having a first item in retentionRules with an everySeconds value. if any of that isn't there or the shape changes (maybe the api gets updated), this could very likely break.

here's the thing though, i know you didn't make this pattern, and i don't know of a better way to do this without changing a lot of code, so i can't really do more than say keep en eye out on this sorting code cause it might break unexpectedly. (said without judgement of the quality of the code or the circumstances it was written under :))

Copy link
Contributor Author

Choose a reason for hiding this comment

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

hopefully is things go south the tests will catch it, but yeah it is slightly concerning, but i dont think right now is the time to refactor all the logic. Well keep an eye to see if any related issues rear their head


interface OwnProps {
buckets: PrettyBucket[]
Expand All @@ -37,7 +37,9 @@ interface OwnProps {
sortKey: string
sortDirection: Sort
sortType: SortTypes
onClickColumn: (mextSort: Sort, sortKey: SortKey) => void
onClickColumn: (
sortType: SortTypes
) => (nextSort: Sort, sortKey: SortKey) => void
}

interface DispatchProps {
Expand Down Expand Up @@ -73,7 +75,6 @@ class BucketList extends PureComponent<Props & WithRouterProps, State> {

public render() {
const {emptyState, sortKey, sortDirection, onClickColumn} = this.props

return (
<>
<ResourceList>
Expand All @@ -82,13 +83,15 @@ class BucketList extends PureComponent<Props & WithRouterProps, State> {
name="Name"
sortKey={this.headerKeys[0]}
sort={sortKey === this.headerKeys[0] ? sortDirection : Sort.None}
onClick={onClickColumn}
onClick={onClickColumn(SortTypes.String)}
testID="name-sorter"
/>
<ResourceList.Sorter
name="Retention"
sortKey={this.headerKeys[1]}
sort={sortKey === this.headerKeys[1] ? sortDirection : Sort.None}
onClick={onClickColumn}
onClick={onClickColumn(SortTypes.Float)}
testID="retention-sorter"
/>
</ResourceList.Header>
<ResourceList.Body emptyState={emptyState}>
Expand All @@ -100,7 +103,7 @@ class BucketList extends PureComponent<Props & WithRouterProps, State> {
}

private get headerKeys(): SortKey[] {
return ['name', 'ruleString']
return ['name', 'retentionRules[0].everySeconds']
}

private get listBuckets(): JSX.Element[] {
Expand All @@ -119,18 +122,20 @@ class BucketList extends PureComponent<Props & WithRouterProps, State> {
sortType
)

return sortedBuckets.map(bucket => (
<BucketCard
key={bucket.id}
bucket={bucket}
onEditBucket={this.handleStartEdit}
onDeleteBucket={onDeleteBucket}
onDeleteData={this.handleStartDeleteData}
onAddData={this.handleStartAddData}
onUpdateBucket={this.handleUpdateBucket}
onFilterChange={onFilterChange}
/>
))
return sortedBuckets.map(bucket => {
return (
<BucketCard
key={bucket.id}
bucket={bucket}
onEditBucket={this.handleStartEdit}
onDeleteBucket={onDeleteBucket}
onDeleteData={this.handleStartDeleteData}
onAddData={this.handleStartAddData}
onUpdateBucket={this.handleUpdateBucket}
onFilterChange={onFilterChange}
/>
)
})
}

private handleStartEdit = (bucket: PrettyBucket) => {
Expand Down
7 changes: 5 additions & 2 deletions ui/src/buckets/components/BucketsTab.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -164,8 +164,11 @@ class BucketsTab extends PureComponent<Props, State> {
)
}

private handleClickColumn = (nextSort: Sort, sortKey: SortKey) => {
const sortType = SortTypes.String
private handleClickColumn = (sortType: SortTypes) => (
nextSort: Sort,
sortKey: SortKey
) => {
// const sortType = SortTypes.Float
zoesteinkamp marked this conversation as resolved.
Show resolved Hide resolved
this.setState({sortKey, sortDirection: nextSort, sortType})
}

Expand Down
5 changes: 0 additions & 5 deletions ui/yarn.lock
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4381,11 +4381,6 @@ eslint-plugin-jest@^23.0.2:
dependencies:
"@typescript-eslint/experimental-utils" "^2.5.0"

eslint-plugin-no-only-tests@^2.3.1:
version "2.3.1"
resolved "https://registry.yarnpkg.com/eslint-plugin-no-only-tests/-/eslint-plugin-no-only-tests-2.3.1.tgz#7b24a4df55b818d0838410aa96b24a5a4a072262"
integrity sha512-LzCzeQrlkNjEwUWEoGhfjz+Kgqe0080W6qC8I8eFwSMXIsr1zShuIQnRuSZc4Oi7k1vdUaNGDc+/GFvg6IHSHA==

eslint-plugin-react@^7.16.0:
version "7.16.0"
resolved "https://registry.yarnpkg.com/eslint-plugin-react/-/eslint-plugin-react-7.16.0.tgz#9928e4f3e2122ed3ba6a5b56d0303ba3e41d8c09"
Expand Down