Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Generic Object Definition tree with Custom Buttons, Groups and Actions as children #2481

Conversation

AparnaKarve
Copy link
Contributor

In order to give a visual context to the user, we need to use a Treeview to display Custom Button Groups and Custom Buttons for a particular Generic Object Definition.

With the Generic Object Definition as a parent node, the Custom Button Groups and Custom Buttons (inside of the Actions node) are displayed as the children of the Definition node.

screen shot 2017-10-20 at 10 40 15 am

Highlights of this PR -

  • Despite the treeview on the left, the Generic Object Definition view cannot be categorized as an "Explorer" view (traditionally all Explorer views have a tree on the left)
    By taking this approach, the generic_object_definition controller code has been kept very clean with no traces of the presenter code

  • Uses an angular component that uses the REST API to populate the tree

  • Creates Menu "hooks" to add CRUD for Custom Button Groups and Custom Buttons depending on the node selected as shown in the few examples below -

screen shot 2017-10-20 at 10 43 14 am

screen shot 2017-10-20 at 10 42 41 am

Display the accordion in `show_list` and `show` views
Modify the function to take both `custom_button_sets` and
`custom_buttons` into account
Set the url with query params based on node selected.
These query params will be used by the backend to set
appropraite menus and summary screens
Save trips to the backend by saving treedata in the `window` object
to be used by the toolbar buttons
taking into account cases for Custom Buttons, Groups and Actions
taking into account cases for Custom Buttons, Groups and Actions
The "singular" toolbar also uses Button classes for appropriate
Button visibility via the instance variables set in `show` method
@AparnaKarve
Copy link
Contributor Author

/cc @skateman

@skateman As we discussed the other day, one of the caveats here is that the tree would redraw itself when a node is selected, but that's a known (and expected) thing at the moment; and something we could work on going forward.

Let me know what you think about da19b02
IMO, redrawing the tree should not always execute the API. If we can avoid making a trip to the backend, then we should.

nodes: godNodes,
}];

vm.treeData = JSON.stringify(treeDataObj);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is wrong, you should not build nodes in the JS, they should be created in a TreeBuilder subclass as every other tree.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please explain why since I do not see a problem with this.

Using TreeBuilder effectively means saying NO to using REST API.
Why would we want that when we can use REST API?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@AparnaKarve because it is inconsistent, introduces a lot of exceptions and technical debt. Last time we were chatting about this, I told you that we're starting to build an API-like controller for the trees that will eventually land in the REST API.

Copy link
Member

@martinpovolny martinpovolny Oct 20, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@AparnaKarve : how many API calls do you need to build the tree? Can you estimate that number based on the number of generic objects or provide any other metric that will show us how this will scale?

I thought we had an agreement that we want to build the tree on the server side.

Also I thought we had an agreement that we want to find a common pattern for all the trees and convert all the trees to use the same API.

It may make sense to use client-side tree building if

  1. it will not send too many API requests -- therefor I am asking above.
  2. this tree is going to be special in some way so that it would not make sense to share the common pattern

Thx!

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@martinpovolny One API only, and that's it!

Everything that I need can be accomplished with one single API call, and that is precisely why I went with the REST API approach.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

well then it's pretty neat

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How many generic object will it be able to handle? A single API request, that's nice, but will it not kill the client when there are 100.000 or them?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@skateman, @AparnaKarve : I am trying to uderstand why the pattern that @skateman suggested (and that I believe was meant to be generic for all the trees) was not used here.

A am not sure if we would be able to build the other trees in a similar way on the client. What do you think?

Generally I'd prefer to limit the number of different ways to do a single thing for sake of maintainability.

@miq-bot
Copy link
Member

miq-bot commented Oct 20, 2017

Checked commits AparnaKarve/manageiq-ui-classic@4c77c3a~...a9baffe with ruby 2.3.3, rubocop 0.47.1, and haml-lint 0.20.0
11 files checked, 0 offenses detected
Everything looks fine. 👍

@martinpovolny
Copy link
Member

Don't get me wrong with the comment above. This looks like a nice piece of work. I just need to understand how this matches to the general tree-related work that we are doing.

@skateman
Copy link
Member

We invested a lot in moving all the fon/fileicon definitions into a single place (decorators) while this code introduces a new way where the decorators are unusable...

@martinpovolny
Copy link
Member

We invested a lot in moving all the fon/fileicon definitions into a single place (decorators) while this code introduces a new way where the decorators are unusable...

