diff --git a/app/controllers/watchers_controller.rb b/app/controllers/watchers_controller.rb index f0692e31d..c741a9326 100644 --- a/app/controllers/watchers_controller.rb +++ b/app/controllers/watchers_controller.rb @@ -42,7 +42,9 @@ class WatchersController < ApplicationController else user_ids << params[:user_id] end - users = User.active.visible.where(:id => user_ids.flatten.compact.uniq) + user_ids = user_ids.flatten.compact.uniq + users = User.active.visible.where(:id => user_ids).to_a + users += Group.givable.active.visible.where(:id => user_ids).to_a users.each do |user| @watchables.each do |watchable| Watcher.create(:watchable => watchable, :user => user) @@ -59,6 +61,7 @@ class WatchersController < ApplicationController if params[:watcher] user_ids = params[:watcher][:user_ids] || [params[:watcher][:user_id]] @users = User.active.visible.where(:id => user_ids).to_a + @users += Group.givable.active.visible.where(:id => user_ids).to_a end if @users.blank? head 200 @@ -66,7 +69,7 @@ class WatchersController < ApplicationController end def destroy - user = User.find(params[:user_id]) + user = Principal.find(params[:user_id]) @watchables.each do |watchable| watchable.set_watcher(user, false) end @@ -126,6 +129,7 @@ class WatchersController < ApplicationController scope = User.all.limit(100) end users = scope.active.visible.sorted.like(params[:q]).to_a + users += Group.givable.active.visible.sorted.like(params[:q]).to_a if @watchables && @watchables.size == 1 users -= @watchables.first.watcher_users end diff --git a/app/helpers/avatars_helper.rb b/app/helpers/avatars_helper.rb index 7e2a1fffd..49be006a8 100644 --- a/app/helpers/avatars_helper.rb +++ b/app/helpers/avatars_helper.rb @@ -51,6 +51,8 @@ module AvatarsHelper gravatar(email.to_s.downcase, options) rescue nil elsif user.is_a?(AnonymousUser) anonymous_avatar(options) + elsif user.is_a?(Group) + group_avatar(options) else nil end @@ -72,4 +74,8 @@ module AvatarsHelper def anonymous_avatar(options={}) image_tag 'anonymous.png', GravatarHelper::DEFAULT_OPTIONS.except(:default, :rating, :ssl).merge(options) end + + def group_avatar(options={}) + image_tag 'group.png', GravatarHelper::DEFAULT_OPTIONS.except(:default, :rating, :ssl).merge(options) + end end diff --git a/app/helpers/issues_helper.rb b/app/helpers/issues_helper.rb index 2ea81d566..34411470f 100644 --- a/app/helpers/issues_helper.rb +++ b/app/helpers/issues_helper.rb @@ -365,8 +365,8 @@ module IssuesHelper # on the new issue form def users_for_new_issue_watchers(issue) users = issue.watcher_users.select{|u| u.status == User::STATUS_ACTIVE} - if issue.project.users.count <= 20 - users = (users + issue.project.users.sort).uniq + if issue.project.users.count + Group.givable.count <= 20 + users = (users + issue.project.users.sort + Group.givable.sort).uniq end users end diff --git a/app/helpers/watchers_helper.rb b/app/helpers/watchers_helper.rb index dab5e6b76..ac732c339 100644 --- a/app/helpers/watchers_helper.rb +++ b/app/helpers/watchers_helper.rb @@ -47,7 +47,7 @@ module WatchersHelper def watchers_list(object) remove_allowed = User.current.allowed_to?("delete_#{object.class.name.underscore}_watchers".to_sym, object.project) content = ''.html_safe - lis = object.watcher_users.preload(:email_address).collect do |user| + lis = object.watcher_users.collect do |user| s = ''.html_safe s << avatar(user, :size => "16").to_s s << link_to_user(user, :class => 'user') diff --git a/app/models/group.rb b/app/models/group.rb index 21166f1af..c1106e07f 100644 --- a/app/models/group.rb +++ b/app/models/group.rb @@ -115,6 +115,7 @@ class Group < Principal return if self.id.nil? Issue.where(['assigned_to_id = ?', id]).update_all('assigned_to_id = NULL') + Watcher.where('user_id = ?', id).delete_all end end diff --git a/app/models/query.rb b/app/models/query.rb index 0e38b47d8..5672a6e9e 100644 --- a/app/models/query.rb +++ b/app/models/query.rb @@ -600,7 +600,7 @@ class Query < ActiveRecord::Base def watcher_values watcher_values = [["<< #{l(:label_me)} >>", "me"]] - watcher_values += users.sort_by(&:status).collect{|s| [s.name, s.id.to_s, l("status_#{User::LABEL_BY_STATUS[s.status]}")] } if User.current.allowed_to?(:view_issue_watchers, self.project) + watcher_values += principals.sort_by(&:status).collect{|s| [s.name, s.id.to_s, l("status_#{User::LABEL_BY_STATUS[s.status]}")] } if User.current.allowed_to?(:view_issue_watchers, self.project) watcher_values end @@ -913,7 +913,7 @@ class Query < ActiveRecord::Base if v.delete("me") if User.current.logged? v.push(User.current.id.to_s) - v += User.current.group_ids.map(&:to_s) if field == 'assigned_to_id' + v += User.current.group_ids.map(&:to_s) if %w(assigned_to_id watcher_id).include?(field) else v.push("0") end diff --git a/app/models/watcher.rb b/app/models/watcher.rb index 8ea457060..15bfaa6db 100644 --- a/app/models/watcher.rb +++ b/app/models/watcher.rb @@ -19,7 +19,7 @@ class Watcher < ActiveRecord::Base belongs_to :watchable, :polymorphic => true - belongs_to :user + belongs_to :user, :class_name => 'Principal' validates_presence_of :user validates_uniqueness_of :user_id, :scope => [:watchable_type, :watchable_id] @@ -54,7 +54,8 @@ class Watcher < ActiveRecord::Base protected def validate_user - errors.add :user_id, :invalid unless user.nil? || user.active? + errors.add :user_id, :invalid \ + unless user.nil? || (user.is_a?(User) && user.active?) || (user.is_a?(Group) && user.givable?) end def self.prune_single_user(user, options={}) diff --git a/lib/plugins/acts_as_watchable/lib/acts_as_watchable.rb b/lib/plugins/acts_as_watchable/lib/acts_as_watchable.rb index 1ebe9e2fc..e8dc9ee7b 100644 --- a/lib/plugins/acts_as_watchable/lib/acts_as_watchable.rb +++ b/lib/plugins/acts_as_watchable/lib/acts_as_watchable.rb @@ -31,9 +31,9 @@ module Redmine # Returns an array of users that are proposed as watchers def addable_watcher_users - users = self.project.users.sort - self.watcher_users + users = (self.project.users.sort + Group.givable.sort) - self.watcher_users if respond_to?(:visible?) - users.reject! {|user| !visible?(user)} + users.reject! {|user| user.is_a?(User) && !visible?(user)} end users end @@ -47,7 +47,7 @@ module Redmine # Removes user from the watchers list def remove_watcher(user) - return nil unless user && user.is_a?(User) + return nil unless user && (user.is_a?(User) || user.is_a?(Group)) # Rails does not reset the has_many :through association watcher_users.reset watchers.where(:user_id => user.id).delete_all @@ -73,6 +73,8 @@ module Redmine def notified_watchers notified = watcher_users.active.to_a + notified = notified.map {|n| n.is_a?(Group) ? n.users : n}.flatten + notified.uniq! notified.reject! {|user| user.mail.blank? || user.mail_notification == 'none'} if respond_to?(:visible?) notified.reject! {|user| !visible?(user)} diff --git a/test/functional/issues_controller_test.rb b/test/functional/issues_controller_test.rb index 11d93cf21..2dcf06bb1 100644 --- a/test/functional/issues_controller_test.rb +++ b/test/functional/issues_controller_test.rb @@ -2443,28 +2443,41 @@ class IssuesControllerTest < Redmine::ControllerTest def test_show_should_display_watchers @request.session[:user_id] = 2 - Issue.find(1).add_watcher User.find(2) + issue = Issue.find(1) + issue.add_watcher User.find(2) + issue.add_watcher Group.find(10) get(:show, :params => {:id => 1}) assert_select 'div#watchers ul' do - assert_select 'li' do + assert_select 'li.user-2' do assert_select 'a[href="/users/2"]' assert_select 'a[class*=delete]' end + assert_select "li.user-10" do + assert_select 'a[href="/users/10"]', false + assert_select 'a[class*=delete]' + end end end def test_show_should_display_watchers_with_gravatars @request.session[:user_id] = 2 - Issue.find(1).add_watcher User.find(2) + issue = Issue.find(1) + issue.add_watcher User.find(2) + issue.add_watcher Group.find(10) with_settings :gravatar_enabled => '1' do get(:show, :params => {:id => 1}) end assert_select 'div#watchers ul' do - assert_select 'li' do - assert_select 'img.gravatar' + assert_select 'li.user-2' do + assert_select 'img.gravatar[title=?]', 'John Smith' assert_select 'a[href="/users/2"]' assert_select 'a[class*=delete]' end + assert_select "li.user-10" do + assert_select 'img.gravatar[title=?]', 'A Team' + assert_select 'a[href="/users/10"]', false + assert_select 'a[class*=delete]' + end end end @@ -3995,7 +4008,7 @@ class IssuesControllerTest < Redmine::ControllerTest ActionMailer::Base.deliveries.clear with_settings :notified_events => %w(issue_added) do - assert_difference 'Watcher.count', 2 do + assert_difference 'Watcher.count', 3 do post( :create, :params => { @@ -4005,7 +4018,7 @@ class IssuesControllerTest < Redmine::ControllerTest :subject => 'This is a new issue with watchers', :description => 'This is the description', :priority_id => 5, - :watcher_user_ids => ['2', '3'] + :watcher_user_ids => ['2', '3', '10'] } } ) @@ -4016,12 +4029,15 @@ class IssuesControllerTest < Redmine::ControllerTest assert_redirected_to :controller => 'issues', :action => 'show', :id => issue # Watchers added - assert_equal [2, 3], issue.watcher_user_ids.sort + assert_equal [2, 3, 10], issue.watcher_user_ids.sort assert issue.watched_by?(User.find(3)) + assert issue.watched_by?(Group.find(10)) # Watchers notified - mail = ActionMailer::Base.deliveries.last - assert_not_nil mail + assert_equal 3, ActionMailer::Base.deliveries.size + mail = ActionMailer::Base.deliveries[1] assert [mail.bcc, mail.cc].flatten.include?(User.find(3).mail) + mail = ActionMailer::Base.deliveries[2] + assert [mail.bcc, mail.cc].flatten.include?(User.find(8).mail) end def test_post_create_subissue @@ -4740,8 +4756,10 @@ class IssuesControllerTest < Redmine::ControllerTest def test_new_as_copy_should_preserve_watchers @request.session[:user_id] = 2 + issue = Issue.find(1) user = User.generate! - Watcher.create!(:watchable => Issue.find(1), :user => user) + Watcher.create!(:watchable => issue, :user => user) + Watcher.create!(:watchable => issue, :user => Group.find(10)) get( :new, :params => { @@ -4749,8 +4767,9 @@ class IssuesControllerTest < Redmine::ControllerTest :copy_from => 1 } ) - assert_select 'input[type=checkbox][name=?][checked=checked]', 'issue[watcher_user_ids][]', 1 + assert_select 'input[type=checkbox][name=?][checked=checked]', 'issue[watcher_user_ids][]', 2 assert_select 'input[type=checkbox][name=?][checked=checked][value=?]', 'issue[watcher_user_ids][]', user.id.to_s + assert_select 'input[type=checkbox][name=?][checked=checked][value=?]', 'issue[watcher_user_ids][]', '10' assert_select 'input[type=hidden][name=?][value=?]', 'issue[watcher_user_ids][]', '', 1 end @@ -5174,13 +5193,13 @@ class IssuesControllerTest < Redmine::ControllerTest :copy_from => copied.id, :issue => { :subject => 'Copy cleared watchers', - :watcher_user_ids => ['', '3'] + :watcher_user_ids => ['', '3', '10'] } } ) end issue = Issue.order('id DESC').first - assert_equal [3], issue.watcher_user_ids + assert_equal [3, 10], issue.watcher_user_ids end def test_create_as_copy_without_watcher_user_ids_should_not_copy_watchers @@ -7329,7 +7348,9 @@ class IssuesControllerTest < Redmine::ControllerTest end test "issue bulk copy copy watcher" do - Watcher.create!(:watchable => Issue.find(1), :user => User.find(3)) + issue = Issue.find(1) + Watcher.create!(:watchable => issue, :user => User.find(3)) + Watcher.create!(:watchable => issue, :user => Group.find(10)) @request.session[:user_id] = 2 assert_difference 'Issue.count' do post( @@ -7345,7 +7366,8 @@ class IssuesControllerTest < Redmine::ControllerTest ) end copy = Issue.order(:id => :desc).first - assert_equal 1, copy.watchers.count + assert_equal 2, copy.watchers.count + assert_equal [3, 10], copy.watcher_user_ids end def test_bulk_copy_should_not_copy_selected_subtasks_twice diff --git a/test/functional/queries_controller_test.rb b/test/functional/queries_controller_test.rb index 39e4a20b7..fe5501e32 100644 --- a/test/functional/queries_controller_test.rb +++ b/test/functional/queries_controller_test.rb @@ -834,7 +834,7 @@ class QueriesControllerTest < Redmine::ControllerTest assert_equal [["<< me >>", "me"]], json end - def test_watcher_filter_with_permission_should_show_members + def test_watcher_filter_with_permission_should_show_members_and_groups # This user has view_issue_watchers permission @request.session[:user_id] = 1 @@ -847,10 +847,11 @@ class QueriesControllerTest < Redmine::ControllerTest assert_equal 'application/json', response.media_type json = ActiveSupport::JSON.decode(response.body) - assert_equal 6, json.count + assert_equal 7, json.count # "me" value should not be grouped assert_include ["<< me >>", "me"], json assert_include ["Dave Lopper", "3", "active"], json assert_include ["Dave2 Lopper2", "5", "locked"], json + assert_include ["A Team", "10", "active"], json end end diff --git a/test/functional/watchers_controller_test.rb b/test/functional/watchers_controller_test.rb index 8500d2359..35c946531 100644 --- a/test/functional/watchers_controller_test.rb +++ b/test/functional/watchers_controller_test.rb @@ -189,6 +189,19 @@ class WatchersControllerTest < Redmine::ControllerTest assert Issue.find(2).watched_by?(User.find(4)) end + def test_create_group_as_html + @request.session[:user_id] = 2 + assert_difference('Watcher.count') do + post :create, :params => { + :object_type => 'issue', :object_id => '2', + :watcher => {:user_id => '10'} + } + assert_response :success + assert_include 'Watcher added', response.body + end + assert Issue.find(2).watched_by?(Group.find(10)) + end + def test_create @request.session[:user_id] = 2 assert_difference('Watcher.count') do @@ -205,45 +218,56 @@ class WatchersControllerTest < Redmine::ControllerTest def test_create_with_mutiple_users @request.session[:user_id] = 2 - assert_difference('Watcher.count', 2) do + assert_difference('Watcher.count', 3) do post :create, :params => { :object_type => 'issue', :object_id => '2', - :watcher => {:user_ids => ['4', '7']} + :watcher => {:user_ids => ['4', '7', '10']} }, :xhr => true assert_response :success assert_match /watchers/, response.body assert_match /ajax-modal/, response.body end - assert Issue.find(2).watched_by?(User.find(4)) - assert Issue.find(2).watched_by?(User.find(7)) + issue = Issue.find(2) + assert issue.watched_by?(User.find(4)) + assert issue.watched_by?(User.find(7)) + assert issue.watched_by?(Group.find(10)) end def test_create_with_mutiple_objects @request.session[:user_id] = 2 - assert_difference('Watcher.count', 4) do + assert_difference('Watcher.count', 6) do post :create, :params => { :object_type => 'issue', :object_id => ['1', '2'], - :watcher => {:user_ids => ['4', '7']} + :watcher => {:user_ids => ['4', '7', '10']} }, :xhr => true assert_response :success assert_match /watchers/, response.body assert_match /ajax-modal/, response.body end - assert Issue.find(1).watched_by?(User.find(4)) - assert Issue.find(2).watched_by?(User.find(4)) - assert Issue.find(1).watched_by?(User.find(7)) - assert Issue.find(2).watched_by?(User.find(7)) + issue1 = Issue.find(1) + issue2 = Issue.find(2) + user4 = User.find(4) + user7 = User.find(7) + group10 = Group.find(10) + assert issue1.watched_by?(user4) + assert issue2.watched_by?(user4) + assert issue1.watched_by?(user7) + assert issue2.watched_by?(user7) + assert issue1.watched_by?(group10) + assert issue2.watched_by?(group10) end def test_autocomplete_on_watchable_creation @request.session[:user_id] = 2 + group = Group.generate!(:name => 'Group Minimum') get :autocomplete_for_user, :params => {:q => 'mi', :project_id => 'ecookbook'}, :xhr => true assert_response :success - assert_select 'input', :count => 4 + assert_select 'input', :count => 5 assert_select 'input[name=?][value="1"]', 'watcher[user_ids][]' assert_select 'input[name=?][value="2"]', 'watcher[user_ids][]' assert_select 'input[name=?][value="8"]', 'watcher[user_ids][]' assert_select 'input[name=?][value="9"]', 'watcher[user_ids][]' + assert_select %(input[name=?][value="#{group.id}"]), 'watcher[user_ids][]' end def test_search_non_member_on_create @@ -342,6 +366,23 @@ class WatchersControllerTest < Redmine::ControllerTest assert !Issue.find(2).watched_by?(User.find(3)) end + def test_destroy_group_as_html + @request.session[:user_id] = 2 + issue = Issue.find(2) + group = Group.find(10) + issue.add_watcher(group) + assert issue.watched_by?(group) + assert_difference('Watcher.count', -1) do + delete :destroy, :params => { + :object_type => 'issue', :object_id => '2', :user_id => '10' + } + assert_response :success + assert_include 'Watcher removed', response.body + end + issue.reload + assert_not issue.watched_by?(group) + end + def test_destroy @request.session[:user_id] = 2 assert_difference('Watcher.count', -1) do diff --git a/test/helpers/avatars_helper_test.rb b/test/helpers/avatars_helper_test.rb index e02755683..de8573f9b 100644 --- a/test/helpers/avatars_helper_test.rb +++ b/test/helpers/avatars_helper_test.rb @@ -43,7 +43,7 @@ class AvatarsHelperTest < Redmine::HelperTest end def test_avatar_with_group - assert_nil avatar(Group.first) + assert_match %r{src="/images/group.png(\?\d+)?"}, avatar(Group.first) end def test_avatar_with_invalid_arg_should_return_nil diff --git a/test/unit/group_test.rb b/test/unit/group_test.rb index 77f79500b..23f2a78da 100644 --- a/test/unit/group_test.rb +++ b/test/unit/group_test.rb @@ -23,10 +23,9 @@ class GroupTest < ActiveSupport::TestCase fixtures :projects, :trackers, :issue_statuses, :issues, :enumerations, :users, :projects_trackers, - :roles, - :member_roles, - :members, - :groups_users + :roles, :member_roles, :members, + :groups_users, + :watchers include Redmine::I18n @@ -128,14 +127,21 @@ class GroupTest < ActiveSupport::TestCase assert !User.find(8).member_of?(Project.find(5)) end - def test_destroy_should_unassign_issues + def test_destroy_should_unassign_and_unwatch_issues group = Group.find(10) Issue.where(:id => 1).update_all(["assigned_to_id = ?", group.id]) + issue = Issue.find(2) + issue.set_watcher(group) + issue.save + issue.reload + assert issue.watcher_user_ids.include?(10) assert group.destroy assert group.destroyed? assert_nil Issue.find(1).assigned_to_id + issue.reload + assert_not issue.watcher_user_ids.include?(10) end def test_builtin_groups_should_be_created_if_missing diff --git a/test/unit/query_test.rb b/test/unit/query_test.rb index 56ec82d14..8293c89b5 100644 --- a/test/unit/query_test.rb +++ b/test/unit/query_test.rb @@ -421,8 +421,6 @@ class QueryTest < ActiveSupport::TestCase issues = find_issues_with_query(query) assert issues.any? assert_nil issues.detect {|issue| !issue.is_private?} - ensure - User.current = nil end def test_operator_is_not_on_is_private_field @@ -436,8 +434,6 @@ class QueryTest < ActiveSupport::TestCase issues = find_issues_with_query(query) assert issues.any? assert_nil issues.detect {|issue| issue.is_private?} - ensure - User.current = nil end def test_operator_greater_than @@ -950,7 +946,20 @@ class QueryTest < ActiveSupport::TestCase assert_not_nil result assert !result.empty? assert_equal Issue.visible.watched_by(User.current).sort_by(&:id), result.sort_by(&:id) - User.current = nil + end + + def test_filter_watched_issues_with_groups_also + user = User.find(2) + group = Group.find(10) + group.users << user + Issue.find(3).add_watcher(user) + Issue.find(7).add_watcher(group) + User.current = user + query = IssueQuery.new(:name => '_', :filters => { 'watcher_id' => {:operator => '=', :values => ['me']}}) + result = find_issues_with_query(query) + assert_not_nil result + assert_not result.empty? + assert_equal [3, 7], result.sort_by(&:id).pluck(:id) end def test_filter_unwatched_issues @@ -960,7 +969,6 @@ class QueryTest < ActiveSupport::TestCase assert_not_nil result assert !result.empty? assert_equal((Issue.visible - Issue.watched_by(User.current)).sort_by(&:id).size, result.sort_by(&:id).size) - User.current = nil end def test_filter_on_watched_issues_with_view_issue_watchers_permission @@ -974,9 +982,6 @@ class QueryTest < ActiveSupport::TestCase result = find_issues_with_query(query) assert_includes result, Issue.find(1) assert_includes result, Issue.find(3) - ensure - User.current.reload - User.current = nil end def test_filter_on_watched_issues_without_view_issue_watchers_permission @@ -990,9 +995,6 @@ class QueryTest < ActiveSupport::TestCase result = find_issues_with_query(query) assert_includes result, Issue.find(1) assert_not_includes result, Issue.find(3) - ensure - User.current.reload - User.current = nil end def test_filter_on_custom_field_should_ignore_projects_with_field_disabled diff --git a/test/unit/watcher_test.rb b/test/unit/watcher_test.rb index 761bf5a71..48e619807 100644 --- a/test/unit/watcher_test.rb +++ b/test/unit/watcher_test.rb @@ -40,9 +40,12 @@ class WatcherTest < ActiveSupport::TestCase end def test_watch + group = Group.find(10) + assert @issue.add_watcher(group) assert @issue.add_watcher(@user) @issue.reload assert @issue.watchers.detect {|w| w.user == @user} + assert @issue.watchers.detect {|w| w.user == group} end def test_cant_watch_twice @@ -103,12 +106,14 @@ class WatcherTest < ActiveSupport::TestCase def test_addable_watcher_users addable_watcher_users = @issue.addable_watcher_users assert_kind_of Array, addable_watcher_users - assert_kind_of User, addable_watcher_users.first + addable_watcher_users.each do |addable_watcher| + assert_equal true, addable_watcher.is_a?(User) || addable_watcher.is_a?(Group) + end end def test_addable_watcher_users_should_not_include_user_that_cannot_view_the_object issue = Issue.new(:project => Project.find(1), :is_private => true) - assert_nil issue.addable_watcher_users.detect {|user| !issue.visible?(user)} + assert_nil issue.addable_watcher_users.detect {|user| user.is_a?(User) && !issue.visible?(user)} end def test_any_watched_should_return_false_if_no_object_is_watched @@ -147,9 +152,12 @@ class WatcherTest < ActiveSupport::TestCase end def test_unwatch + group = Group.find(10) + assert @issue.add_watcher(group) assert @issue.add_watcher(@user) @issue.reload assert_equal 1, @issue.remove_watcher(@user) + assert_equal 1, @issue.remove_watcher(group) end def test_prune_with_user