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

Jump to bottom

fixing dashboard state filters #20480

Merged
merged 3 commits into from Jul 9, 2018
Merged

fixing dashboard state filters #20480

merged 3 commits into from Jul 9, 2018

Conversation

ppisljar
Copy link
Member

@ppisljar ppisljar commented Jul 5, 2018

resolves #20477

@ppisljar ppisljar added review Feature:Dashboard Dashboard related features v6.4.0 labels Jul 5, 2018
@timroes
Copy link
Contributor

timroes commented Jul 5, 2018

Can we add a test for this one? :)

@ppisljar
Copy link
Member Author

ppisljar commented Jul 5, 2018

hard, as nothing fails at the moment, but it will once we remove angular from visualize ... so maybe we add test then ?

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

@ppisljar
Copy link
Member Author

ppisljar commented Jul 5, 2018

actually the test is already there, but is at the moment passing even if the behaviour is wrong. however with #20295 the test is failing unless this fix is applied

@ppisljar ppisljar mentioned this pull request Jul 5, 2018
Merged
chrisdavies
chrisdavies suggested changes Jul 5, 2018
View reviewed changes
Copy link
Contributor

@chrisdavies chrisdavies left a comment

Choose a reason for hiding this comment

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

I think the cloneDeep should go into the updateFilters function, rather than being the responsibility of the callers. But I could be convinced otherwise!

src/core_plugins/kibana/public/dashboard/dashboard_state_manager.js Outdated
@@ -204,7 +204,7 @@ export class DashboardStateManager {
FilterUtils.cleanFiltersForComparison(this.appState.filters),
FilterUtils.cleanFiltersForComparison(getFilters(state))
)) {
store.dispatch(updateFilters(this.appState.filters));
store.dispatch(updateFilters(_.cloneDeep(this.appState.filters)));
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 fixes the problem, it indicates that some code is mutating appState.filters, which seems like the real source of the bug, but also probably too tricky to fix right now. Given that redux expects its state to be immutable, I think we need to change where this fix goes by putting the clone deep in the action helper (updateFilters). Putting the cloneDeep in the action helper means that any other code that dispatches an updateFilters action will not be victim to the same bug. Does that make sense?

@ppisljar
Copy link
Member Author

ppisljar commented Jul 5, 2018

@chrisdavies like this ?

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

timroes
timroes reviewed Jul 9, 2018
View reviewed changes
src/core_plugins/kibana/public/dashboard/reducers/view.ts Outdated
@@ -17,6 +17,7 @@
* under the License.
*/

import _ from 'lodash';
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe use named import here :-)

stacey-gammon
stacey-gammon approved these changes Jul 9, 2018
View reviewed changes
Copy link
Contributor

@stacey-gammon stacey-gammon left a comment

Choose a reason for hiding this comment

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

lgtm, thanks for fixing this! FWIW, I think adding more typescript would have caught this issue because the redux tree will be marked as readonly.

@ppisljar ppisljar merged commit 0c7b44d into elastic:master Jul 9, 2018
@ppisljar ppisljar deleted the fix/dashboardFiltersState branch July 9, 2018 13:29
@ppisljar ppisljar mentioned this pull request Jul 9, 2018
Merged
ppisljar added a commit to ppisljar/kibana that referenced this pull request Jul 9, 2018
@ppisljar
fixing dashboard state filters (elastic#20480)
d4ce8a2
@elasticmachine
Copy link
Contributor

💚 Build Succeeded

ppisljar added a commit that referenced this pull request Jul 9, 2018
@ppisljar
fixing dashboard state filters (#20480) (#20555)
83e2a05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@timroes timroes timroes left review comments

@chrisdavies chrisdavies chrisdavies approved these changes

@stacey-gammon stacey-gammon stacey-gammon approved these changes

@nreese nreese Awaiting requested review from nreese

@markov00 markov00 Awaiting requested review from markov00

Assignees
No one assigned
Labels
Feature:Dashboard Dashboard related features review v6.4.0
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

dashboard state manager issue with removing filters
5 participants
@ppisljar @timroes @elasticmachine @chrisdavies @stacey-gammon