Feature #37674
closed
Upgrade Admin/Users list to use the query system
Added by Jens Krämer over 3 years ago. Updated over 3 years ago.
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
Related issues
Updated by Go MAEDA over 3 years ago
- File screenshot-20220915.png screenshot-20220915.png added
Screenshot of this feature:
Updated by Go MAEDA over 3 years 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
Updated by Jens Krämer over 3 years ago
- File 0001-introduces-a-UserQuery-model-for-admin-users.patch 0001-introduces-a-UserQuery-model-for-admin-users.patch added
Thank you for looking into this. Attached is the first patch with your solution built in.
Updated by Go MAEDA over 3 years 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?
Updated by Jens Krämer over 3 years 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.
Updated by Marius BĂLTEANU over 3 years ago
I agree that each user attribute should have its specific filter.
Updated by Jens Krämer over 3 years ago
- File 0003-confirm-user-update-deletion.patch 0003-confirm-user-update-deletion.patch added
- File 0002-user-bulk-destroy.patch 0002-user-bulk-destroy.patch added
- File 0001-introduces-a-UserQuery-model-for-admin-users.patch 0001-introduces-a-UserQuery-model-for-admin-users.patch added
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
Updated by Go MAEDA over 3 years ago
- Target version set to 5.1.0
Setting the target version to 5.1.0.
Updated by Go MAEDA over 3 years 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.
Updated by Go MAEDA over 3 years ago
- Related to Feature #4295: Support more filters on the user list page added
Updated by Go MAEDA over 3 years ago
- Related to Feature #18193: Add ability to filter users list by Authentication mode added
Updated by Go MAEDA over 3 years ago
- Related to Patch #21055: Add Authentication mode filter in the Users admin page added
Updated by Go MAEDA over 3 years ago
- Tracker changed from Patch to Feature
- Resolution set to Fixed
Updated by Mischa The Evil over 3 years ago
- Related to Feature #4023: User Custom Fields in List view added
Updated by Mischa The Evil over 3 years ago
- Related to Feature #11501: More filters on page admin -> users added
Updated by Mischa The Evil over 3 years ago
- Related to Feature #31043: Search/filter users in backend added
Updated by Jens Krämer over 3 years ago
- File 0001-adds-tests-for-UserQuery-model-37674.patch 0001-adds-tests-for-UserQuery-model-37674.patch added
- Status changed from Closed to Reopened
I just noticed that I forgot to add the tests for the new UserQuery model.
Updated by Go MAEDA over 3 years 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.
Updated by Marius BĂLTEANU over 3 years 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?
Updated by Jens Krämer over 3 years ago
- File 0001-adds-missing-join-when-ordering-by-authsource-37674.patch 0001-adds-missing-join-when-ordering-by-authsource-37674.patch added
My bad, I didn't override joins_for_order_statement to account for that. Attached patch fixes this.
Updated by Go MAEDA over 3 years ago
- Status changed from Reopened to Closed
Jens Krämer wrote:
My bad, I didn't override
joins_for_order_statementto account for that. Attached patch fixes this.
Committed the fix. Thank you for quickly writing the fix.
Updated by Go MAEDA over 3 years 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
Updated by Marius BĂLTEANU over 3 years 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.
Updated by Marius BĂLTEANU over 3 years 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.
Updated by Marius BĂLTEANU over 3 years 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.
Updated by Go MAEDA about 3 years ago
- Related to Defect #38182: Exporting users query does not use the query name as file name added
Updated by Go MAEDA almost 3 years ago
- Has duplicate Feature #5017: Custom columns on Admin/Users page added
Updated by Holger Just over 2 years ago
- Related to Patch #39181: /users backwards API compatibility added
Updated by Marius BĂLTEANU about 2 years ago
- Has duplicate Feature #36659: Add "auth_source_id" to GET request for Endpoint /users.:format added
Updated by Marius BĂLTEANU about 2 years ago
- Has duplicate Feature #13529: to allow query with parameters for Users of REST API added