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

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/tree.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
>> ​
>>
>>