Re: RM#3294 - User need to reset the layout to see the changed preferences parameters

[Victoria Henry]

Hi Aditya,


It is not possible to fire event in another tab/new browser window. For
>>> example, query tool can be open in another tab. And thus, changes are not
>>> reflected there. There are solutions available like updating the
>>> localStorage of the browser but those are not reliable and does not work
>>> properly on different browsers.
>>>
>> To communicate between browser tabs we can use cookie polling on client
>> side it self (at least it will avoid polling over http).
>> The main tab will update only preference specific cookie when preference
>> is updated and other tabs will poll required cookies (not all)
>> with specific interval (1 second can be configurable).
>>
> Polling is a solution but I think it should be the last option. http polls
> will not be required anyway as we have preference cache in the browser
> object.
>

Maybe it's better to poll only when an editor is open in a separate
window?  What would we be polling for and how would be tell the backend
that something changed during the poll?

Thanks
Victoria & Joao

Re: [pgAdmin4][Patch]: RM 3362 - Fix the functions for PG v11, and add support procedure for PG v11

[Victoria Henry]

Hi Khushboo,

The following change is allowing the creation of procedures in postgresql
versions less then 11 and also GreenPlum

--- 
a/web/pgadmin/browser/server_groups/servers/databases/schemas/functions/static/js/procedure.js
+++ 
b/web/pgadmin/browser/server_groups/servers/databases/schemas/functions/static/js/procedure.js
@@ -71,11 +71,7 @@ define('pgadmin.node.procedure', [
 if ('catalog' in node_hierarchy)
   return false;

-// Procedures supported only in PPAS
-return (
-  'server' in node_hierarchy &&
-node_hierarchy['server'].server_type == 'ppas'
-);
+return true;

Now that the Procedures are a thing in Postgresql maybe they should live in
their own module.
In the tests for trigger functions we are not consistent on the naming of
the utils , in some places we call it funcs_utils in others
trigger_funcs_utils.

Thanks
​
Victoria & Joao

On Tue, Jun 12, 2018 at 3:10 AM Khushboo Vashi <
khushboo.va...@enterprisedb.com> wrote:

> Hi,
>
> Please find the attached updated patch.
>
> On Fri, Jun 8, 2018 at 2:21 PM, Dave Page  wrote:
>
>> Hi
>>
>> On Thu, Jun 7, 2018 at 11:27 AM, Khushboo Vashi <
>> khushboo.va...@enterprisedb.com> wrote:
>>
>>> Hi,
>>>
>>> Please ignore my previous patch, find the attached updated one.
>>>
>>
>> I found a couple of issues with this:
>>
>> - Clicking the + button on the Parameters tab does nothing in either
>> Create or Edit modes
>>
>> Fixed
>
>> - The debugger fails to start (though, perhaps that's because the plugin
>> doesn't have Ashesh's latest patches in it).
>>
>> FYI, I was trying to test the debugger with:
>>
>> -- PROCEDURE: public.dummy_proc(integer)
>>
>> -- DROP PROCEDURE public.dummy_proc(integer);
>>
>> CREATE OR REPLACE  PROCEDURE public.dummy_proc(
>> id integer)
>> LANGUAGE 'plpgsql'
>>
>> AS $BODY$BEGIN
>>   raise notice 'id is %', id;
>> END;$BODY$;
>>
>> Fixed. Tested with the latest code of the plugin.
>
>> Thanks!
>>
>> Thanks,
> Khushboo
>
>> --
>> Dave Page
>> Blog: http://pgsnake.blogspot.com
>> Twitter: @pgsnake
>>
>> EnterpriseDB UK: http://www.enterprisedb.com
>> The Enterprise PostgreSQL Company
>>
>
>

Re: [pgadmin4][Patch]: Test cases for the backup module

[Victoria Henry]

Hi Khushboo
can you explain a little bit more this while loop?


cnt = 0
while 1:
if cnt > 1:
break
# Check the process list
response1 = self.tester.get('/misc/bgprocess/?_='.format(
random.randint(1, 999)))
self.assertEquals(response1.status_code, 200)
process_list = json.loads(response1.data.decode('utf-8'))

if len(process_list) > 0 and 'execution_time' in process_list[0]:
break
time.sleep(0.5)
cnt += 1

>From what it looks like this will only run twice, maybe a for would be a
better solution because we know it will only run twice. Also are we sure we
only want it to run twice?

We are using PyCharm to do our developments and we notice there are a big
group of unused variables throughout. We should remove them if they are not
needed. Not sure if your editor also shows that information or not.
Do you know if there is a configuration in pycodestyle to enable the check
for unused variables? That would help a lot.

The code

assert 'execution_time' in process_list[0]
assert 'stime' in process_list[0]
assert 'exit_code' in process_list[0]
assert process_list[0]['exit_code'] in self.expected_exit_code

in test_Create_restore_job should use self.assertEqual or similar from
unittest instead of plain assert. Because when something fails we do not
have a way to understand what was wrong.
The tests on the restore are still failing GreenPlum.
​



Thanks
Victoria & Joao

On Tue, Jun 12, 2018 at 6:44 AM Khushboo Vashi <
khushboo.va...@enterprisedb.com> wrote:

> Please find the attached updated patch with some code cleanup.
>
> On Tue, Jun 12, 2018 at 3:54 PM, Khushboo Vashi <
> khushboo.va...@enterprisedb.com> wrote:
>
>> Hi,
>>
>> Please find the attached patch excluding feature test cases.
>> Python test cases are working fine, so we can commit this patch. I am
>> working on fixing the feature tests which are failing on the different
>> window sizes.
>>
>> Thanks,
>> Khushboo
>>
>> On Fri, Jun 8, 2018 at 2:38 PM, Dave Page  wrote:
>>
>>> Hi
>>>
>>> On Fri, Jun 8, 2018 at 6:33 AM, Khushboo Vashi <
>>> khushboo.va...@enterprisedb.com> wrote:
>>>
 Hi Dave,

 As per our discussion I have changed the window size to 1280X800,
 before it was 1280X900.
 Please find the attached updated patch

>>>
>>> I'm not sure that actually made any difference on my system. The window
>>> continued to look taller than it is wide, so I wonder if the code to set
>>> the size is being ignored, or is at the wrong place?
>>>
>>> Anyway, I got 10 failures with this patch :-(
>>>
>>> ==
>>>
>>> ERROR: runTest
>>> (pgadmin.feature_tests.pg_utilities_backup_restore_test.PGUtilitiesBackupFeatureTest)
>>>
>>> Test for PG utilities - Backup and Restore
>>>
>>> --
>>>
>>> Traceback (most recent call last):
>>>
>>>   File
>>> "/Users/dpage/git/pgadmin4/web/pgadmin/feature_tests/pg_utilities_backup_restore_test.py",
>>> line 97, in runTest
>>>
>>> self.page.find_by_xpath("//div[contains(@class,'wcFloatingFocus')"
>>>
>>>   File
>>> "/Users/dpage/git/pgadmin4/web/regression/feature_utils/pgadmin_page.py",
>>> line 171, in find_by_xpath
>>>
>>> lambda driver: driver.find_element_by_xpath(xpath)
>>>
>>>   File
>>> "/Users/dpage/git/pgadmin4/web/regression/feature_utils/pgadmin_page.py",
>>> line 263, in wait_for_element
>>>
>>> return self._wait_for("element to exist", element_if_it_exists)
>>>
>>>   File
>>> "/Users/dpage/git/pgadmin4/web/regression/feature_utils/pgadmin_page.py",
>>> line 337, in _wait_for
>>>
>>> "Timed out waiting for " + waiting_for_message
>>>
>>>   File
>>> "/Users/dpage/.virtualenvs/pgadmin4/lib/python2.7/site-packages/selenium/webdriver/support/wait.py",
>>> line 80, in until
>>>
>>> raise TimeoutException(message, screen, stacktrace)
>>>
>>> TimeoutException: Message: Timed out waiting for element to exist
>>>
>>>
>>>
>>> ==
>>>
>>> ERROR: runTest
>>> (pgadmin.feature_tests.xss_checks_pgadmin_debugger_test.CheckDebuggerForXssFeatureTest)
>>>
>>> Tests to check if Debugger is vulnerable to XSS
>>>
>>> --
>>>
>>> Traceback (most recent call last):
>>>
>>>   File
>>> "/Users/dpage/git/pgadmin4/web/pgadmin/feature_tests/xss_checks_pgadmin_debugger_test.py",
>>> line 42, in runTest
>>>
>>> self._function_node_expandable()
>>>
>>>   File
>>> "/Users/dpage/git/pgadmin4/web/pgadmin/feature_tests/xss_checks_pgadmin_debugger_test.py",
>>> line 57, in _function_node_expandable
>>>
>>> self.page.select_tree_item("a_test_function()")
>>>
>>>   File
>>> "/Users/dpage/git/pgadmin4/web/regression/feature_utils/pgadmin_page.py",
>>> line 135, in select_tree_item
>>>
>>> "' and @class='aciTreeItem']").click()
>>>
>>>   File
>>> 

[pgAdmin][patch] RM3409 - Retrieving SQL from table throws exception

[Victoria Henry]

Hi hackers,

Attached is a patch that fixes an exception when trying to view the SQL tab
on a GreenPlum database.

Sincerely,

Joao && Victoria


RM3409.patch
Description: Binary data

Re: [pgadmin][patch] Electron version 4.X

[Victoria Henry]

On Tue, Jun 5, 2018 at 12:28 PM Dave Page  wrote:

> Hi
>
> On Mon, Jun 4, 2018 at 10:27 PM, Joao De Almeida Pereira <
> jdealmeidapere...@pivotal.io> wrote:
>
>> Hi Hackers,
>>
>> Attached you can find the patch that introduces electron to our code base.
>>
> Cool. FYI, I'd like to get this into rough shape and then push it to a dev
> branch for fine-tuning. I think it'll be easier to work that way.
>
Sure, you can create a dev branch and push this patch to it.

Great. So here's my initial feedback:
>
> - The Mac build you provided doesn't work for me. It hangs on the loading
> screen.
>
Could we get some more information about the machine?  Using the python
from the venv directory, are you able to run the app directly?

cd /Applications/pgAdmin.app/Contents/Resources/app/
venv/bin/python web/pgAdmin4.py

​
Do you get and error?
We are setting PGADMIN_PORT, PGADMIN_KEY and SERVER_MODE environment
variables prior to starting it.


>
>
- A number of the changes are related to the naming of requirejs. I'd be
> inclined to pull that out into a separate patch and get it committed to
> master ASAP.
>
This change only makes sense in the Electron context. Neverthless fell free
to add it to master if you think it is relevant.


> - I think the build instructions need to be more generic (particularly on
> macOS). For example, I do not use HomeBrew (largely due to some nasty
> security issues they had in the past). I was able to mostly port the
> instructions and build script over to work using MacPorts (without PyEnv)
> which actually turned out to be somewhat more simple than what's there now.
>
Since we don't use MacPorts, we cannot provide installation instructions.


> - I'm not sure what this is intended to do: "git checkout electron".
> Clearly that isn't correct.
>
That was the name of our development branch.  It can be removed.


>
> - All new builds should be using Python 3.6. We need to deprecate 2.7 as
> there are some Unicode related issues that cannot be fixed in it.
>
For Windows, we are using 2.7 because of external library compilation
issues.   Let us know if you are able to get around this or how to make
this work.


> - I would like to see the new build code adapted to follow the existing
> conventions as much as reasonable, to avoid having to change build systems
> or other processes/procedures that folks use. For example, build scripts
> should be under pkg/, completed packages left in dist/, build staging done
> in xxx-build directories rather than elsewhere.
>
That sounds reasonable.


> - It may be a result of my use of MacPorts, but I'm getting the following
> failure building:
>

> yarn run v1.3.2
> $ electron-forge make --platfrom=darwin --arch=x64 --targets=dmg
> ✔ Checking your system
> ✔ Resolving Forge Config
> We need to package your application before we can make it
> ✔ Preparing to Package Application for arch: x64
> ⠼ Compiling ApplicationFailed to compile file:
> /var/folders/c6/pwf0k2d509s2xx6vh0h633vmgn/T/electron-packager/darwin-x64/pgAdmin-darwin-x64/Electron.app/Contents/Resources/app/venv/lib/python3.6/site-packages/setuptools/command/launcher
> manifest.xml
> Compiling
> /var/folders/c6/pwf0k2d509s2xx6vh0h633vmgn/T/electron-packager/darwin-x64/pgAdmin-darwin-x64/Electron.app/Contents/Resources/app/venv/lib/python3.6/site-packages/setuptools/command/launcher
> manifest.xml resulted in a MIME type of application/xml, which we don't
> know how to handle
> ⠋ Compiling ApplicationFailed to compile file:
> /var/folders/c6/pwf0k2d509s2xx6vh0h633vmgn/T/electron-packager/darwin-x64/pgAdmin-darwin-x64/Electron.app/Contents/Resources/app/web/pgadmin/browser/server_groups/servers/templates/servers/supported_servers.js
> /var/folders/c6/pwf0k2d509s2xx6vh0h633vmgn/T/electron-packager/darwin-x64/pgAdmin-darwin-x64/Electron.app/Contents/Resources/app/web/pgadmin/browser/server_groups/servers/templates/servers/supported_servers.js:
> Unexpected token (6:7)
> ⠦ Compiling ApplicationFailed to compile file:
> /var/folders/c6/pwf0k2d509s2xx6vh0h633vmgn/T/electron-packager/darwin-x64/pgAdmin-darwin-x64/Electron.app/Contents/Resources/app/web/pgadmin/browser/templates/browser/js/endpoints.js
> /var/folders/c6/pwf0k2d509s2xx6vh0h633vmgn/T/electron-packager/darwin-x64/pgAdmin-darwin-x64/Electron.app/Contents/Resources/app/web/pgadmin/browser/templates/browser/js/endpoints.js:
> Unexpected token (5:6)
> Failed to compile file:
> /var/folders/c6/pwf0k2d509s2xx6vh0h633vmgn/T/electron-packager/darwin-x64/pgAdmin-darwin-x64/Electron.app/Contents/Resources/app/web/pgadmin/browser/templates/browser/js/messages.js
> /var/folders/c6/pwf0k2d509s2xx6vh0h633vmgn/T/electron-packager/darwin-x64/pgAdmin-darwin-x64/Electron.app/Contents/Resources/app/web/pgadmin/browser/templates/browser/js/messages.js:
> Unexpected token (37:1)
> Failed to compile file:
> 

Re: [pgadmin4][patch] Initial patch to decouple from ACI Tree

[Victoria Henry]

Hi Aditya

Sure. I did not find moving
> web/pgadmin/tools/datagrid/static/js/datagrid.js. Please correct me if I am
> missing anything.

Generally speaking, I agree with moving/deleting files if it makes sense.
But in regards to web/pgadmin/tools/datagrid/static/js/datagrid.js, it
looks like this is still being used in
web/pgadmin/tools/sqleditor/static/js/sqleditor.js with
Datagrid.create_transaction

Sincerely,

Victoria
​



On Thu, Jun 7, 2018 at 12:35 AM Aditya Toshniwal <
aditya.toshni...@enterprisedb.com> wrote:

> Hi Victoria,
>
> On Wed, Jun 6, 2018 at 8:55 PM, Victoria Henry  wrote:
>
>> Hi Aditya,
>>
>> 1) Why don't we start using webpack alias's instead of using absolute
>>> path. For eg,
>>> import {RestoreDialogWrapper} from
>>> '../../../pgadmin/static/js/restore/restore_dialog_wrapper';
>>> can be used as import {RestoreDialogWrapper} from
>>> 'pgadmin_static/js/restore/restore_dialog_wrapper';
>>> by adding pgadmin_static alias to webpack config.
>>
>>
>> Great point. In some areas of the code, we began making this change.
>> There is already an alias in webpack shims for `
>> ../../../pgadmin/static/js` called `sources`.  You can find an example
>> of this in import statements for `supported_database_node.js`
>>
>> 2) Few of the js are left behind, the ones which are used in python
>>> __init__.py. Can't we move them too ? It would be nicer to not to leave
>>> behind a single js.
>>
>> I'm not sure what you mean.  Could you point to an example of a single js
>> file?
>>
>
> Sure. I did not find moving
> web/pgadmin/tools/datagrid/static/js/datagrid.js. Please correct me if I am
> missing anything.
>
>>
>> Sincerely,
>>
>> Victoria
>>
>> On Wed, Jun 6, 2018 at 7:07 AM Aditya Toshniwal <
>> aditya.toshni...@enterprisedb.com> wrote:
>>
>>> Hi Anthony/Victoria/Joao,
>>>
>>> I know I am very late to review on patch 004. The idea of moving js
>>> files from tools to static folder looks good, but I have a few suggestions:
>>>
>>> 1) Why don't we start using webpack alias's instead of using absolute
>>> path. For eg,
>>> import {RestoreDialogWrapper} from
>>> '../../../pgadmin/static/js/restore/restore_dialog_wrapper';
>>> can be used as import {RestoreDialogWrapper} from
>>> 'pgadmin_static/js/restore/restore_dialog_wrapper';
>>> by adding pgadmin_static alias to webpack config.
>>>
>>> 2) Few of the js are left behind, the ones which are used in python
>>> __init__.py. Can't we move them too ? It would be nicer to not to leave
>>> behind a single js.
>>>
>>> Kindly let me know your views on this.
>>>
>>>
>>> Thanks and Regards,
>>> Aditya Toshniwal
>>> Software Engineer | EnterpriseDB Software Solutions | Pune
>>> "Don't Complain about Heat, Plant a tree"
>>>
>>> On Sat, Jun 2, 2018 at 12:47 AM, Victoria Henry 
>>> wrote:
>>>
>>>> Hey Ashesh,
>>>>
>>>> LGTM!  The some of the CI tests failed but it looks like a flake.  But
>>>> you can go ahead and merge this.
>>>>
>>>> Sincerely,
>>>>
>>>> Victoria
>>>>
>>>> On Fri, Jun 1, 2018 at 2:36 PM Ashesh Vashi <
>>>> ashesh.va...@enterprisedb.com> wrote:
>>>>
>>>>> On Fri, Jun 1, 2018 at 10:09 PM, Victoria Henry 
>>>>> wrote:
>>>>>
>>>>>> Hi Ashesh,
>>>>>>
>>>>>> We just attempted to apply your patch over master but it did not
>>>>>> work.  We don't want to introduce any bugs or break any functionality.
>>>>>> Please update the patch to make sure it is synced up with the master 
>>>>>> branch.
>>>>>>
>>>>> Please find the updated patch.
>>>>>
>>>>>>
>>>>>> Sincerely,
>>>>>>
>>>>>> Victoria
>>>>>>
>>>>>> On Fri, Jun 1, 2018 at 11:18 AM Anthony Emengo 
>>>>>> wrote:
>>>>>>
>>>>>>> Hey Ashesh,
>>>>>>>
>>>>>>> Thanks for the explanation. It was great and it really helped!
>>>>>>>
>>>>>>> C 
>>>>>>> pgadmin/browser/server_groups/servers/databases/schemas/static/js/child.js
>>>>>>> C 
>>>>>>> pgad

