jro_apps/redmine Commit - r15673:3a2b6c7c9653

Enforce issue assignee validation (#23921)....

Jean-Philippe Lang -

r15673:3a2b6c7c9653

parent child

Context file:

r15673:3a2b6c7c9653

Collapse all files

app/models/issue.rb +15 -9

                   if category
                     self.category = project.issue_categories.find_by_name(category.name)
                   end
+                  # Clear the assignee if not available in the new project for new issues (eg. copy)
+                  # For existing issue, the previous assignee is still valid, so we keep it
+                  if new_record? && assigned_to && !assignable_users.include?(assigned_to)
+                    self.assigned_to_id = nil
+                  end
                   # Keep the fixed_version if it's still valid in the new_project
                   if fixed_version && fixed_version.project != project && !project.shared_versions.include?(fixed_version)
                     self.fixed_version = nil
                   self.status = statuses_allowed.first || default_status
                 end
                 if (u = attrs.delete('assigned_to_id')) && safe_attribute?('assigned_to_id')
-                  if u.blank?
+                  self.assigned_to_id = u
-                    self.assigned_to_id = nil
-                  else
-                    u = u.to_i
-                    if assignable_users.any?{|assignable_user| assignable_user.id == u}
-                      self.assigned_to_id = u
-                    end
-                  end
                 end
                   end
                 end
+                if assigned_to_id_changed? && assigned_to_id.present?
+                  unless assignable_users.include?(assigned_to)
+                    errors.add :assigned_to_id, :invalid
+                  end
+                end
                 # Checks parent issue assignment
                 if @invalid_parent_issue_id.present?
                   errors.add :parent_issue_id, :invalid
               def assignable_users
                 users = project.assignable_users(tracker).to_a
                 users << author if author && author.active?
-                users << assigned_to if assigned_to
+                if assigned_to_id_was.present? && assignee = Principal.find_by_id(assigned_to_id_was)
+                  users << assignee
+                end
                 users.uniq.sort
               end

app/models/project.rb +1 -1

               def copy(project, options={})
                 project = project.is_a?(Project) ? project : Project.find(project)
-                to_be_copied = %w(wiki versions issue_categories issues members queries boards)
+                to_be_copied = %w(members wiki versions issue_categories issues queries boards)
                 to_be_copied = to_be_copied & Array.wrap(options[:only]) unless options[:only].nil?
                 Project.transaction do

test/functional/issues_controller_test.rb +2 -2

                                 :assigned_to_id => nil),
                   Issue.create!(:project_id => 2, :tracker_id => 3, :status_id => 2,
                                 :priority_id => 1, :subject => 'issue 2', :author_id => 2,
-                                :assigned_to_id => 3)
+                                :assigned_to_id => 2)
                 ]
                 assert_difference 'Issue.count', issues.size do
                   post :bulk_update, :ids => issues.map(&:id), :copy => '1',
                   post :bulk_update, :ids => [1], :copy => '1',
                        :notes => 'Copying one issue',
                        :issue => {
-                         :project_id => '', :tracker_id => '', :assigned_to_id => '4',
+                         :project_id => '', :tracker_id => '',
                          :status_id => '3', :start_date => '2009-12-01', :due_date => '2009-12-31'
                        }
                 end

test/integration/api_test/issues_test.rb +8 0

                 assert_select 'errors error', :text => "Subject cannot be blank"
               end
+              test "PUT /issues/:id.xml with invalid assignee should return error" do
+                user = User.generate!
+                put '/issues/6.xml', {:issue => {:assigned_to_id => user.id}}, credentials('jsmith')
+                assert_response :unprocessable_entity
+                assert_select 'errors error', :text => "Assignee is invalid"
+              end
               test "PUT /issues/:id.json" do
                 assert_difference('Journal.count') do
                   put '/issues/6.json',

test/unit/issue_test.rb +57 -71

               def test_anonymous_should_not_see_private_issues_with_issues_visibility_set_to_default
                 Role.anonymous.update!(:issues_visibility => 'default')
-                issue = Issue.generate!(:author => User.anonymous, :assigned_to => User.anonymous, :is_private => true)
+                issue = Issue.generate!(:author => User.anonymous, :is_private => true)
                 assert_nil Issue.where(:id => issue.id).visible(User.anonymous).first
                 assert !issue.visible?(User.anonymous)
               end
               def test_anonymous_should_not_see_private_issues_with_issues_visibility_set_to_own
                 assert Role.anonymous.update!(:issues_visibility => 'own')
