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 1fQWIR-00080N-Gk for pgadmin-hackers@arkaria.postgresql.org; Wed, 06 Jun 2018 11:08:07 +0000 Received: from localhost ([127.0.0.1] helo=malur.postgresql.org) by malur.postgresql.org with esmtp (Exim 4.89) (envelope-from ) id 1fQWIP-0000io-FH for pgadmin-hackers@arkaria.postgresql.org; Wed, 06 Jun 2018 11:08:05 +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 1fQWIO-0000id-NQ for pgadmin-hackers@lists.postgresql.org; Wed, 06 Jun 2018 11:08:05 +0000 Received: from mail-lf0-x242.google.com ([2a00:1450:4010:c07::242]) by makus.postgresql.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_CBC_SHA1:256) (Exim 4.89) (envelope-from ) id 1fQWIF-0001SU-Pt for pgadmin-hackers@postgresql.org; Wed, 06 Jun 2018 11:08:02 +0000 Received: by mail-lf0-x242.google.com with SMTP id 36-v6so8433726lfr.11 for ; Wed, 06 Jun 2018 04:07:55 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=enterprisedb-com.20150623.gappssmtp.com; s=20150623; h=mime-version:in-reply-to:references:from:date:message-id:subject:to :cc; bh=rN47d87mg/uIavPuG0+zKTW2+N0s7K9FDoM0OWJUPf8=; b=IsNDo9QT4o4yHQb/KnN+djTWZcM5oMwkQcFR8CwAsfuIh+BYU/5E2hi8u6d1DUAjiH e5A1cLfZVTflkPJ2W7BkEwSwPMfsmCMxMyXwvq8OXW+q5yq0c5Tp06rK7Y+ww2eUfDrW vj+70Z5XrVRI5DUOJ1CjtK42M9PS61kWNR1jw/W65e7SQaAYnLd9Bat9SWbaE+TSDXIn HQqSQbYQBIDwp6tVrLdsXBUfE3nFhHb7sdM1zInMhUXPq492flQIH7NVRBSKxDPhIgUX B8G2XKMwk/a89TgLjcOWjWCL1woa1h69LoSuqTEBRnY+v0/37TIzkE8HfF4at3ZynSL+ ndWg== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:in-reply-to:references:from:date :message-id:subject:to:cc; bh=rN47d87mg/uIavPuG0+zKTW2+N0s7K9FDoM0OWJUPf8=; b=aQ2CxeIKTiy/l0SBspQPM/B0aMyaRPHG/8hMDkPmeoALeEQv1PAjOVHpU5IPHzQJUV Idh4598y7FcEh+Nw2FrltoRRqWvcjfbQ9tAu74a3q2q5UQHUiFy9YELaI+ekcYeiu/K6 vc9bH8KSg49Gzg+Cv0+hGDZa+VnXArA0WLpmkYusVrjAUTI52hemROZTyKWM+6qTLP+e gH2i7g2IiqfAM5BA8knhuaMoNo6zMxi1yp1u9V5AKt10xyLs7Kc/Yu8B733WzMOtpt4l 8bSEgcNy1fEnaXsHbFTbcPmshkvco4SVUByqklT0LWyGf4czH2GSUqzvCODTmDTXuriy YjsA== X-Gm-Message-State: APt69E0fsLNbd8mr+DZR9uHhs4ur76RRNZ73iaxet2p4qJVlxYu0TnVc UUFkMjf9snJCUy08JcKshoL3bhAUqjTDnFldpK8B/g== X-Google-Smtp-Source: ADUXVKJc7Pa1nq/gti2RFm9DAhSxlyMDLg0Y6QcnLiQTHcsi7TMLt/2f8i7HwtJg+yV368Gx/aQI/gyoWHvD6uUrwhs= X-Received: by 2002:a2e:1545:: with SMTP id 5-v6mr1785103ljv.56.1528283271792; Wed, 06 Jun 2018 04:07:51 -0700 (PDT) MIME-Version: 1.0 Received: by 2002:a2e:9e8a:0:0:0:0:0 with HTTP; Wed, 6 Jun 2018 04:07:50 -0700 (PDT) In-Reply-To: References: From: Aditya Toshniwal Date: Wed, 6 Jun 2018 16:37:50 +0530 Message-ID: Subject: Re: [pgadmin4][patch] Initial patch to decouple from ACI Tree To: Victoria Henry Cc: Ashesh Vashi , Anthony Emengo , Joao De Almeida Pereira , Dave Page , Akshay Joshi , Murtuza Zabuawala , pgadmin-hackers , Khushboo Vashi Content-Type: multipart/alternative; boundary="00000000000088227a056df72d4b" List-Id: List-Help: List-Subscribe: List-Post: List-Owner: List-Archive: Precedence: bulk --00000000000088227a056df72d4b Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable 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 suggestions: 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 yo= u > 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/ch= ild.js >>>> C pgadmin/browser/server_groups/servers/databases/schemas/static/js/sc= hema_child_tree_node.js >>>> >>>> It makes sense to remove duplication by extracting these attributes ou= t >>>> 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 >>>> 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 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. C= ould we >>>>>> please get a brief synopsis of the changes you have made - just so w= e can >>>>>> better understand the rationale behind them? Just like we've done fo= r 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 t= o use 'pgbrowser/browser' in the 'schema_child_tree_node.js' script.D pgadm= in/browser/server_groups/servers/databases/schemas/static/js/can_drop_child= .js >>>>> - We don't need this with the new implementation.C pgadmin/browser/se= rver_groups/servers/databases/schemas/static/js/child.js >>>>> - All the children of schema node have common properties as 'parent_t= ype', 'canDrop', 'canDropCascase', 'canCreate'. >>>>> Hence - instead of defining them in each node, we have created a ba= se 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_tre= e_node.js >>>>> - In this script, we're defining three functions 'childCreateMenuEnab= led', 'isTreeItemOfChildOfSchema', & 'isTreeNodeOfSchemaChild', which are u= sed 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/c= atalog node). >>>>> The file was not defintely placed under the wrong directory, becaus= e - the similar logic was under 'can_drop_child.js', and it was defined und= er 'pgadmin/browser/server_groups/servers/databases/schemas/static/js' dire= ctory.D pgadmin/static/js/menu/menu_enabled.jsC pgadmin/static/js/nodes/sup= ported_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/jav= ascript/fake_browser/browser.js >>>>> C regression/javascript/nodes/schema/child_menu_spec.js >>>>> - Modified the regression to test the new functionalies.M pgadmin/bro= wser/server_groups/servers/databases/schemas/**/*.js >>>>> - Extending the schema child nodes from the 'SchemaChildNode' class d= efined 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 li= ghter >>>>>>>> node, I don=E2=80=99t even see the need for an additional method t= o 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 will= come >>>>>>>> across more generic functions. At that point we can revisit this a= nd 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, whi= ch was >>>>>>>> happening because we have contextual menu. >>>>>>>> To give a better solution, I can think of putting the menus relate= d >>>>>>>> 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 *= application >>>>>>>> state* from *business domain* logic as much as we can - because >>>>>>>> this makes the code much easier to understand. We achieve lower co= upling 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 d= on=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 >>>>>>>> >>>>>>>> >>>>>>>> >>>>>>>> >>>>>>> >>>>> >> --00000000000088227a056df72d4b Content-Type: text/html; charset="UTF-8" Content-Transfer-Encoding: quoted-printable
Hi Anthony/Victoria/Joao,

