Re: [pgadmin4][patch] Initial patch to decouple from ACI Tree

On Fri, Jun 1, 2018 at 10:09 PM, Victoria Henry <vhenry(at)pivotal(dot)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 <aemengo(at)pivotal(dot)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(dot)vashi(at)enterprisedb(dot)com> wrote:
>>
>>> On Thu, May 24, 2018 at 8:13 PM, Joao De Almeida Pereira <
>>> jdealmeidapereira(at)pivotal(dot)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(dot)vashi(at)enterprisedb(dot)com> wrote:
>>>>
>>>>> Sorry for the late reply.
>>>>> On Wed, May 16, 2018 at 8:55 PM, Anthony Emengo <aemengo(at)pivotal(dot)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
>>>>>> ​
>>>>>>
>>>>>>
>>>>>>
>>>>>>
>>>>>
>>>