Re: [pgadmin4][patch] Initial patch to decouple from ACI Tree - Mailing list pgadmin-hackers

Hi Victoria,

On Wed, Jun 6, 2018 at 8:55 PM, Victoria Henry <vhenry@pivotal.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.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 <vhenry@pivotal.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.vashi@enterprisedb.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 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@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.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 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.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/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 <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’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

​