Received: from malur.postgresql.org ([217.196.149.56]) by arkaria.postgresql.org with esmtp (Exim 4.84_2) (envelope-from ) id 1dFb8s-0000kX-0e for pgadmin-hackers@arkaria.postgresql.org; Tue, 30 May 2017 07:00:34 +0000 Received: from localhost ([127.0.0.1] helo=postgresql.org) by malur.postgresql.org with smtp (Exim 4.84_2) (envelope-from ) id 1dFb8r-00057F-GJ for pgadmin-hackers@arkaria.postgresql.org; Tue, 30 May 2017 07:00:33 +0000 Received: from magus.postgresql.org ([2a02:c0:301:0:ffff::29]) by malur.postgresql.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_CBC_SHA384:256) (Exim 4.84_2) (envelope-from ) id 1dFb8b-0004h7-Ur for pgadmin-hackers@postgresql.org; Tue, 30 May 2017 07:00:18 +0000 Received: from mail-io0-x234.google.com ([2607:f8b0:4001:c06::234]) by magus.postgresql.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_CBC_SHA1:256) (Exim 4.84_2) (envelope-from ) id 1dFb8U-0001Lp-Qu for pgadmin-hackers@postgresql.org; Tue, 30 May 2017 07:00:17 +0000 Received: by mail-io0-x234.google.com with SMTP id p24so51370866ioi.0 for ; Tue, 30 May 2017 00:00:10 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=enterprisedb-com.20150623.gappssmtp.com; s=20150623; h=mime-version:in-reply-to:references:from:date:message-id:subject:to :cc; bh=BhMZUc+JC+abwiMp911xBxvog5Som9eYvXn+WtZ/4To=; b=rqUwgyaZSu9XsGxN0KC6g3bRFCICmPiS6Ujfq4FUnpJqVoX/9X2KdR493cDfQZtCML b9KgkeF86U6TIJYDaOJ7SorzndrWYX/jd1hsNzseDiGkO1laxG4RJuv/M7oktnOkfCl9 Oz2PqDgtx68x0ZOUMm0HgrHGLkfgqc9nRgos+QIm2oVchsZtTv0Nbv9qK4XBl1WjnE74 dgVa+wOm7s2BXF2zqCgbtOt8djNR9y5ox5EGBfLC6TbkdnTVm8Ql89hAD/foR5H6Z9Dq 9Esjqe5aIfmneRVFo2WTaK/MImiUIs5gfu0aDGz483F4hjeQBrd29ZiCPLZYSSXoA5/v WErA== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:in-reply-to:references:from:date :message-id:subject:to:cc; bh=BhMZUc+JC+abwiMp911xBxvog5Som9eYvXn+WtZ/4To=; b=BFx1HDTH0asTGBZ612VycfifZ6oCu5l2Maicf2LgOMBoOlTaLPlqZUL5KB2eRIO4nC SmvoTDl42cNSS0psbDPrzhSchKb+NwG96CTaYsz7iy/BrP8es9do8iKOTOLfVnpuAvNQ e2zPln7gEazCBoAK23hYlrvoLR7SQKv3tpMLYNlwwmOD82UV+5b/+6j6Adu6BdgrMk2y JC23EQG25LWNRvI6abByaZMehdWr6v2TIEFL4MBu2ckfBAYBOW3YgUAdYVM0UNIbvPFp QBRB3MqpRZbwmMzN6YJwqyYqXXwVD6Y6VMFIjgDM6QVLiBwv4+kvVL8/aM3tqSpVLsb1 D/RA== X-Gm-Message-State: AODbwcD9qfA3JgpnpSxh8fyMKD3zaJ0JWibOZhIgrEO2k/d2Kn0o7bj5 +oI8hTMwMe6JFZGwFXidLAawNlJgWzn1 X-Received: by 10.107.6.218 with SMTP id f87mr15714773ioi.2.1496127607849; Tue, 30 May 2017 00:00:07 -0700 (PDT) MIME-Version: 1.0 Received: by 10.79.38.15 with HTTP; Tue, 30 May 2017 00:00:07 -0700 (PDT) In-Reply-To: References: From: Harshal Dhumal Date: Tue, 30 May 2017 12:30:07 +0530 Message-ID: Subject: Re: Fix for RM2421 [pgAdmin4][patch] To: Joao Pedro De Almeida Pereira Cc: pgadmin-hackers , Shruti B Iyer Content-Type: multipart/alternative; boundary="001a113fb1ca9adc740550b85a20" X-Pg-Spam-Score: -2.6 (--) List-Archive: List-Help: List-ID: List-Owner: List-Post: List-Subscribe: List-Unsubscribe: X-Mailing-List: pgadmin-hackers Precedence: bulk Sender: pgadmin-hackers-owner@postgresql.org --001a113fb1ca9adc740550b85a20 Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable Hi, Please ignore this patch as I forgot to include few changes. I'll send updated one. --=20 *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 functio= ns. >>> 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 =3D field.max, >>> isValid =3D true, >>> intPattern =3D new RegExp("^-?[0-9]*$"), >>> - isMatched =3D intPattern.test(value); >>> + isMatched =3D intPattern.test(value), >>> + trigger_invalid_event =3D function(msg) { >>> >>> =E2=80=8B >>> 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.co= llection || this.model.handler) >>> - ); >>> + // Check if other fields of same model are valid before >>> + // triggering 'session:valid' event >>> + if(_.size(this.model.errorModel.attributes) =3D=3D 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 >>> + ); >>> + } >>> >>> =E2=80=8B >>> 4) We also noticed that the following change sets look very similiar. I= s >>> there any reason to have this code duplicated? If not this could be a g= ood >>> 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 @@ >>> >>> =E2=80=8B >>> >>> 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 >>>> >>>> >>> >> > --001a113fb1ca9adc740550b85a20 Content-Type: text/html; charset="UTF-8" Content-Transfer-Encoding: quoted-printable
Hi,

