Ownership refactor #1578

PanSpagetka · 2017-06-21T10:28:32Z

Ownership refactor

Compute -> Infrastructure -> VMs -> select VM -> Lifecycle -> Retire/Set Retirement date

romanblanco

@PanSpagetka please see the comments and update PR

romanblanco · 2017-06-28T12:21:19Z

app/controllers/mixins/actions/vm_actions/ownership.rb

-            @refresh_partial = "layouts/flash_msg"
-            return
-          end
+          ownership_items = recs.collect(&:to_i)


do we need ownership_items variable here in set_ownership method?

romanblanco · 2017-06-28T12:37:54Z

app/controllers/mixins/actions/vm_actions/ownership.rb

-          user = record.evm_owner if ownership_items.length == 1
-          @user = user ? user.id.to_s : nil
+          ownershipitems = find_records_with_rbac(klass, ownership_items).sort_by(&:name)
+          record = ownershipitems.first


This part can be extracted in new method, you are using the same code in build_ownership_hash

ownershipitems = find_records_with_rbac(klass, ownership_items).sort_by(&:name) record = ownershipitems.first if ownership_items.length > 1 @user = @group = 'dont-change' else @user = record.evm_owner&.id&.to_s @group = record.miq_group&.id&.to_s end

romanblanco · 2017-06-28T12:47:01Z

app/controllers/mixins/actions/vm_actions/ownership.rb

-              add_flash(_("All changes have been reset"), :warning)
-              request.parameters[:controller] == "service" ? replace_right_cell(:nodetype => "ownership") : replace_right_cell
+          result = klass.set_ownership(param_ids, opts)
+          unless result == true


unless result == true can we get rid of this?

lpichler · 2017-06-28T13:06:43Z

app/controllers/mixins/actions/vm_actions/ownership.rb

@@ -136,7 +116,7 @@ def build_ownership_hash(ownership_items)
          @group = group ? group.id.to_s : nil
          Rbac.filtered(MiqGroup).each { |g| @groups[g.description] = g.id.to_s }
          @user = @group = 'dont-change' if ownership_items.length > 1
-          @ownershipitems = Rbac.filtered(klass.where(:id => ownership_items).order(:name), :class => klass)
+          @ownershipitems = find_records_with_rbac(klass, ownership_items).sort_by(&:name)


you can use SQL for ordering

@ownershipitems = find_records_with_rbac(klass.order(:name), ownership_items)

lpichler

I am getting error:

