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 1fQwk0-0000Hh-OV for pgadmin-hackers@arkaria.postgresql.org; Thu, 07 Jun 2018 15:22:21 +0000 Received: from localhost ([127.0.0.1] helo=malur.postgresql.org) by malur.postgresql.org with esmtp (Exim 4.89) (envelope-from ) id 1fQwjz-0002Vr-96 for pgadmin-hackers@arkaria.postgresql.org; Thu, 07 Jun 2018 15:22:19 +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 1fQwjy-0002Vh-W8 for pgadmin-hackers@lists.postgresql.org; Thu, 07 Jun 2018 15:22:19 +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 1fQwjq-0002RR-Sf for pgadmin-hackers@postgresql.org; Thu, 07 Jun 2018 15:22:17 +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 w57FKku6011404 for ; Thu, 7 Jun 2018 15:22:07 GMT Received: from mail-io0-f197.google.com (mail-io0-f197.google.com [209.85.223.197]) by mx0a-00296801.pphosted.com with ESMTP id 2jbj0md7fd-1 (version=TLSv1.2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128 verify=NOT) for ; Thu, 07 Jun 2018 15:22:05 +0000 Received: by mail-io0-f197.google.com with SMTP id n21-v6so7729006iob.19 for ; Thu, 07 Jun 2018 08:22:05 -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=FWe8VbDaUElnFdd5WuIxbVOwSSLK3NqVTlViQM8ubhM=; b=dSFFlPXo4otxkbRKdUm2qCi0sho6PJB04ZnqZeh0gZczTimk5rI4fYcllx3B7XRC5B eG72B6a+0/wFnPLTpbRUkOYAVmTvO/Eu5IFHX0mUok/wajxH4yzYWuzeriWvWhYSfoeo jiURsGFyY4Bkc8qiva2gHUs50rN2K/z4FNp5QFLo7hSYXcKfbfX+tLoODbjbTZHt7ctm 21LzZO3wbSP7LXyPPafs+gL+EtQjjWgWgPNcmadCbxU0vqXzZ3GqFTyGiYuIMt9e/RIv nhI/HWN4RpU9ch7QbP0LaZsmCHx61soqlws5tN2E01a26waaSE+uCa+eYzKFYyLRFVUB ryTA== X-Gm-Message-State: APt69E2Jj86ZNVbmoaOo+qVoqgOe2NSgPX2IUvMW6OzKMdWVYroPIaVV iGQYH+uU3Nuu29vgW7yUn+d4fxuvnEhehM37MKKKfbtHE+N3XIfGRT1faSEiOeS412RNzKgJIEA Jcm/x3s+01ROd2waorKX0CqEWCTjvyYB3lVMOwJBFNV6Tk//9GOtLDCAG9hCSOUsPGNI6 X-Received: by 2002:a6b:b513:: with SMTP id e19-v6mr2066472iof.250.1528384924618; Thu, 07 Jun 2018 08:22:04 -0700 (PDT) X-Google-Smtp-Source: ADUXVKJlu6bsGqGA6FQTalsI9YR90W2QnffgIN03j9S7MmDqV3dDdfF7YbfjQywrQeMg+X81ZoSNueXC7Wr8jVxgTV4= X-Received: by 2002:a6b:b513:: with SMTP id e19-v6mr2066438iof.250.1528384924229; Thu, 07 Jun 2018 08:22:04 -0700 (PDT) MIME-Version: 1.0 References: In-Reply-To: From: Victoria Henry Date: Thu, 7 Jun 2018 11:21:52 -0400 Message-ID: Subject: Re: [pgadmin4][patch] Initial patch to decouple from ACI Tree To: Aditya Toshniwal Cc: Ashesh Vashi , Anthony Emengo , Joao De Almeida Pereira , Dave Page , Akshay Joshi , Murtuza Zabuawala , pgadmin-hackers , Khushboo Vashi Content-Type: multipart/alternative; boundary="0000000000007d3b17056e0ed893" X-Proofpoint-Virus-Version: vendor=fsecure engine=2.50.10434:,, definitions=2018-06-07_06:,, signatures=0 X-Proofpoint-Spam-Details: rule=notspam policy=default score=0 priorityscore=1501 malwarescore=0 suspectscore=2 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-1806070172 List-Id: List-Help: List-Subscribe: List-Post: List-Owner: List-Archive: Precedence: bulk --0000000000007d3b17056e0ed893 Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable Hi Aditya Sure. I did not find moving > web/pgadmin/tools/datagrid/static/js/datagrid.js. Please correct me if I = am > missing anything. Generally speaking, I agree with moving/deleting files if it makes sense. But in regards to web/pgadmin/tools/datagrid/static/js/datagrid.js, it looks like this is still being used in web/pgadmin/tools/sqleditor/static/js/sqleditor.js with Datagrid.create_transaction Sincerely, Victoria =E2=80=8B On Thu, Jun 7, 2018 at 12:35 AM Aditya Toshniwal < aditya.toshniwal@enterprisedb.com> wrote: > Hi Victoria, > > On Wed, Jun 6, 2018 at 8:55 PM, Victoria Henry wrote: > >> Hi Aditya, >> >> 1) Why don't we start using webpack alias's instead of using absolute >>> path. For eg, >>> import {RestoreDialogWrapper} from >>> '../../../pgadmin/static/js/restore/restore_dialog_wrapper'; >>> can be used as import {RestoreDialogWrapper} from >>> 'pgadmin_static/js/restore/restore_dialog_wrapper'; >>> by adding pgadmin_static alias to webpack config. >> >> >> Great point. In some areas of the code, we began making this change. >> There is already an alias in webpack shims for ` >> ../../../pgadmin/static/js` called `sources`. You can find an example >> of this in import statements for `supported_database_node.js` >> >> 2) Few of the js are left behind, the ones which are used in python >>> __init__.py. Can't we move them too ? It would be nicer to not to leave >>> behind a single js. >> >> I'm not sure what you mean. Could you point to an example of a single j= s >> file? >> > > Sure. I did not find moving > web/pgadmin/tools/datagrid/static/js/datagrid.js. Please correct me if I = am > missing anything. > >> >> Sincerely, >> >> Victoria >> >> On Wed, Jun 6, 2018 at 7:07 AM Aditya Toshniwal < >> aditya.toshniwal@enterprisedb.com> wrote: >> >>> Hi Anthony/Victoria/Joao, >>> >>> I know I am very late to review on patch 004. The idea of moving js >>> files from tools to static folder looks good, but I have a few suggesti= ons: >>> >>> 1) Why don't we start using webpack alias's instead of using absolute >>> path. For eg, >>> import {RestoreDialogWrapper} from >>> '../../../pgadmin/static/js/restore/restore_dialog_wrapper'; >>> can be used as import {RestoreDialogWrapper} from >>> 'pgadmin_static/js/restore/restore_dialog_wrapper'; >>> by adding pgadmin_static alias to webpack config. >>> >>> 2) Few of the js are left behind, the ones which are used in python >>> __init__.py. Can't we move them too ? It would be nicer to not to leave >>> behind a single js. >>> >>> Kindly let me know your views on this. >>> >>> >>> Thanks and Regards, >>> Aditya Toshniwal >>> Software Engineer | EnterpriseDB Software Solutions | Pune >>> "Don't Complain about Heat, Plant a tree" >>> >>> On Sat, Jun 2, 2018 at 12:47 AM, Victoria Henry >>> wrote: >>> >>>> 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 < >>>> ashesh.vashi@enterprisedb.com> 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 functionalit= y. >>>>>> 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= /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=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 enoug= h >>>>> 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 th= e >>>>>>> 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 mor= e >>>>>>> intuitive. >>>>>>> >>>>>>> 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 goo= d! >>>>>>> >>>>>> 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 ou= r >>>>>>>>> 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 s= o 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 nee= d to use 'pgbrowser/browser' in the 'schema_child_tree_node.js' script.D pg= admin/browser/server_groups/servers/databases/schemas/static/js/can_drop_ch= ild.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 'paren= t_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 pgad= min/browser/server_groups/servers/databases/schemas/static/js/schema_child_= tree_node.js >>>>>>>> - In this script, we're defining three functions 'childCreateMenuE= nabled', 'isTreeItemOfChildOfSchema', & 'isTreeNodeOfSchemaChild', which ar= e used by the 'SchemaChildNode' objects.M pgadmin/browser/static/js/collect= ion.js >>>>>>>> - Fixed an issue related to wrongly defined 'error' function for t= he 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 schem= a/catalog node). >>>>>>>> The file was not defintely placed under the wrong directory, bec= ause - the similar logic was under 'can_drop_child.js', and it was defined = under 'pgadmin/browser/server_groups/servers/databases/schemas/static/js' d= irectory.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' t= ree-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 - mo= ved it out of 'pgadmin/static/js/menu' directory.M pgadmin/static/js/tree/t= ree.js >>>>>>>> - Introduced a function, which returns the ancestor node object, f= ow 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' clas= s defined 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 eno= ugh task. >>>>>>>>> I'd hate for the scope of the changes we're making to expand beyo= nd 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 < >>>>>>>>>> aemengo@pivotal.io> wrote: >>>>>>>>>> >>>>>>>>>>> export function canCreate(pgBrowser, childOfCatalogType) { >>>>>>>>>>> return canCreateObject.bind({ >>>>>>>>>>> browser: pgBrowser, >>>>>>>>>>> childOfCatalogType: childOfCatalogType, >>>>>>>>>>> }); >>>>>>>>>>> } >>>>>>>>>>> >>>>>>>>>>> With respect to the above code: this bind pattern looks good an= d >>>>>>>>>>> seems like the idiomatic way to handle this in JavaScript. On a= lighter >>>>>>>>>>> node, I don=E2=80=99t even see the need for an additional metho= d 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 name. >>>>>>>>>>> >>>>>>>>>>> We=E2=80=99re not sure; maybe after continued refactoring, we w= ill come >>>>>>>>>>> across more generic functions. At that point we can revisit thi= s 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 dir= ectory. >>>>>>>>>>> >>>>>>>>>>> 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 decoupl= e *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=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 onl= y >>>>>>>>>> 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 i= t as is >>>>>>>>>>> >>>>>>>>>>> Thanks >>>>>>>>>>> Anthony && Victoria >>>>>>>>>>> =E2=80=8B >>>>>>>>>>> >>>>>>>>>>> >>>>>>>>>>> >>>>>>>>>>> >>>>>>>>>> >>>>>>>> >>>>> >>> > --0000000000007d3b17056e0ed893 Content-Type: text/html; charset="UTF-8" Content-Transfer-Encoding: quoted-printable

