Received: from malur.postgresql.org ([217.196.149.56]) by arkaria.postgresql.org with esmtp (Exim 4.84_2) (envelope-from ) id 1dBMP5-00051q-Js for pgadmin-hackers@arkaria.postgresql.org; Thu, 18 May 2017 14:27:47 +0000 Received: from localhost ([127.0.0.1] helo=postgresql.org) by malur.postgresql.org with smtp (Exim 4.84_2) (envelope-from ) id 1dBMP5-0003Ly-3G for pgadmin-hackers@arkaria.postgresql.org; Thu, 18 May 2017 14:27:47 +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 1dBMP4-0003IK-0M for pgadmin-hackers@postgresql.org; Thu, 18 May 2017 14:27:46 +0000 Received: from mail-vk0-x22b.google.com ([2607:f8b0:400c:c05::22b]) by makus.postgresql.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_CBC_SHA1:256) (Exim 4.84_2) (envelope-from ) id 1dBMP0-00066b-UX for pgadmin-hackers@postgresql.org; Thu, 18 May 2017 14:27:44 +0000 Received: by mail-vk0-x22b.google.com with SMTP id x71so24203786vkd.0 for ; Thu, 18 May 2017 07:27:41 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=pivotal-io.20150623.gappssmtp.com; s=20150623; h=mime-version:in-reply-to:references:from:date:message-id:subject:to :cc; bh=04Kbf7tTtFhrPQWMZrDprlskhpZiv8RnQPOSi6xpYMQ=; b=XLdmclOyI/PDYGSdTWiPRajGmHiXYeJcxQYRkTSvdrlL+UBw0VkQF6NjqcKRq6YUEE fmKM127lqfQUa7P6SRgqaQrUw+dYga42MuB1CV2yobZiVKZt0JjPqHWMLFLc0w9e1SMh wVrkkpYGA1D8K+l4zEMYcC6EOCVUhvYiAV83NW0lS+mp/ADw1vRaC9/yyUHzRBxtJMgq 8tLozevarhUZtjV6ADHaIvGhxneqh1oC77lKhYhmvmaGX/JNCVNCS6splOqIZrjrTR0+ cCoRAJW3kdIFwkEa8s1/XVr8tlnhqQyri5g0aBV2VCV6MUFn6Scya09UylFb2ao+j2PF pK4Q== 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=04Kbf7tTtFhrPQWMZrDprlskhpZiv8RnQPOSi6xpYMQ=; b=moWCOl5Jwq8o1nNHaCrZx/c0AsFteb8INVGmRe36naeLrIRP2xKRxZFRHJ9gNqsdjE xAGAaJn6ZP0U+B22NxGkJlm8KwfbsNHQQFK7VmRT4Ml4l9Xxk3B855fXSTJdTb++oVoW vga8CN2pPwSUj88/5i5RCnbz/bAbbTkXyzfrXjKu9ebZPbKJq6InriaroEbbA12VSeyl ZLtiQhWll2hcL0Sprj/TUTzfxD5DVfvVyGY3TKYxOW/rPmnEAX9caFzglS2srf7wqzXR wemVrVuAaKvI6bytyBu+9W2mIOboQWrsR+6QZ+gnmEasGQ+si2pbVcw9oy8fShORoICL YDhg== X-Gm-Message-State: AODbwcBF7ROFlJKiuuikG5Fj7mKzTg5qxiYo63FbZekDtBOgT9zwHxvF GJW9yOaVn84ysb9GOeyCUwzFEDoN2Mcj X-Received: by 10.31.242.79 with SMTP id q76mr2211758vkh.128.1495117661313; Thu, 18 May 2017 07:27:41 -0700 (PDT) MIME-Version: 1.0 Received: by 10.103.47.13 with HTTP; Thu, 18 May 2017 07:27:40 -0700 (PDT) In-Reply-To: References: From: Joao Pedro De Almeida Pereira Date: Thu, 18 May 2017 10:27:40 -0400 Message-ID: Subject: Re: Fix for RM2421 [pgAdmin4][patch] To: Harshal Dhumal Cc: pgadmin-hackers , Shruti B Iyer Content-Type: multipart/alternative; boundary="94eb2c19abda19c09e054fcd3594" 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 --94eb2c19abda19c09e054fcd3594 Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable 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: +++ 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. 3) The following change is very similar to the trigger_invalid_event, was there a reason not to use it? +++ 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) =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.mod= el + ); + } =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 good time to refactor it. +++ 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 > > --94eb2c19abda19c09e054fcd3594 Content-Type: text/html; charset="UTF-8" Content-Transfer-Encoding: quoted-printable
Hello Harshal,

We review the patch and = have some questions:
1) Is there any particular reason to initial= ize variables and functions in the same place? We believe that it would be = more readable there were no chaining of variable creation, specially if tho= se variables are functions. Check line:=C2=A0
+++ 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.

3) The following change is very s= imilar to the trigger_invalid_event, was there a reason not to use it?
<=
code style=3D"font-size:0.85em;font-family:Consolas,Inconsolata,Courier,mon=
ospace;margin:0px 0.15em;background-color:rgb(248,248,248);white-space:pre;=
overflow:auto;border-radius:3px;border:1px solid rgb(204,204,204);padding:0=
.5em 0.7em;display:block">+++ 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.
=E2=80=8B

Thanks=
Joao & Shruti

On Thu, May 18, 2017 at 6:01 AM, Harshal Dhumal <harshal.dhumal@enterprisedb.com> wrote:
Hi,

Please fin= d attached patch for RM2421

Issue fixed: 1. Intege= r/numeric Validation is not working properly.
2. Wrong CPU rate u= nit
--=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


--94eb2c19abda19c09e054fcd3594--