Well yes, but we should not be blind to other options if there are. Let's discuss this properly and see it from all the necesary angles.

@AparnaKarve
Copy link
Contributor Author

@skateman The REST API output is the single source of truth.
I got everything that I wanted from the API output. TBH, I didn't even think about decorators.

@skateman
Copy link
Member

skateman commented Oct 20, 2017

@AparnaKarve last week I explicitly asked you to extend and use the new tree controller endpoint with a new TreeBuilder. The main reason is that all our trees are build by TreeBuilder and all our nodes are decorated by MiqDecorator. I understand that the REST API is above everything, but prematurely using it while introducing inconsistencies doesn't feels for me as the right way.

@martinpovolny
Copy link
Member

I got everything that I wanted from the API output. TBH, I didn't even think about decorators.

@AparnaKarve : I understand your reasoning. But for other trees this approach is probably not going to work, is it? We will need decorators and we have more complex trees. Do you think the approach presented here is usable for more trees? All trees?

Or do you want to do each tree in a different way? Or is tree going to be an exception? Please, help, me understand this PR in the wider context.

@AparnaKarve
Copy link
Contributor Author

@martinpovolny Using REST API for every Treeview should be considered similar to using TreeBuilder for every treeview.
The only difference really is getting the treenodes via frontend Vs getting the treenodes via backend
I chose the frontend approach here since that's the direction we are all headed - REST API.

I think the Parent nodes/Child nodes relationships come out just beautifully in the REST API output because of how the associations are done on the models.

I do think that this could be a generic approach across the board.

@skateman
Copy link
Member

@AparnaKarve I'm 100% in for doing the trees through the REST API, but then let's do it properly:

  • Export the decorators to a separate repo together with the icons
  • Make the REST API code be able to consume the decorators and provide icons for nodes
  • Make the TreeBuilders stateless so they can be extracted from the UI repo
  • Move the TreeBuilders to the REST API

If these are properly implemented and tested, we can go for API-driven trees, but as you see this is a lot of work and until then consistency & maintainability wins 😞

@AparnaKarve
Copy link
Contributor Author

@martinpovolny There are 2 big CRUD PRs - one for CustomButton Groups and one for Custom Buttons that I have been working on in the background.
These PRs would need the tree to exist first, because selecting the tree nodes yield the right toolbar buttons needed to invoke the CRUD forms.

I don't mean to rush anyone on the reviews, but now that we are in the time crunch mode, I would really need this PR to be merged soon since the other tasks depend on this PR.
Thanks.

@martinpovolny
Copy link
Member

martinpovolny commented Oct 20, 2017

@AparnaKarve : I have stated my questions and I don't understand what you are saying in #2481 (comment) even after reading it 4 times. Sorry. I really don't get it. I don't see how this is going to be applicable to the other trees and I think that @skateman is correct in #2481 (comment).

I thought we discussed this on Monday and that you and @skateman will cooperate on an approach that will be applicable to all the trees. That you will create an example that we will be able to apply to all the tree builders and all the trees. I don't understand how this is accomplished with this PR :-( It might be a problem on my side but I don't understand your explanation.

I uderstand the time pressure so I will leave the review of this one to other team members. It seems to work but it's really not what I hoped for (or I don't understand it). Sorry. This will most likely stay a separate, exceptional piece of code the same way as the previous explorer-style GO UI by ErikC.

All I am doing on this project is trying to work towards unification and finding generic solutions that will help us maintain a large codebase and a huge flow of new code. I don't see, how this PR is going to help with that.

But I do see that this works and solves the immediate problem that you are working on. It's just not the way I want to work.

@skateman
Copy link
Member

@martinpovolny 👍
@AparnaKarve imagine the scenario when you have ~100 generic objects each with ~100 custom buttons. I am pretty sure the REST API worker will scream and maybe even try to commit suicide. Sorry but this is a HUGE performance issue and I can't approve this approach 😭

BTW this problem is already solved by the lazy loading in the TreeBuilder 😉

@AparnaKarve
Copy link
Contributor Author

@martinpovolny Let me try again.

If a clear association exists between 2 models, then with 1 single API call, one can easily construct the whole tree.
In the current case, for e.g. we have a one-to-many association between a Generic Object Definition and Custom Buttons, and that is pretty much all we need.

With the below API -
/api/generic_object_definitions?expand=resources&attributes=custom_buttons

