Prevent rendering User to Look Up form field twice and with same id

[hstastna]

Issue: #2316

This PR solves the specific situation in LDAP user lookup form: DOM ID "user" was there twice. This was caused by ManageIQ/manageiq#4042. The exactly same field with the same id, that was already there, was added in the haml, together with the javascript for showing/hiding the divs with the appropriate fields. This (not the best) solution was implemented for displaying/hiding Retrieve button next to the field.

Later, thanks to some cleanup in that area, this issue became obvious: the same field was rendered twice in the screen, all the divs were shown because the javascript was not executed - and this because @edit was nil - this all thanks to this change.

This PR cares about:

• removing unnecessary extra field from the appropriate haml
• adding a simple condition for displaying Retrieve button if mode == "httpd"
  Note that mode is already set here, no need to use the whole condition as here.
• adding another similar condition for displaying Username and Password fields: unless mode == "httpd" because of this
• removing the unnecessary javascript from the haml
• removing extra Retrieve button as, according to the existing logic in the haml, Retrieve button will always be there

Note:
Adding - if @edit to the haml (added in the haml during the later cleanup) is not necessary because it's already there in the right place.

[hstastna]

@miq-bot add_label bug

[hstastna]

@martinpovolny @himdel Could you, please, review this? Thanks :)

[himdel]

Before:

user_id_ext_auth_form_group - show when httpd
user_id_form_group - hide when httpd
user_name_form_group - hide when httpd
user_password_form_group - hide when httpd

Now:

user_id_ext_auth_form_group & user_id_form_group - merged, show the ext_auth variant when httpd
user_name_form_group, user_password_form_group - hide when httpd

We're losing the actual id (because the same form group can't have 2), but it's not used elsewhere 👍

(not tested yet)

[hstastna]

@himdel I am not sure if I should use #user_name_form_group or #user_id_ext_auth_form_group for the field (with or without Retrieve button) which remains in the haml. It looks like it does not matter (it was there only for showing/hiding the exact div). But maybe #user_name_form_group is more universal. And yes, better to test it.

Just to be clear, the div - 'the ext_auth variant' - as you've written, is now displayed no matter the authentication mode. The Retrieve button next to the field - this is what we really want to display if httpd (or not to display if no httpd). The field by itself should be there no matter the mode - I think this is the original logic and I tried to keep it, just to prevent the issues.

[himdel]

So, LGTM, this works.

But you can go one step further ;)...

the Retrieve button is present and identical in both the - if mode == "httpd" branch and the -unless mode == "httpd" branch.

So, what about...

diff --git a/app/views/ops/_rbac_group_details.html.haml b/app/views/ops/_rbac_group_details.html.haml
index 51fd8d9d52..c50a872f17 100644
--- a/app/views/ops/_rbac_group_details.html.haml
+++ b/app/views/ops/_rbac_group_details.html.haml
@@ -124,7 +124,7 @@
         %h3
           = _("LDAP Group Look Up")
         .form-horizontal
-          .form-group#user_id_ext_auth_form_group
+          .form-group#user_id_form_group
             %label.col-md-2.control-label
               = _("User to Look Up")
             .col-md-8
@@ -134,19 +134,6 @@
                                :class             => "form-control",
                                "data-miq_observe" => {:interval => '.5',
                                                       :url      => url}.to_json)
-            - if mode == "httpd"
-              .col-md-12{:align => "right", :valign => "bottom"}
-                = link_to("Retrieve",
-                          {:action => "rbac_group_user_lookup",
-                           :button => "submit",
-                           :id     => @edit[:group_id] || "new"},
-                          :class                 => "btn btn-primary",
-                          :alt                   => t = _("LDAP Group Lookup"),
-                          "data-miq_sparkle_on"  => true,
-                          "data-miq_sparkle_off" => true,
-                          :remote                => true,
-                          "data-method"          => :post,
-                          :title                 => t)
 
           - unless mode == "httpd"
             .form-group#user_name_form_group
@@ -170,18 +157,19 @@
                                      :class => "form-control",
                                      "data-miq_observe" => {:interval => '.5',
                                                             :url      => url}.to_json)
-              .col-md-12{:align => "right", :valign => "bottom"}
-                = link_to("Retrieve",
-                          {:action => "rbac_group_user_lookup",
-                           :button => "submit",
-                           :id     => @edit[:group_id] || "new"},
-                          :class                 => "btn btn-primary",
-                          :alt                   => t = _("LDAP Group Lookup"),
-                          "data-miq_sparkle_on"  => true,
-                          "data-miq_sparkle_off" => true,
-                          :remote                => true,
-                          "data-method"          => :post,
-                          :title                 => t)
+
+          .col-md-12{:align => "right", :valign => "bottom"}
+            = link_to("Retrieve",
+                      {:action => "rbac_group_user_lookup",
+                       :button => "submit",
+                       :id     => @edit[:group_id] || "new"},
+                      :class                 => "btn btn-primary",
+                      :alt                   => t = _("LDAP Group Lookup"),
+                      "data-miq_sparkle_on"  => true,
+                      "data-miq_sparkle_off" => true,
+                      :remote                => true,
+                      "data-method"          => :post,
+                      :title                 => t)
 
   %hr
   %h3

(on top of the current changes)

[miq-bot]

Checked commit hstastna@23a5a41 with ruby 2.5.5, rubocop 0.69.0, haml-lint 0.20.0, and yamllint 1.10.0
1 file checked, 0 offenses detected
Everything looks fine. 🏆

[hstastna]

@himdel I've made suggested changes regarding the Retrieve button :)

[himdel]

LGTM, merging :)

with mode database - no lookup:

with mode ldap or ldaps - 3 fields:

with mode httpd (and no saml or oidc):