Hi Aditya

Sure. I = did not find moving web/pgadmin/tools/datagrid/static/js/datagrid.js. Pleas= e correct me if I am missing anything.

Generally speaking, I agree wit= h moving/deleting files if it makes sense. But in regards to web/pgadmin/tools/datagrid/static/js/datagrid.js, it looks lik= e this is still being used in web/pgadmin/tools/sqleditor/= static/js/sqleditor.js with Datagrid.create_transac= tion

Sincerely,

Victoria

=E2=80=8B



On Thu, Jun 7, 2018= at 12:35 AM Aditya Toshniwal <aditya.toshniwal@enterprisedb.com> wrote:
Hi Victoria,

On Wed, Jun 6, 2018 at 8:55 PM, Victoria Henry <vhenry@pivot= al.io> wrote:
Hi Aditya,

1) = Why don't we start using webpack alias's instead of using absolute = path. For eg,
import {RestoreDialogWrapper} from '../../../pgadmin/s= tatic/js/restore/restore_dialog_wrapper';
can be used as import {Res= toreDialogWrapper} from 'pgadmin_static/js/restore/restore_dialog_wrapp= er';=C2=A0
by adding pgadmin_static alias to webpack config.
Great point. In some areas of the code, we began= making this change.=C2=A0 There is already an alias in webpack shims for `= ../../../pgadmin/static/js` called `sources`.=C2= =A0 You can find an example of this in import statements for `supported_dat= abase_node.js`

