Re: Fix for RM2421 [pgAdmin4][patch]

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 <dpage(at)pgadmin(dot)org> wrote:

> Can you rebase this please? I think Ashesh broke it :-p
>
> On Tue, Jun 6, 2017 at 7:42 AM, Harshal Dhumal
> <harshal(dot)dhumal(at)enterprisedb(dot)com> wrote:
> > Hi,
> >
> > On Mon, Jun 5, 2017 at 9:25 PM, Dave Page <dpage(at)pgadmin(dot)org> 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
> >> <harshal(dot)dhumal(at)enterprisedb(dot)com> 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
> >> > <harshal(dot)dhumal(at)enterprisedb(dot)com> 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
> >> >> <harshal(dot)dhumal(at)enterprisedb(dot)com> 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
> >> >>> <harshal(dot)dhumal(at)enterprisedb(dot)com> wrote:
> >> >>>>
> >> >>>> Hi,
> >> >>>>
> >> >>>> On Thu, May 18, 2017 at 7:57 PM, Joao Pedro De Almeida Pereira
> >> >>>> <jdealmeidapereira(at)pivotal(dot)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 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
> >> >>>>> <harshal(dot)dhumal(at)enterprisedb(dot)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(at)postgresql(dot)org)
> >> >>>>>> To make changes to your subscription:
> >> >>>>>> http://www.postgresql.org/mailpref/pgadmin-hackers
> >> >>>>>>
> >> >>>>>
> >> >>>>
> >> >>>
> >> >>
> >> >
> >> >
> >> >
> >> > --
> >> > Sent via pgadmin-hackers mailing list (pgadmin-hackers(at)postgresql(dot)org
> )
> >> > 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
>