Refactor quadicon settings logic under My Settings #4034

skateman · 2018-06-04T13:21:44Z

Instead of explicitly specifying the quadicon settings in three different places, we can just determine things from our beloved decorators. When adding a new model, now it's enough to have the decorator with the quadicon method and everything will be sorted out automagically.

The key naming for quadicon types was too fragmented in DEFAULT_SETTINGS. Adding new exceptions just made the code more chaotic and it was necessary to keep this sync with the big ugly structure in the ui_1.html.haml. So instead of using base_model + exceptions I'm proposing to decorate the given class and remove the Decorator from the resulted class' name. Then this class should be demodulized and converted to snake_case and so we have our new key. We don't even have to declare each of those in DEFAULT_SETTINGS, as by default all of them default to true.

In the UI it's easy to retrieve all decorators that respond to the quadicon method and each of them can be mapped to the key described above using the same procedure. The proper model names can be retrieved using ui_lookup so we can get rid of the big ugly structure.

The RBAC is a little bit tricky, in general the feature field in the big ugly structure is not a good enough solution as not just the _show_list features can lead to quadicon rendering. As the quadicon settings are per-user, I assume the slider/checkbox hiding here has been introduced because we have deployments where only some features are exposed for a given user and we don't want this user to know about the possibility of others. Based on this I think we can be a little bit more permissive and display the quadicon setting if there's at least one rule allowed in the given feature subtree.

Mapping the models into RBAC features was made by calling model_name.singular_route_key on the given model class. I have a feeling that this is too good to be true and I might be totally wrong about this. There's a double twist with the storage managers, but I'm addressing this in a separate core PR.

If you have read all the text above, I have a little test, that if you pass, I will buy you a 🍺
If you want to read more, look at #4027 😉

Depends on:
ManageIQ/manageiq-schema#213
ManageIQ/manageiq#17512
ManageIQ/manageiq#17513
#4030
#4026

@miq-bot add_label refactoring, gaprindashvili/no, pending core

skateman · 2018-06-04T13:22:38Z

app/helpers/configuration_helper.rb

+
+  # Returns with a hash of allowed quadicons for the current user
+  def allowed_quadicons
+    MiqDecorator.descendants. # Get all the decorator classes


@kbrock I'm a little bit concerned about the descendants method here. It's a little bit slow for the first run in development, but I assume it's faster in production as we have eager_load. Is this correct?

miq-bot · 2018-06-04T13:23:27Z

@skateman unrecognized command 'add_reviwer', ignoring...

Accepted commands are: add_label, add_reviewer, assign, close_issue, move_issue, remove_label, rm_label, set_milestone

miq-bot · 2018-06-04T13:23:27Z

@skateman unrecognized command 'add_reviwer', ignoring...

Accepted commands are: add_label, add_reviewer, assign, close_issue, move_issue, remove_label, rm_label, set_milestone

miq-bot · 2018-06-04T13:23:28Z

@skateman unrecognized command 'add_reviwer', ignoring...

Accepted commands are: add_label, add_reviewer, assign, close_issue, move_issue, remove_label, rm_label, set_milestone

skateman · 2018-06-04T13:26:02Z

@miq-bot add_reviewer @martinpovolny
@miq-bot add_reviewer @himdel
@miq-bot add_reviewer @PanSpagetka

skateman · 2018-06-04T13:30:31Z

We might need to write a migration that converts old settings keys to the new format in the DB, but I'm not sure about this.

miq-bot · 2018-06-04T19:44:31Z

This pull request is not mergeable. Please rebase and repush.

kbrock

sure gets rid of a bunch of hardcoded lists \o/

kbrock · 2018-06-05T16:11:37Z

app/controllers/application_controller.rb

-      :storage            => true,
-      :vm                 => true
-    },
+    :quadicons => Hash.new(true), # Show quad icons by resource type, true by default


kbrock · 2018-06-05T16:11:48Z

app/controllers/configuration_controller.rb

-      @edit[:new][:quadicons][:miq_template] = params[:quadicons_miq_template] == "true" if params[:quadicons_miq_template]
-      if ::Settings.product.proto # Hide behind proto setting - Sprint 34
-        @edit[:new][:quadicons][:service] = params[:quadicons_service] == "true" if params[:quadicons_service]
+      view_context.allowed_quadicons.each_key do |key|


