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 1fQaJn-0001l1-0a for pgadmin-hackers@arkaria.postgresql.org; Wed, 06 Jun 2018 15:25:47 +0000 Received: from localhost ([127.0.0.1] helo=malur.postgresql.org) by malur.postgresql.org with esmtp (Exim 4.89) (envelope-from ) id 1fQaJl-0001eO-0r for pgadmin-hackers@arkaria.postgresql.org; Wed, 06 Jun 2018 15:25:45 +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 1fQaJk-0001eE-Nn for pgadmin-hackers@lists.postgresql.org; Wed, 06 Jun 2018 15:25:44 +0000 Received: from mx0b-00296801.pphosted.com ([148.163.153.148]) by magus.postgresql.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_CBC_SHA384:256) (Exim 4.89) (envelope-from ) id 1fQaJX-00058T-3V for pgadmin-hackers@postgresql.org; Wed, 06 Jun 2018 15:25:40 +0000 Received: from pps.filterd (m0114586.ppops.net [127.0.0.1]) by mx0b-00296801.pphosted.com (8.16.0.22/8.16.0.22) with SMTP id w56FLjlK012594 for ; Wed, 6 Jun 2018 15:25:27 GMT Received: from mail-it0-f69.google.com (mail-it0-f69.google.com [209.85.214.69]) by mx0b-00296801.pphosted.com with ESMTP id 2jeb8bg9us-1 (version=TLSv1.2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128 verify=NOT) for ; Wed, 06 Jun 2018 15:25:26 +0000 Received: by mail-it0-f69.google.com with SMTP id r7-v6so5344227ith.5 for ; Wed, 06 Jun 2018 08:25:26 -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=KuaOHaUYSBtMPgmCt12DqpXnFzxTcrEdYtvEDrVpd4w=; b=VEU9R5lv4O0NAl6+jVrGcu9n1/ulybFwY8hw6rwaT+wFiSN+AKjYtIPulChwE924jr eGmfJxQNY9a5IKOIV6wukPYLsABt+UBIAHgpKaO9c6SnB9HwVSu7HxCEdEZOnmIQRsDH N7HDuL1lR+3udiq5mgE2CbPU02tlagdRtmr+akqfOymLLhY+o7EvRX6XYxy0xPAwLvzm IOEvBNyKTiNEyfDBDe8ykzOqtrd4pl9k8ufjPyYPC+d5ylayS6Dqpi2mTqMk9p/rTU0P ceNkyA2wkp7f7jgtNL1mIw86/1YCKVUHP1FSFR6n/aFyHJqmSxHkN3ifX3K7bBFQHlhY MnSA== X-Gm-Message-State: APt69E2pSlofVdOS/5iGkx9Ji6T/9HnXnytK9Aq6I9xEhoF1PuBgWDvB 1wWqDCfJY1jltl1hKNGBprS6wFCwaPWSag4Pythz14nfV9Um2CIGVYLc6oy9Br2avp5Q5EdOSZV Y1gyHI+9GnBTNlhw61f8Nu3lHozkf833F5dBA7Azg5FEu1R4LXd0pJ9fm3ycJgc3WYV1h X-Received: by 2002:a6b:2653:: with SMTP id m80-v6mr3304115iom.301.1528298725582; Wed, 06 Jun 2018 08:25:25 -0700 (PDT) X-Google-Smtp-Source: ADUXVKI4ej8ieGwJ1CP91u35jgGMI2b5mZFecxRMEi0TASyG84cut4FLJ6RFvDEZzjFa66pwcOjMD0WmiDgKGCwiU3U= X-Received: by 2002:a6b:2653:: with SMTP id m80-v6mr3304071iom.301.1528298725134; Wed, 06 Jun 2018 08:25:25 -0700 (PDT) MIME-Version: 1.0 References: In-Reply-To: From: Victoria Henry Date: Wed, 6 Jun 2018 11:25:13 -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="0000000000009f6fba056dfac68b" X-Proofpoint-Virus-Version: vendor=fsecure engine=2.50.10434:,, definitions=2018-06-06_07:,, 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-1806060175 List-Id: List-Help: List-Subscribe: List-Post: List-Owner: List-Archive: Precedence: bulk --0000000000009f6fba056dfac68b Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable 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 js file? 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 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. Pleas= e >>>> 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/c= hild.js >>>>> C pgadmin/browser/server_groups/servers/databases/schemas/static/js/s= chema_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 s= o 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 f= or 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 pgad= min/browser/server_groups/servers/databases/schemas/static/js/can_drop_chil= d.js >>>>>> - We don't need this with the new implementation.C pgadmin/browser/s= erver_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 b= ase node, which will have all these properties. >>>>>> And, modified all schema children node to inherit from it.C pgadmi= n/browser/server_groups/servers/databases/schemas/static/js/schema_child_tr= ee_node.js >>>>>> - In this script, we're defining three functions 'childCreateMenuEna= bled', 'isTreeItemOfChildOfSchema', & 'isTreeNodeOfSchemaChild', which are = used by the 'SchemaChildNode' objects.M pgadmin/browser/static/js/collectio= n.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 o= f 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, becau= se - the similar logic was under 'can_drop_child.js', and it was defined un= der 'pgadmin/browser/server_groups/servers/databases/schemas/static/js' dir= ectory.D pgadmin/static/js/menu/menu_enabled.jsC pgadmin/static/js/nodes/su= pported_database_node.js >>>>>> - Used by the external tools for checking whether the 'selected' tre= e-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 n= o defined pattern, also it can be used by other functions too. Hence - move= d it out of 'pgadmin/static/js/menu' directory.M pgadmin/static/js/tree/tre= e.js >>>>>> - Introduced a function, which returns the ancestor node object, fow= which the condition is true.D regression/javascript/menu/can_create_spec.j= s >>>>>> D regression/javascript/menu/menu_enabled_spec.js >>>>>> D regression/javascript/schema/can_drop_child_spec.jsC regression/ja= vascript/fake_browser/browser.js >>>>>> C regression/javascript/nodes/schema/child_menu_spec.js >>>>>> - Modified the regression to test the new functionalies.M pgadmin/br= owser/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 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 l= ighter >>>>>>>>> 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 i= n >>>>>>>>> for testing the tree data. Please suggest me the right place, and= name. >>>>>>>>> >>>>>>>>> We=E2=80=99re not sure; maybe after continued refactoring, we wil= l 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, wh= ich 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 direc= tory. >>>>>>>>> >>>>>>>>> We=E2=80=99re particularly worried because we=E2=80=99re trying t= o 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 c= oupling 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 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 >>>>>>>>> >>>>>>>>> >>>>>>>>> >>>>>>>>> >>>>>>>> >>>>>> >>> > --0000000000009f6fba056dfac68b Content-Type: text/html; charset="UTF-8" Content-Transfer-Encoding: quoted-printable
Hi Aditya,

