Received: from malur.postgresql.org ([2a02:16a8:dc51::56]) by arkaria.postgresql.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_CBC_SHA384:256) (Exim 4.89) (envelope-from ) id 1fOpYa-0005bH-Ke for pgadmin-hackers@arkaria.postgresql.org; Fri, 01 Jun 2018 19:17:49 +0000 Received: from localhost ([127.0.0.1] helo=malur.postgresql.org) by malur.postgresql.org with esmtp (Exim 4.89) (envelope-from ) id 1fOpYZ-0006Vo-CV for pgadmin-hackers@arkaria.postgresql.org; Fri, 01 Jun 2018 19:17:47 +0000 Received: from magus.postgresql.org ([2a02:c0:301:0:ffff::29]) by malur.postgresql.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_CBC_SHA384:256) (Exim 4.89) (envelope-from ) id 1fOpYZ-0006Ve-3A for pgadmin-hackers@lists.postgresql.org; Fri, 01 Jun 2018 19:17:47 +0000 Received: from mx0a-00296801.pphosted.com ([148.163.150.38]) by magus.postgresql.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_CBC_SHA384:256) (Exim 4.89) (envelope-from ) id 1fOpYS-0006CK-8G for pgadmin-hackers@postgresql.org; Fri, 01 Jun 2018 19:17:45 +0000 Received: from pps.filterd (m0114583.ppops.net [127.0.0.1]) by mx0a-00296801.pphosted.com (8.16.0.22/8.16.0.22) with SMTP id w51JFhvv000652 for ; Fri, 1 Jun 2018 19:17:36 GMT Received: from mail-io0-f199.google.com (mail-io0-f199.google.com [209.85.223.199]) by mx0a-00296801.pphosted.com with ESMTP id 2j9euutsjm-1 (version=TLSv1.2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128 verify=NOT) for ; Fri, 01 Jun 2018 19:17:36 +0000 Received: by mail-io0-f199.google.com with SMTP id n24-v6so19945786ioh.12 for ; Fri, 01 Jun 2018 12:17:35 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:references:in-reply-to:from:date :message-id:subject:to:cc; bh=BHrxRdaQlJHnLAGCxTamu56pw4Du0woFcp6eIga3640=; b=rZvUweu9XqadQzzvbDpCsSnI+YdMIa9IXiVIJNdPy10+Shc7+FBcunWm4ve8bLfnmw V2EuyA8o5dXPX6AS6BgmACEuWg/Ghkv6z+i2bCbNpuBWsYAxSoYuc7QYlNWqiKvr+DAT KbHMN/znnUy57Sg4dWMcMwgcwe0Tod2aW6C+qkeoanGlqa4WJnoiOnABP34vJjcK2MwB wR9zpBB99ErJX94f9kDf/7JA/8uxAiLY7TzWr3TiLA1zeFhCTeKC79ftgQ+N5Pjk2T7+ ah6mSoQA/yxCacs5jUSiCLmzkEYYxAHCBlpUh4ZQlV+QtsOL0VrO6obYbeg9U0ux9yVk HVBA== X-Gm-Message-State: APt69E23dAPUWFbmRaALwbLCpFee/Pv0kizX3RB3+pQ80zBdU5mfbgHM HIZYOnpI75jLIlzBF4nSXOtfeb42MUKxDFG2ZQVJiL3Z1SnWmiyixQ+7ld03C3O5Lwf1lj1iA45 GAFh3gT994w+bgujLyCX8kwuWGnpJM5DtLXpvSXxz01n/D2xWYeOzqiiy31vXfqsaJvMx X-Received: by 2002:a24:6d86:: with SMTP id m128-v6mr5809156itc.75.1527880654768; Fri, 01 Jun 2018 12:17:34 -0700 (PDT) X-Google-Smtp-Source: ADUXVKKAppGGiZUQR8FJ3JSxvs9KsJibMPnR6EYMYkgHwOQG3RbwjtTB5kmZnnFBvX8X0Qq6zHR8eZGToZRONS1U4E4= X-Received: by 2002:a24:6d86:: with SMTP id m128-v6mr5809122itc.75.1527880654288; Fri, 01 Jun 2018 12:17:34 -0700 (PDT) MIME-Version: 1.0 References: In-Reply-To: From: Victoria Henry Date: Fri, 1 Jun 2018 15:17:23 -0400 Message-ID: Subject: Re: [pgadmin4][patch] Initial patch to decouple from ACI Tree To: Ashesh Vashi Cc: Anthony Emengo , Joao De Almeida Pereira , Dave Page , Akshay Joshi , Murtuza Zabuawala , pgadmin-hackers , Khushboo Vashi Content-Type: multipart/alternative; boundary="000000000000a897e0056d996fbc" X-Proofpoint-Virus-Version: vendor=fsecure engine=2.50.10434:,, definitions=2018-06-01_12:,, signatures=0 X-Proofpoint-Spam-Details: rule=notspam policy=default score=0 priorityscore=1501 malwarescore=0 suspectscore=1 phishscore=0 bulkscore=0 spamscore=0 clxscore=1015 lowpriorityscore=0 mlxscore=0 impostorscore=0 mlxlogscore=999 adultscore=0 classifier=spam adjust=0 reason=mlx scancount=1 engine=8.0.1-1805220000 definitions=main-1806010223 List-Id: List-Help: List-Subscribe: List-Post: List-Owner: List-Archive: Precedence: bulk --000000000000a897e0056d996fbc Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable 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/chi= ld.js >>> C pgadmin/browser/server_groups/servers/databases/schemas/static/js/sch= ema_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=E2=80=99t 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=E2=80=99s 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 >>> =3D=3D=3D undefined or return _.isUndefined(obj) as this is more intuit= ive. >>> >>> 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 > >> =E2=80=8B >>> >>> Thanks >>> Anthony && Victoria >>> >>> On Fri, Jun 1, 2018 at 7:52 AM Ashesh Vashi < >>> ashesh.vashi@enterprisedb.com> wrote: >>> >>>> On Thu, May 24, 2018 at 8:13 PM, Joao De Almeida Pereira < >>>> jdealmeidapereira@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. Co= uld 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 pgadmi= n/browser/server_groups/servers/databases/schemas/static/js/can_drop_child.= js >>>> - We don't need this with the new implementation.C pgadmin/browser/ser= ver_groups/servers/databases/schemas/static/js/child.js >>>> - All the children of schema node have common properties as 'parent_ty= pe', 'canDrop', 'canDropCascase', 'canCreate'. >>>> Hence - instead of defining them in each node, we have created a bas= e 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 'childCreateMenuEnabl= ed', 'isTreeItemOfChildOfSchema', & 'isTreeNodeOfSchemaChild', which are us= ed by the 'SchemaChildNode' objects.M pgadmin/browser/static/js/collection.= js >>>> - Fixed an issue related to wrongly defined 'error' function for the C= ollection 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/ca= talog 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 unde= r 'pgadmin/browser/server_groups/servers/databases/schemas/static/js' direc= tory.D pgadmin/static/js/menu/menu_enabled.jsC pgadmin/static/js/nodes/supp= orted_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 w= hich 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/java= script/fake_browser/browser.js >>>> C regression/javascript/nodes/schema/child_menu_spec.js >>>> - Modified the regression to test the new functionalies.M pgadmin/brow= ser/server_groups/servers/databases/schemas/**/*.js >>>> - Extending the schema child nodes from the 'SchemaChildNode' class de= fined 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 introduce >>>>> this abstraction into the code base, which is a big enough task. I'd = hate >>>>> for the scope of the changes we're making to expand beyond that. >>>>> >>>> >>>> I have the mutual feeling. >>>> >>>> -- Thanks, Ashesh >>>> >>>>> >>>>> Thanks >>>>> Joao && Anthony >>>>> >>>>> >>>>> On Thu, May 24, 2018 at 2:59 AM Ashesh Vashi < >>>>> ashesh.vashi@enterprisedb.com> wrote: >>>>> >>>>>> Sorry for the late reply. >>>>>> On Wed, May 16, 2018 at 8:55 PM, Anthony Emengo >>>>>> wrote: >>>>>> >>>>>>> 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 lig= hter >>>>>>> node, I don=E2=80=99t 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=E2=80=99t feel too strongly here. >>>>>>> >>>>>> I do agree - we can handle the same problem many ways. >>>>>> I prefer object oriented pardigm more in general. >>>>>> Any way - I have modified the code with some other changes. >>>>>> >>>>>>> I renamed it as isValidTreeNodeData, because - we were using it in >>>>>>> for testing the tree data. Please suggest me the right place, and n= ame. >>>>>>> >>>>>>> We=E2=80=99re not sure; maybe after continued refactoring, we will = come >>>>>>> across more generic functions. At that point we can revisit this an= d create >>>>>>> a utils.js file. >>>>>>> >>>>>> Sure. >>>>>> >>>>>>> The original patch was separating them in different places, but - >>>>>>> still uses some of the functionalities directly from the tree, whic= h was >>>>>>> happening because we have contextual menu. >>>>>>> To give a better solution, I can think of putting the menus related >>>>>>> code understand =E2=80=98sources/tree/menu=E2=80=99 directory. >>>>>>> >>>>>>> We=E2=80=99re particularly worried because we=E2=80=99re trying to = avoid the >>>>>>> coupling that we see in the code base today. We want to decouple *a= pplication >>>>>>> state* from *business domain* logic as much as we can - because >>>>>>> this makes the code much easier to understand. We achieve lower cou= pling by >>>>>>> have more suitable interfaces to retrieve *application state* like: >>>>>>> anyParent (the menu doesn=E2=80=99t care how this happens). This is= the >>>>>>> direction that we=E2=80=99re trying to move towards, we just don=E2= =80=99t want the package >>>>>>> structure to undermine that developer intent. >>>>>>> >>>>>> I realized after revisiting the code, menu/can_create.js was only >>>>>> applicable to the children of the schema/catalog nodes, same as >>>>>> 'can_drop_child'. >>>>>> We should have put both scripts in the same directory. >>>>>> >>>>>> Please find the updated patch for the same. >>>>>> >>>>>> Please review it, and let me know your concerns. >>>>>> >>>>>> -- Thanks, Ashesh >>>>>> >>>>>>> How about nodeMenu.isSupportedNode(=E2=80=A6)? >>>>>>> >>>>>>> Naming is one of the hardest problems in programming. I don=E2=80= =99t feel >>>>>>> too strongly about this one. For now, let=E2=80=99s keep it as is >>>>>>> >>>>>>> Thanks >>>>>>> Anthony && Victoria >>>>>>> =E2=80=8B >>>>>>> >>>>>>> >>>>>>> >>>>>>> >>>>>> >>>> > --000000000000a897e0056d996fbc Content-Type: text/html; charset="UTF-8" Content-Transfer-Encoding: quoted-printable
Hey Ashesh,

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

Sincerely,

Vi= ctoria

On Fri, J= un 1, 2018 at 2:36 PM Ashesh Vashi <ashesh.vashi@enterprisedb.com> wrote:
On Fri, Jun 1, 2018 at 10:09 PM, Victoria Henry <v= henry@pivotal.io> wrote:
Hi = Ashesh,

We just attempted to apply your patch over maste= r but it did not work.=C2=A0 We don't want to introduce any bugs or bre= ak any functionality.=C2=A0 Please update the patch to make sure it is sync= ed up with the master branch.
Please find the = updated patch.
Sincerely,

Victoria

On Fri, Jun 1, 2018 at 11:18 AM Anthony Emengo <aemengo@pivotal.io>= ; wrote:

Hey Ashesh,

Thanks for the explanation. It = was great and it really helped!

C pgadmin/browser/se=
rver_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 ar= e related so we don=E2=80=99t 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).= =C2=A0
M pgadmin/static/js/=
tree/tree.js

The creation of th= e ancestorNode function feels like a pre-optimizati= on. That function is not used any where outside of the tre= e.js file, so it=E2=80=99s more confusing to have it defined.

It is being used = in the latest changes. :-)
=C2=A0

O= n a lighter note, could we avoid the !! syntax when possible? For example,= instead of return !!obj, we could do something lik= e return obj =3D=3D=3D undefined or return _.isUndefined(obj) as this is more intuitive.

