Re: [PATCH] Tables node (pgAdmin4)

Hi Harshal,

Below are my review comments:

I got below warning when I tried to apply the patch for table node as
mentioned below:

Table creation:

- Trailing white spaces warnings

$ git apply
/projects/patches/pgadmin4/Table/table_14_May_V6.patch

/projects/patches/pgadmin4/Table/table_14_May_V6.patch:6008: trailing
whitespace.
return false;

/projects/patches/pgadmin4/Table/table_14_May_V6.patch:6016: trailing
whitespace.
return false;
warning: 2 lines add whitespace errors.

- In Table creation dialog, while adding a new primary key, it does not
allow to change the tablespace to empty. (which is not the case in case of
tablespace in table)
- In Table creation dialog, while adding a new column, data type and
name field must be mandatory. otherwise while clicking on save it gives
below error

File
"/projects/pgadmin4/web/pgadmin/browser/server_groups/servers/databases/schemas/tables/__init__.py",
line 1319, in _parse_format_columns
c['cltype'] = self._cltype_formatter(c['cltype'])
KeyError: 'cltype

- In Table creation dialog, While adding a new column, in primary check
box is needed to click twice in order to check it. Ideally it should be
checked by only one click.
- In Table creation dialog, While adding a new column, primary key
should not be allowed to added unless user has provided name and data type
for atleast one column.
currently if user has clicked on add column button and immediately click
on add primary key button, it will add a row in primary key data grid
- When delete table/drop cascade is apply on any table, i got a
javascript error as mentioned below

node.js:94 Uncaught TypeError: self.canDrop.apply is not a
function

- Once the above error generated, every time user tries to open a
context menu by right clicking on any existing table, that same error comes
- In table creation dialog, if table is inherited from another table, if
a new primary key is added manually there, the create sql will not have an
entry for primary key
- In table creation dialog, if primary key check box is checked while
adding the row, a new row is added in primary key datagrid but unchecking
the primary key checkbox from column datagrid, does not removes that row
from primary key data grid.
- In AutoVacuum tab, if user provides any invalid value to any
parameter, then a error message should be prompted, only background color
change would not tell user to change the value.
- In table creation dialog, security label are not being added.
javascript error is coming as mentioned below:
{"success": 0, "info": "", "result": null, "data": null, "errormsg":
"can't adapt type 'Undefined'"}
- In Table creation dialog, while adding foreign key, in action tab. if
user click on 'x' button in "on update" or "on delete" select2 control, it
gives error "Uncaught SyntaxError: Unexpected end of input"
- In Table creation dialog, while adding a check constraint, "validated"
button does not work properly
- After successfully creation of table, "table name cannot be empty"
error is not getting cleared.
- In Table creation dialog, if user has added an empty column without
entering its name or type and trying to add check constraint, it will add
an empty constraint
- In Table creation dialog, while adding an exclude constraint, for
"character varying" column type, no operators are being listed
- In Table creation dialog, while adding an exclude constraint, below
mentioned error comes if user removes operator class by clicking 'x' on
that control Uncaught TypeError: Cannot read property 'id' of undefined
- In Table creation dialog, SQL is not getting generated for exclude
constraint
- In Table creation dialog, schema should be prefixed with table name in
"of type" control
- In Table creation dialog, while adding privileges, it always shows
default privileges even if user has selected different privileges. (This
works fine once user edit the privileges in edit table mode and shows only
those privileges which user selects). Ashesh, please confirm the behaviour.

Table edit mode:

- If in edit mode, any constraint is already having any comment, then
remove it. It will not create the SQL for the same.
- Changing Schema will give server error

Column Creation:

- Save button is enabled by default
- Data type validation is not provided. Save button is enabled just
after providing column name
- Length field limitation is not provided. (i.e. for numeric type,
length should be allowed greater than 1000)

Exclusion constraint creation:

- Access method should not be allowed to be empty. (currently by
clicking 'x' will remove the selection in it)

Index creation:

- No error message for name field when empty
- No error message when column name is not provided while adding a
column in index
- While adding a column if no name is provided, "None" appears in SQL
tab which will give error on OK button click
- when comment is provided while creation, it gives error saying index
does not exists. because schema name is not added before it.

Rule creation:

- Name is empty error does not come till user enters something in
definition tab
- DO INSTEAD button does not make any difference to SQL (it works in
edit mode)

Rule edit mode:

- Add comment in edit mode, check the SQL in sql tab. Now come back to
general tab and removes comment and check the sql tab again.
SQL for comment is still there with empty string as comment

Trigger Creation:

- SQL is not proper when creating a trigger. "()" should be appended to
function name in SQL.
It gives error while creating a trigger
- "+" sign is visible in browser tree in front of trigger. either On
expanding trigger, it should show the trigger function name or that "+"
sign should not appear

Trigger edit node:

- On removing comment, nothing happens. No sql is being created. Comment
is still there in properties.

Regards,
Sanket Mehta
Sr Software engineer
Enterprisedb

On Mon, May 16, 2016 at 7:02 PM, Harshal Dhumal <
harshal(dot)dhumal(at)enterprisedb(dot)com> wrote:

