Archived container records should not be displayed in the UI

[zakiva]

@himdel We use this to narrow the search before displaying the objects list (from container_group_controller.rb) :

  def show_list
    process_show_list(:where_clause => 'container_groups.deleted_on IS NULL')
  end

However, the displayed list contains all the records including the archived ones.
This may be related to the new way of fetching the data introduced in #592? I think it ignores the options passed from show_list, is there a new way of narrowing the search?

cc @simon3z @zeari @karelhala

[zakiva]

@himdel can you please take a look?

[himdel]

I think this is the same problem as in #1553 .. so please read the discussion there..

I'm not sure if the current state of that PR will get merged (the special case is small enough, but I'm baffled as to why it's not needed elsewhere as well, and would really like to see the SQL live in models, not UI), but at least there's more examples...

Also, maybe they should be visible somewhere, same as archived/retired VMs, shouldn't it?

[zakiva]

Also, maybe they should be visible somewhere, same as archived/retired VMs, shouldn't it?

Archived container objects should not be visible in the UI, we keep them in the DB to be used in other areas (e.g. reports, chargeback)

[himdel]

Some progres.. looks a lot like the other issue is just a missing association on the backend.
Possibly the same here?

EDIT: not sure about that though..

[zakiva]

It seems like a similar issue. As mentioned in #1553 (comment) the get_view method is called twice, the problem is in the second call: report_data calls get_view without the options that were passed in from the first place.
It can be fixed by adding a special case like done in https://github.com/ManageIQ/manageiq-ui-classic/pull/1553/files#diff-55c5b7aecfb519d0e4880eaf2788eb6eR315, but why not fix this issue as expected- we are losing here a functionality which may come up in some other places too.

[karelhala]

We could send options object while calling report_data endpoint. However we aim to have this as API endpoint and I don't think it's wise to send where clauses to fetch data. I think that this should be generated based on the model and controller which invoked such function.

I recommend creating new function in controller which would be let's say populate_options and each controller would add some specific where clauses and return them, report_data function could later on change these options based on some other things.

What do you think about this @himdel?

[himdel]

@karelhala Definitely all for consistency 👍 Maybe populate_options sounds to generic, populate_report_options maybe? Or just report_options..

Agreed we can't be sending any SQL when calling report_data, otherwise I would have preferred to explicitly send it all :(.

EDIT: .. I think Martin had an idea about "report profiles" .. as in, you'd have a place which defines all the options for all the gtl reports, and simply call report data with the name of that profile, and it would probably be easier to move that to the API at some point.. but we can start with the populate_options idea and slowly transform it to that - for many controllers, it is the same thing.

[simon3z]

cc @Ladas

[Ladas]

btw. a correct way is to use named_scope https://github.com/ManageIQ/manageiq-ui-classic/pull/1462/files

let make sure we don't put SQL queries(not even partial where conditions) anywhere in the UI code

[zakiva]

@himdel @karelhala are there any updates here?

[himdel]

@zakiva Looks like there's a PR out there fixing this issue - #1524 .. so I guess when we can merge that..

EDIT: oh, it's yours .. so .. umm, what are you asking?

[zakiva]

@himdel yeah #1524 is indeed my PR :) it's actually not fixing this issue, it only deals with the new archiving of Container Node.
As discussed above, there is currently a problem for all archived Container entities being displayed in the UI (while they should be hidden). We used to narrow the results in the UI by passing a :where_clause from the controller which was changed to :named_scope instead (in #1462), but the problem remains the same: it seems that the report_data method (when it called twice?) wouldn't consider the options passed from the controller.
So, I may just missing something here, but I guess my question is if there is any work done to change report_data to allow the controller the option of narrowing the search (or other options, as may be needed)?

[himdel]

Aaah, that makes sense, thanks :) ..

I think we should just be passing that named scope with all the other params to the report_data request (something we can't do for where_clause, but should be perfectly safe for named scopes).. Will take a look..

[martinpovolny]

process_show_list(:where_clause => 'container_groups.deleted_on IS NULL')

There should be no SQL anywhere in the UI

Why is this not part of the report definition? If this cannot be directly in the report then is should be a named scope at least. Although I'd rather have just one place to define the nested list/report.

As a sidenote: we really need an API for this: ManageIQ/manageiq#14924

Please, avoid the use of :where_clause we need to remove that.

Please don't add new exceptions. Try to make this particular case the same as are the other cases.

[zakiva]

process_show_list(:where_clause => 'container_groups.deleted_on IS NULL')
There should be no SQL anywhere in the UI

Why is this not part of the report definition? If this cannot be directly in the report then is should be a named scope at least. Although I'd rather have just one place to define the nested list/report.

As a sidenote: we really need an API for this: ManageIQ/manageiq#14924

Please, avoid the use of :where_clause we need to remove that.

Please don't add new exceptions. Try to make this particular case the same as are the other cases.

@martinpovolny We already changed that to named scope, please see my comment above: #1526 (comment)

[cben]

detailed repro:

1. compute -> containers -> container nodes
   

2. rails c:

   n = ContainerNode.first
   n.archived?
   #=> true
   n.disconnect_inv
   

3. reload => actual: same picture, the archived node still shows (expected: shouldn't show)

4. undo damage, in rails c:

   n.update(deleted_on: nil)
   

https://gist.github.com/cben/30cb93f5991d222a2315d503028cf501#file-rails-s-L48 has the actual query, does not filter by deleted_on.

[cben]

aha, the problem only happens with global view of all nodes; a specific provider's list of nodes (eg. http://localhost:3000/ems_container/1?display=container_nodes) does filter out archived nodes:

And, as @zakiva says, this happens with all container entities that support archiving, not just nodes.

[himdel]

Aaah.. so that PR fixed just nested lists, but not the main view.. Now I undestand, thanks @cben! :)

[cben]

P.S. anybody looking into how it works and where the query actually happens, see also #2163 about it doing 3N+1 queries.

[zeari]

I no longer see archived nodes in the UI, can this be closed?

[cben]

👍 Confirmed works on gaprindashvili too (both provider's nodes and all nodes views).

For the record the fast hacky way to test this is ContainerNode.last.disconnect_inv and undo by ContainerNode.last.update(deleted_on: nil).