From f33231181f9591ee67577e229a8bf6de24516ba0 Mon Sep 17 00:00:00 2001 From: Jean-Philippe Lang Date: Sun, 13 Dec 2009 12:39:22 +0000 Subject: [PATCH] Makes user unwatch what he can no longer view after its permissions have changed (#3589). A rake task (redmine:watchers:prune) is also added to prune existing watchers. git-svn-id: svn+ssh://rubyforge.org/var/svn/redmine/trunk@3167 e93f8b46-1217-0410-a6f0-8f06a7374b81 --- app/models/board.rb | 4 ++ app/models/member.rb | 17 +++++++- app/models/member_role.rb | 7 +++- app/models/message.rb | 4 ++ app/models/watcher.rb | 35 ++++++++++++++++ app/models/wiki.rb | 4 ++ app/models/wiki_page.rb | 4 ++ lib/tasks/watchers.rake | 9 ++++ test/fixtures/enabled_modules.yml | 4 ++ test/fixtures/messages.yml | 11 +++++ test/unit/member_test.rb | 68 ++++++++++++++++++++++++++++++- test/unit/watcher_test.rb | 42 +++++++++++++++++-- 12 files changed, 203 insertions(+), 6 deletions(-) create mode 100644 lib/tasks/watchers.rake diff --git a/app/models/board.rb b/app/models/board.rb index ada138375..e7310da65 100644 --- a/app/models/board.rb +++ b/app/models/board.rb @@ -27,6 +27,10 @@ class Board < ActiveRecord::Base validates_length_of :name, :maximum => 30 validates_length_of :description, :maximum => 255 + def visible?(user=User.current) + !user.nil? && user.allowed_to?(:view_messages, project) + end + def to_s name end diff --git a/app/models/member.rb b/app/models/member.rb index 6fffb2161..44a421745 100644 --- a/app/models/member.rb +++ b/app/models/member.rb @@ -24,6 +24,8 @@ class Member < ActiveRecord::Base validates_presence_of :principal, :project validates_uniqueness_of :user_id, :scope => :project_id + + after_destroy :unwatch_from_permission_change def name self.user.name @@ -39,7 +41,11 @@ class Member < ActiveRecord::Base # Add new roles new_role_ids.each {|id| member_roles << MemberRole.new(:role_id => id) } # Remove roles (Rails' #role_ids= will not trigger MemberRole#on_destroy) - member_roles.select {|mr| !ids.include?(mr.role_id)}.each(&:destroy) + member_roles_to_destroy = member_roles.select {|mr| !ids.include?(mr.role_id)} + if member_roles_to_destroy.any? + member_roles_to_destroy.each(&:destroy) + unwatch_from_permission_change + end end def <=>(member) @@ -63,4 +69,13 @@ class Member < ActiveRecord::Base def validate errors.add_to_base "Role can't be blank" if member_roles.empty? && roles.empty? end + + private + + # Unwatch things that the user is no longer allowed to view inside project + def unwatch_from_permission_change + if user + Watcher.prune(:user => user, :project => project) + end + end end diff --git a/app/models/member_role.rb b/app/models/member_role.rb index 5a31c17c5..286659fc3 100644 --- a/app/models/member_role.rb +++ b/app/models/member_role.rb @@ -49,6 +49,11 @@ class MemberRole < ActiveRecord::Base end def remove_role_from_group_users - MemberRole.find(:all, :conditions => { :inherited_from => id }).each(&:destroy) + MemberRole.find(:all, :conditions => { :inherited_from => id }).group_by(&:member).each do |member, member_roles| + member_roles.each(&:destroy) + if member && member.user + Watcher.prune(:user => member.user, :project => member.project) + end + end end end diff --git a/app/models/message.rb b/app/models/message.rb index f37413286..1e59719dd 100644 --- a/app/models/message.rb +++ b/app/models/message.rb @@ -42,6 +42,10 @@ class Message < ActiveRecord::Base after_create :add_author_as_watcher + def visible?(user=User.current) + !user.nil? && user.allowed_to?(:view_messages, project) + end + def validate_on_create # Can not reply to a locked topic errors.add_to_base 'Topic is locked' if root.locked? && self != root diff --git a/app/models/watcher.rb b/app/models/watcher.rb index b13039f84..a6c0c331c 100644 --- a/app/models/watcher.rb +++ b/app/models/watcher.rb @@ -21,10 +21,45 @@ class Watcher < ActiveRecord::Base validates_presence_of :user validates_uniqueness_of :user_id, :scope => [:watchable_type, :watchable_id] + + # Unwatch things that users are no longer allowed to view + def self.prune(options={}) + if options.has_key?(:user) + prune_single_user(options[:user], options) + else + pruned = 0 + User.find(:all, :conditions => "id IN (SELECT DISTINCT user_id FROM #{table_name})").each do |user| + pruned += prune_single_user(user, options) + end + pruned + end + end protected def validate errors.add :user_id, :invalid unless user.nil? || user.active? end + + private + + def self.prune_single_user(user, options={}) + return unless user.is_a?(User) + pruned = 0 + find(:all, :conditions => {:user_id => user.id}).each do |watcher| + next if watcher.watchable.nil? + + if options.has_key?(:project) + next unless watcher.watchable.respond_to?(:project) && watcher.watchable.project == options[:project] + end + + if watcher.watchable.respond_to?(:visible?) + unless watcher.watchable.visible?(user) + watcher.destroy + pruned += 1 + end + end + end + pruned + end end diff --git a/app/models/wiki.rb b/app/models/wiki.rb index b31b03482..b9a76fb32 100644 --- a/app/models/wiki.rb +++ b/app/models/wiki.rb @@ -25,6 +25,10 @@ class Wiki < ActiveRecord::Base validates_presence_of :start_page validates_format_of :start_page, :with => /^[^,\.\/\?\;\|\:]*$/ + def visible?(user=User.current) + !user.nil? && user.allowed_to?(:view_wiki_pages, project) + end + # find the page with the given title # if page doesn't exist, return a new page def find_or_new_page(title) diff --git a/app/models/wiki_page.rb b/app/models/wiki_page.rb index d6009c2e3..1b0bf4d49 100644 --- a/app/models/wiki_page.rb +++ b/app/models/wiki_page.rb @@ -40,6 +40,10 @@ class WikiPage < ActiveRecord::Base validates_format_of :title, :with => /^[^,\.\/\?\;\|\s]*$/ validates_uniqueness_of :title, :scope => :wiki_id, :case_sensitive => false validates_associated :content + + def visible?(user=User.current) + !user.nil? && user.allowed_to?(:view_wiki_pages, project) + end def title=(value) value = Wiki.titleize(value) diff --git a/lib/tasks/watchers.rake b/lib/tasks/watchers.rake new file mode 100644 index 000000000..caa9e05f6 --- /dev/null +++ b/lib/tasks/watchers.rake @@ -0,0 +1,9 @@ +desc 'Removes watchers from what they can no longer view.' + +namespace :redmine do + namespace :watchers do + task :prune => :environment do + Watcher.prune + end + end +end diff --git a/test/fixtures/enabled_modules.yml b/test/fixtures/enabled_modules.yml index b0fbf63ea..0a83168df 100644 --- a/test/fixtures/enabled_modules.yml +++ b/test/fixtures/enabled_modules.yml @@ -59,3 +59,7 @@ enabled_modules_015: name: wiki project_id: 2 id: 15 +enabled_modules_016: + name: boards + project_id: 2 + id: 16 diff --git a/test/fixtures/messages.yml b/test/fixtures/messages.yml index b1c59772b..b193599d5 100644 --- a/test/fixtures/messages.yml +++ b/test/fixtures/messages.yml @@ -66,3 +66,14 @@ messages_006: author_id: 3 parent_id: 4 board_id: 1 +messages_007: + created_on: <%= 2.days.ago.to_date.to_s(:db) %> + updated_on: <%= 2.days.ago.to_date.to_s(:db) %> + subject: 'Message on a private project' + id: 7 + replies_count: 0 + last_reply_id: + content: "This is a private message" + author_id: 1 + parent_id: + board_id: 3 diff --git a/test/unit/member_test.rb b/test/unit/member_test.rb index 41ac48859..323ec1e9e 100644 --- a/test/unit/member_test.rb +++ b/test/unit/member_test.rb @@ -18,7 +18,7 @@ require File.dirname(__FILE__) + '/../test_helper' class MemberTest < ActiveSupport::TestCase - fixtures :users, :projects, :roles, :members, :member_roles + fixtures :all def setup @jsmith = Member.find(1) @@ -68,4 +68,70 @@ class MemberTest < ActiveSupport::TestCase assert_raise(ActiveRecord::RecordNotFound) { Member.find(@jsmith.id) } end + + context "removing permissions" do + setup do + Watcher.delete_all("user_id = 9") + user = User.find(9) + # public + Watcher.create!(:watchable => Issue.find(1), :user_id => user) + # private + Watcher.create!(:watchable => Issue.find(4), :user => user) + Watcher.create!(:watchable => Message.find(7), :user => user) + Watcher.create!(:watchable => Wiki.find(2), :user => user) + Watcher.create!(:watchable => WikiPage.find(3), :user => user) + end + + context "of user" do + setup do + @member = Member.create!(:project => Project.find(2), :principal => User.find(9), :role_ids => [1, 2]) + end + + context "by deleting membership" do + should "prune watchers" do + assert_difference 'Watcher.count', -4 do + @member.destroy + end + end + end + + context "by updating roles" do + should "prune watchers" do + Role.find(2).remove_permission! :view_wiki_pages + member = Member.first(:order => 'id desc') + assert_difference 'Watcher.count', -2 do + member.role_ids = [2] + member.save + end + assert !Message.find(7).watched_by?(@user) + end + end + end + + context "of group" do + setup do + group = Group.find(10) + @member = Member.create!(:project => Project.find(2), :principal => group, :role_ids => [1, 2]) + group.users << User.find(9) + end + + context "by deleting membership" do + should "prune watchers" do + assert_difference 'Watcher.count', -4 do + @member.destroy + end + end + end + + context "by updating roles" do + should "prune watchers" do + Role.find(2).remove_permission! :view_wiki_pages + assert_difference 'Watcher.count', -2 do + @member.role_ids = [2] + @member.save + end + end + end + end + end end diff --git a/test/unit/watcher_test.rb b/test/unit/watcher_test.rb index f49365edf..db73f8ccc 100644 --- a/test/unit/watcher_test.rb +++ b/test/unit/watcher_test.rb @@ -1,5 +1,5 @@ -# redMine - project management software -# Copyright (C) 2006-2007 Jean-Philippe Lang +# Redmine - project management software +# Copyright (C) 2006-2009 Jean-Philippe Lang # # This program is free software; you can redistribute it and/or # modify it under the terms of the GNU General Public License @@ -18,7 +18,11 @@ require File.dirname(__FILE__) + '/../test_helper' class WatcherTest < ActiveSupport::TestCase - fixtures :issues, :users + fixtures :projects, :users, :members, :member_roles, :roles, :enabled_modules, + :issues, + :boards, :messages, + :wikis, :wiki_pages, + :watchers def setup @user = User.find(1) @@ -66,4 +70,36 @@ class WatcherTest < ActiveSupport::TestCase @issue.reload assert_equal 1, @issue.remove_watcher(@user) end + + def test_prune + Watcher.delete_all("user_id = 9") + user = User.find(9) + + # public + Watcher.create!(:watchable => Issue.find(1), :user => user) + Watcher.create!(:watchable => Issue.find(2), :user => user) + Watcher.create!(:watchable => Message.find(1), :user => user) + Watcher.create!(:watchable => Wiki.find(1), :user => user) + Watcher.create!(:watchable => WikiPage.find(2), :user => user) + + # private project (id: 2) + Member.create!(:project => Project.find(2), :principal => user, :role_ids => [1]) + Watcher.create!(:watchable => Issue.find(4), :user => user) + Watcher.create!(:watchable => Message.find(7), :user => user) + Watcher.create!(:watchable => Wiki.find(2), :user => user) + Watcher.create!(:watchable => WikiPage.find(3), :user => user) + + assert_no_difference 'Watcher.count' do + Watcher.prune(:user => User.find(9)) + end + + Member.delete_all + + assert_difference 'Watcher.count', -4 do + Watcher.prune(:user => User.find(9)) + end + + assert Issue.find(1).watched_by?(user) + assert !Issue.find(4).watched_by?(user) + end end