Re: [pgadmin4][patch] Use ESLinter options instead of using Javascript

[Khushboo Vashi]

Hi Anthony,

The code looks good to me and working fine.

Thanks,
Khushboo

On Wed, May 16, 2018 at 9:23 PM, Anthony Emengo  wrote:

> Hey all, we'd definitely like some feedback on this patch. Does does it
> not apply cleanly? Do we need to resend it?
>
> Thanks
> Anthony
>
> On Mon, May 14, 2018 at 3:47 PM Anthony Emengo  wrote:
>
>> Apologies, here's a redo that should apply cleanly on top of master
>>
>> Anthony && Joao
>>
>> On Mon, May 14, 2018 at 3:25 PM Anthony Emengo 
>> wrote:
>>
>>> Hi Hackers,
>>>
>>> Attached you can find a patch that changes the way we are running ESLint
>>> on the Javascript files. Instead of using Javascript we can use ESLint
>>> parameters to archive the same goal.
>>>
>>> In the future this will also allow us to use the rest of the cli
>>> parameters like (--fix and others...)
>>>
>>> Thanks
>>> Anthony && Joao
>>>
>>

Re: [pgadmin4][patch] Use ESLinter options instead of using Javascript

[Anthony Emengo]

Hey all, we'd definitely like some feedback on this patch. Does does it not
apply cleanly? Do we need to resend it?

Thanks
Anthony

On Mon, May 14, 2018 at 3:47 PM Anthony Emengo  wrote:

> Apologies, here's a redo that should apply cleanly on top of master
>
> Anthony && Joao
>
> On Mon, May 14, 2018 at 3:25 PM Anthony Emengo  wrote:
>
>> Hi Hackers,
>>
>> Attached you can find a patch that changes the way we are running ESLint
>> on the Javascript files. Instead of using Javascript we can use ESLint
>> parameters to archive the same goal.
>>
>> In the future this will also allow us to use the rest of the cli
>> parameters like (--fix and others...)
>>
>> Thanks
>> Anthony && Joao
>>
>

pgAdmin 4 commit: Fix a couple of find/replace errors.

[Dave Page]

Fix a couple of  find/replace errors.

Branch
--
master

Details
---
https://git.postgresql.org/gitweb?p=pgadmin4.git;a=commitdiff;h=bd356ff9fa442e244ece107d56e8e3bb2ed30116

Modified Files
--
Make.bat | 21 ++---
1 file changed, 10 insertions(+), 11 deletions(-)

pgAdmin 4 commit: Further cleanup of the environment setup

[Dave Page]

Further cleanup of the environment setup

Branch
--
master

Details
---
https://git.postgresql.org/gitweb?p=pgadmin4.git;a=commitdiff;h=765e0d03be71da07f504a3b41fb845d59a555f5c

Modified Files
--
Make.bat | 51 ++-
1 file changed, 26 insertions(+), 25 deletions(-)

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

[Anthony Emengo]

export function canCreate(pgBrowser, childOfCatalogType) {
  return canCreateObject.bind({
browser: pgBrowser,
childOfCatalogType: childOfCatalogType,
  });
}

With respect to the above code: this bind pattern looks good and seems like
the idiomatic way to handle this in JavaScript. On a lighter node, I don’t
even see the need for an additional method to wrap it. The invocation could
have easily been like canCreate: canCreateObject.bind({ browser: pgBrowser,
childOfCatalogType: childOfCatalogType }), I don’t feel too strongly here.

I renamed it as isValidTreeNodeData, because - we were using it in for
testing the tree data. Please suggest me the right place, and name.

We’re not sure; maybe after continued refactoring, we will come across more
generic functions. At that point we can revisit this and create a utils.js
file.

The original patch was separating them in different places, but - still
uses some of the functionalities directly from the tree, which was
happening because we have contextual menu.
To give a better solution, I can think of putting the menus related code
understand ‘sources/tree/menu’ directory.

We’re particularly worried because we’re trying to avoid the coupling that
we see in the code base today. We want to decouple *application state*
from *business
domain* logic as much as we can - because this makes the code much easier
to understand. We achieve lower coupling by have more suitable interfaces
to retrieve *application state* like: anyParent (the menu doesn’t care how
this happens). This is the direction that we’re trying to move towards, we
just don’t want the package structure to undermine that developer intent.