Re: [pgadmin4][patch] Initial patch to decouple from ACI Tree

[Victoria Henry]

Hi Aditya,

1) Why don't we start using webpack alias's instead of using absolute path.
> For eg,
> import {RestoreDialogWrapper} from
> '../../../pgadmin/static/js/restore/restore_dialog_wrapper';
> can be used as import {RestoreDialogWrapper} from
> 'pgadmin_static/js/restore/restore_dialog_wrapper';
> by adding pgadmin_static alias to webpack config.


Great point. In some areas of the code, we began making this change.  There
is already an alias in webpack shims for `../../../pgadmin/static/js`
called `sources`.  You can find an example of this in import statements for
`supported_database_node.js`

2) Few of the js are left behind, the ones which are used in python
> __init__.py. Can't we move them too ? It would be nicer to not to leave
> behind a single js.

I'm not sure what you mean.  Could you point to an example of a single js
file?

Sincerely,

Victoria

On Wed, Jun 6, 2018 at 7:07 AM Aditya Toshniwal <
aditya.toshni...@enterprisedb.com> wrote:

> Hi Anthony/Victoria/Joao,
>
> I know I am very late to review on patch 004. The idea of moving js files
> from tools to static folder looks good, but I have a few suggestions:
>
> 1) Why don't we start using webpack alias's instead of using absolute
> path. For eg,
> import {RestoreDialogWrapper} from
> '../../../pgadmin/static/js/restore/restore_dialog_wrapper';
> can be used as import {RestoreDialogWrapper} from
> 'pgadmin_static/js/restore/restore_dialog_wrapper';
> by adding pgadmin_static alias to webpack config.
>
> 2) Few of the js are left behind, the ones which are used in python
> __init__.py. Can't we move them too ? It would be nicer to not to leave
> behind a single js.
>
> Kindly let me know your views on this.
>
>
> Thanks and Regards,
> Aditya Toshniwal
> Software Engineer | EnterpriseDB Software Solutions | Pune
> "Don't Complain about Heat, Plant a tree"
>
> On Sat, Jun 2, 2018 at 12:47 AM, Victoria Henry  wrote:
>
>> Hey Ashesh,
>>
>> LGTM!  The some of the CI tests failed but it looks like a flake.  But
>> you can go ahead and merge this.
>>
>> Sincerely,
>>
>> Victoria
>>
>> On Fri, Jun 1, 2018 at 2:36 PM Ashesh Vashi <
>> ashesh.va...@enterprisedb.com> wrote:
>>
>>> On Fri, Jun 1, 2018 at 10:09 PM, Victoria Henry 
>>> wrote:
>>>
>>>> Hi Ashesh,
>>>>
>>>> We just attempted to apply your patch over master but it did not work.
>>>> We don't want to introduce any bugs or break any functionality.  Please
>>>> update the patch to make sure it is synced up with the master branch.
>>>>
>>> Please find the updated patch.
>>>
>>>>
>>>> Sincerely,
>>>>
>>>> Victoria
>>>>
>>>> On Fri, Jun 1, 2018 at 11:18 AM Anthony Emengo 
>>>> wrote:
>>>>
>>>>> Hey Ashesh,
>>>>>
>>>>> Thanks for the explanation. It was great and it really helped!
>>>>>
>>>>> C 
>>>>> pgadmin/browser/server_groups/servers/databases/schemas/static/js/child.js
>>>>> C 
>>>>> pgadmin/browser/server_groups/servers/databases/schemas/static/js/schema_child_tree_node.js
>>>>>
>>>>> It makes sense to remove duplication by extracting these attributes
>>>>> out and setting the canDrop and canCreate functions here. But is it
>>>>> possible to combine these two files into one since they are related so we
>>>>> don’t need to import schema_child_tree_node?
>>>>>
>>>> That was the original plan, but 'pgadmin/browser/static/js//node.js'
>>> script has too many dependecies, which are not easily portable.
>>> And - that may lead to change the scope of the patch.
>>>
>>> Hence - I decided to use the separate file to make sure we have enough
>>> test coverage (which is more imprortant than changing the scope).
>>>
>>>> M pgadmin/static/js/tree/tree.js
>>>>>
>>>>> The creation of the ancestorNode function feels like a
>>>>> pre-optimization. That function is not used any where outside of the
>>>>> tree.js file, so it’s more confusing to have it defined.
>>>>>
>>>> It is being used in the latest changes. :-)
>>>
>>>
>>>> On a lighter note, could we avoid the !! syntax when possible? For
>>>>> example, instead of return !!obj, we could do something like return
>>>>> obj === undefined or return _.isUndefined(obj) as this is more
>>>>> intuitive.
>&g

