Display Generic Object Associations correctly (Post `/report_data` related GTL fixes) #2596

AparnaKarve · 2017-11-01T17:44:30Z

PR #2420 was cleaned up here, post /report_data related GTL fixes.

The only GTL change retained from #2420 is the dynamic creation of create_display_methods, which requires an association value to be passed in order to render the List successfully.

https://bugzilla.redhat.com/show_bug.cgi?id=1500829

AparnaKarve · 2017-11-01T17:47:07Z

/cc @martinpovolny

AparnaKarve · 2017-11-01T17:48:05Z

@miq_bot add_label gaprindashvili/yes,bug

martinpovolny · 2017-11-02T13:01:24Z

app/controllers/generic_object_controller.rb

@@ -2,6 +2,8 @@ class GenericObjectController < ApplicationController
  before_action :check_privileges
  before_action :get_session_data

+  before_action :create_display_methods, :only => [:show]


Please, do not use before_action. Just call the method where it is needed. I believe we already discussed this. It really makes the code far less readable than it could be.

I think it's just a personal preference to declare the method this way.
I somehow prefer to keep it concealed, the intention being - not to add too many things in show

But if you prefer an explicit show, I can change that.

@AparnaKarve before_action adds extra complexity and non-linearity to the code that is hard to follow. Especially if you're using it for just a single controller method, it doesn't even make sense, just complicates the life of anyone who wants to understand it later.

martinpovolny · 2017-11-02T13:02:46Z

app/controllers/generic_object_controller.rb

@@ -27,6 +29,13 @@ def self.populate_display_methods(record)
    end
  end

+  def create_display_methods


If I read this right, this will define a method for each and any value of display that the browser sends. Is that the intended behavior?

The intended behavior is to create a "display_#{key}" method on-the-fly.

key can be any value in the generic_object_definition.properties[:associations][key] that is associated with a valid Model Name.
To give you an example, I have a generic_object_definition record with the following keys/associations -

{"vms"=>"Vm", "services"=>"Service", "cp"=>"ManageIQ::Providers::CloudManager"}

The key cp in the above case (which is a good example of a non-standard display parameter) would be sent via the display parameter to the show method

martinpovolny · 2017-11-03T05:45:44Z

app/controllers/service_controller.rb

@@ -256,6 +256,9 @@ def get_node_info(treenodeid, _show_list = true)
    @nodetype, id = parse_nodetype_and_id(valid_active_node(treenodeid))
    # resetting action that was stored during edit to determine what is being edited
    @sb[:action] = nil
+
+    @nodetype, id = parse_nodetype_and_id(params[:id]) if params[:id]


@AparnaKarve : I understand you need this behavior added here, but it creates another hidden data path in the code.

What I mean: now the input to get_node_info can be passed as an argument treenodeid or (newly) through params[:id].

That is a bad design and it's going to be hard to fix in the future.

Can you move this logic to show? That is where you get with the click, right?

Can you move this logic to show? That is where you get with the click, right?

@martinpovolny I understand, that is the most logical thing to do -- only if it worked.

If you check the Service explorer code, you will notice that show redirects to explorer
and then you get this -

F, [2017-11-03T07:54:49.876401 #79870] FATAL -- : Error caught: [NoMethodError] undefined method `name' for nil:NilClass ~/manageiq/plugins/manageiq-ui-classic/app/controllers/service_controller.rb:292:in `get_node_info' ~/manageiq/plugins/manageiq-ui-classic/app/controllers/application_controller.rb:2365:in `set_active_elements' ~/manageiq/plugins/manageiq-ui-classic/app/controllers/application_controller.rb:2383:in `build_accordions_and_trees' ~/manageiq/plugins/manageiq-ui-classic/app/controllers/service_controller.rb:70:in `explorer'

The whole show method looks really questionable to me.

The nodetype evaluation happens in get_node_info - where the above check that I have added really matters - since show is going to eventually end up there via explorer

Unfortunately, most explorers are just poor quality code, full of bad surprises - precisely the reason why I wanted to stay away from Explorer in Generic Objects.

Without agitating the Service Explorer code too much (as in , a refactor), I think what I have here is our best bet.

Unfortunately, most explorers are just poor quality code, full of bad surprises - precisely the reason why I wanted to stay away from Explorer in Generic Objects.

Well unless people actually start fixing it, it will shay poor quality code.

Adding functionality is an opportunity to cleanup stuff and remove exceptions. Because to add functionality you read the code and see what is bad. Well unless you use try-test-fail approach that actually leads to even worse code ;-)

