Let Select Provision Template screen use GTL #4509

PanSpagetka · 2018-08-21T12:24:51Z

When navigate to Compute -> Cloud -> Instances -> Provision with public images enabled, UI breaks down. It was caused by trying to load and display all Templates in single table. When rewritten using GTL, table is now paginated and lazy loading record for each page.

Links

https://bugzilla.redhat.com/show_bug.cgi?id=1610927
ManageIQ/manageiq#17884 & ManageIQ/manageiq#18217

Screenshot

New screenshot, without checkboxes

Steps for Testing/QA

You need to run bin/webpack in ui-classic directory

Add an ec2 provider with public images enabled
Go to Compute -> Cloud -> Instances
Select provision an instance

Try more provision screens, to be sure nothing is broken.

Backend PR needs to be merged first

PanSpagetka · 2018-08-22T13:26:57Z

@himdel @karelhala please look at it :)

PanSpagetka · 2018-08-23T07:49:37Z

@miq-bot add_label gtls
@miq-bot add_label pending core

miq-bot · 2018-08-23T07:50:18Z

@PanSpagetka Cannot apply the following label because they are not recognized: pending_core

PanSpagetka · 2018-08-23T08:01:01Z

@miq-bot add_label pending core

himdel · 2018-08-27T12:54:58Z

I haven't tested this, pending core, but LGTM. As far as I know, this is the first time we need to select a line in gtl (other than checkboxes).

romanblanco · 2018-09-19T08:31:50Z

app/views/miq_request/_pre_prov.html.haml

-    %tbody
-      - @vms.each do |row|
-        - if row.respond_to?(:volume_template?) || row.respond_to?(:volume_snapshot_template?)
-          = render :partial => "miq_request/pre_prov_volumes_snapshots", :locals => {:row => row}


@PanSpagetka these partials can be removed as well. This was their only usage

romanblanco

Before:

After:

@PanSpagetka the checkboxes shouldn't be present in the GTL table

karelhala

Looks good! Just don't include the pack if it's namaed *-common.

karelhala · 2018-09-20T08:50:01Z

app/views/miq_request/pre_prov.html.haml

@@ -7,3 +7,4 @@
      :ajax_buttons                 => true,
      :continue_button              => true,
      :noreset                      => true})
+= javascript_pack_tag "manageiq-ui-classic/toolbar-actions-common"


If your pack is named *-common you don't have to import it, it's imported automatically.

JPrause · 2018-10-11T20:26:52Z

@miq-bot add_label blocker

h-kataria · 2018-10-16T16:14:08Z

@PanSpagetka only a single template or Image is allowed to be selected on Pre-Provisioning screen, so i don't think we need Select All Checkbox on the screen and also need to make sure to enable/disable "Continue" button only when single item is selected.

JPrause · 2018-10-25T12:00:27Z

@PanSpagetka as this for a blocker issue, do you have an update on this PR.

PanSpagetka · 2018-11-01T13:33:06Z

@miq-bot remove_label pending core

romanblanco

ReferenceError: "provisioningListenToRx is not defined" error after clicking the "Provision Instances" button

himdel · 2018-11-05T13:39:24Z

@PanSpagetka you probably need this for the JS specs to pass:

diff --git a/spec/javascripts/support/jasmine.yml b/spec/javascripts/support/jasmine.yml
index 83de8be4d..86757641a 100644
--- a/spec/javascripts/support/jasmine.yml
+++ b/spec/javascripts/support/jasmine.yml
@@ -23,6 +23,7 @@ src_files:
 - __spec__/helpers/toolbar.js
 - packs/manageiq-ui-classic/application-common.js
 - packs/manageiq-ui-classic/toolbar-actions-common.js
+- packs/manageiq-ui-classic/provisioning-actions-common.js
 
 # stylesheets
 #

Not sure what's happening with ruby specs..

himdel · 2018-11-27T14:07:12Z

app/controllers/application_controller/miq_request_methods.rb

+      %w(auth_key_pair_cloud availability_zone cloud_tenant ems_cloud host_aggregate orchestration_stack vm_cloud).include?(request.parameters[:controller])
+      'ProvisionCloudTemplates.yaml'
+    elsif request.parameters[:template_klass] == 'infra' ||
+      %w(ems_cluster ems_infra host resource_pool storage vm_infra)


