Patch #39181
closed
/users backwards API compatibility
0%
Description
#37674 changed the /users endpoint behavior by dropping support for the previously supported status and name URL parameters. This might break API clients that relied on these parameters. Further the functionality of the 'Name' filter, which allowed to do a combined search across users' name, email and login was lost for both API and Browser clients.
The attached patch that's been extracted from Planio introduces a new 'Name, email or login' query filter and maps the legacy 'name' URL parameter to this new filter. Also, a 'status' URL parameter will be mapped to a query on 'status_id' if present.
Files
Related issues
Updated by Holger Just about 1 year ago
- Related to Feature #37674: Upgrade Admin/Users list to use the query system added
Updated by Mischa The Evil about 1 year ago
- Description updated (diff)
Jens Krämer wrote:
#37674 changed the
/usersendpoint behavior by dropping support for the previously supportedstatusandnameURL parameters. This might break API clients that relied on these parameters. Further the functionality of the 'Name' filter, which allowed to do a combined search across users' name, email and login was lost for both API and Browser clients.
Tentatively confirmed (by code review only).
Besides the previously supported status and name URL parameters, Redmine also supported a group_id URL parameter (per #7893), which allowed clients to "get only users who are members of the given group1".
AFAICT this functionality got lost with the implementation of #37674 too.
Extending the attached patch like this (untested code):
diff --git a/app/controllers/users_controller.rb b/app/controllers/users_controller.rb
index 77ec1a601..e1d3a7521 100644
--- a/app/controllers/users_controller.rb
+++ b/app/controllers/users_controller.rb
@@ -45,6 +45,20 @@ class UsersController < ApplicationController
use_session = !request.format.csv?
retrieve_query(UserQuery, use_session)
+ # API backwards compatibility: handle legacy status, name and
+ # group_id filter parameters
+ unless request.format.html?
+ if status_id = params[:status].presence
+ @query.add_filter 'status', '=', [status_id]
+ end
+ if name = params[:name].presence
+ @query.add_filter 'name', '~', [name]
+ end
+ if group_id = params[:group_id].presence
+ @query.add_filter 'is_member_of_group', '=', [group_id]
+ end
+ end
+
if @query.valid?
scope = @query.results_scope
would probably suffice to fix this additional API compatibility regression.
It should additionally map the 'group_id' URL parameter to a query on 'is_member_of_group' if present.
I think that it would also be good to have some test coverage for these API features in case this patch (with or without my extension) is accepted.
1 from Rest_Users#GET
Updated by Jens Krämer about 1 year ago
- File 0001-API-compatibility-to-legacy-status-and-name-query-pa.patch 0001-API-compatibility-to-legacy-status-and-name-query-pa.patch added
thanks for looking into this, and yes, adding group_id as well seems right to me. I attached a revised patch including this as well as an integration test covering this.
Updated by Go MAEDA about 1 year ago
- Status changed from New to Closed
- Assignee set to Go MAEDA
Committed the fix as a part of #37674. Thank you.
Updated by Mischa The Evil about 1 year ago
- Status changed from Closed to Reopened
Updated by Go MAEDA about 1 year ago
Mischa The Evil wrote in #note-6:
Go MAEDA: for sake of correctness, can you also adapt the comment to actually reflect the current situation like something I proposed in #note-3?
Thank you for reviewing the commits. Am I correct that you are saying that the following change should also be committed?
diff --git a/app/controllers/users_controller.rb b/app/controllers/users_controller.rb
index f64c469da..cd430dc32 100644
--- a/app/controllers/users_controller.rb
+++ b/app/controllers/users_controller.rb
@@ -45,7 +45,8 @@ class UsersController < ApplicationController
use_session = !request.format.csv?
retrieve_query(UserQuery, use_session)
- # API backwards compatibility: handle legacy status and name filter parameters
+ # API backwards compatibility: handle legacy status and name and
+ # group_id filter parameters
unless request.format.html?
if status_id = params[:status].presence
@query.add_filter 'status', '=', [status_id]
Updated by Mischa The Evil about 1 year ago
Go MAEDA wrote in #note-7:
Mischa The Evil wrote in #note-6:
Go MAEDA: for sake of correctness, can you also adapt the comment to actually reflect the current situation like something I proposed in #note-3?
Thank you for reviewing the commits. Am I correct that you are saying that the following change should also be committed?
[...]
Yes, although I prefer the punctuation I proposed in #note-3. Nevertheless, something more general like the following might be fine (or even better) too:
diff --git a/app/controllers/users_controller.rb b/app/controllers/users_controller.rb
index f64c469da..cd430dc32 100644
--- a/app/controllers/users_controller.rb
+++ b/app/controllers/users_controller.rb
@@ -45,7 +45,8 @@ class UsersController < ApplicationController
use_session = !request.format.csv?
retrieve_query(UserQuery, use_session)
- # API backwards compatibility: handle legacy status and name filter parameters
+ # API backwards compatibility: handle legacy filter parameters
unless request.format.html?
if status_id = params[:status].presence
@query.add_filter 'status', '=', [status_id]
Off-topic: it seems Rouge's diff lexer is affected by a bug that handles code comments wrongly...
Updated by Mischa The Evil about 1 year ago
Thanks. Also thanks to Jens and the Plan crew for sharing this work.
Updated by Go MAEDA 11 months ago
- Related to Defect #38165: Users API ignores name and group_id parameter added