Re: [pgadmin-hackers] Fix for RM2421 [pgAdmin4][patch]
Thanks, applied. On Thu, Jun 8, 2017 at 1:59 PM, Harshal Dhumal 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 wrote: >> >> Can you rebase this please? I think Ashesh broke it :-p >> >> On Tue, Jun 6, 2017 at 7:42 AM, Harshal Dhumal >> wrote: >> > Hi, >> > >> > On Mon, Jun 5, 2017 at 9:25 PM, Dave Page 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 >> >> 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 >> >> > 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 >> >> >> 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 >> >> >>> wrote: >> >> >> >> Hi, >> >> >> >> On Thu, May 18, 2017 at 7:57 PM, Joao Pedro De Almeida Pereira >> >> 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); >> >> >
Re: [pgadmin-hackers] 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 wrote: > Can you rebase this please? I think Ashesh broke it :-p > > On Tue, Jun 6, 2017 at 7:42 AM, Harshal Dhumal > wrote: > > Hi, > > > > On Mon, Jun 5, 2017 at 9:25 PM, Dave Page 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 > >> 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 > >> > 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 > >> >> 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 > >> >>> wrote: > >> > >> Hi, > >> > >> On Thu, May 18, 2017 at 7:57 PM, Joao Pedro De Almeida Pereira > >> 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 sa
Re: [pgadmin-hackers] Fix for RM2421 [pgAdmin4][patch]
Can you rebase this please? I think Ashesh broke it :-p On Tue, Jun 6, 2017 at 7:42 AM, Harshal Dhumal wrote: > Hi, > > On Mon, Jun 5, 2017 at 9:25 PM, Dave Page 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 >> 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 >> > 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 >> >> 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 >> >>> wrote: >> >> Hi, >> >> On Thu, May 18, 2017 at 7:57 PM, Joao Pedro De Almeida Pereira >> 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(
Re: [pgadmin-hackers] Fix for RM2421 [pgAdmin4][patch]
Hi, On Mon, Jun 5, 2017 at 9:25 PM, Dave Page 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 > 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 > > 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 > >> 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 > >>> wrote: > > Hi, > > On Thu, May 18, 2017 at 7:57 PM, Joao Pedro De Almeida Pereira > 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 >
Re: [pgadmin-hackers] Fix for RM2421 [pgAdmin4][patch]
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. Thanks. On Tue, May 30, 2017 at 8:28 AM, Harshal Dhumal 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 > 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 >> 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 >>> wrote: Hi, On Thu, May 18, 2017 at 7:57 PM, Joao Pedro De Almeida Pereira 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 > >>>
Re: [pgadmin-hackers] Fix for RM2421 [pgAdmin4][patch]
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.dhu...@enterprisedb.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.dhu...@enterprisedb.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.dhu...@enterprisedb.com> wrote: >> >>> Hi, >>> >>> On Thu, May 18, 2017 at 7:57 PM, Joao Pedro De Almeida Pereira < >>> jdealmeidapere...@pivotal.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.dhu...@enterprisedb.com> wrote: > Hi, > > Please find attached patch for RM2421 > > Issue fixed: 1. Integer/numeric Validation is not working properly. >
Re: [pgadmin-hackers] Fix for RM2421 [pgAdmin4][patch]
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.dhu...@enterprisedb.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.dhu...@enterprisedb.com> wrote: > >> Hi, >> >> On Thu, May 18, 2017 at 7:57 PM, Joao Pedro De Almeida Pereira < >> jdealmeidapere...@pivotal.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.dhu...@enterprisedb.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@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgadmin-hackers >>> >> >
Re: [pgadmin-hackers] Fix for RM2421 [pgAdmin4][patch]
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.dhu...@enterprisedb.com> wrote: > Hi, > > On Thu, May 18, 2017 at 7:57 PM, Joao Pedro De Almeida Pereira < > jdealmeidapere...@pivotal.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.dhu...@enterprisedb.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@postgresql.org) >>> To make changes to your subscription: >>> http://www.postgresql.org/mailpref/pgadmin-hackers >>> >>> >> > diff --git a/web/pgadmin/browser/server_groups/servers/databases/schemas/foreign_tables/templates/foreign_tables/js/foreign_tables.js b/web/pgadmin/browser/server_groups/servers/databases/schemas/foreign_tables/templates/foreign_tables/js/foreign_tables.js index 5f3dc69..b7df585 100644 --- a/web/pgadmin/browser/server_groups/servers/databases/schemas/foreign_tables/templates/foreign_tables/js/foreign_tables.js +++ b/web/pgadmin/browser/server_groups/servers/databases/schemas/fo
Re: [pgadmin-hackers] Fix for RM2421 [pgAdmin4][patch]
Hi, On Thu, May 18, 2017 at 7:57 PM, Joao Pedro De Almeida Pereira < jdealmeidapere...@pivotal.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.dhu...@enterprisedb.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@postgresql.org) >> To make changes to your subscription: >> http://www.postgresql.org/mailpref/pgadmin-hackers >> >> >
Re: [pgadmin-hackers] Fix for RM2421 [pgAdmin4][patch]
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 < harshal.dhu...@enterprisedb.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@postgresql.org) > To make changes to your subscription: > http://www.postgresql.org/mailpref/pgadmin-hackers > >
[pgadmin-hackers] Fix for RM2421 [pgAdmin4][patch]
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 diff --git a/web/pgadmin/browser/server_groups/servers/resource_groups/templates/resource_groups/js/resource_groups.js b/web/pgadmin/browser/server_groups/servers/resource_groups/templates/resource_groups/js/resource_groups.js index 3cc822e..596a4ee 100644 --- a/web/pgadmin/browser/server_groups/servers/resource_groups/templates/resource_groups/js/resource_groups.js +++ b/web/pgadmin/browser/server_groups/servers/resource_groups/templates/resource_groups/js/resource_groups.js @@ -72,7 +72,7 @@ define( id: 'name', label: '{{ _('Name') }}', cell: 'string', type: 'text', },{ - id: 'cpu_rate_limit', label:'{{ _('CPU rate limit (%%)') }}', cell: 'string', + id: 'cpu_rate_limit', label:'{{ _('CPU rate limit (%)') }}', cell: 'string', type: 'numeric', min:0, max:16777216 },{ id: 'dirty_rate_limit', label:'{{ _('Dirty rate limit (KB)') }}', cell: 'string', diff --git a/web/pgadmin/static/js/backform.pgadmin.js b/web/pgadmin/static/js/backform.pgadmin.js index 553676e..a822da8 100644 --- a/web/pgadmin/static/js/backform.pgadmin.js +++ 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) { +if (this.model.collection || this.model.handler) { + (this.model.collection || this.model.handler).trigger( + 'pgadmin-session:model:invalid', msg, this.model +); +} else { + (this.model).trigger( + 'pgadmin-session:invalid', msg, this.model +); +} + }.bind(this); // Below logic will validate input if (!isMatched) { @@ -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 + ); + } } else { - (this.model).trigger( - 'pgadmin-session:valid', this.model.sessChanged(), this.model -); + trigger_invalid_event(_.values(this.model.errorModel.attributes)[0]); } } else { -if (this.model.collection || this.model.handler) { - (this.model.collection || this.model.handler).trigger( - 'pgadmin-session:model:invalid', this.model.errorModel.get(name), this.model -); -} else { - (this.model).trigger( - 'pgadmin-session:invalid', this.model.errorModel.get(name), this.model -); -} +trigger_invalid_event(this.model.errorModel.get(name)); } } }); @@ -1631,7 +1640,18 @@ max_value = field.max, isValid = true, intPattern = new RegExp("^-?[0-9]+(\.?[0-9]*)?$"), - isMatched = intPattern.test(value); + isMatched = intPattern.test(value), + trigger_invalid_event = function(msg) { +if (this.model.collection || this.model.handler) { + (this.model.collection || this.model.handler).trigger( + 'pgadmin-session:model:invalid', msg, this.model +); +} else { + (this.model).trigger( + 'pgadmin-session:invalid', msg, this.model +); +} + }.bind(this); // Below logic will validate input if (!isMatched) { @@ -1676,25 +1696,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( - 'pgadm