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 1fQme0-0000PI-3A for pgadmin-hackers@arkaria.postgresql.org; Thu, 07 Jun 2018 04:35:28 +0000 Received: from localhost ([127.0.0.1] helo=malur.postgresql.org) by malur.postgresql.org with esmtp (Exim 4.89) (envelope-from ) id 1fQmdx-0002Uz-LD for pgadmin-hackers@arkaria.postgresql.org; Thu, 07 Jun 2018 04:35:25 +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 1fQmdx-0002Um-5k for pgadmin-hackers@lists.postgresql.org; Thu, 07 Jun 2018 04:35:25 +0000 Received: from mail-lf0-x229.google.com ([2a00:1450:4010:c07::229]) by magus.postgresql.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_CBC_SHA1:256) (Exim 4.89) (envelope-from ) id 1fQmdr-00075w-Re for pgadmin-hackers@postgresql.org; Thu, 07 Jun 2018 04:35:24 +0000 Received: by mail-lf0-x229.google.com with SMTP id n3-v6so12448937lfe.12 for ; Wed, 06 Jun 2018 21:35:19 -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=b6JdlHCSsOaxKxNPJ7oC3TRMmzCYjbx0tJfPPWtQzRc=; b=nXXns270kgmGIyadbl9X+TYbP4BvDtiX29UzeScfsjXmRGdDeP/DhcRmMZ0D5x4YU2 UlAW/iVWQkAAIq3AlgL3no6zPswwEJfwzcmq2lbrmz+kpUGSqrHXg8yTtUGAeLJI1EmO wcbCxM+Zg7Dvp6GDN8XooyTA55hIO6DThaZ/otDS5KzqbWTquC/naKuRGmZeYLhBDY4q 8GkQqU19Ye+xN6OU52k3uPR8jouthEOar9PXfi8CachlAvczOyrz+qhMiRPUAsjajh2W EzA9O2AE7QmXOHdi3Gihe1VCF/QWLAuqu5nhhHj3KN3K2TIBsRUCvbFUmfR7NIC4jH5Y +6Tg== 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=b6JdlHCSsOaxKxNPJ7oC3TRMmzCYjbx0tJfPPWtQzRc=; b=NWZWeX3oPifsnojvBJsQuN8H9zIiiGjjK7hJiJZ+Wep8Z4PWYt4kOevlHTcFGJZORX 0iNxt5/ckK0ZoPTmxI/x/giimnecV+a4JwcBkvoOAGc3wtMahBoIq0QAjioGkxG53z8U b0AS5vDR+tvPYdgrSYPZzup1EDohlJdzwyPno7AStLdIrR9yOP/E+MvVEu7F2cxlEhU9 cTGMmFhmJBzB67U/5h5aFGVbNpex9KRhYgSekARufZjAFXI+qWRffW2/ZVASUAjOD9e5 reqQOENCdYqvnLcoVmR643d8UOrrxdkWdS6lb0J1bxGaaAq/yormivZAvn1XKGvLRFco e2/Q== X-Gm-Message-State: APt69E211T9BqbWNl/oQ4sQsRss1GSDfKU7LDwHU8rg08wc4LvgrFEjZ VbJu79ZwLc4JYnKsPQ/NXp+6ilToXMHKzyuAUs025A== X-Google-Smtp-Source: ADUXVKJY/7flq7++qYhIXMMrW6WApKC6nPGNHdN+uqg6iOspBcD8zt+QMEcDCaPQiAvqd4qJpNnSnfEMWQilx01F/Nk= X-Received: by 2002:a2e:2ac3:: with SMTP id q186-v6mr218889ljq.44.1528346117660; Wed, 06 Jun 2018 21:35:17 -0700 (PDT) MIME-Version: 1.0 Received: by 2002:a2e:9e8a:0:0:0:0:0 with HTTP; Wed, 6 Jun 2018 21:35:16 -0700 (PDT) In-Reply-To: References: From: Aditya Toshniwal Date: Thu, 7 Jun 2018 10:05:16 +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="0000000000006ff4f8056e05cfea" List-Id: List-Help: List-Subscribe: List-Post: List-Owner: List-Archive: Precedence: bulk --0000000000006ff4f8056e05cfea Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable 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/j= s` > called `sources`. You can find an example of this in import statements f= or > `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 js > 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 enterprisedb.com> wrote: > >> Hi Anthony/Victoria/Joao, >> >> I know I am very late to review on patch 004. The idea of moving js file= s >> 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 >>> 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 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/= 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 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.= Could we >>>>>>>> please get a brief synopsis of the changes you have made - just so= we can >>>>>>>> better understand the rationale behind them? Just like we've done = for you >>>>>>>> previously. >>>>>>>> >>>>>>> Please find the changes from your original patch: >>>>>>> >>>>>>> M webpack.shim.js >>>>>>> M webpack.test.config.js >>>>>>> - In order to specify the fake_browser in regression tests, we need= to use 'pgbrowser/browser' in the 'schema_child_tree_node.js' script.D pga= dmin/browser/server_groups/servers/databases/schemas/static/js/can_drop_chi= ld.js >>>>>>> - We don't need this with the new implementation.C pgadmin/browser/= server_groups/servers/databases/schemas/static/js/child.js >>>>>>> - All the children of schema node have common properties as 'parent= _type', 'canDrop', 'canDropCascase', 'canCreate'. >>>>>>> Hence - instead of defining them in each node, we have created a = base node, which will have all these properties. >>>>>>> And, modified all schema children node to inherit from it.C pgadm= in/browser/server_groups/servers/databases/schemas/static/js/schema_child_t= ree_node.js >>>>>>> - In this script, we're defining three functions 'childCreateMenuEn= abled', 'isTreeItemOfChildOfSchema', & 'isTreeNodeOfSchemaChild', which are= used by the 'SchemaChildNode' objects.M pgadmin/browser/static/js/collecti= on.js >>>>>>> - Fixed an issue related to wrongly defined 'error' function for th= e Collection object.D pgadmin/static/js/menu/can_create.js >>>>>>> - It defined the function, which was defining a check for creation = of a schema child node, or not by looking at the parent node (i.e. a schema= /catalog node). >>>>>>> The file was not defintely placed under the wrong directory, beca= use - the similar logic was under 'can_drop_child.js', and it was defined u= nder 'pgadmin/browser/server_groups/servers/databases/schemas/static/js' di= rectory.D pgadmin/static/js/menu/menu_enabled.jsC pgadmin/static/js/nodes/s= upported_database_node.js >>>>>>> - Used by the external tools for checking whether the 'selected' tr= ee-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 - mov= ed it out of 'pgadmin/static/js/menu' directory.M pgadmin/static/js/tree/tr= ee.js >>>>>>> - Introduced a function, which returns the ancestor node object, fo= w 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/j= avascript/fake_browser/browser.js >>>>>>> C regression/javascript/nodes/schema/child_menu_spec.js >>>>>>> - Modified the regression to test the new functionalies.M pgadmin/b= rowser/server_groups/servers/databases/schemas/**/*.js >>>>>>> - Extending the schema child nodes from the 'SchemaChildNode' class= defined in 'pgadmin/.../schemas/static/js/child.js' script. >>>>>>> >>>>>>> Let me know if you need more information. >>>>>>> >>>>>>> >>>>>>>> Let's keep in mind that the original intent was simply to introduc= e >>>>>>>> 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 < >>>>>>>>> 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 and >>>>>>>>>> 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 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 name. >>>>>>>>>> >>>>>>>>>> We=E2=80=99re not sure; maybe after continued refactoring, we wi= ll come >>>>>>>>>> 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, w= hich 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 dire= ctory. >>>>>>>>>> >>>>>>>>>> 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 = 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 j= ust 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 >>>>>>>>>> >>>>>>>>>> >>>>>>>>>> >>>>>>>>>> >>>>>>>>> >>>>>>> >>>> >> --0000000000006ff4f8056e05cfea Content-Type: text/html; charset="UTF-8" Content-Transfer-Encoding: quoted-printable
Hi Victoria,

