Fixes #35274 - Make columns on host index page selectable #9319

ofedoren · 2022-07-25T12:56:55Z

This PR is logically divided into 4 smaller commits for easier review. The actual change which fixes https://projects.theforeman.org/issues/35274 is in 92f68ca, but 03dca15 and f23cd03 is required to be able to make those changes. 059ecc7 is extra commit which adds plugin support and some documentation.

This works only for ERB based pages. Some information can be used in React components (such as PF4 Selector @pkoprda works on), but in order to make columns selectable via React, we would need to rewrite the whole page in it and come with a different approach (probably it will require resource presenters or a way to extend actions' response from plugins and add some Slot&Fill support with filtering).

This solution is not the best in terms of efficiency, so if there will be a better suggestion with a different approach, I'm open to it. Otherwise I propose this for 3.4, so we can collect users' feedback early. Meanwhile, I'm going to create a post on our forum about what additional columns we want to include on host index page.

Also, I've tried to make it as general as I can imagine, so there is support for plugins to be able to add some columns to existing pages or even use the storage within plugin itself to be able to make plugin's pages with selectable columns.

How to test it: this feature uses users' table preferences which can be defined via API or hammer (latest develop branch only for now). For this particular page you would need to call, e.g.

hammer user table-preference create --user login --name hosts --columns name,os_title

There are some issues:

I'm not sure what to do with Power column, since we use a setting to show/hide it. Can be refactored, so we no longer need to use settings for it.
Until we update some plugins (e.g. foreman_puppet, foreman_rh_cloud, etc), some columns will be shown regardless of user preferences.
Until we have UI selector, we would need to tell users how to select the columns via API/hammer. This can lead to potentially large list of tables + columns mentioned in API docs.

Concerns:

By default users have empty table preferences. To not break UX with this feature, there are some column categories that can be defined as default. In this case default categories will be shown if user didn't configure any preferences for a table.

TODO:

- post about additional columns: https://community.theforeman.org/t/rfc-categories-columns-for-hosts-index-page/29579
- a way to list available columns for API requests
- tests :/

theforeman-bot · 2022-07-25T12:56:58Z

Issues: #35274

ofedoren · 2022-07-25T13:16:06Z

@ares, I guess you're the first one wanting to test this out :)

ares

a quick comment from the first read, looks straightforward but don't take it as a full review :-)

ares · 2022-07-25T17:41:01Z

app/registries/foreman/selectable_columns/storage.rb

@@ -16,6 +16,12 @@ def define(name, &block)
          tables[name] = table
        end

+        def register(name, &block)
+          Foreman::Logging.logger('selectable columns').info _('Table %s is not defined, ignoring.') % name unless tables[name]


new logger will require a new entry in settings.yml.example

Actually, I don't think we need a new logger. This message is for devs only. Ideally, users shouldn't see those messages at all. I'll use app logger here.

adamruzicka

Just to get my mental model straight:
Tables have columns, columns belong to categories. Categories themselves are not really displayed anywhere in the ui, they are just used as a container to mark some columns as default? Is there any additional functionality planned for them?

From what I've seen it works nicely, however the page looks slightly odd if you remove too many columns, but we can live with that for now.

app/helpers/selectable_columns_helper.rb

app/registries/foreman/selectable_columns/storage.rb

developer_docs/how_to_create_a_plugin.asciidoc

ofedoren · 2022-07-27T12:22:40Z

Tables have columns, columns belong to categories. Categories themselves are not really displayed anywhere in the ui, they are just used as a container to mark some columns as default? Is there any additional functionality planned for them?

Well, yes:

It's used for internal filtering. If it seems redundant, we can get rid of it. DSL should not differ, but not sure yet. Also, we would need to add default and category info into columns themselves, then go throught all of them and check/grab such info later. I guess it's up to taste mostly.
But mainly it's used for UI. After is done, we would show selector with categories which logically divide columns. E.g. Puppet related columns could go under Puppet category. Katello related could go into Content category, etc. Internal mapping kinda ignores categories: users create their preferences based on columns directly, we map those preferences on defined columns to get a subset of them. Although categories come in handy in case users don't have any defined preferences: we don't map anything, just grub all columns from default categories.

ares · 2022-07-27T14:30:09Z

I guess the categories would help with having presets? As a user I don't want always to nitpick 5 out of 40 columns. I'd love to have a preset "compliance manager" which shows me compliance relevant data or "devops" which would show me puppet/ansible data or "patching sysadmin" which shows me counters of applicable errata. I'm not sure if it was meant that way or not, perhaps we'd need a way to have single column in multiple categories. But this is certainly something we should consider in future.

ofedoren · 2022-07-27T15:32:34Z

I guess the categories would help with having presets? As a user I don't want always to nitpick 5 out of 40 columns. I'd love to have a preset "compliance manager" which shows me compliance relevant data or "devops" which would show me puppet/ansible data or "patching sysadmin" which shows me counters of applicable errata. I'm not sure if it was meant that way or not, perhaps we'd need a way to have single column in multiple categories. But this is certainly something we should consider in future.

@ares:
Well, my understanding is that it goes like this:

all columns is a superset, which is created by different category subsets.
category is a set of columns, uniq subset of all columns.
preset is a set of columns from different categories.

Currently if users change their table preference in a way we have now, it means they create their own preset and to be able to do so, we need to show all columns (this leads to nitpick 5 out of 40 for example).

I just need to be sure what we want to share with users:

First approach: all columns, users can nitpick 5 out of 40 via column selector. Creates users' own one preset. Just flexible. Currently done.
Second approach: subset of columns, users can choose preset via preset selector. Less flexible since we define the presets and users can't modify them. Kinda done: we have default preset. Other presets can be added after we have any idea about what they should contain as well as we need plugins to use this feat sooner. This also makes presets depended on plugin presence more. In this approach we could also treat categories as presets. It would mean that a plugin could define a category with their own new columns and reuse columns from the default category or even from different plugin's category (simply tries to find a column by its key if it was defined, if not -- nothing happens)
Third approach: mixed solution. We define presets, users can choose preset via preset selector. Users can create their own preset from all available columns. Most flexible, but this PR is not ready for this yet. I guess we would need to rewrite TablePreference model if we want users to be able to create multiple presets, because currently it works like this: define a set of columns for a table. Which basically means creation of one preset defined by users themselves.
Fourth approach: the third approach, but limits users to have only one their own preset, which means simply changing the default preset. Doesn't require much changes in this PR, no need to touch current TablePreference model.

UPD: If we will go with the fourth approach, then we can merge this PR as is, so we have something for 3.4. For 3.5 I'll be updating current solution, so we fully support fourth approach. From users perspective the only change will be visually updated selector (if we manage to merge the current one as well). API side is not going to be changed. We would only add things (probably only one new preset parameter).

UPD2: Currently, this feature works as described in #9319 (comment), but with fourth approach compatibility. We will treat categories as presets. The only preset users can change is the default one as per table preferences. In UI we will have selector (Dropdown with checkboxed TreeView) which will allow to pick either provided preset or modify the default one with any columns. Internally it will still be an array of columns.

app/helpers/selectable_columns_helper.rb

app/controllers/concerns/application_shared.rb

ares · 2022-07-28T10:55:23Z

From user perspective, I always want to tweak which columns to see, e.g. specific 5 out of all 40. But if I'm a new user, I'd like the UI to guide me, e.g. if I chose I'm a compliance manager, I want the UI to preselect 7 specific checkboxes. I can still tick or untick more, but I'm happy for the app's suggestion of what I'll probably be interested in.

Note that some of the columns would be shared among these presets, e.g. host name is probably a good candidate for all of them.

My preference would be go the the 3.4 version of the DSL ready for additions in 3.5 so that plugins will specify the columns just once. So I'd be ok with merging something we don't use in the current version in 3.4. But I can also understand it is hard to predict and perhaps a later change will be inevitable.

ofedoren · 2022-07-28T19:34:48Z

@adamruzicka, I guess it's ready now :)

adamruzicka

One last suggestion, apart from that it looks good

config/initializers/foreman_register.rb

adamruzicka

This looks good to me and I'd vote for getting it in.

Even though we're in stabilization week, I'd like to get this in for 3.4. I'll leave this open until wednesday to give people time to voice their concerns, if they have any.

adamruzicka · 2022-08-03T08:54:05Z

As far as I know no concerns have been raised, merging.

adamruzicka · 2022-08-03T08:54:36Z

Thank you @ofedoren !

ares · 2022-08-03T13:42:56Z

Please don't forget to demo this, this is a great addition. Worth listing as a 3.4 headline feature.

…foreman#9319)" This reverts commit b1b46b6.

…)" This reverts commit b1b46b6.

