Fix resize approval to work for editing requests #2598

sseago · 2017-11-01T20:41:06Z

The initial "resize approval workflow" feature work is missing some follow-on work needed to allow admin editing of resize/reconfigure requests before approval.

This PR depends on core PR ManageIQ/manageiq#16381

sseago · 2017-11-07T17:36:01Z

FYI, this isn't quite ready to merge. I'm working through some issues that @mansam and I discovered (redirect problems, etc.) I'm working on an update to this.

sseago · 2017-11-13T14:03:20Z

The latest update should include the fixes that @mansam and I discovered.

ZitaNemeckova · 2018-03-20T14:10:12Z

app/assets/javascripts/controllers/vm_cloud/vm_cloud_resize_form_controller.js

+    if (vmCloudResizeFormId == 'new')
+      vm.newRecord = true;
+    else
+      vm.newRecord = false;


This can be replaced by vm.newRecord = vmCloudResizeFormId == 'new';.

ZitaNemeckova · 2018-03-20T14:11:38Z

@miq-bot remove_label pending core

@sseago Please add some steps to reproduce so I can check it does what it's supposed to. Thanks :)

ZitaNemeckova · 2018-03-20T14:12:05Z

@miq-bot add_label gaprindashvili/yes

sseago · 2018-03-21T16:33:26Z

@ZitaNemeckova To test:
(make sure that there are at least two additional flavors available in addition to the current flavor of your instance)
go to compute -> cloud -> instances
choose instance
configuration -> "reconfigure selected instance"
Choose a flavor to reconfigure to
submit
go to services -> requests
Click on the request. You should be able to edit the request before
approving it. If you make the original reconfigure request as an
admininstrator, the request may auto-approve. If that happens you may
need to try again, starting with a non-admin user.

ZitaNemeckova · 2018-03-22T10:40:48Z

app/assets/javascripts/controllers/vm_cloud/vm_cloud_resize_form_controller.js

-    $http.get('/vm_cloud/resize_form_fields/' + vmCloudResizeFormId)
+    vm.newRecord = vmCloudResizeFormId == 'new';
+
+    $http.get('/vm_cloud/resize_form_fields/' + vmCloudResizeFormId + ',' + vm.objectId)


Don't send two ids in one param. Later you have to split them and it's too "hacky". You can send the second id as '&req_id=' + vm.objectId as you do in js redirect later. That way it's always clear what it is.

ZitaNemeckova · 2018-03-22T10:43:26Z

app/controllers/mixins/actions/vm_actions/resize.rb

@@ -86,7 +98,9 @@ def resize_vm

        def resize_form_fields
          assert_privileges("instance_resize")
-          @record = find_record_with_rbac(VmOrTemplate, params[:id])
+
+          @request_id, record_id = params[:id].split(/\s*,\s*/, 2)


Please send those ids in separate params. Do something like this:

record_id = params[:id] @request_id = params[:req_is]

(I hope I didn't switch them in my example :D )

Sure. I can switch this around. The above was modeled after the existing reconfigure code, so if we're changing this here, at some point perhaps the other should be modified as well.

And yes, you did switch them in your example, but that's OK -- I know what you meant.

ZitaNemeckova · 2018-03-22T10:50:48Z

Before:

After:

Editing looks good but redirect after submit/cancel is weird. It redirects to instance summary page but it's missing the tree. Do you want it to be redirected there or to previous page (request summary page). If latter you can change this line https://github.com/ManageIQ/manageiq-ui-classic/blob/master/app/controllers/mixins/actions/vm_actions/resize.rb#L78 to if @sb[:explorer] && !previous_breadcrumb_url.include?('miq_request') (which is ugly and any nicer solution is welcome :) ). It would look like this:

sseago · 2018-04-09T02:38:53Z

So I made the first suggested changes (around the URL param passing). The redirect on submit suggestion didn't work, though -- the issue is that with the suggestion applied, on the initial resize form, that prevents the form from going anywhere. You click submit and the form stays as-is. However irregular the redirect behavior is with the PR as-is, it's better than having a form that doesn't disappear on submit. Also, I remember when first going through this functionality, I was unable to come up with a way that worked as desired for both initial form submission and on edit in the approval process.

ZitaNemeckova · 2018-04-19T13:25:09Z

@sseago I didn't test my suggestion on initial form (shame on me). I fixed and tested it on both forms. Could you try if @sb[:explorer] && !(@breadcrumbs.length >= 2 && previous_breadcrumb_url.include?('miq_request')) instead ofif @sb[:explorer] && !previous_breadcrumb_url.include?('miq_request'). Thanks :)

sseago · 2018-05-09T13:29:22Z

@ZitaNemeckova With the prior update, the suggested changes have been made.

ZitaNemeckova · 2018-05-14T08:26:28Z

@sseago Thanks :) I'm not sure about one thing. When editing a request original flavor is not set and it's hard to say which one was set before. Is that an intension or not?

