Re: [pgAdmin4][Patch]: RM 3284 - F5 key not working consistently

[Joao De Almeida Pereira]

Hi Khushboo,

I did some changes on your patch:
 0001 - Your original patch
 0002 - Convert keyboard.js to ES6
 0003 - Refactoring of the keyboard.js file(some one letter variables and
other code)



Thanks
Joao

On Thu, Apr 26, 2018 at 5:34 AM Khushboo Vashi <
khushboo.va...@enterprisedb.com> wrote:

> Hi,
>
> Please find the attached patch to fix the RM #3284 : F5 key not working
> consistently.
>
> - Added the configurable keyboard shortcut (default F5) to refresh the
> browser tree nodes.
>
>
> Thanks,
> Khushboo
>


0001-RM_3284_v1.patch
Description: Binary data


0002-Convert-keyboard.js-to-ES6.patch
Description: Binary data


0003-Refactoring-of-keyboard.js.patch
Description: Binary data

Re: [pgAdmin4][Patch] Feature #1447 SSH Tunnel

[Joao De Almeida Pereira]

Hi Akshay,

Some suggestions:
- browser/server_groups/servers/__init__py
  This file could have been split into separate functionalities. There is a
chunk of changes for connect, why not move that out? Same thing for create.
Do we really need to have full integrationy tests that do a HTTP request
and connect to a real database to do make sure the functionalities are
working? If we isolate these into their own actions we can more easily get
more coverage on the code with tests that would be much faster and
directed. The big advantage of this is that by reading the tests we can
understand what the functions do. Self documenting code.

- utils/driver/psycopg2.py same comments has above

- browser/server_groups/servers/static/js/server.js:
  - The patterm of using `m` for `model`? is a bad pattern, so why not
change it?
  - We could extract the model creation from this file. This will allow us
to add some tests around disabled methods that are a bit everywhere
  - We could also convert this file to ES6

- utils/driver/psycopg2/server_manager.py
  - Do we have Unit Tests around this?
  - Maybe this SSH part could be isolated into it's own class, as it is not
100% related to the class in question. We need to use it but is is not part
of the ServerManager domain

- JS template. Eventually I would like to see if completely removed, and
the information that we are generating using the template can be passed to
the frontend via a Ajax call as an example( Do not think this is the time
to do it.)

- start_running_query.py
 - we could enrich the tests of this functionality


And example of naming is for example on psycopg2/connection.py

mgr = self.manager

How much to we win by having this variable name versus manager =
self.manager or even using the self.manager?


This is not for you in specific, but for @hackers in general:
The book https://www.amazon.com/dp/0132350882/
 is a pretty
nice book that gives you an introduction to clean code, that is self
documenting and that is much easily maintained.

Thanks
Joao

On Thu, Apr 26, 2018 at 3:44 AM Akshay Joshi 
wrote:

