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: Disable saving threshold check if no threshold selected #15348

Merged
merged 6 commits into from
Oct 8, 2019

Conversation

ebb-tide
Copy link
Contributor

@ebb-tide ebb-tide commented Oct 8, 2019

Closes #15334

Describe your proposed changes here.
Disable saving threshold check if no threshold is selected

  • CHANGELOG.md updated with a link to the PR (not the Issue)
  • Well-formatted commit messages
  • Rebased/mergeable
  • Tests pass
  • http/swagger.yml updated (if modified Go structs or API)
  • Documentation updated or issue created (provide link to issue/pr)
  • Signed CLA (if not already signed)

@ebb-tide ebb-tide changed the title Bugfix check saving Disable saving threshold check if no threshold selected Oct 8, 2019
@ebb-tide ebb-tide changed the title Disable saving threshold check if no threshold selected bugfix: Disable saving threshold check if no threshold selected Oct 8, 2019
@ebb-tide ebb-tide changed the title bugfix: Disable saving threshold check if no threshold selected fix: Disable saving threshold check if no threshold selected Oct 8, 2019
@ebb-tide ebb-tide requested a review from a team October 8, 2019 18:05
@ghost ghost requested review from hoorayimhelping and removed request for a team October 8, 2019 18:05
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.

I had one question about false vs null, but other than that it looks awesome! Worked great for me.

const oneOrMoreThresholds =
check.type === 'threshold'
? check.thresholds && !!check.thresholds.length
: null
Copy link
Contributor

Choose a reason for hiding this comment

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

i think this should be false, not null. CheckEOSaveButton is expecting oneOrMoreThresholds to be a boolean and making this explicitly false will let us not rely on JS's sometimes confusing type casting. what do you think?

@@ -98,6 +98,10 @@ const CheckEOHeader: FC<Props> = ({
}

const {singleField, singleAggregateFunc} = isDraftQueryAlertable(draftQueries)
const oneOrMoreThresholds =
check.type === 'threshold'
? check.thresholds && !!check.thresholds.length
Copy link
Contributor

Choose a reason for hiding this comment

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

do we prefer !! over Boolean()?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

no strong preference but !! feels more readable?

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.

👍

@ebb-tide ebb-tide closed this Oct 8, 2019
@ebb-tide ebb-tide reopened this Oct 8, 2019
@ebb-tide ebb-tide merged commit 9f011d6 into master Oct 8, 2019
@ebb-tide ebb-tide deleted the bugfix-check-saving branch October 8, 2019 19:23
hoorayimhelping pushed a commit that referenced this pull request Oct 8, 2019
* Prevent check saving if no thresholds

* Add tests

* Add changes to changelog

* make optional props optional

* use false instead of null for boolean
hoorayimhelping added a commit that referenced this pull request Oct 8, 2019
add test for hydrateVars

dashboard variable dropdown test: inspect values, not just array length

add RTL test for variable dropdown changes

lint

fix: Disable saving threshold check if no threshold selected (#15348)

* Prevent check saving if no thresholds

* Add tests

* Add changes to changelog

* make optional props optional

* use false instead of null for boolean

changelog

fix(ui): ignore false change events in VariableForm (#15317)

closes #15059

the issue is to persist user data across variable type selection interfaces within the variable editor. this commit pushes all of the variable editor information down to redux to allow persistence outside of the view state until the user clicks "cancel" or "create" in the interface.
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.

Editing a check which doesn't have thresholds causes visualization to crash
2 participants