On Wed, Jun 6, 2018 at 8:55 PM, V= ictoria Henry <vhenry@pivotal.io> wrote:
Hi Aditya,

1) Why don't we start using webpack alias's instead of using ab= solute path. For eg,
import {RestoreDialogWrapper} from '../../../pg= admin/static/js/restore/restore_dialog_wrapper';
can be us= ed as import {RestoreDialogWrapper} from 'pgadmin_static/js/restore/restore_dialog_wrapper';=C2=A0
by adding pgadmin_static alias to w= ebpack 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 statemen= ts 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.= =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 <<= a href=3D"mailto:aditya.toshniwal@enterprisedb.com" target=3D"_blank">adity= a.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= suggestions:

1) Why don't we start using webp= ack alias's instead of using absolute path. For eg,
import {R= estoreDialogWrapper} from '../../../pgadmin/static/js/restore/rest= ore_dialog_wrapper';
can be used as import {RestoreDialo= gWrapper} from 'pgadmin_static/js/restore/restore_dialog_wrapper&#= 39;;=C2=A0
by adding pgadmin_static alias to webpack config.

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

Kindly let me k= now your views on this.


Thanks and = Regards,
Aditya Tosh= niwal
S= oftware 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,

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

On Fri, Jun 1, 2018 at 2:36 PM Ashesh Vashi <ashesh.vashi@enterprisedb.com<= /a>> wrote:
Hi Ashesh,

