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 1fSFdS-0004PJ-DC for pgadmin-hackers@arkaria.postgresql.org; Mon, 11 Jun 2018 05:44:58 +0000 Received: from localhost ([127.0.0.1] helo=malur.postgresql.org) by malur.postgresql.org with esmtp (Exim 4.89) (envelope-from ) id 1fSFdQ-000272-Px for pgadmin-hackers@arkaria.postgresql.org; Mon, 11 Jun 2018 05:44:56 +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 1fSFdQ-00026o-GX for pgadmin-hackers@lists.postgresql.org; Mon, 11 Jun 2018 05:44:56 +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 1fSFdG-0000yP-6s for pgadmin-hackers@postgresql.org; Mon, 11 Jun 2018 05:44:55 +0000 Received: by mail-lf0-x229.google.com with SMTP id i83-v6so28601127lfh.5 for ; Sun, 10 Jun 2018 22:44:45 -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=khnJ4lUerREQ1RCdBTup1cyaURkYCcKv/BKM40JqvDg=; b=nMRET8zYA+bMzVweoIh2kI/HhvtBEiSt7LkhIKFlCk37qi4QV/+A+AZtU859g9pwwV 3Cy4brstFaFvymSdU1XOJNOC98GNj+okuWtvn+v1YcDo6Gxk/urSscAjWSPHgXqAgDUQ bsaSNKtgGZActQkfPKsZDBwUY7aZNhbo3DpwmJHvR7fD4vdFr0ICJCriKB3KWFsmRQ31 nhMRfaclZexXha0H1Rqco2kJJqRZCg20oBrUXnY72ANFF6+0msLV6OOQuFAgzNi/dAAt KK728kdcX8xIFJwIP9exYxmqLrRBRhSaKN4nbM0tEwMQZPOQv4RB/nZmdLxLlO/1w+Sn BwaQ== 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=khnJ4lUerREQ1RCdBTup1cyaURkYCcKv/BKM40JqvDg=; b=ZcBDD/oiNeES2w9gJIN3Ftl+zAYeygSLH1NxCIZWph+NhvsjL8xRRWpmaYTblkoR0W 56ZOQn78ITdwQoG4rTaI4cyX2sOp5mEK+T+toTDlmJybdr7J91ZHXPWaRVb45zWlQPJi wBMWoJWTpiKKO39YcYAZGRed17MoZdzAhkkL8sUAqxYoN7TZOanZTwhRGc9sKKgd771L HObN+jzEDMbDlsrB8kLx7LITWcP+WME617pYLzGyp3VHdT2a9pd2DNuUfE3xnWHcUX7e eskw0eLsmqnXY9f44/YxEc/SDsR3CiYSXef9BqzfGUi2rZ8ag+9nvjVZQVyINHcr0A6o yEMA== X-Gm-Message-State: APt69E1xLKAiQKi3eSH4r7GBjPI50IIZRqW9N5ew/fisHd+cDb+vnpxk 69sWu9/Gbgl1B5MBc/Q0vuSXsxiiSv6+4P9sP/f9/A== X-Google-Smtp-Source: ADUXVKJ074QHy3Hb0lqo4Etb4gfIvxwtsOFURDsP43bcRL/CB6I/5rBABypU+eL3An8pM5HvEe9PRmGLHDPikvzmlHk= X-Received: by 2002:a2e:750d:: with SMTP id q13-v6mr8283452ljc.56.1528695884932; Sun, 10 Jun 2018 22:44:44 -0700 (PDT) MIME-Version: 1.0 Received: by 2002:a2e:7a02:0:0:0:0:0 with HTTP; Sun, 10 Jun 2018 22:44:44 -0700 (PDT) In-Reply-To: References: From: Aditya Toshniwal Date: Mon, 11 Jun 2018 11:14:44 +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="00000000000030fd73056e573fab" List-Id: List-Help: List-Subscribe: List-Post: List-Owner: List-Archive: Precedence: bulk --00000000000030fd73056e573fab Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable Hi Victoria, On Thu, Jun 7, 2018 at 8:51 PM, Victoria Henry wrote: > 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 > And this is another js which is not moved.(sqleditor.js). If we are moving js files, why not this too. > Sincerely, > > Victoria > =E2=80=8B > > > > On Thu, Jun 7, 2018 at 12:35 AM 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 leav= e >>>> 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/datagri= d.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 >>>> files from tools to static folder looks good, but I have a few suggest= ions: >>>> >>>> 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 leav= e >>>> 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. Bu= t >>>>> 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 functionali= ty. >>>>>>> Please update the patch to make sure it is synced up with the maste= r 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/j= s/child.js >>>>>>>> C pgadmin/browser/server_groups/servers/databases/schemas/static/j= s/schema_child_tree_node.js >>>>>>>> >>>>>>>> It makes sense to remove duplication by extracting these attribute= s >>>>>>>> out and setting the canDrop and canCreate functions here. But is >>>>>>>> it possible to combine these two files into one since they are rel= ated 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 sco= pe). >>>>>> >>>>>>> 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 t= he >>>>>>>> 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 retur= n >>>>>>>> obj =3D=3D=3D undefined or return _.isUndefined(obj) as this is mo= re >>>>>>>> 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 go= od! >>>>>>>> >>>>>>> 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 f= ollow. >>>>>>>>>> Could we please get a brief synopsis of the changes you have mad= e - 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 ne= ed to use 'pgbrowser/browser' in the 'schema_child_tree_node.js' script.D p= gadmin/browser/server_groups/servers/databases/schemas/static/js/can_drop_c= hild.js >>>>>>>>> - We don't need this with the new implementation.C pgadmin/browse= r/server_groups/servers/databases/schemas/static/js/child.js >>>>>>>>> - All the children of schema node have common properties as 'pare= nt_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 pga= dmin/browser/server_groups/servers/databases/schemas/static/js/schema_child= _tree_node.js >>>>>>>>> - In this script, we're defining three functions 'childCreateMenu= Enabled', 'isTreeItemOfChildOfSchema', & 'isTreeNodeOfSchemaChild', which a= re used by the 'SchemaChildNode' objects.M pgadmin/browser/static/js/collec= tion.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 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 directory, be= cause - the similar logic was under 'can_drop_child.js', and it was defined= under 'pgadmin/browser/server_groups/servers/databases/schemas/static/js' = directory.D pgadmin/static/js/menu/menu_enabled.jsC pgadmin/static/js/nodes= /supported_database_node.js >>>>>>>>> - Used by the external 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 i= s no defined pattern, also it can be used by other functions too. Hence - m= oved 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_spe= c.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' cla= ss 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 en= ough task. >>>>>>>>>> I'd hate for the scope of the changes we're making to expand bey= ond 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 meth= od 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 i= t >>>>>>>>>>>> in for testing the tree data. Please suggest me the right plac= e, 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 th= is and create >>>>>>>>>>>> a utils.js file. >>>>>>>>>>>> >>>>>>>>>>> Sure. >>>>>>>>>>> >>>>>>>>>>>> The original patch was separating them in different places, bu= t >>>>>>>>>>>> - still uses some of the functionalities directly from the tre= e, 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 di= rectory. >>>>>>>>>>>> >>>>>>>>>>>> We=E2=80=99re particularly worried because we=E2=80=99re tryin= g to avoid the >>>>>>>>>>>> coupling that we see in the code base today. We want to decoup= le *application >>>>>>>>>>>> state* from *business domain* logic as much as we can - >>>>>>>>>>>> because this makes the code much easier to understand. We achi= eve lower >>>>>>>>>>>> coupling by have more suitable interfaces to retrieve *applica= tion >>>>>>>>>>>> state* like: anyParent (the menu doesn=E2=80=99t care how this >>>>>>>>>>>> happens). This is the direction that we=E2=80=99re trying to m= ove towards, we just >>>>>>>>>>>> don=E2=80=99t want the package structure to undermine that dev= eloper intent. >>>>>>>>>>>> >>>>>>>>>>> I realized after revisiting the code, menu/can_create.js was >>>>>>>>>>> only applicable to the children of the schema/catalog nodes, sa= me 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 >>>>>>>>>>>> >>>>>>>>>>>> >>>>>>>>>>>> >>>>>>>>>>>> >>>>>>>>>>> >>>>>>>>> >>>>>> >>>> >> --00000000000030fd73056e573fab Content-Type: text/html; charset="UTF-8" Content-Transfer-Encoding: quoted-printable
Hi Victoria,
<= div dir=3D"ltr">

