Physical server page #970

gabrielsvinha · 2017-04-07T22:32:25Z

This Pull Request implements the toolbar buttons for Physical Server summary page.

Add pages for physical server and physical infrastructure
Add buttons to physical server toolbar
Fix uuid error to ems_ref
Add buttons to physical infrastructure toolbar
Fix bug that makes physical server toolbar does not appear

Depends on core #14709

1. Adding routes to physical_server in routes.rb file

1. Adding physical_server flag into ems_common/_show.html.haml file.

1. Adding PhysicalServer structure to ui_constants.rb file.

1. Adding PhysicalServer structure to quadicon_helper.rb file to render properly physical_server's quadicon.

1. Adding physical_server flag into host textual summary.

1. Adding PhysicalServer structure into textual_summary of ems_physical_infra.

Adding physical server flags into three files: 1. app/helpers/application_helper/toolbar_chooser.rb 2. app/helpers/application_helper.rb 3, app/controllers/ems_common.rb This last one has view_setup_param structure with only physical_server.

Add all helpers and list_nav to physical server.

Adapt physical infra helpers to show physical servers.

Add assets to physical server health state.

…ical_server

1. using `@record.machine_type` instead of `@record.machineType` 2. using `@record.serial_number` instead of `@record.serialNumber` 3. using `@record.product_name` instead of `@record.productName`

miq-bot · 2017-04-25T19:38:27Z

Checked commits https://github.com/lenovo/manageiq-ui-classic/compare/ca99b62bed310f1cd9ed6c704190b348915d54b6~...fd8c51eb0016cc63384334340e431aaeb05e1fa0 with ruby 2.2.6, rubocop 0.47.1, and haml-lint 0.20.0
17 files checked, 0 offenses detected
Everything looks good. 🍰

gabrielsvinha · 2017-04-25T21:12:41Z

@miq-bot remove_label wip

AparnaKarve · 2017-04-25T21:13:32Z

app/controllers/application_controller/ci_processing.rb

@@ -208,7 +187,7 @@ def ownership_update
  # Retire 1 or more VMs
  def retirevms
    assert_privileges(params[:pressed])
-    vms = find_checked_items_with_rbac(VmOrTemplate)
+    vms = find_checked_items


Why was the rbac call replaced with the non-rbac one?

AparnaKarve · 2017-04-25T21:17:50Z

app/controllers/application_controller/ci_processing.rb


    items = []

    # Either a list or coming from a different controller
-    if @lastaction == "show_list" || %w(cloud_object_store_containers cloud_object_store_objects).include?(@display)
+    if @lastaction == "show_list" || !%w(cloud_object_store_container).include?(request.parameters["controller"])


Instead of making these changes here in ci_processing, could you make all the Physical Server related changes in it's own controller, which would be physical_server_controller?

AparnaKarve · 2017-04-25T21:19:05Z

app/controllers/application_controller/ci_processing.rb

@@ -2511,6 +2478,18 @@ def refreshhosts
    host_button_operation('refresh_ems', _('Refresh'))
  end

+  def refreshphysicalservers


Same comment as above.
It's best to handle all Physical Server actions in physical_server_controller

AparnaKarve · 2017-04-25T21:30:25Z

app/controllers/ems_common.rb

@@ -722,7 +782,6 @@ def form_instance_vars
    @vmware_cloud_api_versions = retrieve_vmware_cloud_api_versions
    @emstype_display = model.supported_types_and_descriptions_hash[@ems.emstype]
    @nuage_api_versions = retrieve_nuage_api_versions
-    @hawkular_security_protocols = retrieve_hawkular_security_protocols


How is this change related to Physical Servers?

Same as below.

AparnaKarve · 2017-04-25T21:31:14Z

app/controllers/ems_common.rb

@@ -775,13 +834,6 @@ def retrieve_container_security_protocols
     [_('SSL without validation'), 'ssl-without-validation']]
  end

-  def retrieve_hawkular_security_protocols


Why is this unrelated method being removed?

I believe this was changes in the master branch and came up with a merge made. But this is fixed in the next commits:

https://github.com/lenovo/manageiq-ui-classic/blob/physical_server_page/app/controllers/ems_common.rb#L776

AparnaKarve · 2017-04-25T21:32:29Z

app/controllers/ems_common.rb

+      # Physical server
+      when "physical_server_refresh"          then refreshphysicalservers
+      when "physical_server_delete"           then deletephysicalservers
+      when "physical_server_edit"             then editphysicalservers


I would expect this button handling in physical_server_controller, not here in ems_common

AparnaKarve · 2017-04-25T21:35:37Z

app/controllers/ems_common.rb

+    drop_breadcrumb(:name => @ems.name + _(" (Topology)"), :url => show_link(@ems))
+  end
+
+  def view_setup_params


Where is this method being called? What does it do?

AparnaKarve · 2017-04-25T21:37:53Z

app/controllers/ems_common.rb