Since b1b46b6 the labels are translated during initialization. This is because it uses () instead of N(). The latter only marks a string for translation. Doing this leads to infinite recursion while starting up when updating to fast_gettext 2.1+. It can also lead to other issues where a column can be translated multiple times. Fixes: b1b46b6 ("Fixes #35274 - Make columns on host index page selectable (theforeman#9319)")

Since b1b46b6 the labels are translated during initialization. This is because it uses () instead of N(). The latter only marks a string for translation. Doing this leads to infinite recursion while starting up when updating to fast_gettext 2.1+. It can also lead to other issues where a column can be translated multiple times. Fixes: b1b46b6 ("Fixes #35274 - Make columns on host index page selectable (#9319)")

Since b1b46b6 the labels are translated during initialization. This is because it uses () instead of N(). The latter only marks a string for translation. Doing this leads to infinite recursion while starting up when updating to fast_gettext 2.1+. It can also lead to other issues where a column can be translated multiple times. Fixes: b1b46b6 ("Fixes #35274 - Make columns on host index page selectable (theforeman#9319)")

ofedoren added 3 commits July 25, 2022 12:41

Fixes #35274 - Add SelectableColumns registry

03dca15

Refs #35274 - Add SelectableColumns renderer

f23cd03

Refs #35274 - Use SelectableColumns on hosts

92f68ca

theforeman-bot added Needs testing Not yet reviewed Docs labels Jul 25, 2022

