Received: from malur.postgresql.org ([217.196.149.56]) by arkaria.postgresql.org with esmtp (Exim 4.84_2) (envelope-from ) id 1dIxyT-0000iN-Oq for pgadmin-hackers@arkaria.postgresql.org; Thu, 08 Jun 2017 13:59:45 +0000 Received: from localhost ([127.0.0.1] helo=postgresql.org) by malur.postgresql.org with smtp (Exim 4.84_2) (envelope-from ) id 1dIxyT-0000Fv-BZ for pgadmin-hackers@arkaria.postgresql.org; Thu, 08 Jun 2017 13:59:45 +0000 Received: from makus.postgresql.org ([2001:4800:1501:1::229]) by malur.postgresql.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_CBC_SHA384:256) (Exim 4.84_2) (envelope-from ) id 1dIxyS-0000Ax-0h for pgadmin-hackers@postgresql.org; Thu, 08 Jun 2017 13:59:44 +0000 Received: from mail-it0-x232.google.com ([2607:f8b0:4001:c0b::232]) by makus.postgresql.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_CBC_SHA1:256) (Exim 4.84_2) (envelope-from ) id 1dIxyP-0003wJ-4Y for pgadmin-hackers@postgresql.org; Thu, 08 Jun 2017 13:59:42 +0000 Received: by mail-it0-x232.google.com with SMTP id x129so17140672ite.0 for ; Thu, 08 Jun 2017 06:59:40 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=pgadmin-org.20150623.gappssmtp.com; s=20150623; h=mime-version:in-reply-to:references:from:date:message-id:subject:to :cc; bh=4fd5VTyQL/BGkXWnby/xuedzLfq7lZLc93mybqQ1kRo=; b=np2y9nk+aaNk8IeZTCgQyVYy7K0jFJxKe17vaUCbEPjBFUI4QpX41pfpVTdnexFHQd l2i0uiSTakM68aqtAAdJycm26ylBaG+y+27/jVfSzYBy8DyTKF5cXIaFGzOxEC9ObhMT T1ETMNjwzZYeZr9vczlSAO2KLF9PgBoHPFnMp3Z/4SOtGAYnIx+uMAj+JiNIGKtYMjHp Y8gGShcoWvOvMluko/K+i8THvWyo+WXBfd7tAgJ4KnEVhW1eK+I64f9QRls3QqkePNK6 0BzDxwc1jPUULS/QI6oXnGUGoy02xE5T37qliJtirx7mdXY+5GaQR6smM3YrYAl7O3Kc nsyA== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:in-reply-to:references:from:date :message-id:subject:to:cc; bh=4fd5VTyQL/BGkXWnby/xuedzLfq7lZLc93mybqQ1kRo=; b=Ojd6FQGrxLNefcTb8xkASfnjK7zcxUohbgIjLJWdZ4dS1rpvab9P9B9UEI0FBlWD7D 3H8+EZDCsy3i/7CilBG1lIUmIctIcA39IBRECvFZtbRWJfrwkazOAJXwoD/v7mn80Wqc T/ewqSj0GvVK+MHrgikRVt6PqC6R9+rdbhoBQZ6fpLFgkDd/m5ra9D37vxTCAL5//jyy vreDFRCV1/NsPXg8gcni2y6KFt9s8Po/EZNpFs7MT/aYjIKLM22vxxLIEdp/FnMqRQ2x s6nC3xQyTZukqpgTA2GekCtUz/u9fsjZ1okgmzf78Wq2wtFWJ1M6gn7rw4LlWUadvJI9 2ziA== X-Gm-Message-State: AODbwcCoJSeYgRW3/hrdUe+7OXX/tNeGfGkOx+OzbnDOSBf18VWb4rTe mR6/4t9tVFWE+YEIg1SGv6EA1pYlPfe2 X-Received: by 10.36.31.74 with SMTP id d71mr5418911itd.85.1496930380292; Thu, 08 Jun 2017 06:59:40 -0700 (PDT) MIME-Version: 1.0 Received: by 10.107.174.147 with HTTP; Thu, 8 Jun 2017 06:59:39 -0700 (PDT) In-Reply-To: References: From: Dave Page Date: Thu, 8 Jun 2017 14:59:39 +0100 Message-ID: Subject: Re: Fix for RM2421 [pgAdmin4][patch] To: Harshal Dhumal Cc: pgadmin-hackers Content-Type: text/plain; charset="UTF-8" X-Pg-Spam-Score: -2.6 (--) List-Archive: List-Help: List-ID: List-Owner: List-Post: List-Subscribe: List-Unsubscribe: X-Mailing-List: pgadmin-hackers Precedence: bulk Sender: pgadmin-hackers-owner@postgresql.org 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); >> >> >>>>> - 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 >> >> >>>>> 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 -- Sent via pgadmin-hackers mailing list (pgadmin-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgadmin-hackers