UI for create and delete flavors #1818

alexander-demicev · 2017-08-03T10:19:13Z

Compute > Clouds > Flavors
Depends on: ManageIQ/manageiq#15552.

Add UI for create and delete flavors

Adding flavor:

Removing flavor:

himdel · 2017-08-03T12:47:59Z

app/views/flavor/_flavor_form.html.haml

+        %span.help-block{"ng-show" => "angularForm.rxtx_factor.$error.floatGreaterThanZero"}
+          = _("RXTX factor must be greater than or equal to 0")
+
+    = render :partial => "layouts/angular/x_edit_buttons_angular"


You want generic_form_buttons here instead .. those x_edit are for non-controllerAs controllers.

himdel · 2017-08-03T12:49:50Z

app/controllers/flavor_controller.rb

+    assert_privileges('flavor_delete')
+    checked = find_checked_items
+    checked[0] = params[:id] if checked.blank? && params[:id]
+    FlavorController.model.where(:id => checked).each do |flavor|


This should be find_records_with_rbac(model, checked_or_params), and you don't need to do the checked[0] = params[:id] etc..

himdel · 2017-08-03T12:50:13Z

app/controllers/flavor_controller.rb

  end

  private

  def textual_group_list
-    [%i(properties relationships), %i(tags)]
+    [%i[properties relationships], %i[tags]]


Please change back to how it was.

himdel · 2017-08-03T12:50:18Z

app/controllers/flavor_controller.rb

@@ -10,15 +10,59 @@ class FlavorController < ApplicationController
  include Mixins::GenericSessionMixin

  def self.display_methods
-    %w(instances)
+    %w[instances]


Please change back to how it was.

himdel · 2017-08-03T12:51:55Z

app/assets/javascripts/controllers/flavor_credentials/flavor_form_controller.js

+  var getBack = function(response) {
+    var message = '';
+    var err = false;
+    if (response.hasOwnProperty('results')) {


I guess the API really returns a 200 on error?

Can you perhaps separate this function into 2? (error and non-error) .. looks like this has too many if (! err) all around, and could be more readable as 2 functions.

himdel · 2017-08-03T12:54:10Z

app/assets/javascripts/controllers/flavor_credentials/flavor_form_controller.js

+    miqService.sparkleOff();
+  };
+
+  function getEmsFormDataComplete(response) {


You're changing the model here, but setForm can already have been called in getManagerResource .. I guess you want to wait for both requests to suceed before calling setForm and sparkleOff.

($q.all is your friend :))

himdel · 2017-08-03T12:55:05Z

app/assets/javascripts/controllers/flavor_credentials/flavor_form_controller.js

+      .then(getManagerResource)
+      .catch(miqService.handleFailure);
+  };
+


Also looks like you may be missing resetClicked

