diff --git a/app/controllers/auto_completes_controller.rb b/app/controllers/auto_completes_controller.rb index 27226ea8..f1fe99c7 100644 --- a/app/controllers/auto_completes_controller.rb +++ b/app/controllers/auto_completes_controller.rb @@ -48,8 +48,14 @@ class AutoCompletesController < ApplicationController end @removed_users ||= [] + + if params[:include_groups] + user_finder = Principal + else + user_finder = User + end - @users = User.active.like(params[:q]).find(:all, :limit => 100) - @removed_users + @users = user_finder.active.like(params[:q]).find(:all, :limit => 100) - @removed_users render :layout => false end diff --git a/app/controllers/watchers_controller.rb b/app/controllers/watchers_controller.rb index 5d5d2d50..2325634a 100644 --- a/app/controllers/watchers_controller.rb +++ b/app/controllers/watchers_controller.rb @@ -53,7 +53,7 @@ class WatchersController < ApplicationController end def destroy - @watched.set_watcher(User.find(params[:user_id]), false) if request.post? + @watched.set_watcher(Principal.find(params[:user_id]), false) if request.post? respond_to do |format| format.html { redirect_to :back } format.js do diff --git a/app/helpers/application_helper.rb b/app/helpers/application_helper.rb index f4396b56..dde5943a 100644 --- a/app/helpers/application_helper.rb +++ b/app/helpers/application_helper.rb @@ -912,6 +912,12 @@ module ApplicationHelper # +user+ can be a User or a string that will be scanned for an email address (eg. 'joe ') def avatar(user, options = { }) if Setting.gravatar_enabled? + if user.is_a?(Group) + size = options[:size] || 50 + size = "#{size}x#{size}" # image_tag uses WxH + options[:class] ||= 'gravatar' + return image_tag("group.png", options.merge(:size => size)) + end options.merge!({:ssl => (defined?(request) && request.ssl?), :default => Setting.gravatar_default}) email = nil if user.respond_to?(:mail) diff --git a/app/models/group.rb b/app/models/group.rb index 2e7d3526..2b29a8b9 100644 --- a/app/models/group.rb +++ b/app/models/group.rb @@ -22,6 +22,11 @@ class Group < Principal validates_uniqueness_of :lastname, :case_sensitive => false validates_length_of :lastname, :maximum => 30 + # Returns an array of all of the email addresses of the group's users + def mails + users.collect(&:mail) + end + def to_s lastname.to_s end diff --git a/app/models/principal.rb b/app/models/principal.rb index 3c390bb2..ada2cfaa 100644 --- a/app/models/principal.rb +++ b/app/models/principal.rb @@ -48,6 +48,82 @@ class Principal < ActiveRecord::Base end end + def active? + true + end + + def logged? + true # TODO: should all principals default to logged or not? + end + + # Return true if the user is allowed to do the specified action on a specific context + # Action can be: + # * a parameter-like Hash (eg. :controller => 'projects', :action => 'edit') + # * a permission Symbol (eg. :edit_project) + # Context can be: + # * a project : returns true if user is allowed to do the specified action on this project + # * a group of projects : returns true if user is allowed on every project + # * nil with options[:global] set : check if user has at least one role allowed for this action, + # or falls back to Non Member / Anonymous permissions depending if the user is logged + def allowed_to?(action, context, options={}) + if context && context.is_a?(Project) + # No action allowed on archived projects + return false unless context.active? + # No action allowed on disabled modules + return false unless context.allows_to?(action) + # Admin users are authorized for anything else + return true if admin? + + roles = roles_for_project(context) + return false unless roles + roles.detect {|role| (context.is_public? || role.member?) && role.allowed_to?(action)} + + elsif context && context.is_a?(Array) + # Authorize if user is authorized on every element of the array + context.map do |project| + allowed_to?(action,project,options) + end.inject do |memo,allowed| + memo && allowed + end + elsif options[:global] + # Admin users are always authorized + return true if admin? + + # authorize if user has at least one role that has this permission + roles = memberships.collect {|m| m.roles}.flatten.uniq + roles.detect {|r| r.allowed_to?(action)} || (self.logged? ? Role.non_member.allowed_to?(action) : Role.anonymous.allowed_to?(action)) + else + false + end + end + + # Is the user allowed to do the specified action on any project? + # See allowed_to? for the actions and valid options. + def allowed_to_globally?(action, options) + allowed_to?(action, nil, options.reverse_merge(:global => true)) + end + + # Return user's roles for project + def roles_for_project(project) + roles = [] + # No role on archived projects + return roles unless project && project.active? + if logged? + # Find project membership + membership = memberships.detect {|m| m.project_id == project.id} + if membership + roles = membership.roles + else + @role_non_member ||= Role.non_member + roles << @role_non_member + end + else + @role_anonymous ||= Role.anonymous + roles << @role_anonymous + end + roles + end + protected # Make sure we don't try to insert NULL values (see #4632) diff --git a/app/models/user.rb b/app/models/user.rb index 49a9bf4f..63296e8b 100644 --- a/app/models/user.rb +++ b/app/models/user.rb @@ -345,27 +345,6 @@ class User < Principal !logged? end - # Return user's roles for project - def roles_for_project(project) - roles = [] - # No role on archived projects - return roles unless project && project.active? - if logged? - # Find project membership - membership = memberships.detect {|m| m.project_id == project.id} - if membership - roles = membership.roles - else - @role_non_member ||= Role.non_member - roles << @role_non_member - end - else - @role_anonymous ||= Role.anonymous - roles << @role_anonymous - end - roles - end - # Return true if the user is a member of project def member_of?(project) !roles_for_project(project).detect {|role| role.member?}.nil? @@ -388,53 +367,6 @@ class User < Principal @projects_by_role end - # Return true if the user is allowed to do the specified action on a specific context - # Action can be: - # * a parameter-like Hash (eg. :controller => 'projects', :action => 'edit') - # * a permission Symbol (eg. :edit_project) - # Context can be: - # * a project : returns true if user is allowed to do the specified action on this project - # * a group of projects : returns true if user is allowed on every project - # * nil with options[:global] set : check if user has at least one role allowed for this action, - # or falls back to Non Member / Anonymous permissions depending if the user is logged - def allowed_to?(action, context, options={}) - if context && context.is_a?(Project) - # No action allowed on archived projects - return false unless context.active? - # No action allowed on disabled modules - return false unless context.allows_to?(action) - # Admin users are authorized for anything else - return true if admin? - - roles = roles_for_project(context) - return false unless roles - roles.detect {|role| (context.is_public? || role.member?) && role.allowed_to?(action)} - - elsif context && context.is_a?(Array) - # Authorize if user is authorized on every element of the array - context.map do |project| - allowed_to?(action,project,options) - end.inject do |memo,allowed| - memo && allowed - end - elsif options[:global] - # Admin users are always authorized - return true if admin? - - # authorize if user has at least one role that has this permission - roles = memberships.collect {|m| m.roles}.flatten.uniq - roles.detect {|r| r.allowed_to?(action)} || (self.logged? ? Role.non_member.allowed_to?(action) : Role.anonymous.allowed_to?(action)) - else - false - end - end - - # Is the user allowed to do the specified action on any project? - # See allowed_to? for the actions and valid options. - def allowed_to_globally?(action, options) - allowed_to?(action, nil, options.reverse_merge(:global => true)) - end - safe_attributes 'login', 'firstname', 'lastname', diff --git a/app/models/watcher.rb b/app/models/watcher.rb index bc1bed6b..aa0fc92a 100644 --- a/app/models/watcher.rb +++ b/app/models/watcher.rb @@ -14,8 +14,8 @@ class Watcher < ActiveRecord::Base belongs_to :watchable, :polymorphic => true - belongs_to :user - + belongs_to :user, :class_name => 'Principal', :foreign_key => 'user_id' + validates_presence_of :user validates_uniqueness_of :user_id, :scope => [:watchable_type, :watchable_id] diff --git a/app/views/watchers/_watchers.rhtml b/app/views/watchers/_watchers.rhtml index 741366cd..00b4954c 100644 --- a/app/views/watchers/_watchers.rhtml +++ b/app/views/watchers/_watchers.rhtml @@ -12,12 +12,12 @@ :object_id => watched}, :method => :post, :html => {:id => 'new-watcher-form', :style => 'display:none;'}) do |f| %> - <% users = User.active.find(:all, :limit => 25) - watched.watcher_users %> + <% users = Principal.active.find(:all, :limit => 25) - watched.watcher_users %>