it is possible to retrieve all GO Defs with their respective Custom Buttons
Once the association is established in the models, the API can get us what we need in 1 call.
So why not implement something similar for our other trees?
That said, I'm sure there will be edge cases. But for the most part we should be covered.

So to answer your question once again... this idea, in general, can be applied to all the trees, yes. But each tree will have it's own angular component implementation since each tree will have it's own unique API.
Remember that this is version 1.0 of using the REST API approach for trees (and hence just a start), and by now we all know it too well that we don't stop at v1.0 :-) the idea evolves, gets better with every iteration, things get more performant, more efficient, more easy-to-use and more generic.

BTW, I don't recollect anyone saying to me on Monday that the REST API approach was a complete no-no.

This will most likely stay a separate, exceptional piece of code the same way as the previous explorer-style GO UI by ErikC.

I would have to disagree with you there.
As far as the JS code goes, there is a clear commonality, a clear common pattern of this code with our other angular forms code. It is extremely consistent.
Don't let the REST API on trees approach throw you off - Look carefully and you'll see it is more of the same.

It might seem different because it is a hybrid of show_list and explorer - I don't think something like this was attempted before, so in that sense, yes it is different.

@AparnaKarve
Copy link
Contributor Author

AparnaKarve commented Oct 20, 2017

@skateman I think you are simply over-reacting here -- cool your jets! ;-)
Lazy loading can be easily implemented here as well.

@martinpovolny
Copy link
Member

martinpovolny commented Oct 21, 2017

In the current case, for e.g. we have a one-to-many association between a Generic Object Definition and Custom Buttons, and that is pretty much all we need.

You are right here.

So why not implement something similar for our other trees?

Look at the trees. You can start for example with the 4 trees in the catalog explorer. Or the VM and template tree. There's 90 trees right now. What do you think is the percentage of the trees that your statatement applies to?

This GO tree is not the same as the other trees. No. This one is trivial NOW. But will it stay trivial? Or is trivial just the first iteration and then it will get complex with different icons, groupings, filters?

Sure you can add it bit by bit to your new pattern. Then you can do the same with the remaining 90 trees. See you in 2 years ;-)

BTW, I don't recollect anyone saying to me on Monday that the REST API approach was a complete no-no.

Right. Noone said that. Noone meant that.

There's a team of people that worked on a strategy of moving to the client with the trees. It involved 5 or 6 people from the team. They did a lot of work.

  • We started by splitting the ball of code into individual tree builder classes.
  • Then we separated the structure from the presentation
  • Then we separated all the node presentation details into decorators and other classes.
  • Then the structure of the trees was made a bit declarative with the has_kids_for.
  • Due to the systematic work we where able to switch the component for rendering the trees 2x in the process as the UI landscape changed w/o any significant rewrites just by generalizing and actually removing files and code.
  • Lately @skateman implemented the Angular component what you are using in this PR.

Next we planned on creating an pattern for a JSON based endpoint for the trees and I hoped you would help with that. Instead you took a shortcut.

As I said it works. It works for you and for now.

@AparnaKarve
Copy link
Contributor Author

This GO tree is not the same as the other trees. No.

Not true. It is very similar to Catalog Item tree.
In fact I have been using the Catalog Item tree as a reference to build the GOD tree.
So you see what's happening? If the GOD tree was built with this approach, the catalog item tree could be built with this approach as well.

(Also let me make a correction here, the acronym is - GOD tree (and not GO tree) - this acronym correction is very relevant to the tree discussion. I will explain why below.)

This one is trivial NOW.

I don't consider a tree with 3 levels as trivial. I will categorize it as a "Medium complexity" tree.

But will it stay trivial? Or is trivial just the first iteration and then it will get complex with different icons, groupings, filters?

Not sure. But based on how the Catalog item tree is, this tree will probably stay the same.
If by "complex", you mean more levels, then sure we could add more levels and may have to use lazy loading at that point.

I hoped you would help with that. Instead you took a shortcut.

On the contrary, I was hoping that you would help me with my on-going tasks required for the release.
First of all, it will be a HUGE help if you guys could just keep an open mind about fresh, new ideas.
Sometimes, a good idea is right under your nose, and we fail to see it.
Here I am offering you a fresh perspective on things with a living, breathing example in this PR on how we can make this work.
Will there be challenges to apply this across the board? Sure! But addressing those challenges will only make the API better.
It's a win-win!!

I don't know what your definition of "shortcut" is.
But if you mean a shorter, efficient route, then this surely is!