Re: [pgadmin4][Patch]: Test cases for the backup module

[Victoria Henry]

Hi Khushboo

The tests are still failing and seems flaky:
https://gpdb-dev.bosh.pivotalci.info/teams/pgadmin/pipelines/pgadmin-patch/jobs/run-tests/builds/113

Sincerely,

Victoria

On Tue, Jun 5, 2018 at 4:50 AM Khushboo Vashi <
khushboo.va...@enterprisedb.com> wrote:

>
>
> On Tue, Jun 5, 2018 at 2:09 PM, Dave Page  wrote:
>
>>
>>
>> On Tue, Jun 5, 2018 at 9:37 AM, Khushboo Vashi <
>> khushboo.va...@enterprisedb.com> wrote:
>>
>>>
>>>
>>> On Tue, Jun 5, 2018 at 1:36 PM, Dave Page  wrote:
>>>
>>>> Hi
>>>>
>>>> On Tue, Jun 5, 2018 at 4:39 AM, Khushboo Vashi <
>>>> khushboo.va...@enterprisedb.com> wrote:
>>>>
>>>>>
>>>>>
>>>>> On Mon, Jun 4, 2018 at 8:41 PM, Joao De Almeida Pereira <
>>>>> jdealmeidapere...@pivotal.io> wrote:
>>>>>
>>>>>> Hi Khushboo,
>>>>>>
>>>>>> Some tests are failing in greenplum:
>>>>>>
>>>>>> https://gpdb-dev.bosh.pivotalci.info/teams/pgadmin/pipelines/pgadmin-patch/jobs/run-tests/builds/108
>>>>>> The piece of code responsible for the error is:
>>>>>>
>>>>>> if server['default_binary_paths'] is not None:
>>>>>> test_utils.set_preference(server['default_binary_paths'])
>>>>>>
>>>>>> config.DEFAULT_BINARY_PATHS = {
>>>>>> "pg": str(server['default_binary_paths']['pg']),
>>>>>> "ppas": str(server['default_binary_paths']['ppas']),
>>>>>> "gpdb": ""
>>>>>> }
>>>>>>
>>>>>>
>>>>>> Can you send me the test_config.json file?  The above code sets the
>>>>> paths to the SQLite database and through the logs couldn't figure out the
>>>>> exact failure.
>>>>>
>>>>
>>>> It seems clear from the code shown that it's not setting the binary
>>>> paths for gpdb database servers. Shouldn't it be something like:
>>>>
>>>> config.DEFAULT_BINARY_PATHS = {
>>>> "pg": str(server['default_binary_paths']['pg']),
>>>> "ppas": str(server['default_binary_paths']['ppas']),
>>>> "gpdb": str(server['default_binary_paths']['gpdb'])
>>>> }
>>>>
>>>> Without this code, the test cases should work as I already set  paths
>>> through below code.
>>>
>>> test_utils.set_preference(server['default_binary_paths'])
>>>
>>>
>> In that case, why is the code above required at all?
>>
>> My bad. Removed this code and also updated set_preference function for
> greenplum database.
> Please find the attached updated patch.
>
>>
>>
>>>
>>>>
>>>>> test_backup_utils.py file name is misleading, these are not tests,
>>>>>> are helpers.
>>>>>> ​
>>>>>>
>>>>>>
>>>>>> Thanks
>>>>>> Victoria & Joao
>>>>>>
>>>>>> On Mon, Jun 4, 2018 at 1:36 AM Khushboo Vashi <
>>>>>> khushboo.va...@enterprisedb.com> wrote:
>>>>>>
>>>>>>>
>>>>>>>
>>>>>>> On Sat, Jun 2, 2018 at 3:01 AM, Dave Page  wrote:
>>>>>>>
>>>>>>>> Hi
>>>>>>>>
>>>>>>>> This looks good, except that it's leaving the test_restore_database
>>>>>>>> behind. Can we clean that up please?
>>>>>>>>
>>>>>>>> PFA updated patch.
>>>>>>>
>>>>>>>> Thanks.
>>>>>>>>
>>>>>>>> On Fri, Jun 1, 2018 at 7:06 AM, Khushboo Vashi <
>>>>>>>> khushboo.va...@enterprisedb.com> wrote:
>>>>>>>>
>>>>>>>>> Hi Victoria,
>>>>>>>>>
>>>>>>>>> Thanks for reviewing the patch.
>>>>>>>>> The tests were failing due to the latest commit
>>>>>>>>> #2b4605a9d390cb44e5dfe9967c3adf2b28d04f1f  - Ensure
>>>>>>>>> backup/restore/maintenance work via SSH tunnels. Fixes #3355
>>>>>>>>>
>>>>>&

Re: [pgAdmin4][patch] Moved 'Notifications' tab before 'Query History' in Query Tool

[Victoria Henry]

Hi Akshay,

LGTM and passes tests.

Sincerely,

Victoria

On Tue, Jun 5, 2018 at 8:28 AM Akshay Joshi 
wrote:

