Re: [pgadmin-hackers] Declarative partitioning in pgAdmin4

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.

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.

--

Thanks & Regards,

Ashesh Vashi
EnterpriseDB INDIA: Enterprise PostgreSQL Company
<http://www.enterprisedb.com>

*http://www.linkedin.com/in/asheshvashi*
<http://www.linkedin.com/in/asheshvashi>

On Fri, Jun 23, 2017 at 6:55 PM, Harshal Dhumal <
harshal(dot)dhumal(at)enterprisedb(dot)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(at)pivotal(dot)io> wrote:
>
>>
>>
>> On Mon, Jun 19, 2017 at 1:59 AM Akshay Joshi <
>> akshay(dot)joshi(at)enterprisedb(dot)com> wrote:
>>
>>> On Fri, Jun 16, 2017 at 11:16 PM, Shirley Wang <swang(at)pivotal(dot)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
>> [image: expression.png]
>> 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.
>>
>>
>>
>>>
>>>> [image: coluns_partitioning.png]
>>>> 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.
>>
>>>
>