Cat tree refactoring #22
Conversation
… specs) were created from old files, changes in javascript, controller and view was made by hand.
…in query - read_only
|
@MilanPospisil Cannot apply the following label because they are not recognized: ui |
| @@ -138,11 +138,11 @@ def action_field_changed | |||
| def action_tag_pressed | |||
| @edit = session[:edit] | |||
| @action = @edit[:action_id] ? MiqAction.find_by_id(@edit[:action_id]) : MiqAction.new | |||
| tag_name = params[:id].split('__')[1] | |||
| id = TreeBuilder.from_cid(params[:id].split('-')[1]) | |||
There was a problem hiding this comment.
from_cid should be available in controllers without the TreeBuilder...
Or even better, @ZitaNemeckova don't we have a TreeBuilder method that gives you that parsed out id?
.. I'm thinking _, id, _ = TreeBuilder.extract_node_model_and_id(params[:id]) (if it works with Classifications..)
| :click_url => "/miq_policy/action_tag_pressed/", | ||
| :onclick => "miqOnClickTagCat"}) | ||
| = render(:partial => 'shared/tree', | ||
| :locals => {:tree => @cat_tree, :name => @cat_tree.name}) |
| leaf = !object.entries.any? | ||
| img = "tag.png" unless leaf | ||
| node[:icon] = img | ||
| node[:image] = "100/#{img}" |
There was a problem hiding this comment.
You shouldn't need to override the image here, what happens if you don't?
| img = "blank.gif" | ||
| leaf = !object.entries.any? | ||
| img = "tag.png" unless leaf | ||
| node[:icon] = img |
There was a problem hiding this comment.
node[:icon] should not set, unless you really have a fonticon
There was a problem hiding this comment.
Sorry, i forgot it here from old version...
| @@ -68,12 +68,8 @@ | |||
| .form-group | |||
| .col-md-8 | |||
| #action_tags_treebox.treeview-pf-hover.treeview-pf-select | |||
There was a problem hiding this comment.
Either this whole line doesn't belong here anymore, or just the id, but shared/tree already creates a action_tags_treebox, and we don't want two :)
| @tenant = "TestTenant" | ||
| @tenant = "#{@tenant} Tags" | ||
| end | ||
| context 'read-only mode' do |
There was a problem hiding this comment.
newlines please ;) (before every it and context and before, except when first in a block)
And you probably want to look into using let and let!, instead of instance variables here..
| @@ -138,7 +138,7 @@ def action_field_changed | |||
| def action_tag_pressed | |||
| @edit = session[:edit] | |||
| @action = @edit[:action_id] ? MiqAction.find_by_id(@edit[:action_id]) : MiqAction.new | |||
| id = from_cid(params[:id].split('-')[1]) | |||
| _, id = parse_nodetype_and_id(params[:id]) | |||
MilanPospisil
force-pushed
the
cat_tree_refactoring
branch
from
8297934 to
644ea17
Compare
|
Please add instructions on where to find the tree to the description, and, in the future, I recommend doing a before and after screenshot. (It is required in PRs that change the UI in any visible way, not when they don't ... but:... :) ) Found 2 differences:
The list of categories before and after:
Selected tag before:
Selected tag after:
|
|
|
||
| def override(node, object, _pid, _options) | ||
| img = "blank.gif" | ||
| leaf = !object.entries.any? |
There was a problem hiding this comment.
leaf = object.parent_id != 0 doesn't trigger a DB query and probably does the same thing.
(Well, there is a difference for objects with non-null parent_id which do have entries, but that doesn't seem to happen.)
EDIT: verified that the difference is a good thing...
Classification.all.select { |c| c.parent_id != 0 && c.entries.any? } yields [] for me..
..and Classification.all.select { |c| c.parent_id == 0 && c.entries.none? } gives me 2 entries which are clearly categories, but don't have any values that could be assigned as a tag
There was a problem hiding this comment.
Even better .. leaf = !object.category? (category? does parent_id == 0)
| def override(node, object, _pid, _options) | ||
| img = "blank.gif" | ||
| leaf = !object.entries.any? | ||
| img = "tag.png" unless leaf |
There was a problem hiding this comment.
Please try to always keep the whole image name in one place, 100/#{img} means that if you search for 100/tag.png, you won't find it :) (Granted, slightly less evil than 100/#{img}.png but still..)
Also, blank.gif is no longer needed, simple nil would suffice.
So, I'd propose something like
image = leaf ? nil : "100/tag.png"
node[:image] = image && ActionController::Base.helpers.image_path(image)
| leaf = !object.entries.any? | ||
| img = "tag.png" unless leaf | ||
| node[:image] = ActionController::Base.helpers.image_path("100/#{img}") | ||
| node[:title] = object.description |
There was a problem hiding this comment.
Setting node[:title] here seems completely superfluous, it already is that way.
| img = "tag.png" unless leaf | ||
| node[:image] = ActionController::Base.helpers.image_path("100/#{img}") | ||
| node[:title] = object.description | ||
| node[:tooltip] = _("Category: %{description}") % {:description => object.description} |
There was a problem hiding this comment.
Setting node[:tooltip] is also superfluous, already there.
| node[:title] = object.description | ||
| node[:tooltip] = _("Category: %{description}") % {:description => object.description} | ||
| node[:cfmeNoClick] = !leaf | ||
| node[:hideCheckbox] = true |
There was a problem hiding this comment.
node[:hideCheckbox] is also already true, always
| node[:tooltip] = _("Category: %{description}") % {:description => object.description} | ||
| node[:cfmeNoClick] = !leaf | ||
| node[:hideCheckbox] = true | ||
| node |
There was a problem hiding this comment.
No need to return anything here, the call relies on override mutating node, not returning a new one.
There was a problem hiding this comment.
In code yes, but i need some easy way to test this method. Anyway I do not see anyone to test override in other trees, this is maybe because the only way to test it without returing node is using tree traversal on the json result?
| node[:image] = ActionController::Base.helpers.image_path("100/#{img}") | ||
| node[:title] = object.description | ||
| node[:tooltip] = _("Category: %{description}") % {:description => object.description} | ||
| node[:cfmeNoClick] = !leaf |
There was a problem hiding this comment.
Maybe node[:cfmeNoClick] = false if leaf would be more explicit here? But either is fine, feel free to ignore :)
| if cats.any? | ||
| r_kids = [] | ||
| cats.sort_by { |c| c.description.downcase }.each do |c| | ||
| next if c.read_only |
There was a problem hiding this comment.
This is the missing filter that affects those Parent Folder Path (*) (thanks @ZitaNemeckova for the find! :))
|
Checked commits MilanPospisil/manageiq-ui-classic@80e5ec6~...4811281 with ruby 2.2.5, rubocop 0.37.2, and haml-lint 0.16.1 **
app/controllers/miq_policy_controller/miq_actions.rb
|
|
@himdel About that tag to apply problem. It does work on my computer. Maybe check the javascript cache. The js method |
|
@MilanPospisil will do, but if it works for you, then most likely there's another missing EDIT: never mind, you were right, works now :) |
|
Tested in the UI 👍 |
Cat tree renewed from the old saved files - two files (tree class an specs) were created from old files, changes in javascript, controller and view was made by hand.
@miq-bot add_label ui,technical debt
ManageIQ/manageiq#12868
Location:
controller/miq_policy_controller/miq_actions.rb
action_build_cat_tree
app/views/miq_policy/_action_options.html.haml
UI location:
Menu/Control/Explorer/Actions/Configuration(Add new action)/Action type - tags
old pr: ManageIQ/manageiq#12869