From b4b0f5b23dc10361ed6b3389c836ef9c6dc105cc Mon Sep 17 00:00:00 2001 From: Chris Lowis Date: Tue, 3 Oct 2023 15:51:41 +0100 Subject: [PATCH] Prevent duplicate rows in /users when filters applied Trello: https://trello.com/c/i27MQaus This fixes an issue with duplication of a user in the table on /users (and the corresponding CSV export) when multiple permissions for the same application were selected in the filter. For example if a user had the "GDS editor" and "signon" permission for the "Transition" app and we selected both of those permissions in the filter drop down, this user would appear twice in table. The solution is to call `distinct` on the `User#with_permission` scope, which is in turn called by `UsersFilter#users`. I've decided to add a test case in both places to avoid a regression in `UsersFilter` in case it changes in the future. Note we have to add the `unscoped` method to `SupportedPermission` in the `User#with_permission` scope since `SupportedPermission` has an `order(:name)` `default_scope` and calling `distinct` on an order query is not supported by MySQL: Mysql2::Error: Expression #1 of ORDER BY clause is not in SELECT list, references column 'signon_test.supported_permissions.name' which is not in SELECT list; this is incompatible with DISTINCT --- app/models/user.rb | 2 +- test/models/user_test.rb | 10 ++++++++++ test/models/users_filter_test.rb | 11 +++++++++++ 3 files changed, 22 insertions(+), 1 deletion(-) diff --git a/app/models/user.rb b/app/models/user.rb index 18d4a3971..b94965632 100644 --- a/app/models/user.rb +++ b/app/models/user.rb @@ -88,7 +88,7 @@ class User < ApplicationRecord scope :not_setup_2sv, -> { not_exempt_from_2sv.does_not_have_2sv } scope :with_role, ->(role) { where(role:) } - scope :with_permission, ->(permission) { joins(:supported_permissions).merge(SupportedPermission.where(id: permission)) } + scope :with_permission, ->(permission) { joins(:supported_permissions).merge(SupportedPermission.unscoped.where(id: permission)).distinct } scope :with_organisation, ->(organisation) { where(organisation:) } scope :with_partially_matching_name, ->(name) { where(arel_table[:name].matches("%#{name}%")) } scope :with_partially_matching_email, ->(email) { where(arel_table[:email].matches("%#{email}%")) } diff --git a/test/models/user_test.rb b/test/models/user_test.rb index 9af06194b..2a5dfd6b7 100644 --- a/test/models/user_test.rb +++ b/test/models/user_test.rb @@ -1056,6 +1056,16 @@ def setup specified_permissions = [app1.signin_permission, app2.signin_permission] assert_equal [user1, user2], User.with_permission(specified_permissions.map(&:to_param)) end + + should "only return a user once even when they have two permissions for the same app" do + app = create(:application) + + permission = create(:supported_permission, application: app) + create(:user, supported_permissions: [app.signin_permission, permission]) + + specified_permissions = [app.signin_permission, permission] + assert_equal 1, User.with_permission(specified_permissions.map(&:to_param)).count + end end context ".with_organisation" do diff --git a/test/models/users_filter_test.rb b/test/models/users_filter_test.rb index 3db9b54ef..6d1b4768f 100644 --- a/test/models/users_filter_test.rb +++ b/test/models/users_filter_test.rb @@ -211,6 +211,17 @@ class UsersFilterTest < ActiveSupport::TestCase assert_equal %w[user1 user3], filter.users.map(&:name) end + + should "return a user once if they have multiple selected permissions for the same app" do + app = create(:application, name: "App 1") + permission = create(:supported_permission, application: app, name: "Permission 1") + + create(:user, name: "user", supported_permissions: [app.signin_permission, permission]) + + filter = UsersFilter.new(User.all, @current_user, permissions: [app.signin_permission, permission].map(&:to_param)) + + assert_equal %w[user], filter.users.map(&:name) + end end context "#permission_option_select_options" do