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

[Anthony Emengo]

Hey Akshay

This patch passed our test pipelines.

Anthony and Victoria

On Thu, Apr 19, 2018 at 1:48 AM, Akshay Joshi  wrote:

> Hi Hackers
>
> I have implemented the SSH Tunnel support using https://pypi.org/
> project/sshtunnel/ python package. Added "SSH Tunnel" Tab in server
> dialog. This implementation supports user name /password and private/public
> key combination with Passphrase to crate SSH Tunnel. I have added
> regression test case to add server using SSH Tunnel options.
>
> The given python package(https://pypi.org/project/sshtunnel/) support
> Python version *2.7, 3.4+*.
> It uses Paramiko (Python implementation of SSHv2 protocol) which actually
> drops support for Python 2.6. So I have added *SUPPORT_SSH_TUNNEL* parameter
> in config.py which checks the python version and set the flag accordingly.
> In case of Python 2.6, 3.0, 3.1, 3.2 and 3.3 control on the "SSH Tunnel"
> tab of server dialog will be disabled.
>
> Please review it, and if looks good please commit the code.
>
> --
> *Akshay Joshi*
>
> *Sr. Software Architect *
>
>
>
> *Phone: +91 20-3058-9517Mobile: +91 976-788-8246*
>

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

[Anthony Emengo]

For what it is worth, I manually verified that the feature worked, as well
as looked through the code.

I'd like to see end-to-end testing for regression sake, but it's hard to so
at this moment.

- Anthony and Joao.

On Mon, Apr 23, 2018 at 5:09 AM, Akshay Joshi  wrote:

>
>
> On Mon, Apr 23, 2018 at 1:30 PM, Dave Page  wrote:
>
>> Hi
>>
>> On Thu, Apr 19, 2018 at 6:56 PM, Anthony Emengo 
>> wrote:
>>
>>> Hey Akshay
>>>
>>> This patch passed our test pipelines.
>>>
>>
>> Did you test the feature and//or review the code and tests? Passing the
>> tests is great, *if* the whole feature is covered (and the nature of this
>> patch will make that quite difficult, maybe impossible to do without
>> external infrastructure and config).
>>
>
> Agreed, it's been difficult to write test case to test the complete
> feature.
>
>>
>>
>>>
>>> Anthony and Victoria
>>>
>>> On Thu, Apr 19, 2018 at 1:48 AM, Akshay Joshi <
>>> akshay.jo...@enterprisedb.com> wrote:
>>>
>>>> Hi Hackers
>>>>
>>>> I have implemented the SSH Tunnel support using
>>>> https://pypi.org/project/sshtunnel/ python package. Added "SSH Tunnel"
>>>> Tab in server dialog. This implementation supports user name /password and
>>>> private/public key combination with Passphrase to crate SSH Tunnel. I have
>>>> added regression test case to add server using SSH Tunnel options.
>>>>
>>>> The given python package(https://pypi.org/project/sshtunnel/) support
>>>> Python version *2.7, 3.4+*.
>>>> It uses Paramiko (Python implementation of SSHv2 protocol) which
>>>> actually drops support for Python 2.6. So I have added
>>>> *SUPPORT_SSH_TUNNEL* parameter in config.py which checks the python
>>>> version and set the flag accordingly. In case of Python 2.6, 3.0, 3.1, 3.2
>>>> and 3.3 control on the "SSH Tunnel" tab of server dialog will be disabled.
>>>>
>>>> Please review it, and if looks good please commit the code.
>>>>
>>>> --
>>>> *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
>>
>
>
>
> --
> *Akshay Joshi*
>
> *Sr. Software Architect *
>
>
>
> *Phone: +91 20-3058-9517Mobile: +91 976-788-8246*
>

Re: [PATCH] [RM# 3290] Close button missing for the message window from the backend server

[Anthony Emengo]

Just manually verified that this worked, and the patch successfully went
through our pipelines.

Thanks so much for fixing this issue!

- Joao and Anthony

On Mon, Apr 23, 2018 at 3:40 AM, Ashesh Vashi  wrote:

> Hi Team,
>
> Please find the patch for fixing the RM #3290.
> The 'Close button' was missing in the message box, which shows the error
> in the backend server.
>
> This patch also fixes some of the missing keycode 'ESCAPE' on 'Cancel'
> buttons in different modules.
>
> Please review.
>
> --
>
> Thanks & Regards,
>
> Ashesh Vashi
> EnterpriseDB INDIA: Enterprise PostgreSQL Company
> 
>
>
> *http://www.linkedin.com/in/asheshvashi*
> 
>

Re: [pgAdmin4][Patch] Feature #3270 Add support for running regression tests against Firefox

[Anthony Emengo]

We also tried running the tests with this patch. It didn't launch without
some code changes and several tests were failing. We should really defer
pulling this in until we have more robust results on Firefox

In order to have the tests running we had to do the following change:

diff --git a/web/regression/feature_utils/app_starter.py
b/web/regression/feature_utils/app_starter.py
index 77b0400c..50d3e307 100644
--- a/web/regression/feature_utils/app_starter.py
+++ b/web/regression/feature_utils/app_starter.py
@@ -42,10 +44,18 @@ class AppStarter:
 )

 self.driver.set_window_size(1280, 1024)
-self.driver.get(
-"http://"; + self.app_config.DEFAULT_SERVER + ":" +
-random_server_port
-)
+# self.driver.implicitly_wait(60)
+
+def launch_browser():
+try:
+self.driver.get(
+"http://"; + self.app_config.DEFAULT_SERVER + ":" +
+random_server_port
+)
+except WebDriverException as e:
+time.sleep(5)
+launch_browser()
+launch_browser()

This change was required because firefox was throwing an exception when we
tried to get the address and the server was not running. We saw this
behavior in Ubuntu.


- Anthony and Joao

On Mon, Apr 23, 2018 at 9:12 AM, Dave Page  wrote:

> Hi
>
> On Mon, Apr 23, 2018 at 2:05 PM, Akshay Joshi <
> akshay.jo...@enterprisedb.com> wrote:
>
>> Hi Hackers,
>>
>> I have added support for running feature tests against FireFox. For that
>> user will have to download gecko driver from https://github.com/mozilla/
>> geckodriver/releases and follow the below steps:
>>
>>- Extract the gecko driver.
>>- Run chmod +x geckodriver.
>>- Either copy the geckodriver to /usr/local/bin or the path of the
>>geckodriver must be specified in PATH.
>>- Apply the attached patch.
>>- Change the parameter "DEFAULT_TEST_BROWSER = 'Firefox' "
>>- Start the feature test.
>>
>> The config option needs to be in test_config.json. We don't really want
> test-specific config options in config.py (arguably, TEST_SQLITE_PATH
> shoudn't be there either, as it's useless for end users).
>
> I'm surprised to not see any updates to tests. When I tried firefox
> manually a few weeks back, it was failing at lot.
>
> --
> Dave Page
> Blog: http://pgsnake.blogspot.com
> Twitter: @pgsnake
>
> EnterpriseDB UK: http://www.enterprisedb.com
> The Enterprise PostgreSQL Company
>

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

[Anthony Emengo]

Hi there,

Yes, there's a lot of TODO that we intend for this effort - to be
submitted. We'd like to remove as much *direct* invocations on the ACI Tree
library, as it causes a lot of coupling to the library. It is not the final
patch, but we cannot come up with a definitive list of the things we intend
to do, at this time.

On Mon, Apr 30, 2018 at 2:16 AM, Ashesh Vashi  wrote:

>
>
> On Sat, Apr 28, 2018 at 3:55 AM, Joao De Almeida Pereira <
> jdealmeidapere...@pivotal.io> wrote:
>
>> Hi Hackers,
>> As you are aware we kept on working on the patch, so we are attaching to
>> this email a new version of the patch.
>> This new version contains all the changes in the previous one plus more
>> extractions of functions and refactoring of code.
>>
>> The objective of this patch is to create a separation between pgAdmin and
>> the ACI Tree. We are doing this because we realized that at this point in
>> time we have the ACI Tree all over the code of pgAdmin. I found a very
>> interesting article that really talks about this:
>> https://medium.freecodecamp.org/code-dependencies-are-the-de
>> vil-35ed28b556d
>>
>> In this patch there are some visions and ideas about the location of the
>> code, the way to organize it and also try to pave the future for a
>> application that is stable, easy to develop on and that can be release at a
>> times notice.
>>
>> We are investing a big chunk of our time in doing this refactoring, but
>> while doing that we also try to respond to the patches sent to the mailing
>> list. We would like the feedback from the community because we believe this
>> is a changing point for the application. The idea is to change the way we
>> develop this application, instead of only correcting a bug of developing a
>> feature, with every commit we should correct the bug or develop a feature
>> but leave the code a little better than we found it (Refactoring,
>> refactoring, refactoring). This is hard work but that is what the users
>> from pgAdmin expect from this community of developers.
>>
>>
>> ==
>>
>>
>>
>> It is a huge patch
>>   86 files changed, 5492 inserts, 1840 deletions
>> and we would like to get your feedback as soon as possible, because we
>> are continuing to work on it which means it is going to grow in size.
>>
>>
>> At this point in time we still have 124 of 176 calls to the function
>> itemData from ACITree.
>>
>> What does each patch contain:
>> 0001:
>>   Very simple patch, we found out that the linter was not looking into
>> all the javascript test files, so this patch will ensure it is
>>
>> 0002:
>>   New Tree abstraction. This patch contains the new Tree that works as an
>> adaptor for ACI Tree and is going to be used on all the extractions that we
>> are doing
>>
>> 0003:
>>   Code that extracts, wrap with tests and replace ACI Tree invocations.
>>   We start creating new pattern for the location of Javascript files and
>> their structure.
>>   Create patterns for creation of dialogs (backup and restore)
>>
>
> Do you have some TODO left for the same?
> Or, is this the final one? Because - it gives us the better understanding
> during reviewing the patch.
>
> -- Thanks, Ashesh
>
>>
>>
>> Thanks
>> Joao
>>
>>
>> On Fri, Apr 27, 2018 at 5:34 AM Ashesh Vashi <
>> ashesh.va...@enterprisedb.com> wrote:
>>
>>> I have quite a few comments for the patch.
>>> I will send them soon.
>>>
>>> On Fri, Apr 27, 2018, 14:45 Dave Page  wrote:
>>>
 How is your work on this going Ashesh? Will you be committing today?

 On Wed, Apr 25, 2018 at 8:52 AM, Dave Page  wrote:

> Ashesh; you had agreed to work on this early this week. Please ensure
> you do so today.
>
> Thanks.
>
> On Tue, Apr 24, 2018 at 8:13 PM, Joao De Almeida Pereira <
> jdealmeidapere...@pivotal.io> wrote:
>
>> Hi Hackers,
>>
>> Can someone review and merge this patch?
>>
>> Thanks
>> Joao
>>
>> On Wed, Apr 18, 2018 at 10:23 AM Joao De Almeida Pereira <
>> jdealmeidapere...@pivotal.io> wrote:
>>
>>> Hi Hackers,
>>> Any other comment about this patch?
>>>
>>> Thanks
>>> Joao
>>>
>>> On Tue, Apr 10, 2018 at 12:00 PM Joao De Almeida Pereira <
>>> jdealmeidapere...@pivotal.io> wrote:
>>>
 Hello Khushboo

 On Mon, Apr 9, 2018 at 1:59 AM Khushboo Vashi <
 khushboo.va...@enterprisedb.com> wrote:

> Hi Joao,
>
> I have reviewed your patch and have some suggestions.
>
> On Sat, Apr 7, 2018 at 12:43 AM, Joao De Almeida Pereira <
> jdealmeidapere...@pivotal.io> wrote:
>
>> Hello Murtuza/Dave,
>> Yes now the extracted functions are spread into different files.
>> The intent would be to make the files as small as possible, and also 
>> to
>> group and name them in a way that would be easy to understand what 
>> each
>> file is doing without t

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

[Anthony Emengo]

>
> I was expecting a separate layer between the tree implementation, and
> aciTree adaptor.
> Please find the patch for the example.
> It will separate the two layers, and easy to replace with the new
> implementation in future.


In general, we want defer the separation of the layers for now. Even though
we might assume that this is the direction we want to go in. It's simply
too early to be making such an architectural leap. For right now, we just
know that we need the decoupling, but don't know what if we'd need the 2
layers *as implemented*. The principle we're adhering to here is the Last
Responsible Moment principle, which states that you only make the changes
that you feel is necessary for the given problems you're facing:
https://blog.codinghorror.com/the-last-responsible-moment/

I would not like to see that changes in this patch.
> I would like us to come up with the actual design about the hot
> pluggability before going in this direction.


In our point of view these 2 changes are not related. One thing is the
internal code organization of the application, other thing is allowing
third party to drop code in the application and it just work. These 2
should be talked separately, but the hot pluggability is not something that
will be address by this work we are doing right now.

Anthony && Joao

On Mon, Apr 30, 2018 at 11:03 AM, Ashesh Vashi <
ashesh.va...@enterprisedb.com> wrote:

>
>
>
> On Mon, Apr 30, 2018 at 8:30 PM, Ashesh Vashi <
> ashesh.va...@enterprisedb.com> wrote:
>
>> On Sat, Apr 28, 2018 at 3:55 AM, Joao De Almeida Pereira <
>> jdealmeidapere...@pivotal.io> wrote:
>>
>>> Hi Hackers,
>>> As you are aware we kept on working on the patch, so we are attaching to
>>> this email a new version of the patch.
>>> This new version contains all the changes in the previous one plus more
>>> extractions of functions and refactoring of code.
>>>
>>> The objective of this patch is to create a separation between pgAdmin
>>> and the ACI Tree. We are doing this because we realized that at this point
>>> in time we have the ACI Tree all over the code of pgAdmin. I found a very
>>> interesting article that really talks about this:
>>> https://medium.freecodecamp.org/code-dependencies-are-the-de
>>> vil-35ed28b556d
>>>
>>> In this patch there are some visions and ideas about the location of the
>>> code, the way to organize it and also try to pave the future for a
>>> application that is stable, easy to develop on and that can be release at a
>>> times notice.
>>>
>>> We are investing a big chunk of our time in doing this refactoring, but
>>> while doing that we also try to respond to the patches sent to the mailing
>>> list. We would like the feedback from the community because we believe this
>>> is a changing point for the application. The idea is to change the way we
>>> develop this application, instead of only correcting a bug of developing a
>>> feature, with every commit we should correct the bug or develop a feature
>>> but leave the code a little better than we found it (Refactoring,
>>> refactoring, refactoring). This is hard work but that is what the users
>>> from pgAdmin expect from this community of developers.
>>>
>>>
>>> ==
>>>
>>>
>>>
>>> It is a huge patch
>>>   86 files changed, 5492 inserts, 1840 deletions
>>> and we would like to get your feedback as soon as possible, because we
>>> are continuing to work on it which means it is going to grow in size.
>>>
>>>
>>> At this point in time we still have 124 of 176 calls to the function
>>> itemData from ACITree.
>>>
>>> What does each patch contain:
>>> 0001:
>>>   Very simple patch, we found out that the linter was not looking into
>>> all the javascript test files, so this patch will ensure it is
>>>
>> Committed the patch along with the regression introduced because of this
>> patch.
>>
>>>
>>> 0002:
>>>   New Tree abstraction. This patch contains the new Tree that works as
>>> an adaptor for ACI Tree and is going to be used on all the extractions that
>>> we are doing.
>>>
>>
>> I was expecting a separate layer between the tree implementation, and
>> aciTree adaptor.
>> Please find the patch for the example.
>>
>> It will separate the two layers, and easy to replace with the new
>> implemenation in future.
>>
>
> Oops forgot to attach the patch.
> Please find the patch attached.
>
> -- Thanks, Ashesh
>
>>
>>> 0003:
>>>   Code that extracts, wrap with tests and replace ACI Tree invocations.
>>>
>> There are many small cases left in the patches.
>> Hence - I would like to know the TODO list created by you.
>>
>> e.g. When we remove any of the object from the database server, we're not
>> yet removing the respective node from the new implementation, and its
>> children.
>>
>>>
>>>   We start creating new pattern for the location of Javascript files and
>>> their structure.
>>>
>> I would not like to see that changes in this patch.
>> I would like us to come up with the actual design about the hot
>> pluggabi

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

[Anthony Emengo]

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
​


0001-fix-external-tables-on-windows.patch
Description: Binary data

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

[Anthony Emengo]

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


0001-fix-external-tables-on-windows-2.patch
Description: Binary data

Re: [pgAdmin4][Patch] Feature #3270 Add support for running regression tests against Firefox

[Anthony Emengo]

Hey Akshay,

Regarding your previous comment:

As I know time.sleep is not a good idea and I am new to feature test and in
> learning phase, can someone correct/suggest the way to fix those issues.

Although the feature tests already have some sleeps, it’s generally not a
best practice for browser automation. The better way is to use Waits that
the selenium API provides, mainly because they can accept a condition to
poll until fulfilled for rather than blocking the main thread of execution.
http://selenium-python.readthedocs.io/waits.html#explicit-waits. With
regards to the  "fe_sendauth" password error . I’d probably would have done
it like so (I haven’t tested this):

WebDriverWait(self.driver, 10)\
.until(EC.text_to_be_present_in_element(
(By.NAME, 'password', server_config['db_password']))
)

In general, I ran the tests with the Firefox option and am seeing more
failures than I usually would with Chrome. I don’t feel particularly
strongly about the patch since our team mostly likely won’t be running it
over the course of our development. But I still couldn’t *run* the tests
without modify the code so that the server is up and running before the
browser automation starts. I think that this should be addressed.

Any thing in particular that I can help with regarding this, please let me
know! 😀

Anthony
​

[pgadmin][patch] Fix regression tests failures on Windows

[Anthony Emengo]

Hey hackers,

Attached is a patch that accommodates the recent `os.path` implementation
changes for Windows. The tests were previously working on Unix but not yet
Windows

Joao && Anthony


0001-Fix-Windows-Tests.patch
Description: Binary data

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

[Anthony Emengo]

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


0001-Use-Eslinter-Options-Instead-Of-Javascript.patch
Description: Binary data

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

[Anthony Emengo]

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
>


0001-Use-Eslinter-Options-Instead-Of-Javascript.patch
Description: Binary data

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][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

[pgadmin] Prefer CSS selectors to XPath selectors in feature tests

[Anthony Emengo]

Hey all,

We've noticed that our feature tests make extensive use of xpath with the
selenium api. It's well documented that css selectors are easier to read
and more efficient. I think we should start to favor these if we have not
considered so already.

http://bugsareinthegaps.com/what-is-the-best-by-selector-for-selenium-css-or-xpath/

Thanks,
Anthony and Victoria

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
​

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

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

[Anthony Emengo]

Hey all,

We haven't heard from you all in a while regarding our last statements. Is
there any thing that I need to clarify? We feel left in the dark here and
just want to know what we can do help.

Cheers!
Anthony && Joao

Re: [pgadmin4][patch] Use pytest test runner for unit tests

[Anthony Emengo]

Here’s a followup patch with the relevant README and Makefile changes. To
be clear, both patches need to be applied in succession to run the tests.

The error that you were running into was because the appropriate PYTHONPATH
environment variable was not set. We updated the README to reflect that,
but haven’t done anything to the code for that

Thanks
Joao && Anthony
​


0002-Use-Pytests.diff.ci-skip
Description: Binary data

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

[Anthony Emengo]

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 origin