Re: Fix for RM2421 [pgAdmin4][patch]

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(dot)dhumal(at)enterprisedb(dot)com> wrote:

> Hi,
>
> On Thu, May 18, 2017 at 7:57 PM, Joao Pedro De Almeida Pereira <
> jdealmeidapereira(at)pivotal(dot)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(dot)dhumal(at)enterprisedb(dot)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(at)postgresql(dot)org)
>>> To make changes to your subscription:
>>> http://www.postgresql.org/mailpref/pgadmin-hackers
>>>
>>>
>>
>