Merge pull request #5131 from opf/fix/multiple_roles_less_work_packages

prevent WorkPackage#visible to result in duplicate entries for pluck

app/models/query/results.rb

@@ -87,12 +87,8 @@ class ::Query::Results
     includes = ([:status, :project] +
       includes_for_columns(query.involved_columns) + (options[:include] || [])).uniq
 
-    # A 'distinct' is added by the visible scope which is not necessary for
-    # filtering the work packages and which might conflict with ordering in
-    # mysql.
     WorkPackage
       .visible
-      .distinct(false)
       .where(::Query.merge_conditions(query.statement, options[:conditions]))
       .includes(includes)
       .joins((query.group_by_column ? query.group_by_column.join : nil))

app/models/work_package.rb

@@ -69,8 +69,7 @@ class WorkPackage < ActiveRecord::Base
   }
 
   scope :visible, ->(*args) {
-    joins(:project)
-      .merge(Project.allowed_to(args.first || User.current, :view_work_packages))
+    where(project_id: Project.allowed_to(args.first || User.current, :view_work_packages))
   }
 
   scope :in_status, -> (*args) do

lib/api/v3/work_packages/work_package_collection_representer.rb

@@ -139,7 +139,7 @@ module API
         end
 
         def paged_models(models)
-          models.page(@page).per_page(@per_page).pluck(:id).uniq
+          models.page(@page).per_page(@per_page).pluck(:id)
         end
 
         def full_work_packages(ids_in_order)

spec/models/work_package/work_package_visibility_spec.rb

@@ -29,51 +29,71 @@
 require 'spec_helper'
 
 describe 'WorkPackage-Visibility', type: :model do
-  let(:admin)    { FactoryGirl.create(:admin) }
+  let(:admin) { FactoryGirl.create(:admin) }
   let(:anonymous) { FactoryGirl.create(:anonymous) }
-  let(:user)    { FactoryGirl.create(:user) }
+  let(:user) { FactoryGirl.create(:user) }
   let(:public_project) { FactoryGirl.create(:project, is_public: true) }
   let(:private_project) { FactoryGirl.create(:project, is_public: false) }
   let(:other_project) { FactoryGirl.create(:project, is_public: true) }
   let(:view_work_packages) { FactoryGirl.create(:role, permissions: [:view_work_packages]) }
+  let(:view_work_packages_role2) { FactoryGirl.create(:role, permissions: [:view_work_packages]) }
 
   describe 'of public projects' do
     subject { FactoryGirl.create(:work_package, project: public_project) }
 
-    it 'should be viewable by anonymous users, when the anonymous-role has the permission to view packages' do
+    it 'is viewable by anonymous, with the view_work_packages permissison' do
       # it is not really clear, where these kind of "preconditions" belong to: This setting
       # is a default in Redmine::DefaultData::Loader - but this not loaded in the tests: here we
       # just make sure, that the workpackage is visible, when this permission is set
       Role.anonymous.add_permission! :view_work_packages
-      expect(WorkPackage.visible(anonymous)).to include subject
+      expect(WorkPackage.visible(anonymous)).to match_array [subject]
     end
   end
 
   describe 'of private projects' do
     subject { FactoryGirl.create(:work_package, project: private_project) }
 
-    it 'should be visible for the admin, even if the project is private' do
-      expect(WorkPackage.visible(admin)).to include subject
+    it 'is visible for the admin, even if the project is private' do
+      expect(WorkPackage.visible(admin)).to match_array [subject]
     end
 
-    it 'should not be visible for anonymous users, when the project is private' do
-      expect(WorkPackage.visible(anonymous)).not_to include subject
+    it 'is not visible for anonymous users, when the project is private' do
+      expect(WorkPackage.visible(anonymous)).to match_array []
     end
 
-    it 'should be visible for members of the project, that are allowed to view workpackages' do
-      member = FactoryGirl.create(:member, user: user, project: private_project, role_ids: [view_work_packages.id])
-      expect(WorkPackage.visible(user)).to include subject
+    it 'is visible for members of the project, with the view_work_packages permissison' do
+      FactoryGirl.create(:member,
+                         user: user,
+                         project: private_project,
+                         role_ids: [view_work_packages.id])
+
+      expect(WorkPackage.visible(user)).to match_array [subject]
+    end
+
+    it 'is only returned once for members with two roles having view_work_packages permission' do
+      subject
+
+      FactoryGirl.create(:member,
+                         user: user,
+                         project: private_project,
+                         role_ids: [view_work_packages.id,
+                                    view_work_packages_role2.id])
+
+      expect(WorkPackage.visible(user).pluck(:id)).to match_array [subject.id]
     end
 
-    it 'should __not__ be visible for non-members of the project without the permission to view workpackages' do
-      expect(WorkPackage.visible(user)).not_to include subject
+    it 'is not visible for non-members of the project without the view_work_packages permissison' do
+      expect(WorkPackage.visible(user)).to match_array []
     end
 
-    it 'should __not__ be visible for members of the project, without the right to view work_packages' do
+    it 'is not visible for members of the project, without the view_work_packages permissison' do
       no_permission = FactoryGirl.create(:role, permissions: [:no_permission])
-      member = FactoryGirl.create(:member, user: user, project: private_project, role_ids: [no_permission.id])
+      FactoryGirl.create(:member,
+                         user: user,
+                         project: private_project,
+                         role_ids: [no_permission.id])
 
-      expect(WorkPackage.visible(user)).not_to include subject
+      expect(WorkPackage.visible(user)).to match_array []
     end
   end
 end