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

Also post check tree nodes top-to-bottom when required #1057

Merged
merged 1 commit into from
Apr 19, 2017
Merged

Also post check tree nodes top-to-bottom when required #1057

merged 1 commit into from
Apr 19, 2017

Conversation

skateman
Copy link
Member

@skateman skateman commented Apr 18, 2017 •
edited
Loading

The post_check feature is a post-processing for hierarchical checkboxes, it was able to set a node's checked state based on their childrens'. This PR introduces the same logic in the opposite way, if a parent node is checked, it also sets the child nodes' state to checked.

FYI @hayesr

himdel
himdel reviewed Apr 19, 2017
View reviewed changes
spec/javascripts/miq_tree_spec.js Outdated

debugger;
Copy link
Contributor

Choose a reason for hiding this comment

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

debugger ;)

himdel
himdel reviewed Apr 19, 2017
View reviewed changes
spec/javascripts/miq_tree_spec.js Outdated
expect(miqTreeFindNodeByKey('test', '1').state.checked).toBe(undefined);
expect(miqTreeFindNodeByKey('test', '2-1').state.checked).toBe(true);
expect(miqTreeFindNodeByKey('test', '2-2').state.checked).toBe(true);
expect(miqTreeFindNodeByKey('test', '3-1').state.checked).toBe(true);
Copy link
Contributor

@himdel himdel Apr 19, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

Maybe also add

expect(miqTreeFindNodeByKey('test', '2').state.checked).toBe(true);
expect(miqTreeFindNodeByKey('test', '3').state.checked).toBe(true);

to be sure the parent always wins?

himdel
himdel reviewed Apr 19, 2017
View reviewed changes
app/assets/javascripts/miq_tree.js
var node = nodes.pop();
if (!node.state) node.state = {};
var parent = nodes.pop();
if (!parent.nodes) {
Copy link
Contributor

Choose a reason for hiding this comment

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

!parent.nodes never happens because if (node.nodes) parents.push(node), am I missing something or just defensive programming? :)

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, you're confused by the variable name, check where it is popped from 😉

Copy link
Contributor

Choose a reason for hiding this comment

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

Aah :D

@himdel
Copy link
Contributor

himdel commented Apr 19, 2017

LGTM, as long as we never get any data with the parent wrongly checked, this should not break anything 👍

@himdel
Copy link
Contributor

himdel commented Apr 19, 2017

Verified in the UI, SmartProxyAffinity tree (ManageIQ/manageiq#10374) works the same after this.

Also checked that changin the data in that tree so that a folder has select true makes the change propagate both up and down 👍

@miq-bot
Copy link
Member

miq-bot commented Apr 19, 2017

Checked commit skateman@b78ed65 with ruby 2.2.6, rubocop 0.47.1, and haml-lint 0.20.0
0 files checked, 0 offenses detected
Everything looks good. 🍪

@himdel himdel merged commit 8d04045 into ManageIQ:master Apr 19, 2017
@himdel himdel added this to the Sprint 59 Ending Apr 24, 2017 milestone Apr 19, 2017
@himdel
Copy link
Contributor

himdel commented Apr 19, 2017 •
edited
Loading

@hayesr is #137 meant for fine or gaprindashvili only? (Never mind, fine/no it is :).)

@skateman skateman deleted the post-check branch April 19, 2017 12:48
@himdel himdel added the fine/no label Apr 19, 2017
@himdel himdel mentioned this pull request Apr 19, 2017
Merged
3 tasks
@hayesr
Copy link
Contributor

hayesr commented Apr 21, 2017 •
edited
Loading

@himdel Actually, this seems to work well and could help with fixes needed for the RBAC Tree in Fine. If there's a way we can backport this to Fine, it would make things easier. Otherwise I'll carry forward my Euwe changes.

@skateman
Copy link
Member Author

@hayesr maybe I am missing something, do you want #137 to be backported to fine? Or is there some fine-related issue that I haven't seen?

@hayesr
Copy link
Contributor

hayesr commented Apr 21, 2017

@skateman Not #137, just this one. It could mean a smaller fix to Fine than the one I did for Euwe.

@himdel
Copy link
Contributor

himdel commented Apr 24, 2017 •
edited
Loading

Ahh, alright, changed to fine/yes, please mention this PR when you add a fine fix for the bug :).

(And please keep it minimal ;))

@himdel himdel added fine/yes and removed fine/no labels Apr 24, 2017
simaishi pushed a commit that referenced this pull request Apr 24, 2017
@himdel @simaishi
Merge pull request #1057 from skateman/post-check
c7c9e0e 
Also post check tree nodes top-to-bottom when required
(cherry picked from commit 8d04045)
@simaishi
Copy link
Contributor

Fine backport details:

$ git log -1
commit c7c9e0e06714866d354673679bddff8ee6ae6f81
Author: Martin Hradil <[email protected]>
Date:   Wed Apr 19 12:10:36 2017 +0000

    Merge pull request #1057 from skateman/post-check
    
    Also post check tree nodes top-to-bottom when required
    (cherry picked from commit 8d04045a11ed370b8295168c698bf374e761e178)

@hayesr hayesr mentioned this pull request May 6, 2017
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@himdel himdel himdel left review comments

Assignees

@himdel himdel

Labels
enhancement fine/backported trees
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@skateman @himdel @miq-bot @hayesr @simaishi