Added changes to show Catalog Item type in UI #13516
Conversation
|
|
||
| default_value_for :service_type, 'unknown' | ||
| default_value_for(:generic_subtype) { |st| 'custom' if st.prov_type == 'generic' } | ||
|
|
||
| virtual_has_one :custom_actions, :class_name => "Hash" | ||
| virtual_has_one :custom_action_buttons, :class_name => "Array" | ||
|
|
||
| def prov_type_display | ||
| prov_type ? CATALOG_ITEM_TYPES[prov_type] : '' |
There was a problem hiding this comment.
This could be simplified to: CATALOG_ITEM_TYPES[prov_type].to_s
| @@ -63,6 +63,21 @@ | |||
| end | |||
| end | |||
|
|
|||
| context "#prov_type_display" do | |||
| before(:each) do | |||
There was a problem hiding this comment.
I realized this is used elsewhere in this file, but you do not need to include (:each) as it is the default.
|
Two minor comments but otherwise looks good. |
h-kataria
force-pushed
the
show_catalog_item_type
branch
from
6f4194c to
95ac58a
Compare
|
@gmcculloug addressed your comments. |
|
|
||
| default_value_for :service_type, 'unknown' | ||
| default_value_for(:generic_subtype) { |st| 'custom' if st.prov_type == 'generic' } | ||
|
|
||
| virtual_has_one :custom_actions, :class_name => "Hash" | ||
| virtual_has_one :custom_action_buttons, :class_name => "Array" | ||
|
|
||
| def prov_type_display |
There was a problem hiding this comment.
@h-kataria what is a prov_type?
There was a problem hiding this comment.
@h-kataria Bigger question is that this looks like UI code that should probably live in a UI presenter in the Classic UI repo? /cc @Fryguy @dclarizio
There was a problem hiding this comment.
@chessbyte prov_type is a column in service_templates table, i needed to show nice display value for prov_type in the list views, which is why i have added a virtual column here
| @@ -10,6 +10,20 @@ class ServiceTemplate < ApplicationRecord | |||
| "storage" => _("Storage") | |||
| }.freeze | |||
|
|
|||
|
|
|||
| CATALOG_ITEM_TYPES = { | |||
There was a problem hiding this comment.
@h-kataria Is the idea to change the catalog_item_types method from the classic UI repo and have it reference this constant? We would not want to have this defined in multiple places.
@Fryguy @dclarizio Any thoughts here?
Ultimately this list needs to make it's way into the pluggable providers. @durandom
There was a problem hiding this comment.
@gmcculloug usually we try to keep list of types in a constant in models, but in this case it has been in controller from the beginning, but now since we need to expose the type as a nice display value in the list view. I thought we should move this into the model and reference the constant from controllers/views.
There was a problem hiding this comment.
Is there an object you can infer the name from? Or something like all subclasses of a base class? Who fills prov_type? Maybe this can be filled with some inflection methods https://github.com/ManageIQ/manageiq/blob/master/app/models/manageiq/providers/inflector.rb ?
I also think this is rather a UI issue, they should infer the display string from prov_type
There was a problem hiding this comment.
I think that more that just one UI might need to have a nice name for the catalog item type. Also it does not make sense to me for the UIs to track what types of catalog item the backend has.
So I'd say it should stay on the backend side.
Note: although it needs not look like that now, the values might need i18n care. Ping @mzazrivec
There was a problem hiding this comment.
@martinpovolny addressed you comment, moving code to display nice value for prov_type back to UI PR.
| @@ -10,6 +10,20 @@ class ServiceTemplate < ApplicationRecord | |||
| "storage" => _("Storage") | |||
| }.freeze | |||
|
|
|||
|
|
|||
| CATALOG_ITEM_TYPES = { | |||
There was a problem hiding this comment.
Is there an object you can infer the name from? Or something like all subclasses of a base class? Who fills prov_type? Maybe this can be filled with some inflection methods https://github.com/ManageIQ/manageiq/blob/master/app/models/manageiq/providers/inflector.rb ?
I also think this is rather a UI issue, they should infer the display string from prov_type
h-kataria
force-pushed
the
show_catalog_item_type
branch
from
95ac58a to
c02d804
Compare
h-kataria
force-pushed
the
show_catalog_item_type
branch
from
c02d804 to
a729022
Compare
|
Checked commit h-kataria@a729022 with ruby 2.2.6, rubocop 0.46.0, and haml-lint 0.19.0 app/models/service_template.rb
|
|
@durandom i have revised this PR and moved out the code that deals with displaying nice value for prov_type in UI. Can you please re-review. |
|
@h-kataria Still you'll have to re-visit that list once we add a new provider.
I thought of leveraging something like these. But I leave it up to @gmcculloug |
|
@gmcculloug please review comments from @durandom |
|
We should have the value somewhere at the provider so that we don't have to revisit this list when adding a new provider. But let's fix that in a follow up PR. This is taking far too long. |
Moved CATALOG_ITEM_TYPES and added virtual column in model so it can be used to display Catalog Item type in list view and other screens.
https://bugzilla.redhat.com/show_bug.cgi?id=1348239
@gmcculloug @dclarizio please review
see UI screenshots below show "Item Type" on list view and other screens