From ad9dc7e4173708ad5f515e22c9c6a96e49f3ad95 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Marius=20B=C4=82LTEANU?= Date: Fri, 9 Oct 2026 12:02:39 +0300 Subject: [PATCH] Paginate the watchers selection dialog and load it using a Stimulus controller (#35192). --- app/assets/stylesheets/application.css | 1 + app/controllers/watchers_controller.rb | 79 ++++++-- .../searchable_selection_controller.js | 180 ++++++++++++++++++ app/views/watchers/_new.html.erb | 48 +++-- .../watchers/_users_for_watcher.html.erb | 19 ++ .../watchers/autocomplete_for_user.html.erb | 2 +- config/locales/en.yml | 1 + config/locales/ja.yml | 1 + test/functional/watchers_controller_test.rb | 137 +++++++++++++ test/system/watchers_test.rb | 41 ++++ 10 files changed, 472 insertions(+), 37 deletions(-) create mode 100644 app/javascript/controllers/searchable_selection_controller.js create mode 100644 app/views/watchers/_users_for_watcher.html.erb diff --git a/app/assets/stylesheets/application.css b/app/assets/stylesheets/application.css index 302cdba14..34c7a19f9 100644 --- a/app/assets/stylesheets/application.css +++ b/app/assets/stylesheets/application.css @@ -1481,6 +1481,7 @@ select.bool_cf {inline-size: auto !important;} #users_for_watcher {block-size: 200px; overflow: auto;} #users_for_watcher label {display: block;} +#users_for_watcher .watcher-more-users.ajax-loading {background: url(/loading.gif) no-repeat left center; padding-inline-start: 20px;} input#principal_search, input#user_search {inline-size: 90%} .roles-selection label {display: inline-block; inline-size: 210px;} diff --git a/app/controllers/watchers_controller.rb b/app/controllers/watchers_controller.rb index 13ede5d75..42e2cbfc0 100644 --- a/app/controllers/watchers_controller.rb +++ b/app/controllers/watchers_controller.rb @@ -18,6 +18,8 @@ # Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301, USA. class WatchersController < ApplicationController + WATCHER_USERS_PER_PAGE = 100 + before_action :require_login, :find_watchables, :only => [:watch, :unwatch] def watch @@ -148,26 +150,71 @@ class WatchersController < ApplicationController end end + # Returns a paginated array of users that can be added as watchers to the object(s). def users_for_new_watcher - scope = nil - if params[:q].blank? - if @project.present? - scope = @project.principals.assignable_watchers - elsif @projects.present? && @projects.size > 1 - scope = Principal.joins(:members).where(:members => { :project_id => @projects }).assignable_watchers.distinct + limit = WATCHER_USERS_PER_PAGE + offset = params[:offset].to_i + offset = 0 if offset < 0 + q = params[:q].to_s.strip + + valid_users, next_offset, more = scan_watcher_candidates(watcher_candidate_scope(q), offset, limit) + + @more_users_for_watcher = more + @watcher_users_next_offset = next_offset + valid_users + end + + def watcher_candidate_scope(q) + scope = + if q.blank? + if @project.present? + @project.principals.assignable_watchers + elsif @projects.present? && @projects.size > 1 + Principal.joins(:members).where(:members => {:project_id => @projects}).assignable_watchers.distinct + else + Principal.none + end + else + Principal.assignable_watchers end - else - scope = Principal.assignable_watchers.limit(100) - end - users = scope.sorted.like(params[:q]).to_a - if @watchables && @watchables.size == 1 - watchable_object = @watchables.first - users -= watchable_object.visible_watcher_users + + if @watchables&.size == 1 + watcher_ids = @watchables.first.visible_watcher_users.map(&:id) + scope = scope.where.not(:id => watcher_ids) if watcher_ids.present? end - @watchables&.each do |watchable| - users.reject!{|user| !watchable.valid_watcher?(user)} + + scope.sorted.like(q) + end + + def scan_watcher_candidates(scope, offset, limit) + valid_users = [] + current_offset = offset + has_more = false + + while valid_users.size < limit + batch = scope.offset(current_offset).limit(limit + 1).to_a + has_more = batch.size > limit + batch = batch.first(limit) + break if batch.empty? + + batch.each_with_index do |user, idx| + if @watchables.blank? || @watchables.all? {|w| w.valid_watcher?(user)} + valid_users << user + if valid_users.size == limit + has_more = has_more || (idx + 1 < batch.size) + current_offset += (idx + 1) + break + end + end + end + + break if valid_users.size == limit + + current_offset += batch.size + break unless has_more end - users + + [valid_users, current_offset, has_more && (valid_users.size == limit)] end def users_for_mention diff --git a/app/javascript/controllers/searchable_selection_controller.js b/app/javascript/controllers/searchable_selection_controller.js new file mode 100644 index 000000000..64d660a64 --- /dev/null +++ b/app/javascript/controllers/searchable_selection_controller.js @@ -0,0 +1,180 @@ +/** + * Redmine - project management software + * Copyright (C) 2006- Jean-Philippe Lang + * This code is released under the GNU General Public License. + */ + +import { Controller } from "@hotwired/stimulus" +import { get } from "@rails/request.js" + +export default class extends Controller { + static targets = ["input", "list", "hiddenContainer"] + static values = { + url: String, + checkboxSelector: String, + checkboxName: String, + debounceDelay: { type: Number, default: 300 } + } + + connect() { + this.selectedValues = new Set() + this.isLoadingMore = false + this.debounceTimer = null + this.searchAbortController = null + + // Track any checkboxes that are checked initially or stored in hidden inputs + if (this.hasHiddenContainerTarget) { + this.hiddenContainerTarget.querySelectorAll('input[type="hidden"]').forEach(input => { + this.selectedValues.add(input.value) + }) + } + if (this.hasListTarget) { + this.listTarget.querySelectorAll(`${this.checkboxSelector}:checked`).forEach(cb => { + this.selectedValues.add(cb.value) + }) + } + } + + disconnect() { + clearTimeout(this.debounceTimer) + if (this.searchAbortController) { + this.searchAbortController.abort() + } + } + + get checkboxName() { + if (this.hasCheckboxNameValue && this.checkboxNameValue) { + return this.checkboxNameValue + } + const firstCheckbox = this.hasListTarget ? this.listTarget.querySelector('input[type="checkbox"]') : null + return firstCheckbox ? firstCheckbox.name : "" + } + + get checkboxSelector() { + return this.hasCheckboxSelectorValue && this.checkboxSelectorValue + ? this.checkboxSelectorValue + : (this.checkboxName ? `input[name="${this.checkboxName}"]` : 'input[type="checkbox"]') + } + + search() { + clearTimeout(this.debounceTimer) + this.debounceTimer = setTimeout(() => { + this.performSearch() + }, this.debounceDelayValue) + } + + keydown(event) { + if (event.key === "Enter") { + event.preventDefault() + clearTimeout(this.debounceTimer) + this.performSearch() + } + } + + performSearch() { + const q = this.hasInputTarget ? this.inputTarget.value.trim() : "" + this.fetchResults({ q }, { append: false }) + } + + async loadMore(event) { + event.preventDefault() + if (this.isLoadingMore) return + + const trigger = event.currentTarget.closest('[data-role="load-more"]') + const url = new URL(event.currentTarget.href, window.location.origin) + const offset = url.searchParams.get("offset") + const q = this.hasInputTarget ? this.inputTarget.value.trim() : "" + + await this.fetchResults({ q, offset }, { append: true, trigger }) + } + + toggle(event) { + const checkbox = event.target + if (!checkbox.matches(this.checkboxSelector)) return + + if (checkbox.checked) { + this.selectedValues.add(checkbox.value) + } else { + this.selectedValues.delete(checkbox.value) + } + this.syncHiddenInputs() + } + + async fetchResults(params, { append = false, trigger = null } = {}) { + if (append) { + if (this.isLoadingMore) return + this.isLoadingMore = true + } else { + if (this.searchAbortController) { + this.searchAbortController.abort() + } + this.searchAbortController = new AbortController() + } + + const targetElement = trigger || (this.hasInputTarget ? this.inputTarget : this.element) + targetElement.classList.add("ajax-loading") + + try { + const url = new URL(this.urlValue, window.location.origin) + Object.entries(params).forEach(([key, value]) => { + if (value !== undefined && value !== null && value !== "") { + url.searchParams.set(key, value) + } + }) + + const signal = append ? null : this.searchAbortController.signal + const response = await get(url.toString(), { responseKind: "html", signal }) + if (!response.ok) return + + const html = await response.html + + if (append && trigger) { + trigger.insertAdjacentHTML("beforebegin", html) + const nextTrigger = this.hasListTarget ? this.listTarget.querySelector('[data-role="load-more"] a') : null + trigger.remove() + if (nextTrigger) nextTrigger.focus() + } else if (this.hasListTarget) { + this.listTarget.innerHTML = html + } + + this.syncCheckboxes() + this.syncHiddenInputs() + } catch (error) { + if (error.name === "AbortError") return + throw error + } finally { + if (append) { + this.isLoadingMore = false + } + if (targetElement.isConnected) { + targetElement.classList.remove("ajax-loading") + } + } + } + + syncCheckboxes() { + if (!this.hasListTarget) return + this.listTarget.querySelectorAll(this.checkboxSelector).forEach(cb => { + cb.checked = this.selectedValues.has(cb.value) + }) + } + + syncHiddenInputs() { + if (!this.hasHiddenContainerTarget) return + + const name = this.checkboxName + if (!name) return + + this.hiddenContainerTarget.innerHTML = "" + this.selectedValues.forEach(val => { + const escaped = typeof CSS !== "undefined" && CSS.escape ? CSS.escape(val) : val + if (!this.hasListTarget || !this.listTarget.querySelector(`${this.checkboxSelector}[value="${escaped}"]`)) { + const input = document.createElement("input") + input.type = "hidden" + input.name = name + input.value = val + this.hiddenContainerTarget.appendChild(input) + } + }) + } +} diff --git a/app/views/watchers/_new.html.erb b/app/views/watchers/_new.html.erb index 85a1530fc..07cfd2a75 100644 --- a/app/views/watchers/_new.html.erb +++ b/app/views/watchers/_new.html.erb @@ -11,7 +11,17 @@ title = <%= form_tag(watchables.present? ? watchers_path : watchers_append_path, :remote => true, :method => :post, - :id => 'new-watcher-form') do %> + :id => 'new-watcher-form', + :data => { + :controller => 'searchable-selection', + :searchable_selection_url_value => url_for( + :controller => 'watchers', + :action => 'autocomplete_for_user', + :object_type => (watchables.present? ? watchables.first.class.name.underscore : nil), + :object_id => (watchables.present? ? watchables.map(&:id) : nil), + :project_id => @project + ) + }) do %> <% if watchables.present? %> <%= hidden_field_tag 'object_type', watchables.first.class.name.underscore %> @@ -21,27 +31,25 @@ title = <% end %> <%= hidden_field_tag 'project_id', @project.id if @project %> -

<%= label_tag 'user_search', l(:label_user_search) %><%= text_field_tag 'user_search', nil %>

- <%= javascript_tag( - "observeSearchfield( - 'user_search', - 'users_for_watcher', - '#{escape_javascript( - url_for( - :controller => 'watchers', - :action => 'autocomplete_for_user', - :object_type => (watchables.present? ? watchables.first.class.name.underscore : nil), - :object_id => (watchables.present? ? watchables.map(&:id) : nil), - :project_id => @project - ) - )}', - {checkboxSelector: 'input[name=\"watcher[user_ids][]\"]'} - )" - ) %> -
- <%= principals_check_box_tags('watcher[user_ids][]', users) %> +

+ <%= label_tag 'user_search', l(:label_user_search) %> + <%= text_field_tag 'user_search', nil, + :class => 'autocomplete', + :autocomplete => 'off', + :data => { + :searchable_selection_target => 'input', + :action => 'input->searchable-selection#search keydown->searchable-selection#keydown' + } %> +

+ +
+ <%= render :partial => 'watchers/users_for_watcher', :locals => {:users => users} %>
+
+

<%= submit_tag l(:button_add), :name => nil, :onclick => "hideModal(this);" %> <%= link_to_function l(:button_cancel), "hideModal(this);" %> diff --git a/app/views/watchers/_users_for_watcher.html.erb b/app/views/watchers/_users_for_watcher.html.erb new file mode 100644 index 000000000..a83ee00a6 --- /dev/null +++ b/app/views/watchers/_users_for_watcher.html.erb @@ -0,0 +1,19 @@ +<%= principals_check_box_tags 'watcher[user_ids][]', users %> + +<% if @more_users_for_watcher %> +

+ <%= link_to( + l(:label_show_more), + url_for( + :controller => 'watchers', + :action => 'autocomplete_for_user', + :object_type => (params[:object_type].presence || (@watchables.present? ? @watchables.first.class.name.underscore : nil)), + :object_id => (params[:object_id].presence || (@watchables.present? ? @watchables.map(&:id) : nil)), + :project_id => (params[:project_id].presence || (@project ? @project.to_param : nil)), + :q => params[:q].presence, + :offset => @watcher_users_next_offset + ), + :data => {:action => 'searchable-selection#loadMore'} + ) %> +

+<% end %> diff --git a/app/views/watchers/autocomplete_for_user.html.erb b/app/views/watchers/autocomplete_for_user.html.erb index a24d28eb4..640be84f0 100644 --- a/app/views/watchers/autocomplete_for_user.html.erb +++ b/app/views/watchers/autocomplete_for_user.html.erb @@ -1 +1 @@ -<%= principals_check_box_tags 'watcher[user_ids][]', @users %> +<%= render :partial => 'watchers/users_for_watcher', :locals => {:users => @users} %> diff --git a/config/locales/en.yml b/config/locales/en.yml index 77e7de2db..f9bead7a0 100644 --- a/config/locales/en.yml +++ b/config/locales/en.yml @@ -796,6 +796,7 @@ en: label_nobody: nobody label_next: Next label_previous: Previous + label_show_more: Show more label_used_by: Used by label_details: Details label_add_note: Add a note diff --git a/config/locales/ja.yml b/config/locales/ja.yml index 1cd8a9721..0f415e182 100644 --- a/config/locales/ja.yml +++ b/config/locales/ja.yml @@ -602,6 +602,7 @@ ja: label_nobody: 無記名 label_next: 次 label_previous: 前 + label_show_more: さらに表示 label_used_by: 使用中 label_details: 詳細 label_add_note: コメントを追加 diff --git a/test/functional/watchers_controller_test.rb b/test/functional/watchers_controller_test.rb index 62a5e5955..583373e1c 100644 --- a/test/functional/watchers_controller_test.rb +++ b/test/functional/watchers_controller_test.rb @@ -668,4 +668,141 @@ class WatchersControllerTest < Redmine::ControllerTest assert_response :not_found end end + + def test_new_renders_searchable_selection_controller + @request.session[:user_id] = 2 + get :new, :params => {:object_type => 'issue', :object_id => '1'}, :xhr => true + assert_response :success + assert_match 'data-controller=\"searchable-selection\"', response.body + assert_match 'data-searchable-selection-target=\"input\"', response.body + assert_match 'data-searchable-selection-target=\"list\"', response.body + assert_match 'data-searchable-selection-target=\"hiddenContainer\"', response.body + end + + def test_autocomplete_for_user_without_q_should_not_show_load_more_when_count_is_within_limit + @request.session[:user_id] = 2 + get :autocomplete_for_user, :params => {:project_id => 'ecookbook'}, :xhr => true + assert_response :success + assert_select 'p.watcher-more-users', :count => 0 + end + + def test_autocomplete_for_user_without_q_should_show_load_more_when_count_exceeds_limit + @request.session[:user_id] = 2 + role = Role.find(1) + project = Project.find('ecookbook') + 105.times do + u = User.generate! + Member.create!(:project => project, :principal => u, :roles => [role]) + end + + get :autocomplete_for_user, :params => {:project_id => 'ecookbook'}, :xhr => true + assert_response :success + assert_select 'p.watcher-more-users', :count => 1 + assert_select 'p.watcher-more-users a[href*="offset=100"]' + assert_select 'p.watcher-more-users a[data-action="searchable-selection#loadMore"]' + assert_select 'input[name=?]', 'watcher[user_ids][]', :count => 100 + end + + def test_autocomplete_for_user_with_offset_should_load_next_page + @request.session[:user_id] = 2 + role = Role.find(1) + project = Project.find('ecookbook') + 105.times do + u = User.generate! + Member.create!(:project => project, :principal => u, :roles => [role]) + end + + get :autocomplete_for_user, :params => {:project_id => 'ecookbook', :offset => 100}, :xhr => true + assert_response :success + assert_select 'p.watcher-more-users', :count => 0 + assert_select 'input[name=?]', 'watcher[user_ids][]', :count => (project.principals.assignable_watchers.count - 100) + end + + def test_autocomplete_for_user_with_negative_offset_should_default_to_zero + @request.session[:user_id] = 2 + get :autocomplete_for_user, :params => {:project_id => 'ecookbook', :offset => -10}, :xhr => true + assert_response :success + end + + def test_autocomplete_for_user_excludes_existing_watchers_in_scope + @request.session[:user_id] = 2 + issue = Issue.find(1) + project = issue.project + role = Role.find(1) + + 20.times do + u = User.generate! + Member.create!(:project => project, :principal => u, :roles => [role]) + Watcher.create!(:watchable => issue, :user => u) + end + + 105.times do + u = User.generate! + Member.create!(:project => project, :principal => u, :roles => [role]) + end + + get :autocomplete_for_user, :params => {:object_type => 'issue', :object_id => issue.id}, :xhr => true + assert_response :success + assert_select 'input[name=?]', 'watcher[user_ids][]', :count => 100 + assert_select 'p.watcher-more-users', :count => 1 + end + + def test_autocomplete_for_user_with_q_should_not_show_load_more_when_count_is_within_limit + @request.session[:user_id] = 2 + get :autocomplete_for_user, :params => {:project_id => 'ecookbook', :q => 'jsmith'}, :xhr => true + assert_response :success + assert_select 'p.watcher-more-users', :count => 0 + end + + def test_autocomplete_for_user_with_q_should_show_load_more_when_count_exceeds_limit + @request.session[:user_id] = 2 + 105.times do |i| + User.generate!(:firstname => sprintf("SearchTarget%03d", i), :lastname => "User") + end + + get :autocomplete_for_user, :params => {:project_id => 'ecookbook', :q => 'SearchTarget'}, :xhr => true + assert_response :success + assert_select 'p.watcher-more-users', :count => 1 + assert_select 'p.watcher-more-users a[href*="offset=100"]' + assert_select 'p.watcher-more-users a[href*="q=SearchTarget"]' + assert_select 'input[name=?]', 'watcher[user_ids][]', :count => 100 + end + + def test_autocomplete_for_user_with_blank_whitespace_q_defaults_to_project_members + @request.session[:user_id] = 2 + # Non-member user + User.generate!(:firstname => 'WhitespaceNonMember') + + get :autocomplete_for_user, :params => {:project_id => 'ecookbook', :q => ' '}, :xhr => true + assert_response :success + assert_not_include 'WhitespaceNonMember', response.body + end + + def test_autocomplete_for_user_with_q_should_skip_users_without_object_visibility_across_batches + @request.session[:user_id] = 1 + issue = Issue.find(14) + assert issue.is_private? + + # Generate 105 users that cannot view the private issue (not members of the project) + 105.times do |i| + User.generate!(:firstname => sprintf("SearchSkip%03d", i), :lastname => "User") + end + + # Generate 1 admin user matching the search term who can view the private issue + valid_admin = User.generate!(:firstname => "SearchSkipAdmin", :lastname => "User", :admin => true) + + get :autocomplete_for_user, :params => { + :object_type => 'issue', + :object_id => issue.id, + :project_id => issue.project.identifier, + :q => 'SearchSkip' + }, :xhr => true + + assert_response :success + # The valid admin should be returned on the first page even though 105 invalid users matched first + assert_include valid_admin.name, response.body + # Since only 1 valid watcher was found, there are no more pages + assert_select 'p.watcher-more-users', :count => 0 + end end + diff --git a/test/system/watchers_test.rb b/test/system/watchers_test.rb index 4dc23fe13..b0a879580 100644 --- a/test/system/watchers_test.rb +++ b/test/system/watchers_test.rb @@ -73,4 +73,45 @@ class WatchersSystemTest < ApplicationSystemTestCase assert issue.watched_by?(dlopper), "Expected Dave Lopper to be added as watcher" end + + def test_add_watcher_with_show_more + issue = Issue.find(1) + project = issue.project + role = Role.find(1) + issue.watcher_users = [] + + users = [] + 105.times do |i| + u = User.generate!(:firstname => sprintf("Watcher%03d", i), :lastname => "User") + Member.create!(:project => project, :principal => u, :roles => [role]) + users << u + end + + log_user('admin', 'admin') + visit "/issues/#{issue.id}" + + within '#sidebar' do + click_on 'Add' + end + + within '#ajax-modal' do + assert page.has_css?('p.watcher-more-users a', text: 'Show more') + + user1 = users.first + find('label', text: "#{user1.firstname} #{user1.lastname}").click + + click_on 'Show more' + + user2 = users.last + assert page.has_css?('label', text: "#{user2.firstname} #{user2.lastname}") + find('label', text: "#{user2.firstname} #{user2.lastname}").click + + click_on 'Add' + end + + assert page.has_css?('#sidebar', text: "#{user1.firstname} #{user1.lastname}", wait: 5) + issue.reload + assert issue.watched_by?(user1) + assert issue.watched_by?(user2) + end end -- 2.54.0 (Apple Git-157)