Received: from malur.postgresql.org ([217.196.149.56]) by arkaria.postgresql.org with esmtp (Exim 4.80) (envelope-from ) id 1aLOPo-0005Vj-Nf for pgadmin-hackers@arkaria.postgresql.org; Tue, 19 Jan 2016 05:01:13 +0000 Received: from localhost ([127.0.0.1] helo=postgresql.org) by malur.postgresql.org with smtp (Exim 4.84) (envelope-from ) id 1aLOPo-0000Vv-AO for pgadmin-hackers@arkaria.postgresql.org; Tue, 19 Jan 2016 05:01:12 +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) (envelope-from ) id 1aLOPm-0000VN-Kv for pgadmin-hackers@postgresql.org; Tue, 19 Jan 2016 05:01:10 +0000 Received: from mail-pa0-x232.google.com ([2607:f8b0:400e:c03::232]) by makus.postgresql.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_CBC_SHA1:256) (Exim 4.84) (envelope-from ) id 1aLOPi-0005FJ-Tg for pgadmin-hackers@postgresql.org; Tue, 19 Jan 2016 05:01:09 +0000 Received: by mail-pa0-x232.google.com with SMTP id ho8so184073324pac.2 for ; Mon, 18 Jan 2016 21:01:06 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=enterprisedb-com.20150623.gappssmtp.com; s=20150623; h=subject:to:references:cc:from:message-id:date:user-agent :mime-version:in-reply-to:content-type; bh=VLzC1uu9JlhA6OnRl1ZX8gmd2Sop/gTO/NSqcHUGNw0=; b=t9WoZ8XAIToEhmvH6xgaZID0YdNK1V2pr30bKTMgMuL5/mUp37Wor+5GbFkNBgmNk8 7GacIPXhqNCO+c6RRMNXj3PuJpF8HgQQBbM4DvIvkOuu5jAFr6RPrGOCzWt+jnaloWxF i8sPMC7b02GIP2GWScD2ROi7zhzr6c5LD/XLrIR4BsARuILfZYa1eZntBD8y2EuscoyP WsychI0OJrB+dqi2aDW2DmaGUwOxlYTUB85C4m2KaW6u+t1zQxYOl9XC+TMinb6lluxs LU87rQBG/NkrISD15/Rvxi7iRlh0PRMK/Onr3iWJ5QAV2DgAm1EuOCGD0ymcdr+46mMM EMzw== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20130820; h=x-gm-message-state:subject:to:references:cc:from:message-id:date :user-agent:mime-version:in-reply-to:content-type; bh=VLzC1uu9JlhA6OnRl1ZX8gmd2Sop/gTO/NSqcHUGNw0=; b=ZbnuC+tnRN80iTgiMox/1N/UOd6ytd6UTWWfwlrZpA2yRU+1E+6ia+CGYjUjJ33apa UahjR/37ysUVecAx07ri9c3lAINn9iG+E+MipeRxgqjiaioudINKL4/wF/McPJGGfQkW NVY3bWNGNcNW88JxMMRhGhqoMawdxM9YU5CDGeQHbUP423W1S+/z5vl22vTsP7wriaCE BfQ3AdWiLGb3ZQa2Kb4Uyz5zBa+X+Nzcxk0+a9yFeKBzgnaxHLYVNZgD7NSPveZ6uRv2 w4tCPIqT8y2TgHd/bSrgfiSDrmDNNXfjI2VYRNMNaRxcEW4v7QmgPwnFNRYiOhnUVmfk fe5Q== X-Gm-Message-State: ALoCoQnZDMMlTwalCHUElocxOApZ8A+V9Az3sReOdBqfaxKHRR9egKwxTZex2WFG/gWGUS7XR+0dN+kJpv0nx0QwSvY6xMYo79Qrqhm2UAz0MJkha5omyrt8XivUSha8K5yVNSuD7kRMg/EsuW6w2mbm4fOplllNpuF3rWWr3jNpdPLkkQiJAcX1zYW10FJjBWCTFk3URbqmHObNlpKG4paBKYdc78uAIQ== X-Received: by 10.66.252.136 with SMTP id zs8mr41439593pac.110.1453179665818; Mon, 18 Jan 2016 21:01:05 -0800 (PST) Received: from [172.24.35.244] ([59.162.78.200]) by smtp.gmail.com with ESMTPSA id 16sm37612936pfh.48.2016.01.18.21.01.03 (version=TLSv1/SSLv3 cipher=OTHER); Mon, 18 Jan 2016 21:01:05 -0800 (PST) Subject: Re: PATCH: added "Collation" & "Catalog Objects" nodes in pgAdmin4 To: Neel Patel , Dave Page References: <568D05F9.6070809@enterprisedb.com> Cc: pgadmin-hackers From: Murtuza Zabuawala Message-ID: <569DC30E.3020004@enterprisedb.com> Date: Tue, 19 Jan 2016 10:31:02 +0530 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:38.0) Gecko/20100101 Thunderbird/38.5.0 MIME-Version: 1.0 In-Reply-To: Content-Type: multipart/alternative; boundary="------------000308020005030101000305" 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 This is a multi-part message in MIME format. --------------000308020005030101000305 Content-Type: text/plain; charset=utf-8; format=flowed Content-Transfer-Encoding: 7bit Thanks Neel. I'll work on below mentioned issues & send new patch. Regards, Murtuza On Monday 18 January 2016 10:12 PM, Neel Patel wrote: > Hi Murtuza, > > Please find below review comments for the collation node. > > 1. "Owner" field should be changed from text control to > Backform.NodeListByNameControl. > > 2. Remove "Use Slony" option, we will implement it as separate module. > > 3. Use 2 space indentation instead of 4 space in javascript file. > > 4. In some of the sql file, 'qtIdent' and 'qtLiteral' is not used. > Please check all the SQL files and make sure we use the same. > e.g. In update.sql -- Change '{{ data.description }}' to {{ > data.description|qtLiteral }} > > 5. While creating the new collation, there is a spelling mistake in > "Definition" tab text. Currently it is displayed as "Defination" which > is wrong. > > 6. Wrong SQL is getting generated when we create the new collation > from "Copy collation" as below. Please correct the generated SQL. > > CREATE COLLATION public.my_collation FROM pg_catalog.\; > > 7. If we "Edit" the existing created collation with collation Name > then below wrong SQL is getting generated. In below SQL - if user > change the name of the collation then "... RENAME TO ..." query should > be first executed then other modified parameters should be applied. > We also noticed that when we change only collation name then also for > the unchanged parameters, queries are getting generated which should > be corrected. Query should be generated only for modified parameters. > > Below is the wrong SQL. > > ALTER COLLATION my_schema.my_collation_1_up > OWNER TO ; > COMMENT ON COLLATION my_schema.my_collation_1_up > IS 'testing comment....'; > ALTER COLLATION my_schema.my_collation_1_up > RENAME TO my_collation_1; > ALTER COLLATION my_schema.my_collation_1 SET SCHEMA None; > > 8. When we create the new collation with below parameters then we are > getting "*IndexError: list index out of range*" error at python side. > Use below parameter to reproduce the error. > > CREATE COLLATION public.collation_23 > ( > LOCALE = 'en_AG.utf8' > ); > ALTER COLLATION public.collation_23 > OWNER TO postgres; > COMMENT ON COLLATION public.collation_23 > IS 'My comment....'; > 9. As per Dave's comment, change the copyright year to 2016 and some > of the spelling mistakes. > > 10. In "validate" function in collation.js, multiple time > "this.get('')" is used. Instead of using multiple time, > we can assign in one variable > and use that variable in all the places. > > Do let us know for any comments/issues. > > Thanks, > Neel Patel > > On Wed, Jan 6, 2016 at 6:10 PM, Dave Page > wrote: > > On Wed, Jan 6, 2016 at 12:18 PM, Murtuza Zabuawala > > wrote: > > Hi, > > > > Please find patch to add "Collation" & "Catalog Objects" nodes > in pgAdmin4. > > > > Please note that this patch is mainly for "Collation" & "Catalog > Objects" > > nodes, Schema/Catalog node is not yet complete as we are yet to > implement > > privileges control for the same. > > Thanks - a couple of quick comments: > > - Please ensure the copyright notices are updated for 2016. > > - The text: > > Below SQL will .... > > Should be: > > The SQL below will .... > > -- > 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 > > --------------000308020005030101000305 Content-Type: text/html; charset=utf-8 Content-Transfer-Encoding: 8bit Thanks Neel.