Please ignore this patch as I forgo= t to include few changes. I'll send updated one.

=
--=C2=A0
Harshal Dhumal
Sr. Software Engineer


On Mon, May 29, 2017 at 3:18 PM, Harshal Dhu= mal <harshal.dhumal@enterprisedb.com> wrote:
Hi,

He= re is updated patch for RM2421.

Now I have moved a= ll Numeric control level validations to datamodel. As existing implementati= on was causing=C2=A0
issues with error messages in create/edit di= alog 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



=
--=C2=A0
Harshal Dhumal
Sr. Software Engineer

EnterpriseDB India:=C2=A0http://www.enterprisedb.com
The Enterprise PostgreSQL Company
<= /div>

On Fri, May 19= , 2017 at 12:22 PM, Harshal Dhumal <harshal.dhumal@enterpris= edb.com> wrote:
Hi,

On= Thu, May 18, 2017 at 7:57 PM, Joao Pedro De Almeida Pereira <j= dealmeidapereira@pivotal.io> wrote:
<= /span>
Hello Harshal,

We review = the patch and have some questions:
1) Is there any particular rea= son to initialize variables and functions in the same place? We believe tha= t it would be more readable there were no chaining of variable creation, sp= ecially if those variables are functions. Check line:=C2=A0
That function is only going to be used in checkNumeric= function (in case of Number control) and checkInt function (in case of Int= eger control) so declared them locally.
Anyway I'm going to r= efactor both the controls as Number and Integer shares some common properti= es.

+++ b/= web/pgadmin/static/js/backform.pgadmin.js @@ -1528,7 +1528,18 @@ max_value =3D field.max, isValid =3D true, intPattern =3D new RegExp("^-?[0-9]*$"), - isMatched =3D intPattern.test(value); + isMatched =3D intPattern.test(value), + trigger_invalid_event =3D function(msg) {
=E2=80=8B
2) The functions added in both pl= aces look very similar, can they be merged and extracted? We are talking ab= out the trigger_invalid_event=C2= =A0function.
Yes they can be merge= d. As of now both NumericControl and IntegerControl are derived from InputC= ontrol. Ideally=C2=A0
only NumericControl should be derived from = InputControl and IntegerControl should be derive from NumericControl.
=

=C2=A0
3) The following change is ve= ry similar to the trigger_invalid_event= , was there a reason not to use it?
<= div>Below code triggers "model valid" event; opposite to "mo= del invalid" event (tr= igger_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.mode=
l.collection || this.model.handler)
-            );
+        // Check if other fields of same model are valid before
+        // triggering 'session:valid' event
+        if(_.size(this.model.errorModel.attributes) =3D=3D 0) {
+          if (this.model.collection || this.model.handler) {
+            (this.model.collection || this.model.handler).trigger(
+               'pgadmin-session:model:valid', this.model, (this.mo=
del.collection || this.model.handler)
+              );
+          } else {
+            (this.model).trigger(
+               'pgadmin-session:valid', this.model.sessChanged(), =
this.model
+              );
+          }
=E2=80=8B
4) We also noticed that the following change sets look very similia= r. Is there any reason to have this code duplicated? If not this could be a= good time to refactor it.
As said earl= ier in response of point 2 code duplication is because the way controls are= derived.
=C2=A0
+++ b/web/pgadmin/static/js/backform.pgadmin.js
@@ -1528,7 +1528,18 @@

@@ -1573,25 +1584,23 @@

@@ -1631,7 +1640,18 @@

@@ -1676,25 +1696,23 @@
=E2=80=8B

Thanks
Joao & Shruti

On Thu, May 18, 2017 at 6:01 AM, Harshal Dh= umal <harshal.dhumal@enterprisedb.com> wr= ote:
=
Hi,

Please find attached patch for RM2421
=

Issue fixed: 1. Integer/numeric Validation is not worki= ng properly.
2. Wrong CPU rate unit
=
--=C2=A0
Harshal Dhumal
Sr. Sof= tware Engineer

EnterpriseDB India:=C2=A0http://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-ha= ckers





--001a113fb1ca9adc740550b85a20--