public inbox for [email protected]
help / color / mirror / Atom feedFrom: Dave Page <[email protected]>
To: Harshal Dhumal <[email protected]>
Cc: pgadmin-hackers <[email protected]>
Subject: Re: Fix for RM2421 [pgAdmin4][patch]
Date: Thu, 8 Jun 2017 14:59:39 +0100
Message-ID: <CA+OCxoz_=L8VSThSQQ+sFguYMHdXV57h21HmFpzMVPfb1duvTg@mail.gmail.com> (raw)
In-Reply-To: <CAFiP3vxj+gB1Yf6Br3D9h4hOCiRYS_w4X4F3ZNKn4h56=87vEA@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>
<CAFiP3vwR206Rv9-889zNchRQot=CpybiB54_1HnpWjQ4ytVUuA@mail.gmail.com>
<CAFiP3vyr0=6b3xaHJ0zNcQCDFCxSaunNb=tRHJmQMgs=e54N=w@mail.gmail.com>
<CA+OCxozZS46R9sEvHz+H007ycL0xQA2WaRQBN_ForLAXo5sHBQ@mail.gmail.com>
<CAFiP3vy620AnY=Smqt92hV=CoPrEhFVR6z6EXLtMHbZ9bw8eTw@mail.gmail.com>
<CA+OCxoyosM-33aod9hCq9wuHeEU_C2K1hS7wZ2r6AAhJvkXiJg@mail.gmail.com>
<CAFiP3vxj+gB1Yf6Br3D9h4hOCiRYS_w4X4F3ZNKn4h56=87vEA@mail.gmail.com>
List-Unsubscribe: <mailto:[email protected]?body=unsub%20pgadmin-hackers>
Thanks, applied.
On Thu, Jun 8, 2017 at 1:59 PM, Harshal Dhumal
<[email protected]> wrote:
> Please find attached rebased patch.
>
> --
> Harshal Dhumal
> Sr. Software Engineer
>
> EnterpriseDB India: http://www.enterprisedb.com
> The Enterprise PostgreSQL Company
>
> On Wed, Jun 7, 2017 at 6:59 PM, Dave Page <[email protected]> wrote:
>>
>> Can you rebase this please? I think Ashesh broke it :-p
>>
>> On Tue, Jun 6, 2017 at 7:42 AM, Harshal Dhumal
>> <[email protected]> wrote:
>> > Hi,
>> >
>> > On Mon, Jun 5, 2017 at 9:25 PM, Dave Page <[email protected]> wrote:
>> >>
>> >> Hi
>> >>
>> >> With this patch applied, it uses the field names instead of the labels
>> >> in error messages - e.g.
>> >>
>> >> 'dirty_rate_limit' must be numeric
>> >>
>> >> instead of:
>> >>
>> >> 'Dirty Rate Limit (KB)' must be numeric.
>> >
>> > Fixed. Please find attached updated patch.
>> >
>> >>
>> >>
>> >> Thanks.
>> >>
>> >> On Tue, May 30, 2017 at 8:28 AM, Harshal Dhumal
>> >> <[email protected]> wrote:
>> >> > Hi,
>> >> >
>> >> > Please find updated patch.
>> >> >
>> >> > --
>> >> > Harshal Dhumal
>> >> > Sr. Software Engineer
>> >> >
>> >> > EnterpriseDB India: http://www.enterprisedb.com
>> >> > The Enterprise PostgreSQL Company
>> >> >
>> >> > On Tue, May 30, 2017 at 12:30 PM, Harshal Dhumal
>> >> > <[email protected]> wrote:
>> >> >>
>> >> >> 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
>> >> >>>>>>
>> >> >>>>>
>> >> >>>>
>> >> >>>
>> >> >>
>> >> >
>> >> >
>> >> >
>> >> > --
>> >> > Sent via pgadmin-hackers mailing list
>> >> > ([email protected])
>> >> > To make changes to your subscription:
>> >> > http://www.postgresql.org/mailpref/pgadmin-hackers
>> >> >
>> >>
>> >>
>> >>
>> >> --
>> >> Dave Page
>> >> Blog: http://pgsnake.blogspot.com
>> >> Twitter: @pgsnake
>> >>
>> >> EnterpriseDB UK: http://www.enterprisedb.com
>> >> The Enterprise PostgreSQL Company
>> >
>> >
>>
>>
>>
>> --
>> Dave Page
>> Blog: http://pgsnake.blogspot.com
>> Twitter: @pgsnake
>>
>> EnterpriseDB UK: http://www.enterprisedb.com
>> The Enterprise PostgreSQL Company
>
>
--
Dave Page
Blog: http://pgsnake.blogspot.com
Twitter: @pgsnake
EnterpriseDB UK: 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)
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]
Subject: Re: Fix for RM2421 [pgAdmin4][patch]
In-Reply-To: <CA+OCxoz_=L8VSThSQQ+sFguYMHdXV57h21HmFpzMVPfb1duvTg@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