On Thu, Jun 7, 2018 at 8:51 PM, Victoria Henry <vhenry@pivota= l.io> wrote:

Hi Aditya

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

Generally speaking, I ag= ree 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

And this is ano= ther js which is not moved.(sqleditor.js). If we are moving js files, why n= ot this too.
=C2=A0

Sincerely,

Victoria

=E2=80=8B



On Thu, Jun 7, 2018 at 12:35 AM Aditya Toshniwal <<= a href=3D"mailto:aditya.toshniwal@enterprisedb.com" target=3D"_blank">adity= a.toshniwal@enterprisedb.com> wrote:
Hi Victoria,
=

On Wed, Jun 6, 2018 at 8:55 PM, Victoria Henry <vhe= nry@pivotal.io> wrote:
Hi Aditya,

1) Why don't we start using webpack alias= 9;s instead of using absolute path. For eg,
import {RestoreDialogWrapper= } from '../../../pgadmin/static/js/restore/restore_dialog_wra= pper';
can be used as import {RestoreDialogWrapper} from 'pgadmi= n_static/js/restore/restore_dialog_wrapper';=C2=A0
by adding pg= admin_static alias to webpack config.

Gre= at point. In some areas of the code, we began making this change.=C2=A0 The= re is already an alias in webpack shims for `../../../pg= admin/static/js` called `sources`.=C2=A0 You can find an example of = this in import statements for `supported_database_node.js`