-                issue = Issue.generate!(:author => User.anonymous, :assigned_to => User.anonymous, :is_private => true)
+                issue = Issue.generate!(:author => User.anonymous, :is_private => true)
                 assert_nil Issue.where(:id => issue.id).visible(User.anonymous).first
                 assert !issue.visible?(User.anonymous)
               end
               def test_visible_scope_for_member_with_groups_should_return_assigned_issues
                 user = User.find(8)
                 assert user.groups.any?
-                Member.create!(:principal => user.groups.first, :project_id => 1, :role_ids => [2])
+                group = user.groups.first
+                Member.create!(:principal => group, :project_id => 1, :role_ids => [2])
                 Role.non_member.remove_permission!(:view_issues)
-                issue = Issue.create!(:project_id => 1, :tracker_id => 1, :author_id => 3,
+                with_settings :issue_group_assignment => '1' do
-                  :status_id => 1, :priority => IssuePriority.all.first,
+                  issue = Issue.create!(:project_id => 1, :tracker_id => 1, :author_id => 3,
-                  :subject => 'Assignment test',
+                    :status_id => 1, :priority => IssuePriority.all.first,
-                  :assigned_to => user.groups.first,
+                    :subject => 'Assignment test',
-                  :is_private => true)
+                    :assigned_to => group,
+                    :is_private => true)
-                Role.find(2).update! :issues_visibility => 'default'
-                issues = Issue.visible(User.find(8)).to_a
+                  Role.find(2).update! :issues_visibility => 'default'
-                assert issues.any?
+                  issues = Issue.visible(User.find(8)).to_a
-                assert issues.include?(issue)
+                  assert issues.any?
+                  assert issues.include?(issue)
-                Role.find(2).update! :issues_visibility => 'own'
-                issues = Issue.visible(User.find(8)).to_a
+                  Role.find(2).update! :issues_visibility => 'own'
-                assert issues.any?
+                  issues = Issue.visible(User.find(8)).to_a
-                assert_include issue, issues
+                  assert issues.any?
+                  assert_include issue, issues
+                end
               end
               def test_visible_scope_for_member_with_limited_tracker_ids
               def test_visible_and_nested_set_scopes
                 user = User.generate!
+                Member.create!(:project_id => 1, :principal => user, :role_ids => [1])
                 parent = Issue.generate!(:assigned_to => user)
                 assert parent.visible?(user)
                 child1 = Issue.generate!(:parent_issue_id => parent.id, :assigned_to => user)
                                         :project_id => 1, :author => user,
                                         :assigned_to => user)
                 assert_equal [1, 2, 3, 4, 5], issue.new_statuses_allowed_to(user).map(&:id)
-                group = Group.generate!
-                group.users << user
-                issue = Issue.generate!(:tracker => tracker, :status => status,
-                                        :project_id => 1, :author => user,
-                                        :assigned_to => group)
-                assert_equal [1, 2, 3, 4, 5], issue.new_statuses_allowed_to(user).map(&:id)
               end
               def test_new_statuses_allowed_to_should_consider_group_assignment
                 WorkflowTransition.create!(:role_id => 1, :tracker_id => 1,
                                            :old_status_id => 1, :new_status_id => 4,
                                            :author => false, :assignee => true)
-                user = User.find(2)
                 group = Group.generate!
+                Member.create!(:project_id => 1, :principal => group, :role_ids => [1])
+                user = User.find(2)
                 group.users << user
-                issue = Issue.generate!(:author_id => 1, :assigned_to => group)
+                with_settings :issue_group_assignment => '1' do
-                assert_include 4, issue.new_statuses_allowed_to(user).map(&:id)
+                  issue = Issue.generate!(:author_id => 1, :assigned_to => group)
+                  assert_include 4, issue.new_statuses_allowed_to(user).map(&:id)
+                end
               end
               def test_new_statuses_allowed_to_should_return_all_transitions_for_admin
                 assert_nil issue.custom_field_value(cf2)
               end
-              def test_safe_attributes_should_ignore_unassignable_assignee
-                issue = Issue.new(:project_id => 1, :tracker_id => 1, :author_id => 3,
-                                  :status_id => 1, :priority => IssuePriority.all.first,
-                                  :subject => 'test_create')
-                assert issue.valid?
-                # locked user, not allowed
-                issue.safe_attributes=({'assigned_to_id' => '5'})
-                assert_nil issue.assigned_to_id
-                # no member
-                issue.safe_attributes=({'assigned_to_id' => '1'})
-                assert_nil issue.assigned_to_id
-                # user 2 is ok
-                issue.safe_attributes=({'assigned_to_id' => '2'})
-                assert_equal 2, issue.assigned_to_id
-                assert issue.save
-                issue.reload
-                assert_equal 2, issue.assigned_to_id
-                issue.safe_attributes=({'assigned_to_id' => '5'})
-                assert_equal 2, issue.assigned_to_id
-                issue.safe_attributes=({'assigned_to_id' => '1'})
-                assert_equal 2, issue.assigned_to_id
-                # user 3 is also ok
-                issue.safe_attributes=({'assigned_to_id' => '3'})
-                assert_equal 3, issue.assigned_to_id
-                assert issue.save
-                # removal of assignee
-                issue.safe_attributes=({'assigned_to_id' => ''})
-                assert_nil issue.assigned_to_id
-                assert issue.save
-              end
               def test_editable_custom_field_values_should_return_non_readonly_custom_values
                 cf1 = IssueCustomField.create!(:name => 'Writable field', :field_format => 'string',
                                                :is_for_all => true, :tracker_ids => [1, 2])
                 assert_equal "125", issue.custom_value_for(2).value
               end
