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

Restore the Delete button for servers in a zone #2213

Merged

Conversation

lgalis
Copy link
Contributor

@lgalis lgalis commented Sep 21, 2017

Restore the Delete button for servers in a zone, selected from the right-side "Roles By Servers" tab

Links

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


Before:
screenshot from 2018-02-07 21-58-06

After:
screenshot from 2018-03-14 15-04-08

@miq-bot miq-bot added the wip label Sep 21, 2017
@lgalis lgalis force-pushed the restore_delete_button_for_servers_in_zone branch 2 times, most recently from fd8da67 to a24c251 Compare September 21, 2017 21:17
@lgalis lgalis force-pushed the restore_delete_button_for_servers_in_zone branch 6 times, most recently from 24c5756 to d724594 Compare February 7, 2018 22:20
@lgalis
Copy link
Contributor Author

lgalis commented Feb 8, 2018

@miq-bot add_label bug, ui, gaprindashvili/yes

@lgalis lgalis changed the title [WIP] Restore the Delete button for servers in a zone Restore the Delete button for servers in a zone Feb 8, 2018
@miq-bot
Copy link
Member

miq-bot commented Feb 8, 2018

@lgalis Cannot apply the following label because they are not recognized: ui

@skateman
Copy link
Member

@lgalis are you sure this is not breaking anything? Just by analyzing the code, I found some places where the @selected_zone is being used:

ag "@selected_zone" app/
app/views/ops/_settings_evm_servers_tab.html.haml
11:            = @selected_zone.name
17:            = @selected_zone.description
23:            = @selected_zone.settings.key?(:proxy_server_ip) ? @selected_zone.settings[:proxy_server_ip] : ''
29:            = @selected_zone.settings[:ntp] ? @selected_zone.settings[:ntp][:server].join(", ") : ''
36:            = (@selected_zone.settings.key?(:concurrent_vm_scans) && @selected_zone.settings[:concurrent_vm_scans] > 0) ? @selected_zone.settings[:concurrent_vm_scans] : _("Unlimited")
45:            = @selected_zone.miq_servers.count
51:            = @selected_zone.ext_management_systems.count
57:            = @selected_zone.miq_schedules.count

app/views/ops/_smartproxy_affinity.html.haml
2:  = _("Assign Hosts and Datastores to Embedded SmartProxies in Zone: \"%{zone_description}\"") % {:zone_description => @selected_zone.description}
7:      = _("No Servers with the SmartProxy role are in Zone: \"%{zone_description}\"") % {:zone_description => @selected_zone.description}

app/controllers/ops_controller/settings/common.rb
624:    @edit[:key] = "#{@sb[:active_tab]}_edit__#{@selected_zone.id}"
625:    @sb[:selected_zone_id] = @selected_zone.id
628:    children[:hosts] = @selected_zone.hosts.collect(&:id)
629:    children[:storages] = @selected_zone.storages.collect(&:id)
631:    @selected_zone.miq_servers.each do |server|
639:    if @selected_zone.miq_servers.select(&:is_a_proxy?).present?
644:                                                                    @selected_zone)
911:      @right_cell_text = my_zone_name == @selected_zone.name ?
912:        _("Settings %{model} \"%{name}\" (current)") % {:name  => @selected_zone.description,
913:                                                        :model => ui_lookup(:model => @selected_zone.class.to_s)} :
914:        _("Settings %{model} \"%{name}\"") % {:name  => @selected_zone.description,
915:                                              :model => ui_lookup(:model => @selected_zone.class.to_s)}
1210:      @record = @zone = @selected_zone = Zone.find(nodes.last)
1211:      @right_cell_text = my_zone_name == @selected_zone.name ?
1212:          _("Settings %{model} \"%{name}\" (current)") % {:name  => @selected_zone.description,
1213:                                                          :model => ui_lookup(:model => @selected_zone.class.to_s)} :
1214:          _("Settings %{model} \"%{name}\"") % {:name  => @selected_zone.description,
1215:                                                :model => ui_lookup(:model => @selected_zone.class.to_s)}
1217:        if ms.zone_id == @selected_zone.id

app/helpers/application_helper.rb
561:      :selected_zone    => @selected_zone,

app/helpers/application_helper/button/zone_delete.rb
2:  needs :@selected_zone
5:    @error_message = if @selected_zone.name.downcase == 'default'
16:    @selected_zone.ext_management_systems.count > 0 ||
17:      @selected_zone.miq_schedules.count > 0 ||
18:      @selected_zone.miq_servers.count > 0

@lgalis
Copy link
Contributor Author

lgalis commented Feb 12, 2018

