Re: pgAdmin4 PATCH: Domain Module - Mailing list pgadmin-hackers
From | Khushboo Vashi |
---|---|
Subject | Re: pgAdmin4 PATCH: Domain Module |
Date | |
Msg-id | CAFOhELePQ_m8zmbORLmO3PUY5yTA5o_9suQKk0Tox8A7Fb8ovw@mail.gmail.com Whole thread Raw |
In response to | Re: pgAdmin4 PATCH: Domain Module (Neel Patel <neel.patel@enterprisedb.com>) |
Responses |
Re: pgAdmin4 PATCH: Domain Module
|
List | pgadmin-hackers |
Hi,
Please find attached Revised patch for the Domain module and also my comments inline as below.
Thanks,Please find attached Revised patch for the Domain module and also my comments inline as below.
On Wed, Feb 3, 2016 at 4:22 PM, Neel Patel <neel.patel@enterprisedb.com> wrote:
Hi Khushboo,Please find below review comments.- Reverse engineering SQL generation is not implemented for domain node.
Done
- "Length" and "Precision" fields should be enabled/disabled based on the selection of "Base Type" value.
This implementation is dependent on the 'Type' module. Once that will be done, I will merge my code.
- Query is not getting generated properly. Some of the parameters are not reflected in query. As we have provided Length and Precision value inbelow numeric base type. Also do proper indentation in generated query.Wrong Query :-CREATE DOMAIN my_schema.test_123AS "numeric"DEFAULT 5;Correct Query :-CREATE DOMAIN my_schema.test_123AS numeric(22,4)DEFAULT 5NOT NULL;
Done
- After creation of new domain with base type "aclitem" , wrong "Length" field value is getting displayed.
Done
- We are getting error saying "TypeError: the JSON object must be str, not 'dict'" when we add constraint w
ith "NOT VALID" and NO INHERIT.
Done
- We should add property "System Domain?" when we select any domain node.
Done
- We think for creation of Security Label, we should include the schema name along with domain name.Wrong Query :-SECURITY LABEL FOR pv_label ON DOMAIN test_123 IS 'label_val';Correct Query :-SECURITY LABEL FOR pv_label ON DOMAIN <schema_name>.test_123 IS 'label_val';
Done
Let us know in case of any issues.Thanks,Neel PatelOn Tue, Feb 2, 2016 at 3:51 PM, Khushboo Vashi <khushboo.vashi@enterprisedb.com> wrote:KhushbooThanks,Please find attached patch for the same.I have modified the code as per your suggestions and also fixed some of the issues got while doing unit testing.Hi Neel,Thanks for reviewing my patch.On Wed, Jan 20, 2016 at 10:56 PM, Neel Patel <neel.patel@enterprisedb.com> wrote:Hi Khushboo,Please find below review comments.- While creating new Domain and clicking on SQL tab, python side we are getting error saying "TypeError: 'bool' object is not callable".We are not able to create any domain. Fix this issue so that we can test other functionality.- Implement the reverse engineering SQL generation for the domain node.- As per the checklist, remove the "Use Slony" from Constraints tab, as it is not required.- No need to pass "qtIdent=self.qtIdent" as function argument in "create" and "getSQL" function in domains/__init__.py- In "Security" tab , provider and security label fields are not editable.- In PG version 9.1, when we update the existing domain name then "ALTER DOMAIN" is not supported.Currently there is no checking for the PG version 9.1 and 9.2_plus. It will fail when we connect to database 9.1e.g.For PG version 9.1 - Update command should be as below.ALTER TYPE xyz RENAME TO abc;For PG version 9.2 onwards - Update command should be as below.ALTER DOMAIN xyz RENAME TO abc;- Some of the SQL file, qtIdent is not used. Please check all the related SQL files.e.g. - In update.sql file "data.owner" should be "conn|qtIdent(data.owner)"{% if data.owner %}ALTER DOMAIN {{ conn|qtIdent(o_data.basensp, name) }}OWNER TO {{ data.owner }};{% endif %}Let us know for any issues.Thanks,Neel PatelOn Wed, Jan 20, 2016 at 2:50 PM, Khushboo Vashi <khushboo.vashi@enterprisedb.com> wrote:KhushbooThanks,Hi Neel,Please find updated patch.On Wed, Jan 20, 2016 at 12:50 PM, Neel Patel <neel.patel@enterprisedb.com> wrote:Hi Khushboo,While applying the patch file, we are getting below warnings.#########################################domains (1).patch:1340: trailing whitespace.oid: undefined,domains (1).patch:1483: trailing whitespace.(nspname = 'pg_catalog' AND EXISTSdomains (1).patch:1487: trailing whitespace.OR (nspname = 'information_schema' AND EXISTSdomains (1).patch:1489: trailing whitespace.OR (nspname LIKE '_%' AND EXISTSdomains (1).patch:1642: trailing whitespace.(select 1 from pg_class where relnamespace=typnamespace and relname = typname and relkind != 'c') AND (typname not like '_%' OR NOT EXISTS (select 1 from pg_class where relnamespace=typnamespace and relname = substring(typname from 2)::name and relkind != 'c'))warning: squelched 4 whitespace errorswarning: 9 lines add whitespace errors.#########################################Can you please remove the whitespace and regenerate the patch ?Thanks,Neel PatelOn Wed, Jan 20, 2016 at 12:37 PM, Khushboo Vashi <khushboo.vashi@enterprisedb.com> wrote:--Resending patch with binary option.On Wed, Jan 20, 2016 at 10:18 AM, Khushboo Vashi <khushboo.vashi@enterprisedb.com> wrote:KhushbooThe patch will be modified after Types module implementation as we need to populate Base Type and some Type related validations from the Types module.Hi,Please find attached patch for the Domain Module.Please review it and let me know the feedback.Thanks,
Sent via pgadmin-hackers mailing list (pgadmin-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgadmin-hackers
Attachment
pgadmin-hackers by date: