public inbox for [email protected]  

help / color / mirror / Atom feed

From: Joao Pedro De Almeida Pereira <[email protected]>
To: Harshal Dhumal <[email protected]>
Cc: pgadmin-hackers <[email protected]>
Cc: Shruti B Iyer <[email protected]>
Subject: Re: Fix for RM2421 [pgAdmin4][patch]
Date: Thu, 18 May 2017 10:27:40 -0400
Message-ID: <CAE+jja=+qP+rTkXf=Gcciyiyz7B2ep5f-YO49Nf_5Aq_zx9t=A@mail.gmail.com> (raw)
In-Reply-To: <CAFiP3vx-B-8LzOQ+hiP0SNTJ22CqsXM2hLJUUgJWYm6XVtgMDQ@mail.gmail.com>
References: <CAFiP3vx-B-8LzOQ+hiP0SNTJ22CqsXM2hLJUUgJWYm6XVtgMDQ@mail.gmail.com>
List-Unsubscribe:  <mailto:[email protected]?body=unsub%20pgadmin-hackers>

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 = 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.

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) == 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.

+++ 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 <
[email protected]> 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 ([email protected])
> To make changes to your subscription:
> http://www.postgresql.org/mailpref/pgadmin-hackers
>
>

view thread (11+ messages)  latest in thread

reply

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Reply to all the recipients using the --to and --cc options:
  reply via email

  To: [email protected]
  Cc: [email protected], [email protected], [email protected]
  Subject: Re: Fix for RM2421 [pgAdmin4][patch]
  In-Reply-To: <CAE+jja=+qP+rTkXf=Gcciyiyz7B2ep5f-YO49Nf_5Aq_zx9t=A@mail.gmail.com>

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

This inbox is served by agora; see mirroring instructions
for how to clone and mirror all data and code used for this inbox