Chosen flavor is unknown:

Chosen flavor is known:

Otherwise it looks good :)

sseago · 2018-06-05T12:48:30Z

@ZitaNemeckova I've removed WIP from the title. I don't think I have permission to remove labels, but perhaps miq-bot removes WIP automatically in response to the title change like it does when adding [WIP]

himdel · 2018-06-12T09:11:41Z

app/assets/javascripts/controllers/vm_cloud/vm_cloud_resize_form_controller.js

  var vm = this;

  var init = function() {
    vm.vmCloudModel = {
      flavor_id: null,
    };
    vm.flavors = [];
-    vm.formId = vmCloudResizeFormId;
-    vm.modelCopy = angular.copy(vm.vmCloudModel);
+    vm.vmCloudResizeformId = vmCloudResizeFormId;


prefer formId, but if you need this name, please make sure this is vmCloudResizeFormId and not ...formId

also, note that formId should be the id of the thing being edited - which is not a request, it's the vm

I'll fix the capitalization of the 'F' there. With so many moving parts (and a review dragging out six months), I'd rather not mess with the name beyond that and risk breaking something that I'd forgotten about. In the context of this form (just like the reconfigure javascript form controller that I'm not touching), the ID is in fact the request, since that's the way the existing UI is already designed (again, I'm just fixing a bug here, not writing a new feature). It's a "resize instance request" that's being created or edited -- the actual VM doesn't get changed until the request is approved.

Well, the thing is - there is a possibility you're introducing new bugs, just by removing the vm.formId as it was before.

