Fix specs related to widget set factories #7594

lpichler · 2021-01-21T11:36:10Z

Continuation of #7593,
where all changes from #7593 were added and
in addition other fixes added to fix specs.

Needs core PR: ManageIQ/manageiq#20972

Links

Improvements for MiqWidgetSet model manageiq#20972 (required core PR)
Update widget specs to set owner and set_data #7593 can be replaced with this PR

After changes, we want to ensure all records have an owner and the proper set_data There is a new requirement that a widget set contains at least one widget and that there is a group assigned to all widget sets

widget sets need widgets and those need reports Also, to have proper date time, a server needs to be present

lpichler · 2021-01-21T11:36:40Z

@miq-bot cross-repo-tests ManageIQ/manageiq#20972

From Pull Request: ManageIQ/manageiq-ui-classic#7594

lpichler · 2021-01-21T13:53:31Z

@miq-bot cross_repo_tests #7594 including ManageIQ/manageiq#20972

From Pull Request: ManageIQ/manageiq-ui-classic#7594

kbrock

Thanks @lpichler

kbrock · 2021-01-21T14:24:27Z

spec/controllers/dashboard_controller_spec.rb

-                        :set_data => {:last_group_db_updated => Time.now.utc, :col1 => [1], :col2 => [], :col3 => []})
+                        :name     => @user.userid.to_s,
+                        :owner    => @user,
+                        :group_id => @user.current_group_id,


Does this mean we should fix the before validation in the model to work with users?
Thinking having a user_with_group may be what we want

kbrock · 2021-01-21T14:27:03Z

