Re: Fix for RM2421 [pgAdmin4][patch]

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