> Hi Hackers,
>
> Attached is the minor patch to move 'Notifications' tab before "Query
> History" in Query Tool.
>
> --
> *Akshay Joshi*
>
> *Sr. Software Architect *
>
>
>
> *Phone: +91 20-3058-9517Mobile: +91 976-788-8246*
>

Re: [pgadmin4][patch] Initial patch to decouple from ACI Tree

[Victoria Henry]

Hey Ashesh,

LGTM!  The some of the CI tests failed but it looks like a flake.  But you
can go ahead and merge this.

Sincerely,

Victoria

On Fri, Jun 1, 2018 at 2:36 PM Ashesh Vashi 
wrote:

> On Fri, Jun 1, 2018 at 10:09 PM, Victoria Henry  wrote:
>
>> Hi Ashesh,
>>
>> We just attempted to apply your patch over master but it did not work.
>> We don't want to introduce any bugs or break any functionality.  Please
>> update the patch to make sure it is synced up with the master branch.
>>
> Please find the updated patch.
>
>>
>> Sincerely,
>>
>> Victoria
>>
>> On Fri, Jun 1, 2018 at 11:18 AM Anthony Emengo 
>> wrote:
>>
>>> Hey Ashesh,
>>>
>>> Thanks for the explanation. It was great and it really helped!
>>>
>>> C pgadmin/browser/server_groups/servers/databases/schemas/static/js/child.js
>>> C 
>>> pgadmin/browser/server_groups/servers/databases/schemas/static/js/schema_child_tree_node.js
>>>
>>> It makes sense to remove duplication by extracting these attributes out
>>> and setting the canDrop and canCreate functions here. But is it
>>> possible to combine these two files into one since they are related so we
>>> don’t need to import schema_child_tree_node?
>>>
>> That was the original plan, but 'pgadmin/browser/static/js//node.js'
> script has too many dependecies, which are not easily portable.
> And - that may lead to change the scope of the patch.
>
> Hence - I decided to use the separate file to make sure we have enough
> test coverage (which is more imprortant than changing the scope).
>
>> M pgadmin/static/js/tree/tree.js
>>>
>>> The creation of the ancestorNode function feels like a
>>> pre-optimization. That function is not used any where outside of the
>>> tree.js file, so it’s more confusing to have it defined.
>>>
>> It is being used in the latest changes. :-)
>
>
>> On a lighter note, could we avoid the !! syntax when possible? For
>>> example, instead of return !!obj, we could do something like return obj
>>> === undefined or return _.isUndefined(obj) as this is more intuitive.
>>>
>>> https://softwareengineering.stackexchange.com/a/80092
>>>
>> I am kind of disagree here. But - I have changed it anyway.
>
>> In addition, please update this patch as it is out of sync with the
>>> latest commit on the master branch. Otherwise, everything looks good!
>>>
>> Here - you go!
>
> -- Thanks, Ashesh
>
>> ​
>>>
>>> Thanks
>>> Anthony && Victoria
>>>
>>> On Fri, Jun 1, 2018 at 7:52 AM Ashesh Vashi <
>>> ashesh.va...@enterprisedb.com> wrote:
>>>
>>>> On Thu, May 24, 2018 at 8:13 PM, Joao De Almeida Pereira <
>>>> jdealmeidapere...@pivotal.io> wrote:
>>>>
>>>>> Hey, Thanks so much for the reply.
>>>>>
>>>>> We've noticed that you've made several modifications on top of our
>>>>> original patch. Unfortunately, we've found it very hard to follow. Could 
>>>>> we
>>>>> please get a brief synopsis of the changes you have made - just so we can
>>>>> better understand the rationale behind them? Just like we've done for you
>>>>> previously.
>>>>>
>>>> Please find the changes from your original patch:
>>>>
>>>> M webpack.shim.js
>>>> M webpack.test.config.js
>>>> - In order to specify the fake_browser in regression tests, we need to use 
>>>> 'pgbrowser/browser' in the 'schema_child_tree_node.js' script.D 
>>>> pgadmin/browser/server_groups/servers/databases/schemas/static/js/can_drop_child.js
>>>> - We don't need this with the new implementation.C 
>>>> pgadmin/browser/server_groups/servers/databases/schemas/static/js/child.js
>>>> - All the children of schema node have common properties as 'parent_type', 
>>>> 'canDrop', 'canDropCascase', 'canCreate'.
>>>>   Hence - instead of defining them in each node, we have created a base 
>>>> node, which will have all these properties.
>>>>   And, modified all schema children node to inherit from it.C 
>>>> pgadmin/browser/server_groups/servers/databases/schemas/static/js/schema_child_tree_node.js
>>>> - In this script, we're defining three functions 'childCreateMenuEnabled', 
>>>> 'isTreeItemOfChildOfSchema', & 'isTreeNodeOfSchemaChild', which are used 
>>>> by the 'SchemaChildNode' objects.M pgadm

Re: [pgadmin4][patch] Initial patch to decouple from ACI Tree

[Victoria Henry]

Hi Ashesh,

We just attempted to apply your patch over master but it did not work.  We
don't want to introduce any bugs or break any functionality.  Please update
the patch to make sure it is synced up with the master branch.

Sincerely,

Victoria

On Fri, Jun 1, 2018 at 11:18 AM Anthony Emengo  wrote:

> Hey Ashesh,
>
> Thanks for the explanation. It was great and it really helped!
>
> C pgadmin/browser/server_groups/servers/databases/schemas/static/js/child.js
> C 
> pgadmin/browser/server_groups/servers/databases/schemas/static/js/schema_child_tree_node.js
>
> It makes sense to remove duplication by extracting these attributes out
> and setting the canDrop and canCreate functions here. But is it possible
> to combine these two files into one since they are related so we don’t need
> to import schema_child_tree_node?
>
> M pgadmin/static/js/tree/tree.js
>
> The creation of the ancestorNode function feels like a pre-optimization.
> That function is not used any where outside of the tree.js file, so it’s
> more confusing to have it defined. On a lighter note, could we avoid the !!
> syntax when possible? For example, instead of return !!obj, we could do
> something like return obj === undefined or return _.isUndefined(obj) as
> this is more intuitive.
>
> https://softwareengineering.stackexchange.com/a/80092
>
> In addition, please update this patch as it is out of sync with the latest
> commit on the master branch. Otherwise, everything looks good!
> ​
>
> Thanks
> Anthony && Victoria
>
> On Fri, Jun 1, 2018 at 7:52 AM Ashesh Vashi 
> wrote:
>
>> On Thu, May 24, 2018 at 8:13 PM, Joao De Almeida Pereira <
>> jdealmeidapere...@pivotal.io> wrote:
>>
>>> Hey, Thanks so much for the reply.
>>>
>>> We've noticed that you've made several modifications on top of our
>>> original patch. Unfortunately, we've found it very hard to follow. Could we
>>> please get a brief synopsis of the changes you have made - just so we can
>>> better understand the rationale behind them? Just like we've done for you
>>> previously.
>>>
>> Please find the changes from your original patch:
>>
>> M webpack.shim.js
>> M webpack.test.config.js
>> - In order to specify the fake_browser in regression tests, we need to use 
>> 'pgbrowser/browser' in the 'schema_child_tree_node.js' script.D 
>> pgadmin/browser/server_groups/servers/databases/schemas/static/js/can_drop_child.js
>> - We don't need this with the new implementation.C 
>> pgadmin/browser/server_groups/servers/databases/schemas/static/js/child.js
>> - All the children of schema node have common properties as 'parent_type', 
>> 'canDrop', 'canDropCascase', 'canCreate'.
>>   Hence - instead of defining them in each node, we have created a base 
>> node, which will have all these properties.
>>   And, modified all schema children node to inherit from it.C 
>> pgadmin/browser/server_groups/servers/databases/schemas/static/js/schema_child_tree_node.js
>> - In this script, we're defining three functions 'childCreateMenuEnabled', 
>> 'isTreeItemOfChildOfSchema', & 'isTreeNodeOfSchemaChild', which are used by 
>> the 'SchemaChildNode' objects.M pgadmin/browser/static/js/collection.js
>> - Fixed an issue related to wrongly defined 'error' function for the 
>> Collection object.D pgadmin/static/js/menu/can_create.js
>> - It defined the function, which was defining a check for creation of a 
>> schema child node, or not by looking at the parent node (i.e. a 
>> schema/catalog node).
>>   The file was not defintely placed under the wrong directory, because - the 
>> similar logic was under 'can_drop_child.js', and it was defined under 
>> 'pgadmin/browser/server_groups/servers/databases/schemas/static/js' 
>> directory.D pgadmin/static/js/menu/menu_enabled.jsC 
>> pgadmin/static/js/nodes/supported_database_node.js
>> - Used by the external tools for checking whether the 'selected' tree-node 
>> is:
>>   + 'database' node, and it is allowed to connect it.
>>   + Or, it is one of the schema child (and, not 'catalog' child).
>> - Finding the correct location was difficult for this, as there is no 
>> defined pattern, also it can be used by other functions too. Hence - moved 
>> it out of 'pgadmin/static/js/menu' directory.M pgadmin/static/js/tree/tree.js
>> - Introduced a function, which returns the ancestor node object, fow which 
>> the condition is true.D regression/javascript/menu/can_create_spec.js
>> D regression/javascript/menu/menu_enabled_spec.js
>> D regression/javascript/schema/can_drop_child_spec.jsC 
>> regression/javascript/fake_browser/browser.js
>> C regression/javascript/nodes/schema/child_menu_spec.js
>> - Modified the regression to test the new functionalies.M 
>> pgadmin/browser/server_groups/servers/databases/schemas/**/*.js
>> - Extending the schema child nodes from the 'SchemaChildNode' class defined 
>> in 'pgadmin/.../schemas/static/js/child.js' script.
>>
>> Let me know if you need more information.
>>
>>
>>> Let's keep in mind that the original intent was simply to 