Adding functionality is an opportunity to cleanup stuff and remove exceptions.

Sure, but not at this given time.
It's an upstream effort.
A refactor going in for the Release, is just plain risky and not a good idea.

martinpovolny · 2017-11-03T07:26:23Z

@AparnaKarve : removing the comments that are no longer valid. The only valid bit is in-line above.

martinpovolny · 2017-11-03T09:55:19Z

@AparnaKarve : linking your GIST so that other people can test/review.

GIST to create necessary GO: https://gist.github.com/martinpovolny/b08a799e2174551a4d7fc6f4bac85ad2

Feel free to replace this comment with your newer/better version.

martinpovolny · 2017-11-03T16:42:59Z

Let me see what I can do with that one ugly line....

martinpovolny · 2017-11-03T17:04:28Z

Well @AparnaKarve : look at 57ae873 where @lgalis added a call to build_accordions_and_trees and below a call to get_node_info.

Turns out build_accordions_and_trees calls set_active_elements and that in turn calls get_node_info. So in case of params[:id] present, get_node_info is called 2x. Total mess.

The commit comes from ManageIQ/manageiq@9281a96 and that is from ManageIQ/manageiq#9809 and there seems to be no spec for that change so I am pretty dammed if I try to fix it.

Now you are suggesting that INSIDE get_node_info (I am reminding that get_node_info is called 2x if params[:id] is passed in) you will look at params[:id] and ignore the input.

So you are adding a hack on top of already ugly hack.

In that context your remark about:

Unfortunately, most explorers are just poor quality code, full of bad surprises - precisely the reason why I wanted to stay away from Explorer in Generic Objects.

sounds like a joke.

I am sorry @AparnaKarve but I will not merge this.

