Project

General

Profile

Actions

Feature #37674

closed

Upgrade Admin/Users list to use the query system

Added by Jens Krämer over 1 year ago. Updated over 1 year ago.

Status:
Closed
Priority:
Normal
Assignee:
Category:
Administration
Target version:
Start date:
Due date:
% Done:

0%

Estimated time:
Resolution:
Fixed

Description

This patch series that was extracted from Planio introduces a UserQuery
and upgrades the Admin/Users list with corresponding filter, sort and (CSV) export capabilities.
Also introduced are a context menu and an option to delete multiple users at once. The third patch just
adds flash messages to the (single) user deletion operation, for symmetry with the messages rendered by the bulk delete.

This relates to issues #4295, #18193, #21055 (probably there are more I didn't find).


Files

0002-user-bulk-destroy.patch (6.5 KB) 0002-user-bulk-destroy.patch Jens Krämer, 2022-09-14 04:37
0001-introduces-a-UserQuery-model-for-admin-users.patch (30.4 KB) 0001-introduces-a-UserQuery-model-for-admin-users.patch Jens Krämer, 2022-09-14 04:37
0003-confirm-user-update-deletion.patch (971 Bytes) 0003-confirm-user-update-deletion.patch Jens Krämer, 2022-09-14 04:37
0004-adds-column-and-filter-for-authentication-mode-to-ad.patch (3.2 KB) 0004-adds-column-and-filter-for-authentication-mode-to-ad.patch Jens Krämer, 2022-09-14 04:37
screenshot-20220915.png (238 KB) screenshot-20220915.png Go MAEDA, 2022-09-15 05:46
0001-introduces-a-UserQuery-model-for-admin-users.patch (30.6 KB) 0001-introduces-a-UserQuery-model-for-admin-users.patch patch with fix for SQLite Jens Krämer, 2022-09-15 07:39
0002-user-bulk-destroy.patch (6.5 KB) 0002-user-bulk-destroy.patch Jens Krämer, 2022-09-16 01:39
0003-confirm-user-update-deletion.patch (971 Bytes) 0003-confirm-user-update-deletion.patch Jens Krämer, 2022-09-16 01:39
0001-introduces-a-UserQuery-model-for-admin-users.patch (31.7 KB) 0001-introduces-a-UserQuery-model-for-admin-users.patch Jens Krämer, 2022-09-16 01:39
0001-adds-tests-for-UserQuery-model-37674.patch (5.86 KB) 0001-adds-tests-for-UserQuery-model-37674.patch Jens Krämer, 2022-09-21 06:06
0001-adds-missing-join-when-ordering-by-authsource-37674.patch (1.67 KB) 0001-adds-missing-join-when-ordering-by-authsource-37674.patch Jens Krämer, 2022-09-27 03:42

Related issues

Related to Redmine - Feature #4295: Support more filters on the user list pageClosed2009-11-26

Actions
Related to Redmine - Feature #18193: Add ability to filter users list by Authentication modeClosed

Actions
Related to Redmine - Patch #21055: Add Authentication mode filter in the Users admin pageClosed

Actions
Related to Redmine - Feature #4023: User Custom Fields in List viewClosed2009-10-13

Actions
Related to Redmine - Feature #11501: More filters on page admin -> usersClosed

Actions
Related to Redmine - Feature #31043: Search/filter users in backendClosed

Actions
Related to Redmine - Defect #38182: Exporting users query does not use the query name as file nameClosed

Actions
Related to Redmine - Patch #39181: /users backwards API compatibility ClosedGo MAEDA

Actions
Has duplicate Redmine - Feature #5017: Custom columns on Admin/Users pageClosed2010-03-09

Actions
Has duplicate Redmine - Feature #36659: Add "auth_source_id" to GET request for Endpoint /users.:formatClosedMarius BĂLTEANU

Actions
Has duplicate Redmine - Feature #13529: to allow query with parameters for Users of REST APIClosed

Actions
Actions #1

Updated by Go MAEDA over 1 year ago

Screenshot of this feature:

Actions #2

Updated by Go MAEDA over 1 year ago

Thank you for posting the nice improvement. However, the following error occurred with SQLite.