1) Why don't we start using webpack alias's i= nstead of using absolute path. For eg,
import {RestoreDialogWrapper} fro= m '../../../pgadmin/static/js/restore/restore_dialog_wrapper';
c= an be used as import {RestoreDialogWrapper} from 'pgadmin_static/js/res= tore/restore_dialog_wrapper';=C2=A0
by adding pgadmin_static alias t= o webpack config.

Great point. In some areas of = the code, we began making this change.=C2=A0 There is already an alias in w= ebpack shims for `../../../pgadmin/static/js` cal= led `sources`.=C2=A0 You can find an example of this in import statements f= or `supported_database_node.js`

2) Few of the js are left beh= ind, 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 exampl= e of a single js file?

Sincerely,

Victoria

On Wed, Jun 6, 2018 at 7:07 AM Aditya Toshniwal <aditya.toshniwal@enterprisedb.com> w= rote:
Hi Anthony/V= ictoria/Joao,

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

1) Why don't we sta= rt using webpack alias's instead of using absolute path. For eg,
<= div>import {RestoreDialogWrapper} from '../../../pgadmin/static/js/rest= ore/restore_dialog_wrapper';
can be used as import {RestoreDi= alogWrapper} from 'pgadmin_static/js/restore/restore_dialog_wrapper'= ;;=C2=A0
by adding pgadmin_static alias to webpack config.
<= div>
2) Few of the js are left behind, the ones which are use= d 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 kno= w your views on this.