> Hi Hackers,
>
> Attached is the updated patch which includes documentation of the feature
> and also updated screenshots of server dialog with new "SSH Tunnel" tab.
>
> On Wed, Apr 25, 2018 at 11:45 AM, Akshay Joshi <
> akshay.jo...@enterprisedb.com> wrote:
>
>> Hi Joao
>>
>> On Tue, Apr 24, 2018 at 10:04 PM, Joao De Almeida Pereira <
>> jdealmeidapere...@pivotal.io> wrote:
>>
>>> Hi Akshay,
>>>
>>> After looking through the patch we found some one letter variable names
>>> and this is a regression on what we have been trying to accomplish in the
>>> last year.
>>>
>>> An objective that we have for pgAdmin source code is to increase the
>>> testability of it and make it more readable. If we keep on adding one
>>> letter variables and if we continue adding code to already convoluted
>>> source files it is going to be very hard to achieve this objective.
>>>
>>
>>   At my level I have tried not to give one letter variable names.
>> Are you talking about the variable "m" in server.js file which
>> represents the Model? If yes then I have followed the code written for
>> whole schema and I thought we have to maintain the consistency, so use that
>> as it is. Apart from that I haven't seen any other one letter variable,
>> please correct me so that I'll rename it.
>>
>>>
>>> Our recommendations for this change are:
>>> - Name the variables with comprehensive names
>>>
>>   Can you please suggest from the patch.
>>
>>
>>> - Extract functions where we can and try to wrap some tests around them
>>> (ex: the javascript disabled functions)
>>>
>>
>> I have tried to do that too, if you can see the "server/__init__.py"
>> file I have created "*get_response_for_password" function to remove
>> redundant code. Based on the condition it will return the json response.*
>>
>>
>>> - We really need to find a better pattern than templated Javascript to
>>> pass information from the backend to the frontend
>>>
>> - When changing a piece of code, if we see code that is confusing or that
>>> is hard to read, we should refactor instead of adding to the pattern.
>>>
>>
>> Please elaborate more with respect to my patch, which part of code
>> should required modification?
>>
>>>
>>> Thanks
>>> Victoria & Joao
>>>
>>>
>>> On Tue, Apr 24, 2018 at 10:13 AM Akshay Joshi <
>>> akshay.jo...@enterprisedb.com> wrote:
>>>
 Hi Hackers

 As per suggestion by Dave, I have moved "Advanced" tab at the last for
 Server dialog. Attached is the modified patch.

 On Mon, Apr 23, 2018 at 7:32 PM, Anthony Emengo 
 wrote:

> For what it is worth, I manually verified that the feature worked, as
> well as looked through the code.
>
> I'd like to see end-to-end testing for regression sake, but it's hard
> to so at this 

pgAdmin 4 commit: Cleanup some old code that was failing CI.

[Dave Page]

Cleanup some old code that was failing CI.

Branch
--
master

Details
---
https://git.postgresql.org/gitweb?p=pgadmin4.git;a=commitdiff;h=201f6d359ff61bf07544db844711f9b3fc4b881c
Author: Joao Pedro De Almeida Pereira 

Modified Files
--
web/pgadmin/utils/route.py | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)

Re: [pgadmin4][patch] Old code failing CI

[Dave Page]

Thanks, applied.

On Thu, Apr 26, 2018 at 2:52 PM, Joao De Almeida Pereira <
jdealmeidapere...@pivotal.io> wrote:

> Hi Hackers,
> Attached you can find a patch of some leftover code that we didn't realize
> was still there.
>
> Thanks
> Joao
>



-- 
Dave Page
Blog: http://pgsnake.blogspot.com
Twitter: @pgsnake

EnterpriseDB UK: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