kbrock · 2018-06-05T16:12:44Z

app/helpers/configuration_helper.rb

+
+  # Returns with a hash of allowed quadicons for the current user
+  def allowed_quadicons
+    MiqDecorator.descendants # Get all the decorator classes


descendants make me nervous. but seems to be our trend

kbrock · 2018-06-05T16:13:55Z

app/views/configuration/_ui_1.html.haml

-                                  @edit[:new][:quadicons][icons_checkbox[2].to_sym], :data => {:on_text => _('Yes'), :off_text => _('No')})
-                :javascript
-                  miqInitBootstrapSwitch("quadicons_#{icons_checkbox[2]}", "#{url}")
+          - allowed_quadicons.each do |key, name|


kbrock · 2018-06-05T16:16:54Z

app/helpers/configuration_helper.rb

@@ -1,3 +1,31 @@
 module ConfigurationHelper
  include_concern 'ConfigurationViewHelper'
+
+  # Model class name comparison method that sorts provider models first
+  def compare_decorator_class(a, b)


not a fan, but seems necessary

kbrock · 2018-06-05T16:17:19Z

app/helpers/configuration_helper.rb

+  end
+
+  # Returns with a hash of allowed quadicons for the current user
+  def allowed_quadicons


really like that this is "allowed" quadicons

kbrock · 2018-06-05T16:26:33Z

app/helpers/configuration_helper.rb

+  # Returns with a hash of allowed quadicons for the current user
+  def allowed_quadicons
+    MiqDecorator.descendants # Get all the decorator classes
+                .select { |klass| klass.new(nil).respond_to?(:quadicon) } # Select only the decorators that define a quadicon


does method_defined?(:quadicon) work instead of klass.new(nil).respond_to?(:quadicon)?

It does and I like it more, calling new 145 times less 😉

miq-bot · 2018-06-06T08:48:58Z

This pull request is not mergeable. Please rebase and repush.

miq-bot · 2018-06-06T09:59:12Z

Checked commit skateman@817ded5 with ruby 2.3.3, rubocop 0.52.1, haml-lint 0.20.0, and yamllint 1.10.0
5 files checked, 1 offense detected

**

💣 💥 🔥 🚒 - Linter/Haml - Linter::Haml STDERR:

warning: parser/current is loading parser/ruby23, which recognizes
warning: 2.3.6-compliant syntax, but you are running 2.3.3.
warning: please see https://github.com/whitequark/parser#compatibility-with-ruby-mri.

mzazrivec · 2018-06-13T08:15:36Z

@kbrock is this one ok to merge?

skateman · 2018-06-13T08:44:09Z

@mzazrivec I think @kbrock is okay with this, but I'd like @martinpovolny to take a look at this.

martinpovolny · 2018-06-13T15:37:18Z

I like it! We need to write more parts of the UI like this. I like this guy ;-)

Let's merge it. @kbrock ?

kbrock · 2018-06-13T15:54:49Z

I do wish we could have written allowed_quadicons differently. We are calculating essentially static information on each page load.

Lets just keep that in mind as we move forward and introduce similar refactoring
LGTM 👍

skateman · 2018-06-13T15:55:02Z

@martinpovolny there's also a schema PR but it won't break anything if this gets merged first.

himdel · 2018-11-19T18:17:04Z

app/views/configuration/_ui_1.html.haml

@@ -46,8 +30,6 @@
                           @edit[:new][:display][:quad_truncate]),
                           :class    => "selectpicker")

-            :javascript
-              miqSelectPickerEvent("quad_truncate", "#{url}")


https://bugzilla.redhat.com/show_bug.cgi?id=1650461

Fixed in #4942

skateman commented Jun 4, 2018

View reviewed changes

miq-bot added gaprindashvili/no pending core refactoring labels Jun 4, 2018

miq-bot added the wip label Jun 4, 2018

miq-bot requested review from martinpovolny, himdel and PanSpagetka June 4, 2018 13:26

skateman mentioned this pull request Jun 4, 2018

Define model_name with route keys for the StorageManager model ManageIQ/manageiq#17513

Merged

