Hi
On Mon, May 17, 2021 at 11:01 AM Nikhil Mohite <
nikhil.moh...@enterprisedb.com> wrote:
> Hi Akshay/ Team,
>
> Please find the attached updated patch for the psql tool.
>
Hmm, this version is also broken. There's a typo in editor_template.html on
line 138 - it splits a string across two lines which throws an error.
Having fixed that...
I also note there's a lot of Javascript in that HTML file. That should be
pushed into the webpacked bundle I think, and not included inline in HTML.
A couple of other things I noticed:
- The button is enabled if the treeview has a Server selected. It could be
argued that the query tool should do the same (defaulting to the
maintenance database), however, that would be a separate change, and psql
should be consistent with the query tool.
- If I do a "select * from pg_class;" I still get:
postgres=# select * from pg_class;
WARNING: terminal is not fully functional
- I'm sure using \q in the previous version displayed a message saying the
session exited (the one on line 138 of editor_template.html). It no longer
seems to do so.
>
>
> On Tue, May 11, 2021 at 3:40 PM Dave Page <dp...@pgadmin.org> wrote:
>
>> 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
>>
>> Regards,
> Nikhil Mohite
>
--
Dave Page
Blog: https://pgsnake.blogspot.com
Twitter: @pgsnake
EDB: https://www.enterprisedb.com