missing include?(something)

miq-bot · 2018-11-27T14:21:45Z

Some comments on commits PanSpagetka/manageiq-ui-classic@0491563~...b6a0e6f

spec/controllers/vm_cloud_controller_spec.rb

⚠️ - 177 - Detected expect_any_instance_of. This RSpec method is highly discouraged, please only use when absolutely necessary.

miq-bot · 2018-11-27T14:22:11Z

Checked commits PanSpagetka/manageiq-ui-classic@0491563~...b6a0e6f with ruby 2.3.3, rubocop 0.52.1, haml-lint 0.20.0, and yamllint 1.10.0
10 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.

himdel · 2018-11-28T13:12:05Z

app/controllers/application_controller/miq_request_methods.rb

+       %w(auth_key_pair_cloud availability_zone cloud_tenant ems_cloud host_aggregate orchestration_stack vm_cloud).include?(request.parameters[:controller])
+      'ProvisionCloudTemplates.yaml'
+    elsif request.parameters[:template_klass] == 'infra' ||
+          %w(ems_cluster ems_infra host resource_pool storage vm_infra).include?(request.parameters[:controller])


one of these lines is badly indented, either 1067 or 1070 ;)

himdel · 2018-11-28T13:14:24Z

app/assets/javascripts/controllers/report_data_controller.js

+      sendDataWithRx({
+        type: 'GTL_CLICKED',
+        actionType: this.initObject.additionalOptions.custom_action.type,
+        payload: { item: item, action: this.initObject.additionalOptions.custom_action }});


eslint should complain here, should be

sendDataWithRx({ type: 'GTL_CLICKED', actionType: this.initObject.additionalOptions.custom_action.type, payload: { item: item, action: this.initObject.additionalOptions.custom_action, }, });

EDIT: #5003

himdel · 2018-11-28T13:14:57Z

app/assets/javascripts/controllers/report_data_controller.js

+    var selectedItem = _.find(this.gtlData.rows, {long_id: item.long_id});
+    selectedItem.selected = true;
+    this.$window.sendDataWithRx({rowSelect: selectedItem});
+    ManageIQ.gridChecks =  [];


extra space before []

EDIT: #5003

himdel · 2018-11-28T13:17:12Z

spec/controllers/vm_cloud_controller_spec.rb

@@ -173,6 +173,22 @@
      expect(response.status).to eq(200)
    end

+    it 'renders gtl when open pre provision screen' do


we should probably have a similar test in vm_infra

himdel · 2018-11-28T13:37:22Z

Testing in UI:

vm_infra
- Compute > Infra > VMs, Lifecycle > Provision
- sorting works on all fields
- paging works
- seeing the same templates
- seeing the same columns
- same values (different format for memory, was 1GB, now 1,073,741,824)
vm_cloud
- Compute > Cloud > Instances, Lifecycle > Provision
- sorting works on all fields
- paging works
- hide deprecated works
- seeing the same images
- seeing the same columns
- but different values (slightly, see comment below)

Nested lists:

auth_key_pair_cloud - redirects to /miq_request/pre_prov?template_klass=cloud 👍
availability_zone - redirects to /miq_request/pre_prov?template_klass=cloud 👍
cloud_tenant - redirects to /miq_request/pre_prov?template_klass=cloud 👍
ems_cloud - redirects to /miq_request/pre_prov?template_klass=cloud 👍
host_aggregate - redirects to /miq_request/pre_prov?template_klass=cloud 👍
orchestration_stack - redirects to /miq_request/pre_prov?template_klass=cloud 👍

ems_cluster - redirects to /miq_request/pre_prov?template_klass=infra 👍
ems_infra - redirects to /miq_request/pre_prov?template_klass=infra 👍
host - redirects to /miq_request/pre_prov?template_klass=infra 👍
resource_pool - redirects to /miq_request/pre_prov?template_klass=infra 👍
storage - redirects to /miq_request/pre_prov?template_klass=infra 👍

himdel · 2018-11-28T13:49:26Z

The different values for cloud:

name, type, OS, platform, provider - always match

cpu - there used to be a distinction between 0 and N/A, now it's 0 in both cases (but not sure how meaningful that distinction is in the first place)

memory - the same as cpu, plus when it was not N/A, you'd get "0 bytes", not 0.