+              def test_copy_to_another_project_should_clear_assignee_if_not_valid
+                issue = Issue.generate!(:project_id => 1, :assigned_to_id => 2)
+                project = Project.generate!
+                issue = Issue.new.copy_from(1)
+                issue.project = project
+                assert_nil issue.assigned_to
+              end
               def test_copy_should_copy_status
                 orig = Issue.find(8)
                 assert orig.status != orig.default_status
               end
               test "#copy should not create a journal" do
-                copy = Issue.find(1).copy({:project_id => 3, :tracker_id => 2, :assigned_to_id => 3}, :link => false)
+                copy = Issue.find(1).copy({:project_id => 3, :tracker_id => 2}, :link => false)
                 copy.save!
                 assert_equal 0, copy.reload.journals.size
               end
               test "#copy should allow assigned_to changes" do
-                copy = Issue.find(1).copy(:project_id => 3, :tracker_id => 2, :assigned_to_id => 3)
+                user = User.generate!
-                assert_equal 3, copy.assigned_to_id
+                Member.create!(:project_id => 3, :principal => user, :role_ids => [1])
+                copy = Issue.find(1).copy(:project_id => 3, :tracker_id => 2, :assigned_to_id => user.id)
+                assert_equal user.id, copy.assigned_to_id
               end
               test "#copy should allow status changes" do
                 assert !issue.assignable_users.include?(user)
               end
-              test "#assignable_users should include the current assignee" do
+              def test_assignable_users_should_include_the_current_assignee
                 user = User.generate!
+                Member.create!(:project_id => 1, :principal => user, :role_ids => [1])
                 issue = Issue.generate!(:assigned_to => user)
                 user.lock!
               end
               test "Issue#recipients should include the assigned to user if the assigned to user is active" do
-                issue = Issue.generate!(:assigned_to => User.generate!)
+                user = User.generate!
+                Member.create!(:project_id => 1, :principal => user, :role_ids => [1])
+                issue = Issue.generate!(:assigned_to => user)
                 assert issue.assigned_to, "No assigned_to set for Issue"
                 assert issue.recipients.include?(issue.assigned_to.mail)
               end
               end
               test "Issue#recipients should not include the assigned user if they are only notified of owned issues" do
-                issue = Issue.generate!(:assigned_to => User.generate!)
+                user = User.generate!
+                Member.create!(:project_id => 1, :principal => user, :role_ids => [1])
+                issue = Issue.generate!(:assigned_to => user)
                 issue.assigned_to.update!(:mail_notification => :only_owner)
                 assert !issue.recipients.include?(issue.assigned_to.mail)
               end
               def test_assigned_to_was_with_a_group
                 group = Group.find(10)
+                Member.create!(:project_id => 1, :principal => group, :role_ids => [1])
-                issue = Issue.generate!(:assigned_to => group)
+                with_settings :issue_group_assignment => '1' do
-                issue.reload.assigned_to = nil
+                  issue = Issue.generate!(:assigned_to => group)
-                assert_equal group, issue.assigned_to_was
+                  issue.reload.assigned_to = nil
+                  assert_equal group, issue.assigned_to_was
+                end
               end
               def test_issue_overdue_should_respect_user_timezone

test/unit/mailer_test.rb +2 -1

               end
               def test_reminder_should_include_issues_assigned_to_groups
-                with_settings :default_language => 'en' do
+                with_settings :default_language => 'en', :issue_group_assignment => '1' do
                   group = Group.generate!
+                  Member.create!(:project_id => 1, :principal => group, :role_ids => [1])
                   group.users << User.find(2)
                   group.users << User.find(3)

test/unit/query_test.rb +18 -12