F, [2017-06-28T15:20:08.564031 #32689] FATAL -- : Error caught: [ActionView::Template::Error] undefined method `count' for nil:NilClass
/Users/liborpichler/manageiq/manageiq-ui-classic/app/views/shared/views/_ownership.html.haml:47:in `___sers_liborpichler_manageiq_manageiq_ui_classic_app_views_shared_views__ownership_html_haml__795864017557801983_70221982635380'
/usr/local/opt/rbenv/versions/2.3.1/lib/ruby/gems/2.3.0/gems/actionview-5.0.4/lib/action_view/template.rb:159:in `block in render'
/usr/local/opt/rbenv/versions/2.3.1/lib/ruby/gems/2.3.0/gems/activesupport-5.0.4/lib/active_support/notifications.rb:166:in `instrument'
/usr/local/opt/rbenv/versions/2.3.1/lib/ruby/gems/2.3.0/gems/actionview-5.0.4/lib/action_view/template.rb:354:in `instrument'
/usr/local/opt/rbenv/versions/2.3.1/lib/ruby/gems/2.3.0/gems/actionview-5.0.4/lib/action_view/template.rb:157:in `render'
/usr/local/opt/rbenv/versions/2.3.1/lib/ruby/gems/2.3.0/gems/actionview-5.0.4/lib/action_view/renderer/partial_renderer.rb:343:in `render_partial'
/usr/local/opt/rbenv/versions/2.3.1/lib/ruby/gems/2.3.0/gems/actionview-5.0.4/lib/action_view/renderer/partial_renderer.rb:311:in `block in render'
/usr/local/opt/rbenv/versions/2.3.1/lib/ruby/gems/2.3.0/gems/actionview-5.0.4/lib/action_view/renderer/abstract_renderer.rb:42:in `block in instrument'
/usr/local/opt/rbenv/versions/2.3.1/lib/ruby/gems/2.3.0/gems/activesupport-5.0.4/lib/active_support/notifications.rb:164:in `block in instrument'
/usr/local/opt/rbenv/versions/2.3.1/lib/ruby/gems/2.3.0/gems/activesupport-5.0.4/lib/active_support/notifications/instrumenter.rb:21:in `instrument'

with this scenario:

Test scenario - reach set ownership screen from non-explorer screen
(Steps to Reproduce:

Add RHOS provider with OSP mapping enabled as Admin user
Navigate to Compute -> Clouds -> Providers -> RHOS7GA -> Images
Select one image (without clicking on it).
Configuration -> Set ownership)

taken from #843

martinpovolny · 2017-07-06T16:53:11Z

app/controllers/mixins/actions/vm_actions/ownership.rb

-          end
+
+          recs = session[:checked_items]
+          recs = checked_or_params if recs.nil?


recs = session[:checked_items] || checked_or_params ?

martinpovolny · 2017-07-06T16:54:37Z

app/controllers/mixins/actions/vm_actions/ownership.rb

-          ownership_items = params[:rec_ids] if params[:rec_ids]
-          build_ownership_info(ownership_items)
+          ownership_ids = params[:rec_ids] if params[:rec_ids]
+          @origin_ownership_items = ownership_ids


do we really need to introduce a new instance variable in refactored code?

It's not new instance variable, I just change the location where it's set. It's used in ownership.html.haml.

martinpovolny · 2017-07-17T13:55:46Z

app/controllers/mixins/actions/vm_actions/ownership.rb

-                opts[:group] = MiqGroup.find_by_id(params[:group])
-              end
+          unless params[:group] == 'dont-change'
+            if params[:group].blank?    # to clear previously set group


opts[:group] = if ...

martinpovolny · 2017-07-17T13:55:56Z

app/controllers/mixins/actions/vm_actions/ownership.rb

+        def ownership_handle_save_button
+          opts = {}
+          unless params[:user] == 'dont-change'
+            if params[:user].blank?     # to clear previously set user


params[:user] = if ...

…commit by mistake

miq-bot · 2017-07-19T11:33:51Z

Checked commits PanSpagetka/manageiq-ui-classic@d116446~...d6ac2d8 with ruby 2.2.6, rubocop 0.47.1, and haml-lint 0.20.0
1 file checked, 5 offenses detected

app/controllers/mixins/actions/vm_actions/ownership.rb

❗ - Line 153, Col 13 - Style/MethodCallWithArgsParentheses - Use parentheses for method calls with arguments.
❗ - Line 163, Col 28 - Style/IndentationWidth - Use 2 (not 1) spaces for indentation.
❗ - Line 175, Col 1 - Style/EmptyLines - Extra blank line detected.
❗ - Line 207, Col 13 - Style/MethodCallWithArgsParentheses - Use parentheses for method calls with arguments.
❗ - Line 51, Col 15 - Style/MethodCallWithArgsParentheses - Use parentheses for method calls with arguments.

romanblanco

Tested in UI, works for me 👍

himdel · 2018-12-07T14:01:07Z

app/controllers/mixins/actions/vm_actions/ownership.rb

+
+        def ownership_handle_save_button
+          opts = {}
+          opts[:owner] = unless params[:user] == 'dont-change'


This breaks multi-target set ownership...

Before, owner or group only got assigned to if the user chose to clear the previously set owner/group.
Now, this is always nil, so it's impossible to change user without resetting groups, and vice versa.

Fixing in #5064

miq-bot added the wip label Jun 21, 2017

PanSpagetka force-pushed the ownership-refactor branch 3 times, most recently from 1b7b0e4 to ca1e904 Compare June 22, 2017 12:54

PanSpagetka changed the title ~~[WIP] Ownership refactor~~ Ownership refactor Jun 27, 2017

miq-bot removed the wip label Jun 27, 2017

chessbyte added the refactoring label Jun 28, 2017

romanblanco suggested changes Jun 28, 2017

View reviewed changes

lpichler reviewed Jun 28, 2017

View reviewed changes

lpichler suggested changes Jun 28, 2017

View reviewed changes

PanSpagetka force-pushed the ownership-refactor branch 2 times, most recently from 0635b87 to b71c2b0 Compare June 29, 2017 11:32

PanSpagetka closed this Jun 29, 2017

PanSpagetka reopened this Jun 29, 2017

PanSpagetka force-pushed the ownership-refactor branch from b71c2b0 to 1723e38 Compare July 3, 2017 11:47

dclarizio added fine/no rbac labels Jul 5, 2017

dclarizio assigned martinpovolny Jul 5, 2017

martinpovolny reviewed Jul 6, 2017

View reviewed changes

martinpovolny reviewed Jul 17, 2017

View reviewed changes

PanSpagetka force-pushed the ownership-refactor branch from f4db12b to b4b864f Compare July 19, 2017 08:43

PanSpagetka added 7 commits July 19, 2017 12:27

Change RBAC in ownership

d116446

Refactor ownership methods

2d2f690

Extract button handling code into methods

13a5cc1

Fix setting ownership from non-explorer screen

a6833a2

Remove ownership_items variable in set_ownership

230aec9

Rename ownership_items to ownership_ids

1ba6504

Flip condition

97a144d

PanSpagetka added 7 commits July 19, 2017 12:27

Set @ownershipitems in build_ownership_info method

1b3a9a8

Extract duplicated code for loading instance variables

bf43761

Dont use lonely operator

53de303

Simplify recs assignment

689562d

Create filter_ownership_items method which follows RBAC pattern

d4560da

Use filter_ownership_items method

eef25a2

Add back check if record can be owned, which was removed in previous …

0a95917

…commit by mistake

PanSpagetka force-pushed the ownership-refactor branch 4 times, most recently from 20d3477 to 03842bb Compare July 19, 2017 10:37

PanSpagetka added 2 commits July 19, 2017 12:59

Extract assignment before conditions

903e62a

Replace find_by_id with find

d6ac2d8

PanSpagetka force-pushed the ownership-refactor branch from 03842bb to d6ac2d8 Compare July 19, 2017 11:32

romanblanco approved these changes Jul 19, 2017

View reviewed changes

martinpovolny merged commit 0719317 into ManageIQ:master Jul 20, 2017

martinpovolny added this to the Sprint 65 Ending Jul 24, 2017 milestone Jul 20, 2017

himdel mentioned this pull request Dec 7, 2018

Replaces angular ownership form with data-driven-form #5046

Merged

himdel reviewed Dec 7, 2018

View reviewed changes

himdel mentioned this pull request Dec 7, 2018

Fix multi-target Set Ownership #5064

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Ownership refactor #1578

Ownership refactor #1578

PanSpagetka commented Jun 21, 2017 •

edited

romanblanco left a comment

romanblanco Jun 28, 2017

romanblanco Jun 28, 2017

romanblanco Jun 28, 2017

lpichler Jun 28, 2017

lpichler left a comment

martinpovolny Jul 6, 2017

PanSpagetka Jul 11, 2017

martinpovolny Jul 6, 2017

PanSpagetka Jul 10, 2017

martinpovolny Jul 17, 2017

martinpovolny Jul 17, 2017

martinpovolny Jul 17, 2017

miq-bot commented Jul 19, 2017

romanblanco left a comment

himdel Dec 7, 2018 •

edited

Ownership refactor #1578

Ownership refactor #1578

Conversation

PanSpagetka commented Jun 21, 2017 • edited

romanblanco left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lpichler left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

miq-bot commented Jul 19, 2017

romanblanco left a comment

Choose a reason for hiding this comment

himdel Dec 7, 2018 • edited

Choose a reason for hiding this comment

PanSpagetka commented Jun 21, 2017 •

edited

himdel Dec 7, 2018 •

edited