[WIP] Removing File Depot Edit Page #8235
Open
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
MelsHyrule
force-pushed
the
file-depot-edit
branch
2 times, most recently
from
8c498ef to
0d2ae60
Compare
|
@miq-bot add-reviewer @Fryguy |
Fryguy
reviewed
Apr 22, 2022
Fryguy
reviewed
Apr 22, 2022
Fryguy
reviewed
Apr 22, 2022
| 'pficon pficon-edit fa-lg', | ||
| N_('Edit the Log Depot settings for the selected Server'), | ||
| N_('Edit'), | ||
| :klass => ApplicationHelper::Button::LogDepotEdit), |
There was a problem hiding this comment.
Looks like these can all go away now too?
$ git grep LogDepotEdit
app/helpers/application_helper/button/collect_logs.rb:1:class ApplicationHelper::Button::CollectLogs < ApplicationHelper::Button::LogDepotEdit
app/helpers/application_helper/button/log_depot_edit.rb:1:class ApplicationHelper::Button::LogDepotEdit < ApplicationHelper::Button::Basic
app/helpers/application_helper/button/zone_collect_logs.rb:1:class ApplicationHelper::Button::ZoneCollectLogs < ApplicationHelper::Button::LogDepotEdit
app/helpers/application_helper/toolbar/diagnostics_server_center.rb:87: :klass => ApplicationHelper::Button::LogDepotEdit),
app/helpers/application_helper/toolbar/diagnostics_zone_center.rb:95: :klass => ApplicationHelper::Button::LogDepotEdit),
spec/helpers/application_helper/buttons/log_depot_edit_spec.rb:1:describe ApplicationHelper::Button::LogDepotEdit do
$ git grep CollectLogs
app/helpers/application_helper/button/collect_logs.rb:1:class ApplicationHelper::Button::CollectLogs < ApplicationHelper::Button::LogDepotEdit
app/helpers/application_helper/button/zone_collect_logs.rb:1:class ApplicationHelper::Button::ZoneCollectLogs < ApplicationHelper::Button::LogDepotEdit
app/helpers/application_helper/toolbar/diagnostics_server_center.rb:71: :klass => ApplicationHelper::Button::CollectLogs
app/helpers/application_helper/toolbar/diagnostics_server_center.rb:78: :klass => ApplicationHelper::Button::CollectLogs
app/helpers/application_helper/toolbar/diagnostics_zone_center.rb:79: :klass => ApplicationHelper::Button::ZoneCollectLogs
app/helpers/application_helper/toolbar/diagnostics_zone_center.rb:86: :klass => ApplicationHelper::Button::ZoneCollectLogs
spec/helpers/application_helper/buttons/collect_logs_spec.rb:1:describe ApplicationHelper::Button::CollectLogs do
spec/helpers/application_helper/buttons/zone_collect_logs_spec.rb:1:describe ApplicationHelper::Button::ZoneCollectLogs do
There was a problem hiding this comment.
Yup yup! I'm already on it
Fryguy
reviewed
Apr 22, 2022
| @@ -1,15 +0,0 @@ | |||
| class ApplicationHelper::Button::ZoneCollectLogs < ApplicationHelper::Button::LogDepotEdit | |||
| include ApplicationHelper::Button::Mixins::ButtonPromptMixin | |||
There was a problem hiding this comment.
Interestingly, from what I can tell, ButtonPromptMixin is only used by ZoneCollectLogs and CollectLogs, so this mixin can also be deleted 🗑️
Fryguy
reviewed
Apr 22, 2022
| ManageIQ.angular.app.component('logCollectionForm', { | ||
| controllerAs: 'vm', | ||
| controller: logCollectionFormController, | ||
| templateUrl: '/static/ops/log_collection/log_collection.html.haml', |
There was a problem hiding this comment.
Not sure where this template comes from, but I think it can also be deleted.
Fryguy
reviewed
Apr 22, 2022
| @@ -125,8 +123,6 @@ | |||
| = render :partial => "diagnostics_servers_roles_tab" | |||
| = miq_tab_content("diagnostics_server_list", @sb[:active_tab]) do | |||
| = render :partial => "diagnostics_server_list_tab" | |||
| = miq_tab_content("diagnostics_collect_logs", @sb[:active_tab]) do | |||
| = render :partial => "diagnostics_collect_logs_tab" | |||
There was a problem hiding this comment.
With this partial removed here and below you can also delete app/views/ops/_diagnostics_collect_logs_tab.html.haml
Fryguy
reviewed
Apr 22, 2022
| @@ -67,7 +67,6 @@ def display_ae_namespaces | |||
| 'fetch_audit_log' => :fetch_audit_log, | |||
| 'fetch_log' => :fetch_log, | |||
| 'fetch_production_log' => :fetch_production_log, | |||
| 'log_depot_edit' => :log_depot_edit, | |||
There was a problem hiding this comment.
Once this goes, I think all (most?) of the following can go too:
$ git grep log_depot_edit
app/controllers/ops_controller.rb:70: 'log_depot_edit' => :log_depot_edit,
app/controllers/ops_controller.rb:111: 'zone_log_depot_edit' => :log_depot_edit,
app/controllers/ops_controller.rb:474: action_url = "log_depot_edit"
app/controllers/ops_controller.rb:628: elsif nodetype == "log_depot_edit"
app/controllers/ops_controller.rb:794: if ["log_depot_edit", "ze"].include?(nodetype)
app/controllers/ops_controller/diagnostics.rb:77: def log_depot_edit
app/controllers/ops_controller/diagnostics.rb:78: assert_privileges("#{@sb[:selected_typ] == "miq_server" ? "" : "zone_"}log_depot_edit")
app/controllers/ops_controller/diagnostics.rb:147: replace_right_cell(:nodetype => "log_depot_edit")
app/controllers/ops_controller/diagnostics.rb:280: assert_privileges("#{@sb[:selected_typ] == "miq_server" ? "" : "zone_"}log_depot_edit")
app/helpers/application_helper/toolbar/diagnostics_server_center.rb:83: :log_depot_edit,
app/helpers/application_helper/toolbar/diagnostics_zone_center.rb:91: :zone_log_depot_edit,
app/views/layouts/angular-bootstrap/_auth_credentials_angular_bootstrap.html.haml:2:- validate_url ||= 'log_depot_edit'
app/views/layouts/angular/_auth_service_account_angular.html.haml:2:- validate_url ||= 'log_depot_edit'
app/views/ops/_log_collection.html.haml:5: 'save-url' => "/#{controller_name}/log_depot_edit/",
config/routes.rb:2400: log_depot_edit
spec/config/routes.pending.yml:887:- log_depot_edit
spec/controllers/ops_controller/diagnostics_spec.rb:322: context "#log_depot_edit" do
spec/controllers/ops_controller/diagnostics_spec.rb:342: controller.send(:log_depot_edit)
spec/controllers/ops_controller_spec.rb:294: post :x_button, :params => {:id => @miq_server.id, :pressed => 'log_depot_edit', :format => :js}
spec/controllers/ops_controller_spec.rb:301: post :x_button, :params => {:id => @miq_server.id, :pressed => 'log_depot_edit', :button => "cancel", :format => :js}
spec/controllers/ops_controller_spec.rb:308: post :x_button, :params => {:id => @miq_server.id, :pressed => 'log_depot_edit', :button => "save", :format => :js}
spec/javascripts/components/ops/log_collection_form_spec.js:20: saveUrl: "/ops/log_depot_edit/",
spec/javascripts/components/ops/log_collection_form_spec.js:68: expect(miqService.miqAjaxButton).toHaveBeenCalledWith("/ops/log_depot_edit/123456?button=cancel", true);
spec/javascripts/components/ops/log_collection_form_spec.js:78: var url = "/ops/log_depot_edit/123456?button=save";
spec/routing/ops_routing_spec.rb:53: log_depot_edit
There was a problem hiding this comment.
yeah i've already removed most of those on my local just testing that nothings broken will push once done
jrafanie
reviewed
Apr 22, 2022
app/controllers/ops_controller.rb
Outdated
| @@ -570,7 +565,7 @@ def get_node_info(treenodeid, show_list = true) | |||
| region_text = _("[Region: %{description} [%{region}]]") % {:description => MiqRegion.my_region.description, | |||
| :region => MiqRegion.my_region.region} | |||
| @right_cell_text ||= case x_active_tree | |||
| when :diagnostics_tree then _("Diagnostics %{text}") % {:text => region_text} | |||
| when :diagnostics_tree then _("Diagnostics %{text} 121212") % {:text => region_text} ## this is the header for the diagonists page (that is now empty) | |||
There was a problem hiding this comment.
whoop true true, although this comment isnt meant to stay here forever, its more a reminder so i can ask about what we want to do about this particular page
since now its basically an empty page, is there any note or comment or label we want to add here or do we want to simply leave it as is?
There was a problem hiding this comment.
If there's nothing to show, then we should remove the page.
There was a problem hiding this comment.
Are there even any Zone diagnostics after this?
There was a problem hiding this comment.
If you mean "is there anything left inside the zone diagnostics tab after this?" then the answer is yes
There was a problem hiding this comment.
Interesting - maybe not for this PR, but then I wonder if it's useful to even have the servers in the left side nav anymore, because you can get that same information in the servers tab there. Then again, it's nice to be able to find a particular server if you don't know what zone it's in.
There was a problem hiding this comment.
Update for here: Just pushed up a change to finish emptying out the page (see this screenshot) however i haven't been able to remove the 'clickyness' of the page and i don't think theres a way to specifically remove it since that functionality is tied to this method https://github.com/ManageIQ/manageiq-ui-classic/blob/master/app/javascript/oldjs/controllers/tree_view_controller.js#L101-L105 which looks to be pretty generic so i dont think we want to be adding diagnostics tree specific code there
29 tasks
|
I checked out your branch locally and you might be missing more opportunities for deletions. Keep in mind, I think we need to delete only the LogFile / log collection stuff and not the FileDepot Yet. I cobbled together this grep command to look for things that might need to be removed: I can't tell if all of these are log collection/ log file references but I think many are. |
|
Note, here's my understanding of the need for FileDepot for PxeServer class... ManageIQ/manageiq#21377 (comment) |
4 tasks
MelsHyrule
force-pushed
the
file-depot-edit
branch
from
2cc474c to
fbf201a
Compare
|
Looking better, note, I updated my git grep for the backend PR so there's still references to Please review to see if these are also part of the log collection UI and can be removed: |
|
Looks like the tests are failing due to a missing Unless these tests are validating non-depot authentication, maybe the callers can be removed too. I don't know if diff --git a/app/views/layouts/angular-bootstrap/_auth_credentials_angular_bootstrap.html.haml b/app/views/layouts/angular-bootstrap/_auth_credentials_angular_bootstrap.html.haml
index 2b8711037d..4305255b7a 100644
--- a/app/views/layouts/angular-bootstrap/_auth_credentials_angular_bootstrap.html.haml
+++ b/app/views/layouts/angular-bootstrap/_auth_credentials_angular_bootstrap.html.haml
@@ -1,5 +1,4 @@
- ng_show ||= true
-- validate_url ||= 'log_depot_edit'
- prefix ||= 'log'
- userid_label ||= _("Username")
- password_label ||= _("Password")
@@ -82,7 +81,6 @@
.col-md-4
= render :partial => "layouts/angular/form_buttons_verify_angular",
:locals => {:ng_show => "#{ng_show}",
- :validate_url => validate_url,
diff --git a/app/views/layouts/angular/_auth_service_account_angular.html.haml b/app/views/layouts/angular/_auth_service_account_angular.html.haml
index 41e3dde7ff..ce0faddebf 100644
--- a/app/views/layouts/angular/_auth_service_account_angular.html.haml
+++ b/app/views/layouts/angular/_auth_service_account_angular.html.haml
@@ -1,5 +1,4 @@
- ng_show ||= true
-- validate_url ||= 'log_depot_edit'
- prefix ||= 'log'
- vm_scope ||= false
- main_scope = vm_scope ? "$parent.vm" : "$parent"
@@ -26,7 +25,6 @@
.col-md-4
= render :partial => "layouts/angular/form_buttons_verify_angular",
:locals => {:ng_show => ng_show,
- :validate_url => validate_url,
:id => id,
:valtype => prefix,
:ng_model => "#{ng_model}",Or maybe we need to delete more things, such as the list of log_depot items I listed above... |
MelsHyrule
force-pushed
the
file-depot-edit
branch
2 times, most recently
from
2b6b56d to
94b91f0
Compare
MelsHyrule
force-pushed
the
file-depot-edit
branch
from
9dbc980 to
3de553c
Compare
|
Checked commits MelsHyrule/manageiq-ui-classic@0755c98~...3de553c with ruby 2.6.9, rubocop 1.19.1, haml-lint 0.35.0, and yamllint app/controllers/ops_controller.rb
|
|
This pull request is not mergeable. Please rebase and repush. |
|
This pull request has been automatically closed because it has not been updated for at least 3 months. Feel free to reopen this pull request if these changes are still valid. Thank you for all your contributions! More information about the ManageIQ triage process can be found in the triage process documentation. |
|
Reopening however this needs a rebase, or perhaps needs to be reopened. |
|
This pull request is not mergeable. Please rebase and repush. |
|
This pull request has been automatically closed because it has not been updated for at least 3 months. Feel free to reopen this pull request if these changes are still valid. Thank you for all your contributions! More information about the ManageIQ triage process can be found in the triage process documentation. |
|
This pull request is not mergeable. Please rebase and repush. |
|
This pull request has been automatically marked as stale because it has not been updated for at least 3 months. If these changes are still valid, please remove the |
3 similar comments
|
This pull request has been automatically marked as stale because it has not been updated for at least 3 months. If these changes are still valid, please remove the |
|
This pull request has been automatically marked as stale because it has not been updated for at least 3 months. If these changes are still valid, please remove the |
|
This pull request has been automatically marked as stale because it has not been updated for at least 3 months. If these changes are still valid, please remove the |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Fixes 8159.
Before
After
Collect Logs tab was removed
Collect Logs tab was removed
Edit Button was removed
Because the
Edit Buttonwas removedEditpage was also removed.