Expose provider disable for containers providers #1663

zeari · 2017-07-09T16:16:52Z

Compute > Containers > Providers
pick a provider
toolbar Configuration > Pause/Resume this Containers Provider

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

~~BEFORE anyone looks at the code, im trying to figure out if this is something we want to expose to users at all. We can discuss that here.~~
Disable Providers data collection from UI
Screenshots:

@simon3z @Loicavenel @bronaghs @dclarizio

zeari · 2017-07-09T16:20:32Z

cc @moolitayer @cben

Loicavenel · 2017-07-10T00:35:08Z

@zeari Does "pause/Resume" will make more sense ? Disable seems more definitive that what we are doing here..

moolitayer · 2017-07-10T06:47:20Z

app/helpers/application_helper/toolbar/ems_container_center.rb

+          :confirm => proc do
+            if @record.enabled?
+              N_("Warning: While this provider is disabled you will not collect ANY data from it. " \
+                 "This may cause gaps in inventory, metrics and events!")


s/While this provider is disabled you will not collect ANY data from it./While this provider is disabled no data will be collected from it./

moolitayer · 2017-07-10T06:47:55Z

app/controllers/ems_common.rb

+                 :target_id    => id,
+                 :target_class => model.to_s,
+                 :userid       => session[:userid]}
+        AuditEvent.success(audit)


moolitayer · 2017-07-10T06:57:52Z

app/controllers/ems_common.rb

+    return if @flash_array.present?
+
+    add_flash(n_("Disable initiated for %{count} %{model} from the %{product} Database",
+                 "Disable initiated for %{count} %{models} from the %{product} Database", emss.length) %


I think you only allowed disable for individual managers, right?
That's probably a good idea. You would have a weird behavior for more then one, if some are already disabled.
(some would be disabled and some enabled)

I think disabling multiple managers is a valid use case. To avoid the behavior we can go with separate buttons for Enable selected Providers and Disable selected providers when dealing with multiple select.

moolitayer · 2017-07-10T06:59:46Z

app/controllers/ems_common.rb

@@ -548,6 +549,26 @@ def process_emss(emss, task)
         :product => I18n.t('product.name'),
         :model   => ui_lookup(:table => table_name),
         :models  => ui_lookup(:tables => table_name)}) if @flash_array.nil?
+    elsif task == "disable_ems"


please raise an error emss if there is more then one ems

zeari · 2017-07-10T08:05:51Z

@zeari Does "pause/Resume" will make more sense ? Disable seems more definitive that what we are doing here..

I think that would work well. Its more easily understandable.

we need to choose icons though.

zeari · 2017-07-10T08:38:16Z

@miq-bot add_label wip

zeari · 2017-07-10T10:47:51Z

@simon3z i think its enough to warn users that they will be missing data if they keep their provider paused(disabled). I can also add a 'Resumed\Paused' row under the 'Status' table in the summary screen to indicate the current state.

Loicavenel · 2017-07-10T11:57:47Z

Yes, we need a warning... if people are putting a provider in pause for Maintenance on the provider, they may miss some data.

cben · 2017-07-11T10:33:27Z

app/controllers/ems_common.rb

