Fixes #22624 - User selectable columns model + api #5262

parthaa · 2018-02-20T04:46:14Z

Adding initial bindings to user selectable columns. This model + api is
going to be used in work related Katello plugin's subscription pages.
The main intent of this PR is to provide a basic model where a user can
store "desirable or interested columns for a resource"
When the user for example says "I want to only see name column in
subscriptions" , we need model that would store that information
(similar to widgets or preferences.)
This commit facilitates that.

theforeman-bot · 2018-02-20T04:46:22Z

Issues: #22624

stbenjam · 2018-02-20T14:13:39Z

app/controllers/api/v2/user_columns_controller.rb

+    class UserColumnsController < V2::BaseController
+      include Api::Version2
+
+      api :GET, "/user_columns/", N_("List of User columns for a user")


user, capitalization should be consistent

What about delete and update? Also I feel like GET /user_columns should return all the columns for each resource (would it ever make sense to get that info?) and GET /user_columns/:resource should return them for a resource

ekohl · 2018-02-20T14:15:53Z

Would it be possible to replace https://github.com/GregSutcliffe/foreman_column_view with this?

stbenjam · 2018-02-20T14:16:17Z

app/controllers/api/v2/user_columns_controller.rb

+      param :resource, String, :required => true
+      param :columns, Array, :desc => N_("List of user selected columns")
+      def create
+        User.current.user_columns.where(:resource => params[:resource]).destroy_all


CREATE "updates" via this destroy_all?

Good point will figure out a better way.

stbenjam · 2018-02-20T14:17:43Z

app/models/user_column.rb

+
+  # Returns user columns representation as the hash object used in memory
+  def to_hash
+    { :columns => columns, :resource => resource }


attr_exportable might be usable here instead, take a look at the exportable concern

tstrachota · 2018-02-22T09:12:55Z

@parthaa do we already have some PR or branch with a table that uses this new functionality? I'm curious about how the UI part looks.

waldenraines · 2018-02-22T15:06:54Z

@parthaa do we already have some PR or branch with a table that uses this new functionality? I'm curious about how the UI part looks.

@tstrachota we will be using this on the RH subscriptions page:

Due to time constraints we aren't planning on using this anywhere in foreman yet.

parthaa · 2018-03-02T01:21:17Z

Here are the Actions implemented so far

Create


$ curl -k -u admin:changeme -H "Content-Type: application/json" -X POST -d '{"resource":"organizations", "columns":["name"]}' "https://localhost/api/v2/user_columns"

{"resource":"organizations","columns":["name"],"created_at":"2018-03-02 01:14:14 UTC","updated_at":"2018-03-02 01:14:14 UTC","user":{"id":4,"login":"admin","description":null}}

$ curl -k -u admin:changeme -H "Content-Type: application/json" -X POST -d '{"resource":"subscriptions", "columns":["name","account"]}' "https://localhost/api/v2/user_columns"

{"resource":"subscriptions","columns":["name","account"],"created_at":"2018-03-02 01:14:43 UTC","updated_at":"2018-03-02 01:14:43 UTC","user":{"id":4,"login":"admin","description":null}}

Index

$ curl -k -u admin:changeme -H "Content-Type: application/json" -X GET "https://localhost/api/v2/user_columns"
{
  "total": 2,
  "subtotal": 2,
  "page": 1,
  "per_page": 20,
  "search": null,
  "sort": {
    "by": null,
    "order": null
  },
  "results": [{"resource":"subscriptions","columns":["name","account"],"created_at":"2018-03-02 01:14:43 UTC","updated_at":"2018-03-02 01:14:43 UTC","user":{"id":4,"login":"admin","description":null}},{"resource":"organizations","columns":["name"],"created_at":"2018-03-02 01:14:14 UTC","updated_at":"2018-03-02 01:14:14 UTC","user":{"id":4,"login":"admin","description":null}}]
}

Information about an individual resource

$ curl -k -u admin:changeme -H "Content-Type: application/json" -X GET -d '{"resource":"subscriptions"}'   "https://localhost/api/v2/user_columns/columns"

{"results":["name","account"]}

Reset user columns for a given resource

$ curl -k -u admin:changeme -H "Content-Type: application/json" -X DELETE -d '{"resource":"subscriptions"}' "https://localhost/api/v2/user_columns/reset"
[]

#verified by
$ curl -k -u admin:changeme -H "Content-Type: application/json" -X GET -d '{"resource":"subscriptions"}'   "https://localhost/api/v2/user_columns/columns"
{"results":[]}

