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

Hi Joao

Below are my review comments:

- In *menu/can_create.js,* "parentCatalogOfTableChild" function name is
unable to understand.
- Below code is bit confusing in terms of naming convention canCreate.
canCreate inside "canCreate".

canCreate: function(itemData, item, data) {

return canCreate.canCreate(pgBrowser, 'coll-collation', item, data);

}

Can we have something like "*menuCreator.canCreate*"?

- Why we have "*menu_utils.js*" for Backup, Maintenance, Restore and
Grant Wizard? Can we have one single file with all the supported node for
respective module? Also I have seen that the folder hierarchy for grant
wizard is "/*web/pgadmin/static/js/grant/wizard/menu_utils.js*" why
there are two separate folders grant and wizard (sub folder of grant)?

On Wed, May 9, 2018 at 4:32 PM, Khushboo Vashi <
khushboo(dot)vashi(at)enterprisedb(dot)com> wrote:

> Hi Joao,
>
> Patch 0001 & 0002 (esp the new tree implementation) look good to me.
>
> Patch 0003 - I have some concerns regarding the approach taken to separate
> out the PG utilities. The dialogue factory is responsible for creating the
> dialogue which uses dialogue wrapper. So, any module should call the
> dialogue factory to create a new dialogue instance. Here dialogue itself
> call the dialogue factory for creating it.
>
> In the enable_disable_triggers.js file, can we just import alertifyjs
> instead of passing in all the functions?
>
> Patch 0004 - Right now with the new proposed architecture only JS files
> are re-arranged. To maintain the pluggability I would also like to have
> some discussion on the back end side. Right now, the code maintains
> everything. Let say I want to add any node under schema I will just drop
> the related folder and files in that hierarchy and it will work. But with
> the proposed architecture I can not visualize the backend architecture and
> this should not be ended with only JS restructuring.
>
> Thanks,
> Khushboo
>
> On Tue, May 8, 2018 at 12:31 AM, Joao De Almeida Pereira <
> jdealmeidapereira(at)pivotal(dot)io> wrote:
>
>> Hi Hackers,
>> We noticed that some of the latest changes created some conflicts with
>> this patch. So you can find attached the patch rebased on the new master.
>>
>> Thanks
>> Victoria && Joao
>>
>> On Fri, May 4, 2018 at 6:03 PM Joao De Almeida Pereira <
>> jdealmeidapereira(at)pivotal(dot)io> wrote:
>>
>>> And now the patches......
>>>
>>>
>>> On Fri, May 4, 2018 at 6:01 PM Joao De Almeida Pereira <
>>> jdealmeidapereira(at)pivotal(dot)io> wrote:
>>>
>>>> Hi Hackers,
>>>>
>>>> *Be prepared this is going to be a big email.*
>>>> Objectives of this patch:
>>>>
>>>> 1. Start the work to split ACI from pgAdmin code
>>>> 2. Add more tests around the front end components
>>>> 3. Start the discussion on application architecture
>>>>
>>>> 1. Start the work to split ACI from pgAdmin code Why
>>>>
>>>> This journey started on our first attempt to store state of the current
>>>> tree, so that when a user accessed again pgAdmin
>>>> there would be no need to reopen all the nodes and make the flow
>>>> better. Another problem users brought to us was related
>>>> to the UI behavior when the number of objects was too big.
>>>>
>>>> In order to do implement this features we would have to play around
>>>> with aciTree or some functions from our code that
>>>> called and leveraged the aciTree functionalities. To do this we needed
>>>> to have some confidence that our changes would
>>>> implement correctly the new feature and it would not break the current
>>>> functionality.
>>>> The process that we used was first extract the functions that
>>>> interacted with the tree, wrap them with
>>>> tests and then refactor them in some way. While doing this work we
>>>> realized that the tree started spreading roots
>>>> throughout the majority of the application.
>>>> How
>>>>
>>>> Options:
>>>>
>>>> 1. Rewrite the front end
>>>> 2. One bang replace ACI Tree without change on the application
>>>> 3. Create an abstraction layer between ACI Tree and the application
>>>>
>>>> 1. Rewrite the front end
>>>> Pros Const
>>>> Achieve decoupling by recreating the frontend Cost is too high
>>>> Do not rely on deprecated or unmaintained libraries Turn around is too
>>>> long
>>>> One shot change 2. One bang replace ACI Tree
>>>> Pros Const
>>>> Achieve decoupling by recreating the frontend Cost is too high
>>>> Does not achieve the decoupling unless we change the application
>>>> Need to create an adaptor
>>>> No garantee of success
>>>> One shot change 3. Create an abstraction layer between ACI Tree and
>>>> the application
>>>> Pros Const
>>>> Achieve decoupling of the application and the tree 90% of the time on
>>>> Code archaeology
>>>> Lower cost
>>>> Ability to change or keep ACI Tree, decision TBD
>>>> Can be done in a iterative way
>>>>
>>>> We decided that options 3 looked more attractive specially because we
>>>> could do it in a iterative way.
>>>>
>>>> The next image depicts the current state of the interactions between
>>>> the application and the ACI and the place we want
>>>> to be in.
>>>>
>>>> [image: Screen Shot 2018-05-04 at 4.06.17 PM.png]
>>>>
>>>> for retrieving of data from the ACI Tree that then is used in different
>>>> ways.
>>>>
>>>> Because we are doing this in a iterative process we need to keep the
>>>> current tree working and create our adaptor next
>>>> to it. In order to do that we created a new property on PGBrowser
>>>> class called treeMenu and when ACI Tree receives
>>>> the node information we also populate the new Tree with all the needed
>>>> information.
>>>>
>>>> This approach allow us not to worry, for now, with data retrieval, URL
>>>> system and other issues that should be addressed
>>>> in the future when the adaptor is done.
>>>>
>>>> This first patch tries to deal with the low hanging fruit, functions
>>>> that are triggered by events from ACI Tree.
>>>> Cases like registering to events and triggering events need to be
>>>> handled in the future as well.
>>>> 2. Add more tests around the front end components Why
>>>>
>>>> Go Fast Forever: https://builttoadapt.io/why-tdd-489fdcdda05e
>>>> I think this is a very good summary of why do we need tests in our
>>>> applications.
>>>> 3. Start the discussion on application architecture
>>>>
>>>> Why should we care about location of files inside a our application?
>>>>
>>>> Why is this way better the another?
>>>>
>>>> These are 2 good questions that have very lengthy answers. Trying to
>>>> more or less summarize the answers we care about
>>>> the location of the files, because we want our application to
>>>> communicate intent and there are always pros and cons
>>>> on all the decisions that we make.
>>>>
>>>> At this point the application structure follows our menu, this approach
>>>> eventually make is easier to follow the code
>>>> but at the same time if the menu changes down the line, will we change
>>>> the structure of our folders?
>>>>
>>>> The proposal that we do with the last diff of this patch is to change
>>>> to a structure that slices vertically the
>>>> application. This way we can understand intent behind the code and more
>>>> easily find what we are looking for.
>>>>
>>>> In the current structure if you want to see the tables code you need to
>>>> go to
>>>> pgAdmin/browser/server_groups/servers/databases/schemas/tables/ this
>>>> is a huge path to remember and to get to. What
>>>> do we win with this? If we open pgAdmin we know which nodes to click in
>>>> order to get to tables. But for development
>>>> every time that you are looking for a specific functionality you need
>>>> to run the application, navigate the menu so that
>>>> you know where you can find the code. This doesn’t sound very appealing.
>>>>
>>>> What if our structure would look like this:
>>>>
>>>> - web
>>>> - tables
>>>> - controller
>>>> - get_nodes.py
>>>> - get_sql.py
>>>> - __init__.py
>>>> - frontend
>>>> - component
>>>> - ddl_component.js
>>>> - services
>>>> - table-service.js
>>>> - schemas
>>>> - servers
>>>> - ....
>>>>
>>>> This would saves us time because all the information that we need is
>>>> what are we working on and everything is right there.
>>>> Menu driven structure Intent Driven Structure
>>>> *Pros:* *Pros:*
>>>> Already in place Explicitly shows features
>>>> Self contained features Self contained features
>>>> Support for drop in features Support for drop in features
>>>> *Cons:* *Cons:*
>>>> Follows the menu, and it could change Need to change current code
>>>> Hard to find features Some additional plumbing might be needed
>>>> Drop in features need to be placed in a specific location according to
>>>> the menu location
>>>>
>>>> What are your thought about this architecture?
>>>>
>>>> Around minute 7 of this video
>>>> <https://www.youtube.com/watch?v=hALFGQNeEnU> Uncle Bob shows an
>>>> application written
>>>> in rails to talk about architecture. It is a long KeyNote if you are
>>>> curious I would advise you to see the full video.
>>>> His approach to architecture of the application is pretty interesting.
>>>> ------------------------------
>>>> Patches 0001 Change the order of the shims on the shim file
>>>>
>>>> Simple change that orders the shims inside the shim file
>>>> 0002 New tree implementation
>>>>
>>>> This is the first version of our Tree implementation. At this point is
>>>> a very simple tree without no abstractions and
>>>> with code that eventually is not very performant, but this is only the
>>>> first iteration and we are trying to follow the
>>>> Last Responsible Moment Principle
>>>> <https://medium.com/@aidanjcasey/guiding-principles-for-an-evolutionary-software-architecture-b6dc2cb24680>
>>>> .
>>>>
>>>> What can you find in this patch:
>>>>
>>>> - Creation of PGBrowser.treeMenu
>>>> - Initial version of the Tree Adaptor web/pgadmin/static/js/tree/tre
>>>> e.js
>>>> - TreeFake test double
>>>> <https://martinfowler.com/bliki/TestDouble.html> that can replace
>>>> the Tree for testing purposes
>>>> - Tests. As an interesting asside because Fake’s need to behave
>>>> like the real object you will noticed that there are
>>>> tests for this type of double and they the same as of the real
>>>> object.
>>>>
>>>> 0003 Extract, Test and Refactor Methods
>>>>
>>>> This is the patch that contains the change that we talked a in the
>>>> objectives. It touches in a subset of the places
>>>> where itemData function is called. To have a sense of progress the
>>>> following image depicts, on the left all places
>>>> where this functions can be found in the code and on the right the
>>>> places where it still remains after this patch.
>>>>
>>>> But this patch is not only related to the ACI Tree, it also creates
>>>> some abstractions from code that we found repeated.
>>>> Some examples of this are the dialogs for Backup and Restore, where the
>>>> majority of the logic was common, so we created
>>>> in web/pgadmin/static/js/alertify/ 3 different objects:
>>>> A Factory DialogFactory that is responsible for creating new Dialog
>>>> and these dialogs will use the DialogWrapper.
>>>>
>>>> This is also a good example of the Last Responsible Moment Principle
>>>> <https://medium.com/@aidanjcasey/guiding-principles-for-an-evolutionary-software-architecture-b6dc2cb24680>
>>>> ,
>>>> in Dialog there are still some functions that are used in a Recover
>>>> and Backup scenario. When we have more examples
>>>> we will be able to, if we want, to extract that into another function.
>>>>
>>>> While doing some code refactoring we found out that the Right Click
>>>> Menu as some checks that can be centralized. Examples
>>>> of these are checks for Enable/Disable of an option of the display of
>>>> the Create/Edit/Drop option in some menus.
>>>> Check web/pgadmin/static/js/menu/ for more information. This
>>>> simplified code like:
>>>>
>>>> canCreate: function(itemData, item, data) {
>>>> - //If check is false then , we will allow create menu
>>>> - if (data && data.check == false)
>>>> - return true;
>>>> -
>>>> - var t = pgBrowser.tree, i = item, d = itemData;
>>>> - // To iterate over tree to check parent node
>>>> - while (i) {
>>>> - // If it is schema then allow user to create domain
>>>> - if (_.indexOf(['schema'], d._type) > -1)
>>>> - return true;
>>>> -
>>>> - if ('coll-domain' == d._type) {
>>>> - //Check if we are not child of catalog
>>>> - var prev_i = t.hasParent(i) ? t.parent(i) : null,
>>>> - prev_d = prev_i ? t.itemData(prev_i) : null;
>>>> - if( prev_d._type == 'catalog') {
>>>> - return false;
>>>> - } else {
>>>> - return true;
>>>> - }
>>>> - }
>>>> - i = t.hasParent(i) ? t.parent(i) : null;
>>>> - d = i ? t.itemData(i) : null;
>>>> - }
>>>> - // by default we do not want to allow create menu
>>>> - return true;
>>>> + return canCreate.canCreate(pgBrowser, 'coll-domain', item, data);
>>>> }
>>>>
>>>> This refactor was made in a couple of places throughout the code.
>>>>
>>>> Another refactor that had to be done was the creation of the functions
>>>> getTreeNodeHierarchyFromElement and
>>>> getTreeNodeHierarchyFromIdentifier that replace the old
>>>> getTreeNodeHierarchy. This implementation was done
>>>> because for the time being we have 2 different types of trees and calls
>>>> to them.
>>>>
>>>> The rest of the changes resulted from our process of refactoring that
>>>> we are following:
>>>>
>>>> 1. Find a location where itemData is used
>>>> 2. Extract that into a function outside of the current code
>>>> 3. Wrap it with tests
>>>> 4. When we have enough confidence on the tests, we refactor the
>>>> function to use the new Tree
>>>> 5. We refactor the function to become more readable
>>>> 6. Start over
>>>>
>>>> 0004 Architecture
>>>>
>>>> The proposed structure is our vision for the application, this is not
>>>> the state we will be in when this patch lands.
>>>> For now the only change we are doing is move the javascript into
>>>> static/js/FEATURE_NAME. This is a first step in the
>>>> direction that we would like to see the product heading.
>>>>
>>>> Even if we decide to keep using the same architecture the move of
>>>> Javascript to a centralized place will
>>>> not break any plugability as webpack can retrieve all the Javascript
>>>> from any place in the code.
>>>>
>>>> Have a nice weekend
>>>> Joao
>>>> ​
>>>>
>>>>
>

--
*Akshay Joshi*

*Sr. Software Architect *

*Phone: +91 20-3058-9517Mobile: +91 976-788-8246*