2) Few of th= e js are left behind, the ones which are used in python __init__.py. Can= 9;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 Coul= d 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.

<= /div>
Sincerely,

Victoria

On Wed, Jun 6, 2018 at 7:07 AM Aditya Toshniwal &l= t;ad= itya.toshniwal@enterprisedb.com> wrote:
Hi Anthony/Victoria/Jo= ao,

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

1) Why don't we start using w= ebpack alias's instead of using absolute path. For eg,
import= {RestoreDialogWrapper} from '../../../pgadmin/static/js/restore/r= estore_dialog_wrapper';
can be used as import {RestoreDi= alogWrapper} from 'pgadmin_static/js/restore/restore_dialog_wrappe= r';=C2=A0
by adding pgadmin_static alias to webpack config.

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

Kindly let m= e know your views on this.


Thanks and Regards,
Aditya Toshniwal
= Software Engineer |=C2=A0Enterp= riseDB Software Solutions |=C2=A0Pune
"Don't Complain abo= ut 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@enterpri= sedb.com> wrote:
On Fri, Jun 1, 2018 at 10:09 PM, Victoria Henry <vhenry@= pivotal.io> wrote:
Hi Ashesh,

We just attempted to apply your pat= ch over master but it did not work.=C2=A0 We don't want to introduce an= y bugs or break any functionality.=C2=A0 Please update the patch to make su= re it is synced up with the master branch.
Ple= ase find the updated patch.

Sincerely,

Victoria

On Fri, Jun 1, 2018 at 1= 1: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 example, instead of return !!obj, we could do something like return obj =3D=3D=3D undefined or return _.isUndef= ined(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 specify the fake_browser in regress=
ion tests, we need to use 'pgbrowser/browser' in the 'schema_child_tree_node.js' scri=
pt.D pgadmin/browser/server_groups/servers/databases/sche=
mas/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 have common properties as =
'parent_type', 'canDrop', 'canDropCascase', 'ca=
nCreate'.
  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/browser/s=
erver_groups/servers/databases/schemas/static/js/schema_child_tre=
e_node.js
- In this script, we're defining three f=
unctions 'childCreateMenuEnabled&#=
39;, 'isTreeItemOfChildOfSchema', &am=
p; 'isTreeNodeOfSchemaChi=
ld', which are=
 used by the 'SchemaChildNode' objects.M pgadmin/browser=
/static/js/collection.js
- Fixed an issue related t=
o wrongly defined 'error' function for the Collection object.D pgadmin/static/js/menu=
/can_create.js
-=
 It defined the function, which was defining a check for creation of a sche=
ma 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 under=
 'pgadmin/browser/server_groups/=
servers/databases/schemas/static/js' directory.D pgadmin/static/js/menu/menu_enabled.j=
sC pgadmin/static/js/nodes/support=
ed_database_node.js
- Used by the external tools for checki=
ng 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 fo=
r this, as there is no defined pattern, also it can be used by other functi=
ons too. Hence - moved it out of 'pgadmin/static/js/menu' directory=
.M pgadmin/static/js/tree/tree.j=
s
- 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/javascript/nodes/schema/child_menu_spec.js
- Modified the regression to test the new functionalies.M pgadmin/browser/se=
rver_groups/servers/databases/schemas/**/*.js
- E=
xtending the schema child nodes from the '<=
/font>SchemaChildNode' class defined in 'pgadmin/...=
/=
schemas/static/js/child.js' script.
=C2=A0
<= /div>
Let's keep in mind that the original intent was simply to int= roduce this abstraction into the code base, which is a big enough task. I&#= 39;d hate for the scope of the changes we're making to expand beyond th= at.

I have the mutual feeling.<= /div>

-- Thanks, Ashesh=C2=A0

Thanks
Joao && Anthony


Sorry for the late reply.
On We= d, May 16, 2018 at 8:55 PM, Anthony Emengo <aemengo@pivotal.io> wrote:
= 
exp=
ort function canCreate(pgBrowser, childOfCatalogType) {
  return canCreateObject.bind({
    browser: pgBrowser,
    childOfCatalogType: childOfCatalogType,
  });
}

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

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

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

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

Sure.=C2=A0

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

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

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

Please find the updated patch for the= same.

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

-- Thanks, Ashesh

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

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

Thanks
Anthony && Victoria

=E2=80=8B

=








--00000000000030fd73056e573fab--