Thread: [pgadmin-hackers] Fix for RM2421 [pgAdmin4][patch]

[pgadmin-hackers] Fix for RM2421 [pgAdmin4][patch]

From
Harshal Dhumal
Date:
Hi,

Please find attached patch for RM2421

Issue fixed: 1. Integer/numeric Validation is not working properly.
2. Wrong CPU rate unit
-- 
Harshal Dhumal
Sr. Software Engineer

EnterpriseDB India: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
Attachment

Re: [pgadmin-hackers] Fix for RM2421 [pgAdmin4][patch]

From
Joao Pedro De Almeida Pereira
Date:
Hello Harshal,

We review the patch and have some questions:
1) Is there any particular reason to initialize variables and functions in the same place? We believe that it would be more readable there were no chaining of variable creation, specially if those variables are functions. Check line: 
+++ b/web/pgadmin/static/js/backform.pgadmin.js
@@ -1528,7 +1528,18 @@          max_value = field.max,          isValid = true,          intPattern = new RegExp("^-?[0-9]*$"),
-          isMatched = intPattern.test(value);
+          isMatched = intPattern.test(value),
+          trigger_invalid_event = function(msg) {
2) The functions added in both places look very similar, can they be merged and extracted? We are talking about the trigger_invalid_event function.

3) The following change is very similar to the trigger_invalid_event, was there a reason not to use it?
+++ b/web/pgadmin/static/js/backform.pgadmin.js
@@ -1573,25 +1584,23 @@        this.model.errorModel.unset(name);        this.model.set(name, value);        this.listenTo(this.model, "change:" + name, this.render);
-        if (this.model.collection || this.model.handler) {
-          (this.model.collection || this.model.handler).trigger(
-             'pgadmin-session:model:valid', this.model, (this.model.collection || this.model.handler)
-            );
+        // Check if other fields of same model are valid before
+        // triggering 'session:valid' event
+        if(_.size(this.model.errorModel.attributes) == 0) {
+          if (this.model.collection || this.model.handler) {
+            (this.model.collection || this.model.handler).trigger(
+               'pgadmin-session:model:valid', this.model, (this.model.collection || this.model.handler)
+              );
+          } else {
+            (this.model).trigger(
+               'pgadmin-session:valid', this.model.sessChanged(), this.model
+              );
+          }
4) We also noticed that the following change sets look very similiar. Is there any reason to have this code duplicated? If not this could be a good time to refactor it.
+++ b/web/pgadmin/static/js/backform.pgadmin.js
@@ -1528,7 +1528,18 @@

@@ -1573,25 +1584,23 @@

@@ -1631,7 +1640,18 @@

@@ -1676,25 +1696,23 @@

Thanks
Joao & Shruti

On Thu, May 18, 2017 at 6:01 AM, Harshal Dhumal <harshal.dhumal@enterprisedb.com> wrote:
Hi,

Please find attached patch for RM2421

Issue fixed: 1. Integer/numeric Validation is not working properly.
2. Wrong CPU rate unit
-- 
Harshal Dhumal
Sr. Software Engineer

EnterpriseDB India: http://www.enterprisedb.com
The Enterprise PostgreSQL Company


--
Sent via pgadmin-hackers mailing list (pgadmin-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgadmin-hackers


Re: [pgadmin-hackers] Fix for RM2421 [pgAdmin4][patch]

From
Harshal Dhumal
Date:
Hi,

On Thu, May 18, 2017 at 7:57 PM, Joao Pedro De Almeida Pereira <jdealmeidapereira@pivotal.io> wrote:
Hello Harshal,

We review the patch and have some questions:
1) Is there any particular reason to initialize variables and functions in the same place? We believe that it would be more readable there were no chaining of variable creation, specially if those variables are functions. Check line: 
That function is only going to be used in checkNumeric function (in case of Number control) and checkInt function (in case of Integer control) so declared them locally.
Anyway I'm going to refactor both the controls as Number and Integer shares some common properties.

+++ b/web/pgadmin/static/js/backform.pgadmin.js
@@ -1528,7 +1528,18 @@          max_value = field.max,          isValid = true,          intPattern = new RegExp("^-?[0-9]*$"),
-          isMatched = intPattern.test(value);
+          isMatched = intPattern.test(value),
+          trigger_invalid_event = function(msg) {
2) The functions added in both places look very similar, can they be merged and extracted? We are talking about the trigger_invalid_event function.
Yes they can be merged. As of now both NumericControl and IntegerControl are derived from InputControl. Ideally 
only NumericControl should be derived from InputControl and IntegerControl should be derive from NumericControl.

 
3) The following change is very similar to the trigger_invalid_event, was there a reason not to use it?
Below code triggers "model valid" event; opposite to "model invalid" event (trigger_invalid_event)
+++ b/web/pgadmin/static/js/backform.pgadmin.js
@@ -1573,25 +1584,23 @@        this.model.errorModel.unset(name);        this.model.set(name, value);        this.listenTo(this.model, "change:" + name, this.render);
-        if (this.model.collection || this.model.handler) {
-          (this.model.collection || this.model.handler).trigger(
-             'pgadmin-session:model:valid', this.model, (this.model.collection || this.model.handler)
-            );
+        // Check if other fields of same model are valid before
+        // triggering 'session:valid' event
+        if(_.size(this.model.errorModel.attributes) == 0) {
+          if (this.model.collection || this.model.handler) {
+            (this.model.collection || this.model.handler).trigger(
+               'pgadmin-session:model:valid', this.model, (this.model.collection || this.model.handler)
+              );
+          } else {
+            (this.model).trigger(
+               'pgadmin-session:valid', this.model.sessChanged(), this.model
+              );
+          }
4) We also noticed that the following change sets look very similiar. Is there any reason to have this code duplicated? If not this could be a good time to refactor it.
As said earlier in response of point 2 code duplication is because the way controls are derived.
 
+++ b/web/pgadmin/static/js/backform.pgadmin.js
@@ -1528,7 +1528,18 @@

@@ -1573,25 +1584,23 @@

@@ -1631,7 +1640,18 @@

@@ -1676,25 +1696,23 @@

Thanks
Joao & Shruti

On Thu, May 18, 2017 at 6:01 AM, Harshal Dhumal <harshal.dhumal@enterprisedb.com> wrote:
Hi,

Please find attached patch for RM2421

Issue fixed: 1. Integer/numeric Validation is not working properly.
2. Wrong CPU rate unit
-- 
Harshal Dhumal
Sr. Software Engineer

EnterpriseDB India: http://www.enterprisedb.com
The Enterprise PostgreSQL Company


--
Sent via pgadmin-hackers mailing list (pgadmin-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgadmin-hackers



Re: [pgadmin-hackers] Fix for RM2421 [pgAdmin4][patch]

From
Harshal Dhumal
Date:
Hi,

Here is updated patch for RM2421.

Now I have moved all Numeric control level validations to datamodel. As existing implementation was causing 
issues with error messages in create/edit dialog when schema contains two or more Numeric controls.

This is generic issue and not related to resource group. Also I have updated all other nodes which uses Numeric controls



-- 
Harshal Dhumal
Sr. Software Engineer

EnterpriseDB India: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

On Fri, May 19, 2017 at 12:22 PM, Harshal Dhumal <harshal.dhumal@enterprisedb.com> wrote:
Hi,

On Thu, May 18, 2017 at 7:57 PM, Joao Pedro De Almeida Pereira <jdealmeidapereira@pivotal.io> wrote:
Hello Harshal,

We review the patch and have some questions:
1) Is there any particular reason to initialize variables and functions in the same place? We believe that it would be more readable there were no chaining of variable creation, specially if those variables are functions. Check line: 
That function is only going to be used in checkNumeric function (in case of Number control) and checkInt function (in case of Integer control) so declared them locally.
Anyway I'm going to refactor both the controls as Number and Integer shares some common properties.

+++ b/web/pgadmin/static/js/backform.pgadmin.js
@@ -1528,7 +1528,18 @@          max_value = field.max,          isValid = true,          intPattern = new RegExp("^-?[0-9]*$"),
-          isMatched = intPattern.test(value);
+          isMatched = intPattern.test(value),
+          trigger_invalid_event = function(msg) {
2) The functions added in both places look very similar, can they be merged and extracted? We are talking about the trigger_invalid_event function.
Yes they can be merged. As of now both NumericControl and IntegerControl are derived from InputControl. Ideally 
only NumericControl should be derived from InputControl and IntegerControl should be derive from NumericControl.

 
3) The following change is very similar to the trigger_invalid_event, was there a reason not to use it?
Below code triggers "model valid" event; opposite to "model invalid" event (trigger_invalid_event)
+++ b/web/pgadmin/static/js/backform.pgadmin.js
@@ -1573,25 +1584,23 @@        this.model.errorModel.unset(name);        this.model.set(name, value);        this.listenTo(this.model, "change:" + name, this.render);
-        if (this.model.collection || this.model.handler) {
-          (this.model.collection || this.model.handler).trigger(
-             'pgadmin-session:model:valid', this.model, (this.model.collection || this.model.handler)
-            );
+        // Check if other fields of same model are valid before
+        // triggering 'session:valid' event
+        if(_.size(this.model.errorModel.attributes) == 0) {
+          if (this.model.collection || this.model.handler) {
+            (this.model.collection || this.model.handler).trigger(
+               'pgadmin-session:model:valid', this.model, (this.model.collection || this.model.handler)
+              );
+          } else {
+            (this.model).trigger(
+               'pgadmin-session:valid', this.model.sessChanged(), this.model
+              );
+          }
4) We also noticed that the following change sets look very similiar. Is there any reason to have this code duplicated? If not this could be a good time to refactor it.
As said earlier in response of point 2 code duplication is because the way controls are derived.
 
+++ b/web/pgadmin/static/js/backform.pgadmin.js
@@ -1528,7 +1528,18 @@

@@ -1573,25 +1584,23 @@

@@ -1631,7 +1640,18 @@

@@ -1676,25 +1696,23 @@

Thanks
Joao & Shruti

On Thu, May 18, 2017 at 6:01 AM, Harshal Dhumal <harshal.dhumal@enterprisedb.com> wrote:
Hi,

Please find attached patch for RM2421

Issue fixed: 1. Integer/numeric Validation is not working properly.
2. Wrong CPU rate unit
-- 
Harshal Dhumal
Sr. Software Engineer

EnterpriseDB India: http://www.enterprisedb.com
The Enterprise PostgreSQL Company


--
Sent via pgadmin-hackers mailing list (pgadmin-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgadmin-hackers




Attachment

Re: [pgadmin-hackers] Fix for RM2421 [pgAdmin4][patch]

From
Harshal Dhumal
Date:
Hi,

Please ignore this patch as I forgot to include few changes. I'll send updated one.

-- 
Harshal Dhumal
Sr. Software Engineer

EnterpriseDB India: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

On Mon, May 29, 2017 at 3:18 PM, Harshal Dhumal <harshal.dhumal@enterprisedb.com> wrote:
Hi,

Here is updated patch for RM2421.

Now I have moved all Numeric control level validations to datamodel. As existing implementation was causing 
issues with error messages in create/edit dialog when schema contains two or more Numeric controls.

This is generic issue and not related to resource group. Also I have updated all other nodes which uses Numeric controls



-- 
Harshal Dhumal
Sr. Software Engineer

EnterpriseDB India: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

On Fri, May 19, 2017 at 12:22 PM, Harshal Dhumal <harshal.dhumal@enterprisedb.com> wrote:
Hi,

On Thu, May 18, 2017 at 7:57 PM, Joao Pedro De Almeida Pereira <jdealmeidapereira@pivotal.io> wrote:
Hello Harshal,

We review the patch and have some questions:
1) Is there any particular reason to initialize variables and functions in the same place? We believe that it would be more readable there were no chaining of variable creation, specially if those variables are functions. Check line: 
That function is only going to be used in checkNumeric function (in case of Number control) and checkInt function (in case of Integer control) so declared them locally.
Anyway I'm going to refactor both the controls as Number and Integer shares some common properties.

+++ b/web/pgadmin/static/js/backform.pgadmin.js
@@ -1528,7 +1528,18 @@          max_value = field.max,          isValid = true,          intPattern = new RegExp("^-?[0-9]*$"),
-          isMatched = intPattern.test(value);
+          isMatched = intPattern.test(value),
+          trigger_invalid_event = function(msg) {
2) The functions added in both places look very similar, can they be merged and extracted? We are talking about the trigger_invalid_event function.
Yes they can be merged. As of now both NumericControl and IntegerControl are derived from InputControl. Ideally 
only NumericControl should be derived from InputControl and IntegerControl should be derive from NumericControl.

 
3) The following change is very similar to the trigger_invalid_event, was there a reason not to use it?
Below code triggers "model valid" event; opposite to "model invalid" event (trigger_invalid_event)
+++ b/web/pgadmin/static/js/backform.pgadmin.js
@@ -1573,25 +1584,23 @@        this.model.errorModel.unset(name);        this.model.set(name, value);        this.listenTo(this.model, "change:" + name, this.render);
-        if (this.model.collection || this.model.handler) {
-          (this.model.collection || this.model.handler).trigger(
-             'pgadmin-session:model:valid', this.model, (this.model.collection || this.model.handler)
-            );
+        // Check if other fields of same model are valid before
+        // triggering 'session:valid' event
+        if(_.size(this.model.errorModel.attributes) == 0) {
+          if (this.model.collection || this.model.handler) {
+            (this.model.collection || this.model.handler).trigger(
+               'pgadmin-session:model:valid', this.model, (this.model.collection || this.model.handler)
+              );
+          } else {
+            (this.model).trigger(
+               'pgadmin-session:valid', this.model.sessChanged(), this.model
+              );
+          }
4) We also noticed that the following change sets look very similiar. Is there any reason to have this code duplicated? If not this could be a good time to refactor it.
As said earlier in response of point 2 code duplication is because the way controls are derived.
 
+++ b/web/pgadmin/static/js/backform.pgadmin.js
@@ -1528,7 +1528,18 @@

@@ -1573,25 +1584,23 @@

@@ -1631,7 +1640,18 @@

@@ -1676,25 +1696,23 @@

Thanks
Joao & Shruti

On Thu, May 18, 2017 at 6:01 AM, Harshal Dhumal <harshal.dhumal@enterprisedb.com> wrote:
Hi,

Please find attached patch for RM2421

Issue fixed: 1. Integer/numeric Validation is not working properly.
2. Wrong CPU rate unit
-- 
Harshal Dhumal
Sr. Software Engineer

EnterpriseDB India: http://www.enterprisedb.com
The Enterprise PostgreSQL Company


--
Sent via pgadmin-hackers mailing list (pgadmin-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgadmin-hackers





Re: [pgadmin-hackers] Fix for RM2421 [pgAdmin4][patch]

From
Harshal Dhumal
Date:
Hi,

Please find updated patch.

-- 
Harshal Dhumal
Sr. Software Engineer

EnterpriseDB India: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

On Tue, May 30, 2017 at 12:30 PM, Harshal Dhumal <harshal.dhumal@enterprisedb.com> wrote:
Hi,

Please ignore this patch as I forgot to include few changes. I'll send updated one.

-- 
Harshal Dhumal
Sr. Software Engineer

EnterpriseDB India: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

On Mon, May 29, 2017 at 3:18 PM, Harshal Dhumal <harshal.dhumal@enterprisedb.com> wrote:
Hi,

Here is updated patch for RM2421.

Now I have moved all Numeric control level validations to datamodel. As existing implementation was causing 
issues with error messages in create/edit dialog when schema contains two or more Numeric controls.

This is generic issue and not related to resource group. Also I have updated all other nodes which uses Numeric controls



-- 
Harshal Dhumal
Sr. Software Engineer

EnterpriseDB India: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

On Fri, May 19, 2017 at 12:22 PM, Harshal Dhumal <harshal.dhumal@enterprisedb.com> wrote:
Hi,

On Thu, May 18, 2017 at 7:57 PM, Joao Pedro De Almeida Pereira <jdealmeidapereira@pivotal.io> wrote:
Hello Harshal,

We review the patch and have some questions:
1) Is there any particular reason to initialize variables and functions in the same place? We believe that it would be more readable there were no chaining of variable creation, specially if those variables are functions. Check line: 
That function is only going to be used in checkNumeric function (in case of Number control) and checkInt function (in case of Integer control) so declared them locally.
Anyway I'm going to refactor both the controls as Number and Integer shares some common properties.

+++ b/web/pgadmin/static/js/backform.pgadmin.js
@@ -1528,7 +1528,18 @@          max_value = field.max,          isValid = true,          intPattern = new RegExp("^-?[0-9]*$"),
-          isMatched = intPattern.test(value);
+          isMatched = intPattern.test(value),
+          trigger_invalid_event = function(msg) {
2) The functions added in both places look very similar, can they be merged and extracted? We are talking about the trigger_invalid_event function.
Yes they can be merged. As of now both NumericControl and IntegerControl are derived from InputControl. Ideally 
only NumericControl should be derived from InputControl and IntegerControl should be derive from NumericControl.

 
3) The following change is very similar to the trigger_invalid_event, was there a reason not to use it?
Below code triggers "model valid" event; opposite to "model invalid" event (trigger_invalid_event)
+++ b/web/pgadmin/static/js/backform.pgadmin.js
@@ -1573,25 +1584,23 @@        this.model.errorModel.unset(name);        this.model.set(name, value);        this.listenTo(this.model, "change:" + name, this.render);
-        if (this.model.collection || this.model.handler) {
-          (this.model.collection || this.model.handler).trigger(
-             'pgadmin-session:model:valid', this.model, (this.model.collection || this.model.handler)
-            );
+        // Check if other fields of same model are valid before
+        // triggering 'session:valid' event
+        if(_.size(this.model.errorModel.attributes) == 0) {
+          if (this.model.collection || this.model.handler) {
+            (this.model.collection || this.model.handler).trigger(
+               'pgadmin-session:model:valid', this.model, (this.model.collection || this.model.handler)
+              );
+          } else {
+            (this.model).trigger(
+               'pgadmin-session:valid', this.model.sessChanged(), this.model
+              );
+          }
4) We also noticed that the following change sets look very similiar. Is there any reason to have this code duplicated? If not this could be a good time to refactor it.
As said earlier in response of point 2 code duplication is because the way controls are derived.
 
+++ b/web/pgadmin/static/js/backform.pgadmin.js
@@ -1528,7 +1528,18 @@

@@ -1573,25 +1584,23 @@

@@ -1631,7 +1640,18 @@

@@ -1676,25 +1696,23 @@

Thanks
Joao & Shruti

On Thu, May 18, 2017 at 6:01 AM, Harshal Dhumal <harshal.dhumal@enterprisedb.com> wrote:
Hi,

Please find attached patch for RM2421

Issue fixed: 1. Integer/numeric Validation is not working properly.
2. Wrong CPU rate unit
-- 
Harshal Dhumal
Sr. Software Engineer

EnterpriseDB India: http://www.enterprisedb.com
The Enterprise PostgreSQL Company


--
Sent via pgadmin-hackers mailing list (pgadmin-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgadmin-hackers






Attachment

Re: [pgadmin-hackers] Fix for RM2421 [pgAdmin4][patch]

From
Dave Page
Date:
Hi

With this patch applied, it uses the field names instead of the labels
in error messages - e.g.

'dirty_rate_limit' must be numeric

instead of:

'Dirty Rate Limit (KB)' must be numeric.

Thanks.

On Tue, May 30, 2017 at 8:28 AM, Harshal Dhumal
<harshal.dhumal@enterprisedb.com> wrote:
> Hi,
>
> Please find updated patch.
>
> --
> Harshal Dhumal
> Sr. Software Engineer
>
> EnterpriseDB India: http://www.enterprisedb.com
> The Enterprise PostgreSQL Company
>
> On Tue, May 30, 2017 at 12:30 PM, Harshal Dhumal
> <harshal.dhumal@enterprisedb.com> wrote:
>>
>> Hi,
>>
>> Please ignore this patch as I forgot to include few changes. I'll send
>> updated one.
>>
>> --
>> Harshal Dhumal
>> Sr. Software Engineer
>>
>> EnterpriseDB India: http://www.enterprisedb.com
>> The Enterprise PostgreSQL Company
>>
>> On Mon, May 29, 2017 at 3:18 PM, Harshal Dhumal
>> <harshal.dhumal@enterprisedb.com> wrote:
>>>
>>> Hi,
>>>
>>> Here is updated patch for RM2421.
>>>
>>> Now I have moved all Numeric control level validations to datamodel. As
>>> existing implementation was causing
>>> issues with error messages in create/edit dialog when schema contains two
>>> or more Numeric controls.
>>>
>>> This is generic issue and not related to resource group. Also I have
>>> updated all other nodes which uses Numeric controls
>>>
>>>
>>>
>>> --
>>> Harshal Dhumal
>>> Sr. Software Engineer
>>>
>>> EnterpriseDB India: http://www.enterprisedb.com
>>> The Enterprise PostgreSQL Company
>>>
>>> On Fri, May 19, 2017 at 12:22 PM, Harshal Dhumal
>>> <harshal.dhumal@enterprisedb.com> wrote:
>>>>
>>>> Hi,
>>>>
>>>> On Thu, May 18, 2017 at 7:57 PM, Joao Pedro De Almeida Pereira
>>>> <jdealmeidapereira@pivotal.io> wrote:
>>>>>
>>>>> Hello Harshal,
>>>>>
>>>>> We review the patch and have some questions:
>>>>> 1) Is there any particular reason to initialize variables and functions
>>>>> in the same place? We believe that it would be more readable there were no
>>>>> chaining of variable creation, specially if those variables are functions.
>>>>> Check line:
>>>>
>>>> That function is only going to be used in checkNumeric function (in case
>>>> of Number control) and checkInt function (in case of Integer control) so
>>>> declared them locally.
>>>> Anyway I'm going to refactor both the controls as Number and Integer
>>>> shares some common properties.
>>>>
>>>>> +++ b/web/pgadmin/static/js/backform.pgadmin.js
>>>>> @@ -1528,7 +1528,18 @@
>>>>>            max_value = field.max,
>>>>>            isValid = true,
>>>>>            intPattern = new RegExp("^-?[0-9]*$"),
>>>>> -          isMatched = intPattern.test(value);
>>>>> +          isMatched = intPattern.test(value),
>>>>> +          trigger_invalid_event = function(msg) {
>>>>>
>>>>> 2) The functions added in both places look very similar, can they be
>>>>> merged and extracted? We are talking about the trigger_invalid_event
>>>>> function.
>>>>
>>>> Yes they can be merged. As of now both NumericControl and IntegerControl
>>>> are derived from InputControl. Ideally
>>>> only NumericControl should be derived from InputControl and
>>>> IntegerControl should be derive from NumericControl.
>>>>
>>>>
>>>>>
>>>>> 3) The following change is very similar to the trigger_invalid_event,
>>>>> was there a reason not to use it?
>>>>
>>>> Below code triggers "model valid" event; opposite to "model invalid"
>>>> event (trigger_invalid_event)
>>>>>
>>>>> +++ b/web/pgadmin/static/js/backform.pgadmin.js
>>>>> @@ -1573,25 +1584,23 @@
>>>>>          this.model.errorModel.unset(name);
>>>>>          this.model.set(name, value);
>>>>>          this.listenTo(this.model, "change:" + name, this.render);
>>>>> -        if (this.model.collection || this.model.handler) {
>>>>> -          (this.model.collection || this.model.handler).trigger(
>>>>> -             'pgadmin-session:model:valid', this.model,
>>>>> (this.model.collection || this.model.handler)
>>>>> -            );
>>>>> +        // Check if other fields of same model are valid before
>>>>> +        // triggering 'session:valid' event
>>>>> +        if(_.size(this.model.errorModel.attributes) == 0) {
>>>>> +          if (this.model.collection || this.model.handler) {
>>>>> +            (this.model.collection || this.model.handler).trigger(
>>>>> +               'pgadmin-session:model:valid', this.model,
>>>>> (this.model.collection || this.model.handler)
>>>>> +              );
>>>>> +          } else {
>>>>> +            (this.model).trigger(
>>>>> +               'pgadmin-session:valid', this.model.sessChanged(),
>>>>> this.model
>>>>> +              );
>>>>> +          }
>>>>>
>>>>> 4) We also noticed that the following change sets look very similiar.
>>>>> Is there any reason to have this code duplicated? If not this could be a
>>>>> good time to refactor it.
>>>>
>>>> As said earlier in response of point 2 code duplication is because the
>>>> way controls are derived.
>>>>
>>>>>
>>>>> +++ b/web/pgadmin/static/js/backform.pgadmin.js
>>>>> @@ -1528,7 +1528,18 @@
>>>>>
>>>>> @@ -1573,25 +1584,23 @@
>>>>>
>>>>> @@ -1631,7 +1640,18 @@
>>>>>
>>>>> @@ -1676,25 +1696,23 @@
>>>>>
>>>>>
>>>>> Thanks
>>>>> Joao & Shruti
>>>>>
>>>>> On Thu, May 18, 2017 at 6:01 AM, Harshal Dhumal
>>>>> <harshal.dhumal@enterprisedb.com> wrote:
>>>>>>
>>>>>> Hi,
>>>>>>
>>>>>> Please find attached patch for RM2421
>>>>>>
>>>>>> Issue fixed: 1. Integer/numeric Validation is not working properly.
>>>>>> 2. Wrong CPU rate unit
>>>>>> --
>>>>>> Harshal Dhumal
>>>>>> Sr. Software Engineer
>>>>>>
>>>>>> EnterpriseDB India: http://www.enterprisedb.com
>>>>>> The Enterprise PostgreSQL Company
>>>>>>
>>>>>>
>>>>>> --
>>>>>> Sent via pgadmin-hackers mailing list (pgadmin-hackers@postgresql.org)
>>>>>> To make changes to your subscription:
>>>>>> http://www.postgresql.org/mailpref/pgadmin-hackers
>>>>>>
>>>>>
>>>>
>>>
>>
>
>
>
> --
> Sent via pgadmin-hackers mailing list (pgadmin-hackers@postgresql.org)
> To make changes to your subscription:
> http://www.postgresql.org/mailpref/pgadmin-hackers
>



--
Dave Page
Blog: http://pgsnake.blogspot.com
Twitter: @pgsnake

EnterpriseDB UK: http://www.enterprisedb.com
The Enterprise PostgreSQL Company


Re: [pgadmin-hackers] Fix for RM2421 [pgAdmin4][patch]

From
Harshal Dhumal
Date:
Hi,

On Mon, Jun 5, 2017 at 9:25 PM, Dave Page <dpage@pgadmin.org> wrote:
Hi

With this patch applied, it uses the field names instead of the labels
in error messages - e.g.

'dirty_rate_limit' must be numeric

instead of:

'Dirty Rate Limit (KB)' must be numeric.
Fixed. Please find attached updated patch.
 

Thanks.

On Tue, May 30, 2017 at 8:28 AM, Harshal Dhumal
<harshal.dhumal@enterprisedb.com> wrote:
> Hi,
>
> Please find updated patch.
>
> --
> Harshal Dhumal
> Sr. Software Engineer
>
> EnterpriseDB India: http://www.enterprisedb.com
> The Enterprise PostgreSQL Company
>
> On Tue, May 30, 2017 at 12:30 PM, Harshal Dhumal
> <harshal.dhumal@enterprisedb.com> wrote:
>>
>> Hi,
>>
>> Please ignore this patch as I forgot to include few changes. I'll send
>> updated one.
>>
>> --
>> Harshal Dhumal
>> Sr. Software Engineer
>>
>> EnterpriseDB India: http://www.enterprisedb.com
>> The Enterprise PostgreSQL Company
>>
>> On Mon, May 29, 2017 at 3:18 PM, Harshal Dhumal
>> <harshal.dhumal@enterprisedb.com> wrote:
>>>
>>> Hi,
>>>
>>> Here is updated patch for RM2421.
>>>
>>> Now I have moved all Numeric control level validations to datamodel. As
>>> existing implementation was causing
>>> issues with error messages in create/edit dialog when schema contains two
>>> or more Numeric controls.
>>>
>>> This is generic issue and not related to resource group. Also I have
>>> updated all other nodes which uses Numeric controls
>>>
>>>
>>>
>>> --
>>> Harshal Dhumal
>>> Sr. Software Engineer
>>>
>>> EnterpriseDB India: http://www.enterprisedb.com
>>> The Enterprise PostgreSQL Company
>>>
>>> On Fri, May 19, 2017 at 12:22 PM, Harshal Dhumal
>>> <harshal.dhumal@enterprisedb.com> wrote:
>>>>
>>>> Hi,
>>>>
>>>> On Thu, May 18, 2017 at 7:57 PM, Joao Pedro De Almeida Pereira
>>>> <jdealmeidapereira@pivotal.io> wrote:
>>>>>
>>>>> Hello Harshal,
>>>>>
>>>>> We review the patch and have some questions:
>>>>> 1) Is there any particular reason to initialize variables and functions
>>>>> in the same place? We believe that it would be more readable there were no
>>>>> chaining of variable creation, specially if those variables are functions.
>>>>> Check line:
>>>>
>>>> That function is only going to be used in checkNumeric function (in case
>>>> of Number control) and checkInt function (in case of Integer control) so
>>>> declared them locally.
>>>> Anyway I'm going to refactor both the controls as Number and Integer
>>>> shares some common properties.
>>>>
>>>>> +++ b/web/pgadmin/static/js/backform.pgadmin.js
>>>>> @@ -1528,7 +1528,18 @@
>>>>>            max_value = field.max,
>>>>>            isValid = true,
>>>>>            intPattern = new RegExp("^-?[0-9]*$"),
>>>>> -          isMatched = intPattern.test(value);
>>>>> +          isMatched = intPattern.test(value),
>>>>> +          trigger_invalid_event = function(msg) {
>>>>>
>>>>> 2) The functions added in both places look very similar, can they be
>>>>> merged and extracted? We are talking about the trigger_invalid_event
>>>>> function.
>>>>
>>>> Yes they can be merged. As of now both NumericControl and IntegerControl
>>>> are derived from InputControl. Ideally
>>>> only NumericControl should be derived from InputControl and
>>>> IntegerControl should be derive from NumericControl.
>>>>
>>>>
>>>>>
>>>>> 3) The following change is very similar to the trigger_invalid_event,
>>>>> was there a reason not to use it?
>>>>
>>>> Below code triggers "model valid" event; opposite to "model invalid"
>>>> event (trigger_invalid_event)
>>>>>
>>>>> +++ b/web/pgadmin/static/js/backform.pgadmin.js
>>>>> @@ -1573,25 +1584,23 @@
>>>>>          this.model.errorModel.unset(name);
>>>>>          this.model.set(name, value);
>>>>>          this.listenTo(this.model, "change:" + name, this.render);
>>>>> -        if (this.model.collection || this.model.handler) {
>>>>> -          (this.model.collection || this.model.handler).trigger(
>>>>> -             'pgadmin-session:model:valid', this.model,
>>>>> (this.model.collection || this.model.handler)
>>>>> -            );
>>>>> +        // Check if other fields of same model are valid before
>>>>> +        // triggering 'session:valid' event
>>>>> +        if(_.size(this.model.errorModel.attributes) == 0) {
>>>>> +          if (this.model.collection || this.model.handler) {
>>>>> +            (this.model.collection || this.model.handler).trigger(
>>>>> +               'pgadmin-session:model:valid', this.model,
>>>>> (this.model.collection || this.model.handler)
>>>>> +              );
>>>>> +          } else {
>>>>> +            (this.model).trigger(
>>>>> +               'pgadmin-session:valid', this.model.sessChanged(),
>>>>> this.model
>>>>> +              );
>>>>> +          }
>>>>>
>>>>> 4) We also noticed that the following change sets look very similiar.
>>>>> Is there any reason to have this code duplicated? If not this could be a
>>>>> good time to refactor it.
>>>>
>>>> As said earlier in response of point 2 code duplication is because the
>>>> way controls are derived.
>>>>
>>>>>
>>>>> +++ b/web/pgadmin/static/js/backform.pgadmin.js
>>>>> @@ -1528,7 +1528,18 @@
>>>>>
>>>>> @@ -1573,25 +1584,23 @@
>>>>>
>>>>> @@ -1631,7 +1640,18 @@
>>>>>
>>>>> @@ -1676,25 +1696,23 @@
>>>>>
>>>>>
>>>>> Thanks
>>>>> Joao & Shruti
>>>>>
>>>>> On Thu, May 18, 2017 at 6:01 AM, Harshal Dhumal
>>>>> <harshal.dhumal@enterprisedb.com> wrote:
>>>>>>
>>>>>> Hi,
>>>>>>
>>>>>> Please find attached patch for RM2421
>>>>>>
>>>>>> Issue fixed: 1. Integer/numeric Validation is not working properly.
>>>>>> 2. Wrong CPU rate unit
>>>>>> --
>>>>>> Harshal Dhumal
>>>>>> Sr. Software Engineer
>>>>>>
>>>>>> EnterpriseDB India: http://www.enterprisedb.com
>>>>>> The Enterprise PostgreSQL Company
>>>>>>
>>>>>>
>>>>>> --
>>>>>> Sent via pgadmin-hackers mailing list (pgadmin-hackers@postgresql.org)
>>>>>> To make changes to your subscription:
>>>>>> http://www.postgresql.org/mailpref/pgadmin-hackers
>>>>>>
>>>>>
>>>>
>>>
>>
>
>
>
> --
> Sent via pgadmin-hackers mailing list (pgadmin-hackers@postgresql.org)
> To make changes to your subscription:
> http://www.postgresql.org/mailpref/pgadmin-hackers
>



--
Dave Page
Blog: http://pgsnake.blogspot.com
Twitter: @pgsnake

EnterpriseDB UK: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

Attachment

Re: [pgadmin-hackers] Fix for RM2421 [pgAdmin4][patch]

From
Dave Page
Date:
Can you rebase this please? I think Ashesh broke it :-p

On Tue, Jun 6, 2017 at 7:42 AM, Harshal Dhumal
<harshal.dhumal@enterprisedb.com> wrote:
> Hi,
>
> On Mon, Jun 5, 2017 at 9:25 PM, Dave Page <dpage@pgadmin.org> wrote:
>>
>> Hi
>>
>> With this patch applied, it uses the field names instead of the labels
>> in error messages - e.g.
>>
>> 'dirty_rate_limit' must be numeric
>>
>> instead of:
>>
>> 'Dirty Rate Limit (KB)' must be numeric.
>
> Fixed. Please find attached updated patch.
>
>>
>>
>> Thanks.
>>
>> On Tue, May 30, 2017 at 8:28 AM, Harshal Dhumal
>> <harshal.dhumal@enterprisedb.com> wrote:
>> > Hi,
>> >
>> > Please find updated patch.
>> >
>> > --
>> > Harshal Dhumal
>> > Sr. Software Engineer
>> >
>> > EnterpriseDB India: http://www.enterprisedb.com
>> > The Enterprise PostgreSQL Company
>> >
>> > On Tue, May 30, 2017 at 12:30 PM, Harshal Dhumal
>> > <harshal.dhumal@enterprisedb.com> wrote:
>> >>
>> >> Hi,
>> >>
>> >> Please ignore this patch as I forgot to include few changes. I'll send
>> >> updated one.
>> >>
>> >> --
>> >> Harshal Dhumal
>> >> Sr. Software Engineer
>> >>
>> >> EnterpriseDB India: http://www.enterprisedb.com
>> >> The Enterprise PostgreSQL Company
>> >>
>> >> On Mon, May 29, 2017 at 3:18 PM, Harshal Dhumal
>> >> <harshal.dhumal@enterprisedb.com> wrote:
>> >>>
>> >>> Hi,
>> >>>
>> >>> Here is updated patch for RM2421.
>> >>>
>> >>> Now I have moved all Numeric control level validations to datamodel.
>> >>> As
>> >>> existing implementation was causing
>> >>> issues with error messages in create/edit dialog when schema contains
>> >>> two
>> >>> or more Numeric controls.
>> >>>
>> >>> This is generic issue and not related to resource group. Also I have
>> >>> updated all other nodes which uses Numeric controls
>> >>>
>> >>>
>> >>>
>> >>> --
>> >>> Harshal Dhumal
>> >>> Sr. Software Engineer
>> >>>
>> >>> EnterpriseDB India: http://www.enterprisedb.com
>> >>> The Enterprise PostgreSQL Company
>> >>>
>> >>> On Fri, May 19, 2017 at 12:22 PM, Harshal Dhumal
>> >>> <harshal.dhumal@enterprisedb.com> wrote:
>> >>>>
>> >>>> Hi,
>> >>>>
>> >>>> On Thu, May 18, 2017 at 7:57 PM, Joao Pedro De Almeida Pereira
>> >>>> <jdealmeidapereira@pivotal.io> wrote:
>> >>>>>
>> >>>>> Hello Harshal,
>> >>>>>
>> >>>>> We review the patch and have some questions:
>> >>>>> 1) Is there any particular reason to initialize variables and
>> >>>>> functions
>> >>>>> in the same place? We believe that it would be more readable there
>> >>>>> were no
>> >>>>> chaining of variable creation, specially if those variables are
>> >>>>> functions.
>> >>>>> Check line:
>> >>>>
>> >>>> That function is only going to be used in checkNumeric function (in
>> >>>> case
>> >>>> of Number control) and checkInt function (in case of Integer control)
>> >>>> so
>> >>>> declared them locally.
>> >>>> Anyway I'm going to refactor both the controls as Number and Integer
>> >>>> shares some common properties.
>> >>>>
>> >>>>> +++ b/web/pgadmin/static/js/backform.pgadmin.js
>> >>>>> @@ -1528,7 +1528,18 @@
>> >>>>>            max_value = field.max,
>> >>>>>            isValid = true,
>> >>>>>            intPattern = new RegExp("^-?[0-9]*$"),
>> >>>>> -          isMatched = intPattern.test(value);
>> >>>>> +          isMatched = intPattern.test(value),
>> >>>>> +          trigger_invalid_event = function(msg) {
>> >>>>>
>> >>>>> 2) The functions added in both places look very similar, can they be
>> >>>>> merged and extracted? We are talking about the trigger_invalid_event
>> >>>>> function.
>> >>>>
>> >>>> Yes they can be merged. As of now both NumericControl and
>> >>>> IntegerControl
>> >>>> are derived from InputControl. Ideally
>> >>>> only NumericControl should be derived from InputControl and
>> >>>> IntegerControl should be derive from NumericControl.
>> >>>>
>> >>>>
>> >>>>>
>> >>>>> 3) The following change is very similar to the
>> >>>>> trigger_invalid_event,
>> >>>>> was there a reason not to use it?
>> >>>>
>> >>>> Below code triggers "model valid" event; opposite to "model invalid"
>> >>>> event (trigger_invalid_event)
>> >>>>>
>> >>>>> +++ b/web/pgadmin/static/js/backform.pgadmin.js
>> >>>>> @@ -1573,25 +1584,23 @@
>> >>>>>          this.model.errorModel.unset(name);
>> >>>>>          this.model.set(name, value);
>> >>>>>          this.listenTo(this.model, "change:" + name, this.render);
>> >>>>> -        if (this.model.collection || this.model.handler) {
>> >>>>> -          (this.model.collection || this.model.handler).trigger(
>> >>>>> -             'pgadmin-session:model:valid', this.model,
>> >>>>> (this.model.collection || this.model.handler)
>> >>>>> -            );
>> >>>>> +        // Check if other fields of same model are valid before
>> >>>>> +        // triggering 'session:valid' event
>> >>>>> +        if(_.size(this.model.errorModel.attributes) == 0) {
>> >>>>> +          if (this.model.collection || this.model.handler) {
>> >>>>> +            (this.model.collection || this.model.handler).trigger(
>> >>>>> +               'pgadmin-session:model:valid', this.model,
>> >>>>> (this.model.collection || this.model.handler)
>> >>>>> +              );
>> >>>>> +          } else {
>> >>>>> +            (this.model).trigger(
>> >>>>> +               'pgadmin-session:valid', this.model.sessChanged(),
>> >>>>> this.model
>> >>>>> +              );
>> >>>>> +          }
>> >>>>>
>> >>>>> 4) We also noticed that the following change sets look very
>> >>>>> similiar.
>> >>>>> Is there any reason to have this code duplicated? If not this could
>> >>>>> be a
>> >>>>> good time to refactor it.
>> >>>>
>> >>>> As said earlier in response of point 2 code duplication is because
>> >>>> the
>> >>>> way controls are derived.
>> >>>>
>> >>>>>
>> >>>>> +++ b/web/pgadmin/static/js/backform.pgadmin.js
>> >>>>> @@ -1528,7 +1528,18 @@
>> >>>>>
>> >>>>> @@ -1573,25 +1584,23 @@
>> >>>>>
>> >>>>> @@ -1631,7 +1640,18 @@
>> >>>>>
>> >>>>> @@ -1676,25 +1696,23 @@
>> >>>>>
>> >>>>>
>> >>>>> Thanks
>> >>>>> Joao & Shruti
>> >>>>>
>> >>>>> On Thu, May 18, 2017 at 6:01 AM, Harshal Dhumal
>> >>>>> <harshal.dhumal@enterprisedb.com> wrote:
>> >>>>>>
>> >>>>>> Hi,
>> >>>>>>
>> >>>>>> Please find attached patch for RM2421
>> >>>>>>
>> >>>>>> Issue fixed: 1. Integer/numeric Validation is not working properly.
>> >>>>>> 2. Wrong CPU rate unit
>> >>>>>> --
>> >>>>>> Harshal Dhumal
>> >>>>>> Sr. Software Engineer
>> >>>>>>
>> >>>>>> EnterpriseDB India: http://www.enterprisedb.com
>> >>>>>> The Enterprise PostgreSQL Company
>> >>>>>>
>> >>>>>>
>> >>>>>> --
>> >>>>>> Sent via pgadmin-hackers mailing list
>> >>>>>> (pgadmin-hackers@postgresql.org)
>> >>>>>> To make changes to your subscription:
>> >>>>>> http://www.postgresql.org/mailpref/pgadmin-hackers
>> >>>>>>
>> >>>>>
>> >>>>
>> >>>
>> >>
>> >
>> >
>> >
>> > --
>> > Sent via pgadmin-hackers mailing list (pgadmin-hackers@postgresql.org)
>> > To make changes to your subscription:
>> > http://www.postgresql.org/mailpref/pgadmin-hackers
>> >
>>
>>
>>
>> --
>> Dave Page
>> Blog: http://pgsnake.blogspot.com
>> Twitter: @pgsnake
>>
>> EnterpriseDB UK: http://www.enterprisedb.com
>> The Enterprise PostgreSQL Company
>
>



--
Dave Page
Blog: http://pgsnake.blogspot.com
Twitter: @pgsnake

EnterpriseDB UK: http://www.enterprisedb.com
The Enterprise PostgreSQL Company


Re: [pgadmin-hackers] Fix for RM2421 [pgAdmin4][patch]

From
Harshal Dhumal
Date:
Please find attached rebased patch.

-- 
Harshal Dhumal
Sr. Software Engineer

EnterpriseDB India: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

On Wed, Jun 7, 2017 at 6:59 PM, Dave Page <dpage@pgadmin.org> wrote:
Can you rebase this please? I think Ashesh broke it :-p

On Tue, Jun 6, 2017 at 7:42 AM, Harshal Dhumal
<harshal.dhumal@enterprisedb.com> wrote:
> Hi,
>
> On Mon, Jun 5, 2017 at 9:25 PM, Dave Page <dpage@pgadmin.org> wrote:
>>
>> Hi
>>
>> With this patch applied, it uses the field names instead of the labels
>> in error messages - e.g.
>>
>> 'dirty_rate_limit' must be numeric
>>
>> instead of:
>>
>> 'Dirty Rate Limit (KB)' must be numeric.
>
> Fixed. Please find attached updated patch.
>
>>
>>
>> Thanks.
>>
>> On Tue, May 30, 2017 at 8:28 AM, Harshal Dhumal
>> <harshal.dhumal@enterprisedb.com> wrote:
>> > Hi,
>> >
>> > Please find updated patch.
>> >
>> > --
>> > Harshal Dhumal
>> > Sr. Software Engineer
>> >
>> > EnterpriseDB India: http://www.enterprisedb.com
>> > The Enterprise PostgreSQL Company
>> >
>> > On Tue, May 30, 2017 at 12:30 PM, Harshal Dhumal
>> > <harshal.dhumal@enterprisedb.com> wrote:
>> >>
>> >> Hi,
>> >>
>> >> Please ignore this patch as I forgot to include few changes. I'll send
>> >> updated one.
>> >>
>> >> --
>> >> Harshal Dhumal
>> >> Sr. Software Engineer
>> >>
>> >> EnterpriseDB India: http://www.enterprisedb.com
>> >> The Enterprise PostgreSQL Company
>> >>
>> >> On Mon, May 29, 2017 at 3:18 PM, Harshal Dhumal
>> >> <harshal.dhumal@enterprisedb.com> wrote:
>> >>>
>> >>> Hi,
>> >>>
>> >>> Here is updated patch for RM2421.
>> >>>
>> >>> Now I have moved all Numeric control level validations to datamodel.
>> >>> As
>> >>> existing implementation was causing
>> >>> issues with error messages in create/edit dialog when schema contains
>> >>> two
>> >>> or more Numeric controls.
>> >>>
>> >>> This is generic issue and not related to resource group. Also I have
>> >>> updated all other nodes which uses Numeric controls
>> >>>
>> >>>
>> >>>
>> >>> --
>> >>> Harshal Dhumal
>> >>> Sr. Software Engineer
>> >>>
>> >>> EnterpriseDB India: http://www.enterprisedb.com
>> >>> The Enterprise PostgreSQL Company
>> >>>
>> >>> On Fri, May 19, 2017 at 12:22 PM, Harshal Dhumal
>> >>> <harshal.dhumal@enterprisedb.com> wrote:
>> >>>>
>> >>>> Hi,
>> >>>>
>> >>>> On Thu, May 18, 2017 at 7:57 PM, Joao Pedro De Almeida Pereira
>> >>>> <jdealmeidapereira@pivotal.io> wrote:
>> >>>>>
>> >>>>> Hello Harshal,
>> >>>>>
>> >>>>> We review the patch and have some questions:
>> >>>>> 1) Is there any particular reason to initialize variables and
>> >>>>> functions
>> >>>>> in the same place? We believe that it would be more readable there
>> >>>>> were no
>> >>>>> chaining of variable creation, specially if those variables are
>> >>>>> functions.
>> >>>>> Check line:
>> >>>>
>> >>>> That function is only going to be used in checkNumeric function (in
>> >>>> case
>> >>>> of Number control) and checkInt function (in case of Integer control)
>> >>>> so
>> >>>> declared them locally.
>> >>>> Anyway I'm going to refactor both the controls as Number and Integer
>> >>>> shares some common properties.
>> >>>>
>> >>>>> +++ b/web/pgadmin/static/js/backform.pgadmin.js
>> >>>>> @@ -1528,7 +1528,18 @@
>> >>>>>            max_value = field.max,
>> >>>>>            isValid = true,
>> >>>>>            intPattern = new RegExp("^-?[0-9]*$"),
>> >>>>> -          isMatched = intPattern.test(value);
>> >>>>> +          isMatched = intPattern.test(value),
>> >>>>> +          trigger_invalid_event = function(msg) {
>> >>>>>
>> >>>>> 2) The functions added in both places look very similar, can they be
>> >>>>> merged and extracted? We are talking about the trigger_invalid_event
>> >>>>> function.
>> >>>>
>> >>>> Yes they can be merged. As of now both NumericControl and
>> >>>> IntegerControl
>> >>>> are derived from InputControl. Ideally
>> >>>> only NumericControl should be derived from InputControl and
>> >>>> IntegerControl should be derive from NumericControl.
>> >>>>
>> >>>>
>> >>>>>
>> >>>>> 3) The following change is very similar to the
>> >>>>> trigger_invalid_event,
>> >>>>> was there a reason not to use it?
>> >>>>
>> >>>> Below code triggers "model valid" event; opposite to "model invalid"
>> >>>> event (trigger_invalid_event)
>> >>>>>
>> >>>>> +++ b/web/pgadmin/static/js/backform.pgadmin.js
>> >>>>> @@ -1573,25 +1584,23 @@
>> >>>>>          this.model.errorModel.unset(name);
>> >>>>>          this.model.set(name, value);
>> >>>>>          this.listenTo(this.model, "change:" + name, this.render);
>> >>>>> -        if (this.model.collection || this.model.handler) {
>> >>>>> -          (this.model.collection || this.model.handler).trigger(
>> >>>>> -             'pgadmin-session:model:valid', this.model,
>> >>>>> (this.model.collection || this.model.handler)
>> >>>>> -            );
>> >>>>> +        // Check if other fields of same model are valid before
>> >>>>> +        // triggering 'session:valid' event
>> >>>>> +        if(_.size(this.model.errorModel.attributes) == 0) {
>> >>>>> +          if (this.model.collection || this.model.handler) {
>> >>>>> +            (this.model.collection || this.model.handler).trigger(
>> >>>>> +               'pgadmin-session:model:valid', this.model,
>> >>>>> (this.model.collection || this.model.handler)
>> >>>>> +              );
>> >>>>> +          } else {
>> >>>>> +            (this.model).trigger(
>> >>>>> +               'pgadmin-session:valid', this.model.sessChanged(),
>> >>>>> this.model
>> >>>>> +              );
>> >>>>> +          }
>> >>>>>
>> >>>>> 4) We also noticed that the following change sets look very
>> >>>>> similiar.
>> >>>>> Is there any reason to have this code duplicated? If not this could
>> >>>>> be a
>> >>>>> good time to refactor it.
>> >>>>
>> >>>> As said earlier in response of point 2 code duplication is because
>> >>>> the
>> >>>> way controls are derived.
>> >>>>
>> >>>>>
>> >>>>> +++ b/web/pgadmin/static/js/backform.pgadmin.js
>> >>>>> @@ -1528,7 +1528,18 @@
>> >>>>>
>> >>>>> @@ -1573,25 +1584,23 @@
>> >>>>>
>> >>>>> @@ -1631,7 +1640,18 @@
>> >>>>>
>> >>>>> @@ -1676,25 +1696,23 @@
>> >>>>>
>> >>>>>
>> >>>>> Thanks
>> >>>>> Joao & Shruti
>> >>>>>
>> >>>>> On Thu, May 18, 2017 at 6:01 AM, Harshal Dhumal
>> >>>>> <harshal.dhumal@enterprisedb.com> wrote:
>> >>>>>>
>> >>>>>> Hi,
>> >>>>>>
>> >>>>>> Please find attached patch for RM2421
>> >>>>>>
>> >>>>>> Issue fixed: 1. Integer/numeric Validation is not working properly.
>> >>>>>> 2. Wrong CPU rate unit
>> >>>>>> --
>> >>>>>> Harshal Dhumal
>> >>>>>> Sr. Software Engineer
>> >>>>>>
>> >>>>>> EnterpriseDB India: http://www.enterprisedb.com
>> >>>>>> The Enterprise PostgreSQL Company
>> >>>>>>
>> >>>>>>
>> >>>>>> --
>> >>>>>> Sent via pgadmin-hackers mailing list
>> >>>>>> (pgadmin-hackers@postgresql.org)
>> >>>>>> To make changes to your subscription:
>> >>>>>> http://www.postgresql.org/mailpref/pgadmin-hackers
>> >>>>>>
>> >>>>>
>> >>>>
>> >>>
>> >>
>> >
>> >
>> >
>> > --
>> > Sent via pgadmin-hackers mailing list (pgadmin-hackers@postgresql.org)
>> > To make changes to your subscription:
>> > http://www.postgresql.org/mailpref/pgadmin-hackers
>> >
>>
>>
>>
>> --
>> Dave Page
>> Blog: http://pgsnake.blogspot.com
>> Twitter: @pgsnake
>>
>> EnterpriseDB UK: http://www.enterprisedb.com
>> The Enterprise PostgreSQL Company
>
>



--
Dave Page
Blog: http://pgsnake.blogspot.com
Twitter: @pgsnake

EnterpriseDB UK: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

Attachment

Re: [pgadmin-hackers] Fix for RM2421 [pgAdmin4][patch]

From
Dave Page
Date:
Thanks, applied.

On Thu, Jun 8, 2017 at 1:59 PM, Harshal Dhumal
<harshal.dhumal@enterprisedb.com> wrote:
> Please find attached rebased patch.
>
> --
> Harshal Dhumal
> Sr. Software Engineer
>
> EnterpriseDB India: http://www.enterprisedb.com
> The Enterprise PostgreSQL Company
>
> On Wed, Jun 7, 2017 at 6:59 PM, Dave Page <dpage@pgadmin.org> wrote:
>>
>> Can you rebase this please? I think Ashesh broke it :-p
>>
>> On Tue, Jun 6, 2017 at 7:42 AM, Harshal Dhumal
>> <harshal.dhumal@enterprisedb.com> wrote:
>> > Hi,
>> >
>> > On Mon, Jun 5, 2017 at 9:25 PM, Dave Page <dpage@pgadmin.org> wrote:
>> >>
>> >> Hi
>> >>
>> >> With this patch applied, it uses the field names instead of the labels
>> >> in error messages - e.g.
>> >>
>> >> 'dirty_rate_limit' must be numeric
>> >>
>> >> instead of:
>> >>
>> >> 'Dirty Rate Limit (KB)' must be numeric.
>> >
>> > Fixed. Please find attached updated patch.
>> >
>> >>
>> >>
>> >> Thanks.
>> >>
>> >> On Tue, May 30, 2017 at 8:28 AM, Harshal Dhumal
>> >> <harshal.dhumal@enterprisedb.com> wrote:
>> >> > Hi,
>> >> >
>> >> > Please find updated patch.
>> >> >
>> >> > --
>> >> > Harshal Dhumal
>> >> > Sr. Software Engineer
>> >> >
>> >> > EnterpriseDB India: http://www.enterprisedb.com
>> >> > The Enterprise PostgreSQL Company
>> >> >
>> >> > On Tue, May 30, 2017 at 12:30 PM, Harshal Dhumal
>> >> > <harshal.dhumal@enterprisedb.com> wrote:
>> >> >>
>> >> >> Hi,
>> >> >>
>> >> >> Please ignore this patch as I forgot to include few changes. I'll
>> >> >> send
>> >> >> updated one.
>> >> >>
>> >> >> --
>> >> >> Harshal Dhumal
>> >> >> Sr. Software Engineer
>> >> >>
>> >> >> EnterpriseDB India: http://www.enterprisedb.com
>> >> >> The Enterprise PostgreSQL Company
>> >> >>
>> >> >> On Mon, May 29, 2017 at 3:18 PM, Harshal Dhumal
>> >> >> <harshal.dhumal@enterprisedb.com> wrote:
>> >> >>>
>> >> >>> Hi,
>> >> >>>
>> >> >>> Here is updated patch for RM2421.
>> >> >>>
>> >> >>> Now I have moved all Numeric control level validations to
>> >> >>> datamodel.
>> >> >>> As
>> >> >>> existing implementation was causing
>> >> >>> issues with error messages in create/edit dialog when schema
>> >> >>> contains
>> >> >>> two
>> >> >>> or more Numeric controls.
>> >> >>>
>> >> >>> This is generic issue and not related to resource group. Also I
>> >> >>> have
>> >> >>> updated all other nodes which uses Numeric controls
>> >> >>>
>> >> >>>
>> >> >>>
>> >> >>> --
>> >> >>> Harshal Dhumal
>> >> >>> Sr. Software Engineer
>> >> >>>
>> >> >>> EnterpriseDB India: http://www.enterprisedb.com
>> >> >>> The Enterprise PostgreSQL Company
>> >> >>>
>> >> >>> On Fri, May 19, 2017 at 12:22 PM, Harshal Dhumal
>> >> >>> <harshal.dhumal@enterprisedb.com> wrote:
>> >> >>>>
>> >> >>>> Hi,
>> >> >>>>
>> >> >>>> On Thu, May 18, 2017 at 7:57 PM, Joao Pedro De Almeida Pereira
>> >> >>>> <jdealmeidapereira@pivotal.io> wrote:
>> >> >>>>>
>> >> >>>>> Hello Harshal,
>> >> >>>>>
>> >> >>>>> We review the patch and have some questions:
>> >> >>>>> 1) Is there any particular reason to initialize variables and
>> >> >>>>> functions
>> >> >>>>> in the same place? We believe that it would be more readable
>> >> >>>>> there
>> >> >>>>> were no
>> >> >>>>> chaining of variable creation, specially if those variables are
>> >> >>>>> functions.
>> >> >>>>> Check line:
>> >> >>>>
>> >> >>>> That function is only going to be used in checkNumeric function
>> >> >>>> (in
>> >> >>>> case
>> >> >>>> of Number control) and checkInt function (in case of Integer
>> >> >>>> control)
>> >> >>>> so
>> >> >>>> declared them locally.
>> >> >>>> Anyway I'm going to refactor both the controls as Number and
>> >> >>>> Integer
>> >> >>>> shares some common properties.
>> >> >>>>
>> >> >>>>> +++ b/web/pgadmin/static/js/backform.pgadmin.js
>> >> >>>>> @@ -1528,7 +1528,18 @@
>> >> >>>>>            max_value = field.max,
>> >> >>>>>            isValid = true,
>> >> >>>>>            intPattern = new RegExp("^-?[0-9]*$"),
>> >> >>>>> -          isMatched = intPattern.test(value);
>> >> >>>>> +          isMatched = intPattern.test(value),
>> >> >>>>> +          trigger_invalid_event = function(msg) {
>> >> >>>>>
>> >> >>>>> 2) The functions added in both places look very similar, can they
>> >> >>>>> be
>> >> >>>>> merged and extracted? We are talking about the
>> >> >>>>> trigger_invalid_event
>> >> >>>>> function.
>> >> >>>>
>> >> >>>> Yes they can be merged. As of now both NumericControl and
>> >> >>>> IntegerControl
>> >> >>>> are derived from InputControl. Ideally
>> >> >>>> only NumericControl should be derived from InputControl and
>> >> >>>> IntegerControl should be derive from NumericControl.
>> >> >>>>
>> >> >>>>
>> >> >>>>>
>> >> >>>>> 3) The following change is very similar to the
>> >> >>>>> trigger_invalid_event,
>> >> >>>>> was there a reason not to use it?
>> >> >>>>
>> >> >>>> Below code triggers "model valid" event; opposite to "model
>> >> >>>> invalid"
>> >> >>>> event (trigger_invalid_event)
>> >> >>>>>
>> >> >>>>> +++ b/web/pgadmin/static/js/backform.pgadmin.js
>> >> >>>>> @@ -1573,25 +1584,23 @@
>> >> >>>>>          this.model.errorModel.unset(name);
>> >> >>>>>          this.model.set(name, value);
>> >> >>>>>          this.listenTo(this.model, "change:" + name,
>> >> >>>>> this.render);
>> >> >>>>> -        if (this.model.collection || this.model.handler) {
>> >> >>>>> -          (this.model.collection || this.model.handler).trigger(
>> >> >>>>> -             'pgadmin-session:model:valid', this.model,
>> >> >>>>> (this.model.collection || this.model.handler)
>> >> >>>>> -            );
>> >> >>>>> +        // Check if other fields of same model are valid before
>> >> >>>>> +        // triggering 'session:valid' event
>> >> >>>>> +        if(_.size(this.model.errorModel.attributes) == 0) {
>> >> >>>>> +          if (this.model.collection || this.model.handler) {
>> >> >>>>> +            (this.model.collection ||
>> >> >>>>> this.model.handler).trigger(
>> >> >>>>> +               'pgadmin-session:model:valid', this.model,
>> >> >>>>> (this.model.collection || this.model.handler)
>> >> >>>>> +              );
>> >> >>>>> +          } else {
>> >> >>>>> +            (this.model).trigger(
>> >> >>>>> +               'pgadmin-session:valid',
>> >> >>>>> this.model.sessChanged(),
>> >> >>>>> this.model
>> >> >>>>> +              );
>> >> >>>>> +          }
>> >> >>>>>
>> >> >>>>> 4) We also noticed that the following change sets look very
>> >> >>>>> similiar.
>> >> >>>>> Is there any reason to have this code duplicated? If not this
>> >> >>>>> could
>> >> >>>>> be a
>> >> >>>>> good time to refactor it.
>> >> >>>>
>> >> >>>> As said earlier in response of point 2 code duplication is because
>> >> >>>> the
>> >> >>>> way controls are derived.
>> >> >>>>
>> >> >>>>>
>> >> >>>>> +++ b/web/pgadmin/static/js/backform.pgadmin.js
>> >> >>>>> @@ -1528,7 +1528,18 @@
>> >> >>>>>
>> >> >>>>> @@ -1573,25 +1584,23 @@
>> >> >>>>>
>> >> >>>>> @@ -1631,7 +1640,18 @@
>> >> >>>>>
>> >> >>>>> @@ -1676,25 +1696,23 @@
>> >> >>>>>
>> >> >>>>>
>> >> >>>>> Thanks
>> >> >>>>> Joao & Shruti
>> >> >>>>>
>> >> >>>>> On Thu, May 18, 2017 at 6:01 AM, Harshal Dhumal
>> >> >>>>> <harshal.dhumal@enterprisedb.com> wrote:
>> >> >>>>>>
>> >> >>>>>> Hi,
>> >> >>>>>>
>> >> >>>>>> Please find attached patch for RM2421
>> >> >>>>>>
>> >> >>>>>> Issue fixed: 1. Integer/numeric Validation is not working
>> >> >>>>>> properly.
>> >> >>>>>> 2. Wrong CPU rate unit
>> >> >>>>>> --
>> >> >>>>>> Harshal Dhumal
>> >> >>>>>> Sr. Software Engineer
>> >> >>>>>>
>> >> >>>>>> EnterpriseDB India: http://www.enterprisedb.com
>> >> >>>>>> The Enterprise PostgreSQL Company
>> >> >>>>>>
>> >> >>>>>>
>> >> >>>>>> --
>> >> >>>>>> Sent via pgadmin-hackers mailing list
>> >> >>>>>> (pgadmin-hackers@postgresql.org)
>> >> >>>>>> To make changes to your subscription:
>> >> >>>>>> http://www.postgresql.org/mailpref/pgadmin-hackers
>> >> >>>>>>
>> >> >>>>>
>> >> >>>>
>> >> >>>
>> >> >>
>> >> >
>> >> >
>> >> >
>> >> > --
>> >> > Sent via pgadmin-hackers mailing list
>> >> > (pgadmin-hackers@postgresql.org)
>> >> > To make changes to your subscription:
>> >> > http://www.postgresql.org/mailpref/pgadmin-hackers
>> >> >
>> >>
>> >>
>> >>
>> >> --
>> >> Dave Page
>> >> Blog: http://pgsnake.blogspot.com
>> >> Twitter: @pgsnake
>> >>
>> >> EnterpriseDB UK: http://www.enterprisedb.com
>> >> The Enterprise PostgreSQL Company
>> >
>> >
>>
>>
>>
>> --
>> Dave Page
>> Blog: http://pgsnake.blogspot.com
>> Twitter: @pgsnake
>>
>> EnterpriseDB UK: http://www.enterprisedb.com
>> The Enterprise PostgreSQL Company
>
>



--
Dave Page
Blog: http://pgsnake.blogspot.com
Twitter: @pgsnake

EnterpriseDB UK: http://www.enterprisedb.com
The Enterprise PostgreSQL Company