From: | Surinder Kumar <surinder(dot)kumar(at)enterprisedb(dot)com> |
---|---|
To: | Dave Page <dpage(at)pgadmin(dot)org> |
Cc: | pgadmin-hackers <pgadmin-hackers(at)postgresql(dot)org> |
Subject: | Re: [pgAdmin4] [Patch]: Grant Wizard |
Date: | 2016-03-08 05:14:58 |
Message-ID: | CAM5-9D_D3TGq9-DbcR+07=p2cgT=yEtmEJnHU9rtmov_nby9+g@mail.gmail.com |
Views: | Raw Message | Whole Thread | Download mbox | Resend email |
Thread: | |
Lists: | pgadmin-hackers |
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.
On Fri, Mar 4, 2016 at 8:06 PM, Dave Page <dpage(at)pgadmin(dot)org> wrote:
> A couple of corrections below <sigh>
>
> On Fri, Mar 4, 2016 at 1:39 PM, Dave Page <dpage(at)pgadmin(dot)org> wrote:
> > Hi
> >
> > On Thu, Mar 3, 2016 at 12:49 PM, Surinder Kumar
> > <surinder(dot)kumar(at)enterprisedb(dot)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.
>
Fixed.
> >
> > - 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.
>
Fixed.
> >
> > - +""" Implements Grant Wizard""" - why the leading space? Please
> > review all comments and code for such small details.
>
Done
> >
> > - 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.
> >
>
Removed it, as far as it is not required.
> > - 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
>
Done
>
> --
> Dave Page
> Blog: http://pgsnake.blogspot.com
> Twitter: @pgsnake
>
> EnterpriseDB UK: http://www.enterprisedb.com
> The Enterprise PostgreSQL Company
>
Attachment | Content-Type | Size |
---|---|---|
grant_wizard_v2.patch | application/octet-stream | 61.5 KB |
From | Date | Subject | |
---|---|---|---|
Next Message | Harshal Dhumal | 2016-03-08 07:34:01 | Control for selecting multiple columns [pgadmin4] |
Previous Message | Ashesh Vashi | 2016-03-07 18:50:33 | Re: PATCH: Schema/Catalog Node [pgAdmin4] |