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

Hi Hackers,

Can someone review and merge this patch?

Thanks
Joao

On Wed, Apr 18, 2018 at 10:23 AM Joao De Almeida Pereira <
jdealmeidapereira(at)pivotal(dot)io> wrote:

> Hi Hackers,
> Any other comment about this patch?
>
> Thanks
> Joao
>
> On Tue, Apr 10, 2018 at 12:00 PM Joao De Almeida Pereira <
> jdealmeidapereira(at)pivotal(dot)io> wrote:
>
>> Hello Khushboo
>>
>> On Mon, Apr 9, 2018 at 1:59 AM Khushboo Vashi <
>> khushboo(dot)vashi(at)enterprisedb(dot)com> wrote:
>>
>>> Hi Joao,
>>>
>>> I have reviewed your patch and have some suggestions.
>>>
>>> On Sat, Apr 7, 2018 at 12:43 AM, Joao De Almeida Pereira <
>>> jdealmeidapereira(at)pivotal(dot)io> wrote:
>>>
>>>> Hello Murtuza/Dave,
>>>> Yes now the extracted functions are spread into different files. The
>>>> intent would be to make the files as small as possible, and also to group
>>>> and name them in a way that would be easy to understand what each file is
>>>> doing without the need of opening it.
>>>> As a example:
>>>> static/js/backup will contain all the backup related functionality
>>>> inside of this folder we can see the file:
>>>>
>>> menu_utils.js At this moment in time we decided to group all the
>>>> functions that are related to the menu, but we can split that also if we
>>>> believe it is easier to see.
>>>>
>>> It's really very good to see the separated code for backup module. As we
>>> have done for backup, we would like do it for other PG utilities like
>>> restore, maintenance etc.
>>> Considering this, we should separate the code in a way that some of the
>>> common functionalities can be used for other modules like menu (as you
>>> have mentioned above), dialogue factory etc.
>>> Also, I think these functionalities should be in their respective static
>>> folder instead of pgadmin/static.
>>>
>>
>> About the location of the files. The move of the files to
>> pgadmin/static/js was made on purpose in order to clearly separate
>> Javascript from python code.
>> The rational behind it was
>> - Create a clear separation between the backend and frontend
>> - Having Javascript code concentrated in a single place, hopefully, will
>> encourage to developers to look for a functionality, that is already
>> implemented in another modules, because they are right there. (When we
>> started this journey we realized that the 'nodes' have a big groups of code
>> that could be shared, but because the Javascript is spread everywhere it is
>> much harder to look for it)
>>
>>
>> There are some drawbacks of this separation:
>> - When creating a new module we will need to put the javascript in a
>> separate location from the backend code
>>
>>
>>>
>>>
>>>> static/js/datagrid folder contains all the datagrid related
>>>> functionality
>>>>
>>> Same as backup module, this should be in it's respective static/js
>>> folder.
>>>
>>>> Inside of the folder we can see the files:
>>>> get_panel_title.js is responsible for retrieving the name of the panel
>>>> show_data.js is responsible for showing the datagrid
>>>> show_query_tool.js is responsible for showing the query tool
>>>>
>>>> Does this structure make sense?
>>>> Can you give an example of a comment that you think is missing and that
>>>> could help?
>>>>
>>>> As a personal note, unless the algorithm is very obscure or very
>>>> complicated, I believe that if the code needs comments it is a signal that
>>>> something needs to change in terms of naming, structure of the part in
>>>> question. This being said, I am open to add some comments that might help
>>>> people.
>>>>
>>> You are right, with the help of naming convention and structure of the
>>> code, any one can get the idea about the code. But it is very useful to
>>> understand the code
>>> very easily with the proper comments especially when there are multiple
>>> developers working on a single project.
>>>
>>> I found some of the places where it would be great to have comments.
>>>
>>> - treeMenu: new tree.Tree() in a browser.js
>>> - tree.js (especially Tree class)
>>>
>> About the comment point I need a more clear understanding on what kind of
>> comments you are looking for. Because when you read the function names you
>> understand the intent, what they are doing. The parameters also explain
>> what you need to pass into them.
>>
>> If what you are looking for in these comments is the reasoning being the
>> change itself, then that should be present in the commit message. Specially
>> because this is going to be a very big patch with a very big number of
>> changes.
>>
>>>
>>> Thanks
>>>> Joao
>>>> ​
>>>>
>>>> Thanks,
>>> Khushboo
>>>
>>>>
>>>> On Fri, Apr 6, 2018 at 4:48 AM Murtuza Zabuawala <
>>>> murtuza(dot)zabuawala(at)enterprisedb(dot)com> wrote:
>>>>
>>>>> Hi Joao,
>>>>>
>>>>> Patch looks good and working as expected.
>>>>>
>>>>> I also agree with Dave, Can we please add some comments in each file
>>>>> which can help us to understand the flow, I'm saying because now the code
>>>>> is segregated in so many separate files it will be hard to keep track of
>>>>> the flow from one file to another when debugging.
>>>>>
>>>>>
>>>>> --
>>>>> Regards,
>>>>> Murtuza Zabuawala
>>>>> EnterpriseDB: http://www.enterprisedb.com
>>>>> The Enterprise PostgreSQL Company
>>>>>
>>>>>
>>>>> On Thu, Apr 5, 2018 at 7:08 PM, Joao De Almeida Pereira <
>>>>> jdealmeidapereira(at)pivotal(dot)io> wrote:
>>>>>
>>>>>> Hi Khushboo,
>>>>>> Attached you can find both patches rebased
>>>>>>
>>>>>> Thanks
>>>>>>
>>>>>>
>>>>>> On Thu, Apr 5, 2018 at 6:31 AM Khushboo Vashi <
>>>>>> khushboo(dot)vashi(at)enterprisedb(dot)com> wrote:
>>>>>>
>>>>>>> Hi Joao,
>>>>>>>
>>>>>>> Can you please rebase the second patch?
>>>>>>>
>>>>>>> Thanks,
>>>>>>> Khushboo
>>>>>>>
>>>>>>>
>>>>>>> On Tue, Apr 3, 2018 at 12:15 AM, Joao De Almeida Pereira <
>>>>>>> jdealmeidapereira(at)pivotal(dot)io> wrote:
>>>>>>>
>>>>>>>> Hi Hackers,
>>>>>>>>
>>>>>>>> Attached you can find the patch that will start to decouple pgAdmin
>>>>>>>> from ACITree library.
>>>>>>>> This patch is intended to be merged after 3.0, because we do not
>>>>>>>> want to cause any entropy or delay the release, but we want to start the
>>>>>>>> discussion and show some code.
>>>>>>>>
>>>>>>>> This job that we started is a massive tech debt chore that will
>>>>>>>> take some time to finalize and we would love the help of the community to
>>>>>>>> do it.
>>>>>>>>
>>>>>>>> *Summary of the patch:*
>>>>>>>> 0001 patch:
>>>>>>>> - Creates a new tree that will allow us to create a separation
>>>>>>>> between the application and ACI Tree
>>>>>>>> - Creates a Fake Tree (Test double, for reference on the available
>>>>>>>> test doubles: https://martinfowler.com/bliki/TestDouble.html) that
>>>>>>>> can be used to inplace to replace the ACITree and also encapsulate the new
>>>>>>>> tree behavior on our tests
>>>>>>>> - Adds tests for all the tree functionalities
>>>>>>>>
>>>>>>>> 0002 patch:
>>>>>>>> - Extracts, refactors, adds tests and remove dependency from ACI
>>>>>>>> Tree on:
>>>>>>>> - getTreeNodeHierarchy
>>>>>>>> - on backup.js: menu_enabled, menu_enabled_server,
>>>>>>>> start_backup_global_server, backup_objects
>>>>>>>> - on datagrid.js: show_data_grid, get_panel_title, show_query_tool
>>>>>>>> - Start using sprintf-js as Underscore.String is deprecating
>>>>>>>> sprintf function
>>>>>>>>
>>>>>>>> This patch represents only 10 calls to ACITree.itemData out of 176
>>>>>>>> that are spread around our code
>>>>>>>>
>>>>>>>>
>>>>>>>> *In Depth look on the process behind the patch:*
>>>>>>>>
>>>>>>>> We started writing this patch with the idea that we need to
>>>>>>>> decouple pgAdmin4 from ACITree, because ACITree is no longer supported, the
>>>>>>>> documentation is non existent and ACITree is no longer being actively
>>>>>>>> developed.
>>>>>>>>
>>>>>>>> Our process:
>>>>>>>> 1. We "randomly" selected a function that is part of the ACITree.
>>>>>>>> From this point we decided to replace that function with our own version.
>>>>>>>> The function that we choose was "itemData".
>>>>>>>> The function gives us all the "data" that a specific node of the
>>>>>>>> tree find.
>>>>>>>> Given in order to replace the tree we would need to have a function
>>>>>>>> that would give us the same information. We had 2 options:
>>>>>>>> a) Create a tree with a function called itemData
>>>>>>>> Pros:
>>>>>>>> - At first view this was the simpler solution
>>>>>>>> - Would keep the status quo
>>>>>>>> Cons:
>>>>>>>> - Not a OOP approach
>>>>>>>> - Not very flexible
>>>>>>>> b) Create a tree that would return a node given an ID and then
>>>>>>>> the node would be responsible for giving it's data.
>>>>>>>> Pros:
>>>>>>>> - OOP Approach
>>>>>>>> - More flexible and we do not need to bring the tree around, just
>>>>>>>> a node
>>>>>>>> Cons:
>>>>>>>> - Break the current status quo
>>>>>>>>
>>>>>>>> Given these 2 options we decided to go for a more OOP approach
>>>>>>>> creating a Tree and a TreeNode classes, that in the future will be renamed
>>>>>>>> to ACITreeWrapper and TreeNode.
>>>>>>>>
>>>>>>>> 2. After we decided on the starting point we searched for
>>>>>>>> occurrences of the function "itemData" and we found out that there were 303
>>>>>>>> occurrences of "itemData" in the code and roughly 176 calls to the function
>>>>>>>> itself (some of the hits were variable names).
>>>>>>>>
>>>>>>>> 3. We selected the first file on the search and found the function
>>>>>>>> that was responsible for calling the itemData function.
>>>>>>>>
>>>>>>>> 4. Extracted the function to a separate file
>>>>>>>>
>>>>>>>> 5. Wrap this function with tests
>>>>>>>>
>>>>>>>> 6. Refactor the function to ES6, give more declarative names to
>>>>>>>> variables and break the functions into smaller chunks
>>>>>>>>
>>>>>>>> 7. When all the tests were passing we replaced ACITree with our Tree
>>>>>>>>
>>>>>>>> 8. We ensured that all tests were passing
>>>>>>>>
>>>>>>>> 9. Remove function from the original file and use the new function
>>>>>>>>
>>>>>>>> 10. Ensure everything still works
>>>>>>>>
>>>>>>>> 11. Find the next function and execute from step 4 until all the
>>>>>>>> functions are replaced, refactored and tested.
>>>>>>>>
>>>>>>>> As you can see by the process this is a pretty huge undertake,
>>>>>>>> because of the number of calls to the function. This is just the first step
>>>>>>>> on the direction of completely isolating the ACITree so that we can solve
>>>>>>>> the problem with a large number of elements on the tree.
>>>>>>>>
>>>>>>>> *What is on our radar that we need to address:*
>>>>>>>> - Finish the complete decoupling of the ACITree
>>>>>>>> - Performance of the current tree implementation
>>>>>>>> - Tweak the naming of the Tree class to explicitly tell us this is
>>>>>>>> to use only with ACITree.
>>>>>>>>
>>>>>>>>
>>>>>>>> Thanks
>>>>>>>> Joao
>>>>>>>>
>>>>>>>>
>>>>>>>
>>>>>