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 1fOn5G-0003TO-PE for pgadmin-hackers@arkaria.postgresql.org; Fri, 01 Jun 2018 16:39:23 +0000 Received: from localhost ([127.0.0.1] helo=malur.postgresql.org) by malur.postgresql.org with esmtp (Exim 4.89) (envelope-from ) id 1fOn5F-0003Lq-6g for pgadmin-hackers@arkaria.postgresql.org; Fri, 01 Jun 2018 16:39:21 +0000 Received: from makus.postgresql.org ([2001:4800:1501:1::229]) by malur.postgresql.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_CBC_SHA384:256) (Exim 4.89) (envelope-from ) id 1fOn5E-0003Ky-Fv for pgadmin-hackers@lists.postgresql.org; Fri, 01 Jun 2018 16:39:21 +0000 Received: from mx0b-00296801.pphosted.com ([148.163.153.148]) by makus.postgresql.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_CBC_SHA384:256) (Exim 4.89) (envelope-from ) id 1fOn59-0000st-W0 for pgadmin-hackers@postgresql.org; Fri, 01 Jun 2018 16:39:18 +0000 Received: from pps.filterd (m0114585.ppops.net [127.0.0.1]) by mx0b-00296801.pphosted.com (8.16.0.22/8.16.0.22) with SMTP id w51GZpjk015450 for ; Fri, 1 Jun 2018 16:39:14 GMT Received: from mail-io0-f198.google.com (mail-io0-f198.google.com [209.85.223.198]) by mx0b-00296801.pphosted.com with ESMTP id 2jb7apg4qn-1 (version=TLSv1.2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128 verify=NOT) for ; Fri, 01 Jun 2018 16:39:14 +0000 Received: by mail-io0-f198.google.com with SMTP id u16-v6so20823124iol.18 for ; Fri, 01 Jun 2018 09:39:13 -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=cOGD1gLQxWjUnKq2g8PGQ7LNrCwirxBuytkv03D0CLs=; b=sxEEB4GTXz6ZZhQ/PMyOwKSHW34dhQ+EhvaueAEvGnMFKotNiJYe4VXUcwj4tBNXrV cD4VDH48UaXoPu9ZDrRaOIHIwnvGEf0Dary418+x/ovvfvQxm9u3iszB8cru0z+9EJVI vB81KcNzPeKeMiXCVZsipTOQYjTjBtPoO7OEoXQHMIIPgwAEzcXO+FZFMK+qvLOPM/HF d8kOuNoV57RbI2Dbu2kY0VVx6gtwVJoOqpbl7GAA0ChplK7GPLk838gg5MEEyLmSa+76 XV9QLE1dp3X2w234JsfR9tvtyai1Bow8ucYd/x2Skf0MVH7erCXDvuCtbi8NbNuGIHY+ ZdZA== X-Gm-Message-State: ALKqPwe0nY8F/+BppLqBHOJ5cKWaXY2cjXk7ieZCdKwm99Lzz2GO8YYy CGeJYMAgzhjZceBBVPifjYjFEdk/V6iN0mp98nDwTYIcjimD1Uifu5+qePSmSGFgISdQcD99lKh D+8TY72QjXcW97OfgASquNMoY4acg3DvQYSHi9n2HCxnPOxFmriwoKsv/EgCw52aB/mFY X-Received: by 2002:a6b:6b02:: with SMTP id g2-v6mr11776068ioc.250.1527871153139; Fri, 01 Jun 2018 09:39:13 -0700 (PDT) X-Google-Smtp-Source: ADUXVKKBwP0Dfp6JG4ZGQK6FYk//3FLX454wprtHSCvul5LHGmCdgk42WIBvZ/zlnbrTEhux5EF6huJ0Ijc6CIlmELM= X-Received: by 2002:a6b:6b02:: with SMTP id g2-v6mr11776034ioc.250.1527871152652; Fri, 01 Jun 2018 09:39:12 -0700 (PDT) MIME-Version: 1.0 References: In-Reply-To: From: Victoria Henry Date: Fri, 1 Jun 2018 12:39:01 -0400 Message-ID: Subject: Re: [pgadmin4][patch] Initial patch to decouple from ACI Tree To: Anthony Emengo Cc: Ashesh Vashi , Joao De Almeida Pereira , Dave Page , Akshay Joshi , Murtuza Zabuawala , pgadmin-hackers , Khushboo Vashi Content-Type: multipart/alternative; boundary="000000000000511e2e056d97392b" X-Proofpoint-Virus-Version: vendor=fsecure engine=2.50.10434:,, definitions=2018-06-01_11:,, 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-1806010194 List-Id: List-Help: List-Subscribe: List-Post: List-Owner: List-Archive: Precedence: bulk --000000000000511e2e056d97392b Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable Hi Ashesh, We just attempted to apply your patch over master but it did not work. We don't want to introduce any bugs or break any functionality. Please update the patch to make sure it is synced up with the master branch. Sincerely, Victoria On Fri, Jun 1, 2018 at 11:18 AM Anthony Emengo wrote: > Hey Ashesh, > > Thanks for the explanation. It was great and it really helped! > > C pgadmin/browser/server_groups/servers/databases/schemas/static/js/child= .js > C pgadmin/browser/server_groups/servers/databases/schemas/static/js/schem= a_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? > > 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. 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 intuitive. > > https://softwareengineering.stackexchange.com/a/80092 > > In addition, please update this patch as it is out of sync with the lates= t > commit on the master branch. Otherwise, everything looks good! > =E2=80=8B > > 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 < >> 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. Coul= d we >>> please get a brief synopsis of the changes you have made - just so we c= an >>> better understand the rationale behind them? Just like we've done for y= ou >>> 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 u= se '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/serve= r_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/br= owser/server_groups/servers/databases/schemas/static/js/schema_child_tree_n= ode.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 Col= lection 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/cata= log 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' directo= ry.D pgadmin/static/js/menu/menu_enabled.jsC pgadmin/static/js/nodes/suppor= ted_database_node.js >> - Used by the external tools for checking whether the 'selected' tree-no= de 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 de= fined 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 whi= ch 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/javasc= ript/fake_browser/browser.js >> C regression/javascript/nodes/schema/child_menu_spec.js >> - Modified the regression to test the new functionalies.M pgadmin/browse= r/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. >> >> Let me know if you need more information. >> >> >>> Let's keep in mind that the original intent was simply to introduce thi= s >>> abstraction into the code base, which is a big enough task. I'd hate fo= r >>> 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 seem= s >>>>> like the idiomatic way to handle this in JavaScript. On a lighter nod= e, 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 fo= r >>>>> testing the tree data. Please suggest me the right place, and name. >>>>> >>>>> We=E2=80=99re not sure; maybe after continued refactoring, we will co= me across >>>>> more generic functions. At that point we can revisit this and 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, which = 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 av= oid 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 b= y have >>>>> more suitable interfaces to retrieve *application state* like: >>>>> anyParent (the menu doesn=E2=80=99t care how this happens). This is t= he >>>>> 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 >>>>> >>>>> >>>>> >>>>> >>>> >> --000000000000511e2e056d97392b Content-Type: text/html; charset="UTF-8" Content-Transfer-Encoding: quoted-printable
Hi Ashesh,