=
= Thanks= and Regards,
= Aditya= Toshniwal
Software Engineer |=C2=A0EnterpriseDB Software Solutions |=C2=A0= Pune
"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 Vashi <ashesh.vashi@enterprisedb.com<= /a>> wrote:
Hi = Ashesh,

We just attempted to apply your patch over maste= r but it did not work.=C2=A0 We don't want to introduce any bugs or bre= ak any functionality.=C2=A0 Please update the patch to make sure it is sync= ed up with the master branch.
Please find the = updated patch.
Sincerely,

Victoria

On Fri, Jun 1, 2018 at 11:18 AM Anthony Emengo= <aemengo@pivota= l.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 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).= =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, cou= ld we avoid the !! syntax when possible? For example, instead of return !!obj, we could do something like re= turn obj =3D=3D=3D undefined or return _.isUndefine= d(obj) as this is more intuitive.

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

I am kind of disagree here. But - I have changed i= t 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 Pereira <= span dir=3D"ltr"><jdealmeidapereira@pivotal.io> wrote:
Hey, Thanks so much for the repl= y.

We've noticed that you've made several modifi= cations on top of our original patch. Unfortunately, we've found it ver= y hard to follow. Could we please get a brief synopsis of the changes you h= ave made - just so we can better understand the rationale behind them? Just= like we've done for you previously.
= Please find the changes from your original patch:
M w=
ebpack.shim.js
M webpack.test.config.js
=
- In order to specify the fake_browser in re=
gression tests, we need to use 'pgbrowser/browser' in the 'schema_child_tree_node.js'=
 script.D pgadmin/browser/server_groups/servers/databases/sc=
hemas/static/js/can_drop_child.js
- We don'=
t need this with the new implementation.=
C pgadmin/browser/server_groups/servers/dat=
abases/schemas/static/js/child.js
- All the children of schema node have common properties as '=
;parent_type', 'canDrop', 'canDropCascase', 'canCre=
ate'.
  Hence - instead of defining them in each node, we have created a bas=
e node, which will have all these properties.
=
  And, modified all schema children node to inherit from it.<=
/font>C pgadmin/bro=
wser/server_groups/servers/databases/schemas/static/js/schema_child_tree_no=
de.js
- In this script, we're defining three funct=
ions 'childCreateMenuEnabled',=
 'isTreeItemOfChildOfSchema', & &=
#39;=
=
isTreeNodeOfSchemaChild&#=
39;, which are use=
d by the 'SchemaChildNode' objects.<=
span style=3D"color:rgb(191,144,0);font-family:Consolas,Inconsolata,Courier=
,monospace;font-size:12.5538px;font-style:normal;font-variant-ligatures:nor=
mal;font-variant-caps:normal;font-weight:400;letter-spacing:normal;text-ali=
gn:start;text-indent:0px;text-transform:none;white-space:pre-wrap;word-spac=
ing:0px;background-color:rgb(248,248,248);text-decoration-style:initial;tex=
t-decoration-color:initial;float:none;display:inline">M pgadmin/browser/sta=
tic/js/collection.js<=
font color=3D"#0b5394">
- Fixed an issue related to wr=
ongly defined 'error' function for the Collection object.D pgadmin/static/js/menu/can=
_create.js
- I=
t 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 no=
de).
=
  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_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 t=
he 'selected' tree-node is:
  + 'da=
tabase' 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 fun=
ctions too. Hence - moved it out of 'pgadmin/static/js/menu' direct=
ory.M pgadmin/static/js/tree/tre=
e.js
=
- Introduced a function, which returns the ancest=
or 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- Extend=
ing the schema child nodes from the 'SchemaChildNode' class defined in 'pgadmin/.../schem=
as/static/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 thi= s 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.
<= /div>

I have the mutual feeling.
=
-- Thanks, Ashesh=C2=A0

Thanks
Jo= ao && Anthony


On Thu, May 24, 201= 8 at 2:59 AM Ashesh Vashi <ashesh.vashi@enterprisedb.com> wrote:
Sorry for the late reply.
On We= d, 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)?

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

=






--0000000000009f6fba056dfac68b--