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

[Dave Page]

Hi

On Thu, May 3, 2018 at 10:19 AM, Khushboo Vashi <
khushboo.va...@enterprisedb.com> 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.
>>
>
I changed the messages to read more like this: "Maintenance job creation
failed. Databases with = symbols in the name cannot be maintained using
this utility.".

However; I think that throwing the error when the user tries to execute the
process is quite unhelpful, as the user may have spent some time choosing
options etc. Can we do it when they open the dialogue (show the error
instead of the tool's dialogue)?

Thanks.


>
>> Done. Please find the attached updated patch.
>
>> 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*
>>
>
>


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

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

Re: [pgAdmin4][RM#3324] - Windows user unable to expand "External Tables" navigation item

[Dave Page]

Thanks, patch applied!

On Wed, May 2, 2018 at 7:25 PM, Anthony Emengo  wrote:

> Forgive me but please consider the following patch instead - as it better
> adheres to the python style guide.
>
> On Wed, May 2, 2018 at 12:43 PM Anthony Emengo  wrote:
>
>> Hi Hackers,
>>
>> Please find the attached patch to fix the RM #3324 : user cannot load
>> “External Tables” on the navigation pane.
>>
>> The issue was that ultimately paths were not being cleaned after being
>> munged from the “template” input which would result in template_paths that
>> resembled: path//to//dir.sql. These would work on unix based systems but
>> lead to complication on Windows.
>>
>> The following patch resolves this issue, and also included a small file
>> refactor to better convey developer intent.
>>
>> Thanks,
>> Anthony
>> ​
>>
>


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

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

pgAdmin 4 commit: Fix the template loader to work reliably under Window

[Dave Page]

Fix the template loader to work reliably under Windows (fixing external tables 
under Greenplum). Fixes #3324

Branch
--
master

Details
---
https://git.postgresql.org/gitweb?p=pgadmin4.git;a=commitdiff;h=56658a9f8e676bb287e8e191b9acca74a23fc4e1
Author: Anthony Emengo 

Modified Files
--
docs/en_US/release_notes_3_1.rst   |  3 +-
web/pgadmin/utils/versioned_template_loader.py | 93 --
2 files changed, 60 insertions(+), 36 deletions(-)

Re: [pgAdmin4][RM#3238] Proper error handling when connection will be lost to server.

[Dave Page]

Hi

On Thu, May 3, 2018 at 11:23 AM, Aditya Toshniwal <
aditya.toshni...@enterprisedb.com> wrote:

> Hi Hackers,
>
> PFA patch for proper error handling when connection will be lost to
> server. I have created a common function alertify.pgRespErrorNotify in
> web/pgadmin/static/js/alertify.pgadmin.defaults.js on the lines of
> existing function alertify.pgNotifier. The function is called in error
> callback of $.ajax call. The XHR is passed and handled in the function
> itself. Please note that I have used JSON.parse and not $.parseJSON as it
> will be deprecated in jQuery 3.x and thus removing many $.parseJSON calls.
>
> Request you to kindly review.
>

Can you add some appropriate tests for the new function please?

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

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

[pgAdmin4][RM#3238] Proper error handling when connection will be lost to server.

[Aditya Toshniwal]

Hi Hackers,

PFA patch for proper error handling when connection will be lost to server.
I have created a common function alertify.pgRespErrorNotify in
web/pgadmin/static/js/alertify.pgadmin.defaults.js on the lines of existing
function alertify.pgNotifier. The function is called in error callback of
$.ajax call. The XHR is passed and handled in the function itself. Please
note that I have used JSON.parse and not $.parseJSON as it will be
deprecated in jQuery 3.x and thus removing many $.parseJSON calls.

Request you to kindly review.


Thanks and Regards,
Aditya Toshniwal
Software Engineer | EnterpriseDB Software Solutions | Pune
"Don't Complain about Heat, Plant a tree"
diff --git a/web/pgadmin/browser/server_groups/servers/databases/casts/static/js/cast.js b/web/pgadmin/browser/server_groups/servers/databases/casts/static/js/cast.js
index 97e689b..2e41ed6 100644
--- a/web/pgadmin/browser/server_groups/servers/databases/casts/static/js/cast.js
+++ b/web/pgadmin/browser/server_groups/servers/databases/casts/static/js/cast.js
@@ -209,15 +209,8 @@ define('pgadmin.node.cast', [
 },
 
  // On failure show error appropriate error message to user
-error: function(xhr) {
-  try {
-var err = $.parseJSON(xhr.responseText);
-if (err.success == 0) {
-  alertify.error(err.errormsg);
-}
-  } catch (e) {
-// Do nothing
-  }
+error: function(xhr, status, error) {
+  alertify.pgRespErrorNotify(xhr, error);
 },
   });
 }
diff --git a/web/pgadmin/browser/server_groups/servers/databases/schemas/synonyms/static/js/synonym.js b/web/pgadmin/browser/server_groups/servers/databases/schemas/synonyms/static/js/synonym.js
index e97dd05..d0ecaab 100644
--- a/web/pgadmin/browser/server_groups/servers/databases/schemas/synonyms/static/js/synonym.js
+++ b/web/pgadmin/browser/server_groups/servers/databases/schemas/synonyms/static/js/synonym.js
@@ -163,15 +163,8 @@ define('pgadmin.node.synonym', [
   },
 
   // On failure show error appropriate error message to user
-  error: function(xhr) {
-try {
-  var err = $.parseJSON(xhr.responseText);
-  if (err.success == 0) {
-alertify.error(err.errormsg);
-  }
-} catch (e) {
-  // Ignore
-}
+  error: function(xhr, status, error) {
+alertify.pgRespErrorNotify(xhr, error);
   },
 });
 return res;
diff --git a/web/pgadmin/browser/server_groups/servers/databases/schemas/tables/constraints/check_constraint/static/js/check_constraint.js b/web/pgadmin/browser/server_groups/servers/databases/schemas/tables/constraints/check_constraint/static/js/check_constraint.js
index bf7ef55..857cf4c 100644
--- a/web/pgadmin/browser/server_groups/servers/databases/schemas/tables/constraints/check_constraint/static/js/check_constraint.js
+++ b/web/pgadmin/browser/server_groups/servers/databases/schemas/tables/constraints/check_constraint/static/js/check_constraint.js
@@ -73,15 +73,8 @@ define('pgadmin.node.check_constraint', [
 setTimeout(function() {t.select(i);}, 100);
   }
 },
-error: function(xhr) {
-  try {
-var err = $.parseJSON(xhr.responseText);
-if (err.success == 0) {
-  alertify.error(err.errormsg);
-}
-  } catch (e) {
-console.warn(e.stack || e);
-  }
+error: function(xhr, status, error) {
+  alertify.pgRespErrorNotify(xhr, error);
   t.unload(i);
 },
   });
diff --git a/web/pgadmin/browser/server_groups/servers/databases/schemas/tables/constraints/foreign_key/static/js/foreign_key.js b/web/pgadmin/browser/server_groups/servers/databases/schemas/tables/constraints/foreign_key/static/js/foreign_key.js
index 4997d17..29684b6 100644
--- a/web/pgadmin/browser/server_groups/servers/databases/schemas/tables/constraints/foreign_key/static/js/foreign_key.js
+++ b/web/pgadmin/browser/server_groups/servers/databases/schemas/tables/constraints/foreign_key/static/js/foreign_key.js
@@ -665,15 +665,8 @@ define('pgadmin.node.foreign_key', [
 setTimeout(function() {t.select(i);}, 100);
   }
 },
-error: function(xhr) {
-  try {
-var err = $.parseJSON(xhr.responseText);
-if (err.success == 0) {
-  Alertify.error(err.errormsg);
-}
-  } catch (e) {
-console.warn(e.stack || e);
-  }
+error: function(xhr, status, error) {
+

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

[Khushboo Vashi]

Hi Victoria,

On Wed, May 2, 2018 at 9:20 PM, Victoria Henry  wrote:

> Hi Khushboo,
>
> Could you write tests for this change?
>
We don't have any tests for these utilities, so at this point not possible
to write the test cases for the same.

> 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
>
> Thanks,
Khushboo

> On Wed, May 2, 2018 at 8:33 AM, Ashesh Vashi <
> ashesh.va...@enterprisedb.com> 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]: Fixed RMs #1220 & #1221

[Khushboo Vashi]

On Wed, May 2, 2018 at 5:56 PM, Akshay Joshi 
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.
>
> Done. Please find the attached updated patch.

> 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*
>
diff --git a/web/pgadmin/tools/backup/static/js/backup.js b/web/pgadmin/tools/backup/static/js/backup.js
index ddbfaee..8ac2f44 100644
--- a/web/pgadmin/tools/backup/static/js/backup.js
+++ b/web/pgadmin/tools/backup/static/js/backup.js
@@ -984,6 +984,15 @@ commonUtils
 
 var treeInfo = node.getTreeNodeHierarchy.apply(node, [i]);
 
+if (treeInfo.database._label.indexOf('=') >= 0) {
+  alertify.alert(
+gettext('Backup error'),
+gettext('Backup job creation failed. '+
+'The database name with equal to sign is not supported by this utility.')
+  );
+  return;
+}
+
 // Set current database into model
 this.view.model.set('database', treeInfo.database._label);
 
diff --git a/web/pgadmin/tools/maintenance/static/js/maintenance.js b/web/pgadmin/tools/maintenance/static/js/maintenance.js
index 81e4594..c725f37 100644
--- a/web/pgadmin/tools/maintenance/static/js/maintenance.js
+++ b/web/pgadmin/tools/maintenance/static/js/maintenance.js
@@ -359,6 +359,15 @@ define([
 
 var treeInfo = node.getTreeNodeHierarchy.apply(node, [i]);
 
+if (treeInfo.database._label.indexOf('=') >= 0) {
+  Alertify.alert(
+gettext('Maintenance error'),
+gettext('Maintenance job creation failed. '+
+'The database name with equal to sign is not supported by this utility.')
+  );
+  return;
+}
+
 if (treeInfo.schema != undefined) {
   schema = treeInfo.schema._label;
 }
diff --git a/web/pgadmin/tools/restore/static/js/restore.js b/web/pgadmin/tools/restore/static/js/restore.js
index 5c082a9..f15531d 100644
--- a/web/pgadmin/tools/restore/static/js/restore.js
+++ b/web/pgadmin/tools/restore/static/js/restore.js
@@ -616,6 +616,16 @@ commonUtils
 
 var info = node.getTreeNodeHierarchy.apply(node, [i]),
   m = this.view.model;
+
+if (info.database._label.indexOf('=') >= 0) {
+  alertify.alert(
+gettext('Restore error'),
+gettext('Restore job creation failed. '+
+'The database name with equal to sign is not supported by this utility.')
+  );
+  return;
+}
+
 // Set current node info into model
 m.set('database', info.database._label);
 if (!m.get('custom')) {

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

[Akshay Joshi]

Hi

On Thu, May 3, 2018 at 2:20 PM, Dave Page  wrote:

>
>
> On Thu, May 3, 2018 at 6:58 AM, Akshay Joshi <
> akshay.jo...@enterprisedb.com> wrote:
>
>> Hi
>>
>> On Wed, May 2, 2018 at 9:05 PM, Victoria Henry  wrote:
>>
>>> 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?
>>>
>>
>>We will have to write test cases from scratch for both the files and
>> it will take time, there is no point keeping these important feature(SSH
>> Tunnel) on hold. We can create a separate task for this as we have for
>> utility(Backup, Maintenance, Restore) modules.
>>
>>@Dave your thoughts on this?
>>
>
> I agree. Please add a ticket to add these tests in the future. General
> tests for those files should not hold up this feature.
>

Done. Can you please review and commit this feature.

>
>
>>
>>> Sincerely,
>>>
>>> Victoria & Joao
>>>
>>>
>>> On Wed, May 2, 2018 at 5:58 AM, Akshay Joshi <
>>> akshay.jo...@enterprisedb.com> 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. 

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

[Dave Page]

On Thu, May 3, 2018 at 7:59 AM, Ashesh Vashi 
wrote:

> On Thu, May 3, 2018 at 12:25 PM, Khushboo Vashi <
> khushboo.va...@enterprisedb.com> wrote:
>
>>
>>
>> On Wed, May 2, 2018 at 6:03 PM, Ashesh Vashi <
>> ashesh.va...@enterprisedb.com> 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?
>>>
>>>
>> This way we can implement, but should we consider this way only in case
>> of the database name having "=" ?
>>
> I think - we should do it for all for consistent result.
>
>> Also, the command on the dialogue will not have database name, so user
>> might get confused.
>>
> What need to show as the command line is upto us?
> We can also show PGDATABASE= as environment variable for this command.
>

Cost vs. benefit; how many people actually use = in their database names,
and of those, how many will back it up using pgAdmin? My guess is that
number is zero, or extremely close to it - and we've only ever had the
issue reported by our own QA people who are intentionally trying to break
this stuff.

Let's just not support external tools on any objects with = in their names.
If there are complaints from users in the future, we can revisit.


>
>>
>>> 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).
>>>
>>> It's working with this approach.
>>
> Good to know that.
>
> -- Thanks, Ashesh
>
>>
>>> --
>>>
>>> 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*

>>>
>>>
>>
>


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

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

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

[Dave Page]

On Thu, May 3, 2018 at 6:58 AM, Akshay Joshi 
wrote:

> Hi
>
> On Wed, May 2, 2018 at 9:05 PM, Victoria Henry  wrote:
>
>> 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?
>>
>
>We will have to write test cases from scratch for both the files and it
> will take time, there is no point keeping these important feature(SSH
> Tunnel) on hold. We can create a separate task for this as we have for
> utility(Backup, Maintenance, Restore) modules.
>
>@Dave your thoughts on this?
>

I agree. Please add a ticket to add these tests in the future. General
tests for those files should not hold up this feature.


>
>> Sincerely,
>>
>> Victoria & Joao
>>
>>
>> On Wed, May 2, 2018 at 5:58 AM, Akshay Joshi <
>> akshay.jo...@enterprisedb.com> 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 

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

[Ashesh Vashi]

On Thu, May 3, 2018 at 12:25 PM, Khushboo Vashi <
khushboo.va...@enterprisedb.com> wrote:

>
>
> On Wed, May 2, 2018 at 6:03 PM, Ashesh Vashi <
> ashesh.va...@enterprisedb.com> 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?
>>
>>
> This way we can implement, but should we consider this way only in case of
> the database name having "=" ?
>
I think - we should do it for all for consistent result.

> Also, the command on the dialogue will not have database name, so user
> might get confused.
>
What need to show as the command line is upto us?
We can also show PGDATABASE= as environment variable for this command.

>
>
>> 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).
>>
>> It's working with this approach.
>
Good to know that.

-- Thanks, Ashesh

>
>> --
>>
>> 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]: Fixed RMs #1220 & #1221

[Khushboo Vashi]

On Wed, May 2, 2018 at 6:03 PM, 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?
>
>
This way we can implement, but should we consider this way only in case of
the database name having "=" ?
Also, the command on the dialogue will not have database name, so user
might get confused.


> 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).
>
> It's working with this approach.

>
> --
>
> 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*
>>
>
>