disk size - N/A now looks as ""

deprecated - was "false", "N/A" or "true", now it's "", "N/A" or "true" (so false becomes an empty string)

snapshot - N/A becomes 0

tenant - N/A becomes ""

I have no idea what the 0 vs "" vs N/A distinction really means (how is a template with 0 memory different from a template with N/A memory), and it doesn't seem documened, so maybe those differences don't matter.

The one that does look like a bug is that "false" to "" change in deprecated, any chance that can be fixed?

himdel · 2018-11-28T13:52:06Z

Another change is that sorting is now case-insensitive (so "CentOS Atomic" goes after "centos-6" now, and not before), but that matches all the other screens 👍

himdel · 2018-11-28T14:15:07Z

Ok, so with ManageIQ/manageiq#18237, I'm not seeing anything surprising anymore 👍

The memory format change seems inevitable, as I'm not aware of a way to sort by the field numerically but present in in a human-readable form with the current GTL implementation. We can probably fix that later if desired.

(In the miq_request screen, there is no hide deprecated button, even when template_klass=cloud. But thats also present on master.)

himdel · 2018-11-28T14:18:47Z

OK, looks like there are no more reasons to hold off merging :).

Let Select Provision Template screen use GTL (cherry picked from commit 3b71ea1) https://bugzilla.redhat.com/show_bug.cgi?id=1610927

simaishi · 2018-11-28T15:20:06Z

Hammer backport details:

$ git log -1
commit 1e43eb18b956e15c717407a363dfc029a8dab979
Author: Martin Hradil <himdel@seznam.cz>
Date:   Wed Nov 28 15:18:50 2018 +0100

    Merge pull request #4509 from PanSpagetka/bz-1610927
    
    Let Select Provision Template screen use GTL
    
    (cherry picked from commit 3b71ea1e57c239245b43200a733e2fda4f357717)
    
    https://bugzilla.redhat.com/show_bug.cgi?id=1610927

PanSpagetka mentioned this pull request Aug 21, 2018

Add template methods needed for provision report ManageIQ/manageiq#17884

Merged

PanSpagetka force-pushed the bz-1610927 branch 2 times, most recently from 6756adc to 0450049 Compare August 22, 2018 13:26

PanSpagetka force-pushed the bz-1610927 branch from 0450049 to 2fd3d7f Compare August 22, 2018 13:28

miq-bot added the gtls label Aug 23, 2018

PanSpagetka force-pushed the bz-1610927 branch 2 times, most recently from 6499ea6 to 7e13606 Compare August 23, 2018 08:00

miq-bot added the pending core label Aug 23, 2018

PanSpagetka force-pushed the bz-1610927 branch from 7e13606 to 64f6e2c Compare August 23, 2018 08:59

romanblanco suggested changes Sep 19, 2018

View reviewed changes

romanblanco suggested changes Sep 20, 2018

View reviewed changes

karelhala reviewed Sep 20, 2018

View reviewed changes

miq-bot added the blocker label Oct 12, 2018

PanSpagetka force-pushed the bz-1610927 branch from 64f6e2c to fae1ec3 Compare November 1, 2018 13:31

miq-bot removed the pending core label Nov 1, 2018

himdel closed this Nov 1, 2018

himdel reopened this Nov 1, 2018

romanblanco suggested changes Nov 1, 2018

View reviewed changes

PanSpagetka closed this Nov 5, 2018

PanSpagetka reopened this Nov 5, 2018

PanSpagetka force-pushed the bz-1610927 branch from 32e8d88 to 45dbec6 Compare November 26, 2018 13:30

PanSpagetka closed this Nov 27, 2018

PanSpagetka reopened this Nov 27, 2018

himdel reviewed Nov 27, 2018

View reviewed changes

himdel removed the pending core label Nov 27, 2018

Fix Code Climate

b6a0e6f

PanSpagetka force-pushed the bz-1610927 branch from b6452da to b6a0e6f Compare November 27, 2018 14:18

himdel self-assigned this Nov 28, 2018

himdel added bug hammer/yes labels Nov 28, 2018

himdel reviewed Nov 28, 2018

View reviewed changes

PanSpagetka mentioned this pull request Nov 28, 2018

Display deprecated as a true/false string ManageIQ/manageiq#18237

Merged

himdel merged commit 3b71ea1 into ManageIQ:master Nov 28, 2018