@@ -53,6 +53,68 @@ def show_ad_hoc_metrics
    drop_breadcrumb(:name => @ems.name + _(" (Ad hoc Metrics)"), :url => show_link(@ems))
  end

+  def show_topology


You can remove this method and instead use the MoreShowActions mixin which has the show_topology method.

AparnaKarve · 2017-04-25T21:40:36Z

app/helpers/physical_server_helper/textual_summary.rb

@@ -1,44 +1,52 @@
 module PhysicalServerHelper::TextualSummary
+
+


Remove the newlines

AparnaKarve · 2017-04-25T21:41:30Z

app/helpers/application_helper/toolbar/physical_server_center.rb

@@ -61,7 +61,7 @@ class ApplicationHelper::Toolbar::PhysicalServerCenter < ApplicationHelper::Tool
          nil,
          N_('Power off the server'),
          N_('Power Off'),
-          :image     => "power_off",
+            :image     => "power_off",


Why that space?

Just a typo. Sorry for that.

AparnaKarve · 2017-04-25T21:59:50Z

app/controllers/application_controller/ci_processing.rb

@@ -203,7 +203,6 @@ def ownership_update
  # Retire 1 or more items (vms, stacks, services)
  def retirevms
    assert_privileges(params[:pressed])
-    vms = find_checked_items


How is this change related to Physical Server?

If it's not related, please avoid making changes like these since it disturbs the flow of the review process.

We made a merge from ManageIQ/master and this variable remained, this was blocking Travis because it's an unused variable.

AparnaKarve · 2017-04-25T22:10:46Z

app/controllers/ems_common.rb

+    [[_('SSL'), 'ssl-with-validation'],
+     [_('SSL trusting custom CA'), 'ssl-with-validation-custom-ca'],
+     [_('SSL without validation'), 'ssl-without-validation'],
+     [_('Non-SSL'), 'non-ssl']]


Looks like the removed code was added back.
Why was it removed though? Was it removed inadvertently?

I believe this was merged from master.

AparnaKarve · 2017-04-25T22:14:12Z

app/helpers/physical_server_helper/textual_summary.rb

  end

  def textual_host
-    {:label => _("Host"), :value => @record.host.try(:name), :icon =>  "pficon pficon-virtual-machine", :link => url_for(:controller =>'host', :action =>  'show', :id =>  @record.host.try(:id))}
+    {:label=> _("Host"), :value=> @record.host.try(:name), :icon=> "pficon pficon-virtual-machine", :link=> url_for(:controller=> 'host', :action=> 'show', :id=> @record.host.try(:id))}


need a space after label

Does the other attributes need spaces also?

AparnaKarve · 2017-04-25T22:14:43Z

app/helpers/physical_server_helper/textual_summary.rb

-     {:label => _("Serial Number"), :value => @record.serial_number }
-   end
+  def textual_machine_type
+    {:label=> _("Machine Type"), :value => @record.machine_type }


same, space after label

AparnaKarve · 2017-04-25T23:00:03Z

@gabrielsvinha @walteraa There is a lot going on here in this PR.
Ideally this feature should have been added in smaller baby steps as shown below for easier review and quick merging.

Grid/Tile/List (GTL) views
Summary page: Textual Summary/Relationships
Toolbars and actions in GTL views
Toolbars and actions in Summary page
Angular forms

AparnaKarve · 2017-04-25T23:16:19Z

app/views/physical_server/_form.html.haml

@@ -0,0 +1,108 @@
+- @angular_form = true


This seems to be a copy-paste of the host angular form.

To simplify things a little bit, I recommend removing all angular related code from this PR.
It would be a good idea to work on the angular bits after this gets merged.

AparnaKarve · 2017-04-25T23:22:09Z

@gabrielsvinha Please add some screenshots of the new screens added in your first PR comment.

Looks like a separate method, render_physical_server_quadicon, was added to render the quadicon for Physical Servers.
Could you also add some quadicon screenshots? Thanks.

AparnaKarve · 2017-04-25T23:27:15Z

app/controllers/application_controller/ci_processing.rb

+  def editphysicalservers
+    # TODO: edit physical servers
+  end
+


Please move these methods in physical_server_controller.
Also rename the methods to use snake_case (i.e. refresh_physical_server as opposed to refreshphysicalserver)

gabrielsvinha · 2017-04-26T12:33:50Z

@AparnaKarve I am dividing this PR in new PRs with all these alterations. Sounds good?

AparnaKarve · 2017-04-26T14:30:38Z

@gabrielsvinha That will help us immensely.
Thanks, appreciate it.

gabrielsvinha · 2017-04-28T14:03:48Z

@AparnaKarve @rodneyhbrown7 I am closing this Pull Request. It was broke down into smaller ones.

dclarizio · 2017-05-03T22:11:09Z

@gabrielsvinha closing this for you, if incorrect, we can reopen :)

Odravison Amaral and others added 29 commits March 20, 2017 20:52

[Routes] Adds routes to physical_server

ca99b62

1. Adding routes to physical_server in routes.rb file