Error:
UsersControllerTest#test_index_with_name_filter:
ActiveRecord::StatementInvalid: SQLite3::SQLException: no such function: CONCAT
    app/controllers/users_controller.rb:54:in `index'
    lib/redmine/sudo_mode.rb:61:in `sudo_mode'
    test/functional/users_controller_test.rb:54:in `test_index_with_name_filter'

rails test test/functional/users_controller_test.rb:53

Could you update the patch to support SQLite? In my environment, I was able to fix the error with the following code.

--- app/models/user_query.rb.org    2022-09-15 13:34:22.000000000 +0900
+++ app/models/user_query.rb    2022-09-15 13:34:41.000000000 +0900
@@ -134,8 +134,13 @@

   def sql_for_name_field(field, operator, value)
     match = operator == '~'
-    name_sql = sql_contains("CONCAT(login, ' ', firstname, ' ', lastname)",
-                            value.first, match: match)
+    if Redmine::Database.sqlite?
+      name_sql = sql_contains("login || ' ' || firstname || ' ' || lastname",
+                              value.first, match: match)
+    else
+      name_sql = sql_contains("CONCAT(login, ' ', firstname, ' ', lastname)",
+                              value.first, match: match)
+    end

     emails = EmailAddress.table_name
     email_sql = <<-SQL
Actions #3

Updated by Jens Krämer over 1 year ago

Thank you for looking into this. Attached is the first patch with your solution built in.

Actions #4

Updated by Go MAEDA over 1 year ago

This patch uses the "name" filter to search three columns: first name, last name, and email. I think this follows the current search feature. However, I think it may be confusing for users to use the "name" filter to search for email addresses. Actually, neither I nor two of my colleagues could not find a way to search by email until I read the patch.

What about extracting the email address search from the "name" filter and adding a separate "email" filter?

Actions #5

Updated by Jens Krämer over 1 year ago

Good point. I indeed tried to stay close to the existing functionality with this combined name filter, but maybe that's not really necessary after all.

I also just noticed that support for the other filter operators like 'starts with' / 'ends with' is still missing. I'll come up with an updated patch soon.

Actions #6

Updated by Marius BĂLTEANU over 1 year ago

I agree that each user attribute should have its specific filter.

Actions #7

Updated by Jens Krämer over 1 year ago

Please find attached a new series of patches with the following changes:

  • reduced number of commits by merging the last commit (auth source filter and column) into the first.
  • separate filters for login, firstname, lastname, mail
  • fixed the mail filter to support all string operators
Actions #8

Updated by Go MAEDA over 1 year ago

  • Target version set to 5.1.0

Setting the target version to 5.1.0.

Actions #9

Updated by Go MAEDA over 1 year ago

  • Status changed from New to Closed
  • Assignee set to Go MAEDA

Committed patches with fixes of RuboCop warnings. Thank you for providing this improvement.

Actions #10

Updated by Go MAEDA over 1 year ago

  • Related to Feature #4295: Support more filters on the user list page added
Actions #11

Updated by Go MAEDA over 1 year ago

  • Related to Feature #18193: Add ability to filter users list by Authentication mode added
Actions #12

Updated by Go MAEDA over 1 year ago

  • Related to Patch #21055: Add Authentication mode filter in the Users admin page added
Actions #13

Updated by Go MAEDA over 1 year ago

  • Tracker changed from Patch to Feature
  • Resolution set to Fixed
Actions #14

Updated by Mischa The Evil over 1 year ago

  • Related to Feature #4023: User Custom Fields in List view added
Actions #15

Updated by Mischa The Evil over 1 year ago

  • Related to Feature #11501: More filters on page admin -> users added
Actions #16

Updated by Mischa The Evil over 1 year ago

Actions #17

Updated by Jens Krämer over 1 year ago

I just noticed that I forgot to add the tests for the new UserQuery model.

Actions #18

Updated by Go MAEDA over 1 year ago

  • Status changed from Reopened to Closed

Jens Krämer wrote:

I just noticed that I forgot to add the tests for the new UserQuery model.

Committed the code in r21828. Thank you.

Actions #19

Updated by Marius BĂLTEANU over 1 year ago

  • Status changed from Closed to Reopened

There is an SQL error when sorting the users list by column Authentication Mode. Te reproduce, go to users page, add the Authentication Mode to the users list and order by the same column:

PG::UndefinedTable: ERROR: missing FROM-clause entry for table "auth_sources" LINE 1: ...ers"."type" = $2 AND (users.status <> 0) ORDER BY auth_sourc... ^

I've tested only on postgresql, but from a quick check, it seems that the table auth_sources is not joined, but the table is used in the sort. Can you check, please?

Actions #20

Updated by Jens Krämer over 1 year ago

My bad, I didn't override joins_for_order_statement to account for that. Attached patch fixes this.

Actions #21

Updated by Go MAEDA over 1 year ago

  • Status changed from Reopened to Closed

Jens Krämer wrote:

My bad, I didn't override joins_for_order_statement to account for that. Attached patch fixes this.

Committed the fix. Thank you for quickly writing the fix.

Actions #22

Updated by Go MAEDA over 1 year ago

  • Status changed from Closed to Reopened

The test fails with PostgreSQL.

Failure:
UserQueryTest#test_auth_source_ordering [/var/lib/jenkins/workspace/trunk/DATABASE_ADAPTER/postgresql/RUBY_VER/ruby-3.1/test/unit/user_query_test.rb:182]:
--- expected
+++ actual
@@ -1 +1 @@
-#<User id: 1, login: "admin", hashed_password: [FILTERED], firstname: "Redmine", lastname: "Admin", admin: true, status: 1, last_login_on: "2006-07-19 20:57:52.000000000 +0000", language: "en", auth_source_id: 1, created_on: "2006-07-19 17:12:21.000000000 +0000", updated_on: "2006-07-19 20:57:52.000000000 +0000", type: "User", mail_notification: "all", salt: "82090c953c4a0000a7db253b0691a6b4", must_change_passwd: false, passwd_changed_on: nil, twofa_scheme: nil, twofa_totp_key: nil, twofa_totp_last_used_at: nil, twofa_required: false>
+#<User id: 2, login: "jsmith", hashed_password: [FILTERED], firstname: "John", lastname: "Smith", admin: false, status: 1, last_login_on: "2006-07-19 20:42:15.000000000 +0000", language: "en", auth_source_id: nil, created_on: "2006-07-19 17:32:09.000000000 +0000", updated_on: "2006-07-19 20:42:15.000000000 +0000", type: "User", mail_notification: "all", salt: "67eb4732624d5a7753dcea7ce0bb7d7d", must_change_passwd: false, passwd_changed_on: nil, twofa_scheme: nil, twofa_totp_key: nil, twofa_totp_last_used_at: nil, twofa_required: false>

rails test test/unit/user_query_test.rb:172
Actions #23

Updated by Marius BĂLTEANU over 1 year ago

Go MAEDA wrote:

The test fails with PostgreSQL.

[...]

I've fixed the test in r21860 by adding a new AuthSource and ordering by non null values. I think the problem is generated by how PostgreSQL handles the null values by default.

Actions #24

Updated by Marius BĂLTEANU over 1 year ago

I've fixed another issue in r21861 that appears when the column auth source name is added as column and at least one user has an auth source different than internal:

Error:
UsersControllerTest#test_index_with_auth_source_column:
ActionView::Template::Error: undefined method `visible?' for #<AuthSourceLdap id: 1, [...]>

Please let me know what do you think about the fix.

Actions #25

Updated by Jens Krämer over 1 year ago

That looks good to me. Thanks Marius!

Actions #26

Updated by Marius BĂLTEANU over 1 year ago

  • Status changed from Reopened to Closed

Jens Krämer wrote:

That looks good to me. Thanks Marius!

Thanks! All tests pass now, closing this.

Actions #27

Updated by Go MAEDA about 1 year ago

  • Related to Defect #38182: Exporting users query does not use the query name as file name added
Actions #28

Updated by Go MAEDA about 1 year ago

  • Has duplicate Feature #5017: Custom columns on Admin/Users page added
Actions #29

Updated by Holger Just 6 months ago

  • Related to Patch #39181: /users backwards API compatibility added
Actions #30

Updated by Marius BĂLTEANU 2 months ago

  • Has duplicate Feature #36659: Add "auth_source_id" to GET request for Endpoint /users.:format added
Actions #31

Updated by Marius BĂLTEANU 2 months ago

  • Has duplicate Feature #13529: to allow query with parameters for Users of REST API added
Actions

Also available in: Atom PDF