Re: [pgAdmin4][RM#3155] Allow user to lock the Layout

[Murtuza Zabuawala]

Hi Joao,

On Wed, Apr 25, 2018 at 6:55 PM, Joao De Almeida Pereira <
jdealmeidapere...@pivotal.io> wrote:

> Hi,
> @Murtuza: We didn't notice the issue, can you please advise on what need
> to change to make it work? The only change we did was to make one test pass.
>

I moved to another project so I didn't get a chance to look into the code
but a
s you are aware that ​we are no longer considering given the patch as a fix
for the issue instead someone from the team might fork the ​

​code and add the option in the library itself.​

Regards,
Murtuza


> @Hackers: In our point of view it is never good to fork a library. But if
> he really have to do it, then we should fork it in Github, make our code
> accessible to other people, and we should add it as a dependency on
> package.json
>
>
> Thanks
> Anthony & Joao
>
>
> On Wed, Apr 25, 2018 at 7:14 AM Dave Page  wrote:
>
>> On Wed, Apr 25, 2018 at 12:13 PM, Murtuza Zabuawala <
>> murtuza.zabuaw...@enterprisedb.com> wrote:
>>
>>> Hi Dave,
>>>
>>> On Wed, Apr 25, 2018 at 3:36 PM, Dave Page  wrote:
>>>
 All,

 We just had a brief discussion in our EDB sprint planning meeting about
 this. There is a non-zero chance that we're going to have to fork wcDocker
 in the near future, in order to update it to work with jQuery 3. If we do
 that, then it may be significantly easier to fix this issue in that fork
 (perhaps by adding a single lockLayout(bool) function, rather than trying
 to do so from pgAdmin.

 I think (unless Murtuza believes that won't help), that we're better
 off holding on this for now until we know if we've had to do that.

>>>
>>> ​I don't have any objection forking the code and adding the flag to lock
>>> the panel,  But I'm certain that
>>> we will use the same inbuilt method *panel.moveable(false)* which we
>>> have used right now in the patch to prevent a panel from floating and will
>>> face the same issue again which Akshay mentioned in his last email.
>>>
>>> Let me know if you want me to attach latest patch onto the ticket for
>>> future reference and update the ticket accordingly​.
>>>
>>
>> That's probably a good idea - thanks.
>>
>>
>>>
>>>
 On Wed, Apr 25, 2018 at 10:23 AM, Murtuza Zabuawala <
 murtuza.zabuaw...@enterprisedb.com> wrote:

> Hi Akshay,
>
>
> On Wed, Apr 25, 2018 at 2:37 PM, Akshay Joshi <
> akshay.jo...@enterprisedb.com> wrote:
>
>> Hi Joao/Murtuza
>>
>> It break's the functionality, I am able to move "Data output",
>> "Explain" etc.. panel of Query Tool, even if "Lock layout?" is set to 
>> True.
>>
>
> ​It's working properly in v5 patch, Something went wrong while
> refactoring.​
>
> ​
>
> Apart from above I have found more issue. Below are the steps to
>> reproduce:
>>
>>- Set the "Lock layout?" flag to False.
>>- Move out Dashboard panel.
>>- Set the "Lock layout?" flag to True.
>>- Close the Dashboard panel, as layout is locked and empty
>>Dashboard panel is still visible. (Refer attached screenshot)
>>
>> ​That's because we have set the Panel moveable property to False,
> they won't auto resize, As discussed earlier if user drag any panel out of
> panel group it gets render in seprate wcFrame. I think that needs to be
> taken care by user before they decide to lock the layout, We can not
> expilcitly set panel's closeable property to False when layout is locked,
> If we do so user will not be able to close any Query tool, Debugger 
> panels.​
>  ​
>
>
> On Tue, Apr 24, 2018 at 9:11 PM, Joao De Almeida Pereira <
>> jdealmeidapere...@pivotal.io> wrote:
>>
>>> haha,
>>> Just joking, now we have a good version that passes tests and all.
>>>
>>> We found out that a test was failing in the patch version 5:
>>>
>>> HeadlessChrome 0.0.0 (Linux 0.0.0) Panel when we create a panel and 
>>> user created panel without defining isMoveable then it should be 
>>> moveable it should call moveable method with true as argument FAILED
>>> Expected false to be true.
>>> at UserContext. 
>>> (regression/javascript/browser/panel_spec.js:12886:38)
>>>
>>> ​
>>> To solve this problem we decided to change the Panel class to match
>>> what the test say.
>>>
>>> Thanks
>>> Victoria & Joao
>>>
>>>
>>> On Tue, Apr 24, 2018 at 11:08 AM Joao De Almeida Pereira <
>>> jdealmeidapere...@pivotal.io> wrote:
>>>
 Hi,
 Apparently the last version was not applying, here is the new
 version that should apply correctly

 Thanks
 Victoria & Joao

 On Tue, Apr 24, 2018 at 10:56 AM Joao De Almeida Pereira <
 jdealmeidapere...@pivotal.io> wrote:

> Hi Murtuza,
>
> We tested the patch 

[pgadmin4][patch] Old code failing CI

[Joao De Almeida Pereira]

Hi Hackers,
Attached you can find a patch of some leftover code that we didn't realize
was still there.

Thanks
Joao


old-code.patch
Description: Binary data

Re: [pgadmin4][patch] [GreenPlum] Partitioned table icon is the same as non partitioned tables

[Akshay Joshi]

Thanks Patch applied.

On Wed, Apr 25, 2018 at 2:17 AM, Joao De Almeida Pereira <
jdealmeidapere...@pivotal.io> wrote:

> Hi Hackers,
> Attached you can find 2 patches that correct the Redmine: #3308
>  A
> Patches:
> 0001 - In this patch we update the SQL to retrieve the is_partitioned flag
> from the database
> 0002 - Extract and refactor the icon css class retrieval. It was scattered
> around the code and we moved it into it's own small class
>
> We also realize that the class was present in some places in the
> javascript world. What it is strange is that we only override the icon with
> the partition one after a successful truncation or successful reset of
> statistics.
> Why was this behavior added? Can we remove that?
>

Are you referring to "partition.js"? If yes then according to me it's
valid because for partitions we only override the icon with the partition
one. Please correct me if I am wrong.

>
>
> Thanks
> Victoria & Joao
>



-- 
*Akshay Joshi*

*Sr. Software Architect *



*Phone: +91 20-3058-9517Mobile: +91 976-788-8246*

pgAdmin 4 commit: Fixed issue where icon for Partitioned tables was the

[Akshay Joshi]

Fixed issue where icon for Partitioned tables was the same as Non Partitioned 
tables for GreenPlum database. Fixes #3308

Branch
--
master

Details
---
https://git.postgresql.org/gitweb?p=pgadmin4.git;a=commitdiff;h=966279c1b3cc9137ed6bb7ba21e76d0981178b3c
Author: Joao De Almeida Pereira 

Modified Files
--
docs/en_US/release_notes_3_1.rst   |   3 +-
.../servers/databases/schemas/tables/__init__.py   |  34 +++
.../schemas/tables/base_partition_table.py |  23 +
.../schemas/tables/partitions/__init__.py  |   2 +-
.../templates/table/sql/gpdb_5.0_plus/nodes.sql|   3 +-
.../tables/tests/test_base_partition_table.py  | 107 +
.../servers/databases/schemas/tables/utils.py  |  13 +--
7 files changed, 152 insertions(+), 33 deletions(-)

[pgAdmin4][Patch]: RM 3284 - F5 key not working consistently

[Khushboo Vashi]

Hi,

Please find the attached patch to fix the RM #3284 : F5 key not working
consistently.

- Added the configurable keyboard shortcut (default F5) to refresh the
browser tree nodes.


Thanks,
Khushboo
diff --git a/web/pgadmin/browser/register_browser_preferences.py b/web/pgadmin/browser/register_browser_preferences.py
index 178af6b..3c2da91 100644
--- a/web/pgadmin/browser/register_browser_preferences.py
+++ b/web/pgadmin/browser/register_browser_preferences.py
@@ -291,3 +291,18 @@ def register_browser_preferences(self):
 category_label=gettext('Keyboard shortcuts'),
 fields=fields
 )
+
+self.preference.register(
+'keyboard_shortcuts',
+'sub_menu_refresh',
+gettext('Refresh browser tree node'),
+'keyboardshortcut',
+{
+'alt': False,
+'shift': False,
+'control': False,
+'key': {'key_code': 116, 'char': 'F5'}
+},
+category_label=gettext('Keyboard shortcuts'),
+fields=fields
+)
diff --git a/web/pgadmin/browser/static/js/keyboard.js b/web/pgadmin/browser/static/js/keyboard.js
index 5c71b80..37fd61f 100644
--- a/web/pgadmin/browser/static/js/keyboard.js
+++ b/web/pgadmin/browser/static/js/keyboard.js
@@ -24,6 +24,7 @@ function(_, S, pgAdmin, $, Mousetrap, commonUtils, dialogTabNavigator) {
   'sub_menu_properties': commonUtils.parseShortcutValue(pgBrowser.get_preference('browser', 'sub_menu_properties').value),
   'sub_menu_create': commonUtils.parseShortcutValue(pgBrowser.get_preference('browser', 'sub_menu_create').value),
   'sub_menu_delete': commonUtils.parseShortcutValue(pgBrowser.get_preference('browser', 'sub_menu_delete').value),
+  'sub_menu_refresh': commonUtils.parseShortcutValue(pgBrowser.get_preference('browser', 'sub_menu_refresh').value),
   'context_menu': commonUtils.parseShortcutValue(pgBrowser.get_preference('browser', 'context_menu').value),
   'direct_debugging': commonUtils.parseShortcutValue(pgBrowser.get_preference('browser', 'direct_debugging').value),
 };
@@ -41,6 +42,7 @@ function(_, S, pgAdmin, $, Mousetrap, commonUtils, dialogTabNavigator) {
   'bindSubMenuProperties': {'shortcuts': this.keyboardShortcut.sub_menu_properties}, // Sub menu - Edit Properties,
   'bindSubMenuCreate': {'shortcuts': this.keyboardShortcut.sub_menu_create}, // Sub menu - Create Object,
   'bindSubMenuDelete': {'shortcuts': this.keyboardShortcut.sub_menu_delete}, // Sub menu - Delete object,
+  'bindSubMenuRefresh': {'shortcuts': this.keyboardShortcut.sub_menu_refresh, 'bindElem': '#tree'}, // Sub menu - Refresh object,
   'bindContextMenu': {'shortcuts': this.keyboardShortcut.context_menu}, // Sub menu - Open context menu,
   'bindDirectDebugging': {'shortcuts': this.keyboardShortcut.direct_debugging}, // Sub menu - Direct Debugging
 };
@@ -221,6 +223,13 @@ function(_, S, pgAdmin, $, Mousetrap, commonUtils, dialogTabNavigator) {
   // Call delete object callback
   pgAdmin.Browser.Node.callbacks.delete_obj.call(pgAdmin.Browser.Nodes[tree.t.itemData(tree.i)._type]);
 },
+bindSubMenuRefresh: function(e) {
+  e.preventDefault();
+  var tree = pgBrowser.keyboardNavigation.getTreeDetails();
+
+  // Call refresh object callback
+  pgAdmin.Browser.Node.callbacks.refresh.call(pgAdmin.Browser.Nodes[tree.t.itemData(tree.i)._type]);
+},
 bindContextMenu: function(e) {
   var tree = this.getTreeDetails(),
 left = $(e.srcElement).find('.aciTreeEntry').position().left + 70,
@@ -263,5 +272,5 @@ function(_, S, pgAdmin, $, Mousetrap, commonUtils, dialogTabNavigator) {
 },
   });
 
-  return pgAdmin.keyboardNavigation;
+  return pgAdmin.Browser.keyboardNavigation;
 });