Reset button is enabled when record is not new, flavor updating is not possible, for now I don`t need it.

himdel · 2017-08-03T13:02:34Z

app/assets/javascripts/directives/int_greater_than_zero.js

+  return {
+    require: 'ngModel',
+    link: function(_scope, _elem, _attrs, ctrl) {
+      ctrl.$validators.intGreaterThanZero = function(modelValue, viewValue) {


We should probably split these directives differently..

Right now, you have..

is float && >0

is int && >0

is int && >= 0

This looks like it could better split as..

is int

is float

is >0

is >=0

.. and since we're using angular.validators (https://github.com/gkaimakas/angular.validators) ...

We already have is-float

We already have is-int

input type=number min=0 already validates >= 0, but the only way to test a float for >0 would be step="0.01" + min="0.01", which may be undesirable .. but we can have a greaterThanZero directive..

input type=number min=0 already validates >= 0, unless you can't use an input type number, in which case we'd need a greaterOrZero as well.

himdel · 2017-08-03T13:15:52Z

app/helpers/application_helper/toolbar/flavors_center.rb

@@ -1,4 +1,30 @@
 class ApplicationHelper::Toolbar::FlavorsCenter < ApplicationHelper::Toolbar::Basic
+  button_group('flavor_configuration', [
+    select(
+      :aflavor_configuration_choice,


Atypo? :) Guessing this should be :flavor_configuration_choice :)

alexander-demicev · 2017-08-04T10:01:11Z

Thank you for review, @himdel ! Does it look better now?

himdel · 2017-08-16T10:38:38Z

app/assets/javascripts/directives/is_float.js

@@ -0,0 +1,21 @@
+ManageIQ.angular.app.directive('isFloat', function() {


This is a duplicate of angular validator's is-float.

So I think you can just remove this file and it will work equally well ;)

himdel · 2017-08-16T10:38:57Z

app/assets/javascripts/directives/is_integer.js

@@ -0,0 +1,22 @@
+ManageIQ.angular.app.directive('isInteger', function() {


Same here, is-integer already exists in angular-validators ... except it is called is-int ;)

himdel · 2017-08-17T11:29:45Z

@alexander-demichev The code looks fine now (EDIT: except for response, and stuff), but I never see that Configuration button in Flavors.
Does this depend on a manageiq/ PR that's not merged yet? (I assume I'm missing the rbac features needed to see those buttons.)

himdel · 2017-08-17T11:30:57Z

Aah... indeed it does.. ManageIQ/manageiq#15552.

@alexander-demichev for future reference, in such cases, you need to mention the PR in the description, and add a pending core label until it is merged. I've done so for now :).

(I've also added info on where to look for those screens.)

dclarizio · 2017-10-19T17:59:55Z

@himdel can you please review the cc issues, perhaps a few more can be addressed.

tzumainn · 2017-10-20T14:54:09Z

Changes look good to me, thanks!

Loicavenel · 2017-10-25T06:29:23Z

@himdel Martin, we need your help here.. thanks

martinpovolny · 2017-10-25T09:48:56Z

app/controllers/flavor_controller.rb

+    flavors = find_records_with_rbac(Flavor, checked_or_params)
+    flavors.each do |flavor|
+      begin
+        flavor.delete_flavor_queue(User.current_user.id)


this shoud be done through the API, not here :-(

martinpovolny · 2017-10-25T09:49:56Z

Oh, I see that @himdel agreed to get this fixed later...

But we really need to stop doing that!

martinpovolny · 2017-10-25T09:52:43Z

Guys, these CC issues are easy to fix:

'miqFlashLater' is not defined. NEW
    miqFlashLater(flashMessage);
Severity: MinorFound in app/assets/javascripts/controllers/flavor/flavor_form_controller.js by eslint

'miqFlashLater' is not defined. NEW
    miqFlashLater(flashMessage);
Severity: MinorFound in app/assets/javascripts/controllers/flavor/flavor_form_controller.js by eslint

Avoid using a variable and instead use chaining with the getter syntax. NEW

ManageIQ.angular.app.directive('greaterOrZero', function() {
Severity: MinorFound in app/assets/javascripts/directives/greater_or_zero.js by eslint

'modelValue' is defined but never used. NEW
      ctrl.$validators.greaterOrZero = function(modelValue, viewValue) {
Severity: MinorFound in app/assets/javascripts/directives/greater_or_zero.js by eslint

'modelValue' is defined but never used. NEW
      ctrl.$validators.greaterThanZero = function(modelValue, viewValue) {

Loicavenel · 2017-10-26T11:55:26Z

@aufi @tzumainn for visibility...

tzumainn · 2017-10-26T12:50:58Z

The style issues seem okay to me - I think a few of the CC issues are still fixable though?

miq-bot · 2017-10-26T14:03:11Z

Checked commit alexander-demicev@f64713b with ruby 2.3.3, rubocop 0.47.1, and haml-lint 0.20.0
3 files checked, 2 offenses detected

app/helpers/application_helper/toolbar/flavors_center.rb

❗ - Line 28, Col 3 - Style/IndentArray - Indent the right bracket the same as the first position after the preceding left parenthesis.
❗ - Line 3, Col 5 - Style/IndentArray - Use 2 spaces for indentation in an array, relative to the first position after the preceding left parenthesis.

alexander-demicev · 2017-10-26T14:09:04Z

@h-kataria Hi, can you take a look to this PR, it's close to be resolved.

tzumainn · 2017-10-26T18:47:59Z

@alexander-demichev is the chaining issue in cc fixable?

alexander-demicev · 2017-10-26T18:53:09Z

@tzumainn I don't know, didn't find any examples in other controllers/directives.

tzumainn · 2017-10-26T18:57:06Z

@alexander-demichev you're right, this does seem to match the existing code. Okay, nevermind!

h-kataria · 2017-10-26T18:58:46Z

looks like most comments/concerns have been addressed, merging.

martinpovolny · 2017-11-01T11:27:04Z

Oh my!

Gentleman and ladies, https://github.com/ManageIQ/manageiq-ui-classic/pull/1818/files#diff-3dcd700167653275bbf7873cfa7fa7c7R23

The delete in the else branch. That is like when one shoots himself in the leg with a mortar.

Now when I try to tag a flavor I delete it.

We need to aproach things (especially the destructive ones) with care!

We are reviewing and merging in too much hurry :-(

martinpovolny · 2017-11-01T11:36:34Z

app/controllers/flavor_controller.rb

+        add_flash(_("Delete of Flavor \"%{name}\" was successfully initiated.") % {:name => flavor.name})
+      rescue => error
+        add_flash(_("Unable to delete Flavor \"%{name}\": %{details}") % {:name    => flavor.name,
+                                                                          :details => error},


error is an exception, not a message. What do you expect to get displayed here?

alexander-demicev changed the title ~~UI for create and delete flavors~~ [WIP] UI for create and delete flavors Aug 3, 2017

miq-bot added the wip label Aug 3, 2017

alexander-demicev force-pushed the flavors branch 4 times, most recently from 09521f3 to ed37d81 Compare August 3, 2017 12:03

himdel reviewed Aug 3, 2017

View reviewed changes

himdel self-assigned this Aug 3, 2017

alexander-demicev force-pushed the flavors branch from ed37d81 to d9f94cc Compare August 4, 2017 10:00

alexander-demicev changed the title ~~[WIP] UI for create and delete flavors~~ UI for create and delete flavors Aug 4, 2017

miq-bot removed the wip label Aug 4, 2017

alexander-demicev force-pushed the flavors branch 3 times, most recently from 68921cd to e7d7bd2 Compare August 12, 2017 13:50

himdel reviewed Aug 16, 2017

View reviewed changes

alexander-demicev force-pushed the flavors branch from e7d7bd2 to f0e5254 Compare August 16, 2017 19:47

himdel added compute/cloud enhancement labels Aug 17, 2017

alexander-demicev force-pushed the flavors branch 2 times, most recently from b242cad to 0b8e291 Compare October 20, 2017 13:50

martinpovolny reviewed Oct 25, 2017

View reviewed changes

alexander-demicev force-pushed the flavors branch 4 times, most recently from 108e01b to 2e771b5 Compare October 26, 2017 12:45

alexander-demicev force-pushed the flavors branch 2 times, most recently from ecd495b to 1af8de3 Compare October 26, 2017 13:44

UI for create and delete flavors

f64713b

alexander-demicev force-pushed the flavors branch from 1af8de3 to f64713b Compare October 26, 2017 13:51

h-kataria added this to the Sprint 72 Ending Oct 30, 2017 milestone Oct 26, 2017

h-kataria merged commit bd7855a into ManageIQ:master Oct 26, 2017

martinpovolny reviewed Nov 1, 2017

View reviewed changes

martinpovolny mentioned this pull request Nov 1, 2017

Flavor delete vs tag #2590

Merged

ZitaNemeckova mentioned this pull request Aug 20, 2018

Don't show Add Flavor in sublist view #4180

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

UI for create and delete flavors #1818

UI for create and delete flavors #1818

alexander-demicev commented Aug 3, 2017 •

edited by himdel

Loading

himdel Aug 3, 2017

himdel Aug 3, 2017

himdel Aug 3, 2017

himdel Aug 3, 2017

himdel Aug 3, 2017

himdel Aug 3, 2017

himdel Aug 3, 2017

alexander-demicev Aug 4, 2017

himdel Aug 3, 2017

himdel Aug 3, 2017

alexander-demicev commented Aug 4, 2017 •

edited

Loading

himdel Aug 16, 2017

himdel Aug 16, 2017 •

edited

Loading

himdel commented Aug 17, 2017 •

edited

Loading

himdel commented Aug 17, 2017 •

edited

Loading

dclarizio commented Oct 19, 2017

tzumainn commented Oct 20, 2017

Loicavenel commented Oct 25, 2017

martinpovolny Oct 25, 2017

martinpovolny commented Oct 25, 2017 •

edited

Loading

martinpovolny commented Oct 25, 2017 •

edited

Loading

Loicavenel commented Oct 26, 2017

tzumainn commented Oct 26, 2017

miq-bot commented Oct 26, 2017

alexander-demicev commented Oct 26, 2017

tzumainn commented Oct 26, 2017

alexander-demicev commented Oct 26, 2017

tzumainn commented Oct 26, 2017

h-kataria commented Oct 26, 2017

martinpovolny commented Nov 1, 2017 •

edited

Loading

martinpovolny Nov 1, 2017

		@@ -0,0 +1,21 @@
		ManageIQ.angular.app.directive('isFloat', function() {

		@@ -0,0 +1,22 @@
		ManageIQ.angular.app.directive('isInteger', function() {

UI for create and delete flavors #1818

UI for create and delete flavors #1818

Conversation

alexander-demicev commented Aug 3, 2017 • edited by himdel Loading

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

alexander-demicev commented Aug 4, 2017 • edited Loading

Choose a reason for hiding this comment

himdel Aug 16, 2017 • edited Loading

Choose a reason for hiding this comment

himdel commented Aug 17, 2017 • edited Loading

himdel commented Aug 17, 2017 • edited Loading

dclarizio commented Oct 19, 2017

tzumainn commented Oct 20, 2017

Loicavenel commented Oct 25, 2017

Choose a reason for hiding this comment

martinpovolny commented Oct 25, 2017 • edited Loading

martinpovolny commented Oct 25, 2017 • edited Loading

Loicavenel commented Oct 26, 2017

tzumainn commented Oct 26, 2017

miq-bot commented Oct 26, 2017

alexander-demicev commented Oct 26, 2017

tzumainn commented Oct 26, 2017

alexander-demicev commented Oct 26, 2017

tzumainn commented Oct 26, 2017

h-kataria commented Oct 26, 2017

martinpovolny commented Nov 1, 2017 • edited Loading

Choose a reason for hiding this comment

alexander-demicev commented Aug 3, 2017 •

edited by himdel

Loading

alexander-demicev commented Aug 4, 2017 •

edited

Loading

himdel Aug 16, 2017 •

edited

Loading

himdel commented Aug 17, 2017 •

edited

Loading

himdel commented Aug 17, 2017 •

edited

Loading

martinpovolny commented Oct 25, 2017 •

edited

Loading

martinpovolny commented Oct 25, 2017 •

edited

Loading

martinpovolny commented Nov 1, 2017 •

edited

Loading