We just at= tempted to apply your patch over master but it did not work.=C2=A0 We don&#= 39;t want to introduce any bugs or break any functionality.=C2=A0 Please up= date the patch to make sure it is synced up with the master branch.
Please find the updated patch.

Since= rely,

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 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&#= 39; 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 enou= gh test coverage (which is more imprortant than changing the scope).=C2=A0<= /div>
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 lighter note, could we avoid the !! syntax when possible? For ex= ample, instead of return !!obj, we could do somethi= ng 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 any= way.=C2=A0

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<= /a>> wrote:
M webpack.shim.js
M webpack.test.config.js
- In order to sp=
ecify the fake_browser in regression tests, we need to use 'pgbrowser/b=
rowser' in the 'schema_child_tree_node.js' script.D pgadmin/browser/server_g=
roups/servers/databases/schemas/static/js/can_drop_child.js
- We don't need this=
 with the new implementation.C pgadmin/=
browser/server_groups/servers/databases=
/schemas/static/js/child.js
- All the children of schema node ha=
ve common properties as 'parent_type', 'canDrop', 'canD=
ropCascase', 'canCreate'.
  Hence - ins=
tead 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 p=
gadmin/browser/server_groups/servers/databases/schemas/static/js/=
schema_child_tree_node.js
- In this script, we're defining three functions 'childCreateMenuEnabled', 'isTreeItemOfChildOfSchema', & 'isTreeNodeOfSchemaChild', which are used by the '=
Sche=
maChildNode' objects.M pgadmin/browser/static/js/collect=
ion.js
- Fixed an issue=
 related to wrongly defined 'error' function for the Collection obj=
ect.D pgadmin/stat=
ic/js/menu/can_create.js
- It defined the function, which =
was defining a check for creation of a schema child node, or not by looking=
 at the parent node (i.e. a schema/catalog node).  The file was not defintely placed under th=
e 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
- U=
sed by the external tools for checking whether the 'selected' tree-=
node is:
  + 'd=
atabase' 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 difficult for this, as th=
ere is no defined pattern, also it can be used by other functions too. Henc=
e - moved it out of 'pgadmin/static/js/menu' directory.M pgadmin/static/js/tree/tree.js
- Introduced a function, which retur=
ns the ancestor node object, fow which the condition is true.=
D regression/javascript/menu/ca=
n_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/ch=
ild_menu_spec.js
- Modified the regression to test the new functionalies.=
M pgadmin/browser/server_grou=
ps/servers/databases/schemas/**/*.js<=
br class=3D"gmail-m_-7259432427898418545m_-8307064080202625060m_83826618249=
50193576m_3270516565853123642m_-7492672409753239692m_1558859882629566770m_-=
7804285449525201396m_-3582030928604831699gmail-Apple-interchange-newline" s=
tyle=3D"color:rgb(191,144,0);font-family:Consolas,Inconsolata,Courier,monos=
pace;font-size:12.5538px;font-style:normal;font-variant-ligatures:normal;fo=
nt-variant-caps:normal;font-weight:400;letter-spacing:normal;text-align:sta=
rt;text-indent:0px;text-transform:none;white-space:pre-wrap;word-spacing:0p=
x;text-decoration-style:initial;text-decoration-color:initial">- Extending the schema child nodes from the 'SchemaChildNode' c=
lass defined in 'pgadmin/.../schemas/static/js/child.js' script.<=
/font>


-- Thanks, Ashesh=C2=A0
=
Thanks
Joao && Anthony


Sorry for the late rep= ly.
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.isSupportedNode(=E2=80=A6)?<= /font>

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

=







--0000000000006ff4f8056e05cfea--