Display Generic Object instances in Service summary

[AparnaKarve]

Generic Object instances in the Service Detail Page -

Drilling down in the GO instances -
(Tile View displays Definition (Generic Object Class)

(List View displays Definition (Generic Object Class) column

https://www.pivotaltracker.com/n/projects/1613907/stories/148040727

[gmcculloug]

@lfu Please review

[gmcculloug]

@AparnaKarve @dclarizio Was there talk about showing the generic objects on the service page directly like the VMs are? They generic_object_definition supports a picture property much like service_templates so each type could be displayed as a separate element with it's corresponding image.

Also, on the "Drilling down in the GO instances" list-view image I think it would be important to include the generic_object_definition name as a column, as the name may not also be descriptive enough. If it is sortable then it would be nice to group them by generic object definition type.

[AparnaKarve]

Was there talk about showing the generic objects on the service page directly like the VMs are?

@gmcculloug I initially intended to show the generic objects exactly like the VMs.
But there is design flaw with the current angular GTL design.
On a given page, only 1 GTL view is possible. An attempt to render a second GTL gives an angular bootstrap error.
Cc-ing @karelhala here to get more info on this issue and if it could be fixed soon.

That's pretty much the background of why I went with the current design.

Although with the current design, we could still accomplish what you mentioned above.
i.e. Once you get to the GTL for GO, I think we could display the individual GOs with their respective images.

Also, on the "Drilling down in the GO instances" list-view image I think it would be important to include the generic_object_definition name as a column

Agree. I plan to implement that in a separate PR.
And will also make sure that it is sortable by the GO Class name.

[karelhala]

@AparnaKarve well the biggest problem with multiple GTLs per one page is that where to show pagination for each of these. However good idea to split GTL view into GTL object and pagination (pagination is already separate component) and let someone use GTL object in any place (ManageIQ/ui-components#160).
But I think that this should be addressed by UX team as well, because a lot of questions comes up in my head - where to show these generic objects (above, or below VMs), show pagination for each GTL, or just for VMs?

I think that the best approach here would be to show one GTL which would include both VMs and generic objects, no need to discuss where to put pagination and how to position second GTL.

[lfu]

Should we display a clickable generic_object_definition object in the generic object summary page?

[AparnaKarve]

@karelhala Thanks for the reply.

I think that the best approach here would be to show one GTL which would include both VMs and generic objects, no need to discuss where to put pagination and how to position second GTL.

I don't think we want to merge the GTL for VMs + GOs, so the approach that you suggested above is probably not an option.

[gmcculloug]

@lfu I do not think that would be required, most users will only want to interact with the objects and only admins would be concerned with the definition. Once people start using it I would expect that we would get feedback having that link would be useful.

I think it is a good question to ask and would be interested if other had a different opinion.

[AparnaKarve]

@gmcculloug I have pushed a few more changes to support displaying custom images for GOs.
("Drilling down in the GO instances" screenshot has been updated above)

Note that, we also need a model change to be able to view those custom images for GOs.
PR - ManageIQ/manageiq#16006

[chriskacerguis]

@AparnaKarve is this using the DB or the API?

[AparnaKarve]

@chriskacerguis We currently do not have Summary screens that use API in the Classic UI.
Probably something that we will be looking into ... I'm guessing in the H Release.
...So it's the good old Rails way approach that is being used here.

[chriskacerguis]

@AparnaKarve so just playing devils advocate here...are we adding new code that has immediately technical debt attached to it? What about doing something like this here:

# Pseudocode
# @record = Service.find(from_cid(params[:id]))
@record = REST.client("https://domain.com/api/whatever/[:id]

If the endpoint doesn't exist, perhaps it would be prudent to wait on this feature until the API is ready, and instead work on something that either (1) does have an endpoint, (2) making a new endpoint for this first, or (3) working to cleanup more tech debt?

I know it's a balance between getting things done, I'm just wondering if this is a short term win, but in the long run adding new code with tech debt makes it that much harder to add new things.

Thoughts @AparnaKarve @Fryguy @chessbyte @ohadlevy @dclarizio ?

P.S. Regardless...that's some solid Ruby code :)

[AparnaKarve]

@chriskacerguis

$ find . -name textual_summary.rb | wc -l
      65

Currently in this PR, I'm just staying consistent with the 65 Textual Summary screens.

Converting these Textual Summary screens to Angular+API will be a project in itself that we will be undertaking in the future.

Our usual Method of Operation is to first create a POC PR that demonstrates a particular concept, after which the conversion takes effect across the board PR by PR.
I believe all of this will happen for Textual Summaries too, but just not for the G release.

[chriskacerguis]

@AparnaKarve I think that it is a step-by-step process. In the case of this code, you are 100% correct moving to Angular+API will be a BIG project. In this case, I'm just wondering if we should use the API instead of direct DB access. So, in the Ruby code, instead of this:

@record = Service.find(from_cid(params[:id]))

we do something like this:

@record = REST.client("https://domain.com/api/whatever/[:id]

(you would need to maybe change variable / array key names)

In this case, you can keep going as you are, but just use the API for data access. Yes, there is still technical debt that happens (i.e. we aren't using Angular), BUT at least we have reduced the technical debt made (Angular+API > just Angular).

Also, I'm not trying to suggest that this is any different from other merged code (or you are doing anything wrong). I'm just asking that perhaps we need to start somewhere; with the way that code is, there will always be a dependency on something or there will always be a "that's the way it has been done" type scenario.

When we go a post-G release, how do we balance new features? What if there is no API for those new features? Do we keep going without using the API? My thought is that, at some point one has to decide to break that loop (i.e. you have to start somewhere).

[dclarizio]

@chriskacerguis

@record = REST.client("https://domain.com/api/whatever/[:id]

If you are suggesting calling the API from the Ruby (server) side, we had looked into this a while back and ran into some issues (threading, if I recall) and decided to not pursue it and to keep our forward momentum on the browser side client calling the API.

The summary screens have been refactored into a set of Ruby helpers that are quite easy to work with now. The biggest issue here was that we haven't had a need for multiple lists on a summary screen in the past. I think, for this release at least, going with the link of the count of GO instances that, when clicked, shows a sublist of them will hold us off.

We do want to get into this at some point, but we also have to be careful not to start too many refactoring projects at once or we'll not finish any of them, as I'm sure you're aware. 😄

As for actual new features that aren't just additions to other screens, we are using JS based API calls and, when the APIs don't exist, either having the API team work on them or creating them ourselves, as @AparnaKarve has done recently.

[dclarizio]

@AparnaKarve can we make sure that both the list and tile views (tho I don't see the toggle buttons) of the GO instances contains the GO class name?

[chriskacerguis]

@dclarizio I'd like to understand the issue with the API / threading(?) you mentioned.

I understand the refactoring part, but would it not be time better spent making the API, then using it, and resolving the threading(?) issue? Again, that way we still reduce the amount of technical debt? The API will have to get created one way or the other...so we can do it now are both UI's can benefit...or continue to not have alignment with the UIs?

[dclarizio]

@chriskacerguis

I'd like to understand the issue with the API / threading(?) you mentioned.

I'm not sure of the technical details, but something to do with the api coming thru to the Rails server the same as the transaction that was trying to call it. Either way, it wasn't our end goal, so we never pushed solving it.

IMO, it will be easier to tackle the refactoring of the summary screens from the client side. We actually are considering dropping the summary screen boxes into some dashboard widget PF cards, so we can just create API calls for each type of those. Then, the summary screens basically become mashable dashboard components, similar to what the SUI uses (or I hope they use). At that point, both the SUI and OPS UI can show those "widgets" mixed in with the other existing PF cards.

[AparnaKarve]

can we make sure that both the list and tile views (tho I don't see the toggle buttons) of the GO instances contains the GO class name?

@dclarizio You are right, the GTL toggle buttons were not being displayed before due to a bug that I have resolved in this commit 8a8eb23 (part of this PR)
I have also updated the screenshots above.
The screenshots now display the GO class name in the Definition column.

[dclarizio]

@AparnaKarve I think the title to the GO box on the summary screen should say "Generic Objects" (plural).

@bascar For showing the type of Generic Object (i.e. what us engineers call the "Definition"), would you prefer it to say "Class" or "Type" or leave it as "Definition"? Remember, this would be an end user seeing this, not an admin.

[martinpovolny]

@AparnaKarve : the code looks good. But there's no test at all. Can you, please, create some simple tests that make sure that the screens render? We need to prevent regressions and we are going to work on this area in the future for sure ;-)

[martinpovolny]

@chriskacerguis, @dclarizio : I'd like to convert all the GTLs to use the API as one block. To do that we need this: ManageIQ/manageiq#14924

Also I'd like to convert all the textual details screen to use the API as a block.

People using the lists, nested_list and textual summary code such as the TextualGroup class should be ok doing so.

The "platform" should handle converting all these to go through the API rather than have people spend months converting the screens one by one.

[AparnaKarve]

@martinpovolny The Hakiri Security warnings started showing out of the blue after my latest update to this PR (I added the last 2 commits - 14463c0 and ababbed today, which, as you can see are quite straightforward).
If you check the Hakiri report, it talks about some CVE's in the Rails code ... so it's not the usual params[:id] related error.

Could you please take a look and let me know if there is anything that we could do to fix those? Thanks.

[miq-bot]

Checked commits AparnaKarve/manageiq-ui-classic@e90e321~...ababbed with ruby 2.3.1, rubocop 0.47.1, and haml-lint 0.20.0
6 files checked, 0 offenses detected
Everything looks fine. 🍪

[AparnaKarve]

The Hakiri report clearly says that it has 5 security warnings with 0 in code and 5 in dependencies -

5 security wanings in dependencies, which means they need to be resolved outside of this PR.

[martinpovolny]

@AparnaKarve : yes, it's due to the new rails version that we merged yesterday. I will whitelist it today, please, ignore that.