I know I am v= ery late to review on patch 004. The idea of moving js files from tools to = static folder looks good, but I have a few suggestions:

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/resto= re/restore_dialog_wrapper';=C2=A0
by adding pgadmin_static al= ias to webpack config.

2) Few of the js are left b= ehind, the ones which are used in python __init__.py. Can't we move the= m 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 |=C2=A0EnterpriseDB Software Solutions |=C2= =A0Pune
"Don't Complain about Heat, Plant a tree"

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

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

Sincerely,

Victoria

On Fri, Jun 1, 2018 at 2:36 PM Ashesh V= ashi <ashesh.vashi@enterprisedb.com> wrote:
On Fri, Jun 1, 2018 at 10:09 PM, Vict= oria Henry <vhenry@pivotal.io> wrote:
<= /div>
Hi Ashesh,

We just attempted to apply your pa= tch over master but it did not work.=C2=A0 We don't want to introduce a= ny bugs or break any functionality.=C2=A0 Please update the patch to make s= ure it is synced up with the master branch.
Pl= ease 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 portabl= e.
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 scop= e).=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

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 _.i= sUndefined(obj) as this is more intuitive.

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

=
I am kind of disagree here. But - I have chan= ged it 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 Pere= ira <jdealmeidapereira@pivotal.io> wrote:
Hey, Thanks so much for t= he 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 change= s you have made - just so we can better understand the rationale behind the= m? 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.=
Dpgadmin/browser/server_groups/servers/databases/schemas/static/=
js/can_drop_child.js
- We don't need this with the new implementa=
tion.C pgadmin/browser/server_groups/servers/databases/schemas/static/js/child.js
- All the children of schema node ha=
ve common properties as 'parent_type', 'canDrop', 'canD=
ropCascase', 'canCreate'.
  Hence - instead of defining them in each node, we have create=
