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 reloading after notification changes #3264

Merged
merged 1 commit into from Jan 5, 2017
Merged

Fix reloading after notification changes #3264

merged 1 commit into from Jan 5, 2017

Conversation

@edmundoa
Copy link
Member

@edmundoa edmundoa commented Dec 28, 2016

Move decision of which data to load up the component hierarchy, fixing UI inconsistencies after editing or deleting alert notifications from the alert conditions page.

Fixes #3195 and #3235

@edmundoa edmundoa added this to the 2.2.0 milestone Dec 28, 2016
@bernd bernd self-assigned this Dec 30, 2016
@@ -35,13 +37,21 @@ const AlertNotification = React.createClass({

_onSubmit(data) {
AlarmCallbacksActions.update(this.props.alertNotification.stream_id, this.props.alertNotification.id, data)
.then(() => AlertNotificationsActions.listAll());
.then(() => {
if (typeof this.props.onNotificationUpdate === 'function') {

This comment has been minimized.

@bernd

bernd Dec 30, 2016
Member

Wouldn't a simple check for undefined/null be enough here since we specify the propTypes?

This comment has been minimized.

@edmundoa

edmundoa Jan 5, 2017
Author Member

We are very inconsistent with that, but I think it is safer to ensure it is a function when using it. The prop type check may not always be there (for instance if we ever enable the production mode in react), and it only logs a warning, so it will not prevent the function to be called, and the application would be broken in that case.

@bernd
Copy link
Member

@bernd bernd commented Jan 2, 2017

Please rebase against master to fix merge conflict. Thanks!

Move decision of which data to load up the component hierarchy, fixing
inconsistencies after editing or deleting alert notifications from the
alert conditions page.

Fixes #3195 and #3235
@edmundoa edmundoa force-pushed the issue-3195 branch from cb5661b to 33045a8 Jan 5, 2017
@edmundoa
Copy link
Member Author

@edmundoa edmundoa commented Jan 5, 2017

Rebased against master, thank you for reviewing!

@bernd
bernd approved these changes Jan 5, 2017
@bernd bernd merged commit c69a924 into master Jan 5, 2017
2 of 4 checks passed
2 of 4 checks passed
continuous-integration/travis-ci/pr The Travis CI build is in progress
Details
continuous-integration/travis-ci/push The Travis CI build is in progress
Details
@garybot2
ci-web-linter Jenkins build graylog-pr-linter-check 1243 has succeeded
Details
licence/cla Contributor License Agreement is signed.
Details
@bernd bernd deleted the issue-3195 branch Jan 5, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

2 participants