Re: [pgAdmin4] [Patch]: Grant Wizard

On Wed, Mar 30, 2016 at 5:14 PM, Surinder Kumar <surinder.kumar@enterprisedb.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.linkedin.com/in/asheshvashi

 

On Mon, Mar 28, 2016 at 2:09 PM, Surinder Kumar <surinder.kumar@enterprisedb.com> wrote:

Hi Dave,

Please find updated patch with suggested changes.

On Thu, Mar 10, 2016 at 5:26 PM, Dave Page <dpage@pgadmin.org> wrote:

On Thu, Mar 10, 2016 at 9:50 AM, Surinder Kumar
<surinder.kumar@enterprisedb.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@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgadmin-hackers