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

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