Re: [pgAdmin4] [Patch]: Grant Wizard

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.

* Change class name for 'error_msg_div' as it is common name. Please name a
class
with prefixed as the module name.

* 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) {*

* Please make sure, we wrap the code around 80 characters for better
readability.
Line length should not be greater than 80 characters.

* Put the allowed ACLs logic with server version support. We need to be
flexible
enough to accommodate possible future change in ACLs.

* 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.

* Use separate templates for each type of objects.

* 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.

* Please remove unnecessary suffixed white-spaces.

--

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
>
>