ofedoren force-pushed the feat-35274-selectable-columns branch 2 times, most recently from bd2750d to 01eb682 Compare July 25, 2022 13:24

Refs #35274 - Add plugin support and docs

059ecc7

ofedoren force-pushed the feat-35274-selectable-columns branch from 01eb682 to 059ecc7 Compare July 25, 2022 13:25

ares reviewed Jul 26, 2022

View reviewed changes

adamruzicka self-requested a review July 27, 2022 07:53

MariaAga assigned adamruzicka Jul 27, 2022

adamruzicka reviewed Jul 27, 2022

View reviewed changes

app/helpers/selectable_columns_helper.rb Outdated Show resolved Hide resolved

app/registries/foreman/selectable_columns/storage.rb Show resolved Hide resolved

developer_docs/how_to_create_a_plugin.asciidoc Outdated Show resolved Hide resolved

Refs #35274 - Change logger, fix typo, use map instead of reduce

8923e74

Refs #35274 - Reduce DB calls, update UI data

9196997

pkoprda mentioned this pull request Jul 27, 2022

Fixes #35287 - Create column selector on host index page #9323

Merged

adamruzicka reviewed Jul 28, 2022

View reviewed changes

app/helpers/selectable_columns_helper.rb Outdated Show resolved Hide resolved

app/controllers/concerns/application_shared.rb Outdated Show resolved Hide resolved

ofedoren force-pushed the feat-35274-selectable-columns branch 3 times, most recently from 5ab6909 to f4f3752 Compare July 28, 2022 15:50

Refs #35274 - Fix tests, simplify finder

8e1c80c

ofedoren force-pushed the feat-35274-selectable-columns branch from f4f3752 to 8e1c80c Compare July 28, 2022 15:52

Refs #35274 - Add possibility to re-use columns

4211b88

Refs #35274 - Add tests

8406f6c

ofedoren force-pushed the feat-35274-selectable-columns branch from 05b9415 to 8406f6c Compare July 28, 2022 19:33

Refs #35274 - Handle edge cases for use_column

e1970d8

adamruzicka reviewed Jul 29, 2022

View reviewed changes

config/initializers/foreman_register.rb Outdated Show resolved Hide resolved

Refs #35274 - Extract common classes to methods

a57acf6

adamruzicka approved these changes Aug 1, 2022

View reviewed changes

theforeman-bot removed the Not yet reviewed label Aug 1, 2022

adamruzicka merged commit b1b46b6 into theforeman:develop Aug 3, 2022

ares added Demo worthy Headline feature candidate for headline feature labels Aug 3, 2022

ShimShtein added a commit to ShimShtein/foreman that referenced this pull request Aug 9, 2022

Refs #35274 - Revert "Make columns on host index page selectable (the…

2f754f0

…foreman#9319)" This reverts commit b1b46b6.

upadhyeammit pushed a commit that referenced this pull request Aug 10, 2022

Refs #35274 - Revert "Make columns on host index page selectable (#9319…

387608c

…)" This reverts commit b1b46b6.

ofedoren mentioned this pull request Aug 11, 2022

Fixes #35361 - Make columns on host index page selectable #9351

Merged

ekohl mentioned this pull request Jul 7, 2023

Fixes #36575 - Avoid translating labels during startup #9771

Draft

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fixes #35274 - Make columns on host index page selectable #9319

Fixes #35274 - Make columns on host index page selectable #9319

ofedoren commented Jul 25, 2022 •

edited

Loading

theforeman-bot commented Jul 25, 2022

ofedoren commented Jul 25, 2022

ares left a comment

ares Jul 25, 2022

ofedoren Jul 27, 2022

adamruzicka left a comment

ofedoren commented Jul 27, 2022

ares commented Jul 27, 2022

ofedoren commented Jul 27, 2022 •

edited

Loading

ares commented Jul 28, 2022

ofedoren commented Jul 28, 2022

adamruzicka left a comment

adamruzicka left a comment

adamruzicka commented Aug 3, 2022

adamruzicka commented Aug 3, 2022

ares commented Aug 3, 2022

Fixes #35274 - Make columns on host index page selectable #9319

Fixes #35274 - Make columns on host index page selectable #9319

Conversation

ofedoren commented Jul 25, 2022 • edited Loading

theforeman-bot commented Jul 25, 2022

ofedoren commented Jul 25, 2022

ares left a comment

Choose a reason for hiding this comment

ares Jul 25, 2022

Choose a reason for hiding this comment

ofedoren Jul 27, 2022

Choose a reason for hiding this comment

adamruzicka left a comment

Choose a reason for hiding this comment

ofedoren commented Jul 27, 2022

ares commented Jul 27, 2022

ofedoren commented Jul 27, 2022 • edited Loading

ares commented Jul 28, 2022

ofedoren commented Jul 28, 2022

adamruzicka left a comment

Choose a reason for hiding this comment

adamruzicka left a comment

Choose a reason for hiding this comment

adamruzicka commented Aug 3, 2022

adamruzicka commented Aug 3, 2022

ares commented Aug 3, 2022

ofedoren commented Jul 25, 2022 •

edited

Loading

ofedoren commented Jul 27, 2022 •

edited

Loading