Fix UI mapping for label tag mapping. #780
Conversation
|
@cben Look good? |
|
Thanks! LGTM |
|
cc @zgalor |
|
Not sure what's up with Hakiri lately. |
|
@miq-bot add-label bug, i18n Should fix https://bugzilla.redhat.com/show_bug.cgi?id=1435172 |
| else | ||
| if entity | ||
| entity = entity.split('::').last | ||
| entity = 'VmOrTemplate' if entity == 'Image' |
There was a problem hiding this comment.
so 'Amazon::Vm' becomes ui_lookup(model: 'Vm') == "VM and Instance",
Amazon::Image becomes ui_lookup(model: 'VmOrTemplate') == "VM or Template".
Would user understand the distinction? Perhaps you want something like _("VM") and _("VM Image")?
There was a problem hiding this comment.
@cben I don't see a way around it. Unless there's to an option to Dictionary.gettext that we can pass (which is being called in the ui_lookup method in lib/global_methods.rb), I don't see a way around this for now.
There was a problem hiding this comment.
@djberg96 how abut using ui_lookup(:model => 'ManageIQ::Providers::CloudManager::Template')
|
Also this drops the provider prefix "Kubernetes" / "Amazon" from the UI.
|
djberg96
changed the title
|
@miq-bot add_label wip |
|
@cben There's not much I can do for now, but it is working on the backend, i.e. it's creating the proper tag prefix based on the type so I think we have to live with it for now. That, or we live with the "Pod" vs "ContainerGroup" issue for now. |
|
@bronaghs @bascar This is what he means the UI will look like:
It's not really intuitive which one is an "image". We can go with it for now and refactor later, or we can leave this BZ in place: https://bugzilla.redhat.com/show_bug.cgi?id=1435172 What say you? |
|
Thanks @h-kataria @djberg96 - I tried @h-kataria's suggestion and made a similar change for VMs:
|
|
@bronaghs I think that's as good as we can do for now. Maybe we could add some helper text in there (outside the menu) that let's people know that "Vm" and "Image" are Amazon only. |
|
well what is interesting is that when i use the Amazon namespace, like this: the instances are listed nicely as Amazon in the drop down but templates are not, see screenshot.
|
|
@bronaghs that is due to the fact that there are separate entries to handle different types of instances in the dictionary lookup file, see here https://github.com/ManageIQ/manageiq/blob/master/locale/en.yml#L774-L780 |
|
So I added @h-kataria - It seems thats this has to be implemented for multiple languages, who would make that PR? |
|
@bronaghs all you need to do is add it to en.yml, rest is all automated. |
|
@djberg96 - can you push a commit that adds Then i think this is good. |
|
@bronaghs @h-kataria Ok, I'll add that, thanks! |
|
@bronaghs @h-kataria Please take a look and verify. My local dev instance is refusing to pickup local dev changes now for some reason. |
|
Checked commits https://github.com/djberg96/manageiq-ui-classic/compare/d7d90a18a0b6c03605826b53ee32838ebf9d0fc7~...27439ab201235a4a7ca8bb13dcb3f96099bf8ae9 with ruby 2.2.6, rubocop 0.47.1, and haml-lint 0.20.0 |
|
LGTM |
|
@djberg96 verified changes in UI: |
| when 'Vm' | ||
| model = "ManageIQ::Providers::#{provider}::CloudManager::Vm" | ||
| when 'Image' | ||
| model = "ManageIQ::Providers::#{provider}::CloudManager::Template" |
There was a problem hiding this comment.
clever.
MAPPABLE_ENTITIES const is increasingly coupled to UI logic, would be nice to move it here from core repo (or maybe you have better plans with provider column).
There was a problem hiding this comment.
@cben Yes, unfortunately. This will have to be refactored again. But, at least we know where everything is and what needs to be updated going forward.
djberg96
changed the title
|
@miq-bot remove_label wip |
The change to the MAPPABLE_ENTITIES to "Provider::Resource" format caused in issue in the
entity_ui_name_or_allmethod. First, the model name now needs to be parsed out. Second, the "Image" string doesn't map to an existing model, so we have to specially handle that.