Merge pull request #7521 from opf/fix/30677-replace-user-autocompleter-in-admin-group-area

[30677] Replace legacy user autocompleter to be able to assign group members again

[ci skip]

app/controllers/groups_controller.rb

@@ -31,7 +31,7 @@ class GroupsController < ApplicationController
   layout 'admin'
 
   before_action :require_admin
-  before_action :find_group, only: [:destroy, :autocomplete_for_user,
+  before_action :find_group, only: [:destroy,
                                     :show, :create_memberships, :destroy_membership,
                                     :edit_membership]
 
@@ -69,6 +69,8 @@ class GroupsController < ApplicationController
   # GET /groups/1/edit
   def edit
     @group = Group.includes(:members, :users).find(params[:id])
+
+    set_filters_for_user_autocompleter
   end
 
   # POST /groups
@@ -134,11 +136,6 @@ class GroupsController < ApplicationController
     redirect_to controller: '/groups', action: 'edit', id: @group, tab: 'users'
   end
 
-  def autocomplete_for_user
-    @users = User.active.not_in_group(@group).like(params[:q]).limit(100)
-    render layout: false
-  end
-
   def create_memberships
     membership_params = permitted_params.group_membership
     membership_id = membership_params[:membership_id]
@@ -178,6 +175,12 @@ class GroupsController < ApplicationController
     @group = Group.find(params[:id])
   end
 
+  def set_filters_for_user_autocompleter
+    @autocompleter_filters = []
+    @autocompleter_filters.push({ selector: 'status', operator: '=', values: ['active'] })
+    @autocompleter_filters.push({ selector: 'group', operator: '!', values: [@group.id] })
+  end
+
   def default_breadcrumb
     if action_name == 'index'
       t('label_group_plural')

app/models/queries/filters/shared/group_filter.rb

@@ -53,13 +53,29 @@ module Queries::Filters::Shared::GroupFilter
       I18n.t('query_fields.member_of_group')
     end
 
-    def joins
-      :groups
+    def where
+      case operator
+      when '='
+        "users.id IN (#{group_subselect})"
+      when '!'
+        "users.id NOT IN (#{group_subselect})"
+      when '*'
+        "users.id IN (#{any_group_subselect})"
+      when '!*'
+        "users.id NOT IN (#{any_group_subselect})"
+      end
     end
 
-    def where
-      operator_strategy.sql_for_field(values, 'groups_users', 'id')
+    private
+
+    def group_subselect
+      User.in_group(values).select(:id).to_sql
     end
+
+    def any_group_subselect
+      User.within_group([]).select(:id).to_sql
+    end
+
   end
 
   module ClassMethods

app/models/user.rb

@@ -140,13 +140,25 @@ class User < Principal
   before_destroy :reassign_associated
 
   scope :in_group, ->(group) {
-    group_id = group.is_a?(Group) ? group.id : group.to_i
-    where(["#{User.table_name}.id IN (SELECT gu.user_id FROM #{table_name_prefix}group_users#{table_name_suffix} gu WHERE gu.group_id = ?)", group_id])
+    within_group(group)
   }
   scope :not_in_group, ->(group) {
-    group_id = group.is_a?(Group) ? group.id : group.to_i
-    where(["#{User.table_name}.id NOT IN (SELECT gu.user_id FROM #{table_name_prefix}group_users#{table_name_suffix} gu WHERE gu.group_id = ?)", group_id])
+    within_group(group, false)
   }
+  scope :within_group, ->(group, positive = true) {
+    group_id = group.is_a?(Group) ? [group.id] : Array(group).map(&:to_i)
+
+    sql_condition = group_id.any? ? 'WHERE gu.group_id IN (?)' : ''
+    sql_not = positive ? '' : 'NOT'
+
+    sql_query = ["#{User.table_name}.id #{sql_not} IN (SELECT gu.user_id FROM #{table_name_prefix}group_users#{table_name_suffix} gu #{sql_condition})"]
+    if group_id.any?
+      sql_query.push group_id
+    end
+
+    where(sql_query)
+  }
+
   scope :admin, -> { where(admin: true) }
 
   scope :newest, -> { not_builtin.order(created_on: :desc) }

app/views/groups/_users.html.erb

@@ -28,8 +28,8 @@ See docs/COPYRIGHT.rdoc for more details.
 ++#%>
 
 <%= activate_angular_js do %>
-<div class="grid-block">
-  <div class="grid-content">
+  <div class="grid-block -visible-overflow">
+    <div class="grid-content medium-6">
       <% if @group.users.any? %>
         <div class="generic-table--container">
           <div id="group_users_table" class="generic-table--results-container">
@@ -40,39 +40,30 @@ See docs/COPYRIGHT.rdoc for more details.
         <%= no_results_box %>
       <% end %>
     </div>
-  <div class="grid-content">
+    <div class="grid-content medium-6 -visible-overflow">
       <% users = User
         .not_builtin
         .active
         .not_in_group(@group)
-               .limit(100) %>
+        .limit(1) %>
       <% if users.any? %>
-      <%= styled_form_tag(members_of_group_path(@group), method: :post) do |f| %>
-        <remote-field-updater url="<%= url_for(controller: '/groups', action: 'autocomplete_for_user', id: @group)%>">
+        <%= styled_form_tag(members_of_group_path(@group), method: :post) do %>
           <fieldset class="form--fieldset">
             <legend class="form--fieldset-legend"><%=l(:label_user_new)%></legend>
             <div class="form--field -vertical">
-                <%= styled_label_tag "user_search", l(:label_user_search) %>
-                <div class="form--field-container">
-                  <%= styled_text_field_tag 'user_search',
-                                            nil,
-                                            class: 'remote-field--input',
-                                            data: { :'remote-field-key' =>'q' } %>
+              <%= hidden_field_tag :user_ids, nil  %>
+              <user-autocompleter data-update-input="user_ids"
+                                  data-additional-filter="<%= @autocompleter_filters&.to_json %>"
+                                  class="new-group-members--autocomplete">
+              </user-autocompleter>
             </div>
-              </div>
-              <div class="form--field -vertical">
-                <div id="users" class="remote-field--target form--field-container -vertical">
-                  <%= principals_check_box_tags 'user_ids[]', users %>
-                </div>
-              </div>
-          </fieldset>
-        </remote-field-updater>
             <div>
               <%= styled_button_tag l(:button_add),
                                     class: '-highlight -with-icon icon-checkmark' %>
             </div>
+          </fieldset>
         <% end %>
       <% end %>
     </div>
-</div>
+  </div>
 <% end %>

config/routes.rb

@@ -357,7 +357,6 @@ OpenProject::Application.routes.draw do
 
     resources :groups do
       member do
-        get :autocomplete_for_user
         # this should be put into it's own resource
         match '/members' => 'groups#add_users', via: :post, as: 'members_of'
         match '/members/:user_id' => 'groups#remove_user', via: :delete, as: 'member_of'

frontend/src/app/modules/common/autocomplete/user-autocompleter.component.ts

@@ -30,7 +30,7 @@ import {Component, ElementRef, EventEmitter, Input, OnInit, Output, ViewChild} f
 import {DynamicBootstrapper} from "core-app/globals/dynamic-bootstrapper";
 import {HalResourceService} from "core-app/modules/hal/services/hal-resource.service";
 import {PathHelperService} from "core-app/modules/common/path-helper/path-helper.service";
-import {ApiV3FilterBuilder} from "core-components/api/api-v3/api-v3-filter-builder";
+import {ApiV3FilterBuilder, FilterOperator} from "core-components/api/api-v3/api-v3-filter-builder";
 import {NgSelectComponent} from "@ng-select/ng-select/dist";
 import {I18nService} from "core-app/modules/common/i18n/i18n.service";
 
@@ -70,6 +70,7 @@ export class UserAutocompleterComponent implements OnInit {
   private updateInputField:HTMLInputElement|undefined;
 
   public options:any[];
+  public filters:ApiV3FilterBuilder = new ApiV3FilterBuilder();
 
   constructor(protected elementRef:ElementRef,
               protected halResourceService:HalResourceService,
@@ -80,6 +81,7 @@ export class UserAutocompleterComponent implements OnInit {
   ngOnInit() {
     const input = this.elementRef.nativeElement.dataset['updateInput'];
     const allowEmpty = this.elementRef.nativeElement.dataset['allowEmpty'];
+
     if (input) {
       this.updateInputField = document.getElementsByName(input)[0] as HTMLInputElement|undefined;
       this.setInitialSelection();
@@ -89,13 +91,21 @@ export class UserAutocompleterComponent implements OnInit {
       this.allowEmpty = true;
     }
 
-    this.setAvailableUsers(this.url, '');
+
+    let filterInput  = this.elementRef.nativeElement.dataset['additionalFilter'];
+    if (filterInput) {
+      JSON.parse(filterInput).forEach((filter:{selector:string; operator:FilterOperator, values:string[]}) => {
+        this.filters.add(filter['selector'], filter['operator'], filter['values']);
+      })
+    }
+
+    this.setAvailableUsers(this.url, this.filters);
   }
 
   public onModelChange(user:any) {
     if (user) {
       this.onChange.emit(user);
-      this.setAvailableUsers(this.url, '');
+      this.setAvailableUsers(this.url, this.filters);
 
       if (this.clearAfterSelection) {
         this.ngSelectComponent.clearItem(user);
@@ -108,24 +118,26 @@ export class UserAutocompleterComponent implements OnInit {
   }
 
   public onSearch($event:any) {
-    let urlQuery:any;
+    let newFilters = this.filters;
     if($event) {
-      let filters = new ApiV3FilterBuilder();
-      filters.add('name', '~', [$event]);
-      urlQuery = { filters: filters.toJson() };
+      newFilters.add('name', '~', [$event]);
     }
 
-    this.setAvailableUsers(this.url, urlQuery);
+    this.setAvailableUsers(this.url, newFilters);
   }
 
   public onFocus(){
     // For dynamic changes of the available users
     // we have to reload them on focus (e.g. watchers)
-    this.setAvailableUsers(this.url, '');
+    this.setAvailableUsers(this.url, this.filters);
   }
 
-  private setAvailableUsers(url:string, filters:any) {
-    this.halResourceService.get(url, filters)
+  private setAvailableUsers(url:string, filters:ApiV3FilterBuilder) {
+    let params = {
+      filters: filters.toJson()
+    };
+
+    this.halResourceService.get(url, params)
       .subscribe(res => {
         this.options = res.elements.map((el:any) => {
           return {name: el.name, id: el.id, href: el.href, avatar: el.avatar};

spec/features/groups/membership_spec.rb

@@ -110,7 +110,7 @@ feature 'group memberships through project members page', type: :feature do
       allow(User).to receive(:current).and_return admin
     end
 
-    scenario 'adding members to that group adds them to the project too' do
+    scenario 'adding members to that group adds them to the project too', js: true do
       members_page.visit!
 
       expect(members_page).not_to have_user('Alice Wonderland') # Alice not in the project yet

spec/models/queries/members/filters/group_filter_spec.rb

@@ -55,10 +55,8 @@ describe Queries::Members::Filters::GroupFilter, type: :model do
     end
   end
 
-  it_behaves_like 'list_optional query filter' do
-    let(:attribute) { :id }
+  it_behaves_like 'list_optional group query filter' do
     let(:model) { Member.joins(:principal).merge(User.joins(:groups)) }
     let(:valid_values) { [group1.id.to_s] }
-    let(:expected_table_name) { 'groups_users' }
   end
 end

spec/models/queries/users/filters/group_filter_spec.rb

@@ -55,11 +55,8 @@ describe Queries::Users::Filters::GroupFilter, type: :model do
     end
   end
 
-  it_behaves_like 'list_optional query filter' do
-    let(:attribute) { :id }
+  it_behaves_like 'list_optional group query filter' do
     let(:model) { User }
-    let(:joins) { :groups }
     let(:valid_values) { [group1.id.to_s] }
-    let(:expected_table_name) { 'groups_users' }
   end
 end

spec/models/queries/users/user_query_spec.rb

@@ -115,8 +115,7 @@ describe Queries::Users::UserQuery, type: :model do
       it 'is the same as handwriting the query' do
         expected = base_scope
                      .merge(User
-                          .joins(:groups)
-                          .where("groups_users.id IN ('#{group_1.id}')"))
+                              .where(["users.id IN (#{User.in_group([group_1.id.to_s]).select(:id).to_sql})"]))
 
         expect(instance.results.to_sql).to eql expected.to_sql
       end

spec/routing/groups_spec.rb

@@ -68,12 +68,6 @@ describe 'groups routes', type: :routing do
                                                         id: '4')
   }
 
-  it {
-    is_expected.to route(:get, '/admin/groups/4/autocomplete_for_user').to(controller: 'groups',
-                                                                           action: 'autocomplete_for_user',
-                                                                           id: '4')
-  }
-
   it {
     is_expected.to route(:post, '/admin/groups/4/members').to(controller: 'groups',
                                                               action: 'add_users',

spec/support/pages/groups.rb

@@ -64,6 +64,7 @@ module Pages
   end
 
   class Group < Pages::Page
+    include ::Components::NgSelectAutocompleteHelpers
     attr_reader :id
 
     def initialize(id)
@@ -108,7 +109,10 @@ module Pages
 
     def add_user!(user_name)
       open_users_tab!
-      check user_name
+
+      container = page.find('.new-group-members--autocomplete')
+      select_autocomplete container,
+                          query: user_name
       click_on 'Add'
     end
 

spec/support/queries/filters/shared_filter_examples.rb

@@ -220,6 +220,73 @@ shared_examples_for 'list_optional query filter' do
     end
   end
 end
+shared_examples_for 'list_optional group query filter' do
+  include_context 'filter tests'
+  describe '#scope' do
+    let(:values) { valid_values }
+
+    context 'for "="' do
+      let(:operator) { '=' }
+
+      it 'is the same as handwriting the query' do
+        expected = model.where(["users.id IN (#{User.in_group(values).select(:id).to_sql})"])
+        expect(instance.scope.to_sql).to eql expected.to_sql
+      end
+    end
+
+    context 'for "!"' do
+      let(:operator) { '!' }
+
+      it 'is the same as handwriting the query' do
+        expected = model.where(["users.id NOT IN (#{User.in_group(values).select(:id).to_sql})"])
+        expect(instance.scope.to_sql).to eql expected.to_sql
+      end
+    end
+
+    context 'for "*"' do
+      let(:operator) { '*' }
+
+      it 'is the same as handwriting the query' do
+        expected = model.where(["users.id IN (#{User.within_group([]).select(:id).to_sql})"])
+        expect(instance.scope.to_sql).to eql expected.to_sql
+      end
+    end
+
+    context 'for "!*"' do
+      let(:operator) { '!*' }
+
+      it 'is the same as handwriting the query' do
+        expected = model.where(["users.id NOT IN (#{User.within_group([]).select(:id).to_sql})"])
+        expect(instance.scope.to_sql).to eql expected.to_sql
+      end
+    end
+  end
+
+  describe '#valid?' do
+    let(:operator) { '=' }
+    let(:values) { valid_values }
+
+    it 'is valid' do
+      expect(instance).to be_valid
+    end
+
+    context 'for an invalid operator' do
+      let(:operator) { '~' }
+
+      it 'is invalid' do
+        expect(instance).to be_invalid
+      end
+    end
+
+    context 'for an invalid value' do
+      let(:values) { ['inexistent'] }
+
+      it 'is invalid' do
+        expect(instance).to be_invalid
+      end
+    end
+  end
+end
 
 shared_examples_for 'list_all query filter' do
   include_context 'filter tests'

spec_legacy/functional/groups_controller_spec.rb

@@ -110,13 +110,4 @@ describe GroupsController, type: :controller do
       delete :destroy_membership, params: { id: 10, membership_id: 6 }
     end
   end
-
-  it 'should autocomplete for user' do
-    get :autocomplete_for_user, params: { id: 10, q: 'mis' }
-    assert_response :success
-    users = assigns(:users)
-    refute_nil users
-    assert users.any?
-    assert !users.include?(Group.find(10).users.first)
-  end
 end