+      model.where(:id => emss).order("lower(name)").each do |ems|
+        id = ems.id
+        ems_name = ems.name
+        audit = {:event        => "ems_record_pause_initiated",


cben · 2017-07-11T10:37:33Z

app/controllers/ems_common.rb

+    process_emss(emss, "pause_ems") unless emss.empty?
+    return if @flash_array.present?
+
+    add_flash(n_("Disable initiated for %{count} %{model} from the %{product} Database",


I think process_emss(emss, "pause_ems") already adds exactly this flash?

cben · 2017-07-11T10:40:54Z

app/controllers/ems_common.rb

+    end
+  end
+
+  def resumeemss


This is extremely similar to pauseems method. Consider DRYing to one method with a param?

@himdel what would you rather I do here?

Oh yes please :)

refreshems and pauseems seem to differ only in the flash message and calling call_ems_*.

call_ems_* differ only in a flash message and an argument to process_emss.

And even those elsif task == "resume_ems" and elsif task == "pause_ems" differ only in the flash/audit message and calling disable! vs enable!.

So, definitely for DRYing this up a little ;).

Also, I notice that both call_ems_* and those elsif task == .. bits use the same exact flash messages, so maybe those should be shared somehow as well.... oh and there's a difference where call_ems_pause says Disable initiated while task pause_ems says Pause initiated .. which is probably a bug, right? :)

simon3z · 2017-07-11T14:33:59Z

@simon3z i think its enough to warn users that they will be missing data if they keep their provider paused(disabled). I can also add a 'Resumed\Paused' row under the 'Status' table in the summary screen to indicate the current state.

OK, yes @zeari I have the feeling that we should do all we can to make it simple to understand that a provider is disabled. Maybe we can even plan to have an extra symbol (a pause symbol?) on top of the provider icon.

For now having it in the status should be OK.

cc @serenamarie125

zeari · 2017-07-13T15:39:14Z

@simon3z @dclarizio @Loicavenel updated screenshots with indicator on the status table.

Loicavenel · 2017-07-13T15:48:51Z

I love it.

zeari · 2017-07-13T16:09:44Z

@miq-bot remove_label wip

simon3z · 2017-07-13T16:09:58Z

Nice! 👍

zeari · 2017-07-13T16:12:32Z

app/helpers/textual_mixins/textual_paused_state.rb

+  def textual_paused_state
+    state = @record.enabled?
+    h = {:label => _("Running State"), :value => state ? _("Running") : _("Paused")}
+    if state


So the current wording is Running State: Running\Paused
I feel this is a little misleading to users since its not the external provider thats paused but the data collection from it on miq side.
Do you think it should be Data Collection Workers: Running\Paused or something else that better indicates that its only on the miq side?
@simon3z @Loicavenel

Good point... this will be really confusing... it should be: "Collection Status" @serenamarie125 Idea?

or just Data Collection. I think we should use these terms in the dropdowns too.

serenamarie125 · 2017-08-08T21:48:39Z

@zeari ok, now i am understanding, you want to add a flash message (inline notification) when going to the dashboard so that people understand that the provider is disconnected. I agree with that!

I think for this case, it would be an informational inline notification ( http://www.patternfly.org/pattern-library/communication/inline-notifications/ )

And yes, I'm sorry, i meant to change the initial dialog that appears when you choose to pause the provider, if possible

zeari · 2017-08-09T10:28:12Z

@serenamarie125 the flash message:

miq-bot · 2017-08-09T11:54:25Z

Checked commit zeari@3adb20c with ruby 2.2.6, rubocop 0.47.1, and haml-lint 0.20.0
8 files checked, 3 offenses detected

app/controllers/ems_common.rb

❗ - Line 727, Col 30 - Rails/DynamicFindBy - Use find_by instead of dynamic find_by_id.

app/helpers/application_helper/toolbar/ems_container_center.rb

❗ - Line 32, Col 49 - Style/MultilineMethodCallBraceLayout - Closing method call brace must be on the line after the last argument when opening brace is on a separate line from the first argument.
❗ - Line 40, Col 48 - Style/MultilineMethodCallBraceLayout - Closing method call brace must be on the line after the last argument when opening brace is on a separate line from the first argument.

serenamarie125 · 2017-08-10T16:15:36Z

@zeari LGTM! I think we should have a follow up PR once PatternFly gets the appropriate icons in

zeari · 2017-08-10T16:22:44Z

@zeari LGTM! I think we should have a follow up PR once PatternFly gets the appropriate icons in
Merge state

Thanks @serenamarie125 . @himdel is there anything else needed here?

himdel · 2017-08-17T12:26:01Z

@zeari No idea. I'm not seeing any new toolbar buttons anywhere, and not seeing any info on where to find this in the description either :)

EDIT: never mind, right branch, but commented out override_gem O:-)
I can see the buttons now, adding to description.

himdel · 2017-08-17T12:49:04Z

Tested in the UI, I can see it working on all the container provider detail screens.

But.. this looks like we should really change the container provider quadicons to show this too, otherwise there's no indication of paused/resumed in the list view.

zeari · 2017-08-17T12:50:27Z

Tested in the UI, I can see it working on all the container provider detail screens.

But.. this looks like we should really change the container provider quadicons to show this too, otherwise there's no indication of paused/resumed in the list view.

@himdel Any good PRs to copy off of? 😅

serenamarie125 · 2017-08-17T12:55:24Z

Should we determine what goes in the quad icon, as well as where :) I think that would be a great exercise!

