Re: [pgAdmin4][Patch]: Foreign Data Wrapper - Mailing list pgadmin-hackers

Hi

On Thu, Mar 10, 2016 at 7:33 AM, Neel Patel <neel.patel@enterprisedb.com> wrote:
> Hi Dave,
>
> Thank you for the reviewing the patch.
> Please find inline comments and attached updated patch file after fixing all
> the comments.
>
> Do let us know in case of any comments.
>
> Thanks,
> Neel Patel
>
> On Fri, Mar 4, 2016 at 10:19 PM, Dave Page <dpage@pgadmin.org> wrote:
>>
>> Hi
>>
>> On Tue, Feb 23, 2016 at 5:24 PM, Neel Patel <neel.patel@enterprisedb.com>
>> wrote:
>>>
>>> Hi,
>>>
>>> Please find attached patch file for the following three nodes.
>>>
>>> Foreign Data Wrappers
>>> Foreign Servers
>>> User Mapping
>>>
>>> With this patch, we have implemented "Dependencies", "Dependent" tab and
>>> proper comments has been added.
>>>
>>> Do review it and let us know for any comments.
>>
>>
>> This seems to be nearly ready now. Some feedback below - note that a
>> couple of the issues may be caused by infrastructure code, in which case
>> please do fix them, but feel free to put them in a different patch:
>>
>> - When adding a User Mapping, you cannot specify an empty option, e.g. a
>> blank password.
>
>
> Fixed. User will not be able to save with blank password. "Save" button will
> be disabled and error message will be displayed.

That's exactly what shouldn't happen - what if my password is actually
blank? I should be able to specify an option with an empty value.

>> - When granting USAGE to PUBLIC on a foreign server, the WITH GRANT OPTION
>> is set, despite not being selected. e.g. adding "U" permissions for "public"
>> results in:
>>
>> GRANT ALL ON FOREIGN SERVER redis_server TO public;
>> GRANT ALL ON FOREIGN SERVER redis_server TO public WITH GRANT OPTION;
>
>
> Fixed.
>
>>
>>
>> - Why all the extra blank lines in this SQL? I would expect to see only 2,
>> between the ALTER/COMMENT/GRANT sections.
>>
>> ====
>> ALTER SERVER redis_server
>>     VERSION 'Fooo';
>>
>> COMMENT ON SERVER redis_server
>>     IS 'Redis Server x';
>>
>>
>>
>>
>>
>>
>> GRANT ALL ON FOREIGN SERVER redis_server TO public;
>> GRANT ALL ON FOREIGN SERVER redis_server TO public WITH GRANT OPTION;
>>
>>
>> ====
>
>
> Fixed.
>
>>
>>
>> - Error messages from the server are not displayed properly - e.g:
>>
>> invalid option "server"
>> HINT:  Valid options in this context are: <none>
>>
>> Is displayed as:
>>
>> invalid option "server" HINT: Valid options in this context are:
>>
>> (Notice the lack of "<none>")
>
>
> Here we are displaying the generic error message text received from database
> server. We can not add <none> to message text but "HINT" will be displayed
> in new line.
>
> e.g. If you execute below command in PG 9.5 through "plpgsql" then database
> server will give the error message without valid context.
>
> CREATE FOREIGN DATA WRAPPER test_fdw VALIDATOR postgresql_fdw_validator
> OPTIONS (server '123');
>
> Error message will be as below from database server.
>
> ERROR:  invalid option "server"
> HINT:  Valid options in this context are:
>
>> - A foreign server object is not listed as being dependent upon the FDW
>> it's defined as using (but, pgAdmin 3 gets this wrong too).
>
>
> Fixed.
>
>>
>>
>> - The "Options" tabs have a hint message of "Please enter some value!",
>> whether the focus is in the Option or Value field. Please fix to display:
>>
>> Please enter an option name.
>>
>> or
>>
>> Please enter a value.
>>
>> Based on the current focus. Note also that there should not be an
>> exclamation mark.
>
>
> Fixed.
>
>>
>>
>> Thanks!
>>
>> --
>> Dave Page
>> Blog: http://pgsnake.blogspot.com
>> Twitter: @pgsnake
>>
>> EnterpriseDB UK: http://www.enterprisedb.com
>> The Enterprise PostgreSQL Company
>
>



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

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