2)= Few of the js are left behind, the ones which are used in python __init__.= py. Can't we move them too ? It would be nicer to not to leave behind a= single js.
I'm not sure what you mean.= =C2=A0 Could you point to an example of a single js file?

Sure. I did not find moving web/pgadmin/tools/da= tagrid/static/js/datagrid.js. Please correct me if I am missing anything.

Sincerely,

Victoria

On Wed, Jun= 6, 2018 at 7:07 AM Aditya Toshniwal <aditya.toshniwal@enterprisedb.com&= gt; wrote:
Hi Anthony/Victoria/Joao,

I know I am very l= ate to review on patch 004. The idea of moving js files from tools to stati= c folder looks good, but I have a few suggestions:

1) Why don't we start using webpack alias's instead of using absol= ute path. For eg,
import {RestoreDialogWrapper} from '../../.= ./pgadmin/static/js/restore/restore_dialog_wrapper';
can be u= sed as import {RestoreDialogWrapper} from 'pgadmin_static/js/restore/re= store_dialog_wrapper';=C2=A0
by adding pgadmin_static alias t= o webpack config.

2) Few of the js are left behind= , the ones which are used in python __init__.py. Can't we move them too= ? It would be nicer to not to leave behind a single js.

Kindly let me know your views on this.


<= div dir=3D"ltr">
Thanks and Regards,
Aditya Toshniwal
<= span style=3D"background-color:rgb(255,255,255)">Software Engineer |=C2=A0Enterpr= iseDB Software Solutions |=C2=A0Pune
"Don't Complain abou= t Heat, Plant a tree"

On Sat, Jun 2, 2018 at 12:47 AM, Victoria He= nry <vhenry@pivotal.io> wrote:
Hey Ashesh,

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

Sincerely,

Victoria
<= br>
On Fri, Jun 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:
<= div dir=3D"ltr">Hi Ashesh,

We just attempted to apply yo= ur patch over master but it did not work.=C2=A0 We don't want to introd= uce any bugs or break any functionality.=C2=A0 Please update the patch to m= ake 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 <aemengo@pivotal.io>= wrote:
<= p style=3D"margin:0px 0px 1.2em">Hey Ashesh,

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