https://softwareengine= ering.stackexchange.com/a/80092

I am kind of disagree here. But - I have changed i= t anyway.=C2=A0

In addition, please update this= patch as it is out of sync with the latest commit on the master branch. Ot= herwise, everything looks good!

Here - you go!

-- Thanks, Ash= esh=C2=A0
=E2=80=8B

Thanks
Antho= ny && Victoria

On Fri, Jun 1, 2018 at 7:52 AM Ashesh Vashi <ashesh.vashi@en= terprisedb.com> wrote:
On Thu, May 24, 2018 at 8:13 PM, Joao De Almeida Pereira <= span dir=3D"ltr"><jdealmeidapereira@pivotal.io> wrote:
Hey, Thanks so much for the repl= y.

We've noticed that you've made several modifi= cations on top of our original patch. Unfortunately, we've found it ver= y hard to follow. Could we please get a brief synopsis of the changes you h= ave 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 w=
ebpack.shim.js
M webpack.test.config.js
- In order t=
o specify the fake_browser in regression tests, we need to use 'pgbrows=
er/browser' in the 'schema_child_tree_node.js' script.D pgadmin/browser/ser=
ver_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/sche=
mas/static/js/child.js
<=
/span>- All the children of schema node have comm=
on properties as 'parent_type', 'canDrop', 'canDropCasc=
ase', 'canCreate'.
  Hence - instead=
 of defining them in each node, we have created a base node, which will hav=
