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 >>>> >>>> >>> >> >