$ curl -k -u admin:changeme -H "Content-Type: application/json" -X GET -d '{"resource":"organizations"}'   "https://localhost/api/v2/user_columns/columns"
{"results":["name"]}

Destroy all user columns for a user

$ curl -k -u admin:changeme -H "Content-Type: application/json" -X DELETE    "https://localhost/api/v2/user_columns/destroy_all"
[]

# Verified By

$ curl -k -u admin:changeme -H "Content-Type: application/json" -X GET "https://localhost/api/v2/user_columns"
{
  "total": 0,
  "subtotal": 0,
  "page": 1,
  "per_page": 20,
  "search": null,
  "sort": {
    "by": null,
    "order": null
  },
  "results": []
}

parthaa · 2018-03-02T01:45:13Z

Note: I have listed the intended workflow with the Katello subscriptions UI here
Katello/katello#7219 (comment)

iNecas · 2018-03-02T14:42:28Z

This approach looks good to me. I would like to see more +1 from on this one, to make sure this doesn't go against any other plans folks have, as it's introducing new API point, that might be complicated to update later. Any other thoughts, @theforeman/core ?

jlsherrill · 2018-03-02T15:21:57Z

This doesn't have any way to have default columns defined in any way, which means:

fetching the colums for a fresh user would return nothing
Setting invalid columns can't be validated

i think there needs to be a registry where the app or plugin can define what columns are available for a resource (and the default enablement for them).

parthaa · 2018-03-03T21:42:54Z

This doesn't have any way to have default columns defined in any way, which means:

fetching the colums for a fresh user would return nothing
Setting invalid columns can't be validated
i think there needs to be a registry where the app or plugin can define what columns are available for a resource (and the default enablement for them).

Created a registry and a plugin api to make this work. Added validation and updated controller tests for this to work.
Check out -> https://github.com/Katello/katello/pull/7219/files#diff-84f0b46367dabf5e0710a0fe3aca7627R233 for more info o this.

ShimShtein · 2018-03-04T19:09:07Z

Generally, the idea is very interesting. A couple of thoughts on the subject:

How well will this play with graphQL? I know there's an effort to add support to it. /cc @ik5
As far as I can see, this filter is not applied at the server level, meaning no optimization to queries. So I see it solves only eye clutter for the user, but not processing time.

parthaa · 2018-03-05T04:35:08Z

How well will this play with graphQL? I know there's an effort to add support to it. /cc @ik5
As far as I can see, this filter is not applied at the server level, meaning no optimization to queries. So I see it solves only eye clutter for the user, but not processing time.

Good points. However this is not meant to replace graphql in terms of optimization. I would think this will work well in conjunction with graphql whenever that aspect is implemented in a mainstream way. For example if we have information on what columns we want, the api javascript will tell server what it exactly wants using GraphQL. This is purely for the UI (and possibly hammer in the future.) It is merely intended to provide and store enough information for that.

timogoebel

@parthaa: Thanks, two small comments:
Can we please not add an another registry but use one of the existing registries?

Doesn't it make sense to store this in the browser's local storage? We already use that for the "expand the nav" hamburger. And other tools like AWS do it in a similar way.

ohadlevy

thanks @parthaa, some comments inline.

I do agree with @timogoebel we should consider local storage, however i guess that we still need the definition of the columns somewhere..?

ohadlevy · 2018-03-05T10:15:07Z

app/controllers/api/v2/user_columns_controller.rb

+      api :GET, "/user_columns/:resource", N_("List of user columns for a resource")
+      param :resource, String, :required => true
+      def columns
+        render :json => { root_node_name => UserColumn.formatted_columns(params[:resource])}


does it make more sense to reuse rabl?

ohadlevy · 2018-03-05T10:18:12Z

app/controllers/api/v2/user_columns_controller.rb

+
+      api :GET, "/user_columns/:resource", N_("List of user columns for a resource")
+      param :resource, String, :required => true
+      def columns


why deviate from REST? (index, create, update, show, destroy)?

ohadlevy · 2018-03-05T10:19:06Z

app/controllers/api/v2/user_columns_controller.rb

+      api :DELETE, "/user_columns", N_("Delete columns for a resource")
+      param :resource, String, :required => true, :desc => N_("name of the resource")
+      def reset
+        process_response @user_column.destroy_all


this will always remove all? just like destroy_all?

in general I would prefer to have one destroy method that get an array of ids, so it can delete one, many or all.

ohadlevy · 2018-03-05T10:19:34Z

