MIME-Version: 1.0
In-Reply-To: 
 <CAM5-9D8oYyEaL89coD9FozPDziGhYutYAat9dMP9dAwhOp5OKQ@mail.gmail.com>
References: 
 <CAM5-9D8oYyEaL89coD9FozPDziGhYutYAat9dMP9dAwhOp5OKQ@mail.gmail.com>
Date: Fri, 4 Mar 2016 13:39:43 +0000
Message-ID: 
 <CA+OCxozy4aH9=2T0Sp_WworU_3UQaaHBcQXTDhEUKHAkZJT8ow@mail.gmail.com>
Subject: Re: [pgAdmin4] [Patch]: Grant Wizard
From: Dave Page <dpage@pgadmin.org>
To: Surinder Kumar <surinder.kumar@enterprisedb.com>
Cc: pgadmin-hackers <pgadmin-hackers@postgresql.org>
Content-Type: text/plain; charset=UTF-8
Precedence: bulk
Sender: pgadmin-hackers-owner@postgresql.org

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

- 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


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