How about nodeMenu.isSupportedNode(…)?

Naming is one of the hardest problems in programming. I don’t feel too
strongly about this one. For now, let’s keep it as is

Thanks
Anthony && Victoria
​

pgAdmin 4 commit: Rough cleanup of the environment setup

[Dave Page]

Rough cleanup of the environment setup

Branch
--
master

Details
---
https://git.postgresql.org/gitweb?p=pgadmin4.git;a=commitdiff;h=9f8f1e89727ec93d9fcd5da4f953ae64898a78f1

Modified Files
--
Make.bat | 91 ++--
1 file changed, 49 insertions(+), 42 deletions(-)

Re: PATCH: pgagent connection string parsing

[Ashesh Vashi]

[Adding pgadmin-hackers list...]

Hi Team,

Using the mutex around the logging code too to avoid overlapping log
message.
Please review it, and let me know your concern.

Implementation details:
- Moved 'MutexLogger' class, which was used by the connection management
code only, in the include/misc.h
-  Use a static mutex for the non-windows system, and used 'MutexLogger'
instance for the better lock/unlock mechanism in the LogMessage(...)
function.

-- Thanks, Ashesh


On Wed, May 16, 2018 at 6:59 PM, Dave Page  wrote:

>
>
> On Wed, May 16, 2018 at 2:27 PM, Ashesh Vashi <
> ashesh.va...@enterprisedb.com> wrote:
>
>> On Tue, May 8, 2018 at 3:01 PM, Thomas Krennwallner <
>> tk+pg...@postsubmeta.net> wrote:
>>
>>> Hi,
>>>
>>> I've just found this suspicious log messages (I've added line numbers in
>>> the attachment pgagent.log):
>>>
>>>  8  Tue May 8 11:11:23 2018 DEBUG: Creating DB connection:
>>> service=xiserver56_tisdbadm cTue May o8nne ct_timeout11:=151: 23 201
>>> 8applica DEBtUion_nameG: S=pglaeegpeinngt.@.d.e
>>>  9  v_tisdevel dbname=dev_tisdevel
>>>
>>> Note how line 8 and 9 are actually two log messages, which should
>>> have been printed in two lines:
>>>
>>> Tue May 8 11:11:23 2018 DEBUG: Creating DB connection:
>>> service=xiserver56_tisdbadm connect_timeout=5
>>> application_name=pgagent@dev_tisdevel dbname=dev_tisdevel
>>> Tue May 8 11:11:23 2018 DEBUG: Sleeping...
>>>
>>> It appears that two threads are writing to stdout at the same time,
>>> which calls for an exclusive lock in function
>>> void LogMessage(const std::wstring , const int )
>>>
>> Dave,
>>
>> Shall we use a mutex here too?
>>
>>
> I would think so, yes.
>
> --
> Dave Page
> Blog: http://pgsnake.blogspot.com
> Twitter: @pgsnake
>
> EnterpriseDB UK: http://www.enterprisedb.com
> The Enterprise PostgreSQL Company
>


pgagent_logger_mutex.patch
Description: Binary data

Re: [pgAdmin4][Patch] Feature #3204 Notify/Listen not working in version 2.1

[Anthony Emengo]

Hey,

The code looks great! The tests all passed as well.

Sincerely

Anthony and Victoria

Re: Diffs as images

[Anthony Emengo]

Understood. Thanks for bringing this to our attention.

On Wed, May 16, 2018 at 6:52 AM Dave Page  wrote:

> All,
>
> Please don't send images to show code changes. It's unnecessary when you
> could just send a diff file, and just increases the amount of bandwidth and
> storage used in peoples mailboxes and the archives, but worse, can eat into
> bandwidth allowances for those using mobile devices, particularly annoying
> for folks in countries where data isn't cheap.
>
> Thanks.
>
> --
> Dave Page
> Blog: http://pgsnake.blogspot.com
> Twitter: @pgsnake
>
> EnterpriseDB UK: http://www.enterprisedb.com
> The Enterprise PostgreSQL Company
>

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

pgAdmin 4 commit: PYTHON_VERSION is set automatically. Use it to condit

[Dave Page]

PYTHON_VERSION is set automatically. Use it to conditionally fix the backports 
module.

Branch
--
master

Details
---
https://git.postgresql.org/gitweb?p=pgadmin4.git;a=commitdiff;h=ae03b7c226188f347dfcc9346cc37b50cb965094

