Re: [Freeipa-devel] [PATCH] 212 Fix integer validation when boundary value is empty string

2012-09-18 Thread Petr Vobornik 

On 09/18/2012 04:45 AM, Endi Sukma Dewata wrote:

On 9/11/2012 10:09 AM, Petr Vobornik wrote:

There was an error in number validation check. If boundary value was an
empty string, validation of a number always failed. This patch fixes the
problem by not performing the check in these cases.

Basic unit tests for IPA.metadata_validator created.


ACK. Some comments:


Updated patch attached.



1. Instead of IPA.not_defined() it might be better to call it
IPA.defined() to avoid double negations like this:

   if (!IPA.not_defined(metadata.minvalue, true) ...



Function renamed, logic negated. Unit tests for IPA.defined added.


2. The check_empty_str probably could made optional and the value would
be true by default. It will make the code cleaner.



I wanted the function to be more general - used somewhere else in a 
future. In general, I consider empty string as a defined value so for me 
'false' is a good default value of the argument. I agree that in this 
case it would look cleaner.

--
Petr Vobornik
From e094a7d317a57b841c5454de635df6a01944d100 Mon Sep 17 00:00:00 2001
From: Petr Vobornik pvobo...@redhat.com
Date: Tue, 11 Sep 2012 13:52:10 +0200
Subject: [PATCH] Fix integer validation when boundary value is empty string

There was an error in number validation check. If boundary value was an empty string, validation of a number always failed. This patch fixes the problem by not performing the check in these cases.
---
 install/ui/field.js  |   4 +-
 install/ui/ipa.js|   5 ++
 install/ui/test/all_tests.html   |   1 +
 install/ui/test/index.html   |   1 +
 install/ui/test/jsl.conf |   3 +-
 install/ui/test/utils_tests.html |  24 +++
 install/ui/test/utils_tests.js   | 136 +++
 7 files changed, 171 insertions(+), 3 deletions(-)
 create mode 100644 install/ui/test/utils_tests.html
 create mode 100644 install/ui/test/utils_tests.js

diff --git a/install/ui/field.js b/install/ui/field.js
index 42da6f92cd102aa665b05b72783de9d04dcdcfb0..c5c999e685500765f09af084531def144bbbd10b 100644
--- a/install/ui/field.js
+++ b/install/ui/field.js
@@ -448,13 +448,13 @@ IPA.metadata_validator = function(spec) {
 
 if (number) {
 
-if (metadata.minvalue !== undefined  Number(value)  Number(metadata.minvalue)) {
+if (IPA.defined(metadata.minvalue, true)  Number(value)  Number(metadata.minvalue)) {
 message = IPA.messages.widget.validation.min_value;
 message = message.replace('${value}', metadata.minvalue);
 return that.false_result(message);
 }
 
-if (metadata.maxvalue !== undefined  Number(value)  Number(metadata.maxvalue)) {
+if (IPA.defined(metadata.maxvalue, true)  Number(value)  Number(metadata.maxvalue)) {
 message = IPA.messages.widget.validation.max_value;
 message = message.replace('${value}', metadata.maxvalue);
 return that.false_result(message);
diff --git a/install/ui/ipa.js b/install/ui/ipa.js
index 23c9933dfb97cb39a932f78d235fdaf844e42b7c..9b5c9aacf87804372cac369e44f7f392e16f3a25 100644
--- a/install/ui/ipa.js
+++ b/install/ui/ipa.js
@@ -2021,6 +2021,11 @@ IPA.is_empty = function(value) {
 return empty;
 };
 
+IPA.defined = function(value, check_empty_str) {
+return value !== null  value !== undefined 
+((check_empty_str  value !== '') || !check_empty_str);
+};
+
 IPA.array_diff = function(a, b) {
 
 if (a === b || (!a  !b)) return false;
diff --git a/install/ui/test/all_tests.html b/install/ui/test/all_tests.html
index 5a5eae54b5c6606564fd0e7b2df8ccc606dccef2..e9061b6ca8802b83c8f99c234ad7311606672695 100644
--- a/install/ui/test/all_tests.html
+++ b/install/ui/test/all_tests.html
@@ -32,6 +32,7 @@
 script type=text/javascript src=aci_tests.js/script
 script type=text/javascript src=widget_tests.js/script
 script type=text/javascript src=ip_tests.js/script
+script type=text/javascript src=utils_tests.js/script
 /head
 body
 h1 id=qunit-headerComplete Test Suite/h1
diff --git a/install/ui/test/index.html b/install/ui/test/index.html
index 1b623d40f794b1ffda90f6c3352840acbaeec26a..0a135188e518913aa3f3a191ab340f2f5e820abf 100644
--- a/install/ui/test/index.html
+++ b/install/ui/test/index.html
@@ -34,6 +34,7 @@
 lia href=aci_tests.htmlAccess Control Interface Test Suite/a
 lia href=widget_tests.htmlWidget Test Suite/a
 lia href=ip_tests.htmlIP Addresses Test Suite/a
+lia href=utils_tests.htmlUtils Test Suite/a
 /ul
 /div
 
diff --git a/install/ui/test/jsl.conf b/install/ui/test/jsl.conf
index 4654b714f91cb00696043bf3042fccd2b2e394bc..768a295f1ac7c956f588b99c60eecdb2bfb06c67 100644
--- a/install/ui/test/jsl.conf
+++ b/install/ui/test/jsl.conf
@@ -146,4 +146,5 @@
 +process ipa_tests.js
 +process ordered_map_tests.js
 +process widget_tests.js
-+process ip_tests.js
\ No newline at end of

Re: [Freeipa-devel] [PATCH] 212 Fix integer validation when boundary value is empty string

2012-09-18 Thread Endi Sukma Dewata 

On 9/18/2012 6:36 AM, Petr Vobornik wrote:

Updated patch attached.


ACK.


1. Instead of IPA.not_defined() it might be better to call it
IPA.defined() to avoid double negations like this:

   if (!IPA.not_defined(metadata.minvalue, true) ...


Function renamed, logic negated. Unit tests for IPA.defined added.


2. The check_empty_str probably could made optional and the value would
be true by default. It will make the code cleaner.


I wanted the function to be more general - used somewhere else in a
future. In general, I consider empty string as a defined value so for me
'false' is a good default value of the argument. I agree that in this
case it would look cleaner.


OK. It's also possible to use different methods, e.g. IPA.is_null() and 
IPA.is_empty().


--
Endi S. Dewata

___
Freeipa-devel mailing list
Freeipa-devel@redhat.com
https://www.redhat.com/mailman/listinfo/freeipa-devel

Re: [Freeipa-devel] [PATCH] 212 Fix integer validation when boundary value is empty string

2012-09-17 Thread Endi Sukma Dewata 

On 9/11/2012 10:09 AM, Petr Vobornik wrote:

There was an error in number validation check. If boundary value was an
empty string, validation of a number always failed. This patch fixes the
problem by not performing the check in these cases.

Basic unit tests for IPA.metadata_validator created.


ACK. Some comments:

1. Instead of IPA.not_defined() it might be better to call it 
IPA.defined() to avoid double negations like this:


  if (!IPA.not_defined(metadata.minvalue, true) ...

2. The check_empty_str probably could made optional and the value would 
be true by default. It will make the code cleaner.


--
Endi S. Dewata

___
Freeipa-devel mailing list
Freeipa-devel@redhat.com
https://www.redhat.com/mailman/listinfo/freeipa-devel

[Freeipa-devel] [PATCH] 212 Fix integer validation when boundary value is empty string

2012-09-11 Thread Petr Vobornik 
There was an error in number validation check. If boundary value was an 
empty string, validation of a number always failed. This patch fixes the 
problem by not performing the check in these cases.


Basic unit tests for IPA.metadata_validator created.
--
Petr Vobornik
From 4bde0e0f7dd1b02ce6ae1b37a2f6a97efbb99726 Mon Sep 17 00:00:00 2001
From: Petr Vobornik pvobo...@redhat.com
Date: Tue, 11 Sep 2012 13:52:10 +0200
Subject: [PATCH] Fix integer validation when boundary value is empty string

There was an error in number validation check. If boundary value was an empty string, validation of a number always failed. This patch fixes the problem by not performing the check in these cases.
---
 install/ui/field.js  |   4 +-
 install/ui/ipa.js|   4 ++
 install/ui/test/all_tests.html   |   1 +
 install/ui/test/index.html   |   1 +
 install/ui/test/jsl.conf |   3 +-
 install/ui/test/utils_tests.html |  24 
 install/ui/test/utils_tests.js   | 119 +++
 7 files changed, 153 insertions(+), 3 deletions(-)
 create mode 100644 install/ui/test/utils_tests.html
 create mode 100644 install/ui/test/utils_tests.js

diff --git a/install/ui/field.js b/install/ui/field.js
index 42da6f92cd102aa665b05b72783de9d04dcdcfb0..46113b8caac0026d6e8aefd9587552b2e88b9775 100644
--- a/install/ui/field.js
+++ b/install/ui/field.js
@@ -448,13 +448,13 @@ IPA.metadata_validator = function(spec) {
 
 if (number) {
 
-if (metadata.minvalue !== undefined  Number(value)  Number(metadata.minvalue)) {
+if (!IPA.not_defined(metadata.minvalue, true)  Number(value)  Number(metadata.minvalue)) {
 message = IPA.messages.widget.validation.min_value;
 message = message.replace('${value}', metadata.minvalue);
 return that.false_result(message);
 }
 
-if (metadata.maxvalue !== undefined  Number(value)  Number(metadata.maxvalue)) {
+if (!IPA.not_defined(metadata.maxvalue, true)  Number(value)  Number(metadata.maxvalue)) {
 message = IPA.messages.widget.validation.max_value;
 message = message.replace('${value}', metadata.maxvalue);
 return that.false_result(message);
diff --git a/install/ui/ipa.js b/install/ui/ipa.js
index 23c9933dfb97cb39a932f78d235fdaf844e42b7c..d704a0a562404d07f70d09e856f42fed224f9093 100644
--- a/install/ui/ipa.js
+++ b/install/ui/ipa.js
@@ -2021,6 +2021,10 @@ IPA.is_empty = function(value) {
 return empty;
 };
 
+IPA.not_defined = function(value, check_empty_str) {
+return value === null || value === undefined || (check_empty_str  value === '');
+};
+
 IPA.array_diff = function(a, b) {
 
 if (a === b || (!a  !b)) return false;
diff --git a/install/ui/test/all_tests.html b/install/ui/test/all_tests.html
index 5a5eae54b5c6606564fd0e7b2df8ccc606dccef2..e9061b6ca8802b83c8f99c234ad7311606672695 100644
--- a/install/ui/test/all_tests.html
+++ b/install/ui/test/all_tests.html
@@ -32,6 +32,7 @@
 script type=text/javascript src=aci_tests.js/script
 script type=text/javascript src=widget_tests.js/script
 script type=text/javascript src=ip_tests.js/script
+script type=text/javascript src=utils_tests.js/script
 /head
 body
 h1 id=qunit-headerComplete Test Suite/h1
diff --git a/install/ui/test/index.html b/install/ui/test/index.html
index 1b623d40f794b1ffda90f6c3352840acbaeec26a..0a135188e518913aa3f3a191ab340f2f5e820abf 100644
--- a/install/ui/test/index.html
+++ b/install/ui/test/index.html
@@ -34,6 +34,7 @@
 lia href=aci_tests.htmlAccess Control Interface Test Suite/a
 lia href=widget_tests.htmlWidget Test Suite/a
 lia href=ip_tests.htmlIP Addresses Test Suite/a
+lia href=utils_tests.htmlUtils Test Suite/a
 /ul
 /div
 
diff --git a/install/ui/test/jsl.conf b/install/ui/test/jsl.conf
index 4654b714f91cb00696043bf3042fccd2b2e394bc..768a295f1ac7c956f588b99c60eecdb2bfb06c67 100644
--- a/install/ui/test/jsl.conf
+++ b/install/ui/test/jsl.conf
@@ -146,4 +146,5 @@
 +process ipa_tests.js
 +process ordered_map_tests.js
 +process widget_tests.js
-+process ip_tests.js
\ No newline at end of file
++process ip_tests.js
++process utils_tests.js
\ No newline at end of file
diff --git a/install/ui/test/utils_tests.html b/install/ui/test/utils_tests.html
new file mode 100644
index ..5b81cc35930766907f8db310f1e6e77edee95034
--- /dev/null
+++ b/install/ui/test/utils_tests.html
@@ -0,0 +1,24 @@
+!DOCTYPE html
+html
+head
+titleIPA utils test suite/title
+link rel=stylesheet href=qunit.css type=text/css media=screen
+script type=text/javascript src=../jquery.js/script
+script type=text/javascript src=../jquery.ba-bbq.js/script
+script type=text/javascript src=../jquery-ui.js/script
+script type=text/javascript src=../jquery.ordered-map.js/script
+script type=text/javascript