Re: [pgAdmin4][Patch]: Foreign Data Wrapper

Hi Dave,

Please find attached patch file with all the fixes.
Let us know for any comments.

Thanks,
Neel Patel

On Thu, Mar 10, 2016 at 5:36 PM, Dave Page <dpage(at)pgadmin(dot)org> wrote:

> Hi
>
> On Thu, Mar 10, 2016 at 7:33 AM, Neel Patel <neel(dot)patel(at)enterprisedb(dot)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(at)pgadmin(dot)org> wrote:
> >>
> >> Hi
> >>
> >> On Tue, Feb 23, 2016 at 5:24 PM, Neel Patel <
> neel(dot)patel(at)enterprisedb(dot)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
>