Modified Files
--
Make.bat | 9 +++--
1 file changed, 3 insertions(+), 6 deletions(-)

pgAgent commit: Removed the third party library wxWidgets dependency. Y

[Ashesh Vashi]

Removed the third party library wxWidgets dependency. Yay :-)

Using boost instead of wxWidgets for several cross platform functionalities.

Reviewed by: Akshay Joshi & Ashesh Vashi

Branch
--
master

Details
---
https://git.postgresql.org/gitweb?p=pgagent.git;a=commitdiff;h=1573bcc8333d26d229e6ef0246753a7855707279
Author: Neel Patel 

Modified Files
--
.gitignore|   10 +
CMakeLists.txt|  418 -
README|   99 ++--
cmake/FindBoost.cmake | 1214 +
cmake/FindWX.cmake|  323 -
cmake/MakeExt.cmake   |   36 +-
connection.cpp|  444 +-
include/connection.h  |  158 ---
include/job.h |   27 +-
include/misc.h|   10 +-
include/pgAgent.h |   37 +-
job.cpp   |  344 +++---
misc.cpp  |  132 +-
pgAgent.cpp   |  198 
unix.cpp  |  120 +++--
win32.cpp |  251 +-
16 files changed, 2468 insertions(+), 1353 deletions(-)

pgAgent commit: Refactored the connInfo and DBconn and use of PQconninf

[Ashesh Vashi]

Refactored the connInfo and DBconn and use of PQconninfoParse() for
parsing the connection string replacing our own version parsing logic.

Reviewed by Ashesh, also modified it to take care of the removal of
wxWidgets dependency change.

Branch
--
master

Details
---
https://git.postgresql.org/gitweb?p=pgagent.git;a=commitdiff;h=140027c9f2c3fe4174a59f1fce5b95cb808e
Author: Thomas Krennwallner 

Modified Files
--
connection.cpp   | 450 +--
include/connection.h | 113 ++---
include/pgAgent.h|   1 -
job.cpp  |   4 +-
pgAgent.cpp  |   5 +-
5 files changed, 272 insertions(+), 301 deletions(-)

pgAdmin 4 commit: No need to define PGADMIN4_USE_WEBKIT any more.

[Dave Page]

No need to define PGADMIN4_USE_WEBKIT any more.

Branch
--
master

Details
---
https://git.postgresql.org/gitweb?p=pgadmin4.git;a=commitdiff;h=246c434b61830d94744bca37fbf89b64fa120027

Modified Files
--
Make.bat | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)

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