You are complaining about a situation that you are actively creating :-(

Also

stay away from Explorer in Generic Objects

Basically means: "We created so much mess in the explorers that we are going to start over in another place." I don't see how this will fix the situation.

martinpovolny · 2017-11-03T17:11:39Z

@lgalis : Can you, please, show me where the requests that you where fixing in ManageIQ/manageiq#9809 come from? Where should I click to make such request?

I need to help @AparnaKarve fix this BZ and doing that I am likely to break your fix so I need to make sure I handle that situation too. Thank you!

AparnaKarve · 2017-11-03T17:26:16Z

I am sorry @AparnaKarve but I will not merge this.

@martinpovolny Do what you got to do.
I have given you my honest opinion on the code, and what seems like the right thing to do, at this juncture - given that refactoring is probably not a good idea - and it should be considered only as an upstream effort.

lgalis · 2017-11-03T17:56:27Z

@martinpovolny - If I recall correctly, the problem was selecting a node in the foreman tree - then browsing to a different controller then returning to the foreman controller.

martinpovolny · 2017-11-03T18:19:19Z

@AparnaKarve : I don't want to do a refactoring. I want to fix the BZ w/o introducing new technical debt ;-)

AparnaKarve · 2017-11-03T18:32:33Z

@martinpovolny Fixing/Refactoring the show method is what is really needed.
Like I mentioned before, the whole method looks fishy to me with all those redirects.
Also, it is a prominent, high-traffic area and I'm assuming it works for most cases - except for this one that we are trying to resolve.
However bad it might seem right now, it's probably not a good idea to change the code there too much, since we do not know all the use cases (at least, I do not know all the use cases, for sure)

Do take into consideration all the above before making major changes.
Thanks.

martinpovolny · 2017-11-04T17:56:11Z

@AparnaKarve : can you take a look how other places link to Services? Seems to me other places might use x_show. Seems to me you are hacking explorer to accept params[:id] while the functionality you need is already implemented elsewhere.

martinpovolny · 2017-11-04T17:57:50Z

Btw: there seem to be issues with this PR. The services tree seems broken after you come to it from the generic_objects. It seems that the x_active_tree stays set to the one in GO after the entry. It's probably related to some of the hacks old or new.

martinpovolny · 2017-11-04T19:50:50Z

@AparnaKarve : it's actually show that can show a given record so your URL from the quadicon should lead to show rather than to explorer then you don't need to add any hacks. There's even some test coverage for that.

AparnaKarve · 2017-11-06T14:23:25Z

The services tree seems broken after you come to it from the generic_objects. It seems that the x_active_tree stays set to the one in GO after the entry.

@martinpovolny Glad you did this research and now we are finally on the same page!

This was my very first finding and in fact I even discussed with you on gitter (check your Nov 2 gitter)
This finding also happens to be my very detailed commit message in 5b6f26a

AparnaKarve · 2017-11-06T14:50:26Z

It is disturbing how many times you have used the word "hack" in the context of Trees/Explorers.
Why can't it be a straightforward implementation like show_list ?

can you take a look how other places link to Services? Seems to me other places might use x_show. Seems to me you are hacking explorer to accept params[:id] while the functionality you need is already implemented elsewhere.

Before I do that, please explain to me why does the Navigation to VM Explorer from the GO tree works just fine?
Obviously, the Service Explorer is falling short on something.
Or the VM Explorer is doing something extra (as in, looking up the params[:id])

Why are we using the session-based x-node for calculating the nodetype?
Why not just use the routing system that rails offers us out-of-the-box?
Technically every node in the tree can have a unique URL representation - why not leverage that?

The use of params[:id] in the GO tree code has a clear meaning.
I was exploring an approach to be x_node free and base my node calculations on the URL - which did not pan out because of the way the Explorer is written.

AparnaKarve · 2017-11-06T15:02:31Z

@martinpovolny We need to move forward with this BZ/PR - It has taken too long for various reasons -- the GTL fixes that came in so late have really prolonged the whole process :-(

My suggested approach works and currently I am not in the camp of changing the Service Explorer code too much, because it's too close to the release.
However, if you want to take a stab at it, feel free.

Let me know if I should remove the last commit from this PR, which has been the main topic of discussion really.
That way we can at least make the PR available for other cases (i.e. cases other than Services)
Thanks.

martinpovolny · 2017-11-06T15:23:57Z

Yes @AparnaKarve, to do the review of your PR I have to do all this research and as a reward instead of an answer I get another question from you.

2 or the 4 patches in his PR are the result of me spending time on this task figuring out how to do things properly where you hacked your way through!

It's so much faster for me to so the things myself than to do these lengthy reviews and rewrites. An elementary respect that I would like to see would have the form of you actually answering my question so that I don't have to do more research. But no. I get another question instead and then a complaint about ugly code.

Before I do that, please explain to me why does the Navigation to VM Explorer from the GO tree works just fine?

Why should I know that? I have not written that hack there! You tell me, this is your PR! You are adding that here.

I have not YET rewritten the explorers. I have not. I cannot rewrite everything at once. You can do that instead of these discussions. Come up with a design, do a WIP, have it reviewed, fix it.

As to your question: yes, remove the last commit, lets merge the 3.

The last commit is a hack on top of a hack.

If you are not willing to spend time fixing the problem I will try in another PR.

AparnaKarve · 2017-11-06T16:29:45Z

I have to do all this research

Correction. The research was already done by me - you did not have to do it again.

Why should I know that? I have not written that hack there! You tell me, this is your PR! You are adding that here.

Not true.
The PR may be mine, but I haven't added any code in VM Explorer to make it work, and you can look that up.

As to your question: yes, remove the last commit, lets merge the 3.

Thanks. I will remove the last commit.

A different PR for Service Explorer is a good idea - no reason to keep the other working implementation blocked here.

martinpovolny · 2017-11-06T16:47:56Z

To answer your remaning questions here:

Why are we using the session-based x-node for calculating the nodetype?

I don't know. That code was there before I started contributing to this project. I worked on unifying that (all the x_(active|)_(tree|accortion|node) comes from my work. I see such unification as a step towards removal. Help in this area is welcome.

Why not just use the routing system that rails offers us out-of-the-box?

I don't know. That code was there before I started contributing to this project. I have done a lot of work to cleanup and unify the various routes used by various controllers so that we can normalize it.

There's a lot of teamwork happening in that area. The unificafion of the nested lists was started by @Ladas and is continued by me, @karelhala, @tumido and others.

It's a long way but cleaning up the nested views is pre-requisite for using normal Rails routes.

Should you have a better plan or idea on how to "use the routing system that rails offers us out-of-the-box" I'd love to hear your suggestions and plans.

Can I get my answer regarding the the other places that link to Services? I can use your research here so that I don't have to redo it.

You corrected me:

Correction. The research was already done by me - you did not have to do it again.

Well I need the results of your research if I don't have to do it again ;-) Please, share.

Created `display_methods` and `display_nested_generic` to be able to whitelist GOD associations and create nested lists for those associations

AparnaKarve · 2017-11-06T17:22:30Z

First things first - so here's an update to the PR which excludes the last commit.
(5b6f26a was the last commit that was excluded)

miq-bot · 2017-11-06T17:24:24Z

Checked commits AparnaKarve/manageiq-ui-classic@d1f5b65~...9d9789d with ruby 2.3.3, rubocop 0.47.1, and haml-lint 0.20.0
3 files checked, 0 offenses detected
Everything looks fine. 🍪

martinpovolny · 2017-11-06T18:33:07Z

Cool, I guess you can remove the WIP and add the necessary labels. From my perspective this is ok. It removes the redefinition of the static class method on instance method call plus it adds a nice test.

I like it 👍

Ready to merge then?

Then, please, share your findings so that I can have a look at the loose end about link to Services.

AparnaKarve · 2017-11-06T19:36:27Z

Thanks.
WIP should go away in a minute.
@miq-bot remove_label wip

Then, please, share your findings so that I can have a look at the loose end about link to Services.

Sounds good.

Ready to merge now.

AparnaKarve · 2017-11-07T00:11:45Z

@martinpovolny The VM Explorer is probably even worse than Services Explorer.

This is what I have so far --

A single show call -

creates @record many times - effectively making multiple DB calls to get @record
goes through 6 to 7 methods before it does the final render
nodetype related parsing (parse_nodetype_and_id ) seems to be implemented several times

On the positive side, it does not rely on the session-based x_node
Right off the bat, it creates @record using -

@record = identify_record(id || params[:id], VmOrTemplate)

Followed by -

tree_node_id = TreeBuilder.build_node_id(@record)

I think that's the main differentiator between these 2 explorers, and the reason why navigating to VM Explorer works and navigating to Service Explorer doesn't

The overall show path is as shown below (I may have left out a few methods, but you get the idea)

show -> explorer -> set_elements_and_redirect_unauthorized_user -> resolve_node_info -> get_node_info -> show_record -> back to explorer to do the final rendering

martinpovolny · 2017-11-07T07:05:22Z

Thanks @AparnaKarve.

However my main question was about:

... other places that link to Services?

We want to fix a situation here where linking to Services won't work, right? So I am asking still the same question, because I want to fix it while unifying things.

I assumed that you would know, because you where adding such place. If you don't know, just tell me, I am ok with that.

In the context of your earlier statement:

Correction. The research was already done by me - you did not have to do it again.

I wanted to prevent my own research if you already did that.

AparnaKarve · 2017-11-07T14:42:02Z

other places that link to Services?

I am really not aware of other places that link to Services.
Although, what I do know is that when GO used to be purely show_list, we did not have this issue because there was no x_node.

So by extrapolating all the observations so far -

The Services link works if the referrer is a show_list type of controller
The Services link will not work if the referrer is an explorer type of controller where an x_node value is set

martinpovolny · 2017-11-07T14:52:54Z

... and add the necessary labels.

Ok, let me add the labels for you.

…ce_associations_after_gtl_fixes Display Generic Object Associations correctly (Post `/report_data` related GTL fixes) (cherry picked from commit d5d7f4f)

simaishi · 2017-11-15T15:17:21Z

Gaprindashvili backport details:

$ git log -1
commit d43ca17fa7231d16d1182cec478fa0546c436772
Author: Martin Povolny <mpovolny@redhat.com>
Date:   Tue Nov 7 15:53:12 2017 +0100

    Merge pull request #2596 from AparnaKarve/bz1500829_display_go_instance_associations_after_gtl_fixes
    
    Display Generic Object Associations correctly (Post `/report_data` related GTL fixes)
    (cherry picked from commit d5d7f4f91ab69761c7039d6d829a55cab9f83143)

AparnaKarve mentioned this pull request Nov 1, 2017

Display Generic Object Associations correctly #2420

Closed

martinpovolny reviewed Nov 2, 2017

View reviewed changes

AparnaKarve changed the title ~~Display Generic Object Associations correctly (Post /report_data related GTL fixes)~~ [WIP] Display Generic Object Associations correctly (Post /report_data related GTL fixes) Nov 2, 2017

miq-bot added the wip label Nov 2, 2017

AparnaKarve force-pushed the bz1500829_display_go_instance_associations_after_gtl_fixes branch 2 times, most recently from 695321d to 5b6f26a Compare November 2, 2017 21:39

martinpovolny reviewed Nov 3, 2017

View reviewed changes

ManageIQ deleted a comment from AparnaKarve Nov 3, 2017

Martin Povolny and others added 3 commits November 6, 2017 09:19

Generic Show Mixin adjustments to be able to override methods

d1f5b65

Created `display_methods` and `display_nested_generic` to be able to whitelist GOD associations and create nested lists for those associations

Remove the call to populate_display_methods

0be41f5

spec for displaying a nested list of GO associations

9d9789d

AparnaKarve force-pushed the bz1500829_display_go_instance_associations_after_gtl_fixes branch from 5b6f26a to 9d9789d Compare November 6, 2017 17:19

martinpovolny self-assigned this Nov 6, 2017

AparnaKarve changed the title ~~[WIP] Display Generic Object Associations correctly (Post /report_data related GTL fixes)~~ Display Generic Object Associations correctly (Post /report_data related GTL fixes) Nov 6, 2017

miq-bot removed the wip label Nov 6, 2017

martinpovolny added bug gaprindashvili/yes generic objects labels Nov 7, 2017

martinpovolny added this to the Sprint 73 Ending Nov 13, 2017 milestone Nov 7, 2017

martinpovolny merged commit d5d7f4f into ManageIQ:master Nov 7, 2017

yaacov mentioned this pull request Nov 12, 2017

Add placeholder record in display methods #2703

Merged

simaishi added gaprindashvili/backported and removed gaprindashvili/yes labels Nov 15, 2017

martinpovolny mentioned this pull request Nov 17, 2017

Allow linking to Services adding test coverage for Services Explorer #2758

Merged

Display Generic Object Associations correctly (Post /report_data related GTL fixes) #2596

Display Generic Object Associations correctly (Post /report_data related GTL fixes) #2596

Conversation

AparnaKarve commented Nov 1, 2017 • edited

AparnaKarve commented Nov 1, 2017

AparnaKarve commented Nov 1, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

martinpovolny commented Nov 3, 2017

martinpovolny commented Nov 3, 2017

martinpovolny commented Nov 3, 2017

martinpovolny commented Nov 3, 2017 • edited

martinpovolny commented Nov 3, 2017 • edited

AparnaKarve commented Nov 3, 2017

lgalis commented Nov 3, 2017 • edited

martinpovolny commented Nov 3, 2017

AparnaKarve commented Nov 3, 2017

martinpovolny commented Nov 4, 2017 • edited

martinpovolny commented Nov 4, 2017

martinpovolny commented Nov 4, 2017

AparnaKarve commented Nov 6, 2017

AparnaKarve commented Nov 6, 2017

AparnaKarve commented Nov 6, 2017

martinpovolny commented Nov 6, 2017 • edited

AparnaKarve commented Nov 6, 2017

martinpovolny commented Nov 6, 2017

AparnaKarve commented Nov 6, 2017

miq-bot commented Nov 6, 2017

martinpovolny commented Nov 6, 2017

AparnaKarve commented Nov 6, 2017

AparnaKarve commented Nov 7, 2017 • edited

martinpovolny commented Nov 7, 2017

AparnaKarve commented Nov 7, 2017 • edited

martinpovolny commented Nov 7, 2017

simaishi commented Nov 15, 2017

Display Generic Object Associations correctly (Post `/report_data` related GTL fixes) #2596

Display Generic Object Associations correctly (Post `/report_data` related GTL fixes) #2596

AparnaKarve commented Nov 1, 2017 •

edited

martinpovolny commented Nov 3, 2017 •

edited

martinpovolny commented Nov 3, 2017 •

edited

lgalis commented Nov 3, 2017 •

edited

martinpovolny commented Nov 4, 2017 •

edited

martinpovolny commented Nov 6, 2017 •

edited

AparnaKarve commented Nov 7, 2017 •

edited

AparnaKarve commented Nov 7, 2017 •

edited