> Hi,
>
> PFA add-on patch for table and it's child node. (please apply this patch
> on version 6 patch)
>
> Murtuza and I fixed following issues:
>
> 1. SQL formatting
> 2. Vacuum grid should not be editable in properties mode.
> 3. Column datatype does not get displayed in the properties and edit mode.
> 4. Do not allow to add another primary key if one already exist.
>
> And another minor enhancements.
>
>
> --
> *Harshal Dhumal*
> *Software Engineer *
>
>
>
> EenterpriseDB <http://www.enterprisedb.com>
>
> On Sat, May 14, 2016 at 2:03 AM, Harshal Dhumal <
> harshal(dot)dhumal(at)enterprisedb(dot)com> wrote:
>
>>
>> Hi,
>>
>>
>> PFA updated patches (version: 6) for table and it's child nodes.
>>
>> --
>> *Harshal Dhumal*
>> *Software Engineer *
>>
>>
>>
>> EenterpriseDB <http://www.enterprisedb.com>
>>
>> On Fri, May 13, 2016 at 6:55 PM, Khushboo Vashi <
>> khushboo(dot)vashi(at)enterprisedb(dot)com> wrote:
>>
>>> Hi,
>>>
>>> Review Comments:
>>>
>>> - Please replace 'can not' with 'cannot' in all the validation messages.
>>> - PG 9.1+ Inheritance issue as below:
>>>
>>> CREATE TABLE public.table1
>>> (
>>> )
>>> (
>>> )
>>> INHERITS (a)
>>> WITH (
>>> OIDS = FALSE
>>> )
>>> TABLESPACE pg_default;
>>> ALTER TABLE public.table1
>>> OWNER to postgres;
>>>
>>>
>>> brackets are coming twice.
>>>
>> Fixed
>>
>>
>>>
>>> - Please maintain one line spacing between SQL queries In the SQL Tab.
>>>
>> TODO
>>
>>
>>> - Foreign Key Grid in Table css issue: Grid columns expands on the
>>> selection of the cell
>>>
>> Fixed
>>
>>
>>> - Check Constraint: Validated? option should be True by default
>>>
>> Not sure about this. I cross checked in pgadmin3.
>>
>>
>>
>>> - pg 9.4: Exclude constraint does not render in SQL tab
>>>
>> Fixed
>>
>>
>>> - Missing Security validation
>>>
>> Fixed
>>
>>
>>> - Vacuum grid should not be editable in properties mode.
>>>
>> TODO (It's editable but one cannot save it on server from here as there
>> is no save button.)
>>
>>
>>> - Edit mode, Fill Factor can be allowed to be null.
>>>
>> TODO (This is generic issue in Integer and Numeric controls. This issue
>> is covered in this partial patch
>> <http://www.postgresql.org/message-id/CAFiP3vzvtqxJMpZeeaV4pd0p7CevcRRDfgDWuvu4cyHNThpJFQ@mail.gmail.com>
>> )
>>
>>
>>> - While dropping inheritance, related table columns drop SQL are also
>>> populated in the SQL Tab
>>>
>>> ALTER TABLE public."Tbl"
>>> NO INHERIT b;
>>> ALTER TABLE public."Tbl" DROP COLUMN id;
>>> ALTER TABLE public."Tbl" DROP COLUMN name;
>>>
>>>
>> Fixed
>>
>>
>>
>>>
>>> And also render error while clicking on the save button.
>>>
>>> ERROR: syntax error at or near "["
>>> LINE 2: INHERIT [;
>>>
>>> ^
>>>
>>> Fixed
>>
>>
>>> - in a Reverse Engineering SQL tab, schema_name.tablename should be
>>> there, currently only table_name displays.
>>>
>> Fixed
>>
>>
>>
>>> - Column SQL is showing below text with HTML
>>>
>>>
>>> <html><head></head><body>-- Column: id -- ALTER TABLE public.a DROIP
>>> COLUMN id; ALTER TABLE public.a ADD COLUMN id integer;</body></html>
>>>
>>>
>> I was not able to reproduce exact issue but still I have fixed other
>> issue which I found related to column SQL. Hopefully that will fix this
>> issue as well.
>>
>>
>>> - The column datatype dependency does not get cleared upon selection of
>>> another datatype.
>>>
>> For example, if I select numeric and gives the length and precision.
>>> After that I change the dat-type then, dependent fields should be get
>>> cleared.
>>>
>> Fixed.
>>
>>
>>>
>>> - The column datatype does not get displayed in the properties and edit
>>> mode if the length and precision are given while creating a column.
>>>
>>
>> TODO ( I cannot fix this blindly as this might introduce another issue(s)
>> in column node. I will need Murtuza's help as he has worked on column node)
>>
>>
>>>
>>> - Statistics is showing null value even after having value.
>>>
>> Fixed
>>
>>
>>>
>>> - if the check constraints are not validated then put proper icon in
>>> tree and also it should be validated in edit mode.
>>>
>> Not reproducible.
>>
>>
>>>
>>> NOTE: I have not checked the Indexes, Triggers and Rules nodes as I do
>>> not have much knowledge about it.
>>>
>>>
>>
>>
>>> Thanks,
>>> Khushboo
>>>
>>> On Fri, May 13, 2016 at 5:24 PM, Harshal Dhumal <
>>> harshal(dot)dhumal(at)enterprisedb(dot)com> wrote:
>>>
>>>> Hi
>>>>
>>>> PFA attached patches for table and it's child nodes with python 2.7
>>>> compatibility.
>>>>
>>>>
>>
>
>
> --
> 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
>
>