I'll work on below mentioned issues & send new patch.

Regards,
Murtuza

On Monday 18 January 2016 10:12 PM, Neel Patel wrote:
Hi Murtuza,

Please find below review comments for the collation node.

1. "Owner" field should be changed from text control to Backform.NodeListByNameControl.

2. Remove "Use Slony" option, we will implement it as separate module.

3. Use 2 space indentation instead of 4 space in javascript file.

4. In some of the sql file, 'qtIdent' and 'qtLiteral' is not used. Please check all the SQL files and make sure we use the same.
    e.g. In update.sql  -- Change  '{{ data.description }}'   to {{ data.description|qtLiteral }}

5. While creating the new collation, there is a spelling  mistake in "Definition" tab text. Currently it is displayed as "Defination" which is wrong.

6. Wrong SQL is getting generated when we create the new collation from "Copy collation" as below. Please correct the generated SQL.

    CREATE COLLATION public.my_collation FROM pg_catalog.\;

7. If we "Edit" the existing created collation with collation Name then below wrong SQL is getting generated. In below SQL - if user change the name of the collation then "... RENAME TO ..." query should be first executed then other modified parameters should be applied.
We also noticed that when we change only collation name then also for the unchanged parameters, queries are getting generated which should be corrected.  Query should be generated only for modified parameters.