What info is available? We have a matrix somewhere regarding suggested location for data, lemme find it this morning ...

himdel · 2017-08-17T12:59:07Z

Heheh, there's been some quadicon refactorings recently so this should not be that hard...

app/helpers/quadicon_helper.rb

Looks like render_ext_management_system_quadicon has an example for a 4-quadrant quadicon, and else-defaults to what you see now.

So.. looks like we just need to enable this in settings, and add a elsif case that does the right thing.

(Those a72, b72, ... denote the individual quadrants)

himdel · 2017-08-17T13:03:54Z

@serenamarie125 Looks like for ems, we get:

first quadrant (a72): an item count (total vms, number of physical servers, number of hosts..) - so a number of containers here?
second quadrant (b72): total_miq_templates for EmsCloud, else nothing - sounds like we could use this one for suspended/resumed icon?
third quadrant (c72): the provider icon
fourth quadrant (d72): authentication status icon
overlay in the middle (g72): a shield if there's any policies

himdel · 2017-08-17T13:20:07Z

@zeari A quick and dirty test...

diff --git a/app/helpers/quadicon_helper.rb b/app/helpers/quadicon_helper.rb
index 304b41c60f..a38818b79d 100644
--- a/app/helpers/quadicon_helper.rb
+++ b/app/helpers/quadicon_helper.rb
@@ -553,16 +553,18 @@ module QuadiconHelper
   def render_ext_management_system_quadicon(item, options)
     output = []
 
-    if settings(:quadicons, db_for_quadicon)
+    if true #settings(:quadicons, db_for_quadicon)
       output << flobj_img_simple("layout/base.png")
       item_count = case item
                    when EmsPhysicalInfra then item.physical_servers.size
                    when EmsCloud         then item.total_vms
+                   when ::ManageIQ::Providers::ContainerManager then item.containers.size
                    else
                      item.hosts.size
                    end
       output << flobj_p_simple("a72", item_count)
       output << flobj_p_simple("b72", item.total_miq_templates) if item.kind_of?(EmsCloud)
+      output << flobj_img_simple("svg/currentstate-paused.svg", "b72") if item.kind_of?(::ManageIQ::Providers::ContainerManager)
       output << flobj_img_simple("svg/vendor-#{h(item.image_name)}.svg", "c72")
       output << flobj_img_simple(img_for_auth_status(item), "d72")
       output << flobj_img_simple('100/shield.png', "g72") unless item.get_policies.empty?

zeari · 2017-08-17T13:41:50Z

@zeari A quick and dirty test...

We definitely need more @himdels in this project 😄

moolitayer reviewed Jul 10, 2017

View reviewed changes

miq-bot changed the title ~~Expose provider disable for containers providers~~ [WIP] Expose provider disable for containers providers Jul 10, 2017

miq-bot added the wip label Jul 10, 2017

zeari force-pushed the disable_provider_ui branch 2 times, most recently from d7f7f2b to 94a5efe Compare July 10, 2017 13:30

cben reviewed Jul 11, 2017

View reviewed changes

zeari force-pushed the disable_provider_ui branch 2 times, most recently from 4c7bfc2 to 34acebd Compare July 13, 2017 15:38

miq-bot changed the title ~~[WIP] Expose provider disable for containers providers~~ Expose provider disable for containers providers Jul 13, 2017

miq-bot removed the wip label Jul 13, 2017

zeari commented Jul 13, 2017

View reviewed changes

zeari force-pushed the disable_provider_ui branch 3 times, most recently from ce86d99 to 90dbf4c Compare July 17, 2017 15:43

zeari force-pushed the disable_provider_ui branch from f6b2f4f to cb5cc48 Compare August 9, 2017 11:43

