miq_policy/_policy_folders.html.haml is bogus

[cben]

https://github.com/ManageIQ/manageiq/blob/master/app/views/miq_policy/_policy_folders.html.haml

@miq-bot add_label control, ui, technical debt

- The incoming `@folders` array is either `["Compliance", "Control"]` at root level or e.g. `["Host Compliance", "Vm Compliance", "Container Image Compliance"]` at 2nd level. - Root level is detected by literally checking for `%w(Compliance Control)`. In that case we do needless `.split.first` on the single-word string, and call it `model_name` despite not being a model. - We need that for the icon `"100/#{model_name.underscore}.png"`. - At 2nd level, we reconstruct the model_name by from the user-readable string carefully crafted in [miq_policy_controller](https://github.com/ManageIQ/manageiq/blob/69f9b91b700f46f214b7f94ce02f9308fd3f9faf/app/controllers/miq_policy_controller.rb#L957-L959) using `model.name.titleize`: we lope off the last word which we know to be "Control" or "Compliance", join the model name back, and lowerCamelize. - Then we do a bogus ui_lookup attempt. See, in this branch `model_name` _is_ a model name (though not UpperCamelized), but instead we lookup `f` — for any f as long as `f == "Vm"`, which it never is. - However the `_("%s Policies")` part works eg. "»Container Image Compliance ポリシー«".

I'll be happy to rewrite this (split into 2 views, pass structured @folders).
But there is some interaction with #8815 which is darga/yes.
=> Will it be OK to backport this refactoring too?

[dclarizio]

@martinpovolny please reply to @cben's questions and suggest a path forward that is in line with our refactoring strategy.
@cben we rarely backport refactoring work due to the risk.

[cben]

Thanks, this helps me proceed.
I'll try making a minimal PR that just fixes ui_lookup here and see if that gets accepted to darga (if not, actually not a big deal).
After I finish my other darga/yes stuff, I'll be happy to do a bigger darga/no refactor.

Higher-level question

This view (and friends e.g. _condition_folders) renders into the right side essentially the same info as the controllers & tree_builder render into the explorer.
Here we get "dumb" data from controller and the view does some "where-we-are" detection, link target assembly, icon choice & translation.
In the left side, all that logic lives in the controllers & tree_builder.

Are there plans to share code between the 2 sides, for simple list-the-children views?

[martinpovolny]

@cben : Feel free to cleanup the view. There are many to be cleaned up.

I can give you a few points on refactoring:

1. Only now we are finishing the cleanup of trees (moving the code to separate classes)
2. We are also finishing i18n: toolbars, model virtual columns and the ui_lookup being the most prominent things I know about
3. We are still working on toolbars -- see Toolbar Redesign #6554
4. Simple 'list-the-children' is mostly covered by GTLs (which are a priority at this point and @karelhala is working on that).
5. Simple 'show-an-item' usecase is mostly covered by TextualSummary and that has been refactored a lot already and looks quite well I'd say.
6. We work on the stuff that has most effect (affects most screens or users or developers) first. This means that individual screen cleanups are not a priority unless someone is doing some major work in given area. You are welcome ;-)
7. Form screens are to be converted to Angular. So before cleaning up any, think about converting it. Talk to @himdel, @AparnaKarve, @lgalis about that.
8. Sharing code: Honestly I don't know. First thing that comes to my mind are the Decorator classes that where added recently. That might be the right place to put presentation logic that is shared between different places where a particular type of item is displayed. Such as, as you suggested, trees and the main content area.

[martinpovolny]

Notes on the refactoring needed here:

There should be no need for the folders_i18n at this point.

There should be a decorator available (or new one created) to get the image_path.