Re: pgAdmin4 PATCH: Domain Module

Hi,

Please find attached Revised patch for the Domain module and also my
comments inline as below.

Thanks,
Khushboo

On Wed, Feb 3, 2016 at 4:22 PM, Neel Patel <neel(dot)patel(at)enterprisedb(dot)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 in
> below numeric base type. Also do proper indentation in generated query.
>
> * Wrong Query :- *
> CREATE DOMAIN my_schema.test_123
> AS "numeric"
> DEFAULT 5
> ;
>
> * Correct Query :- *
> CREATE DOMAIN my_schema.test_123
> AS numeric(22,4)
> DEFAULT 5
> NOT 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 Patel
>
> On Tue, Feb 2, 2016 at 3:51 PM, Khushboo Vashi <
> khushboo(dot)vashi(at)enterprisedb(dot)com> wrote:
>
>> Hi Neel,
>>
>> Thanks for reviewing my patch.
>>
>> I have modified the code as per your suggestions and also fixed some of
>> the issues got while doing unit testing.
>> Please find attached patch for the same.
>>
>>
>> Thanks,
>> Khushboo
>>
>>
>>
>> On Wed, Jan 20, 2016 at 10:56 PM, Neel Patel <neel(dot)patel(at)enterprisedb(dot)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.1
>>>
>>> e.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 Patel
>>>
>>> On Wed, Jan 20, 2016 at 2:50 PM, Khushboo Vashi <
>>> khushboo(dot)vashi(at)enterprisedb(dot)com> wrote:
>>>
>>>> Hi Neel,
>>>>
>>>> Please find updated patch.
>>>>
>>>> Thanks,
>>>> Khushboo
>>>>
>>>> On Wed, Jan 20, 2016 at 12:50 PM, Neel Patel <
>>>> neel(dot)patel(at)enterprisedb(dot)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 EXISTS
>>>>> domains (1).patch:1487: trailing whitespace.
>>>>> OR (nspname = 'information_schema' AND EXISTS
>>>>> domains (1).patch:1489: trailing whitespace.
>>>>> OR (nspname LIKE '_%' AND EXISTS
>>>>> domains (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 errors
>>>>> warning: 9 lines add whitespace errors.
>>>>> #########################################
>>>>>
>>>>> Can you please remove the whitespace and regenerate the patch ?
>>>>>
>>>>> Thanks,
>>>>> Neel Patel
>>>>>
>>>>> On Wed, Jan 20, 2016 at 12:37 PM, Khushboo Vashi <
>>>>> khushboo(dot)vashi(at)enterprisedb(dot)com> wrote:
>>>>>
>>>>>> Resending patch with binary option.
>>>>>>
>>>>>> On Wed, Jan 20, 2016 at 10:18 AM, Khushboo Vashi <
>>>>>> khushboo(dot)vashi(at)enterprisedb(dot)com> wrote:
>>>>>>
>>>>>>> Hi,
>>>>>>>
>>>>>>> Please find attached patch for the Domain Module.
>>>>>>>
>>>>>>> The patch will be modified after Types module implementation as we
>>>>>>> need to populate Base Type and some Type related validations from the
>>>>>>> Types module.
>>>>>>>
>>>>>>> Please review it and let me know the feedback.
>>>>>>>
>>>>>>> Thanks,
>>>>>>> Khushboo
>>>>>>>
>>>>>>
>>>>>>
>>>>>>
>>>>>> --
>>>>>> Sent via pgadmin-hackers mailing list (pgadmin-hackers(at)postgresql(dot)org
>>>>>> )
>>>>>> To make changes to your subscription:
>>>>>> http://www.postgresql.org/mailpref/pgadmin-hackers
>>>>>>
>>>>>>
>>>>>
>>>>
>>>
>>
>