We just attempted to apply y= our patch over master but it did not work.=C2=A0 We don't want to intro= duce any bugs or break any functionality.=C2=A0 Please update the patch to = make sure it is synced up with the master branch.

= Sincerely,

Victoria

On Fri, Jun 1, 2018 at 11:18 AM Anthony Emengo= <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?

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. On a l= ighter note, could we avoid the !! syntax when possible? For example, inst= ead of return !!obj, we could do something like return obj =3D=3D=3D undefined or ret= urn _.isUndefined(obj) as this is more intuitive.

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

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!

=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 to specify the fake_browser in regression tests, we need to use &=
#39;pgbrowser/browser' in the 'schema_child_tree_node.js' script.D <=
font color=3D"#ff0000" style=3D"font-family:Consolas,Inconsolata,Courier,mo=
nospace;font-size:12.5538px;font-style:normal;font-variant-ligatures:normal=
;font-variant-caps:normal;font-weight:400;letter-spacing:normal;text-align:=
start;text-indent:0px;text-transform:none;white-space:pre-wrap;word-spacing=
:0px;text-decoration-style:initial;text-decoration-color:initial">pgadmin/b=
rowser/server_groups/servers/databases/schemas/static/js/can_drop_child.js<=
span style=3D"color:rgb(34,34,34);font-family:Consolas,Inconsolata,Courier,=
monospace;font-size:12.5538px;font-style:normal;font-variant-ligatures:norm=
al;font-variant-caps:normal;font-weight:400;letter-spacing:normal;text-alig=
n:start;text-indent:0px;text-transform:none;white-space:pre-wrap;word-spaci=
ng:0px;text-decoration-style:initial;text-decoration-color:initial">
- We=
 don't need this with the new implementation.C pgadmin/browser/server_groups/se=
