GTL Lists cleanup and unification

[martinpovolny]

2 paths, 2 calls for the first display of a GTL list

exceptions e.g. in process_params_options several different types exception for foreman in report_data

report_data, process_params_options

model.association

2 main ways to display nested list in the UI:

• In Explorer screens get_view in ExplorerShow keys: :parent :association :conditions :dbname

• and in non-explorer screens get_view in GenericShowMixin :parent :association :parent_method :where_clause

ping @Ladas, @karelhala, @lpichler

[lpichler]

from ./app/controllers/application_controller.rb:433

  def process_params_options(params)
    options = {}
    @explorer = params[:explorer] == "true" if params[:explorer]

    if params[:active_tree] && defined? get_node_info
      node_info = get_node_info(x_node, false)
      options.merge!(node_info) if node_info.kind_of?(Hash)
    end
    if params[:model] && %w(miq_requests).include?(params[:model])
      options = page_params
    end
    if params[:model] && %w(miq_tasks).include?(params[:model])
      options = jobs_info
    end
    if params[:model] && %w(cloud_networks).include?(params[:model])
      options = rbac_params
    end
  end
...

there are 3 exceptions but each one for the different reason. I think we need to catch all exceptions, classificate them to the categories and determine why we have it and how we can solve the category. for example, rbac related things can be inlcuded in rbac(I believe) we don't need any params, but for example jobs_info has the different reason.

[martinpovolny]

This is @karelhala 's PR: https://github.com/ManageIQ/manageiq-ui-classic/pull/592

I hoped to clean this up as we move to an API. But we have no input about API creation from the core team https://github.com/ManageIQ/manageiq/issues/14924 . So I guess to make sure that things work we shall start cleaning up the exceptions w/o an API.

[martinpovolny]

readability fix https://github.com/ManageIQ/manageiq-ui-classic/pull/2098

[Ladas]

1. We should keep the parameters we send e.g. here https://github.com/Ladas/manageiq-ui-classic/blob/36c5a957b6fcad6a89d81bb8238c70ef04a55d1e/app/controllers/mixins/generic_show_mixin.rb#L151
2. In the current controllers we support only explicitly defined paths we can load with controller base list and nested lists limited by https://github.com/Ladas/manageiq-ui-classic/blob/36c5a957b6fcad6a89d81bb8238c70ef04a55d1e/app/controllers/mixins/generic_show_mixin.rb#L32 . I think the report data supports every model association we throw at it (filtered by RBAC at least). Since we do not have routes, we should have it limited by something.
3. We should not render the views twice, should be enough to disable rendering by the generic mixin

[karelhala]

@Ladas to use nested lists as shown by your comment, I created PR https://github.com/ManageIQ/manageiq-ui-classic/pull/1835 (looking at it now, it is not good, fetch of record is wrong) which will call such function.

However it would be much better if we could somehow define what properties has to be used for specific models. Let's say for displaying hosts under ems we need just the model (Hosts), but for storage_managers we need model and parent_method. So my guess is to create some structure where you could define for each controller some specific behavior for specific models:

module EmsCommon
  #...
  nested_models = {
    "block_storage_manager" => {
      :model => ManageIQ::Providers::StorageManager, 
      :options => {:parent_method => :block_storage_managers},
    }
    "storage_manager" => {
      :model => ManageIQ::Providers::StorageManager,
      :options => {:parent_method => :storage_managers}
    },
    "host" => {
      :model => Hosts
    }
  }.freeze
  #...
end

To use this, you can do this:

module ReportData
  def get_report_data
    #...
    current_model, options = nested_models[params[:model_name]]
    if params[:parent_id]
      options[:parent] = #logic to find parent for model from params[:model_name]
    end
    view, pages = get_view(current_model, options)
  end
end

No need to use hash, but it's convenient to show it (we could use something more generic or some better structure).

The most important part for generating report data is part of get_view method application_controller.rb#L1590-L1608 so the ultimate goal is to move this part to it's own method, and somehow figure out where to fetch all these additional properties.

[Ladas]

@karelhala @martinpovolny I think what we are trying to solve here is already solved by Rails for some time, it's called nested resources, i.e. shallow nesting (to avoid nesting hell) http://guides.rubyonrails.org/routing.html#shallow-nesting

But since we do not use routes like that, do we want to mimic the behavior? (I fine with jumping on using routes the right way :-)) I think for the nesting, there should be exact rules of what can be displayed under each entity type. With the generic_show_mixin, those rules were for each controller and each controller could contain a specific params for the nested call if needed (where we would like to get rid of the specific behavior if possible)

Another way is to define just nested_models but I am not sure if I like that we allow that nested call for every model (comparing to strict route schema)? So I think it should be somehow structured.

[martinpovolny]

@Ladas: you might have noticed that with nested routes route A/B/ is handled by the controller for B. While in our case it's the other way round. A/B/ in handled by A.

I see no easy way of reverting that.

Any way, to revert it or not, we have a piece of information that is currently hidden in the display_methods. That information is used in GenericShowMixin::show. We need the very same information in the ../report_data.

We have generally 2 options:

1. pass all the information into the GTL component and have the GTL component send that data when accessing ../report_data.
2. pass throught just an identifier as we do in https://github.com/Ladas/manageiq-ui-classic/blob/36c5a957b6fcad6a89d81bb8238c70ef04a55d1e/app/controllers/mixins/generic_show_mixin.rb#L33 then we would need a shared structure such as @karelhala suggests above that would have the information for both display_methods and report_data.

If we clean up the code enough so that just the definition and some generic code remain we might be able to convert our codebase to use the normal Rails nested routes.

[Ladas]

@martinpovolny @karelhala I think the option 2 is the way for now, since we are passing things like SQL conditions(we should not do that but we do), we can't easily expose that.

[JPrause]

@martinpovolny is this still a valid issue? If yes, lease remove the stale label. If not can you close. If there's no update by next week, I'll be closing this issue.

[JPrause]

Closing issue. If you feel the issue needs to remain open, please either reopen or let me know and it will be reopened. @miq-bot close_issue

[JPrause]

@miq-bot remove_label stale

[gtanzillo]

Closing because the removal of grid and tile and move ot the list view to react