Re: Fix for RM2421 [pgAdmin4][patch]

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