Re: [pgAdmin][RM-2341]: Add menu option for starting PSQL

Hi Nikhil

Following are the review comments:

- Set "ENABLE_PSQL = False", PSQL button from browser tree and context
menu option should not be visible.
- Documentation screenshot should be in standard theme for consistency,
and check the size it's very large. Take the screenshot with the new PSQL
button on the browser tree.
- Update 'menu_bar.rst' and 'toolbar.rst' with new changes.
- Remove commented code (if any)
- Check SonarQube (I haven't run)

On Thu, May 20, 2021 at 2:52 PM Dave Page <dpage(at)pgadmin(dot)org> wrote:

> Thanks Nikhil. Can someone else review this version please?
>
> On Wed, May 19, 2021 at 2:42 PM Nikhil Mohite <
> nikhil(dot)mohite(at)enterprisedb(dot)com> wrote:
>
>> Hi Dave/ Team,
>>
>> On Wed, May 19, 2021 at 1:43 PM Dave Page <dpage(at)pgadmin(dot)org> wrote:
>>
>>> Hi
>>>
>>> On Wed, May 19, 2021 at 8:58 AM Nikhil Mohite <
>>> nikhil(dot)mohite(at)enterprisedb(dot)com> wrote:
>>>
>>>> Hi Dave/ Team,
>>>>
>>>> On Tue, May 18, 2021 at 8:41 PM Dave Page <dpage(at)pgadmin(dot)org> wrote:
>>>>
>>>>> Hi
>>>>>
>>>>> On Tue, May 18, 2021 at 12:12 PM Nikhil Mohite <
>>>> nikhil(dot)mohite(at)enterprisedb(dot)com> wrote:
>>>>
>>>>> Hi Dave/Team,
>>>>>
>>>>>
>>>>> On Mon, May 17, 2021 at 6:47 PM Dave Page <dpage(at)pgadmin(dot)org> wrote:
>>>>>
>>>>>> Hi
>>>>>>
>>>>>> On Mon, May 17, 2021 at 11:01 AM Nikhil Mohite <
>>>>>> nikhil(dot)mohite(at)enterprisedb(dot)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.
>>>>>>
>>>>> I have moved most of the code in the js file, few things are still in
>>>>> HTML.
>>>>>
>>>>
>>>> Hmm, yes - in particular, colours for the different themes. Please move
>>>> them into the css for the themes. You have a mix of style, layout and code
>>>> in this file which needs to be cleaned up.
>>>> xterm V3 onwards they have provided the API to set the theme and other
>>>> settings, earlier I tried with CSS to override the theme but couldn’t able
>>>> to apply the theme properly as some style get applied as in-line style for
>>>> the HTML, so used the API to set the theme.
>>>>
>>>
>>> OK, but either way we can't hard-code styles from themes in HTML
>>> templates for individual features; that way leads to madness.
>>>
>>> Perhaps Aditya or one of the other team members can give some assistance?
>>>
>> I have moved the color settings to the respective theme files. Aditya
>> helped in this.
>>
>>>
>>>
>>>>
>>>>
>>>>> Speaking of themes, the background colour for selected text doesn't
>>>>> seem right (it's barely visible) in the dark theme. Can you fix that to
>>>>> match the colouring in the SQL text boxes please?
>>>>>
>>>> I tried the default selection color from SQL for the dark and standard
>> themes but still, it was not readable so just updated the color code with
>> another color as follows.
>> 1. Dark Theme:
>> [image: Screenshot 2021-05-19 at 6.29.43 PM.png]
>> 2. High Contrast: (using default SQL selection color)
>> [image: Screenshot 2021-05-19 at 6.59.52 PM.png]
>> 3. Standard:
>> [image: image.png]
>> can we go with the colors or should we update it?
>>
>>>
>>>>>
>>>>>>
>>>>>>> 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.
>>>>>>>
>>>>>> It is now 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 am not able to reproduce the warning for the terminal (I am working
>>>>>> on Catalina 10.15.7), I checked on browsers (chrome, firefox, Safari) and
>>>>>> also checked on local nwjs runtime but still not able to reproduce the
>>>>>> warning. but found one limitation:
>>>>>>
>>>>>
>>>>> It looks like that can be fixed by adding:
>>>>>
>>>>> env={'TERM': 'xterm'}
>>>>>
>>>> Added this in the environment when opening the psql panel.
>>
>>>
>>>>> to the subprocess.Popen() call.
>>>>>
>>>>> I noticed while I was playing with that, that you are passing the
>>>>> password as part of the connection string. As I've mentioned in the past,
>>>>> that is absolutely not acceptable; it will expose the password to all
>>>>> manner of tools (such as ps -ef). You *must* pass the password to psql
>>>>> using the PGPASSWORD environment variable.
>>>>>
>>>>>
>>>>>> if we try to load data from the table containing millions of records,
>>>>>> UI gets very slow.
>>>>>>
>>>>> Removed the password from the connection string and added 'PGPASSWORD'
>> in the environment.
>>
>>>
>>>>> Is xtermjs discarding the older buffer contents when it fills up? Can
>>>>> you tell where the memory usage is?
>>>>>
>>>> I checked the psql memory consumption in terminal and pgAdmin psql tool
>>>> memory consumption is the similar. Also tested the performance and query
>>>> execution timing is also similar.
>>>>
>>>
>>> OK, so there's probably not much we can do here.
>>>
>>>
>>>>
>>>>>
>>>>>
>>>>>>
>>>>>>> - 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.
>>>>>>>
>>>>>>
>>>>> In addition to the issue above, it looks like the \! blocking may have
>>>>> lost it's ability to ignore quoted strings:
>>>>>
>>>>> pgweb=# select '\!';
>>>>> ERROR: Shell commands are disabled in psql for security
>>>>>
>>>> Fixed the issue now it is consistent with the psql terminal.
>>
>>>
>>>>>
>>>>
>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>> On Tue, May 11, 2021 at 3:40 PM Dave Page <dpage(at)pgadmin(dot)org>
>>>>>>>> wrote:
>>>>>>>>
>>>>>>>>> Hi
>>>>>>>>>
>>>>>>>>> On Tue, May 11, 2021 at 9:02 AM Akshay Joshi <
>>>>>>>>> akshay(dot)joshi(at)enterprisedb(dot)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(dot)mohite(at)enterprisedb(dot)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(at)pgadmin(dot)org>
>>>>>>>>>>> wrote:
>>>>>>>>>>>
>>>>>>>>>>>> Hi
>>>>>>>>>>>>
>>>>>>>>>>>> On Mon, May 10, 2021 at 1:45 PM Nikhil Mohite <
>>>>>>>>>>>> nikhil(dot)mohite(at)enterprisedb(dot)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
>>>>>>>
>>>>>>
>>>>>> Regards,
>>>>>> Nikhil Mohite
>>>>>>
>>>>>
>>>>>
>>>>> --
>>>>> Dave Page
>>>>> Blog: https://pgsnake.blogspot.com
>>>>> Twitter: @pgsnake
>>>>>
>>>>> EDB: https://www.enterprisedb.com
>>>>>
>>>>
>>>>
>>>> Regards,
>>>> Nikhil Mohite
>>>>
>>>>> <https://www.enterprisedb.com>
>>>>>
>>>>> --
>>>> *Thanks & Regards,*
>>>> *Nikhil Mohite*
>>>> *Software Engineer.*
>>>> *EDB Postgres* <https://www.enterprisedb.com/>
>>>> *Mob.No: +91-7798364578.*
>>>>
>>>
>>>
>>> --
>>> 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
>
>

--
*Thanks & Regards*
*Akshay Joshi*
*pgAdmin Hacker | Principal Software Architect*
*EDB Postgres <http://edbpostgres.com>*

*Mobile: +91 976-788-8246*