Hi
On Tue, May 11, 2021 at 9:02 AM Akshay Joshi <akshay.jo...@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.moh...@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 <dp...@pgadmin.org> wrote:
>>
>>> Hi
>>>
>>> On Mon, May 10, 2021 at 1:45 PM Nikhil Mohite <
>>> nikhil.moh...@enterprisedb.com> wrote:
>>>
>>>> Hi Hackers,
>>>>
>>>> Please find the attached patch for RM-2341
>>>> <https://redmine.postgresql.org/issues/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 <http://edbpostgres.com>*
>
> *Mobile: +91 976-788-8246*
>
--
Dave Page
Blog: https://pgsnake.blogspot.com
Twitter: @pgsnake
EDB: https://www.enterprisedb.com
