Thread: [pgAdmin4] [Patch]: Grant Wizard

A couple of corrections below <sigh>

On Fri, Mar 4, 2016 at 1:39 PM, Dave Page <dpage@pgadmin.org> wrote:
> Hi
>
> On Thu, Mar 3, 2016 at 12:49 PM, Surinder Kumar
> <surinder.kumar@enterprisedb.com> wrote:
>> Hi,
>>
>> PFA patch for Grant Wizard.
>>
>> Before applying grant wizard patch:
>>
>>         1. Apply patch for "wizard JS file" which Khushboo had shared with
>> Ashesh personally.
>>             I am using that patch with few changes in that. Ashesh will
>> review
>>             and commit that patch.
>>
>>         2. Apply patches of "Sequence" and "Functions" macros.
>>
>>
>> Please review the patch and Let me know for any comments.
>
> Initial feedback:
>
> - Why does this add specific knowledge of the Grant Wizard to the
> Browser module? It should be a plugin that loads itself at runtime.
> Any changes to the browser to support that should be entirely generic
> and usable by any module.

>
> - The comment above also applies to the core templates. CSS should be
> advertised by the plugin, and the browser can add it into the rendered
> output when the module is dynamically loaded.

>
> - +""" Implements Grant Wizard""" - why the leading space? Please
> review all comments and code for such small details.

>
> - The Python code to detect and alias various Python 2/3 classes
> should be centralised, as I've seen it in at least one other module.
>

> - In other module names, we've separated multiple words with a hypen.
> e.g. server-groups. s/grantwizard/grant-wizard/g

That should be an underscore, not a hyphen:

s/grantwizard/grant_wizard/g 

>
> - The published routes for this module are all under
>
> - "GET /browser/static/js/wizard.js HTTP/1.1" 404 -
>
> - For consistency, when naming CSS/JS files that are core to a module,
> please use the module name, e.g.
> /web/pgadmin/tools/grant-wizard/static/css/grant-wizard.css

/web/pgadmin/tools/grant_wizard/static/css/grant_wizard.css

--
Dave Page
Blog: http://pgsnake.blogspot.com
Twitter: @pgsnake

EnterpriseDB UK: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

Sorry Surinder - can you rebase this please? I think it was broken by
8a7ec6b45221f042bc39c9bce2c577e12b43cc3a :-( 

How much work would it be to enable it to work at database level as well?

On Tue, Mar 8, 2016 at 1:48 PM, Surinder Kumar
<surinder.kumar@enterprisedb.com> wrote:
> The menu option is dependent only on certain nodes like:
>
> 1. Schema Node
> 2. Views Collection Node
> 3. Tables Collection Node
> 4. Sequences Collection Node
> 5. Functions Collection Node
>
> It will be enabled on click of above listed nodes.
>
> On Tue, Mar 8, 2016 at 6:57 PM, Dave Page <dpage@pgadmin.org> wrote:
>>
>> On Tue, Mar 8, 2016 at 10:00 AM, Surinder Kumar
>> <surinder.kumar@enterprisedb.com> wrote:
>> > Hi,
>> >
>> > I forgot to add 'node.ui.js' file in the previous patch, please ignore
>> > the
>> > previous patch.
>> > Attached is the new patch,
>> >
>> > Please review the patch and let me know for any comments.
>>
>> Is this dependent on any other patches? The menu option never seems to
>> be enabled for me when I test against git-master.
>>
>> --
>> 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

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

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

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

Hi

PFA updated patch with resolved review comments.

On Tue, Apr 5, 2016 at 11:06 AM, Ashesh Vashi <ashesh.vashi@enterprisedb.com> wrote:

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.

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 

 

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.

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

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

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

- The message:

Please select objects from the below list.

should read:

Please select objects from the list below.

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

Thanks!

On Thu, Apr 7, 2016 at 6:10 PM, Surinder Kumar <surinder.kumar@enterprisedb.com> wrote:

Hi Dave,

Please find updated patch with above issue resolved.

On Thu, Apr 7, 2016 at 7:47 PM, Dave Page <dpage@pgadmin.org> wrote:

Hi

On Wed, Apr 6, 2016 at 12:37 PM, Surinder Kumar <surinder.kumar@enterprisedb.com> wrote:

Hi

PFA updated patch with resolved review comments.

On Tue, Apr 5, 2016 at 11:06 AM, Ashesh Vashi <ashesh.vashi@enterprisedb.com> wrote:

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.

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.

 

--

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

Hi

PFA patch with resolved review comments.

On Fri, Apr 8, 2016 at 12:52 AM, Dave Page <dpage@pgadmin.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 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.