Disable editing of NetworkProviders except Nuage #3394
Conversation
| @@ -1,5 +1,6 @@ | |||
| class ApplicationHelper::Button::EmsNetwork < ApplicationHelper::Button::Basic | |||
| def visible? | |||
| ::Settings.product.nuage | |||
| return ::Settings.product.nuage if @record.nil? # add new | |||
| @record.class == ManageIQ::Providers::Nuage::NetworkManager # edit existing | |||
There was a problem hiding this comment.
We want to avoid constructs like the above as much as possible and use the SupportsFeature mixin instead.
In this case, I'd suggest something like:
@record.supports?(:ems_network_new)There was a problem hiding this comment.
Turns out :ems_network_new is even already present on Nuage network manager so I could just literally replace the line with your suggestion and it works, thanks!
I think it's good enough if we use :ems_network_new even to determine if NetworkManager can be edited (not just created) because the two operations are usually paired. Please let me know if you'd prefer to introduce a separate :ems_network_edit , but note that changes in core repo will be needed then.
There was a problem hiding this comment.
I think what matters here is the semantics of :ems_network_new: if you can create a new network you can also edit existing network. So I agree that you don't need to introduce :ems_network_edit.
With this commit we hide option "Edit this Network Provider" for any other NetworkProvider than Nuage. Editing NetworkProviders was disabled prior integrating Nuage into MIQ when we accidently enabled it for all of them. Well not any more, we hide it back for other providers while keeping it for Nuage. Signed-off-by: Miha Pleško <miha.plesko@xlab.si>
miha-plesko
force-pushed
the
disable-edit-network-manager
branch
from
2e3b37f
to
0c59313
Compare
|
Checked commit miha-plesko@0c59313 with ruby 2.3.3, rubocop 0.52.0, haml-lint 0.20.0, and yamllint 1.10.0 |
There was a problem hiding this comment.
Thanks @mzazrivec I totally agree with you, fixed and repushed. Not sure why employing supports mixin didn't strike me in first place :D
| @@ -1,5 +1,6 @@ | |||
| class ApplicationHelper::Button::EmsNetwork < ApplicationHelper::Button::Basic | |||
| def visible? | |||
| ::Settings.product.nuage | |||
| return ::Settings.product.nuage if @record.nil? # add new | |||
| @record.class == ManageIQ::Providers::Nuage::NetworkManager # edit existing | |||
There was a problem hiding this comment.
Turns out :ems_network_new is even already present on Nuage network manager so I could just literally replace the line with your suggestion and it works, thanks!
I think it's good enough if we use :ems_network_new even to determine if NetworkManager can be edited (not just created) because the two operations are usually paired. Please let me know if you'd prefer to introduce a separate :ems_network_edit , but note that changes in core repo will be needed then.
| # therefore we hide drop-down option completely unless Nuage is enabled. | ||
| return ::Settings.product.nuage unless @record # add new | ||
|
|
||
| @record.supports?(:ems_network_new) # edit |
There was a problem hiding this comment.
@miha-plesko Quick question.
Does this work from the list view?
I think, it won't - because @record does not exist in the list view.
I need to do something similar in one other PR and was looking into options of disabling edit in the list view for a particular type of record.
(sadly, there is no easy way to do this)
There was a problem hiding this comment.
@AparnaKarve This doesn't work indeed on the list view. However, I remember we're using something similar on other location like this:
What we do there is we loop all selected items and check for each if it supports specific feature. If there is at least one item found that doesn't support the feature, we refuse performing action and fire up popup saying "Operation does not apply to at least one of the selected items" instead.
Something similar should probably also be implemented here. However, I must apologize, but I'm fully loaded and can't work on this ATM - can it wait a week or two?
There was a problem hiding this comment.
@miha-plesko That may work - although there would be a slight compromise in the UX, I think.
Meaning, you would have to first go through the Edit button click, and then it would be determined if the Edit operation is supported or not -- as opposed to the UI intelligently determining beforehand if the Edit button needs to be visible in the first place.
I was thinking an other easier option would be to simply disable/hide Edit in the list view, because all of the Network Providers, (with the exception of Nuage) do not support Edit
...and, display the Edit button for a Nuage Network Provider only in the summary screen.
There was a problem hiding this comment.
I agree, Edit button would best not appear on list view at all.
Disable editing of NetworkProviders except Nuage (cherry picked from commit fa757f0) https://bugzilla.redhat.com/show_bug.cgi?id=1552895
|
Gaprindashvili backport details: |
With this commit we hide option "Edit this Network Provider" for any other NetworkProvider than Nuage. Editing NetworkProviders was disabled prior integrating Nuage into MIQ when we accidently enabled it for all of them. Well not any more, we hide it back for other providers while keeping it for Nuage.
@AparnaKarve this is the followup PR as promised here.
@miq-bot assign @AparnaKarve
@miq-bot add_label enhancement,gaprindashvili/yes