C p=
gadmin/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 dup= lication 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<= /code>?

Tha= t was the original plan, but 'pgadmin/browser/static/js//node.js' s= cript has too many dependecies, which are not easily portable.
An= d - that may lead to change the scope of the patch.

Hence - I decided to use the separate file to make sure we have enough te= st coverage (which is more imprortant than changing the scope).=C2=A0
= 
M p=
gadmin/static/js/tree/tree.js

The creation of the ancestorNode function feels like a pre-optimization. That f= unction is not used any where outside of the tree.js file, so it=E2=80=99s more confusing to have it defined.

<= /blockquote>
It is being used in the lat= est changes. :-)
=C2=A0

On a li= ghter note, could we avoid the !! syntax when possible? For example, inste= ad of return !!obj, we could do something like return obj =3D=3D=3D undefined or retu= rn _.isUndefined(obj) as this is more intuitive.

https://softwareengineering.stac= kexchange.com/a/80092

I am kind of disagree here. But - I have changed it anyway.= =C2=A0
<= div dir=3D"ltr">

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

Here - you go!

-- Thanks, Ashesh=C2=A0<= br>
=E2=80=8B

Thanks
Anthony && Victoria

On Fri, Jun 1, 2018 at 7:52 AM Ashesh Vashi &= lt;ashes= h.vashi@enterprisedb.com> wrote:
On Thu, May 24, 2018 at 8:13 = PM, Joao De Almeida Pereira <jdealmeidapereira@pivotal.io&g= t; wrote:
Hey= , Thanks so much for the reply.

We've noticed that y= ou've made several modifications on top of our original patch. Unfortun= ately, 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 th= e rationale behind them? Just like we've done for you previously.
=
Please find the changes from your original pat= ch:
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 pga=
dmin/browser/server_groups/servers/databases/schemas/static/js/can_drop_chi=
ld.js<=
br class=3D"m_7058649129291636192gmail-m_-7259432427898418545m_-83070640802=
02625060m_8382661824950193576m_3270516565853123642m_-7492672409753239692m_1=
558859882629566770m_-7804285449525201396m_-3582030928604831699gmail-Apple-i=
nterchange-newline" style=3D"color:rgb(191,144,0);font-family:Consolas,Inco=
nsolata,Courier,monospace;font-size:12.5538px;font-style:normal;font-varian=
t-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">- We don't need this with the new implementation.=
C pgadmin/browser/server_groups/servers/databases/schemas/static/j=
s/child.js
- All the children of schema node have common properties as 'pare=
nt_type', 'canDrop', 'canDropCascase', 'canCreate&#=
39;.
  Hence - instead of defin=
ing them in each node, we have created a base node, which will have all the=
se properties.
  And, modified all schema children node to inherit from =
it.C pgadmin/browser/server_groups/servers/databases/schemas/static/js/sc=
hema_child_tree_node.js
- In this script, we're defining three functions =
9;childCreateMenuEnabled', 'isTreeItemOfChildOfSchema', & 'isTreeNodeOfSchemaChild', which are used by the=
 'SchemaChildNode' objects.=
M pgadmin/browser/static/js/c=
ollection.js
- Fixed an issue related to wrongly defined 'error' func=
tion for the Collection object.D pgadmin/static/js/menu/can_cr=
eate.js
- It defined the function, which was defining a check for creatio=
n of a schema child node, or not by looking at the parent node (i.e. a sche=
ma/catalog node).
  The file was not defintely placed under the wrong d=
irectory, 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.js<=
code style=3D"font-size:0.85em;font-family:Consolas,Inconsolata,Courier,mon=
ospace;margin:0px 0.15em;background-color:rgb(248,248,248);white-space:pre-=
wrap;overflow:auto;border-radius:3px;border:1px solid rgb(204,204,204);padd=
ing:0.5em 0.7em;display:block">C 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 o=
ne of the schema child (and, not 'catalog' child).
- Findi=
ng the correct location was difficult for this, as there is no defined patt=
ern, also it can be used by other functions too. Hence - moved it out of &#=
39;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_creat=
e_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/node=
s/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/...<=
font color=3D"#0b5394" 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">/schemas/=
s=
tatic/js/child.js' script.
= Let me know if you need more information.
=C2=A0
Let= 's keep in mind that the original intent was simply to introduce this a= bstraction 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, May 24, 2018 at 2:59 AM Ashesh Vashi <ashesh.vashi@enterprisedb.com= > wrote:
S= orry for the late reply.
On Wed, May= 16, 2018 at 8:55 PM, Anthony Emengo <aemengo@pivotal.io> w= rote:
export function canCreate(pgBro=
wser, 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.isSupp= ortedNode(=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

=







--0000000000007d3b17056e0ed893--