expose provider disable for containers providers

3adb20c

zeari force-pushed the disable_provider_ui branch from cb5cc48 to 3adb20c Compare August 9, 2017 11:53

serenamarie125 mentioned this pull request Aug 16, 2017

HAWKULAR-1164 - Stop an EAP domain #1753

Merged

himdel added this to the Sprint 67 Ending Aug 21, 2017 milestone Aug 17, 2017

himdel merged commit f3c6e7c into ManageIQ:master Aug 17, 2017

zeari pushed a commit to zeari/manageiq-ui-classic that referenced this pull request Aug 17, 2017

Followup to ManageIQ#1663: quadicon and dashboard fix

b9b993a

zeari mentioned this pull request Aug 17, 2017

Quad icons for containers providers #1950

Merged

zeari pushed a commit to zeari/manageiq-ui-classic that referenced this pull request Aug 20, 2017

Followup to ManageIQ#1663: quadicon and dashboard fix

a504165

zeari pushed a commit to zeari/manageiq-ui-classic that referenced this pull request Aug 20, 2017

Followup to ManageIQ#1663: quadicon and dashboard fix

bbf790d

zeari pushed a commit to zeari/manageiq-ui-classic that referenced this pull request Aug 20, 2017

Followup to ManageIQ#1663: quadicon and dashboard fix

e1ad5b8

zeari pushed a commit to zeari/manageiq-ui-classic that referenced this pull request Aug 20, 2017

Followup to ManageIQ#1663: quadicon and dashboard fix

de0f7de

zeari pushed a commit to zeari/manageiq-ui-classic that referenced this pull request Aug 21, 2017

Followup to ManageIQ#1663: quadicon and dashboard fix

94aa91c

zeari pushed a commit to zeari/manageiq-ui-classic that referenced this pull request Aug 21, 2017

Followup to ManageIQ#1663: quadicon and dashboard fix

eb9b128

zeari pushed a commit to zeari/manageiq-ui-classic that referenced this pull request Aug 21, 2017

Followup to ManageIQ#1663: quadicon and dashboard fix

31c15b8

zeari pushed a commit to zeari/manageiq-ui-classic that referenced this pull request Aug 22, 2017

Followup to ManageIQ#1663: quadicon and dashboard fix

7120acb

yaacov mentioned this pull request Aug 24, 2017

Show card only for single provider #1968

Merged

Expose provider disable for containers providers #1663

Expose provider disable for containers providers #1663

Conversation

zeari commented Jul 9, 2017 • edited by himdel Loading

zeari commented Jul 9, 2017

Loicavenel commented Jul 10, 2017

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

zeari commented Jul 10, 2017 • edited Loading

zeari commented Jul 10, 2017

zeari commented Jul 10, 2017

Loicavenel commented Jul 10, 2017

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

simon3z commented Jul 11, 2017

zeari commented Jul 13, 2017 • edited Loading

Loicavenel commented Jul 13, 2017

zeari commented Jul 13, 2017

simon3z commented Jul 13, 2017

zeari Jul 13, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

serenamarie125 commented Aug 8, 2017

zeari commented Aug 9, 2017 • edited Loading

miq-bot commented Aug 9, 2017

serenamarie125 commented Aug 10, 2017

zeari commented Aug 10, 2017

himdel commented Aug 17, 2017 • edited Loading

himdel commented Aug 17, 2017

zeari commented Aug 17, 2017

serenamarie125 commented Aug 17, 2017

himdel commented Aug 17, 2017

himdel commented Aug 17, 2017 • edited Loading

himdel commented Aug 17, 2017

zeari commented Aug 17, 2017

zeari commented Jul 9, 2017 •

edited by himdel

Loading

zeari commented Jul 10, 2017 •

edited

Loading

zeari commented Jul 13, 2017 •

edited

Loading

zeari Jul 13, 2017 •

edited

Loading

zeari commented Aug 9, 2017 •

edited

Loading

himdel commented Aug 17, 2017 •

edited

Loading

himdel commented Aug 17, 2017 •

edited

Loading