Received: from malur.postgresql.org ([217.196.149.56]) by arkaria.postgresql.org with esmtp (Exim 4.84_2) (envelope-from ) id 1anJfr-00034z-7A for pgadmin-hackers@arkaria.postgresql.org; Tue, 05 Apr 2016 05:37:11 +0000 Received: from localhost ([127.0.0.1] helo=postgresql.org) by malur.postgresql.org with smtp (Exim 4.84_2) (envelope-from ) id 1anJfq-0002Xo-Gq for pgadmin-hackers@arkaria.postgresql.org; Tue, 05 Apr 2016 05:37:10 +0000 Received: from makus.postgresql.org ([2001:4800:1501:1::229]) by malur.postgresql.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_CBC_SHA384:256) (Exim 4.84_2) (envelope-from ) id 1anJfc-0002JC-29 for pgadmin-hackers@postgresql.org; Tue, 05 Apr 2016 05:36:56 +0000 Received: from mail-io0-x229.google.com ([2607:f8b0:4001:c06::229]) by makus.postgresql.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_CBC_SHA1:256) (Exim 4.84_2) (envelope-from ) id 1anJfY-0008Ae-AO for pgadmin-hackers@postgresql.org; Tue, 05 Apr 2016 05:36:54 +0000 Received: by mail-io0-x229.google.com with SMTP id q128so6756393iof.3 for ; Mon, 04 Apr 2016 22:36:52 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=enterprisedb-com.20150623.gappssmtp.com; s=20150623; h=mime-version:in-reply-to:references:from:date:message-id:subject:to :cc; bh=5CFCcDjZ6C4kn6qGaKSCVcx0jvmME77lWJr/bTNlOaM=; b=j/TlZrWU3W9SU3NCjuJ64M1V8YtkpbNKlm815bBL6k5xM8/r+TXso9Zz3WR+S3P9KC 88Y+jZJaeJ8dRki77AjowMO1ZgZ4LiSyvXLgvzfWmEMcRFgoTX4M9NKY4QcN8Vrm7sWF 5/wid95Q6vCb1vB8i8oxVzsQZWNCH2dK+6N8Q+NDo+D0N8iFSVfSuAlk665SEQm3N60T 4pglTOHsgTbWQCA4vbdDT0EGib40Tw7CV2XOaZN/lNRPvB4uAcussOVUpxFJHM+Gv4Xw 5d55x1sUnvb/Ze+sDG40UKQyHCJyPrAW6MC/d02+qeoc1Pn/t4rWMi/Z+5B/VndnwzrW QUPw== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20130820; h=x-gm-message-state:mime-version:in-reply-to:references:from:date :message-id:subject:to:cc; bh=5CFCcDjZ6C4kn6qGaKSCVcx0jvmME77lWJr/bTNlOaM=; b=haR6RZxyN/3dvDrhWWOnxxUOUSTAAESbaDXhIr8CAclvQCjOj8MQXEJd6Bjn7OHmsO tIIZ/WJ4Qw12ulc/mo4A9tQdYaedci1ooOGefBhUVPg3Z8/8BpkQq4OEJX9mwaq3pslH EpuC790fwXJX6aAyECAXDuYdgdpgU0RZzB1wpYh2MWwCoqJeuIWDowUE2jsZCrsuo1uU 38PGuHPz1DZOJluTQA1dZT5Y7opwnk0aOmgt3SoH0LTBBjUnh3pOnwc+JhPsCIqXUVq7 7Jz5qEAoVTASGg2O77bme+T3ndJyw0ShEy3v/6Pg5L0NVsXytPAsNVll42wPFjD8+XMg jjHA== X-Gm-Message-State: AD7BkJKmLH9hKNTIB6vDqpTjVbhyQEtZJMO9Y1NGufmphzKxlXIzNDamvucJk2eRo1QDELOT1gKKWvqzpz/IVq0t X-Received: by 10.107.41.210 with SMTP id p201mr15147969iop.106.1459834611458; Mon, 04 Apr 2016 22:36:51 -0700 (PDT) MIME-Version: 1.0 Received: by 10.107.39.5 with HTTP; Mon, 4 Apr 2016 22:36:31 -0700 (PDT) In-Reply-To: References: From: Ashesh Vashi Date: Tue, 5 Apr 2016 11:06:31 +0530 Message-ID: Subject: Re: [pgAdmin4] [Patch]: Grant Wizard To: Surinder Kumar Cc: Dave Page , pgadmin-hackers Content-Type: multipart/alternative; boundary=001a1141f3a2727801052fb63b38 X-Pg-Spam-Score: -2.6 (--) List-Archive: List-Help: List-ID: List-Owner: List-Post: List-Subscribe: List-Unsubscribe: X-Mailing-List: pgadmin-hackers Precedence: bulk Sender: pgadmin-hackers-owner@postgresql.org --001a1141f3a2727801052fb63b38 Content-Type: text/plain; charset=UTF-8 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 wrote: >> >>> On Thu, Mar 10, 2016 at 9:50 AM, Surinder Kumar >>> 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   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 > > --001a1141f3a2727801052fb63b38 Content-Type: text/html; charset=UTF-8 Content-Transfer-Encoding: quoted-printable
On W= ed, Mar 30, 2016 at 5:14 PM, Surinder Kumar <surinder.kuma= r@enterprisedb.com> wrote:
Hi,

Please find updated patch.=C2=A0

This patch has following changes:=C2=A0
1. Improved code= commenting.
2. Properly handling memory leak issues in js code.<= /div>
Hi Surinder,

As discussed offlin= e, here are the list of some of the review comments:

* CSS should be relative to its pa= rent element. Please make sure - whenever you make
=C2=A0 some chang= es in CSS, it should not affect the existing CSS unless discussed.
* Change class name for 'error_msg_div'=C2=A0as it is common name.= Please name a class
=C2=A0 with prefixed as the module name.=C2=A0

* Add comments for the blow line=C2=A0changed in node.ui.js file. Always add logical
=
=C2=A0 = 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.
=C2=A0 Line length shoul= d not be greater than 80 characters.

* Put the allowed ACLs = logic with server version support. We need to be flexible
=C2=A0 enoug= h to accommodate possible future change in ACLs.

* Avoid usi= ng name as reference in each of the given.=C2=A0It will make the search faster
=C2=A0 in the database and= less prone to character conversion issue.
=C2=A0 i.e.
=C2=A0 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 in= troducing new
=C2=A0 one. That will make the code/results consistent a= cross the application.
=C2=A0 i.e.
=C2=A0 Use existing 'parse= _priv_to_db' method, instead of creating new one.

* Plea= se remove unnecessary suffixed white-spaces.

--

Thanks & Regards,

Ashesh VashiEnterpriseD= B INDIA:=C2=A0Enterprise PostgreSQL Company


http://www.linkedin.com/in/asheshvashi

=C2=A0


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@pgadm= in.org> wrote:
On Thu, Mar 10, 2016= at 9:50 AM, Surinder Kumar
<su= rinder.kumar@enterprisedb.com> wrote:
> Please apply Khusboo's patch for "Privileges macros under Sch= ema" 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 p= laceholder for left side image.

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

- Why are we using a scrolling list AND pagination? I think a
scrolling list alone should be fine.
Pagination= is removed.=C2=A0

- 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.=C2=A0

- 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.= =C2=A0

- 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=C2= =A0

- 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.=C2= =A0

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

- 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).
<= div>done with css.=C2=A0

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

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


--001a1141f3a2727801052fb63b38--