spec/controllers/dashboard_controller_spec.rb

      let(:ws1) do
        FactoryBot.create(:miq_widget_set,
                          :name     => 'A',
-                          :owner_id => group.id,
-                          :set_data => {:col1 => [], :col2 => [], :col3 => []})
+                          :set_data => {:col1 => [widget.id],


I noticed that a lot of these sets were incomplete which made me uncomfortable. That is where the smarter factory with encapsulation came into play. An alternative would be to put the setter into the actual model and treat these like fields (which is probably the correct schema/interface)

kbrock · 2021-01-21T14:28:16Z

spec/controllers/miq_report_controller/trees_spec.rb

@@ -88,6 +88,8 @@
      end

      it 'renders list of Dashboards in Dashboards tree' do


Not thrilled about these but had trouble seeding a widget set without valid data. I guess manually creating the default would have worked as well

kbrock · 2021-01-21T15:45:59Z

spec/controllers/miq_report_controller/trees_spec.rb

      it 'renders show of Dashboards in Dashboards tree' do
        ApplicationController.handle_exceptions = true

        MiqWidgetSet.seed
-        widget_set = FactoryBot.create(:miq_widget_set, :group_id => user.current_group.id)
+        widget_set = FactoryBot.create(:miq_widget_set, :set_data => {:col1 => [miq_widget.id]}, :owner_id => user.current_group.id, :group_id => user.current_group.id)


there are a bunch of places where the owner_type is not set - for this reason the group was passed in. if you pass owner (or owner_id and owner_type) then group will get set correctly

kbrock · 2021-01-21T15:47:12Z

spec/controllers/miq_report_controller/dashboards_spec.rb

-        controller.instance_variable_set(:@edit, :new => new_hash, :db_id => miq_widget_set.id, :current => current)
+        new_widget = FactoryBot.create(:miq_widget)
+        new_hash = {:name => "New Name", :description => "New Description", :col1 => [new_widget.id], :col2 => [], :col3 => []}
+        controller.instance_variable_set(:@edit, :new => new_hash, :db_id => miq_widget_set.id, :current => miq_widget_set.set_data)


wasn't sure if this needed a deep clone. One thing is for certain, setting this to an invalid hash (with no widgets at all) was not productive

kbrock · 2021-01-21T16:23:13Z

This is part of the whole ManageIQ/manageiq#20890 chain

kbrock

While I'm not thrilled with the whole set_data stuff, we can save that for another PR

my main 2 sticking points:

passing owner_id and not owner_type.
passing owner => user and having to manually set the group.

on that second one in particular, I thought the purpose of these PRs were to auto set the group.
In the controllers, do we work the same way?

h-kataria · 2021-01-22T14:47:53Z

While I'm not thrilled with the whole set_data stuff, we can save that for another PR

my main 2 sticking points:
* passing `owner_id` and not `owner_type`.

* passing `owner => user` and having to manually set the group.
on that second one in particular, I thought the purpose of these PRs were to auto set the group.
In the controllers, do we work the same way?

creating a user with FactoryBot.create(:user_with_group) should take care of that

miq-bot · 2021-01-25T12:01:07Z

Checked commits lpichler/manageiq-ui-classic@135c397~...ec55b7a with ruby 2.6.3, rubocop 0.82.0, haml-lint 0.35.0, and yamllint
4 files checked, 4 offenses detected

spec/controllers/dashboard_controller_spec.rb

❗ - Line 335, Col 1 - Layout/EmptyLines - Extra blank line detected.
❗ - Line 341, Col 9 - Layout/HashAlignment - Align the keys and values of a hash literal if they span more than one line.
❗ - Line 472, Col 27 - Layout/HashAlignment - Align the keys and values of a hash literal if they span more than one line.
❗ - Line 477, Col 27 - Layout/HashAlignment - Align the keys and values of a hash literal if they span more than one line.

h-kataria · 2021-01-25T17:46:07Z

@lpichler can you take a look at spec test failures, look related to me.

kbrock

thanks Libor

kbrock and others added 5 commits January 20, 2021 17:31

Update widget specs to set owner and set_data

135c397

After changes, we want to ensure all records have an owner and the proper set_data There is a new requirement that a widget set contains at least one widget and that there is a group assigned to all widget sets

fix dashboard widget specs for report data

6ad6f0f

widget sets need widgets and those need reports Also, to have proper date time, a server needs to be present

Fix specs in dashboard controller specs

b9379a8

Fix specs in report dashboard controller specs

be25fda

Fix specs in tree specs

ceb4ba5

miq-bot pushed a commit to ManageIQ/manageiq-cross_repo-tests that referenced this pull request Jan 21, 2021

Running tests for lpichler

3cf260e

From Pull Request: ManageIQ/manageiq-ui-classic#7594

miq-bot mentioned this pull request Jan 21, 2021

[BOT] Cross repo test for ManageIQ/manageiq-ui-classic#7594 ManageIQ/manageiq-cross_repo-tests#273

Closed

Fix rubocop in dashboard controller spec

c9db235

lpichler force-pushed the fix_spec_for_widget_set branch from 33c1402 to c9db235 Compare January 21, 2021 12:29

miq-bot pushed a commit to ManageIQ/manageiq-cross_repo-tests that referenced this pull request Jan 21, 2021

Running tests for lpichler

0f7a7d8

From Pull Request: ManageIQ/manageiq-ui-classic#7594

miq-bot mentioned this pull request Jan 21, 2021

[BOT] Cross repo test for ManageIQ/manageiq-ui-classic#7594 ManageIQ/manageiq-cross_repo-tests#275

Closed

miq-bot pushed a commit to ManageIQ/manageiq-cross_repo-tests that referenced this pull request Jan 21, 2021

Running tests for lpichler

3645d7e

From Pull Request: ManageIQ/manageiq-ui-classic#7594

miq-bot mentioned this pull request Jan 21, 2021

[BOT] Cross repo test for ManageIQ/manageiq-ui-classic#7594 ManageIQ/manageiq-cross_repo-tests#277

Closed

kbrock reviewed Jan 21, 2021

View reviewed changes

kbrock mentioned this pull request Jan 21, 2021

Update widget specs to set owner and set_data #7593

Closed

kbrock reviewed Jan 21, 2021

View reviewed changes

gtanzillo assigned h-kataria Jan 21, 2021

gtanzillo added bug test labels Jan 21, 2021

Let factories create widget in dashboard controller

ec55b7a

lpichler mentioned this pull request Jan 25, 2021

Improvements for MiqWidgetSet model ManageIQ/manageiq#20972

Merged

lpichler closed this Jan 25, 2021

lpichler reopened this Jan 25, 2021

kbrock approved these changes Jan 25, 2021

View reviewed changes

h-kataria merged commit 37e9605 into ManageIQ:master Jan 25, 2021

lpichler deleted the fix_spec_for_widget_set branch January 26, 2021 06:44

kbrock mentioned this pull request Dec 3, 2021

Missing endpoint for manipulating with dashboards ManageIQ/manageiq-api#953

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix specs related to widget set factories #7594

Fix specs related to widget set factories #7594

lpichler commented Jan 21, 2021 •

edited by kbrock

Loading

lpichler commented Jan 21, 2021

lpichler commented Jan 21, 2021

kbrock left a comment

kbrock Jan 21, 2021

kbrock Jan 21, 2021

kbrock Jan 21, 2021

kbrock Jan 21, 2021

kbrock Jan 21, 2021

kbrock commented Jan 21, 2021

kbrock left a comment

h-kataria commented Jan 22, 2021 •

edited

Loading

miq-bot commented Jan 25, 2021

h-kataria commented Jan 25, 2021

kbrock left a comment

		@@ -88,6 +88,8 @@
		end

		it 'renders list of Dashboards in Dashboards tree' do

Fix specs related to widget set factories #7594

Fix specs related to widget set factories #7594

Conversation

lpichler commented Jan 21, 2021 • edited by kbrock Loading

Links

lpichler commented Jan 21, 2021

lpichler commented Jan 21, 2021

kbrock left a comment

Choose a reason for hiding this comment

kbrock Jan 21, 2021

Choose a reason for hiding this comment

kbrock Jan 21, 2021

Choose a reason for hiding this comment

kbrock Jan 21, 2021

Choose a reason for hiding this comment

kbrock Jan 21, 2021

Choose a reason for hiding this comment

kbrock Jan 21, 2021

Choose a reason for hiding this comment

kbrock commented Jan 21, 2021

kbrock left a comment

Choose a reason for hiding this comment

h-kataria commented Jan 22, 2021 • edited Loading

miq-bot commented Jan 25, 2021

h-kataria commented Jan 25, 2021

kbrock left a comment

Choose a reason for hiding this comment

lpichler commented Jan 21, 2021 •

edited by kbrock

Loading

h-kataria commented Jan 22, 2021 •

edited

Loading