himdel added this to the Sprint 100 Ending Dec 3, 2018 milestone Nov 28, 2018

himdel mentioned this pull request Nov 28, 2018

ReportDataController - fix a few style issues #5003

Merged

simaishi pushed a commit that referenced this pull request Nov 28, 2018

Merge pull request #4509 from PanSpagetka/bz-1610927

1e43eb1

Let Select Provision Template screen use GTL (cherry picked from commit 3b71ea1) https://bugzilla.redhat.com/show_bug.cgi?id=1610927

simaishi added hammer/backported and removed hammer/yes labels Nov 28, 2018

himdel mentioned this pull request Jan 10, 2019

Fix missing paging in provisioning template selection #5149

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Let Select Provision Template screen use GTL #4509

Let Select Provision Template screen use GTL #4509

PanSpagetka commented Aug 21, 2018 •

edited by himdel

Loading

PanSpagetka commented Aug 22, 2018

PanSpagetka commented Aug 23, 2018 •

edited

Loading

miq-bot commented Aug 23, 2018

PanSpagetka commented Aug 23, 2018

himdel commented Aug 27, 2018

romanblanco Sep 19, 2018

romanblanco left a comment

karelhala left a comment

karelhala Sep 20, 2018

JPrause commented Oct 11, 2018

h-kataria commented Oct 16, 2018

JPrause commented Oct 25, 2018

PanSpagetka commented Nov 1, 2018

romanblanco left a comment

himdel commented Nov 5, 2018 •

edited

Loading

himdel Nov 27, 2018

miq-bot commented Nov 27, 2018

miq-bot commented Nov 27, 2018

himdel Nov 28, 2018

himdel Nov 28, 2018 •

edited

Loading

himdel Nov 28, 2018 •

edited

Loading

himdel Nov 28, 2018

himdel commented Nov 28, 2018 •

edited

Loading

himdel commented Nov 28, 2018

himdel commented Nov 28, 2018

himdel commented Nov 28, 2018 •

edited

Loading

himdel commented Nov 28, 2018

simaishi commented Nov 28, 2018

Let Select Provision Template screen use GTL #4509

Let Select Provision Template screen use GTL #4509

Conversation

PanSpagetka commented Aug 21, 2018 • edited by himdel Loading

Links

Screenshot

Steps for Testing/QA

PanSpagetka commented Aug 22, 2018

PanSpagetka commented Aug 23, 2018 • edited Loading

miq-bot commented Aug 23, 2018

PanSpagetka commented Aug 23, 2018

himdel commented Aug 27, 2018

romanblanco Sep 19, 2018

Choose a reason for hiding this comment

romanblanco left a comment

Choose a reason for hiding this comment

karelhala left a comment

Choose a reason for hiding this comment

karelhala Sep 20, 2018

Choose a reason for hiding this comment

JPrause commented Oct 11, 2018

h-kataria commented Oct 16, 2018

JPrause commented Oct 25, 2018

PanSpagetka commented Nov 1, 2018

romanblanco left a comment

Choose a reason for hiding this comment

himdel commented Nov 5, 2018 • edited Loading

himdel Nov 27, 2018

Choose a reason for hiding this comment

miq-bot commented Nov 27, 2018

miq-bot commented Nov 27, 2018

himdel Nov 28, 2018

Choose a reason for hiding this comment

himdel Nov 28, 2018 • edited Loading

Choose a reason for hiding this comment

himdel Nov 28, 2018 • edited Loading

Choose a reason for hiding this comment

himdel Nov 28, 2018

Choose a reason for hiding this comment

himdel commented Nov 28, 2018 • edited Loading

himdel commented Nov 28, 2018

himdel commented Nov 28, 2018

himdel commented Nov 28, 2018 • edited Loading

himdel commented Nov 28, 2018

simaishi commented Nov 28, 2018

PanSpagetka commented Aug 21, 2018 •

edited by himdel

Loading

PanSpagetka commented Aug 23, 2018 •

edited

Loading

himdel commented Nov 5, 2018 •

edited

Loading

himdel Nov 28, 2018 •

edited

Loading

himdel Nov 28, 2018 •

edited

Loading

himdel commented Nov 28, 2018 •

edited

Loading

himdel commented Nov 28, 2018 •

edited

Loading