Re: [pgadmin4][Patch]: Test cases for the backup module

[Victoria Henry]

Hi there,

We've been noticing some issues with the tests on both our CI and local Mac
workstations.

   1. When the following code blocks are invoked - we get plenty of
   app.context() issues. It must not be valid when running tests.

​

from pgadmin.utils.driver import get_driver
driver = get_driver(PG_DEFAULT_DRIVER)
manager = driver.connection_manager(self.sid)

host = manager.local_bind_host if manager.use_ssh_tunnel else s.host
port = manager.local_bind_port if manager.use_ssh_tunnel else s.port

2. When we finally enable

"default_binary_paths": {

in our test_config, we get more failing tests that look like:

==
FAIL: runTest 
(pgadmin.tools.restore.tests.test_restore_create_job_unit_test.RestoreCreateJobTest)
When restore object with option - Miscellaneous
--
Traceback (most recent call last):
  File "/Users/pivotal/.pyenv/versions/3.6.5/lib/python3.6/unittest/mock.py",
line 1179, in patched
return func(*args, **keywargs)
  File 
"/Users/pivotal/workspace/pgadmin4/web/pgadmin/tools/restore/tests/test_restore_create_job_unit_test.py",
line 295, in runTest
self.assertEquals(response.status_code, 200)
AssertionError: 410 != 200

And

When restore object with the sections options ... 2018-05-31
12:24:42,988: ERRORpgadmin:illegal environment variable name
Traceback (most recent call last):
  File 
"/Users/pivotal/workspace/pgadmin4/web/pgadmin/tools/restore/__init__.py",
line 352, in create_restore_job
manager.export_password_env(p.id)
  File 
"/Users/pivotal/workspace/pgadmin4/web/pgadmin/utils/driver/psycopg2/server_manager.py",
line 365, in export_password_env
os.environ[str(env)] = password
  File "/Users/pivotal/.pyenv/versions/3.6.5/lib/python3.6/os.py",
line 675, in __setitem__
self.putenv(key, value)
ValueError: illegal environment variable name
FAIL

​

Sincerely,

Victoria && Anthony

On Thu, May 31, 2018 at 1:16 AM Khushboo Vashi <
khushboo.va...@enterprisedb.com> wrote:

> Hi,
>
> Please find the attached updated patch with the fixes.
> The test cases were only failing on MAC not on Linux.
>
> Thanks,
> Khushboo
>
> On Wed, May 30, 2018 at 10:13 AM, Khushboo Vashi <
> khushboo.va...@enterprisedb.com> wrote:
>
>>
>>
>> On Wed, May 30, 2018 at 1:05 AM, Dave Page  wrote:
>>
>>> Hi
>>>
>>> On Mon, May 28, 2018 at 8:09 AM, Khushboo Vashi <
>>> khushboo.va...@enterprisedb.com> wrote:
>>>
 Hi,

 please find the attached updated patch for the test cases of Backup,
 Restore and Maintenance modules which includes:

 1. Unit test cases
 2. End to end regression test cases
 3. Feature test cases

>>>
>>> Thanks. I've yet to be able to run the feature tests successfully.
>>> Here's what I've found so far:
>>>
>>> 1) DEFAULT_BINARY_PATHS should be default_binary_paths in the JSON
>>> config file.
>>>
>>> Will do.
>>
>>> 2) I've hit screensize related issues:
>>>
>>> ==
>>>
>>> ERROR: runTest
>>> (pgadmin.feature_tests.pg_utilities_maintenance_test.PGUtilitiesMaintenanceFeatureTest)
>>>
>>> Test for PG maintenance: database
>>>
>>> --
>>>
>>> Traceback (most recent call last):
>>>
>>>   File
>>> "/Users/dpage/git/pgadmin4/web/pgadmin/feature_tests/pg_utilities_maintenance_test.py",
>>> line 56, in runTest
>>>
>>> self._open_maintenance_dialogue()
>>>
>>>   File
>>> "/Users/dpage/git/pgadmin4/web/pgadmin/feature_tests/pg_utilities_maintenance_test.py",
>>> line 75, in _open_maintenance_dialogue
>>>
>>> "*[.='" + self.table_name + "']/../*[@class='aciTreeItem'"
>>>
>>>   File
>>> "/Users/dpage/.virtualenvs/pgadmin4/lib/python2.7/site-packages/selenium/webdriver/remote/webelement.py",
>>> line 80, in click
>>>
>>> self._execute(Command.CLICK_ELEMENT)
>>>
>>>   File
>>> "/Users/dpage/.virtualenvs/pgadmin4/lib/python2.7/site-packages/selenium/webdriver/remote/webelement.py",
>>> line 628, in _execute
>>>
>>> return self._parent.execute(command, params)
>>>
>>>   File
>>> "/Users/dpage/.virtualenvs/pgadmin4/lib/python2.7/site-packages/selenium/webdriver/remote/webdriver.py",
>>> line 312, in execute
>>>
>>> self.error_handler.check_response(response)
>>>
>>>   File
>>> "/Users/dpage/.virtualenvs/pgadmin4/lib/python2.7/site-packages/selenium/webdriver/remote/errorhandler.py",
>>> line 242, in check_response
>>>
>>> raise exception_class(message, screen, stacktrace)
>>>
>>> WebDriverException: Message: unknown error: Element >> class="aciTreeItem">... is not clickable at point (223, 604). Other
>>> element would receive the click: ...
>>>
>>>   (Session info: chrome=66.0.3359.181)
>>>
>>>   (Driver info: chromedriver=2.38.552518
>>> (183d19265345f54ce39cbb94cf81ba5f15905011),platform=Mac OS X 10.12.6 x86_64)
>>>
>>> 3) One time the test did start, but then I 

Re: Container build hanging

[Victoria Henry]

Hey Dave,

The reason that it was taking so long was because the linter was acting on
the javascript in the ycache directory in the container. The fix is to add
the ycache directory to the .eslintignore file. We can send a patch file,
if you’d like:

diff --git a/web/.eslintignore b/web/.eslintignore
index 7468ecb8..676a4f03 100644
--- a/web/.eslintignore
+++ b/web/.eslintignore
@@ -3,3 +3,4 @@ node_modules
 vendor
 templates/
 templates\
+ycache

​

We'd prefer using the linter via the command line over the javascript
config (that never worked well).

Thanks
Victoria && Anthony

On Wed, May 30, 2018 at 5:43 PM Dave Page  wrote:

> Hi
>
> So it turns out the hang was a result of:
>
> =
> Using the '.eslintignore' config file for excluding the temporary,
> vendor specific, and templates files, instead of writing our own logic
> to do so.
>
> Patch by: Anthony & Joao
> Reviewed by: Khushboo
>
> Branch
> --
> master
>
> Details
> ---
>
> https://git.postgresql.org/gitweb?p=pgadmin4.git;a=commitdiff;h=a7ee85619dfd4cf42a43919beeb1cd1680f46d53
> Author: Anthony Emengo 
>
> Modified Files
> --
> web/.eslintignore |  5 +
> web/package.json  |  2 +-
> web/pga_eslint.js | 44 
> 3 files changed, 6 insertions(+), 45 deletions(-)
> =
>
> Reverting that one patch fixes the issue. Can one or more of you take a
> look please?
>
> Note that running the Docker build is easy - just install Docker and run
> "make docker". However, it uses committed code in the build, so any changes
> need to be committed to your local tree or they'll be ignored (yes, we need
> to fix that).
>
> Thanks!
>
>
>
> On Wed, May 30, 2018 at 3:33 PM, Dave Page  wrote:
>
>> Hi
>>
>> The container build is hanging for me on 2 different machines at:
>>
>> 20:19:50 $ cross-env NODE_ENV=production yarn run bundle:dev
>> 20:19:51 $ yarn run linter && yarn run webpacker
>> 20:19:52 $ yarn eslint --no-eslintrc -c .eslintrc.js --ext .js --ext .jsx
>> .
>> 20:19:52 $ /pgadmin4/web/node_modules/.bin/eslint --no-eslintrc -c
>> .eslintrc.js --ext .js --ext .jsx .
>>
>> Both systems (a Jenkins build node and my laptop) hang at that step,
>> seemingly indefinitely.
>>
>> Running the same (e.g. yarn eslint --no-eslintrc -c .eslintrc.js --ext
>> .js --ext .jsx .) is fine directly on my laptop.
>>
>> Anyone got any ideas why that may be? The only thing I can think of is
>> the overly cute unicode symbols that yarn is spitting out may be confusing
>> docker.
>>
>> --
>> Dave Page
>> Blog: http://pgsnake.blogspot.com
>> Twitter: @pgsnake
>>
>> EnterpriseDB UK: http://www.enterprisedb.com
>> The Enterprise PostgreSQL Company
>>
>
>
>
> --
> Dave Page
> Blog: http://pgsnake.blogspot.com
> Twitter: @pgsnake
>
> EnterpriseDB UK: http://www.enterprisedb.com
> The Enterprise PostgreSQL Company
>

Re: [pgAdmin4][Patch] RM #3277 Runtime startup error handling is broken on Windows

[Victoria Henry]

Hi Akshay,

Thanks for the patch.  It looks good to me as long as it also works on
windows.

Sincerely,

Victoria & Anthony

On Thu, May 31, 2018 at 4:53 AM Akshay Joshi 
wrote:

