Received: from malur.postgresql.org ([217.196.149.56]) by arkaria.postgresql.org with esmtp (Exim 4.80) (envelope-from ) id 1aLCtF-0000Tz-8V for pgadmin-hackers@arkaria.postgresql.org; Mon, 18 Jan 2016 16:42:49 +0000 Received: from localhost ([127.0.0.1] helo=postgresql.org) by malur.postgresql.org with smtp (Exim 4.84) (envelope-from ) id 1aLCtE-0002Kw-Qp for pgadmin-hackers@arkaria.postgresql.org; Mon, 18 Jan 2016 16:42:48 +0000 Received: from magus.postgresql.org ([2a02:c0:301:0:ffff::29]) by malur.postgresql.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_CBC_SHA384:256) (Exim 4.84) (envelope-from ) id 1aLCtE-0002Kq-9f for pgadmin-hackers@postgresql.org; Mon, 18 Jan 2016 16:42:48 +0000 Received: from mail-yk0-x22b.google.com ([2607:f8b0:4002:c07::22b]) by magus.postgresql.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_CBC_SHA1:256) (Exim 4.84) (envelope-from ) id 1aLCt9-0004M6-ND for pgadmin-hackers@postgresql.org; Mon, 18 Jan 2016 16:42:47 +0000 Received: by mail-yk0-x22b.google.com with SMTP id x67so620887062ykd.2 for ; Mon, 18 Jan 2016 08:42:42 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=enterprisedb-com.20150623.gappssmtp.com; s=20150623; h=mime-version:in-reply-to:references:date:message-id:subject:from:to :cc:content-type; bh=28EzcQCUGL0HUSoWah4R0O7t9JtSPuhmpf01U9pUOBM=; b=khlZP0neMG3ZLMZzZDMs2l3WXqAVfO3fqXffnzJWNfAE5vK8mzlRz2gXdoWnAVstbX yMZ/sjdcN2vxSiD0d4cg7ettbIPj56CKcOBP0I17zP74KADMuvstsX+XX8hgwdfkoJ+Q 06JLhutDcnAnd3/3pfpeXDcqqBSFiRymNSbPFjxrpiuITPfGSLa0/7OQvlkl+FhOiN9A l5bC98tvkDofR+FtdWnTg1zdKxJDs3j5x+aLQL/hXroa7I/uEau9299v/WHhDN7nx7jK j98ur0BT0UchgXiZ4WjEKVnPxsy6mZtCVjwZDKcoo+T5SlgjtCZuVrYUsQXI+OaQGqEa Eg9g== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20130820; h=x-gm-message-state:mime-version:in-reply-to:references:date :message-id:subject:from:to:cc:content-type; bh=28EzcQCUGL0HUSoWah4R0O7t9JtSPuhmpf01U9pUOBM=; b=ZkpeUAwPS1HBvmPxz7v/zn4zyhsZ5e4rGQSj3UmtWU+6++4h4xvf5//eE6SSmsMZ1A 5dGPBSpxc7SlHgkNvzAP7mh0MjAKpvxH52jQlEC7u8/KwvPYol5nmyOvYmzf4Vt6CAEg az9r/zCA8Ev/qArZ8CIjhn/y9Cg6Sbi34dy+6kZboSnuxRRors6+Va4H8FGfM0/A7bS1 cgbGbk70y68eVIEpgMZTkTgIvscl07vJbWxp+SsqnDORZiO4Dtom1n8muCm1/PnKgCAB glmbiiL/bSmS4vpgoeGFSWpt5Cv+JtqQA9yegtxkMjSj/IlRyrisRWYD7An44R+0HCm2 2dyw== X-Gm-Message-State: ALoCoQnWbxUMqXN6n5le3xWB7+eJ22G7fmhZlJmFROGj0FiVqWcTBmcdlSmV3QG/Bxq4OpFl06YEZuebP6A5KIc0abH6EV2IU5hr7+jXspyxaZdiXOcc2mM= MIME-Version: 1.0 X-Received: by 10.13.240.69 with SMTP id z66mr17338066ywe.330.1453135360632; Mon, 18 Jan 2016 08:42:40 -0800 (PST) Received: by 10.37.202.75 with HTTP; Mon, 18 Jan 2016 08:42:40 -0800 (PST) In-Reply-To: References: <568D05F9.6070809@enterprisedb.com> Date: Mon, 18 Jan 2016 22:12:40 +0530 Message-ID: Subject: Re: PATCH: added "Collation" & "Catalog Objects" nodes in pgAdmin4 From: Neel Patel To: Dave Page Cc: Murtuza Zabuawala , pgadmin-hackers Content-Type: multipart/alternative; boundary=94eb2c035cd8fb57a205299e7092 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 --94eb2c035cd8fb57a205299e7092 Content-Type: text/plain; charset=UTF-8 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 > --94eb2c035cd8fb57a205299e7092 Content-Type: text/html; charset=UTF-8 Content-Transfer-Encoding: quoted-printable
Hi Murtuza,