app/controllers/api/v2/user_columns_controller.rb

+      private
+
+      def find_user_column
+        @user_column = User.current.user_columns.where(:resource => params[:resource])


does it make sense to move to a scope? I see we use it in multiple places.

ohadlevy · 2018-03-05T10:21:26Z

app/models/user_column.rb

+  def ensure_columns_valid
+    valid_columns = ::UserColumnRegistry::Manager.columns(resource) || []
+    valid_column_names = valid_columns.map(&:name)
+    invalid_columns = columns.select{ |col| !valid_column_names.include? col.to_s }


instead of looping over all columns, you can use something like

invalid = columns - valid_common_names

ohadlevy · 2018-03-05T10:22:32Z

app/models/user_column.rb

+    current_user_columns = nil
+
+    if User.current.present?
+      uc = User.current.user_columns.where(:resource => resource).first


nitpick: I find variable uc name confusing, how about simple columns?
consider using scope instead of where.

ohadlevy · 2018-03-05T10:23:26Z

app/models/user_column.rb

+
+    if User.current.present?
+      uc = User.current.user_columns.where(:resource => resource).first
+      current_user_columns = uc.columns if uc && uc.columns


can use uc.try(:columns)

ohadlevy · 2018-03-05T10:24:03Z

app/models/user_column.rb

+
+    registry_columns.map do |column|
+      formatted_column = column.to_hash
+      formatted_column[:enabled] = if current_user_columns.nil?


since columns is serialized, empty string will probably be better to use .blank? vs .nil?

ohadlevy · 2018-03-05T10:24:41Z

app/registries/foreman/access_permissions.rb

@@ -720,4 +720,8 @@
    map.permission :revoke_personal_access_tokens,
                   :"api/v2/personal_access_tokens" => [:destroy]
  end
+
+  permission_set.security_block :user_columns do |map|
+    map.permission :access_user_columns, {:"api/v2/user_columns" => [:index, :create, :columns, :reset, :destroy_all]}


does it make more sense to be api/v2/user/id/columns?

waldenraines · 2018-03-05T13:49:11Z

Doesn't it make sense to store this in the browser's local storage?

@timogoebel @ohadlevy persisting the column selections gives a better user experience because it would be rather confusing for a user to have to select their preferred columns again after initially setting them if they user another browser or otherwise clear the local storage. Also if we store this in the browser's local storage then the columns will be different in the CLI.

We also want to expand this in the future to support other table configuration features such as column ordering. Not persisting these items would be frustrating to users who want to customize their table view once and have it shown the same from thereon out.

waldenraines · 2018-03-05T13:49:20Z

/cc @Rohoover

app/controllers/api/v2/table_preferences_controller.rb

parthaa · 2018-03-19T13:26:50Z

@timogoebel @ares @ohadlevy had a chat with @tbrisker and decided to remove the access_table_preferences permission. At this point it seems unnecessary since only User.current is allowed to mess with this table for their own preferences (similar to dashboard). Hope that works for everyone. If permissions is indeed desired in the future we can always add them later.

timogoebel · 2018-03-19T13:27:47Z

Hope that works for everyone.

Works for me. :-)

timogoebel · 2018-03-19T13:44:56Z

app/models/user.rb

@@ -42,7 +42,7 @@ class User < ApplicationRecord
  has_many :widgets, :dependent => :destroy
  has_many :ssh_keys, :dependent => :destroy
  has_many :personal_access_tokens, :dependent => :destroy
-
+  has_many :table_preferences, :dependent => :destroy


How about adding inverse_of here?

tbrisker · 2018-03-19T17:57:09Z

@parthaa looks like test/unit/foreman/access_permissions_test.rb needs to be modified to skip checking for permissions on the new endpoints. The other tests seem to be just transient failures.

parthaa · 2018-03-20T05:14:52Z

@tbrisker updated.

tbrisker

Looking good, any concerns @ares or @ohadlevy ?

ares · 2018-03-20T11:01:30Z

not from my side, I'm fine with the last change

ohadlevy · 2018-03-22T12:51:03Z

app/controllers/api/v2/table_preferences_controller.rb

+        @table_preferences = @user.table_preferences
+      end
+
+      api :GET, "/users/:user_id/table_preferences/:name", N_("List of table preferences for a table")


I find the description confusing, how about "Table preferences details" ? (but not a list of table for table)

ohadlevy · 2018-03-22T12:53:38Z

app/models/table_preference.rb

@@ -0,0 +1,10 @@
+class TablePreference < ApplicationRecord


do we care about auditing or search?