[Aditya Toshniwal]

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"
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 8e5ca23..3c10eaa 100644
--- a/web/pgadmin/browser/server_groups/servers/static/js/server.js
+++ b/web/pgadmin/browser/server_groups/servers/static/js/server.js
@@ -676,7 +676,7 @@ define('pgadmin.node.server', [
   mode: ['properties', 'edit', 'create'], disabled: 'isConnected',
 },{
   id: 'port', label: gettext('Port'), type: 'int', group: gettext('Connection'),
-  mode: ['properties', 'edit', 'create'], disabled: 'isConnected', min: 1024, max: 65535,
+  mode: ['properties', 'edit', 'create'], disabled: 'isConnected', min: 1, max: 65535,
 },{
   id: 'db', label: gettext('Maintenance database'), type: 'text', group: gettext('Connection'),
   mode: ['properties', 'edit', 'create'], disabled: 'isConnected',

pgAdmin 4 commit: Assume Yarn and Node are always in the path

[Dave Page]

Assume Yarn and Node are always in the path

Branch
--
master

Details
---
https://git.postgresql.org/gitweb?p=pgadmin4.git;a=commitdiff;h=4496a97a975f98ca125e7e1f26debbf36afd7419

Modified Files
--
Make.bat | 18 +-
1 file changed, 1 insertion(+), 17 deletions(-)

pgAdmin 4 commit: Use VCINSTALLDIR instead of VCDIR, as that is set by

[Dave Page]

Use VCINSTALLDIR instead of VCDIR, as that is set by Visual Studio

Branch
--
master

Details
---
https://git.postgresql.org/gitweb?p=pgadmin4.git;a=commitdiff;h=7a1d9c750a2662d9cc693268f481babdf9f8b450

Modified Files
--
Make.bat | 14 +++---
1 file changed, 7 insertions(+), 7 deletions(-)

Diffs as images

[Dave Page]

All,

Please don't send images to show code changes. It's unnecessary when you
could just send a diff file, and just increases the amount of bandwidth and
storage used in peoples mailboxes and the archives, but worse, can eat into
bandwidth allowances for those using mobile devices, particularly annoying
for folks in countries where data isn't cheap.

Thanks.

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

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

Re: [pgAdmin4][Patch] Feature #3204 Notify/Listen not working in version 2.1

[Akshay Joshi]

Hi Hackers,

On Tue, May 15, 2018 at 9:52 PM, Akshay Joshi  wrote:

> Hi Joao
>
> On Tue, 15 May 2018, 19:36 Joao De Almeida Pereira, <
> jdealmeidapere...@pivotal.io> wrote:
>
>> Hi Akshay,
>>
>> This patch is flaky; it doesn't always pass the tests in our pipeline.
>>
>> ==
>>  
>> 
>> ERROR: runTest (pgadmin.feature_tests.query_tool_tests.QueryToolFeatureTest)
>>  
>> 
>> Query tool feature test
>>  
>> 
>> --
>>  
>> 
>> Traceback (most recent call last):
>>  
>> 
>>   File 
>> "/tmp/build/5988fb0a/pgadmin-repo/web/pgadmin/feature_tests/query_tool_tests.py",
>>  line 101, in runTest
>>  
>> 
>> self._query_tool_notify_statements()
>>  
>> 
>>   File 
>> "/tmp/build/5988fb0a/pgadmin-repo/web/pgadmin/feature_tests/query_tool_tests.py",
>>  line 643, in _query_tool_notify_statements
>>  
>> 
>> '//div[contains(@class, "sql-editor-message") and '
>>  
>> 
>>   File 
>> "/tmp/build/5988fb0a/pgadmin-repo/web/regression/feature_utils/pgadmin_page.py",
>>  line 169, in find_by_xpath
>>  
>> 
>> lambda driver: driver.find_element_by_xpath(xpath)
>>  
>> 
>>   File 
>> "/tmp/build/5988fb0a/pgadmin-repo/web/regression/feature_utils/pgadmin_page.py",
>>  line 261, in wait_for_element
>>  
>> 
>> return self._wait_for("element to exist", element_if_it_exists)
>>  
>> 
>>   File 
>> "/tmp/build/5988fb0a/pgadmin-repo/web/regression/feature_utils/pgadmin_page.py",
>>  line 327, in _wait_for
>>  
>> 
>> "Timed out waiting for " + waiting_for_message
>>  
>> 
>>   File 
>> "/root/.pyenv/versions/pgadmin/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
>>
>>
>> All the failures are related to query_tool_notify_statements.  Please
>> take another look.
>>
>
>  There is some serious problems of timeout issues. I have tested it
> couple of times and working fine. Will test tomorrow one more time. I have
> followed the code written in the same file. If those test cases have passed
> then this should also.
>

  Issue is with Python 2.7, yesterday i have verified it on Python 3.5.
Attached is the modified patch, please review it.

>
>> Sincerely,
>>
>> Victoria & Joao
>>
>> On Tue, May 15, 2018 at 6:01 AM Akshay Joshi <
>> akshay.jo...@enterprisedb.com> wrote:
>>
>>> Hi Hackers,
>>>
>>> Attached is the patch to capture the notification from psycopg2 and
>>> displayed it in "Messages" tab of query tool. Added feature test to cover
>>> this scenario.
>>>
>>> Refer Notification.png file to how it looks in "Messages" tab. Please
>>> review it.
>>>
>>> --
>>> *Akshay Joshi*
>>>
>>> *Sr. Software Architect *
>>>
>>>
>>>
>>> *Phone: +91 20-3058-9517Mobile: +91 976-788-8246*
>>>
>>


-- 
*Akshay Joshi*

*Sr. Software Architect *



*Phone: +91 

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

[Ashesh Vashi]

Hi Joao,

On Tue, May 15, 2018 at 8:33 PM, Joao De Almeida Pereira <
jdealmeidapere...@pivotal.io> wrote:

> Hello Ashesh,
>
> These are our comments to the patch:
>
>
>-
>
>Can you explain the reasoning behind the change you did on the
>canCreate function?
>
> I do agree as Akshay mentioned ealier, canCreate.canCreate does not look
right to me too, and neither directory structure, nor name of the script
does not make sense to me.
As those functions are always going to apply on the tree nodes only, moved
them under the 'sources/tree' directory, and renamed it as 'node_menu'.

>
>-
>
>Bind creates a new instance of a function, do we really need that?
>
> If you look at the original proposed patch, the implementation looks like
this.


*canCreate: function(itemData, item, data) {*
*   return canCreate.canCreate(pgBrowser, , item,
data);*
*}*

Here - we're already creating an anonymous function, which is an object
under the hood.
And - for every tree node, they will be a separate anonymous function
anyway.

Why not use better object oriented approach?

>
>-
>
>isValidTreeNodeData
>1. If it is not Null or Undefined it really means that the data is
>   Valid? Shouldn’t it be isDataDefined?
>   2. This looks like a generic function that could be used with
>   objects of any type not specific to TreeNodeData. So the file location
>   doesn’t look correct.
>
> I renamed it as isValidTreeNodeData, because - we were using it in for
testing the tree data.
I do agree - it is a generic function that could be used with objects of
any type.

But - when I moved the code, the function name was
'isProvidedDataValid(...)', which was present in
'sources/menu/menu_enabled' file, which was definitely not right place. :-)
Please suggest me the right place, and name.

>
>- The tree folder is just a Tree that we use to store information. The
>menu uses a Tree but the 2 things should be separated.
>
> I think - you're missing the point.

Here - we're dealing with two types of menus:
1. Contextual menu (which will always depend on the current selected tree
node)
2. Normal menus

>
>-
>
>In our point of view the current entanglement of the ACITree into our
>code came from missing concepts into a single place (Menu + Storage of
>information).
>The idea behind having the Tree as a separate block was to ensure that
>we do not have the Menu and Tree coupling.
>
> In my opinion - keeping them in them in different directories/files does
not make them decoupled to be honest.

The original patch was separating them in different places, but - still
uses some of the functionalities directly from the tree, which was
happening because we have contextual menu.
To give a better solution, I can think of putting the menus related code
understand 'sources/tree/menu' directory.
That will give clear distiguation between actual tree, and dependent code.

What do you say?

>
>-
>
>supportedNodesMenu.enabled what it does it check if a Node Menu should
>be enabled or not. The name of it maybe should be nodeMenu.enabled?
>
> nodeMenu.enabled(...) looks to general to me.
How about nodeMenu.isSupportedNode(...)?

If you look at the implementation of the function, it checks for the
current tree node is one of the supported nodes, or not.

-- Thanks, Ashesh

> ​
>
>
> Thanks
> Victoria & Joao
>
> On Tue, May 15, 2018 at 6:37 AM Ashesh Vashi <
> ashesh.va...@enterprisedb.com> wrote:
>
>> Hi Joao,
>> On Mon, May 14, 2018 at 6:11 PM, Ashesh Vashi <
>> ashesh.va...@enterprisedb.com> wrote:
>>
>>>
>>> On Mon, May 14, 2018 at 6:10 PM, Dave Page  wrote:
>>>


 On Mon, May 14, 2018 at 1:38 PM, Ashesh Vashi <
 ashesh.va...@enterprisedb.com> wrote:

> On Mon, May 14, 2018 at 2:59 PM, Dave Page  wrote:
>
>>
>>
>> On Sat, May 12, 2018 at 12:10 AM, Ashesh Vashi <
>> ashesh.va...@enterprisedb.com> wrote:
>>
>>> On Sat, May 12, 2018, 02:58 Joao De Almeida Pereira <
>>> jdealmeidapere...@pivotal.io> wrote:
>>>
 Hello Ashesh,

 1. In TreeNode, we're keepging the reference of DOMElement, do we
>> really need it?
>
> As of right now, our Tree abstraction acts as an adapter to the
>> aciTree library. The aciTree library needs the domElement for most 
>> of its
>> functions (setInode, unload, etc). Thus this is the easiest way to
>> introduce our abstraction and keep the functionality as before - at 
>> least
>> until we decide that whether we want to switch out the library or 
>> not.
>
> I understand that. But - I've not seen any reference of domElement
> the code yet, hence - pointed that out.

 If you look at the function: reload, unload you will see that
 domNode is used to communicate with the ACITree
 ​

> 2.