-
Notifications
You must be signed in to change notification settings - Fork 357
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Redirect delete action to cloud volume controller #1331
Redirect delete action to cloud volume controller #1331
Conversation
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 Signed-off-by: Gregor Berginc <gregor.berginc@gmail.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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.
When `delete_volumes` is called from a controller other than `CloudVolumesController` the redirect that is being made after the volume removal has been initiated could possibly be wrong. The problem is that the `lastaction` of the cloud volumes controller is only set by the controller itself. It could thus happen that the delete from `ems_storage` controller would invoke `delete_volumes` and the cloud volumes controller would still have lastaction set to the one that was used when the controller was last viewed. This patch ensures that the lastaction is cleared before the delete action is invoked, notifying the controller that it should redirect back to where it came from. Signed-off-by: Gregor Berginc <gregor.berginc@gmail.com>
Checked commits gberginc/manageiq-ui-classic@b4754ec~...bbdef76 with ruby 2.2.6, rubocop 0.47.1, and haml-lint 0.20.0 |
@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
|
app/controllers/ems_common.rb
Outdated
@@ -477,6 +477,9 @@ def button | |||
:action => "edit", | |||
:id => find_checked_ids_with_rbac(CloudVolume).first | |||
elsif params[:pressed] == "cloud_volume_delete" | |||
# Clear CloudVolumeController's lastaction, since we are calling the delete_volumes from | |||
# an external controller. This will ensure that the final redirect is properly handled. | |||
session["#{CloudVolumeController.session_key_prefix}_lastaction".to_sym] = nil |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
@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 Since 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. |
@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:
I think that while these sort of "work" they are far from a "good". How to fix this situation? There are variants:
@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 . |
@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 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? |
No. Surely not. It's not stuff for this PR. But I'd like to form some consensus for the future changes. |
@gberginc @martinpovolny Yes, Option2 seems like a good choice. |
@gberginc I have created ManageIQ/manageiq#15097 to add support for Cloud Volume Delete action via the API. |
Cool @AparnaKarve : please, keep pointing people to this approach and your API PR as an example! 👍 |
@gberginc @dclarizio this can be |
…torage Redirect delete action to cloud volume controller (cherry picked from commit 132c945) https://bugzilla.redhat.com/show_bug.cgi?id=1459190
Fine backport details:
|
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 aredeleted from the provider.
Fixes https://bugzilla.redhat.com/show_bug.cgi?id=1449293
@miq-bot add_label bug,storage
@miq-bot assign @AparnaKarve