<%= label_tag "user_search", l(:label_user_search) %><%= text_field_tag 'user_search', nil, :style => "width:98%;" %>

<%= observe_field(:user_search, :frequency => 0.5, :update => :users, - :url => auto_complete_users_path(:remove_watchers => watched.id, :klass => watched.class), + :url => auto_complete_users_path(:remove_watchers => watched.id, :klass => watched.class, :include_groups => true), :with => 'q') %> diff --git a/test/functional/auto_completes_controller_test.rb b/test/functional/auto_completes_controller_test.rb index 4ee6482d..5881862d 100644 --- a/test/functional/auto_completes_controller_test.rb +++ b/test/functional/auto_completes_controller_test.rb @@ -92,6 +92,20 @@ class AutoCompletesControllerTest < ActionController::TestCase end end + context "including groups" do + setup do + @group = Group.generate(:lastname => 'Complete Group').reload + get :users, :q => 'complete', :include_groups => true + end + + should_respond_with :success + + should "include matching groups" do + assert_select "input[type=checkbox][value=?]", @group.id + end + + end + context "restrict by removing group members" do setup do @group = Group.first diff --git a/test/functional/watchers_controller_test.rb b/test/functional/watchers_controller_test.rb index 858e807e..ad839bac 100644 --- a/test/functional/watchers_controller_test.rb +++ b/test/functional/watchers_controller_test.rb @@ -132,6 +132,21 @@ class WatchersControllerTest < ActionController::TestCase assert Issue.find(2).watched_by?(User.find(7)) end + context "POST :new" do + should "add groups" do + @group = Group.generate!.reload + Member.generate!(:project => Project.find(1), :roles => [Role.find(1)], :principal => @group) + + @request.session[:user_id] = 2 + assert_difference('Watcher.count') do + xhr :post, :new, :object_type => 'issue', :object_id => '2', :user_ids => [@group.id.to_s] + assert_response :success + assert_select_rjs :replace_html, 'watchers' + end + assert Issue.find(2).watched_by?(@group) + end + end + def test_remove_watcher @request.session[:user_id] = 2 assert_difference('Watcher.count', -1) do @@ -141,4 +156,23 @@ class WatchersControllerTest < ActionController::TestCase end assert !Issue.find(2).watched_by?(User.find(3)) end + + context "POST :destroy" do + should "remove a group" do + @group = Group.generate!.reload + Member.generate!(:project => Project.find(1), :roles => [Role.find(1)], :principal => @group) + assert Issue.find(2).add_watcher(@group) + assert Issue.find(2).watched_by?(@group) + + @request.session[:user_id] = 2 + assert_difference('Watcher.count', -1) do + xhr :post, :destroy, :object_type => 'issue', :object_id => '2', :user_id => @group.id.to_s + assert_response :success + assert_select_rjs :replace_html, 'watchers' + end + assert !Issue.find(2).watched_by?(@group) + end + + end + end diff --git a/test/unit/watcher_test.rb b/test/unit/watcher_test.rb index c850849e..7ac1bd58 100644 --- a/test/unit/watcher_test.rb +++ b/test/unit/watcher_test.rb @@ -14,11 +14,7 @@ require File.expand_path('../../test_helper', __FILE__) class WatcherTest < ActiveSupport::TestCase - fixtures :projects, :users, :members, :member_roles, :roles, :enabled_modules, - :issues, - :boards, :messages, - :wikis, :wiki_pages, - :watchers + fixtures :all def setup @user = User.find(1) @@ -125,4 +121,34 @@ class WatcherTest < ActiveSupport::TestCase assert Issue.find(1).watched_by?(user) assert !Issue.find(4).watched_by?(user) end + + context "group watch" do + setup do + @group = Group.generate! + Member.generate!(:project => Project.find(1), :roles => [Role.find(1)], :principal => @group) + @group.users << @user = User.find(1) + @group.users << @user2 = User.find(2) + end + + should "be valid" do + assert @issue.add_watcher(@group) + @issue.reload + assert @issue.watchers.detect { |w| w.user == @group } + end + + should "add all group members to recipients" do + @issue.watchers.delete_all + @issue.reload + + assert @issue.watcher_recipients.empty? + assert @issue.add_watcher(@group) + + @user.save + @issue.reload + assert @issue.watcher_recipients.include?(@user.mail) + assert @issue.watcher_recipients.include?(@user2.mail) + + end + + end end diff --git a/vendor/plugins/acts_as_watchable/lib/acts_as_watchable.rb b/vendor/plugins/acts_as_watchable/lib/acts_as_watchable.rb index 2b158383..9d99d2b7 100644 --- a/vendor/plugins/acts_as_watchable/lib/acts_as_watchable.rb +++ b/vendor/plugins/acts_as_watchable/lib/acts_as_watchable.rb @@ -42,7 +42,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?(Principal) Watcher.delete_all "watchable_type = '#{self.class}' AND watchable_id = #{self.id} AND user_id = #{user.id}" end @@ -64,7 +64,14 @@ module Redmine if respond_to?(:visible?) notified.reject! {|user| !visible?(user)} end - notified.collect(&:mail).compact + + notified.collect {|w| + if w.respond_to?(:mail) && w.mail.present? # Single mail + w.mail + elsif w.respond_to?(:mails) && w.mails.present? # Multiple mail + w.mails + end + }.flatten.compact end module ClassMethods; end