The REST API in this PR has delivered results big time, and that too in a very elegant way, and you should all feel happy about it. Instead what I'm seeing is negativity and an overly critical attitude all around me, and that is just sad :-(

As I said it works. It works for you and for now.

It could work for all of us, if you let it.

I also think it is important to dog-food our API in whichever way possible - it will be immensely good for the community and our API users.

@AparnaKarve
Copy link
Contributor Author

(Also let me make a correction here, the acronym is - GOD tree (and not GO tree) - this acronym correction is very relevant to the tree discussion. I will explain why below.)

Let me explain why the above acronym correction was important.
Generic Object Definition (or Generic Object Class) is NOT the same as Generic Object

In a given environment, we do not anticipate to have hundreds of Generic Object Definitions, which effectively means the tree will be quite manageable.
To make it amply clear, note that the tree currently displays Generic Object Definitions

OTOH, we expect to have a big volume of Generic Objects (or GOs)
And hence those will be excluded from the treeview.
It would be counterproductive if we included GOs in our tree.

@martinpovolny
Copy link
Member

martinpovolny commented Oct 22, 2017

@AparnaKarve : You have 3 great points right:

  1. You are right that this tree is not that simple.
  2. You finished the task to get it to the release. I already wrote that.
  3. You used the API.

I know that you can write a lot of code and get things done. But we as a team should think in the context of unification, sustainability, maintainability and technical debt.

