MIME-Version: 1.0
In-Reply-To: 
 <CAFiP3vy620AnY=Smqt92hV=CoPrEhFVR6z6EXLtMHbZ9bw8eTw@mail.gmail.com>
References: 
 <CAFiP3vx-B-8LzOQ+hiP0SNTJ22CqsXM2hLJUUgJWYm6XVtgMDQ@mail.gmail.com>
 <CAE+jja=+qP+rTkXf=Gcciyiyz7B2ep5f-YO49Nf_5Aq_zx9t=A@mail.gmail.com>
 <CAFiP3vzxqm6Utwf4uWXZ=B267W4apToLG8WYVcjG6Q9xSNqgGQ@mail.gmail.com>
 <CAFiP3vwJf5j=u+rq9NY2tiGL2NxqMjKeHv2=OOpc17eM-LdENg@mail.gmail.com>
 <CAFiP3vwR206Rv9-889zNchRQot=CpybiB54_1HnpWjQ4ytVUuA@mail.gmail.com>
 <CAFiP3vyr0=6b3xaHJ0zNcQCDFCxSaunNb=tRHJmQMgs=e54N=w@mail.gmail.com>
 <CA+OCxozZS46R9sEvHz+H007ycL0xQA2WaRQBN_ForLAXo5sHBQ@mail.gmail.com>
 <CAFiP3vy620AnY=Smqt92hV=CoPrEhFVR6z6EXLtMHbZ9bw8eTw@mail.gmail.com>
From: Dave Page <dpage@pgadmin.org>
Date: Wed, 7 Jun 2017 14:29:09 +0100
Message-ID: 
 <CA+OCxoyosM-33aod9hCq9wuHeEU_C2K1hS7wZ2r6AAhJvkXiJg@mail.gmail.com>
Subject: Re: Fix for RM2421 [pgAdmin4][patch]
To: Harshal Dhumal <harshal.dhumal@enterprisedb.com>
Cc: pgadmin-hackers <pgadmin-hackers@postgresql.org>
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk
Sender: pgadmin-hackers-owner@postgresql.org

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


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