Re: [pgAdmin4] [Patch]: Grant Wizard

Поиск

Список

Период

Сортировка

От	Surinder Kumar
Тема	Re: [pgAdmin4] [Patch]: Grant Wizard
Дата	8 марта 2016 г. 08:15:36
Msg-id	CAM5-9D_D3TGq9-DbcR+07=p2cgT=yEtmEJnHU9rtmov_nby9+g@mail.gmail.com обсуждение исходный текст
Ответ на	Re: [pgAdmin4] [Patch]: Grant Wizard (Dave Page <dpage@pgadmin.org>)
Ответы	Re: [pgAdmin4] [Patch]: Grant Wizard (Surinder Kumar <surinder.kumar@enterprisedb.com>)
Список	pgadmin-hackers

Дерево обсуждения

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@pgadmin.org> wrote:

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.

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