skateman force-pushed the quadicon-selection-logic branch 4 times, most recently from 1423be7 to dafe401 Compare June 4, 2018 16:58

miq-bot added the unmergeable label Jun 4, 2018

skateman force-pushed the quadicon-selection-logic branch from dafe401 to 9b9aa30 Compare June 4, 2018 20:23

miq-bot removed the unmergeable label Jun 4, 2018

kbrock reviewed Jun 5, 2018

View reviewed changes

skateman force-pushed the quadicon-selection-logic branch from 9b9aa30 to dd53858 Compare June 5, 2018 16:44

miq-bot removed the pending core label Jun 6, 2018

miq-bot changed the title ~~[WIP] Refactor quadicon settings logic under My Settings~~ Refactor quadicon settings logic under My Settings Jun 6, 2018

miq-bot removed the wip label Jun 6, 2018

skateman changed the title ~~Refactor quadicon settings logic under My Settings~~ [WIP] Refactor quadicon settings logic under My Settings Jun 6, 2018

mzazrivec added the wip label Jun 6, 2018

miq-bot added the unmergeable label Jun 6, 2018

skateman mentioned this pull request Jun 6, 2018

Convert user quadicon settings from the old format to the new one ManageIQ/manageiq-schema#213

Merged

Refactor quadicon settings logic under My Settings

817ded5

skateman force-pushed the quadicon-selection-logic branch from dd53858 to 817ded5 Compare June 6, 2018 09:55

miq-bot removed the unmergeable label Jun 6, 2018

skateman changed the title ~~[WIP] Refactor quadicon settings logic under My Settings~~ Refactor quadicon settings logic under My Settings Jun 6, 2018

miq-bot removed the wip label Jun 6, 2018

mzazrivec self-assigned this Jun 14, 2018

mzazrivec added this to the Sprint 88 Ending Jun 18, 2018 milestone Jun 14, 2018

mzazrivec merged commit f206593 into ManageIQ:master Jun 14, 2018

skateman deleted the quadicon-selection-logic branch June 14, 2018 09:42

himdel mentioned this pull request Nov 19, 2018

Configuration - notify backend when quad_truncate gets changed #4942

Merged

himdel reviewed Nov 19, 2018

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor quadicon settings logic under My Settings #4034

Refactor quadicon settings logic under My Settings #4034

skateman commented Jun 4, 2018 •

edited

Loading

skateman Jun 4, 2018

miq-bot commented Jun 4, 2018

miq-bot commented Jun 4, 2018

miq-bot commented Jun 4, 2018

skateman commented Jun 4, 2018

skateman commented Jun 4, 2018 •

edited

Loading

miq-bot commented Jun 4, 2018

kbrock left a comment

kbrock Jun 5, 2018

kbrock Jun 5, 2018

kbrock Jun 5, 2018

kbrock Jun 5, 2018

kbrock Jun 5, 2018

kbrock Jun 5, 2018

kbrock Jun 5, 2018

skateman Jun 5, 2018

miq-bot commented Jun 6, 2018

miq-bot commented Jun 6, 2018

mzazrivec commented Jun 13, 2018

skateman commented Jun 13, 2018

martinpovolny commented Jun 13, 2018

kbrock commented Jun 13, 2018

skateman commented Jun 13, 2018

himdel Nov 19, 2018

Refactor quadicon settings logic under My Settings #4034

Refactor quadicon settings logic under My Settings #4034

Conversation

skateman commented Jun 4, 2018 • edited Loading

Choose a reason for hiding this comment

miq-bot commented Jun 4, 2018

miq-bot commented Jun 4, 2018

miq-bot commented Jun 4, 2018

skateman commented Jun 4, 2018

skateman commented Jun 4, 2018 • edited Loading

miq-bot commented Jun 4, 2018

kbrock 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

Choose a reason for hiding this comment

miq-bot commented Jun 6, 2018

miq-bot commented Jun 6, 2018

mzazrivec commented Jun 13, 2018

skateman commented Jun 13, 2018

martinpovolny commented Jun 13, 2018

kbrock commented Jun 13, 2018

skateman commented Jun 13, 2018

Choose a reason for hiding this comment

skateman commented Jun 4, 2018 •

edited

Loading

skateman commented Jun 4, 2018 •

edited

Loading