Please find below review co= mments for the collation node.

1. "Owner"= ; field should be changed from text control to Backform.NodeListByNameContr= ol.

2. Remove "Use Slony" option, we wil= l implement it as separate module.

3.=C2=A0Use 2 space indentation instead of 4 space in jav= ascript file.

4. In some of the sql file, &= #39;qtIdent' and 'qtLiteral' is not used. Please check all the = SQL files and make sure we use the same.
=C2=A0 =C2=A0 e.g. In up= date.sql =C2=A0-- Change =C2=A0'{{ data.description }}' =C2=A0 to {= { data.description|qtLiteral }}

5. While creating = the new collation, there is a spelling =C2=A0mistake in "Definition&qu= ot; tab text. Currently it is displayed as "Defination" which is = wrong.

6. Wrong SQL is getting generated when we c= reate the new collation from "Copy collation" as below. Please co= rrect the generated SQL.

=C2=A0 =C2=A0 CREATE COLL= ATION public.my_collation FROM pg_catalog.\;

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

=C2=A0 = =C2=A0 ALTER COLLATION my_schema.my_collation_1_up
=C2=A0 =C2=A0 = OWNER TO ;
=C2=A0 =C2=A0 COMMENT ON COLLATION my_schema.my_collat= ion_1_up
=C2=A0 =C2=A0 IS 'testing comment....';
=C2=A0 =C2=A0 ALTER COLLATION my_schema.my_collation_1_up
=C2= =A0 =C2=A0 RENAME TO my_collation_1;
=C2=A0 =C2=A0 ALTER COLLATIO= N my_schema.my_collation_1 SET SCHEMA None;

= 8.=C2=A0 When we create the new collation with below parameters then we are= getting "IndexError: list index out of range" error at py= thon side.
=C2=A0 =C2=A0 =C2=A0Use below parameter to reproduce t= he error.

=C2=A0 =C2=A0 =C2=A0CREATE COLLATION pub= lic.collation_23
=C2=A0 =C2=A0 =C2=A0(=C2=A0
=C2=A0 LOCALE =3D 'en_AG.utf8'
=C2=A0 =C2=A0 =C2=A0);
=C2=A0 =C2=A0 ALTER COLLATION public.collation_23
=C2=A0 =C2=A0 OWNER TO postgres;
=C2=A0 =C2=A0 COMMENT ON= COLLATION public.collation_23
=C2=A0 =C2=A0 IS 'My comment..= ..';
=C2=A0 =C2=A0 =C2=A0
9. As per Dave's comm= ent, change the copyright year to 2016 and some of the spelling mistakes.

10. In "validate" function in collation.j= s, multiple time "this.get('<column_name>')" is use= d. Instead of using =C2=A0multiple time, we can assign in one variable=C2= =A0
=C2=A0 =C2=A0 =C2=A0 and use that variable in all the places.= =C2=A0

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:
O= n Wed, Jan 6, 2016 at 12:18 PM, Murtuza Zabuawala
<murtuza.zabuawala@enterprisedb.com> wrote:
> Hi,
>
> Please find patch to add "Collation" & "Catalog Obj= ects" 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 implem= ent
> 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=

--94eb2c035cd8fb57a205299e7092--