e all these properties.<=
/span>
  And, modified all schema children node to inherit from it.C p=
gadmin/browser/server_groups/servers/databases/schemas/static/js/schema_chi=
ld_tree_node.js
- In t=
his script, we're defining three functions 'childCreateMenuEnabled', 'isTreeItemOfChildOfSchema', & 'isTreeNodeOfSchemaChild', which are used by the '=
SchemaChild=
Node' objects.M pgadmin/browser/static/js/collection.js
- Fixed an issue related t=
o wrongly defined 'error' function for the Collection object.D pgadmin/static/js/menu=
/can_create.js
- It defined the function, which was definin=
g a check for creation of a schema child node, or not by looking at the par=
ent 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 tool=
s for checking whether the 'selected' tree-node is:
  + 'database' node, a=
nd 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 defi=
ned pattern, also it can be used by other functions too. Hence - moved it o=
ut of 'pgadmin/static/js/menu' directory.M pgadmin/static/js/tree/tree.js- Introduced a function, which returns the ance=
stor 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.js
C regression/javascript/fake_browser/browser.js
C regression/javascript/nodes/schema/child_me=
nu_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 defi=
ned in 'pgadmin/.../schemas/static/js/child.js' script.<=
/code>
Let me know if you need more information.
= =C2=A0
Let's keep in mind that the original intent was sim= ply to introduce this abstraction into the code base, which is a big enough= task. I'd hate for the scope of the changes we're making to expand= beyond that.

I have the mutual= feeling.

-- Thanks, Ashesh=C2=A0

Thanks
Joao && Anthony


On Thu, Ma= y 24, 2018 at 2:59 AM Ashesh Vashi <ashesh.vashi@enterprisedb.com> wrote:=
Sorry for the late reply.
On Wed, May 16, 2018 at 8:55 PM, Anthon= y Emengo <aemengo@pivotal.io> wrote:
<= /div>
e= xport function canCreate(pgBrowser, childOfCatalogType) { return canCreateObject.bind({ browser: pgBrowser, childOfCatalogType: childOfCatalogType, }); }

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

I do agree - we can handle = the same problem many ways.
I prefer object oriented pardigm more= in general.
Any way - I have modified the code with some other c= hanges.

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

We=E2=80=99re not sure; maybe after conti= nued refactoring, we will come across more generic functions. At that poin= t we can revisit this and create a utils.js file.

Sure.=C2=A0

The original patch was separating them in= different places, but - still uses some of the functionalities directly fr= om the tree, which was happening because we have contextual menu.
To giv= e a better solution, I can think of putting the menus related code understa= nd =E2=80=98sources/tree/menu=E2=80=99 directory.

We=E2=80=99re particularly worried= because we=E2=80=99re trying to avoid the coupling that we see in the code= base today. We want to decouple application state from busine= ss domain logic as much as we can - because this makes the code much e= asier to understand. We achieve lower coupling by have more suitable interf= aces to retrieve application state like: anyParen= t (the menu doesn=E2=80=99t care how this happens). This is the dire= ction that we=E2=80=99re trying to move towards, we just don=E2=80=99t want= the package structure to undermine that developer intent.

<= /blockquote>
I realized after revisiting the code, menu/can_create.js w= as only applicable to the children of the schema/catalog nodes, same as = 9;can_drop_child'.
We should have put both scripts in the sam= e directory.

Please find the updated patch for the= same.

Please review it, and let me know your conc= erns.

-- Thanks, Ashesh

How about nodeMenu.isSupportedNode(=E2=80= =A6)?

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

Thanks
Anthony && Victoria

=E2=80=8B

=





--000000000000a897e0056d996fbc--