-
Notifications
You must be signed in to change notification settings - Fork 983
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
Fixes #12843 - lookup values are hidden when creating them if lookup key is hidden #2981
Conversation
@@ -40,7 +40,8 @@ def use_puppet_default_help link_title = nil, title = _("Use Puppet default") | |||
end | |||
|
|||
def hidden_value_field(f, field, value, disabled, options = {}) | |||
if f.object.hidden_value? | |||
hidden = options.delete(:hidden_value) | |||
if f.object.hidden_value? || hidden |
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 would switch the order of this ||
in case the option is passed explicitly to save checking the form object.
@tbrisker I fixed all of your comments |
@@ -40,7 +40,8 @@ def use_puppet_default_help link_title = nil, title = _("Use Puppet default") | |||
end | |||
|
|||
def hidden_value_field(f, field, value, disabled, options = {}) | |||
if f.object.hidden_value? | |||
hidden = options.delete(:hidden_value) | |||
if hidden || f.object.hidden_value? |
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.
if hidden==false
, this will still check f.object.hidden_value?
.
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.
f.object.hidden_value?
will have the same value as hidden
or it will be nil so if hidden
is false the whole condition will be false anyway so that's fine.
@tbrisker I fixed one comment and answered the rest. |
👍 LGTM. |
@orrabin, this pull request is currently not mergeable. Please rebase against the develop branch and push again. If you have a remote called 'upstream' that points to this repository, you can do this by running:
This message was auto-generated by Foreman's prprocessor |
[test] |
@dLobatog can you take a look? I fixed all the comments and the test failures are unrelated. |
@orrabin Thanks. It fixes the behavior the bug describes, but now unticking If I try to hide a smart class parameter, still all fields are shown on the clear and cannot be hidden until I save it: |
@dLobatog I fixed it and rebased. |
[test] |
Test failure is the usual integration test, not related with this. |
No description provided.