Received: from malur.postgresql.org ([217.196.149.56]) by arkaria.postgresql.org with esmtp (Exim 4.84_2) (envelope-from ) id 1dBbmO-0004yU-9K for pgadmin-hackers@arkaria.postgresql.org; Fri, 19 May 2017 06:52:52 +0000 Received: from localhost ([127.0.0.1] helo=postgresql.org) by malur.postgresql.org with smtp (Exim 4.84_2) (envelope-from ) id 1dBbmN-0001oW-SK for pgadmin-hackers@arkaria.postgresql.org; Fri, 19 May 2017 06:52:51 +0000 Received: from makus.postgresql.org ([2001:4800:1501:1::229]) by malur.postgresql.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_CBC_SHA384:256) (Exim 4.84_2) (envelope-from ) id 1dBbmM-0001kk-PZ for pgadmin-hackers@postgresql.org; Fri, 19 May 2017 06:52:51 +0000 Received: from mail-it0-x22d.google.com ([2607:f8b0:4001:c0b::22d]) by makus.postgresql.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_CBC_SHA1:256) (Exim 4.84_2) (envelope-from ) id 1dBbmE-0007sj-Ur for pgadmin-hackers@postgresql.org; Fri, 19 May 2017 06:52:49 +0000 Received: by mail-it0-x22d.google.com with SMTP id g126so41551952ith.0 for ; Thu, 18 May 2017 23:52:42 -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=LxWTRLJcDBGpESQSkNrDaXU7YEl6CVkro2xQ0b4kX94=; b=HJ22Jk6TtsW2lirTcv4lMJPDwTCEN2OeuPBABMn229xcrwFamWIiCbvBsonOMUkjU3 BzDvLALAqqtVc8RBssnSh/6BtRwY+5d/QfqNVFllhpuWMtUIkzc4dHDtF329V/q9/CLN 7HY1wyP2thFwSgtaC4Vg17nmxWpDUuTa3nTRRl341XmDnFKR7inAo02fDwtyEy5+XoZd jj7+hL0mR3O3izXAWwZkGYVBTTZLGZAjTicELlGFz5FtUESQwnHad2vbx6T2EXZJqrHW gSTV6+EgP3+jCBEyILJbhcB39UKR5vJpA05S2xJ3ULdWTSc74SkDYyrXMzW50zZAjjbv I4nw== 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=LxWTRLJcDBGpESQSkNrDaXU7YEl6CVkro2xQ0b4kX94=; b=p3jPv2lNwWEelsPasHAzj5KNxne4Df50c8HAtj5gYeqSEsdmDMX0CCzMdLwFEy8nSv v6DUG5yWABfpa8D/48716fdPyrLBo6axqjeWO1XIOF4opnZbIHuj+UXQVMoDfX293WbI gcfrf+9T3QAa1CGHlJb6wyHH7uWTM951M0bjvgSudw0ucqgiPKBQ7bXM8lQRWNB1gq+r vhR3ZZbzmF8ZMFa96drB/9UYlZcXkpAnksKqCGoyRyL03+1z8EWWbrVWt1ypc3q0hDmw NPaWNUqnL0xWbecosDJetASP8+9Q3NGUBDteEC8mIyNuTckDpMuxal4kgtqxFwaNhjUx SXOg== X-Gm-Message-State: AODbwcCqHq3HFaQFxDOeIs+43BWWz7EAfc6pTsakhWvpAwXyjdR1uFLB VYbLtxxX9TSxwNG1tsbnNpCGGfEcz7PA X-Received: by 10.36.237.201 with SMTP id r192mr10458093ith.84.1495176761757; Thu, 18 May 2017 23:52:41 -0700 (PDT) MIME-Version: 1.0 Received: by 10.79.38.15 with HTTP; Thu, 18 May 2017 23:52:41 -0700 (PDT) In-Reply-To: References: From: Harshal Dhumal Date: Fri, 19 May 2017 12:22:41 +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="f403045c08b6c2f450054fdaf7d2" 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 --f403045c08b6c2f450054fdaf7d2 Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable 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 i= n > 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 =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.coll= ection || 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.co= llection || this.model.handler) > + ); > + } else { > + (this.model).trigger( > + 'pgadmin-session:valid', this.model.sessChanged(), this.m= odel > + ); > + } > > =E2=80=8B > 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 goo= d > 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 >> >> > --f403045c08b6c2f450054fdaf7d2 Content-Type: text/html; charset="UTF-8" Content-Transfer-Encoding: quoted-printable
Hi,

On Thu, May 18, 2017 at 7:57 PM, Joao Pedro D= e 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 pl= ace? We believe that it would be more readable there were no chaining of va= riable creation, specially 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 Integer control) so declared them locally.
Anyway I'= m going to refactor both the controls as Number and Integer shares some com= mon properties.

=
+++ b/web/pgadmin/static/js/backform.p=
gadmin.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 merged. As o= f now both NumericControl and IntegerControl are derived from InputControl.= Ideally=C2=A0
only NumericControl should be derived from InputCo= ntrol and IntegerControl should be derive from NumericControl.
=C2=A0
=
3) The following change is very similar to= the trigger_invalid_event, was = there a reason not to use it?
Below code trigg= ers "model valid" event; opposite to "model invalid" ev= ent (trigger_invalid_event<= /span>)
+++ 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 earlier in = response of point 2 code duplication is because the way controls are derive= d.
=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 Dhumal <harshal.dhumal@enterpr= isedb.com> wrote:
Hi= ,

Please find attached patch for RM2421

Issue fixed: 1. Integer/numeric Validation is not working properly= .
2. Wrong CPU rate unit
=
--=C2=A0<= /div>
Harshal Dhumal
Sr. Software Engineer

EnterpriseDB India:=C2=A0ht= tp://www.enterprisedb.com
The En= terprise PostgreSQL Company
<= /div>


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



--f403045c08b6c2f450054fdaf7d2--