Revert "Merge branch '22481-honour-issue-visibility-for-groups' into 'security'

This reverts commit 5614d724.

app/finders/issuable_finder.rb

@@ -69,26 +69,31 @@ class IssuableFinder
   def project
     return @project if defined?(@project)
 
-    project = Project.find(params[:project_id])
-    project = nil unless Ability.allowed?(current_user, :"read_#{klass.to_ability_name}", project)
+    if project?
+      @project = Project.find(params[:project_id])
 
-    @project = project
+      unless Ability.allowed?(current_user, :read_project, @project)
+        @project = nil
+      end
+    else
+      @project = nil
+    end
+
+    @project
   end
 
   def projects
     return @projects if defined?(@projects)
-    return @projects = project if project?
 
-    projects =
-      if current_user && params[:authorized_only].presence && !current_user_related?
-        current_user.authorized_projects
+    if project?
+      @projects = project
+    elsif current_user && params[:authorized_only].presence && !current_user_related?
+      @projects = current_user.authorized_projects.reorder(nil)
     elsif group
-        GroupProjectsFinder.new(group).execute(current_user)
+      @projects = GroupProjectsFinder.new(group).execute(current_user).reorder(nil)
     else
-        ProjectsFinder.new.execute(current_user)
+      @projects = ProjectsFinder.new.execute(current_user).reorder(nil)
     end
-
-    @projects = projects.with_feature_available_for_user(klass, current_user).reorder(nil)
   end
 
   def search

app/models/concerns/issuable.rb

@@ -172,10 +172,6 @@ module Issuable
 
       grouping_columns
     end
-
-    def to_ability_name
-      model_name.singular
-    end
   end
 
   def today?
@@ -249,7 +245,7 @@ module Issuable
   #   issuable.class           # => MergeRequest
   #   issuable.to_ability_name # => "merge_request"
   def to_ability_name
-    self.class.to_ability_name
+    self.class.to_s.underscore
   end
 
   # Returns a Hash of attributes to be used for Twitter card metadata

app/models/project.rb

@@ -195,38 +195,8 @@ class Project < ActiveRecord::Base
   scope :for_milestones, ->(ids) { joins(:milestones).where('milestones.id' => ids).distinct }
   scope :with_push, -> { joins(:events).where('events.action = ?', Event::PUSHED) }
 
-  scope :with_project_feature, -> { joins('LEFT JOIN project_features ON projects.id = project_features.project_id') }
-
-  # "enabled" here means "not disabled". It includes private features!
-  scope :with_feature_enabled, ->(feature) {
-    access_level_attribute = ProjectFeature.access_level_attribute(feature)
-    with_project_feature.where(project_features: { access_level_attribute => [nil, ProjectFeature::PRIVATE, ProjectFeature::ENABLED] })
-  }
-
-  # Picks a feature where the level is exactly that given.
-  scope :with_feature_access_level, ->(feature, level) {
-    access_level_attribute = ProjectFeature.access_level_attribute(feature)
-    with_project_feature.where(project_features: { access_level_attribute => level })
-  }
-
-  scope :with_builds_enabled, -> { with_feature_enabled(:builds) }
-  scope :with_issues_enabled, -> { with_feature_enabled(:issues) }
-
-  # project features may be "disabled", "internal" or "enabled". If "internal",
-  # they are only available to team members. This scope returns projects where
-  # the feature is either enabled, or internal with permission for the user.
-  def self.with_feature_available_for_user(feature, user)
-    return with_feature_enabled(feature) if user.try(:admin?)
-
-    unconditional = with_feature_access_level(feature, [nil, ProjectFeature::ENABLED])
-    return unconditional if user.nil?
-
-    conditional = with_feature_access_level(feature, ProjectFeature::PRIVATE)
-    authorized = user.authorized_projects.merge(conditional.reorder(nil))
-
-    union = Gitlab::SQL::Union.new([unconditional.select(:id), authorized.select(:id)])
-    where(arel_table[:id].in(Arel::Nodes::SqlLiteral.new(union.to_sql)))
-  end
+  scope :with_builds_enabled, -> { joins('LEFT JOIN project_features ON projects.id = project_features.project_id').where('project_features.builds_access_level IS NULL or project_features.builds_access_level > 0') }
+  scope :with_issues_enabled, -> { joins('LEFT JOIN project_features ON projects.id = project_features.project_id').where('project_features.issues_access_level IS NULL or project_features.issues_access_level > 0') }
 
   scope :active, -> { joins(:issues, :notes, :merge_requests).order('issues.created_at, notes.created_at, merge_requests.created_at DESC') }
   scope :abandoned, -> { where('projects.last_activity_at < ?', 6.months.ago) }

