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

fix(ui/checks): checks render once they've been created #15556

Merged
merged 1 commit into from
Oct 24, 2019

Conversation

asalem1
Copy link
Contributor

@asalem1 asalem1 commented Oct 23, 2019

Closes #15554

Problem

Newly created checks without labels were not rendering on the checks list.

Steps for reproduction can be found on #15554

Solution

Ensure that check labels exist before mapping the values. Added testing to ensure that the checks list page should render a check once it has been created

  • CHANGELOG.md updated with a link to the PR (not the Issue)

@asalem1 asalem1 requested a review from a team October 23, 2019 21:13
@ghost ghost requested review from ebb-tide and hoorayimhelping and removed request for a team October 23, 2019 21:13
describe('Check should be viewable once created', () => {
beforeEach(() => {
// creates a check before each iteration
// TODO: refactor into a request
Copy link
Contributor Author

@asalem1 asalem1 Oct 23, 2019

Choose a reason for hiding this comment

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

the reason why this refactor into a request was punted is because this bug was blocking #15518 I felt it was important to get this merged and move forward with the current work we have since this seems like a pretty low-priority refactor

Copy link
Contributor

@hoorayimhelping hoorayimhelping left a comment

Choose a reason for hiding this comment

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

had a comment, let me know if it's too pedantic :)

check.labels &&
Array.isArray(check.labels) &&
check.labels.map(l => l.id)) ||
[],
Copy link
Contributor

Choose a reason for hiding this comment

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

if this is too nitpicky, tell me to stfu and i'll gladly do it! 😃

from what i've seen in the codebase, i think the more idiomatic way to do this is something like:

import {get} from 'lodash'

//...

const labels = get(check, 'labels', [])

const data = {
  ...check,
  name: clonedName,
  labels: labels.map(l => l.id),
} as PostCheck

lodash's get will check the first argument (check) for the second argument ('labels'). it'll do the falsey checks you're doing as it goes down the property list. if it can't find the property you're looking for, it returns the third property - an empty array.

(it's not a huge deal for a single property object, but it's super valuable when there are deeply nested properties, like here,).

using get like above, it'll guard against falsey values (null, undefined, false, '', etc), and includes the ) || [] that ensures labels stays an array. but it won't guard against truthy values, so if labels is anything but an array, like a string or an object, this will fail. but if you search src/client/generatedRoutes for export type Labels, you'll see it's typed as an array Label[], so i think this check is safe.

the reason i bring it up is i think it's probably a little bit safer and less likely to get tripped up on an edge case, and because we use this pattern (using get) in quite a few places i've seen in the code base, it's fairly well understood.

Copy link
Contributor

Choose a reason for hiding this comment

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

I ❤️ get too

Copy link
Contributor

@hoorayimhelping hoorayimhelping left a comment

Choose a reason for hiding this comment

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

Awesome work!!

Copy link
Contributor

@ebb-tide ebb-tide left a comment

Choose a reason for hiding this comment

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

🍰 🍰 🍰

cy.getByTestID('checkeo--header alerting-tab').click()
cy.getByTestID('add-threshold-condition-WARN').click()
cy.getByTestID('save-cell--button').click()
cy.getByTestID('check-card').should('have.length', 1)
Copy link
Contributor

Choose a reason for hiding this comment

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

❤️ for testing!

ui/src/alerting/actions/checks.ts Show resolved Hide resolved
ui/src/alerting/actions/checks.ts Show resolved Hide resolved
@asalem1 asalem1 merged commit 1dfa493 into master Oct 24, 2019
@asalem1 asalem1 deleted the fix/check-error branch November 8, 2019 21:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Created checks don't display on Monitoring & Alerts page
3 participants