(Not sure, but we'd have to check.)

So... can you fix the bug without chaning the meaning of vm.formId?

himdel · 2018-06-12T09:12:58Z

app/assets/javascripts/controllers/vm_cloud/vm_cloud_resize_form_controller.js

  };

  $scope.submitClicked = function() {
    miqService.sparkleOn();
    var url = '/vm_cloud/resize_vm/' + vmCloudResizeFormId + '?button=submit';
-    miqService.miqAjaxButton(url, vm.vmCloudModel);
+    miqService.miqAjaxButton(url, {objectId: vm.objectId,


bad indent

miqService.miqAjaxButton(url, { objectId: vm.objectId, flavor_id: vm.vmCloudModel.flavor_id, });

himdel · 2018-06-12T09:13:13Z

app/assets/javascripts/controllers/vm_cloud/vm_cloud_resize_form_controller.js

  var vm = this;

  var init = function() {
    vm.vmCloudModel = {
      flavor_id: null,
    };
    vm.flavors = [];
-    vm.formId = vmCloudResizeFormId;
-    vm.modelCopy = angular.copy(vm.vmCloudModel);


missing modelCopy for new

See comment above from @ZitaNemeckova -- modelCopy has been moved into getResizeFormData since it has to be called after setting everything in vmCloudMOdel

Ah, sorry about that. Usually get*FormData only gets called when editing, not new.

But you're right, in your case it handles both, withdrawing my objection :).

himdel · 2018-06-12T09:16:01Z

app/controllers/miq_request_controller.rb

@@ -39,7 +39,11 @@ def request_edit
      session[:checked_items] = provision_request.options[:src_ids]
      @refresh_partial = "reconfigure"
      @_params[:controller] = "vm"
-      reconfigurevms
+      if provision_request.kind_of?(VmCloudReconfigureRequest)
+        resizevms


Why this? Is this really the place?

It doesn't sound right that reconfigure also does resize sometimes.

Is this a new feature? (The PR title does not seem to indicate so)

The back end is "resize" (with request class VmCloudReconfigureRequest) for OpenStack and "reconfigure" (with request class VmReconfigureRequest) for other providers. The issue is that it was decided at some point in the past to expose these two completely different back-end actions as the same thing in the UI. On the model/back end side the actions are completely different, but from a user perspective, they have similar effects (in different providers). The result is that conditionals like this are necessary here.

This is not a new feature -- this PR fixes a bug -- the "edit request" action for administrators does not currently work for resize. A user can request a resize, and an admin can approve it as-is, but without this PR, the request cannot be edited.

Aah, makes sense, thanks for the explanation! :)

himdel · 2018-06-12T09:17:47Z

app/controllers/mixins/actions/vm_actions/resize.rb

@@ -3,16 +3,26 @@ module Actions
    module VmActions
      module Resize
        def resizevms
-          assert_privileges("instance_resize")
-          recs = find_checked_items
+          assert_privileges(params[:pressed])


Definitely not!

We can not allow the user to pick which RBAC feature to check for.

This would need a whitelist at least, but .. what's the value of pressed in question here?

@himdel I'm simply copying what's done in reconfigure.rb, which was the primary model for these changes (since both resize and reconfigure use the same UI, exposed as 'reconfigure') -- also, grepping for this, this seems to be a very common pattern. If this isn't the right way to do this, what exactly should be done here? Part of what makes these forms difficult is that the same form has to be used both for the initial request (in the context of the instance/VM ui) as well as by an admin on editing the request (in the context of the requests UI).

Yes, sorry, it's a pattern we're trying very hard to get rid of.

So... please fix this, by working on this you are in a better position to know the possible privileges and values for params[:pressed] than anyone else looking at it later.

Something like..

case params[:pressed] when "your_new_thing" assert_privileges("your_new_thing") else assert_privileges("instance_resize") end

would be enough.

OK. That works for me.

himdel · 2018-06-12T09:19:12Z

app/controllers/mixins/actions/vm_actions/resize.rb

-          recs = find_checked_items
+          assert_privileges(params[:pressed])
+          # if coming in to edit from miq_request list view
+          recs = checked_or_params


dead code? recs is never used before it is overwritten

EDIT: never mind, but is the

- recs = find_checked_items + recs = checked_or_params

relevant?

I'm fairly certain this wasn't working right before the change. Before this change, this didn't work for editing requests, just for new ones. Also, checked_or_params is what is used in reconfigure.rb in the equivalent line of code.

Ah, ok, the difference is that checked_or_params will also return params[:id] when no checkboxes are found.

Just making sure this was the intended result.

himdel · 2018-06-12T09:19:19Z

app/controllers/mixins/actions/vm_actions/resize.rb

          recs = [params[:id].to_i] if recs.blank?
-          @record = find_record_with_rbac(VmOrTemplate, recs.first) # Set the VM object
+          @record ||= find_record_with_rbac(VmOrTemplate, recs.first) # Set the VM object


This breaks any code where @record was previously set, are you sure none such exists?

@record may have been set a few lines above, in which case we want to keep it. This is to handle the case where it was not set. There is a possible edge case if @record is not set above within this method but is already set before -- we could handle that by setting @record to nil before the if block that optionally sets it.

himdel · 2018-06-12T09:20:03Z

app/controllers/mixins/actions/vm_actions/resize.rb

@@ -33,14 +43,18 @@ def resize
            )
          end
          @sb[:explorer] = @explorer
+          @request_id = params[:req_id] ? params[:req_id] : nil


this is just @request_id = params[:req_id]

fixing this...

himdel · 2018-06-12T09:24:23Z

I still have a big issue here with the id renames.

Before: formId/params[:id] was the id of the record being edited (and there was no mention of requests)

Now: params[:id] is an id of the request, and objectId is the new original id

I'm sorry but why would you do that? Why not just add a new thing, called requestId and make it clear?

More to the point, can you still fix this please? Otherwise we need to go through all the code to make sure the right id is not being depended on by any other piece of code.

himdel · 2018-06-12T09:33:18Z

app/views/vm_common/_resize.html.haml

@@ -18,7 +19,7 @@
      .col-md-8
        %select{:name        => 'flavor_id',
                'ng-model'   => 'vm.vmCloudModel.flavor_id',
-                'ng-options' => 'flavor.id as flavor.name for flavor in vm.flavors track by flavor.id',
+                'ng-options' => 'flavor.id as flavor.name for flavor in vm.flavors',
                :miqrequired => true,
                :checkchange => true}


checkchange should not be used with form-changed, you should be able to remove each checkchange here now :)

sseago · 2018-06-12T14:04:04Z

@himdel As for the record/request ID thing, having gone through this again, it's become a bit clearer (again, with this review taking six months, some of the details are a bit fuzzy to me right now). The basic summary is this: resize and reconfigure backend actions were required to work within the same front end UI framework. That was the starting requirement for all of this. Reconfigure actually works for both creating and editing requests, and for the existing reconfigure functionality (which I'm not modifying at all) uses the request ID as the ID (since that's the actual object being created and modified). Given all of this, the only straightforward way to get them to work together within the constraints I was working under was to follow the design of the existing working form as much as possible. If I had been free to decouple them and create a separate "resize" action, exposed to the UI as a separate action, this whole thing would have gone differently.

himdel · 2018-06-13T11:32:32Z

@sseago I'm sorry ... I get what you're saying but... correct me if I'm wrong but:

Statement 1: params[:id] now means something different than before

I think that's true because...

-  ManageIQ.angular.app.value('vmCloudResizeFormId', '#{@record.id}');
+  ManageIQ.angular.app.value('vmCloudResizeFormId', '#{@request_id || "new"}');
+  ManageIQ.angular.app.value('objectId', '#{@record.id}');

and

-    $http.get('/vm_cloud/resize_form_fields/' + vmCloudResizeFormId)
+    $http.get('/vm_cloud/resize_form_fields/' + vmCloudResizeFormId + '?objectId=' + vm.objectId)

Statement 2: reconfigurevms reads params[:id] and was not changed to account for the different meaning

I think that's true just because it does and it wasn't :).

Result
But If both of those are true, then this PR breaks the existing functionality. Am I wrong?

sseago · 2018-06-14T15:51:36Z

@himdel I don't believe that it breaks existing functionality because 1) for the 'edit' case, it didn't work before anyway and 2) for the 'new' case, everything that was needed to be changed to reflect the change in meaning of :id should be done already. This change was to bring the partially broken resize UI functionality in line with the fully working reconfigure UI functionality. Do you see anything that looks like it is broken with this change? We've tested the 'new' and 'edit' cases with this PR and both seem to be fully functional.

himdel · 2018-06-15T13:28:40Z

for the 'edit' case, it didn't work before anyway

What didn't work, resize or reconfigure? If resize, my point still stands.
(If reconfigure, what do you mean by "fully working reconfigure UI functionality"?)

Do you see anything that looks like it is broken with this change?

Still not sure what to test actually, the description is not really telling me anything.
And just to be clear, I'm not worried at all about the resize functionality. I believe Zita thoroughly tested this.
I am worried about breaking reconfigure.

himdel · 2018-06-15T13:52:46Z

@sseago Maybe what I'm missing could be this..

The screen that's using 'vmCloudResizeFormController' does say "Reconfigure" but maybe this screen is actually resize-only and there is a separate reconfigure screen that does not use any of the changes in your PR?
If so, would that mean that resize and reconfigure only really share the call to request_edit but are otherwise completely separate?

