Re: [pgAdmin4] [Patch]: Grant Wizard

From: Surinder Kumar <surinder(dot)kumar(at)enterprisedb(dot)com>
To: Dave Page <dpage(at)pgadmin(dot)org>
Cc: Ashesh Vashi <ashesh(dot)vashi(at)enterprisedb(dot)com>, pgadmin-hackers <pgadmin-hackers(at)postgresql(dot)org>
Subject: Re: [pgAdmin4] [Patch]: Grant Wizard
Date: 2016-04-08 17:59:57
Message-ID: CAM5-9D_TmnFfVEw1OXTL-f4jgtCiE+e2rQuSKa4yqPX7ZsSBNg@mail.gmail.com
Views: Raw Message | Whole Thread | Download mbox | Resend email
Thread:
Lists: pgadmin-hackers

Hi

PFA patch with resolved review comments.

On Fri, Apr 8, 2016 at 12:52 AM, Dave Page <dpage(at)pgadmin(dot)org> wrote:

> Hi
>
> Nearly there :-). Assuming no regressions, I believe we'll be ready to
> commit once the following issues are resolved:
>
> - The select object grid should fill the available vertical space in the
> dialogue - at present there's a gap below it.
>
Done

>
> - When scrolling to the top of bottom of the select object grid, the
> dialogue jumps up or down the screen a little, if the dialogue has been
> resized to a larger size.
>
As this change is generic for all alertifyjs dialogs, I have added its
style css in overrides.css. Done

>
> - Please allow the wizard to be opened from a Database node, in which case
> objects from all schemas should be listed. This will also require support
> for schemas themselves.
>
Done

>
> - When selecting privileges, each time I click on a checkbox, the row
> closes. Similar grids elsewhere in the app close the row when the cell
> loses focus, *however*, that is also the incorrect behaviour - the row
> should only close when the row itself loses focus (and should open when it
> gets focus).
>
I checked the row closes either when gets clicked on row or outside row, it
doesn't seems to close on click on checkbox.

>
> - The message:
>
> Please select objects from the below list.
>
> should read:
>
> Please select objects from the list below.
>
Done

>
> - The message:
>
> Following query will be executed on the database server for the selected
> objects, and privileges. Please click on Finish to complete the process.
>
> should read:
>
> The SQL below will be executed on the database server to grant the
> selected privileges. Please click on <b>Finish</b> to complete the process.
>
Done

>
> Thanks!
>
> On Thu, Apr 7, 2016 at 6:10 PM, Surinder Kumar <
> surinder(dot)kumar(at)enterprisedb(dot)com> wrote:
>
>> Hi Dave,
>>
>> Please find updated patch with above issue resolved.
>>
>> On Thu, Apr 7, 2016 at 7:47 PM, Dave Page <dpage(at)pgadmin(dot)org> wrote:
>>
>>> Hi
>>>
>>> On Wed, Apr 6, 2016 at 12:37 PM, Surinder Kumar <
>>> surinder(dot)kumar(at)enterprisedb(dot)com> wrote:
>>>
>>>> Hi
>>>>
>>>> PFA updated patch with resolved review comments.
>>>>
>>>> On Tue, Apr 5, 2016 at 11:06 AM, Ashesh Vashi <
>>>> ashesh(dot)vashi(at)enterprisedb(dot)com> wrote:
>>>>
>>>>> On Wed, Mar 30, 2016 at 5:14 PM, Surinder Kumar <
>>>>> surinder(dot)kumar(at)enterprisedb(dot)com> wrote:
>>>>>
>>>>>> Hi,
>>>>>>
>>>>>> Please find updated patch.
>>>>>>
>>>>>> This patch has following changes:
>>>>>> 1. Improved code commenting.
>>>>>> 2. Properly handling memory leak issues in js code.
>>>>>>
>>>>> Hi Surinder,
>>>>>
>>>>> As discussed offline, here are the list of some of the review comments:
>>>>>
>>>>> * CSS should be relative to its parent element. Please make sure -
>>>>> whenever you make
>>>>> some changes in CSS, it should not affect the existing CSS unless
>>>>> discussed.
>>>>>
>>>> Done
>>>>
>>>>>
>>>>> * Change class name for 'error_msg_div' as it is common name. Please
>>>>> name a class
>>>>> with prefixed as the module name.
>>>>>
>>>> Done
>>>>
>>>>>
>>>>> * Add comments for the blow line changed in node.ui.js file. Always
>>>>> add logical
>>>>> explanation for a change as a comment for any changes.
>>>>> *while(p && p.length > 0) {*
>>>>>
>>>> Done
>>>>
>>>>>
>>>>> * Please make sure, we wrap the code around 80 characters for better
>>>>> readability.
>>>>> Line length should not be greater than 80 characters.
>>>>>
>>>> Done
>>>>
>>>>>
>>>>> * Put the allowed ACLs logic with server version support. We need to
>>>>> be flexible
>>>>> enough to accommodate possible future change in ACLs.
>>>>>
>>>> Done
>>>>
>>>>>
>>>>> * Avoid using name as reference in each of the given. It will make
>>>>> the search faster
>>>>> in the database and less prone to character conversion issue.
>>>>> i.e.
>>>>> Use schema/namespace OID instead of nspname, object OID instead of
>>>>> their name.
>>>>>
>>>> Done
>>>>
>>>>>
>>>>> * Use separate templates for each type of objects.
>>>>>
>>>> Done
>>>>
>>>>>
>>>>> * Use the existing functionalities as much as possible instead of
>>>>> introducing new
>>>>> one. That will make the code/results consistent across the
>>>>> application.
>>>>> i.e.
>>>>> Use existing 'parse_priv_to_db' method, instead of creating new one.
>>>>>
>>>> Done
>>>>
>>>>>
>>>>> * Please remove unnecessary suffixed white-spaces.
>>>>>
>>>> Done
>>>>
>>>
>>> I get the attached error in the browser console when selecting "Grant
>>> Wizard" from the menu when the current object is a schema. I've tried all
>>> the normal refreshing/restarting.
>>>
>>> [image: Inline image 1]
>>>
>>>
>>> --
>>> Dave Page
>>> Blog: http://pgsnake.blogspot.com
>>> Twitter: @pgsnake
>>>
>>> EnterpriseDB UK: http://www.enterprisedb.com
>>> The Enterprise PostgreSQL Company
>>>
>>
>>
>
>
> --
> Dave Page
> Blog: http://pgsnake.blogspot.com
> Twitter: @pgsnake
>
> EnterpriseDB UK: http://www.enterprisedb.com
> The Enterprise PostgreSQL Company
>

Attachment Content-Type Size
grant_wizard_v9.patch application/octet-stream 89.7 KB

In response to

Responses

Browse pgadmin-hackers by date

  From Date Subject
Next Message Surinder Kumar 2016-04-11 07:41:40 Re: [pgAdmin4][Patch] - Disable PrivilegeControl for nodes visible under catalog
Previous Message Dave Page 2016-04-08 16:19:29 Re: [pgAdmin4] [Patch] Implementation of the Data Grid and Query Tool