Re: [pgAdmin4] [Patch]: Grant Wizard

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

>
> --
>
> 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 Mon, Mar 28, 2016 at 2:09 PM, Surinder Kumar <
>> surinder(dot)kumar(at)enterprisedb(dot)com> wrote:
>>
>>> Hi Dave,
>>>
>>> Please find updated patch with suggested changes.
>>>
>>> On Thu, Mar 10, 2016 at 5:26 PM, Dave Page <dpage(at)pgadmin(dot)org> wrote:
>>>
>>>> On Thu, Mar 10, 2016 at 9:50 AM, Surinder Kumar
>>>> <surinder(dot)kumar(at)enterprisedb(dot)com> wrote:
>>>> > Please apply Khusboo's patch for "Privileges macros under Schema"
>>>> before
>>>> > using grant wizard patch.
>>>>
>>>> Thanks, that works. Some (hopefully final) feedback:
>>>>
>>>> - Can we add a side-image? Not sure what yet - just a placeholder for
>>>> now until I come up with something.
>>>>
>>> Already a placeholder for left side image.
>>>
>>>>
>>>> - Why is the closed button in an odd position (see File -> Test Alert
>>>> for comparison)
>>>>
>>> Fixed
>>>
>>>>
>>>> - Why are we using a scrolling list AND pagination? I think a
>>>> scrolling list alone should be fine.
>>>>
>>> Pagination is removed.
>>>
>>>>
>>>> - The grid sizing is wrong. See how the scrollbar on the right in the
>>>> screenshot is off the edge of the dialogue, and there's a horizontal
>>>> scrollbar?
>>>>
>>> Fixed.
>>>
>>>>
>>>> - We shouldn't truncate object names as that can be ambiguous. The
>>>> column should extend as necessary, and there should be a horizontal
>>>> scrollbar on the grid itself (not at the bottom of the dialogue
>>>> content).
>>>>
>>> I have fixed it and scrollbar is now on the grid itself.
>>>
>>>>
>>>> - Function names should include the parameters, as they are part of
>>>> the identifier. Without, it can be ambigous - e.g. do_stuff(int) vs.
>>>> do_stuff(text).
>>>>
>>> Fixed.
>>>
>>>>
>>>> - If I select only functions (for example), the Privileges panel
>>>> should only list privileges available for functions. If I select
>>>> multiple object types, it should show the available options for only
>>>> those object types.
>>>>
>>> Implemented this feature.
>>>
>>>>
>>>> - If I select functions and tables, and then choose (for example),
>>>> usage and truncate, it will attempt to set usage on tables and
>>>> truncate on functions. It should only attempt to set privileges on the
>>>> objects for which they are appropriate.
>>>>
>>> Done
>>>
>>>>
>>>> - On the last page, the Next button is disabled. It is turned a
>>>> marginally darker blue, but also highlights on mouse-over. The change
>>>> in shade is so subtle it's hard to see, and the highlight implies the
>>>> button is active when it isn't.
>>>>
>>> Fixed.
>>>
>>>>
>>>> - The buttons appear to have a smaller corner radius than those on the
>>>> main browser, or in other dialogues.
>>>>
>>> Fixed.
>>>
>>>>
>>>> - Button labels should have an &nbsp; before them to properly space
>>>> the label from the icon (or better yet, this should be done in CSS,
>>>> though that would also need to be done elsewhere).
>>>>
>>> done with css.
>>>
>>>>
>>>> - Why do the URLs have a /wizard prefix? I think that should be removed.
>>>>
>>> Removed prefix.
>>>
>>>>
>>>> - The available privileges for each object type seem to be defined in
>>>> both grant_wizard.js and allowed_acl.json. Can we just use
>>>> allowed_acl.json?
>>>>
>>> Yes, allowed_acl.json is now used in grant_wizard.js.
>>>
>>>>
>>>> Thanks.
>>>>
>>>> --
>>>> Dave Page
>>>> Blog: http://pgsnake.blogspot.com
>>>> Twitter: @pgsnake
>>>>
>>>> EnterpriseDB UK: http://www.enterprisedb.com
>>>> The Enterprise PostgreSQL Company
>>>>
>>>
>>>
>>
>>
>> --
>> 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
>>
>>
>