Subject: [pgAdmin4] [Patch]: Grant Wizard

From: Surinder Kumar <[email-masked]>
Date: Thu, 3 Mar 2016 18:19:40 +0530

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.

Thanks
Surinder Kumar

-- 
Sent via pgadmin-hackers mailing list ([email-masked])
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgadmin-hackers

---

From: Dave Page <[email-masked]>
Date: Fri, 4 Mar 2016 13:39:43 +0000

Hi

> 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

- 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

Thanks.

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

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

---

From: Dave Page <[email-masked]>
Date: Fri, 4 Mar 2016 14:36:09 +0000

A couple of corrections below <sigh>

> e.g. server-groups. s/grantwizard/grant-wizard/g

That should be an underscore, not a hyphen:

s/grantwizard/grant_wizard/g

> /web/pgadmin/tools/grant-wizard/static/css/grant-wizard.css

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

---

From: Surinder Kumar <[email-masked]>
Date: Tue, 8 Mar 2016 10:44:58 +0530

Hi

PFA patch with following changes:

1. Extends SqlTabControl to write a new function 'onWizardChange',
instead of writing it in backform.pgadmin.js file.

Also, validations in privilegeControl are not working properly, when
validations gets fixed, I will send an updated patch.

This is complete patch, as Khusboo's patch also merged in attached patch and
patches of "Sequence" and "Functions" macros are already committed.

> > and usable by any module.
Fixed.

> > output when the module is dynamically loaded.
Fixed.

> > review all comments and code for such small details.
Done

> > should be centralised, as I've seen it in at least one other module.
Removed it, as far as it is not required.

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

---

From: Surinder Kumar <[email-masked]>
Date: Tue, 8 Mar 2016 15:30:05 +0530

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.

---

From: Dave Page <[email-masked]>
Date: Tue, 8 Mar 2016 13:27:27 +0000

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

---

From: Surinder Kumar <[email-masked]>
Date: Tue, 8 Mar 2016 19:18:59 +0530

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.

---

From: Dave Page <[email-masked]>
Date: Wed, 9 Mar 2016 17:34:38 +0000

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?

---

From: Surinder Kumar <[email-masked]>
Date: Thu, 10 Mar 2016 14:52:05 +0530

Hi,

PFA patch after rebasing.

It depends on whether we need to enable grant wizard for all nodes under
database level or
enable it for specific nodes under database level.

---

From: Surinder Kumar <[email-masked]>
Date: Thu, 10 Mar 2016 15:20:54 +0530

Please apply Khusboo's patch for "Privileges macros under Schema" before
using grant wizard patch.

On Thu, Mar 10, 2016 at 2:52 PM, Surinder Kumar <
[email-masked]> wrote:

>> >> > I forgot to add 'node.ui.js' file in the previous patch, please
>> ignore
>> >> > the
>> >> > previous patch.

---

From: Dave Page <[email-masked]>
Date: Thu, 10 Mar 2016 11:56:50 +0000

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

- Why is the closed button in an odd position (see File -> Test Alert
for comparison)

- Why are we using a scrolling list AND pagination? I think a
scrolling list alone should be fine.

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

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

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

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

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

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

- The buttons appear to have a smaller corner radius than those on the
main browser, or in other dialogues.

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

- Why do the URLs have a /wizard prefix? I think that should be removed.

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

Thanks.

---

From: Surinder Kumar <[email-masked]>
Date: Mon, 28 Mar 2016 14:09:43 +0530

Hi Dave,

Please find updated patch with suggested changes.

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

> scrolling list alone should be fine.
Pagination is removed.

Fixed.

