Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Unify endpoint data for report data. #2195

Merged
merged 1 commit into from
Oct 2, 2017
Merged

Unify endpoint data for report data. #2195

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 6 additions & 5 deletions app/assets/javascripts/controllers/report_data_controller.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -275,7 +275,7 @@
* modelName: string,
* gtlType: string,
* activeTree: string,
* currId: string,
* parentId: string,
* isExplorer: Boolean
* }
* ```
Expand All @@ -289,7 +289,7 @@
this.setExtraClasses(initObject.gtlType);
return this.getData(initObject.modelName,
initObject.activeTree,
initObject.currId,
initObject.parentId,
initObject.isExplorer,
this.settings,
initObject.records)
Expand Down Expand Up @@ -402,20 +402,21 @@
* Method for fetching data from server. gtlData, settings and pePage is selected after fetching data.
* @param {String} modelName name of current model.
* @param {Number} activeTree ID of active tree node.
* @param {Number} currId current Id, if some nested items are displayed.
* @param {Number} parentId parent Id, if some nested items are displayed.
* @param {Boolean} isExplorer true | false if we are in explorer part of application.
* @param {Object} settings settings object.
* @param {Array} records array of reccords.
* @returns {Object} promise of retriveRowsAndColumnsFromUrl of MiQDataTableService.
*/
ReportDataController.prototype.getData = function(modelName, activeTree, currId, isExplorer, settings, records) {
ReportDataController.prototype.getData = function(modelName, activeTree, parentId, isExplorer, settings, records) {
var basicSettings = {
current: 1,
perpage: 20,
sort_col: 0,
sort_dir: 'DESC',
};
return this.MiQDataTableService.retrieveRowsAndColumnsFromUrl(modelName, activeTree, currId, isExplorer, settings, records)
return this.MiQDataTableService
.retrieveRowsAndColumnsFromUrl(modelName, activeTree, parentId, isExplorer, settings, records)
.then(function(gtlData) {
this.settings = gtlData.settings || basicSettings;
if (this.settings.sort_col === -1) {
Expand Down
26 changes: 13 additions & 13 deletions app/controllers/application_controller.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -410,10 +410,10 @@ def init_report_data(controller_name)
{
:controller_name => controller_name,
:data => {
:modelName => @display.nil? && !self.class.model.nil? ? self.class.model.to_s.tableize : @display,
:model_name => @display.nil? && !self.class.model.nil? ? self.class.model.to_s.tableize : @display,
:activeTree => x_active_tree.to_s,
:gtlType => @gtl_type,
:currId => params[:id],
:parent_id => params[:id],
:sortColIdx => @sortcol,
:sortDir => @sortdir,
:isExplorer => @explorer,
Expand Down Expand Up @@ -444,8 +444,8 @@ def process_params_options(params)
end

# handle exceptions
if params[:model]
options = case params[:model]
if params[:model_name]
options = case params[:model_name]
when 'miq_requests'
page_params
when 'miq_tasks'
Expand All @@ -459,15 +459,15 @@ def process_params_options(params)
end
end

if params[:model_id] && !params[:active_tree]
curr_model_id = from_cid(params[:model_id])
unless curr_model_id.nil?
options[:parent] = identify_record(curr_model_id, controller_to_model) if curr_model_id && options[:parent].nil?
if params[:id] && !params[:active_tree]
parent_id = from_cid(params[:parent_id])
unless parent_id.nil?
options[:parent] = identify_record(parent_id, controller_to_model) if parent_id && options[:parent].nil?
end
end

options[:parent] = options[:parent] || @parent
options[:association] = HAS_ASSOCATION[params[:model]] if HAS_ASSOCATION.include?(params[:model])
options[:association] = HAS_ASSOCATION[params[:model_name]] if HAS_ASSOCATION.include?(params[:model_name])
Copy link
Contributor

@Ladas Ladas Sep 20, 2017 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am a bit confused by mixing :association and :model name, those were 2 different things before?

E.g. model_name=ExtManagementSystem and association=:vms with parent_id=1, I would expect we do
ExtManagementSystem.find(1).vms, which list Vms under that ems where :vms model_name can be e.g Vm

We do that call, but we call is as identify_record(parent_id, controller_to_model).send(HAS_ASSOCATION[params[:model_name]]) ?

I think before we were just using :association directly, while having allowed list of associations for each controller. Now we kind of have allowed list of associations per :model_name globally, in HAS_ASSOCATION?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

e.g before if you had controller_name/1/?diplay=all_vms

it could map to this
https://github.com/Ladas/manageiq-ui-classic/blob/36c5a957b6fcad6a89d81bb8238c70ef04a55d1e/app/controllers/mixins/generic_show_mixin.rb#L151

or going to generic, which does https://github.com/Ladas/manageiq-ui-classic/blob/36c5a957b6fcad6a89d81bb8238c70ef04a55d1e/app/controllers/mixins/generic_show_mixin.rb#L139

while expecting to have list of allowed sub-tables per controller like this https://github.com/Ladas/manageiq-ui-classic/blob/35dbd795ba08c3778866245dfce91b4b5d88a76d/app/controllers/network_port_controller.rb#L18

of course the explorer views were more complex and weren't ported to this format :-(

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

so to sum what I try to say :-)

  • we had allowed list of controller subtables
  • we were catching specific sublists in generic_show_mixins, because they could not be generic for some reason (we are not passing all args to UI and back to report data now)
  • naming is confusing (but it was confusing before) since get_view takes model as a first arg which is taken from @display , so maybe we need to take this up to @lpichler :-)

Copy link
Member

@martinpovolny martinpovolny Sep 21, 2017 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

of course the explorer views were more complex and weren't ported to this format :-(

Yes, the explorer views where/are even more mess. I already limited the number of different patterns for nested lists in explorer view but it's still pretty messy. It combines the single and list views deeper in the code and it will need more work to cleanup.

Naming is and was confusing. The methods on the get_view* code path have different names for the same thing on several places and it's not easy to fix.

Let's do small steps that improve the situation. We don't need to fix too much in a single PR.

options[:selected_ids] = params[:records]
options
end
Expand All @@ -483,15 +483,15 @@ def process_params_options(params)
# @option options :model [Object]
# If model was chosen somehow before calling this method use this model instead of finding it.
def process_params_model_view(params, options)
if options[:model]
model_view = options[:model].constantize
if options[:model_name]
model_view = options[:model_name].constantize
end

if model_view.nil? && params[:active_tree]
model_view = model_from_active_tree(params[:active_tree].to_sym)
end
if model_view.nil? && params[:model]
model_view = model_string_to_constant(params[:model])
if model_view.nil? && params[:model_name]
model_view = model_string_to_constant(params[:model_name])
end

if model_view.nil?
Expand Down
6 changes: 3 additions & 3 deletions app/views/layouts/angular/_gtl.html.haml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
- current_model = model_to_report_data
- model_name = model_to_report_data
- no_flash_div ||= false
- gtl_type_string = j_str(@gtl_type)
- active_tree = x_active_tree unless params[:display] || @use_action
Expand Down Expand Up @@ -40,10 +40,10 @@
sendDataWithRx({initController: {
name: 'reportDataController',
data: {
modelName: '#{h(j_str(current_model))}',
modelName: '#{h(j_str(model_name))}',
activeTree: '#{active_tree}',
gtlType: '#{h(gtl_type_string)}',
currId: '#{h(j_str(params[:id])) unless @display.nil?}',
parent_id: '#{h(j_str(params[:id])) unless @display.nil?}',
sortColIdx: '#{@sortcol}',
sortDir: '#{@sortdir}',
isExplorer: '#{@explorer}' === 'true' ? true : false,
Expand Down
6 changes: 3 additions & 3 deletions spec/controllers/provider_foreman_controller_spec.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -336,7 +336,7 @@
gtl_init_data = controller.init_report_data('reportDataController')
expect(gtl_init_data[:data][:modelName]).to eq("manageiq/providers/configuration_managers")
expect(gtl_init_data[:data][:activeTree]).to eq("configuration_manager_providers_tree")
expect(gtl_init_data[:data][:currId]).to eq(ems_id)
expect(gtl_init_data[:data][:parent_id]).to eq(ems_id)
expect(gtl_init_data[:data][:isExplorer]).to eq(true)
view = controller.instance_variable_get(:@view)
expect(view.table.data[0].description).to eq("testprofile")
Expand All @@ -351,7 +351,7 @@
gtl_init_data = controller.init_report_data('reportDataController')
expect(gtl_init_data[:data][:modelName]).to eq("manageiq/providers/configuration_managers")
expect(gtl_init_data[:data][:activeTree]).to eq("configuration_manager_providers_tree")
expect(gtl_init_data[:data][:currId]).to eq(config_profile_id2)
expect(gtl_init_data[:data][:parent_id]).to eq(config_profile_id2)
expect(gtl_init_data[:data][:isExplorer]).to eq(true)
view = controller.instance_variable_get(:@view)
expect(view.table.data[0].hostname).to eq("test2a_configured_system")
Expand Down Expand Up @@ -386,7 +386,7 @@
gtl_init_data = controller.init_report_data('reportDataController')
expect(gtl_init_data[:data][:modelName]).to eq("manageiq/providers/configuration_managers")
expect(gtl_init_data[:data][:activeTree]).to eq("configuration_manager_providers_tree")
expect(gtl_init_data[:data][:currId]).to eq(ems_id)
expect(gtl_init_data[:data][:parent_id]).to eq(ems_id)
expect(gtl_init_data[:data][:isExplorer]).to eq(true)
expect(view.table.data[0].data).to include('description' => "testprofile")
expect(view.table.data[2]).to include('description' => _("Unassigned Profiles Group"),
Expand Down
2 changes: 1 addition & 1 deletion spec/controllers/storage_controller_spec.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -119,7 +119,7 @@
expect(response.body).to include("modelName: '#{session_storage[:active_accord].to_s.pluralize}'")
expect(response.body).to include("activeTree: '#{session_storage[:active_tree]}'")
expect(response.body).to include("gtlType: 'list'")
expect(response.body).to include("currId: ''")
expect(response.body).to include("parent_id: ''")
expect(response.body).to include("sortColIdx: '0'")
expect(response.body).to include("isExplorer: 'true' === 'true' ? true : false")
expect(response.body).to include("showUrl: '/#{session_storage[:active_accord]}/x_show/'")
Expand Down
4 changes: 2 additions & 2 deletions spec/javascripts/controllers/report_data_controller_spec.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@ describe('reportDataController', function () {
modelName: 'manageiq/providers/infra_manager/vms',
activeTree: 'vandt_tree',
gtlType: 'grid',
currId: '',
parent_id: '',
sortColIdx: '0',
sortDir: 'DESC',
isExplorer: false,
Expand Down Expand Up @@ -44,7 +44,7 @@ describe('reportDataController', function () {
it('should get data', function(done) {
var settings = {isLoading: true};
var result = $controller
.getData(initObject.modelName, initObject.activeTree, initObject.currId, initObject.isExplorer, settings);
.getData(initObject.modelName, initObject.activeTree, initObject.parent_id, initObject.isExplorer, settings);
result.then(function(data) {
expect(angular.equals($controller.gtlData.cols, report_data.data.head)).toBeTruthy();
expect(angular.equals($controller.gtlData.rows, report_data.data.rows)).toBeTruthy();
Expand Down
2 changes: 1 addition & 1 deletion spec/support/controller_helper.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -63,7 +63,7 @@ def assert_nested_list(parent, children, relation, label, child_path: nil, gtl_t
expect(response.body).to include("modelName: '#{relation}'")
expect(response.body).to include("activeTree: ''")
expect(response.body).to include("gtlType: '#{gtl_types.first}'")
expect(response.body).to include("currId: '#{parent.id}'")
expect(response.body).to include("parent_id: '#{parent.id}'")
expect(response.body).to include("showUrl: '/#{child_route}/'")
end

Expand Down