-
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
Disable editing of NetworkProviders except Nuage #3394
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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>
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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