> Hi Hackers,
>
> Attached is the patch file to fix RM #3277 Runtime startup error handling
> is broken on Windows. I have figure out the crash at the following call of
> Server.cpp(line no 326):
>
> PyRun_SimpleFile(fdopen(PyObject_AsFileDescriptor(PyFileObject),"r"), m_
> appfile_utf8.data())
>
> When application exits due to some error it throws *BEX refers to Buffer
> Overflow Exception, *when I debug and found that it throws BEX for above
> function call. I have replace that call with the following:
>
> PyRun_SimpleFile(cp, m_appfile_utf8.data())
>
> Anyways we have open the file using fopen() at the top of the function
> then why to open that again using *fdopen() **in the first call. *
>
> Tested the patch with Python 3.5 and 3.6, please review it.
>
> --
> *Akshay Joshi*
>
> *Sr. Software Architect *
>
>
>
> *Phone: +91 20-3058-9517Mobile: +91 976-788-8246*
>

Re: [pgAdmin4][Patch#3389] To prevent unwanted model changes in Server dialog

[Victoria Henry]

Hey Murtuza,

Thanks for the explanation.  That seems fine.  This patch can be submitted.

Sincerely,

Victoria & Anthony

On Thu, May 31, 2018 at 2:22 AM Murtuza Zabuawala <
murtuza.zabuaw...@enterprisedb.com> wrote:

> Hello Victoria & Joao,
>
>
> On Thu, May 31, 2018 at 2:17 AM, Joao De Almeida Pereira <
> jdealmeidapere...@pivotal.io> wrote:
>
>> Hi Murtuza,
>>
>> We are having a hard time understanding the logic on the change.
>>
>> If tunnel_authentication is not on the model and the tunnel_identify_file
>> is present, we change the tunnel_identify_file in the model to null? Is
>> this what you mean?
>>
> ​Yes, basically we make use of identity file only in case of tunnel
> authentication​ is enabled (true/false).
>
> The earlier logic was written in such way that if tunnel authentication is
> set to disbaled/false then set the identity file filed value to empty
> string in the model, But as you can see that tunnel authentication is by
> default set to '0' in the model, so whenever user opens a edit/properties
> dialog the identitiy files sets to empty string even if ssh tunnel is
> disbaled. In my patch I choose to nullify the value instead of empty string
> so that it will be easy on python side condition checking :)
>
>
>
>
>> Thanks
>> Victoria && Joao
>> ​
>>
>> On Wed, May 30, 2018 at 9:27 AM Murtuza Zabuawala <
>> murtuza.zabuaw...@enterprisedb.com> wrote:
>>
>>> Hi,
>>>
>>> PFA minor patch to fix the issue when you change any field in server
>>> dialog 'tunnel_identity_file' model value get included unnecessarily in the
>>> update request.
>>>
>>> eg: Change the server name and click on Save button, Check the request
>>> payload in network console.
>>>
>>> --
>>> Regards,
>>> Murtuza Zabuawala
>>> EnterpriseDB: http://www.enterprisedb.com
>>> The Enterprise PostgreSQL Company
>>>
>>>
>

