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

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

Sincerely,

Victoria
​

On Thu, Jun 7, 2018 at 12:35 AM Aditya Toshniwal <
aditya(dot)toshniwal(at)enterprisedb(dot)com> wrote:

> Hi Victoria,
>
> On Wed, Jun 6, 2018 at 8:55 PM, Victoria Henry <vhenry(at)pivotal(dot)io> 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 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 <
>> aditya(dot)toshniwal(at)enterprisedb(dot)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 <vhenry(at)pivotal(dot)io>
>>> 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(dot)vashi(at)enterprisedb(dot)com> wrote:
>>>>
>>>>> 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
>>>>>>>>>>> ​
>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>
>>>>>
>>>
>