Re: [pgAdmin][RM-2341]: Add menu option for starting PSQL - Mailing list pgadmin-hackers

Hi

On Tue, May 11, 2021 at 9:02 AM Akshay Joshi <akshay.joshi@enterprisedb.com> wrote:

Hi Nikhil 

Following are the review comments:

GUI specific:

• We need a panel icon for PSQL like query tool, we can also add that on the browser tree toolbar.
• PSQL Tool menu should be visible for all the child nodes of the database node. Follow the same as Query Tool.
• PSQL tab title should be only database server name as the user can change the database/user from PSQL command, so it's been difficult to update the tab title.
• PSQL connection is still open even if we disconnect the database server from the browser tree.

Code specific:

• Remove an extra space from requirements.txt and package.json
• Documentation needs to be updated to let the user know from where the PSQL tool will open and on which node it is applicable.
• psql/__init__.py check there are so many unused imports please remove them.
• We are not using cheroot so it should be removed from requirements.txt and also remove the import statement from pgAdmin4.py 
  
• Test cases are showing successful but actually, there are some routing errors please check.

A few other things I noticed:

- I was prompted to enter a password. This should be passed in the environment to psql as it is for pg_dump etc.

- There seems to be an issue with terminal compatibility (which I didn't have on my prototype):

ml=# select * from pg_class;

WARNING: terminal is not fully functional

-[ RECORD 1 ]-------+----------------------------------------------

oid                 | 79354

relname             | housing

...

- The panel should honour the styleguide. I'm running in dark mode, and see a jet black background. I would expect to see the same background/foreground colours as the treeview.

- I spotted at least one print() statement that shouldn't be there (debug output should go through the logger) - psql/__init__.py:235

- This seems suspect - why would there be a password in a connection string we've built? And why would it be xxx?

    if 'password=xxx' in conn_attr:

        conn_attr = conn_attr.replace('password=xxx', '')

- There's a thick white line at the bottom of the panel, where a horizontal scrollbar might be if there was one.

- The trailing semi-colon should be removed from: "ERROR: Shell commands are disabled in psql for security;"

Once we're happy with the patch in general, I'll do a string review before committing. In particular, I want to be sure the text in config.py is appropriately worded.

This is shaping up nicely! Good work.

 

On Mon, May 10, 2021 at 7:32 PM Nikhil Mohite <nikhil.mohite@enterprisedb.com> wrote:

Hi Dave/ Team,

PFA updated patch, sorry for the inconvenience, while cleanup I removed the unwanted libraries but forgot to remove the code related to them.

On Mon, May 10, 2021 at 7:10 PM Dave Page <dpage@pgadmin.org> wrote:

Hi

On Mon, May 10, 2021 at 1:45 PM Nikhil Mohite <nikhil.mohite@enterprisedb.com> wrote:

Hi Hackers,

Please find the attached patch for RM-2341: Add Menu option for starting PSQL.

1. Added new Option PSQL Tool in Tools menu.

2. Added the same option for Server and Database nodes from the tree view.

Unfortunately there's a trailing comma in package.json that makes it invalid. If I fix that, then I get the error below, so I'm guessing the intention was to actually include another package there?

ERROR in ./pgadmin/tools/psql/static/js/psql_module.js 23:50-82

Module not found: Error: Can't resolve 'local-echo-controller' in '/Users/dpage/git/pgadmin4/web/pgadmin/tools/psql/static/js'

resolve 'local-echo-controller' in '/Users/dpage/git/pgadmin4/web/pgadmin/tools/psql/static/js'

  Parsed request is a module

  using description file: /Users/dpage/git/pgadmin4/web/package.json (relative path: ./pgadmin/tools/psql/static/js)

    aliased with mapping 'local-echo-controller': '/Users/dpage/git/pgadmin4/web/node_modules/local-echo' to '/Users/dpage/git/pgadmin4/web/node_modules/local-echo'

      using description file: /Users/dpage/git/pgadmin4/web/package.json (relative path: ./pgadmin/tools/psql/static/js)

        Field 'browser' doesn't contain a valid alias configuration

        root path /Users/dpage/git/pgadmin4/web

          using description file: /Users/dpage/git/pgadmin4/web/package.json (relative path: ./Users/dpage/git/pgadmin4/web/node_modules/local-echo)

            no extension

              Field 'browser' doesn't contain a valid alias configuration

              /Users/dpage/git/pgadmin4/web/Users/dpage/git/pgadmin4/web/node_modules/local-echo doesn't exist

            .js

              Field 'browser' doesn't contain a valid alias configuration

              /Users/dpage/git/pgadmin4/web/Users/dpage/git/pgadmin4/web/node_modules/local-echo.js doesn't exist

            .jsx

              Field 'browser' doesn't contain a valid alias configuration

              /Users/dpage/git/pgadmin4/web/Users/dpage/git/pgadmin4/web/node_modules/local-echo.jsx doesn't exist

            as directory

              /Users/dpage/git/pgadmin4/web/Users/dpage/git/pgadmin4/web/node_modules/local-echo doesn't exist

        using description file: /Users/dpage/git/pgadmin4/web/package.json (relative path: ./node_modules/local-echo)

          no extension

            Field 'browser' doesn't contain a valid alias configuration

            /Users/dpage/git/pgadmin4/web/node_modules/local-echo doesn't exist

          .js

            Field 'browser' doesn't contain a valid alias configuration

            /Users/dpage/git/pgadmin4/web/node_modules/local-echo.js doesn't exist

          .jsx

            Field 'browser' doesn't contain a valid alias configuration

            /Users/dpage/git/pgadmin4/web/node_modules/local-echo.jsx doesn't exist

          as directory

            /Users/dpage/git/pgadmin4/web/node_modules/local-echo doesn't exist

 @ ./pgadmin/tools/psql/static/js/index.js 17:19-43

2021-05-10 14:38:37: webpack 5.21.2 compiled with 1 error in 60041 ms

 

--

Dave Page
Blog: https://pgsnake.blogspot.com
Twitter: @pgsnake

EDB: https://www.enterprisedb.com

Regards,

Nikhil Mohite 

--

Thanks & Regards

Akshay Joshi

pgAdmin Hacker | Principal Software Architect

EDB Postgres

Mobile: +91 976-788-8246

--

Dave Page
Blog: https://pgsnake.blogspot.com
Twitter: @pgsnake

EDB: https://www.enterprisedb.com