diff --git a/web/pgadmin/static/js/utils.js b/web/pgadmin/static/js/utils.js
index 026297f..879bbd0 100644
--- a/web/pgadmin/static/js/utils.js
+++ b/web/pgadmin/static/js/utils.js
@@ -12,7 +12,7 @@ export function parseShortcutValue(obj) {
   if (obj.alt) { shortcut += 'alt+'; }
   if (obj.shift) { shortcut += 'shift+'; }
   if (obj.control) { shortcut += 'ctrl+'; }
-  shortcut += String.fromCharCode(obj.key.key_code).toLowerCase();
+  shortcut += obj.key.char.toLowerCase();
   return shortcut;
 }
 
diff --git a/web/regression/javascript/parse_shortcut_value_spec.js b/web/regression/javascript/parse_shortcut_value_spec.js
new file mode 100644
index 000..dc20c3e
--- /dev/null
+++ b/web/regression/javascript/parse_shortcut_value_spec.js
@@ -0,0 +1,117 @@
+/
+//
+// pgAdmin 4 - PostgreSQL Tools
+//
+// Copyright (C) 2013 - 2018, The pgAdmin Development Team
+// This software is released under the PostgreSQL Licence
+//
+//
+
+import { parseShortcutValue } from 'sources/utils';
+
+describe('parseShortcutValue', function () {
+