> scrollbar on the grid itself (not at the bottom of the dialogue
I have fixed it and scrollbar is now on the grid itself.

Fixed.

> multiple object types, it should show the available options for only
Implemented this feature.

Done

Fixed.

Fixed.

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

> both grant_wizard.js and allowed_acl.json. Can we just use
Yes, allowed_acl.json is now used in grant_wizard.js.

---

From: Surinder Kumar <[email-masked]>
Date: Wed, 30 Mar 2016 17:14:58 +0530

Hi,

Please find updated patch.

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

---

From: Ashesh Vashi <[email-masked]>
Date: Tue, 5 Apr 2016 11:06:31 +0530

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

---

From: Surinder Kumar <[email-masked]>
Date: Wed, 6 Apr 2016 17:07:35 +0530

Hi

PFA updated patch with resolved review comments.

On Tue, Apr 5, 2016 at 11:06 AM, Ashesh Vashi <[email-masked]
> wrote:

> * CSS should be relative to its parent element. Please make sure -
> whenever you make
Done

>
> * Change class name for 'error_msg_div' as it is common name. Please name
> a class
Done

Done

Done

Done

Done

Done

Done

Done

---

From: Dave Page <[email-masked]>
Date: Thu, 7 Apr 2016 15:17:06 +0100

Hi

On Wed, Apr 6, 2016 at 12:37 PM, Surinder Kumar <
[email-masked]> wrote:

> On Tue, Apr 5, 2016 at 11:06 AM, Ashesh Vashi <
> [email-masked]> wrote:
>
>> * Change class name for 'error_msg_div' as it is common name. Please
>> name a class
>>   Use schema/namespace OID instead of nspname, object OID instead of
>> their name.
>>
> 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.

---

From: Surinder Kumar <[email-masked]>
Date: Thu, 7 Apr 2016 22:40:31 +0530

Hi Dave,

Please find updated patch with above issue resolved.

---

From: Dave Page <[email-masked]>
Date: Thu, 7 Apr 2016 20:22:24 +0100

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 <
[email-masked]> wrote:

>>>> * Add comments for the blow line changed in node.ui.js file. Always
>>>> add logical

---

From: Surinder Kumar <[email-masked]>
Date: Fri, 8 Apr 2016 23:29:57 +0530

Hi

PFA patch with resolved review comments.

Done

> resized to a larger size.
As this change is generic for all alertifyjs dialogs, I have added its
style css in overrides.css. Done

Done

> should only close when the row itself loses focus (and should open when it
I checked the row closes either when gets clicked on row or outside row, it
doesn't seems to close on click on checkbox.

Done

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

>>>>> * Put the allowed ACLs logic with server version support. We need to
>>>>> be flexible
>>>>> * Avoid using name as reference in each of the given. It will make
>>>>> the search faster

---

From: Dave Page <[email-masked]>
Date: Wed, 13 Apr 2016 16:11:58 +0100

Hi

>> dialogue - at present there's a gap below it.

This does not seem to be fixed (see attached screenshot). In fact, the gap
gets bigger proportionally as the window is resized.

> I checked the row closes either when gets clicked on row or outside row,
> it doesn't seems to close on click on checkbox.
>

This is still broken for me. I'm using Chrome on OS X. As soon as I click
any checkbox, the row closes, and I have to click again to see the
checkboxes again.

One additional issue:

- If I select some objects and permissions (in my case, everything in a PEM
database, and ALL), hit Next so I can see the SQL, then hit back *twice*, I
see the second attached screenshot.

I'm going to commit the code with these issues as they are largely
cosmetic. Please submit a patch to fix them (I'll add a new card to our
internal Kanban chart).

Thanks.

---

From: Surinder Kumar <[email-masked]>
Date: Fri, 15 Apr 2016 21:33:50 +0530

Hi,

PFA patch with following issues fixed:

   1. Fixed  'get_schemas.sql' template for PPAS. It was fetching system
   level schemas due to wrong sql query.
   2. Allowed the grant wizard to open on procedures node if server type is
   PPAS.

Also, I will send another patch for the remaining issues.

On Wed, Apr 13, 2016 at 8:41 PM, Dave Page <[email-masked]> wrote:

> - If I select some objects and permissions (in my case, everything in a
> PEM database, and ALL), hit Next so I can see the SQL, then hit back
> *twice*, I see the second attached screenshot.
>

---

From: Dave Page <[email-masked]>
Date: Sat, 16 Apr 2016 10:01:43 +0100

Thanks, patch applied.

On Friday, April 15, 2016, Surinder Kumar <[email-masked]>
wrote:

>    2. Allowed the grant wizard to open on procedures node if server type
>    is PPAS.
>
> On Wed, Apr 13, 2016 at 8:41 PM, Dave Page <[email-masked]
> <javascript:_e(%7B%7D,'cvml',[email-masked]');>> wrote:
>
>> Hi
>>
>> [email-masked]
>> <javascript:_e(%7B%7D,'cvml',[email-masked]');>> wrote:
>>
>>> Hi
>>>
>>> On Fri, Apr 8, 2016 at 12:52 AM, Dave Page <[email-masked]
>>> <javascript:_e(%7B%7D,'cvml',[email-masked]');>> wrote:
>>>
>>>> Hi
>>>>
>>>> - The select object grid should fill the available vertical space in
>>>> the dialogue - at present there's a gap below it.
>>>>
>>> Done
>>>
>>
>> This does not seem to be fixed (see attached screenshot). In fact, the
>> gap gets bigger proportionally as the window is resized.
>>
>>
>>>

---

From: Surinder Kumar <[email-masked]>
Date: Fri, 6 May 2016 15:00:34 +0530

Hi,

PFA patch with following issues fixed:
1. As SqlTab Control is renamed to SqlCtrl, throws error on grant wizard
close. it is Fixed.
2. Moved grant wizard specific css from wizard.css to grant_wizard.css.

Fixed

> any checkbox, the row closes, and I have to click again to see the
I checked with Chrome on OS X, windows and ubuntu, it is working fine. Can
you please send me screenshot if possible?

> *twice*, I see the second attached screenshot.
I followed the same steps and when hit back twice on *previous* button of
SQL page, it takes me to first page which is right.
It would be easy for me to find out the exact cause if you send me output
of browser console panel.

---

From: Surinder Kumar <[email-masked]>
Date: Wed, 11 May 2016 22:58:43 +0530

Hi,

PFA updated patch

*Change*: removed '*ajs_content*' css from *wizard.css* causing padding
issue in alertify dialog.
Thanks murtuza for reporting.