Below is the wrong SQL.

    ALTER COLLATION my_schema.my_collation_1_up
    OWNER TO ;
    COMMENT ON COLLATION my_schema.my_collation_1_up
    IS 'testing comment....';
    ALTER COLLATION my_schema.my_collation_1_up
    RENAME TO my_collation_1;
    ALTER COLLATION my_schema.my_collation_1 SET SCHEMA None;

8.  When we create the new collation with below parameters then we are getting "IndexError: list index out of range" error at python side.
     Use below parameter to reproduce the error.

     CREATE COLLATION public.collation_23
     ( 
  LOCALE = 'en_AG.utf8'
     );
    ALTER COLLATION public.collation_23
    OWNER TO postgres;
    COMMENT ON COLLATION public.collation_23
    IS 'My comment....';
     
9. As per Dave's comment, change the copyright year to 2016 and some of the spelling mistakes.

10. In "validate" function in collation.js, multiple time "this.get('<column_name>')" is used. Instead of using  multiple time, we can assign in one variable 
      and use that variable in all the places. 

Do let us know for any comments/issues.

Thanks,
Neel Patel

On Wed, Jan 6, 2016 at 6:10 PM, Dave Page <dpage@pgadmin.org> wrote:
On Wed, Jan 6, 2016 at 12:18 PM, Murtuza Zabuawala
<murtuza.zabuawala@enterprisedb.com> wrote:
> Hi,
>
> Please find patch to add "Collation" & "Catalog Objects" nodes in pgAdmin4.
>
> Please note that this patch is mainly for "Collation" & "Catalog Objects"
> nodes, Schema/Catalog node is not yet complete as we are yet to implement
> privileges control for the same.

Thanks - a couple of quick comments:

- Please ensure the copyright notices are updated for 2016.

- The text:

Below SQL will ....

Should be:

The SQL below will ....

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


--------------000308020005030101000305--