public inbox for [email protected]
help / color / mirror / Atom feedFrom: Harshal Dhumal <[email protected]>
To: Joao Pedro De Almeida Pereira <[email protected]>
Cc: pgadmin-hackers <[email protected]>
Cc: Shruti B Iyer <[email protected]>
Subject: Re: Fix for RM2421 [pgAdmin4][patch]
Date: Tue, 30 May 2017 12:30:07 +0530
Message-ID: <CAFiP3vwR206Rv9-889zNchRQot=CpybiB54_1HnpWjQ4ytVUuA@mail.gmail.com> (raw)
In-Reply-To: <CAFiP3vwJf5j=u+rq9NY2tiGL2NxqMjKeHv2=OOpc17eM-LdENg@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>
List-Unsubscribe: <mailto:[email protected]?body=unsub%20pgadmin-hackers>
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 <
[email protected]> 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 <
> [email protected]> wrote:
>
>> Hi,
>>
>> On Thu, May 18, 2017 at 7:57 PM, Joao Pedro De Almeida Pereira <
>> [email protected]> 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 <
>>> [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: <CAFiP3vwR206Rv9-889zNchRQot=CpybiB54_1HnpWjQ4ytVUuA@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