Re: [pgAdmin4] [Patch]: Grant Wizard

Hi,

Please find updated patch.

This patch has following changes:
1. Improved code commenting.
2. Properly handling memory leak issues in js code.

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