Re: [pgAdmin4][RM#3289] Can't query SQL_ASCII database.

[Victoria Henry]

Hi Aditya,

We made a minor change to make the patch so the python linter can pass.
Attached is the change we made.
Everything else looks good.

Sincerely,

Victoria & Anthony

On Tue, May 22, 2018 at 4:46 AM Aditya Toshniwal <
aditya.toshni...@enterprisedb.com> wrote:

> Hi,
>
> PFA updated patch. Linter issues are fixed ( we dont have any linter setup
> for python :-( )
> Regarding test cases, they run successfully on my system and the reason it
> failed for pivotal is timeout exception. I am sorry I can't help with that.
>
> Traceback (most recent call last):
>   File
> "/tmp/build/a453582b/pgadmin-repo/web/pgadmin/feature_tests/keyboard_shortcut_test.py",
> line 52, in runTest
> self._check_shortcuts()
>   File
> "/tmp/build/a453582b/pgadmin-repo/web/pgadmin/feature_tests/keyboard_shortcut_test.py",
> line 77, in _check_shortcuts
> ") and contains(@class, 'open')]")
>   File
> "/root/.pyenv/versions/pgadmin36/lib/python3.6/site-packages/selenium/webdriver/support/wait.py",
> line 80, in until
> raise TimeoutException(message, screen, stacktrace)
> selenium.common.exceptions.TimeoutException: Message:
>
> Thanks and Regards,
> Aditya Toshniwal
> Software Engineer | EnterpriseDB Software Solutions | Pune
> "Don't Complain about Heat, Plant a tree"
>
> On Tue, May 22, 2018 at 1:37 PM, Dave Page  wrote:
>
>> Hi
>>
>> Pivotal's buildbot is showing problems with this patch:
>>
>>
>> https://gpdb-dev.bosh.pivotalci.info/teams/pgadmin/pipelines/pgadmin-patch/jobs/run-linter/builds/66
>> (linter failed)
>>
>> https://gpdb-dev.bosh.pivotalci.info/teams/pgadmin/pipelines/pgadmin-patch/jobs/run-tests/builds/84
>> (tests failed)
>>
>>
>> On Tue, May 22, 2018 at 7:05 AM, Aditya Toshniwal <
>> aditya.toshni...@enterprisedb.com> wrote:
>>
>>> Hi Hackers,
>>>
>>> PFA patch for RM#3289 where decode error was thrown on querying a
>>> SQL_ASCII database table. Please note, this problem occurs only on windows.
>>> Sample insert - insert into test_tab values ('é');
>>>
>>> psycopg2 has a encodings dictionary where Postgres Database Encodings
>>> are mapped to python equivalent. It uses 'ascii' decoder of python to
>>> decode for SQL_ASCII encoding. If data has characters beyond the limit of
>>> ascii then it failed. The solution would be to use utf_8 decoder instead of
>>> ascii. I tried setting the client_encoding using
>>> set_client_encoding('UTF8') method of a psycopg2 connection but no luck
>>> (also its not allowed for async connection). I also tried executing "SET
>>> CLIENT_ENCODING='UTF8'" but it didn't work too.
>>> So, as in the patch, I had to set encodings dict value directly to
>>> 'utf_8' and it seems to be working. Please note, the same is added to
>>> psycopg3 milestones
>>> https://github.com/psycopg/psycopg2/milestone/4
>>>
>>> Also fixed a small glitch for sql editor connection status check.
>>>
>>> Kindly review.
>>>
>>> Thanks and Regards,
>>> Aditya Toshniwal
>>> Software Engineer | EnterpriseDB Software Solutions | Pune
>>> "Don't Complain about Heat, Plant a tree"
>>>
>>
>>
>>
>> --
>> Dave Page
>> Blog: http://pgsnake.blogspot.com
>> Twitter: @pgsnake
>>
>> EnterpriseDB UK: http://www.enterprisedb.com
>> The Enterprise PostgreSQL Company
>>
>
>


RM3289_v2.patch
Description: Binary data

Re: [pgAdmin4][RM#3307]User interface unable to connect to database running on port range 1-1024

[Victoria Henry]

Hi Aditya,

Looks good to us!

Sincerely,

Anthony & Victoria


On Wed, May 16, 2018 at 7:24 AM Aditya Toshniwal <
aditya.toshni...@enterprisedb.com> wrote:

> Hi Hackers,
>
> PFA patch to allow server port number below 1024 till 1 in Create Server
> dialog.
>
> Thanks and Regards,
> Aditya Toshniwal
> Software Engineer | EnterpriseDB Software Solutions | Pune
> "Don't Complain about Heat, Plant a tree"
>

Re: [pgAdmin4][RM#3298] Query tool shortcut issue

[Victoria Henry]

Hi Aditya,

The patch looks fine and the tests are passing.

Sincerely,

Victoria & Anthony

On Tue, May 8, 2018 at 8:22 AM, Aditya Toshniwal <
aditya.toshni...@enterprisedb.com> wrote:

> Hi Hackers,
>
> PFA patch for query tool keyboard issue where arrow keys were not behaving
> as expected for execute options dropdown. Also, corrected couple of
> dropdown HTML codes.
>
> Request you to kindly review.
>
> Thanks and Regards,
> Aditya Toshniwal
> Software Engineer | EnterpriseDB Software Solutions | Pune
> "Don't Complain about Heat, Plant a tree"
>

Re: [pgAdmin4][Patch]: Fixed RMs #1220 & #1221

[Victoria Henry]

Hi Khushboo,

Could you write tests for this change?
These files are actually apart of the ACITree refactoring patch. If this
patch was already in master the task of adding tests would be much easier.

Victoria & Joao

On Wed, May 2, 2018 at 8:33 AM, Ashesh Vashi 
wrote:

> On Wed, May 2, 2018 at 5:56 PM, Akshay Joshi <
> akshay.jo...@enterprisedb.com> wrote:
>
>> Hi Khushboo
>>
>> I have reviewed your code and looks good to me. Can we change the message
>> from "The database name is inappropriate" to some meaningful message, so
>> that user should know why it is inappropriate. If user will be able to
>> create database with "=" in name then why Backup, Maintenance and Restore
>> fails.
>>
>
> Just curious, as I understand the problem, we're not able to able to run
> pg_dump/pg_restore/psql against the database, which contains '=' in
> the name.
> Can we use PGDATABASE environment variable for them?
>
> Of course - this tools may still fail when special characters (e.g. '=')
> exists in the name of the database objects (e.g. schema, table, etc).
>
>
> --
>
> Thanks & Regards,
>
> Ashesh Vashi
> EnterpriseDB INDIA: Enterprise PostgreSQL Company
> 
>
>
> *http://www.linkedin.com/in/asheshvashi
> *
>
>>
>> On Wed, May 2, 2018 at 3:44 PM, Khushboo Vashi <
>> khushboo.va...@enterprisedb.com> wrote:
>>
>>> Hi,
>>>
>>> Please find the attached patch which will fix RMs # 1220 and #1221.
>>>
>>> If the database name contains = then the backup, maintenance and restore
>>> jobs are failing.
>>> To fix these, we will display the error message regarding inappropriate
>>> database name.
>>>
>>> Thanks,
>>> Khushboo
>>>
>>
>>
>>
>> --
>> *Akshay Joshi*
>>
>> *Sr. Software Architect *
>>
>>
>>
>> *Phone: +91 20-3058-9517Mobile: +91 976-788-8246*
>>
>
>

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

[Victoria Henry]

Hi Akshay,

Thanks for sending this updated patch.  The linter and tests are all
passing.

> - utils/driver/psycopg2/server_manager.py
>>   - Do we have Unit Tests around this?
>
>  No.


In our opinion, server_manager.py and connection.py should have tests.  Are
you finding it difficult to add tests to these files?

Sincerely,

Victoria & Joao


On Wed, May 2, 2018 at 5:58 AM, Akshay Joshi 
wrote:

> Hi Joao
>
>
> On Thu, Apr 26, 2018 at 10:26 PM, Joao De Almeida Pereira <
> jdealmeidapere...@pivotal.io> wrote:
>
>> 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.
>>
>
> If I understood you correctly, you want separate connect and create
> function for SSH Tunnel. If yes I don't think so we should move the SSH
> code and make the rest of the code duplicated in two different functions.
> For "create" function I have just added logic to pass SSH tunnel parameter
> while creating server object, encrypt the tunnel password and send it to
> connect method. For "connect" function encrypt the password and send it
> connect method as parameter. There is no point for such small changes we
> should create two separate functions.
>
>>
>> - utils/driver/psycopg2.py same comments has above
>>
>
>I think you are talking about "utils/driver/psycopg2/connection.py".
> Same comments as 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?
>>
>
>   Fixed.
>
>>   - 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
>>
>Model creation is the main functionality of the "server.js" (or
> any other module file), code readability wise it should be in the same
> file. If we will do it for rest of the modules then there are so many java
> script files where model creation is in separate file.
>
>   - We could also convert this file to ES6
>>
>
>  I am new to this, so will need to learn first. We can create a
> separate task to do this.
>
>>
>> - utils/driver/psycopg2/server_manager.py
>>   - Do we have Unit Tests around this?
>>
>
>   No.
>
>>   - 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
>>
>
>   According to me SSH Tunnel parameters is the part of server manager
> as we do have other parameters of Server dialog. We can isolate the SSH
> part in other class, but most of the modules (including Server module) have
> access to the ServerManager. If we will isolate that part then anyways we
> will have to write wrapper functions in ServerManager which will
> eventually call functions of new SSH class.
>
>   As one ServerManager object belongs to one server, similarly one
> SSH Tunnel belongs to one server. When SSH tunnel gets created it will
> return local bind port, where rest of the communication should be done on
> local host and the local bind port return by the "SSHTunnelForwarder"
> class, so that need to be in the ServerManager.
>
>  Considering above I have kept that logic in ServerManager.
>
>>
>>
>> - 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
>>
>
>  Added one test case for SSHTunnelConnectionLost.
>
>>
>>
>> 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?
>>
>
>Fixed. Replace "mgr" with "manager" almost at 69 places in the file.
>
>Attached is the modified patch. Please review it.
>
>>
>>
>> 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 

Re: pgAdmin4 - Issue of unmaintained libraries

[Victoria Henry]

Hi Dave,

On Wed, Apr 4, 2018 at 5:03 AM, Dave Page  wrote:

> Hi
>
> On Tue, Apr 3, 2018 at 9:09 PM, Joao De Almeida Pereira <
> jdealmeidapere...@pivotal.io> wrote:
>
>> Hi Dave,
>>
>> On Tue, Apr 3, 2018 at 8:32 AM Dave Page  wrote:
>>
>>> Hi
>>>
>>> On Tue, Apr 3, 2018 at 1:22 PM, Murtuza Zabuawala <
>>> murtuza.zabuaw...@enterprisedb.com> wrote:
>>>
 Hi Team,

 As we are heavily dependent on 3rd party JS libraries and some of them
 are no longer actively maintained by their respective authors (Last commit
 was approximate year ago).

 Some of the core libraries which are no longer actively maintained are,
 - Backbone 
 - wcDocker 
 - aciTree 
 - Backgrid /Backform
 - jQuery 1.x 
 version

 What would be our future plans when it comes to fixing the issues in
 the core libraries or adding new feature?
 (Ref: Email thread
 
 )

>>>
>>> Well jQuery can be updated can't it?
>>>
>>> wcDocker is, as far as I'm aware, the only library of it's kind. Unless
>>> something else has surpassed it in the last couple of years, there's
>>> nothing even close in functionality to it. aciTree was in a similar
>>> position, though Joao and team think there may be other candidates now.
>>>
>>
>> About the wcDocker, in fact it as a lot of features. In our User
>> Interviews we didn't find any person that was using features like dragging
>> and creating new panels. The only feature that we see people using was the
>> tabs. So if the only feature that people use of wcDocker is really tabs
>> there are some other libraries like:
>> react-tabs or react-tabtab (Draggable tabs) or rc-tabs(Static tabs, with
>> 52k Downloads last 7 days).
>>
>
> On a personal level, I do re-arrange the panel layout.
>

> On a practical level, we create new panels every time you use the query
> tool or debugger, or open properties dialogues etc.
>

We do know that and all the libraries that we highlighted have the ability
to create tabs dynamically.


>
>>
>> For ACITree we already started the process of isolating it from the
>> application, This will allow us in the future to replace it with something
>> that is more up to date.
>>
>>
>>> As for Backbone/Backgrid/Backform, I don't know. Backbone could maybe be
>>> replaced with React eventually. Not sure about the others.
>>>
>>> In any case, this is likely to be a problem on an ongoing basis - and I
>>> think we need to consider the future on a case by case basis when
>>> necessary. It may mean moving to a different library, or it may mean
>>> forking components, or it may be the upstream may have not had any commits
>>> for a long time simply because there is no development happening right now,
>>> but bugs may still be fixed.
>>>
>>>
>> We understand and agree that this should be handled case-by-case instead
>> of replacing everything at once since it is not feasible to do a complete
>> rewrite of the application.  But we also think it's hard to build new
>> features on top of libraries that are no longer supported.  We don't really
>> want to fork a library either because we'll become responsible for
>> maintaining that library, which means maintaining more code.  We want to
>> use libraries that are actively maintained so we can get security patches,
>> new features, etc.  Also, if we need a new functionality on a specific
>> library, we can, for example, open an issue and the maintainers of that
>> library can implement it for us or we can create a pull request for them to
>> merge.
>>
>
> Sure, but the main issue is that projects get abandoned all the time. We
> have no way of knowing what will get dropped and what won't until it
> actually happens - and even then it's not always clear, for example the
> wcDocker case where there haven't been code changes in some time, but the
> lead developer is still responding to issues very promptly.
>

There are 2 PR one from 2017 and another one from Feb that have no comments
on them, and there are open issues also because that have no answer or were
never worked on. Despite the fact that he does answer emails, it looks like
the library will not see any further development.


>
> My point is that it's very hard to know when a component needs to be
> replaced, unless the team actually announces its EOL, and even then some
> components require a huge amount of effort to replace them, when in reality
> forking them and maintaining the fork may require significantly less effort.
>

You are right, specially in the world of Javascript everything changes at a
very high rate. The decisions 

Re: [pgadmin][patch] [GreenPlum] When user press Explain Plan and Explain analyze plan an error is displayed

[Victoria Henry]

Hi Hackers,

We fixed the tests and refactored some of the code.  All tests pass now.
Attached is the reviewed patch.

Sincerely,

Joao and Victoria

On Tue, Mar 20, 2018 at 10:05 AM, Dave Page 
wrote:

> Hi
>
> This doesn't pass the Javascript tests for me. Please investigate ASAP:
>
> webpack: Compiled successfully.
> HeadlessChrome 0.0.0 (Mac OS X 10.12.6): Executed 152 of 486 SUCCESS (0
> secs / 0 secs)
> HeadlessChrome 0.0.0 (Mac OS X 10.12.6): Executed 153 of 486 SUCCESS (0
> secs / 0 secs)
> HeadlessChrome 0.0.0 (Mac OS X 10.12.6): Executed 154 of 486 SUCCESS (0
> secs / 0 secs)
> HeadlessChrome 0.0.0 (Mac OS X 10.12.6): Executed 155 of 486 SUCCESS (0
> secs / 0 secs)
> HeadlessChrome 0.0.0 (Mac OS X 10.12.6): Executed 156 of 486 SUCCESS (0
> secs / 0 secs)
> HeadlessChrome 0.0.0 (Mac OS X 10.12.6): Executed 157 of 486 SUCCESS (0
> secs / 0 secs)
> HeadlessChrome 0.0.0 (Mac OS X 10.12.6): Executed 158 of 486 SUCCESS (0
> secs / 0 secs)
> HeadlessChrome 0.0.0 (Mac OS X 10.12.6) ExecuteQuery #poll when SQLEditor
> is the query tool when an error occur when the connection to the server was
> lost when JSON response is available when login is not required should
> highlight the error in the SQL panel FAILED
> Expected spy SqlEditor._highlight_error to have been called with [ 'Some
> error in JSON' ] but it was never called.
> at regression/javascript/sqleditor/execute_query_spec.js:11753:58
> HeadlessChrome 0.0.0 (Mac OS X 10.12.6): Executed 285 of 486 (1 FAILED) (0
> secs / 0 secs)
> HeadlessChrome 0.0.0 (Mac OS X 10.12.6) ExecuteQuery #poll when SQLEditor
> is the query tool when an error occur when the connection to the server was
> lost when JSON response is available when login is not required should
> highlight the error in the SQL panel FAILED
> Expected spy SqlEditor._highlight_error to have been called with [ 'Some
> error in JSON' ] but it was never called.
> HeadlessChrome 0.0.0 (Mac OS X 10.12.6) ExecuteQuery #poll when SQLEditor
> is the query tool when an error occur when the connection to the server was
> lost when JSON response is available when login is not required should add
> new entry to history and update the Messages tab FAILED
> Expected spy SqlEditor.update_msg_history to have been called with [
> false, 'Some error in JSON' ] but it was never called.
> at regression/javascript/sqleditor/execute_query_spec.js:11760:60
> HeadlessChrome 0.0.0 (Mac OS X 10.12.6): Executed 286 of 486 (2 FAILED) (0
> secs / 0 secs)
> HeadlessChrome 0.0.0 (Mac OS X 10.12.6) ExecuteQuery #poll when SQLEditor
> is the query tool when an error occur when the connection to the server was
> lost when JSON response is available when login is not required should add
> new entry to history and update the Messages tab FAILED
> Expected spy SqlEditor.update_msg_history to have been called with [
> false, 'Some error in JSON' ] but it was never called.
> HeadlessChrome 0.0.0 (Mac OS X 10.12.6) ExecuteQuery #poll when SQLEditor
> is the query tool when an error occur when the connection to the server was
> lost when JSON response is available when login is required should login is
> displayed FAILED
> Expected spy UserManagement.pga_login to have been called.
> at regression/javascript/sqleditor/execute_query_spec.js:11840:56
> HeadlessChrome 0.0.0 (Mac OS X 10.12.6): Executed 296 of 486 (3 FAILED) (0
> secs / 0 secs)
> HeadlessChrome 0.0.0 (Mac OS X 10.12.6) ExecuteQuery #poll when SQLEditor
> is the query tool when an error occur when the connection to the server was
> lost when JSON response is available when login is required should login is
> displayed FAILED
> Expected spy UserManagement.pga_login to have been called.
> HeadlessChrome 0.0.0 (Mac OS X 10.12.6) ExecuteQuery #poll when SQLEditor
> is the query tool when an error occur when the connection to the server was
> lost when no JSON response is available when login is not required should
> highlight the error in the SQL panel FAILED
> Expected spy SqlEditor._highlight_error to have been called with [ 'Some
> plain text error' ] but it was never called.
> at regression/javascript/sqleditor/execute_query_spec.js:11875:58
> HeadlessChrome 0.0.0 (Mac OS X 10.12.6): Executed 299 of 486 (4 FAILED) (0
> secs / 0 secs)
> HeadlessChrome 0.0.0 (Mac OS X 10.12.6) ExecuteQuery #poll when SQLEditor
> is the query tool when an error occur when the connection to the server was
> lost when no JSON response is available when login is not required should
> highlight the error in the SQL panel FAILED
> Expected spy SqlEditor._highlight_error to have been called with [ 'Some
> plain text error' ] but it was never called.
> HeadlessChrome 0.0.0 (Mac OS X 10.12.6) ExecuteQuery #poll when SQLEditor
> is the query tool when an error occur when the connection to the server was
> lost when no JSON response is available when login is not required should
> add new entry to history and update the Messages tab FAILED
> Expected spy 

Re: [pgAdmin4][RM#3195] Include service name when executing Backup, Restore etc tools

[Victoria Henry]

Hey Murtuza,

This patch passed our test pipelines and it looks good.
Do we have any tests around this behavior?

Joao and Victoria

Re: [pgAdmin4][RM#3140] Add service parameter

[Victoria Henry]

Hi Dave,

We've made updated our previous patch to fix the error messages.  Attached
are our updated patches.

On Mon, Mar 12, 2018 at 8:48 PM, Dave Page  wrote:

> Hi
>
> On Mon, Mar 12, 2018 at 5:18 PM, Joao De Almeida Pereira <
> jdealmeidapere...@pivotal.io> wrote:
>
>> Hi Dave and Murtuza,
>>
>> Regarding this patch we refactored the Javascript code so that is lives
>> in a different file and added some tests.
>>
>> Also we found an issue with karma-jasmine that does not allow us to use
>> jasmine 3.1 yet. You can find attached a patch that reverts that commit.
>>
>
> Sounds good, but neither patch will apply (in fact, the Jasmine one looks
> entirely backwards). One of the error messages was changed in Murtuza's
> patch, and wasn't reflected in your update for example.
>
> Can you rebase please?
>
> Thanks.
>
>
>>
>> Thanks
>> Victoria && Joao
>>
>> On Mon, Mar 12, 2018 at 4:46 PM Dave Page  wrote:
>>
>>> Thanks, patch applied!
>>>
>>> On Mon, Mar 12, 2018 at 3:31 AM, Murtuza Zabuawala <
>>> murtuza.zabuaw...@enterprisedb.com> wrote:
>>>
 Hi Dave,

 PFA updated patch.

 --
 Regards,
 Murtuza Zabuawala
 EnterpriseDB: http://www.enterprisedb.com
 The Enterprise PostgreSQL Company


 On Fri, Mar 9, 2018 at 9:29 PM, Murtuza Zabuawala <
 murtuza.zabuaw...@enterprisedb.com> wrote:

> Hi Dave,
>
> I'll change the name and send you updated patch.
>
>
> On Fri, Mar 9, 2018 at 9:25 PM, Dave Page  wrote:
>
>> HI
>>
>> On Fri, Mar 9, 2018 at 11:47 AM, Murtuza Zabuawala <
>> murtuza.zabuaw...@enterprisedb.com> wrote:
>>
>>> Hi,
>>>
>>> PFA patch to add service parameter in server dialog.
>>> - Docs updated
>>> - Test case added for Service ID parameter
>>>
>>> Please note,
>>> I have extracted Connection class and Server manager class from our
>>> own custom Psycopg2 driver module.
>>>
>>> Patch also covers RM#3120
>>>
>>
>>  This patch seems a little confused. The "Service" and "Service ID"
>> fields from pgAdmin 3 are very different things. The Redmine ticket seems
>> to be asking for the Service field (the pg_service.conf service name),
>> *not* Service ID (the operating system's service ID, used to start/stop 
>> the
>> database server service).
>>
>> --
>> Dave Page
>> Blog: http://pgsnake.blogspot.com
>> Twitter: @pgsnake
>>
>> EnterpriseDB UK: http://www.enterprisedb.com
>> The Enterprise PostgreSQL Company
>>
>
>

>>>
>>>
>>> --
>>> Dave Page
>>> Blog: http://pgsnake.blogspot.com
>>> Twitter: @pgsnake
>>>
>>> EnterpriseDB UK: http://www.enterprisedb.com
>>> The Enterprise PostgreSQL Company
>>>
>>
>
>
> --
> Dave Page
> Blog: http://pgsnake.blogspot.com
> Twitter: @pgsnake
>
> EnterpriseDB UK: http://www.enterprisedb.com
> The Enterprise PostgreSQL Company
>
diff --git a/web/package.json b/web/package.json
index ad0f3e16..66684fc6 100644
--- a/web/package.json
+++ b/web/package.json
@@ -69,6 +69,7 @@
 "hard-source-webpack-plugin": "^0.4.9",
 "immutability-helper": "^2.2.0",
 "imports-loader": "^0.7.1",
+"ip-address": "^5.8.9",
 "jquery": "1.11.2",
 "jquery-contextmenu": "^2.5.0",
 "jquery-ui": "^1.12.1",
diff --git a/web/pgadmin/browser/server_groups/servers/static/js/server.js b/web/pgadmin/browser/server_groups/servers/static/js/server.js
index 36908733..302fe458 100644
--- a/web/pgadmin/browser/server_groups/servers/static/js/server.js
+++ b/web/pgadmin/browser/server_groups/servers/static/js/server.js
@@ -2,10 +2,13 @@ define('pgadmin.node.server', [
   'sources/gettext', 'sources/url_for', 'jquery', 'underscore', 'backbone',
   'underscore.string', 'sources/pgadmin', 'pgadmin.browser',
   'pgadmin.server.supported_servers', 'pgadmin.user_management.current_user',
-  'pgadmin.alertifyjs', 'pgadmin.backform', 'pgadmin.browser.server.privilege',
+  'pgadmin.alertifyjs', 'pgadmin.backform',
+  'sources/browser/server_groups/servers/model_validation',
+  'pgadmin.browser.server.privilege',
 ], function(
   gettext, url_for, $, _, Backbone, S, pgAdmin, pgBrowser,
-  supported_servers, current_user, Alertify, Backform
+  supported_servers, current_user, Alertify, Backform,
+  modelValidation
 ) {
 
   if (!pgBrowser.Nodes['server']) {
@@ -848,110 +851,8 @@ define('pgadmin.node.server', [
   group: gettext('Connection'),
 }],
 validate: function() {
-  var err = {},
-errmsg,
-self = this;
-
-  var service_id = this.get('service');
-
-  var check_for_empty = function(id, msg) {
-var v = self.get(id);
-if (
-  _.isUndefined(v) || v === null || String(v).replace(/^\s+|\s+$/g, '') == ''
-) {
-  err[id] = msg;
-  errmsg = errmsg || msg;
-  return true;