d a base node, which will have all these properties.
  And, modified=
 all schema children node to inherit from it.C pgadmin/browser/server_gro=
ups/servers/databases/schemas/static/js/schema_child_tree_no=
de.js
- In this script, we're defining three functions 'childCreateMenuEnabled', 'isTreeItemOfChildOfSchema', & 'isTreeNodeOfSchemaChild', which are used by the '=
Sche=
maChildNode' objects.M pgadmin/browser/static/js/collect=
ion.js
- Fixed an issue related to wrongly defined 'error' functi=
on for the Collection object.D pgadmin/static/js/menu/can_cr=
eate.js
- It defined the function, which was defining a check fo=
r 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 un=
der 'pgadmin/browser/server_grou=
ps/servers/databases/schemas/=
static/js' directory.D pgadmin/static/js/menu/menu_enable=
d.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 &=
#39;catalog' child).
- Finding the correct location was di=
fficult for this, as there is no defined pattern, also it can be used by ot=
her functions too. Hence - moved it out of 'pgadmin/static/js/menu'=
 directory.M pgadmin/static/js/t=
ree/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.js
C regression/javascript/fake_browser/browser.js
C regression/javasc=
ript/nodes/schema/child_menu_spec.js
- Modified the regre=
ssion to test the new functionalies.M pgadmin/browser/server_groups/servers/databases/=
schemas/**/*.js
- Extending the schema child nodes from the 'SchemaChildNode' cl=
ass defined in 'pgadmin/.../schemas/static/js/child.js' script.
Let me know if you need more informatio= n.
=C2=A0
=
Let's keep in mind that the original i= ntent 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 mak= ing to expand beyond that.

I ha= ve the mutual feeling.

-- Thanks, Ashesh=C2=A0
<= br>
Thanks
Joao && Anthony

=
On Thu, May 24, 2018 at 2:59 A= M Ashesh Vashi <ashesh.vashi@enterprisedb.com> wrote:
Sorry for the late r= eply.
On Wed, May 16, 2018 at 8:55 P= M, Anthony Emengo <aemengo@pivotal.io> wrote:
export function canCreate(pgBrowser, childOfCa=
talogType) {
  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.isS= upportedNode(=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

=






--00000000000088227a056df72d4b--