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 <vhe...@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.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
>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>
>>>>>
>>
