Skip to content
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

Allow editing and other actions on selected Host Aggregate #6267

Merged

Conversation

hstastna
Copy link
Contributor

@hstastna hstastna commented Oct 3, 2019

Issue: #6227


Various errors occurred after displaying Host Aggregates from Relationships table of a Cloud Provider and providing selected actions on a selected Host Aggregate. Only tagging and Download worked for selected Host Aggregates. The same problems occurred for actions of Host Aggregates displayed in a list directly thru Compute > Clouds > Host Aggregates and also actions provided from details page of a Host Aggregate. This PR fixes errors and allows us to provide selected actions.

Editing Host Aggregate:
The DoubleRenderError regarding editing a Host Aggregate was fixed by removing an extra unnecessary code here (because we already call javascript_redirect with same parameters here) and adding unless performed? here. But that was not enough, we also had to prevent calling unnecessary show method.

Adding Host to Host Aggregate:
This action did not work because of calling javascript_redirect here and show method here. After solving this problem, I also had to specify the name of the controller for redirecting and setting id properly, too, to make it work.

Removing Host from Host Aggregate:
Similarly as for adding Host to Host Aggregate.

Deleting Host Aggregate:
I've refactored delete_host_aggregates little bit, in host_aggregate controller. I've used checked_or_params here which works well, no matter if params[:pressed] or params[:miq_grid_checks] is set.
I've also had to solve calling show_list and displaying the list of Host Aggregates after deleting the selected ones. This is solved here.
For displaying flash message about deleting Host Aggregates, I had to call flash_to_session here. These changes work well for Host Aggregates displayed also in a nested list or also for individual Host Aggregates (while displaying details page of a Host Aggregate). I've also removed some unnecessary code as add_flash was already called earlier.
But how do we call delete_host_aggregates properly? I had to make another change to solve this (also with solving calling unnecessary show method here). And finally, I've added delete_host_aggregates to routes. The reasons for this change, are, hopefully, obvious.
Also note that, originally, delete_host_aggregates method was in the same file as the button method, which is now in ems_common.rb (see ManageIQ/manageiq#11980).
I've also refactored the specs little bit.


After:
edit_host_agg
add_host
remove_host
delete_agg

@hstastna
Copy link
Contributor Author

hstastna commented Oct 3, 2019

@miq-bot add_label bug

@hstastna hstastna force-pushed the Actions_Cloud_Providers_Host_Aggregate branch 14 times, most recently from e0962f3 to 5641b4c Compare October 3, 2019 15:08
@hstastna hstastna changed the title [WIP] Allow editing and other actions on selected Host Aggregate Allow editing and other actions on selected Host Aggregate Oct 3, 2019
@miq-bot miq-bot removed the wip label Oct 3, 2019
@hstastna hstastna force-pushed the Actions_Cloud_Providers_Host_Aggregate branch 3 times, most recently from 683af15 to 37185db Compare October 8, 2019 11:19
@hstastna hstastna force-pushed the Actions_Cloud_Providers_Host_Aggregate branch from 37185db to 631f2bf Compare October 10, 2019 12:10
@miq-bot
Copy link
Member

miq-bot commented Oct 10, 2019

Checked commits hstastna/manageiq-ui-classic@0af0514~...631f2bf with ruby 2.4.6, rubocop 0.69.0, haml-lint 0.20.0, and yamllint 1.10.0
5 files checked, 0 offenses detected
Everything looks fine. ⭐

@martinpovolny
Copy link

This looks good to me codewise.

Can you ask someone to test it in detail in the UI (I do not have much time until the next Monday).

@h-kataria, @ZitaNemeckova, @mzazrivec, any other volunteers?

@ZitaNemeckova
Copy link
Contributor

ZitaNemeckova commented Oct 17, 2019

  • Edit from summary - spinner disappears quickly

  • Edit from GTL

  • Delete from summary

  • Delete from GTL

  • Add from summary

  • Add from GTL - goes to summary after Add / goes to GTL after Cancel

  • Removing Host from Host Aggregate from summary

  • Removing Host from Host Aggregate from GTL - goes to summary after remove / goes to GTL after Cancel

I dont' have working Provider so all actions end with Error message Unable to update Host Aggregate "overcloud-compute-0": No credentials defined

@hstastna It looks really good 👍 I'm not sure about those two 👆 redirects to "wrong" page. Maybe it should be a followup PR. WDYT?

@hstastna
Copy link
Contributor Author

hstastna commented Oct 17, 2019

@ZitaNemeckova Thanks very moch for testing. That's another issue (what's going on after clicking on Cancel button vs. what's going on after success), and I've seen it also in other screens (other than Hosts or Host Aggregates). I think that it can be fixed via another PR. Maybe I should also open a separate issue for it. What do you think? :)

@ZitaNemeckova
Copy link
Contributor

@hstastna Sounds good to me :)

@martinpovolny Tested in UI. LGTM 👍

@mzazrivec mzazrivec self-assigned this Oct 18, 2019
@mzazrivec mzazrivec added this to the Sprint 123 Ending Oct 28, 2019 milestone Oct 18, 2019
@mzazrivec mzazrivec merged commit 7c4574e into ManageIQ:master Oct 18, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants