Re: [pgadmin-hackers] Declarative partitioning in pgAdmin4 - Mailing list pgadmin-hackers

Hi Ashesh

On Fri, Jun 30, 2017 at 12:46 PM, Ashesh Vashi <ashesh.vashi@enterprisedb.com> wrote:

On Thu, Jun 29, 2017 at 5:02 PM, Ashesh Vashi <ashesh.vashi@enterprisedb.com> wrote:

Hi Harshal,

These are initial review comments.

1.

Please share a separate patch for generic code changes from this patch for the following files:

- web/pgadmin/tools/user_management/templates/user_management/js/user_management.js

- web/pgadmin/static/js/backform.pgadmin.js

- web/pgadmin/static/js/backgrid.pgadmin.js

This should be committed as separate functionality, and should not be part of this commit.

      Committed. 

2.

Please put a space after a colon (:) in javascript object definition.

i.e.

{cell: Backgrid.Extension.StringDepCell, cellHeaderClasses: 'width_percent_30'}

3.

Conversion of ptid (partition table OID) to tid (table OID) for its children must not be in 'web/pgadmin/browser/utils.py'  file.

Please create a inherited class of PGChildNodeView, and extend the functionality in it, and use it as the base class for all children of table.

I will keep you posted for further review comments. 

4.

URL definition of the javascript for tables & partition tables utility must be exposed from the table/partition table module, and not from the database module.

i.e.

No changes should be done in the database module for this feature. Hence - I don't expect any change in the file:

web/pgadmin/browser/server_groups/servers/databases/__init__.py

    Fixed.

   Apart from above this patch contains test cases as well.

-- Thanks, Ashesh

--

Thanks & Regards,

Ashesh Vashi
EnterpriseDB INDIA: Enterprise PostgreSQL Company

http://www.linkedin.com/in/asheshvashi

On Fri, Jun 23, 2017 at 6:55 PM, Harshal Dhumal <harshal.dhumal@enterprisedb.com> wrote:

Hi,

Please find attached patch for partition support.

This patch includes all the work done by Akashy and support for child nodes (constraints, rules, index, triggers and partition itself ) under partition node.

Thanks,

-- 

Harshal Dhumal

Sr. Software Engineer

EnterpriseDB India: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

On Tue, Jun 20, 2017 at 12:16 AM, Shirley Wang <swang@pivotal.io> wrote:

On Mon, Jun 19, 2017 at 1:59 AM Akshay Joshi <akshay.joshi@enterprisedb.com> wrote:

On Fri, Jun 16, 2017 at 11:16 PM, Shirley Wang <swang@pivotal.io> wrote:

Looks good. I noticed people clicking back and forth to the columns tab to remember which columns they've created while filling out the Expressions column. It might be better to have a list of the columns and the datatype above the 'Partition Keys' subnode and have columns as a type field rather than a drop down.

   I think we should not duplicate that data as we already have all the information on "Columns" tab and by providing drop down user can select columns from there only. 

Also, I think the fields someone sees after selecting the Key type needs to depend on what they select. Seeing both Column and Expressions type field might lead someone to think they need to fill out both fields.

   We can't, because user can select one column and provide an expression as partition key in this case we will have to show both the columns in subnode control. Anyways when user select columns I have disabled the expression cell and if user selects expression column cell is disabled.  

Ah I see what you mean. What I meant was that the column would change depending on if someone selects Column or Expressions from the dropdown

Can a user select more than one key type? The use case I can see where hiding 'Columns' or 'Expressions' would fail is if someone can create an expression key type and a column key type.

Disabling a feature is one way to guide user behavior, but it doesn't provide enough context for someone to understand why it's disabled. It's better to only display what is absolutely necessary and hide fields that are unrelated to the workflow. 

Typically, disabling a UI element is useful when that disabled UI element also provides some context or value while disabled. In this case, I'm not sure it is.

If hiding options isn't possible, providing some text in the fields (like N/A or --) would be helpful.

 

When is the 'In' column in the Partitions subnode enabled? 

    In case of 'List' Partition. 

It would improve the experience if the 'In' column was removed when a user selects 'Range' partitions then. And then if a user is creating a 'List' partition, 'From/To' should be hidden. In this case, 'From/To' and 'In' are dependent on that first drop down step, so seeing 'In' while on 'Range partitions' (and 'From/To' on 'List partitions') is not providing any value.

 

For the NoteControl on the bottom, what do 'Mode Control' or 'Attach Mode' refer to? And how can I tell the difference between 'Create Mode' and 'Edit Mode'?

   'Mode control' is a switch control in subnode control that should be "Mode switch control". 'Create Mode' is when user creates the new table by clicking create-> table and 'Edit Mode' is when user open the properties dialog for the existing table. In case of 'Edit Mode' there are two ways user can create/attach partitions. In Attach mode we will identify and list down the suitable tables to be attached. 

I see. Describing these various states is great in case a user needs it. What are your thoughts on having it live in the documentation of pgAdmin4 rather than in the dialog? This seems to be the established pattern for all other explanations.

--

Akshay Joshi

Principal Software Engineer 

Phone: +91 20-3058-9517
Mobile: +91 976-788-8246