Btw, you ignored my points and did not answer my questions above :-(

What you are doing is basically ignoring any discussion that you had or where supposed to have on this with @skateman. Also you are ignoring the previous work and direction set by the rework of all the trees that was done by many team members including @himdel, @h-kataria, @ZitaNemeckova and others.

I think that we need more teamplay here.

You asked me to explaing taking shortcuts so here it goes:

Looking at the code it's clear that you are putting presentation logic back together with the structure logic.

Stuff that we are struggling to remove from Ruby is getting back in Javascript:

$window.location.href.split("/").pop().split('?')[0]; - love this one!

if .. else if ... else if .... else, case ...

switch ... case. .. case .... case....

Then concatenate some strings.

$window.location.href = vm.showUrl + '/' + node.parentNodeId + '?cb=' + key[1];

Basically you are making the Angular and Javascript code the same spaghetti that we are still trying to remove from the Ruby codebase.

We designed some patterns to get rid of these. This involves separating tree structure from the presentation details. Separating tree component internals from tree structure. Decorators for the stuff being presented.. Builder for the structure of the tree and a different one for the nodes.

Does it explain the "taking of the shortcuts"?

@skateman
Copy link
Member

@AparnaKarve I am open for new and innovative ideas, but this is a too big and non-trivial change to not have more discussion about it. If I understand it right this is your proposal for the new reference implementation for all our trees. If that's true, then I think this would need a separate issue to discuss and not a PR where you force us to accept it because time is running low. The idea is definitely innovative, but as we described with @martinpovolny it is bleeding from multiple wounds.

I am open for negotiations in the area, but not in a such critical UI feature that we want to deliver ASAP. So if you want to introduce a new architecture for the trees, open a new GH issue where you describe your plan and we can schedule some meetings about it.

@AparnaKarve
Copy link
Contributor Author

I will defer the Merge GO/NO-GO decision to @dclarizio and @ohadlevy - my future CRUD PRs are all grounded to a complete HALT pending this decision. I have nothing more to add to this thread. Thanks.

@mzazrivec
Copy link
Contributor

I consider the objections made by @skateman and @martinpovolny perfectly valid. This PR diverges
from what is done in the rest of the application and it creates technical debt, which someone will have
to deal with in the future.

This was strictly a technical discussion (or was trying to be for the most part) and making this
now a matter of managerial decision, because there are other PRs pending this and because
the time is ticking out, is not exactly fair.

@ohadlevy
Copy link
Member

ohadlevy commented Oct 23, 2017

@AparnaKarve thank you for involving me in the discussion, from an initial review of the PR, I share you wish to use the REST api as a future means of displaying GOD trees.

I also see the concern raised by others that this introduces both a technical debt, and most likely, since its a new approach, it might introduce risk (e.g. bugs with pagination, performance etc - it's hard to tell as there is no test coverage).

Personally, I don't think this is the right technical approach to the problem, as I believe this is yet another example where we should use redux vs angular, your current implementation is tightly coupled with rails current behavior, which while uses the API, does not reduce complexity nor simplify maintainability of the code, e.g. in other words, it uses the API, but does not help moving to a SPA, is not reusable or isolated (e.g. for the ui components).

Please understand that my aim is not to pick on your specific implementation, as its definitely improving the project and adding a needed feature, but rather looking at it from a bigger picture point of view, where IMHO the current ways of operation (again - not only this specific PR), will not allow us to grow in the future.

for me to know - what does it mean to reuse the existing behavior in order to implement this feature? (can someone provide a gist / patch on top of this?) I simply don't have a good understanding of the impact (for this PR - and follow up PR's for CRUD).

lastly, my personal opinion is to do the right thing, even if deadline's are near.

t = N_('Add a new Button Group'),
t,
:klass => ApplicationHelper::Button::GenericObjectDefinitionCustomButtonGroupButtonNew,
:url_parms => "custom_button_group_new_div"
Copy link
Contributor

@himdel himdel Oct 23, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You either also need :send_checked => true here (assuming the url_parms was to send a list of checked items), or you may want to look into whether this needs to be here at all.

Same for the other entries with url_parms.

(Otherwise, sending a list of checked items won't work after #2398.)

@martinpovolny
Copy link
Member

@AparnaKarve : I'd like to repeat my invitation to the discussion on how to move forward with all the trees. @skateman is the last one touching this area so I think that he should be involved and heard. You are an obvious participant in that. More people might have things to say including @himdel, @h-kataria, @mzazrivec and myself. Maybe we can setup a call?

@himdel
Copy link
Contributor

himdel commented Oct 23, 2017

Late to the discussion, most things have already been said, so just a few notes.

IMO this should have been a proper explorer screen, and most of the problems here stem from not following that pattern. Granted, it's not a very modern pattern, but.. converting N+1 explorer screens would be easier that converting N explorer screens and figuring out one special one.

But, right now, half of the pieces to implement the Explorer pattern properly in JS are missing, which brings a lot of undesirable hacks, and.. yes, I agree with the others, it is too many.


@display != 'generic_objects' && @cb_group.nil? && @cb.nil? && @cb_group_actions.nil?

Adding 3 extra fields to toolbars because this should be a different toolbar kind, per entity.

= render :partial => "layouts/listnav/#{controller_name}_show_list_with_treeview" if controller.respond_to?(:display_tree)

Rendering the tree from 2 different places, both explicitly named listnav - nowhere else in the app does a listnav have a left side tree, AFAIK.

Regardless of everything else, let's not use listnav templates for trees.

genericObjectDefinitionTreeviewController

This should have been a TreeBuilder. But even if not, it should have had a concept of TreeNodes of a specific kind, allowing you to replace all those ugly switch/if..else..else with something simple and data-driven.

@martinpovolny
Copy link
Member

martinpovolny commented Oct 23, 2017

IMO this should have been a proper explorer screen, and most of the problems here stem from not following that pattern. Granted, it's not a very modern pattern, but.. converting N+1 explorer screens would be easier that converting N explorer screens and figuring out one special one.

@himdel : actually not doing the explorer was discussed and ageed uppon. We wanted to test that approach and maybe find a target pattern for the explorers. I am happy @AparnaKarve did that.

@martinpovolny
Copy link
Member

oh, I did not mean to close this ;-)

@martinpovolny martinpovolny reopened this Oct 23, 2017
@martinpovolny
Copy link
Member

This should have been a TreeBuilder. But even if not, it should have had a concept of TreeNodes of a specific kind, allowing you to replace all those ugly switch/if..else..else with something simple and data-driven. @

That is my main problem with this.

@himdel
Copy link
Contributor

himdel commented Oct 23, 2017

OK, I take that back then :).

Still.. trees + listnav .. let's find a slightly better pattern :).

}
break;
case 'cbs':
$window.location.href = vm.showUrl + '/' + node.parentNodeId + '?cbs=' + key[1];
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So.. you can actually convince the browser to change the URL without reloading the whole page...

docs: https://developer.mozilla.org/en-US/docs/Web/API/History_API

example:
history.pushState({}, "new_title", new_url);

(you will probably want to do miqAjax to replace the partials together with this)

@miq-bot
Copy link
Member

miq-bot commented Oct 30, 2017

This pull request is not mergeable. Please rebase and repush.

@martinpovolny
Copy link
Member

@AparnaKarve : why is this one not closed? I'd really appreciate if people could keep closing stuff that is to be closed and mark as [WIP] stuff that is WIP.

It saves the time or reviewers and there are very few people doing reviews.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants