Re: Fix for RM2421 [pgAdmin4][patch]

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
>