@skateman - The setting of the @selected_zone in the diganostics.rb that is removed in this PR was added in #829, where the Configuration menu button was incorrectly changed to include Delete zone instead of Delete Server. This PR reverses that change.
The @selected_zone for other areas of ops is set here : https://github.com/lgalis/manageiq-ui-classic/blob/9700419ab6fe87939b884aeb0dfee46f1a8d4123/app/controllers/ops_controller/settings/common.rb#L1210
I could not find a place where the @selected_zone in Diagnostics is used.

(Note - the diagnostics.rb uses the @selected_server for the current record for zone, server and region depending on the treenode)

@lgalis lgalis force-pushed the restore_delete_button_for_servers_in_zone branch from 30f478d to 0c16c60 Compare February 13, 2018 22:09
@lgalis
Copy link
Contributor Author

lgalis commented Feb 20, 2018

@h-kataria - please review

@lgalis lgalis closed this Mar 3, 2018
@lgalis lgalis reopened this Mar 3, 2018
@h-kataria
Copy link
Contributor

@lgalis when i click on a Zone node in tree on left, i see Configuration button group and Delete Server button after applying this fix but the server node on the right is not displayed as selected, i think that should be highlighted as well.

@h-kataria h-kataria self-assigned this Mar 8, 2018
@lgalis lgalis force-pushed the restore_delete_button_for_servers_in_zone branch 2 times, most recently from c3f79ae to 9e7e5c7 Compare March 14, 2018 18:32
@lgalis lgalis requested a review from martinpovolny as a code owner March 14, 2018 18:32
@lgalis lgalis force-pushed the restore_delete_button_for_servers_in_zone branch 2 times, most recently from 3471132 to 513fff8 Compare March 14, 2018 18:48
@lgalis lgalis force-pushed the restore_delete_button_for_servers_in_zone branch from 513fff8 to 4aa683d Compare March 14, 2018 19:55
@lgalis lgalis force-pushed the restore_delete_button_for_servers_in_zone branch from 4aa683d to 9592445 Compare March 14, 2018 20:22
@miq-bot
Copy link
Member

miq-bot commented Mar 14, 2018

Checked commits lgalis/manageiq-ui-classic@aa12252~...9592445 with ruby 2.3.3, rubocop 0.52.1, haml-lint 0.20.0, and yamllint 1.10.0
6 files checked, 0 offenses detected
Everything looks fine. ⭐

@lgalis
Copy link
Contributor Author

lgalis commented Mar 14, 2018

@h-kataria - added code to highlight the selected server in teh roles by server tree, in addition to the existing display of the currently selected server in the 'Selected Item' box.
Please re-review.

@martinpovolny martinpovolny requested review from skateman and removed request for martinpovolny March 15, 2018 08:16
@martinpovolny
Copy link
Member

@lgalis : I'll leave this one to @h-kataria and @skateman. Thx!

@h-kataria
Copy link
Contributor

LGTM, verified changes in UI.

@h-kataria h-kataria added this to the Sprint 82 Ending Mar 26, 2018 milestone Mar 16, 2018
@h-kataria h-kataria merged commit 70e9b64 into ManageIQ:master Mar 16, 2018
@lgalis lgalis deleted the restore_delete_button_for_servers_in_zone branch March 16, 2018 19:11
simaishi pushed a commit that referenced this pull request Mar 19, 2018
…s_in_zone

Restore the Delete button for servers in a zone
(cherry picked from commit 70e9b64)

https://bugzilla.redhat.com/show_bug.cgi?id=1558092
@simaishi
Copy link
Contributor

Gaprindashvili backport details:

$ git log -1
commit 7d279087cfeff7ec2db8d668c86d4d377fff9999
Author: Harpreet Kataria <[email protected]>
Date:   Fri Mar 16 14:58:04 2018 -0400

    Merge pull request #2213 from lgalis/restore_delete_button_for_servers_in_zone
    
    Restore the Delete button for servers in a zone
    (cherry picked from commit 70e9b64659a7b321d878ca1222ae8afe2b0db2a7)
    
    https://bugzilla.redhat.com/show_bug.cgi?id=1558092

@carbonin
Copy link
Member

@miq-bot add_label fine/yes

@simaishi
Copy link
Contributor

@lgalis Can you please confirm this and related PRs (#3650 and #3652) can to go Fine branch as is?

@lgalis
Copy link
Contributor Author

lgalis commented Apr 12, 2018

@simaishi - I will create a separate PR for FINE

@simaishi
Copy link
Contributor

@lgalis Thank you. I'll mark all 3 PRs as conflict for now.

@simaishi
Copy link
Contributor

Backported to Fine via #3763

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.

7 participants