Redirect delete action to cloud volume controller

[gberginc]

As it turned out, the delete action, when invoked from the ems_storage
view, did not have any effect on the selected cloud volumes. This patch
introduces a simple redirect to the
CloudVolumeController.delete_volumes ensuring the selected volumes are
deleted from the provider.

Fixes https://bugzilla.redhat.com/show_bug.cgi?id=1449293

@miq-bot add_label bug,storage
@miq-bot assign @AparnaKarve

[AparnaKarve]

@gberginc As we discussed, while this works well if we only select the Cloud Volumes and delete them, it gives a 204 error when the @lastaction is show.

We might need a little bit of work in delete_volumes, specifically in the area where we redirect back to the list.

[miq-bot]

Checked commits gberginc/manageiq-ui-classic@b4754ec~...bbdef76 with ruby 2.2.6, rubocop 0.47.1, and haml-lint 0.20.0
2 files checked, 0 offenses detected
Everything looks fine. 🏆

[gberginc]

@AparnaKarve we tried different things to overcome the problem of the redirect back. First, we tried adding additional query parameter that would simply reset the lastaction. In the end we decided to propose this. Honestly, I can't think of any other idea of how to overcome the problem here. Tested cases:

• open ebs_storage list and delete selected volumes
• open ebs_storage, show volume details --> delete this volume
• open ebs_storage, show volume details, go back (breadcrumb) --> delete some volumes
• open cloud_volumes list and delete selected volumes
• open cloud_volumes, show volume details --> delete

[AparnaKarve]

Interesting approach :)
Maybe it's just me, but the syntax seems a bit cryptic.
Although, I must say, it's the most logical approach to make @lastaction = nil in this scenario.

I was thinking along the lines of adding an "extra" condition in the elsif in delete_volumes

--- a/app/controllers/cloud_volume_controller.rb
+++ b/app/controllers/cloud_volume_controller.rb
@@ -454,7 +454,7 @@ class CloudVolumeController < ApplicationController
     if @lastaction == "show_list" && @breadcrumbs.last[:url].include?(@lastaction)
       show_list
       @refresh_partial = "layouts/gtl"
-    elsif @lastaction == "show" && @layout == "cloud_volume"
+    elsif @lastaction == "show" && @layout == "cloud_volume" && !params[:redirect]
       @single_delete = true unless flash_errors?
       if @flash_array.nil?
         add_flash(_("The selected %{model} was deleted") % {:model => ui_lookup(:table => "cloud_volume")})

--- a/app/controllers/ems_common.rb
+++ b/app/controllers/ems_common.rb
@@ -479,10 +479,11 @@ module EmsCommon
     elsif params[:pressed] == "cloud_volume_delete"
       javascript_redirect :controller      => "cloud_volume",
                           :action          => "delete_volumes",
-                          :miq_grid_checks => params[:miq_grid_checks]
+                          :miq_grid_checks => params[:miq_grid_checks],
+                          :redirect        => true

LMK what you think of the above approach and we will take it from there.

[gberginc]

@AparnaKarve This is exactly the change I had initially. But then I started looking more closely at the conditions and realised I did not understand why they differ. From what I understand the only way delete_volumes is called is if the Delete volume is requested from the Configuration menu either in the list of cloud volumes or from the single volume details.

Since delete_volumes was not in routes.rb before, it was not possible to launch it from anywhere else. Thus, I don't really understand why the two conditions differ (one checks @breadcrumbs, the other @layout. I think adding another option (redirect) would add to the confusion. Thus, we discussed with @miha-plesko, and proposed the current change (resetting of lastaction).

That being said, since we already thought about introducing additional GET parameter, I have no problem applying your proposed change. It only bothers me that I really don't understand what these conditions are actually protecting. Looking at other controllers (e.g. cloud_network, security_group), they are much simpler.

[martinpovolny]

@gberginc, @AparnaKarve : My current impression is that the button actions is the "hot spot". It is working strangely, the code is ugly, the logic is complex and there's a log of bugs.

The root of the problem is that this:

• Button actions are handled by x_button and button methods in each controller.
• Toolbars route button presses to the current controller.
• In case of nested lists where controller for A displays list of B the controler A needs to handle actions for B.
• This in turn is handled differently by different authors in different places:
  • we have redirects (like here)
  • we have code of button actions either placed into the ApplicationController (then it's available in all the controllers) or it's places in a mixin that is included everywhere needed.

I think that while these sort of "work" they are far from a "good".

How to fix this situation? There are variants:

• Don't make A display list of B. If we used the "normal" Rails nested routes instead of the custom nesting we have, then /A/B/list would be handled by controller for B, Then the B controller would include the actions relevant for B and all would be good. This unfortunately is a very long shot and it also does not fit best with the report-based lists that we have everywhere.
• Use javascript (Angular) based toolbar actions. Toolbar actions can be handled client-side. For example see app/helpers/application_helper/toolbar/generic_object_definition_center.rb or app/helpers/application_helper/toolbar/middleware_server_center.rb. Especially when there's already REST API available for an action, there might be no reason to write any Rails UI server code. On the client side we have the API token and javascript can fire the API action and display a flash message w/o extra Rails UI stuff being used. I think this should be the prefered way. Especially if a given screen is already converted to Angular.
• Route the button action to the appropriate controller. cloud_volume_delete button action could be handled directly by the cloud_volume controller. We can add a way for the buttons to specify what controller should be handling the particular action. That would remove the need for a redirect like this one. Shall we add this? Would it help?

@gberginc, @AparnaKarve: I welcome any input on this. As this PR shows and as I know, we have not yet refactored that to a satisfactory form and this is a problematic place right now .

[gberginc]

@martinpovolny I'd go with the second option.

While third one seems like an interesting choice, I am not sure if it will handle all cases properly; also, the functions like delete_volumes would still need to be changed to support being called from multiple places, i.e. the problem with @lastaction being set in CloudVolumeController would still persist.

The first one seems far to heavy for the time being, although it may be something to think about on the long run (probably there were a lot of thoughts already, though).

Question is should this be changed for the current BZ or should we resolve the issue ASAP and then come back to implement it the right way?

[martinpovolny]

Question is should this be changed for the current BZ or should we resolve the issue ASAP and then come back to implement it the right way?

No. Surely not. It's not stuff for this PR.

But I'd like to form some consensus for the future changes.

[AparnaKarve]

@gberginc @martinpovolny Yes, Option2 seems like a good choice.
Firing a REST API for Cloud Volume Delete action would result in a flash message that we would have to display.
That's one thing to keep in mind since we do not plan to execute any Rails code if we go that route.

[AparnaKarve]

@gberginc I have created ManageIQ/manageiq#15097 to add support for Cloud Volume Delete action via the API.

[martinpovolny]

Cool @AparnaKarve : please, keep pointing people to this approach and your API PR as an example! 👍

[simaishi]

@gberginc @dclarizio this can be fine/yes (as per the BZ)?

[simaishi]

Fine backport details:

$ git log -1
commit c947b6e3539d78f6030d9cc2588c5468d927dcd8
Author: Martin Povolny <mpovolny@redhat.com>
Date:   Tue May 16 21:06:57 2017 +0200

    Merge pull request #1331 from gberginc/fix/delete_cloud_volume_from_storage
    
    Redirect delete action to cloud volume controller
    (cherry picked from commit 132c945fbbf9336897c2736aad8e0c768d8e2ca3)
    
    https://bugzilla.redhat.com/show_bug.cgi?id=1459190