On 01/28/2016 07:28 PM, Petr Vobornik wrote:
Hi, there are few issues, NACK.

1. this patch uses tabs instead of spaces, previous was correct
Fixed.

2. code which focuses first invalid field could be replaced by:
  widget_mod.focus_invalid(that);
The old loop is replaced by this method.

3. there is a convention that field and widget names uses the same name as the param, therefore 'textarea_cert' should be 'csr'. There is no convention for messages in html widget but it might be better to use a name reflecting purpose and not implementation. Instead of 'message_html_widget' use 'instructions' or just 'message' - consistent with spec option.

Fixed. I chose 'message' instead of 'message_html_widget'.
Also I've filed: https://fedorahosted.org/freeipa/ticket/5652
Pavel Vomacka

>From 242a52390de84025efab2fb878aa95d46c1386a4 Mon Sep 17 00:00:00 2001
From: Pavel Vomacka <pvoma...@redhat.com>
Date: Thu, 28 Jan 2016 17:37:29 +0100
Subject: [PATCH] Add validation to Issue new certificate dialog

'Issue new certificate' dialog now validates whether user fills 'principal' and 'csr' field.
In case that one of these fields is empty then it does not allow to submit the dialog.

https://fedorahosted.org/freeipa/ticket/5432
---
 install/ui/src/freeipa/certificate.js | 60 +++++++++++++++++++++++------------
 install/ui/src/freeipa/details.js     |  5 +++
 2 files changed, 45 insertions(+), 20 deletions(-)

diff --git a/install/ui/src/freeipa/certificate.js b/install/ui/src/freeipa/certificate.js
index 93f3cfc68a95bfb8014aaf96d1b571568ac605dc..aaae4d9fb14e0c6e8dd1562b92b0ddb3ff1a6a49 100755
--- a/install/ui/src/freeipa/certificate.js
+++ b/install/ui/src/freeipa/certificate.js
@@ -30,10 +30,11 @@ define([
     './reg',
     './rpc',
     './text',
+    './widget',
     './dialog'],
     function(
         lang, builder, metadata_provider, IPA, $, menu,
-        phases, reg, rpc, text) {
+        phases, reg, rpc, text, widget_mod) {
 
 var exp = IPA.cert = {};
 
@@ -399,11 +400,35 @@ IPA.cert.request_dialog = function(spec) {
     spec = spec || {};
 
     spec.sections = spec.sections || [];
-    var section = { fields: [] };
-    spec.sections.push(section);
+    var section0 = { fields: [] };
+    var section_csr = {
+        show_header: false,
+        fields: [
+            {
+                field: false,
+                $type: 'html',
+                name: 'message',
+                html: spec.message
+            },
+            {
+                $type: 'textarea',
+                name: 'csr',
+                required: true
+            }
+        ],
+        layout:
+        {
+            $factory: widget_mod.fluid_layout,
+            widget_cls: "col-sm-12 controls",
+            label_cls: "hide"
+        }
+    };
+
+    spec.sections.push(section0);
+    spec.sections.push(section_csr);
 
     if (spec.show_principal) {
-        section.fields.push(
+        section0.fields.push(
             {
                 $type: 'text',
                 name: 'principal',
@@ -418,7 +443,7 @@ IPA.cert.request_dialog = function(spec) {
             }
         );
     }
-    section.fields.push(
+    section0.fields.push(
         {
             $type: 'entity_select',
             name: 'profile_id',
@@ -443,7 +468,15 @@ IPA.cert.request_dialog = function(spec) {
         click: function() {
             var values = {};
             that.save(values);
-            var request = $.trim(that.textarea.val());
+
+            // check requested fields
+            if (!that.validate()) {
+                widget_mod.focus_invalid(that);
+                return;
+            }
+
+            // get csr from the textarea
+            var request = $.trim(that.get_field('csr').get_value());
             values.request = IPA.cert.pem_csr_format(request);
 
             if (that.request) {
@@ -461,19 +494,6 @@ IPA.cert.request_dialog = function(spec) {
         }
     });
 
-    that.create_content = function() {
-        that.dialog_create_content();
-        var node = $("<div/>", {
-            'class': 'col-sm-12'
-        });
-        node.append(that.message);
-        that.textarea = $('<textarea/>', {
-            'class': 'certificate'
-        }).appendTo(node);
-        that.body_node.append(node);
-        return that.body_node;
-    };
-
     return that;
 };
 
@@ -1519,4 +1539,4 @@ phases.on('post-metadata', exp.create_cert_metadata);
 phases.on('profile', exp.remove_menu_item, 20);
 
 return exp;
-});
\ No newline at end of file
+});
diff --git a/install/ui/src/freeipa/details.js b/install/ui/src/freeipa/details.js
index c708a878b7d6d29815535b7508da0c4cc30a3b5c..36dbb99fca3440b29ba280283c6c9e098f67d6d6 100644
--- a/install/ui/src/freeipa/details.js
+++ b/install/ui/src/freeipa/details.js
@@ -295,6 +295,11 @@ exp.section_builder = IPA.section_builder = function(spec) {
 
         var widget = that.widget_builder.build_widget(field_spec, section.widgets);
 
+        if (field_spec.field === false) {
+            // widget doesn't have field, skip
+            return;
+        }
+
         if (section.$field_adapter && !field_spec.adapter) {
             field_spec.adapter = section.$field_adapter;
         }
-- 
2.5.0

-- 
Manage your subscription for the Freeipa-devel mailing list:
https://www.redhat.com/mailman/listinfo/freeipa-devel
Contribute to FreeIPA: http://www.freeipa.org/page/Contribute/Code

Reply via email to