If that were so, it is possible that those id changes would indeed not affect reconfigure at all.

mansam · 2018-06-19T19:58:09Z

@himdel Reconfigure and Resize use two different views and two different angular controllers (and different controller methods, reconfigurevms vs resizevms). There's some overlap in labeling, but I don't believe they share code.

himdel · 2018-06-21T12:26:02Z

Aah, @mansam thanks, that helps! :)

Ok, risking it.

I still think it's a very bad idea to call the same resizevms method in 2 different contexts, with the meaning of params being completely different.

But, it does seem to work.

himdel · 2018-06-21T12:33:39Z

Aaah, but I was too rash, sorry :(.

@sseago You promised to fix all the other review comments and didn't.

So, please create a PR to fix:

keep vm.formId in the angular controller, no vmCloudResizeformId, especially not with the small f ( https://github.com/ManageIQ/manageiq-ui-classic/pull/2598/files#r195044205 )
fix the formatting on line 30 in the controller ( https://github.com/ManageIQ/manageiq-ui-classic/pull/2598/files#diff-40f8be8bd21ba536e9696e881ec5b792R30 )
replace that assert_privileges(params[:pressed]) with an explicit check for the rbac features we're expecting ( https://github.com/ManageIQ/manageiq-ui-classic/pull/2598/files#r194668783 )
make sure checked_or_params giving you params[:id] won't break anything, in either version ( https://github.com/ManageIQ/manageiq-ui-classic/pull/2598/files#r194740761 )
fix the problem when @record would already be set ( https://github.com/ManageIQ/manageiq-ui-classic/pull/2598/files#r194669262 )
https://github.com/ManageIQ/manageiq-ui-classic/pull/2598/files#r194740662
remove checkchange ( https://github.com/ManageIQ/manageiq-ui-classic/pull/2598/files#r194673529 )

(Or should we just revert for now?)

(@simaishi Please don't backport without the second PR - #4182)

mansam · 2018-06-21T15:52:53Z

@himdel @sseago went on PTO shortly after you two last exchanged comment, he'll be back to finish the changes next week. Perhaps it would be best for you to revert for now and then merge the completed PR once Scott is back?

himdel · 2018-06-22T12:03:08Z

Ah, thanks @mansam .. we'll address the remaining issues in #4182 (thanks @ZitaNemeckova ! :))

ZitaNemeckova · 2018-06-25T11:52:59Z

@himdel I don't believe that it breaks existing functionality because 1) for the 'edit' case, it didn't work before anyway and 2) for the 'new' case, everything that was needed to be changed to reflect the change in meaning of :id should be done already. This change was to bring the partially broken resize UI functionality in line with the fully working reconfigure UI functionality. Do you see anything that looks like it is broken with this change? We've tested the 'new' and 'edit' cases with this PR and both seem to be fully functional.

Unfortunately @himdel was right. It no longer works when you want to edit reconfigure request for Infra VM. New request for Infra VM reconfigure seems to work.

EDIT: ~~#4202 will fix that,~~ backport all of #2598 and #4182 together please.

EDIT2: I was wrong. It's not breaking editing reconfigure request for Infra VM.

sseago force-pushed the resize_approval_edit branch from 8122a04 to e3b92fe Compare November 1, 2017 21:12

sseago mentioned this pull request Nov 2, 2017

Fix resize approval to work for editing requests ManageIQ/manageiq#16381

Merged

mzazrivec added bug compute/cloud technical debt pending core and removed technical debt labels Nov 3, 2017

sseago force-pushed the resize_approval_edit branch 2 times, most recently from 65e5302 to d16cf5d Compare November 8, 2017 17:08

ZitaNemeckova reviewed Mar 20, 2018

View reviewed changes

miq-bot added gaprindashvili/yes and removed pending core labels Mar 20, 2018

sseago force-pushed the resize_approval_edit branch from d16cf5d to d8b32ca Compare March 21, 2018 16:36

ZitaNemeckova reviewed Mar 22, 2018

View reviewed changes

martinpovolny changed the title ~~Fix resize approval to work for editing requests~~ [WIP] Fix resize approval to work for editing requests Apr 8, 2018

miq-bot added the wip label Apr 8, 2018

sseago force-pushed the resize_approval_edit branch from d8b32ca to 6529936 Compare April 9, 2018 02:32

sseago force-pushed the resize_approval_edit branch 2 times, most recently from fca9083 to 4658b64 Compare April 24, 2018 01:33

sseago changed the title ~~[WIP] Fix resize approval to work for editing requests~~ Fix resize approval to work for editing requests Jun 5, 2018

miq-bot removed the wip label Jun 5, 2018

ZitaNemeckova approved these changes Jun 11, 2018

View reviewed changes

himdel reviewed Jun 12, 2018

View reviewed changes

himdel merged commit 62f7d48 into ManageIQ:master Jun 21, 2018

himdel added this to the Sprint 89 Ending Jul 2, 2018 milestone Jun 21, 2018

himdel self-assigned this Jun 21, 2018

ZitaNemeckova mentioned this pull request Jun 22, 2018

Fix issues in Reconfigure/Resize #4182

Merged

7 tasks

ZitaNemeckova mentioned this pull request Jun 25, 2018

[WIP] Fix infra request reconfig #4202

Closed

3 tasks

Fix resize approval to work for editing requests #2598

Fix resize approval to work for editing requests #2598

Conversation

sseago commented Nov 1, 2017

sseago commented Nov 7, 2017

sseago commented Nov 13, 2017

Choose a reason for hiding this comment

ZitaNemeckova commented Mar 20, 2018

ZitaNemeckova commented Mar 20, 2018

sseago commented Mar 21, 2018

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

ZitaNemeckova commented Mar 22, 2018

sseago commented Apr 9, 2018

ZitaNemeckova commented Apr 19, 2018

sseago commented May 9, 2018

ZitaNemeckova commented May 14, 2018

sseago commented Jun 5, 2018

himdel Jun 12, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

himdel Jun 13, 2018 • edited 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

himdel Jun 12, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

himdel Jun 12, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

himdel Jun 12, 2018 • edited 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

himdel commented Jun 12, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sseago commented Jun 12, 2018

himdel commented Jun 13, 2018 • edited Loading

sseago commented Jun 14, 2018

himdel commented Jun 15, 2018 • edited Loading

himdel commented Jun 15, 2018 • edited Loading

mansam commented Jun 19, 2018 • edited Loading

himdel commented Jun 21, 2018 • edited Loading

himdel commented Jun 21, 2018 • edited Loading

mansam commented Jun 21, 2018

himdel commented Jun 22, 2018

ZitaNemeckova commented Jun 25, 2018 • edited by himdel Loading

himdel Jun 12, 2018 •

edited

Loading

himdel Jun 13, 2018 •

edited

Loading

himdel Jun 12, 2018 •

edited

Loading

himdel Jun 12, 2018 •

edited

Loading

himdel Jun 12, 2018 •

edited

Loading

himdel commented Jun 13, 2018 •

edited

Loading

himdel commented Jun 15, 2018 •

edited

Loading

himdel commented Jun 15, 2018 •

edited

Loading

mansam commented Jun 19, 2018 •

edited

Loading

himdel commented Jun 21, 2018 •

edited

Loading

himdel commented Jun 21, 2018 •

edited

Loading

ZitaNemeckova commented Jun 25, 2018 •

edited by himdel

Loading