@@ -676,19 +676,25 class QueryTest < ActiveSupport::TestCase
676	def test_filter_assigned_to_me	676	def test_filter_assigned_to_me
677	user = User.find(2)	677	user = User.find(2)
678	group = Group.find(10)	678	group = Group.find(10)
679	User.current = user
680	i1 = Issue.generate!(:project_id => 1, :tracker_id => 1, :assigned_to => user)
681	i2 = Issue.generate!(:project_id => 1, :tracker_id => 1, :assigned_to => group)
682	i3 = Issue.generate!(:project_id => 1, :tracker_id => 1, :assigned_to => Group.find(11))
683	group.users << user	679	group.users << user
		680	other_group = Group.find(11)
		681	Member.create!(:project_id => 1, :principal => group, :role_ids => [1])
		682	Member.create!(:project_id => 1, :principal => other_group, :role_ids => [1])
		683	User.current = user
684		684
685	query = IssueQuery.new(:name => '_', :filters => { 'assigned_to_id' => {:operator => '=', :values => ['me']}})	685	with_settings :issue_group_assignment => '1' do
686	result = query.issues	686	i1 = Issue.generate!(:project_id => 1, :tracker_id => 1, :assigned_to => user)
687	assert_equal Issue.visible.where(:assigned_to_id => ([2] + user.reload.group_ids)).sort_by(&:id), result.sort_by(&:id)	687	i2 = Issue.generate!(:project_id => 1, :tracker_id => 1, :assigned_to => group)
688		688	i3 = Issue.generate!(:project_id => 1, :tracker_id => 1, :assigned_to => other_group)
689	assert result.include?(i1)	689
690	assert result.include?(i2)	690	query = IssueQuery.new(:name => '_', :filters => { 'assigned_to_id' => {:operator => '=', :values => ['me']}})
691	assert !result.include?(i3)	691	result = query.issues
		692	assert_equal Issue.visible.where(:assigned_to_id => ([2] + user.reload.group_ids)).sort_by(&:id), result.sort_by(&:id)
		693
		694	assert result.include?(i1)
		695	assert result.include?(i2)
		696	assert !result.include?(i3)
		697	end
692	end	698	end
693		699
694	def test_user_custom_field_filtered_on_me	700	def test_user_custom_field_filtered_on_me
@@ -1689,7 +1695,7 class QueryTest < ActiveSupport::TestCase
1689	@issue1 = Issue.generate!(:project => @project, :assigned_to_id => @manager.id)	1695	@issue1 = Issue.generate!(:project => @project, :assigned_to_id => @manager.id)
1690	@issue2 = Issue.generate!(:project => @project, :assigned_to_id => @developer.id)	1696	@issue2 = Issue.generate!(:project => @project, :assigned_to_id => @developer.id)
1691	@issue3 = Issue.generate!(:project => @project, :assigned_to_id => @boss.id)	1697	@issue3 = Issue.generate!(:project => @project, :assigned_to_id => @boss.id)
1692	@issue4 = Issue.generate!(:project => @project, :assigned_to_id => @guest.id)	1698	@issue4 = Issue.generate!(:project => @project, :author_id => @guest.id, :assigned_to_id => @guest.id)
1693	@issue5 = Issue.generate!(:project => @project)	1699	@issue5 = Issue.generate!(:project => @project)
1694		1700
1695	@query = IssueQuery.new(:name => '_', :project => @project)	1701	@query = IssueQuery.new(:name => '_', :project => @project)

test/unit/user_test.rb +3 0

                 project = Project.find(1)
                 author = User.generate!
                 assignee = User.generate!
+                Member.create!(:user => assignee, :project => project, :role_ids => [1])
                 member = User.generate!
                 Member.create!(:user => member, :project => project, :role_ids => [1])
                 issue = Issue.generate!(:project => project, :assigned_to => assignee, :author => author)
               def test_notify_about_issue_for_previous_assignee
                 assignee = User.generate!(:mail_notification => 'only_assigned')
+                Member.create!(:user => assignee, :project_id => 1, :role_ids => [1])
                 new_assignee = User.generate!(:mail_notification => 'only_assigned')
+                Member.create!(:user => new_assignee, :project_id => 1, :role_ids => [1])
                 issue = Issue.generate!(:assigned_to => assignee)
                 assert assignee.notify_about?(issue)

General Comments 0

Write
Preview

You need to be logged in to leave comments. Login now

No TODOs yet

	Site-wide shortcuts
/	Use quick search box
g h	Goto home page
g g	Goto my private gists page
g G	Goto my public gists page
g 0-9	Goto bookmarked items from 0-9
n r	New repository page
n g	New gist page

	Repositories
g s	Goto summary page
g c	Goto changelog page
g f	Goto files page
g F	Goto files page with file search activated
g p	Goto pull requests page
g o	Goto repository settings
g O	Goto repository access permissions settings
t s	Toggle sidebar on some pages