Re: [pgadmin-hackers] Fix for RM2421 [pgAdmin4][patch]
От | Dave Page |
---|---|
Тема | Re: [pgadmin-hackers] Fix for RM2421 [pgAdmin4][patch] |
Дата | |
Msg-id | CA+OCxoz_=L8VSThSQQ+sFguYMHdXV57h21HmFpzMVPfb1duvTg@mail.gmail.com обсуждение исходный текст |
Ответ на | Re: [pgadmin-hackers] Fix for RM2421 [pgAdmin4][patch] (Harshal Dhumal <harshal.dhumal@enterprisedb.com>) |
Список | pgadmin-hackers |
Thanks, applied. On Thu, Jun 8, 2017 at 1:59 PM, Harshal Dhumal <harshal.dhumal@enterprisedb.com> 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 <dpage@pgadmin.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.dhumal@enterprisedb.com> wrote: >> > Hi, >> > >> > On Mon, Jun 5, 2017 at 9:25 PM, Dave Page <dpage@pgadmin.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.dhumal@enterprisedb.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.dhumal@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.dhumal@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.dhumal@enterprisedb.com> wrote: >> >> >>>> >> >> >>>> Hi, >> >> >>>> >> >> >>>> On Thu, May 18, 2017 at 7:57 PM, Joao Pedro De Almeida Pereira >> >> >>>> <jdealmeidapereira@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.dhumal@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 >> >> >>>>>> >> >> >>>>> >> >> >>>> >> >> >>> >> >> >> >> >> > >> >> > >> >> > >> >> > -- >> >> > Sent via pgadmin-hackers mailing list >> >> > (pgadmin-hackers@postgresql.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 > > -- Dave Page Blog: http://pgsnake.blogspot.com Twitter: @pgsnake EnterpriseDB UK: http://www.enterprisedb.com The Enterprise PostgreSQL Company
В списке pgadmin-hackers по дате отправления:
Предыдущее
От: Dave PageДата:
Сообщение: [pgadmin-hackers] pgAdmin 4 commit: Fix integer/numeric validation on variousdialogues.
Следующее
От: Atul SharmaДата:
Сообщение: [pgadmin-hackers] [pgAdmin4][Patch][RM_2450] : Internal Server Error displayed if clickon statistic tab for PG 10 server