app/models/project_feature.rb

@@ -20,15 +20,6 @@ class ProjectFeature < ActiveRecord::Base
 
   FEATURES = %i(issues merge_requests wiki snippets builds)
 
-  class << self
-    def access_level_attribute(feature)
-      feature = feature.model_name.plural.to_sym if feature.respond_to?(:model_name)
-      raise ArgumentError, "invalid project feature: #{feature}" unless FEATURES.include?(feature)
-
-      "#{feature}_access_level".to_sym
-    end
-  end
-
   # Default scopes force us to unscope here since a service may need to check
   # permissions for a project in pending_delete
   # http://stackoverflow.com/questions/1540645/how-to-disable-default-scope-for-a-belongs-to
@@ -41,8 +32,9 @@ class ProjectFeature < ActiveRecord::Base
   default_value_for :wiki_access_level,           value: ENABLED, allows_nil: false
 
   def feature_available?(feature, user)
-    access_level = public_send(ProjectFeature.access_level_attribute(feature))
-    get_permission(user, access_level)
+    raise ArgumentError, 'invalid project feature' unless FEATURES.include?(feature)
+
+    get_permission(user, public_send("#{feature}_access_level"))
   end
 
   def builds_enabled?

spec/features/groups/issues_spec.rb

-require 'spec_helper'
-
-feature 'Group issues page', feature: true do
-  let(:path) { issues_group_path(group) }
-  let(:issuable) { create(:issue, project: project, title: "this is my created issuable")}
-
-  include_examples 'project features apply to issuables', Issue
-end

spec/features/groups/merge_requests_spec.rb

-require 'spec_helper'
-
-feature 'Group merge requests page', feature: true do
-  let(:path) { merge_requests_group_path(group) }
-  let(:issuable) { create(:merge_request, source_project: project, target_project: project, title: "this is my created issuable")}
-
-  include_examples 'project features apply to issuables', MergeRequest
-end

spec/models/concerns/issuable_spec.rb

@@ -97,11 +97,6 @@ describe Issue, "Issuable" do
     end
   end
 
-  describe '.to_ability_name' do
-    it { expect(Issue.to_ability_name).to eq("issue") }
-    it { expect(MergeRequest.to_ability_name).to eq("merge_request") }
-  end
-
   describe "#today?" do
     it "returns true when created today" do
       # Avoid timezone differences and just return exactly what we want

spec/support/project_features_apply_to_issuables_shared_examples.rb

-shared_examples 'project features apply to issuables' do |klass|
-  let(:described_class) { klass }
-
-  let(:group) { create(:group) }
-  let(:user_in_group) { create(:group_member, :developer, user: create(:user), group: group ).user }
-  let(:user_outside_group) { create(:user) }
-
-  let(:project) { create(:empty_project, :public, project_args) }
-
-  def project_args
-    feature = "#{described_class.model_name.plural}_access_level".to_sym
-
-    args = { group: group }
-    args[feature] = access_level
-
-    args
-  end
-
-  before do
-    _ = issuable
-    login_as(user)
-    visit path
-  end
-
-  context 'public access level' do
-    let(:access_level) { ProjectFeature::ENABLED }
-
-    context 'group member' do
-      let(:user) { user_in_group }
-
-      it { expect(page).to have_content(issuable.title) }
-    end
-
-    context 'non-member' do
-      let(:user) { user_outside_group }
-
-      it { expect(page).to have_content(issuable.title) }
-    end
-  end
-
-  context 'private access level' do
-    let(:access_level) { ProjectFeature::PRIVATE }
-
-    context 'group member' do
-      let(:user) { user_in_group }
-
-      it { expect(page).to have_content(issuable.title) }
-    end
-
-    context 'non-member' do
-      let(:user) { user_outside_group }
-
-      it { expect(page).not_to have_content(issuable.title) }
-    end
-  end
-end