I don't think so, this is just UI preferences, not anything that has impact on the system. It's similar to the dashboard widget settings which we don't audit or search.

Adding initial bindings to user selectable columns. This model + api is going to be used in work related Katello plugin's subscription pages. The main intesnt of this PR is to provide a basic model where a user can store "desirable or interested columns for a resource" When the user for example says "I want to only see name column in subscriptions" , we need model that would store that information (similar to widgets or preferences.) This commit facilitates that.

mmoll · 2018-03-22T19:37:12Z

@ohadlevy all good now? :)

ohadlevy · 2018-03-22T20:49:42Z

Thanks!

parthaa · 2018-05-21T19:04:18Z

Gist connected to Usage -> https://gist.github.com/parthaa/36ead0ff6962d6dac33412a1c38e94f7

theforeman-bot added Needs testing Not yet reviewed labels Feb 20, 2018

stbenjam reviewed Feb 20, 2018

View reviewed changes

parthaa force-pushed the user-columns branch 4 times, most recently from 0efb615 to 7ab182e Compare February 21, 2018 23:47

parthaa force-pushed the user-columns branch from 7ab182e to eec56d8 Compare March 2, 2018 01:20

parthaa mentioned this pull request Mar 2, 2018

Fixes #22748 - API for Selectable columns for Subs Katello/katello#7219

Closed

parthaa force-pushed the user-columns branch 2 times, most recently from 4fb9177 to bffd011 Compare March 3, 2018 21:40

timogoebel previously requested changes Mar 5, 2018

View reviewed changes

theforeman-bot added Waiting on contributor and removed Not yet reviewed labels Mar 5, 2018

ohadlevy requested changes Mar 5, 2018

View reviewed changes

parthaa force-pushed the user-columns branch 2 times, most recently from f9886d3 to 4b6450c Compare March 19, 2018 13:14

houndci-bot reviewed Mar 19, 2018

View reviewed changes

app/controllers/api/v2/table_preferences_controller.rb Show resolved Hide resolved

parthaa force-pushed the user-columns branch from 4b6450c to f3299cd Compare March 19, 2018 13:16

timogoebel reviewed Mar 19, 2018

View reviewed changes

parthaa force-pushed the user-columns branch from f3299cd to e5dfc2f Compare March 19, 2018 14:00

tbrisker added Waiting on contributor and removed Needs re-review Needs testing labels Mar 19, 2018

parthaa force-pushed the user-columns branch from e5dfc2f to d5ef62b Compare March 19, 2018 18:29

theforeman-bot added Needs re-review Needs testing and removed Waiting on contributor labels Mar 19, 2018

tbrisker approved these changes Mar 20, 2018

View reviewed changes

theforeman-bot removed the Needs re-review label Mar 20, 2018

ohadlevy reviewed Mar 22, 2018

View reviewed changes

parthaa force-pushed the user-columns branch from d5ef62b to 894369f Compare March 22, 2018 13:45

ohadlevy approved these changes Mar 22, 2018

View reviewed changes

ohadlevy merged commit e73b804 into theforeman:develop Mar 22, 2018

parthaa deleted the user-columns branch March 22, 2018 21:08

Fixes #22624 - User selectable columns model + api #5262

Fixes #22624 - User selectable columns model + api #5262

Conversation

parthaa commented Feb 20, 2018

theforeman-bot commented Feb 20, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ekohl commented Feb 20, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tstrachota commented Feb 22, 2018

waldenraines commented Feb 22, 2018 • edited

parthaa commented Mar 2, 2018

parthaa commented Mar 2, 2018

iNecas commented Mar 2, 2018

jlsherrill commented Mar 2, 2018

parthaa commented Mar 3, 2018

ShimShtein commented Mar 4, 2018

parthaa commented Mar 5, 2018

timogoebel left a comment

Choose a reason for hiding this comment

ohadlevy left a 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

Choose a reason for hiding this comment

waldenraines commented Mar 5, 2018

waldenraines commented Mar 5, 2018

parthaa commented Mar 19, 2018 • edited

timogoebel commented Mar 19, 2018

Choose a reason for hiding this comment

tbrisker commented Mar 19, 2018

parthaa commented Mar 20, 2018

tbrisker left a comment

Choose a reason for hiding this comment

ares commented Mar 20, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mmoll commented Mar 22, 2018

ohadlevy commented Mar 22, 2018

parthaa commented May 21, 2018

waldenraines commented Feb 22, 2018 •

edited

parthaa commented Mar 19, 2018 •

edited