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.va...@enterprisedb.com> wrote: > On Fri, Jun 1, 2018 at 10:09 PM, Victoria Henry <vhe...@pivotal.io> 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 <aeme...@pivotal.io> >> 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’t 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’s 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 >>> === 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 > >> >>> >>> Thanks >>> Anthony && Victoria >>> >>> On Fri, Jun 1, 2018 at 7:52 AM Ashesh Vashi < >>> ashesh.va...@enterprisedb.com> wrote: >>> >>>> On Thu, May 24, 2018 at 8:13 PM, Joao De Almeida Pereira < >>>> jdealmeidapere...@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 >>>> pgadmin/browser/server_groups/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 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 >>>> pgadmin/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 '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/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.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 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/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' 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.va...@enterprisedb.com> wrote: >>>>> >>>>>> Sorry for the late reply. >>>>>> On Wed, May 16, 2018 at 8:55 PM, Anthony Emengo <aeme...@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’t 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’t 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’re not sure; maybe after continued refactoring, we will 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, which was >>>>>>> happening because we have contextual menu. >>>>>>> To give a better solution, I can think of putting the menus related >>>>>>> code understand ‘sources/tree/menu’ directory. >>>>>>> >>>>>>> We’re particularly worried because we’re 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’t care how this happens). This is the >>>>>>> direction that we’re trying to move towards, we just don’t 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(…)? >>>>>>> >>>>>>> Naming is one of the hardest problems in programming. I don’t feel >>>>>>> too strongly about this one. For now, let’s keep it as is >>>>>>> >>>>>>> Thanks >>>>>>> Anthony && Victoria >>>>>>> >>>>>>> >>>>>>> >>>>>>> >>>>>>> >>>>>> >>>> >