[EmsCommon] Adds physical_server flags

95edeb1

1. Adding physical_server flag into ems_common/_show.html.haml file.

[ui_constants] Adds physical_server structure

0ef30c0

1. Adding PhysicalServer structure to ui_constants.rb file.

[QuadIcon] Adds PhysicalServer structure

d60cc0a

1. Adding PhysicalServer structure to quadicon_helper.rb file to render properly physical_server's quadicon.

[TextualSummary] Adds physical server flag

3c0e12f

1. Adding physical_server flag into host textual summary.

[TextualSummary] Adds physical server flag

8affee9

1. Adding PhysicalServer structure into textual_summary of ems_physical_infra.

[PhysicalServerViews] Adds all physical server views

92c7549

[PhysicalServerController] Adds physical server's controller

4feb863

[PhysicalServerToolbar] Adds physical server's toolbar structure.

7142a11

Add helpers to physical server pages

bef6016

Add all helpers and list_nav to physical server.

Change physical infra helpers

471a4e2

Adapt physical infra helpers to show physical servers.

Add images assets to physical server pages

fb91056

Add assets to physical server health state.

[Fix] Fixes all warning made by rubocop

ea20828

[Fix] Fixes warning in files made by rubocop

0a3a4a3

[Fix] Fixes of warning made by rubocop into views and helpers of phys…

4b2916a

…ical_server

[Fix] Fixing some issues

7bb757a

[Fix] Changes html_safe to safe_join

7ad7e4b

[Typo] Typo some errors warning by rubocop

019c7f4

[Typo] Delete a blank space after backslash

fefe826

[Fix] Turns into hash rockets syntax

4d82e5d

[Fix] Inserts '.try' method call

5c0ef75

Merge remote-tracking branch 'manageiq/master' into physical_server_page

dc082d9

[BugFix] Changes the attribute methods call

6d646bc

1. using `@record.machine_type` instead of `@record.machineType` 2. using `@record.serial_number` instead of `@record.serialNumber` 3. using `@record.product_name` instead of `@record.productName`

Create fetures for physical servers toolbar's buttons.

26de5af

Update ci processing with methods for toolbar buttons

4b08d75

Update toolbar button with modify methods

87c39e4

Insert cases for ems common show toolbar buttons

f3c5a59

Update textual summary name methods

f069e05

miq-bot added the wip label Apr 7, 2017

miq-bot changed the title ~~[WIP] Physical server page~~ Physical server page Apr 25, 2017

miq-bot removed the wip label Apr 25, 2017

AparnaKarve reviewed Apr 25, 2017

View reviewed changes

dclarizio added compute/physical infrastructure enhancement fine/no labels Apr 25, 2017

AparnaKarve reviewed Apr 25, 2017

View reviewed changes

dclarizio closed this May 3, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Physical server page #970

Physical server page #970

gabrielsvinha commented Apr 7, 2017 •

edited

Loading

miq-bot commented Apr 25, 2017

gabrielsvinha commented Apr 25, 2017

AparnaKarve Apr 25, 2017

AparnaKarve Apr 25, 2017

AparnaKarve Apr 25, 2017

AparnaKarve Apr 25, 2017

gabrielsvinha Apr 26, 2017

AparnaKarve Apr 25, 2017

gabrielsvinha Apr 26, 2017

AparnaKarve Apr 25, 2017

AparnaKarve Apr 25, 2017

AparnaKarve Apr 25, 2017

AparnaKarve Apr 25, 2017

AparnaKarve Apr 25, 2017

gabrielsvinha Apr 26, 2017

AparnaKarve Apr 25, 2017

gabrielsvinha Apr 26, 2017

AparnaKarve Apr 25, 2017

gabrielsvinha Apr 26, 2017

AparnaKarve Apr 25, 2017

gabrielsvinha Apr 26, 2017

AparnaKarve Apr 25, 2017

AparnaKarve commented Apr 25, 2017

AparnaKarve Apr 25, 2017

AparnaKarve commented Apr 25, 2017

AparnaKarve Apr 25, 2017

gabrielsvinha commented Apr 26, 2017

AparnaKarve commented Apr 26, 2017

gabrielsvinha commented Apr 28, 2017

dclarizio commented May 3, 2017

		@@ -1,44 +1,52 @@
		module PhysicalServerHelper::TextualSummary

Physical server page #970

Physical server page #970

Conversation

gabrielsvinha commented Apr 7, 2017 • edited Loading

miq-bot commented Apr 25, 2017

gabrielsvinha commented Apr 25, 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

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

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

AparnaKarve commented Apr 25, 2017

Choose a reason for hiding this comment

AparnaKarve commented Apr 25, 2017

Choose a reason for hiding this comment

gabrielsvinha commented Apr 26, 2017

AparnaKarve commented Apr 26, 2017

gabrielsvinha commented Apr 28, 2017

dclarizio commented May 3, 2017

gabrielsvinha commented Apr 7, 2017 •

edited

Loading