rvers/databases/schemas/static/js/child.js
- All the ch=
ildren of schema node have common properties as 'parent_type', '=
;canDrop', 'canDropCascase', 'canCreate'.
  Hence - instead of defining them in each node, w=
e have created a base node, which will have all these properties.
  And, modif=
ied 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', 'isTree=
ItemOfChildOfSchema', & 'isTreeNodeOfSchemaChild', which are used by the 'SchemaChildNode' objects.M pgadmin/browser/static/js/collection.js
- Fixed an issue related to w=
rongly defined 'error' function for the Collection object.D pgadmin/static/js/menu/ca=
n_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).<=
br class=3D"m_-7804285449525201396m_-3582030928604831699gmail-Apple-interch=
ange-newline" style=3D"color:rgb(191,144,0);font-family:Consolas,Inconsolat=
a,Courier,monospace;font-size:12.5538px;font-style:normal;font-variant-liga=
tures:normal;font-variant-caps:normal;font-weight:400;letter-spacing:normal=
;text-align:start;text-indent:0px;text-transform:none;white-space:pre-wrap;=
word-spacing:0px;text-decoration-style:initial;text-decoration-color:initia=
l">  The file was not defintely pla=
ced under the wrong directory, because - the similar logic was under '<=
span style=3D"font-family:Consolas,Inconsolata,Courier,monospace;font-size:=
12.5538px;font-style:normal;font-variant-ligatures:normal;font-variant-caps=
:normal;font-weight:400;letter-spacing:normal;text-align:start;text-indent:=
0px;text-transform:none;white-space:pre-wrap;word-spacing:0px;background-co=
lor:rgb(248,248,248);text-decoration-style:initial;text-decoration-color:in=
itial;float:none;display:inline">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.js<=
/font>C pgadmin/static/js/nodes/supported_data=
base_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 th=
e 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 'pg=
admin/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/sche=
ma/child_menu_spec.js
<=
font color=3D"#000000" style=3D"font-family:Consolas,Inconsolata,Courier,mo=
nospace;font-size:12.5538px;font-style:normal;font-variant-ligatures:normal=
;font-variant-caps:normal;font-weight:400;letter-spacing:normal;text-transf=
orm:none;white-space:pre-wrap;word-spacing:0px;text-align:start;text-indent=
:0px;text-decoration-style:initial;text-decoration-color:initial;background=
-color:rgb(248,248,248)">- Modified the regression to test the new function=
alies.M pgadmin/=
browser/server_groups/servers/databases/schemas/**/*.js
- Extending the schema child n=
odes from the 'pgadmin/.../schemas/static/js/child.js<=
/font>' script.
Let me know if you= need more information.
=C2=A0
Let's keep in min= d that the original intent was simply to introduce this abstraction into th= e 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.

-- Tha= nks, Ashesh=C2=A0

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= <aemengo@pivotal.io> wrote:
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.isSupportedNo= de(=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

=




--000000000000511e2e056d97392b--