Implement review comments from @jameslopez

1. Extract an `admin_or_auditor?` method to clean up multiple uses of
  `user.admin? || user.auditor?`

2. Follow the four phase test rule.

3. Clean up the `project_policy_spec` by using %i for literal symbols

4. A number of other minor improvements.

app/finders/group_projects_finder.rb

@@ -18,7 +18,7 @@ class GroupProjectsFinder < UnionFinder
     projects = []
 
     if current_user
-      if @group.users.include?(current_user) || current_user.admin? || current_user.auditor?
+      if @group.users.include?(current_user) || current_user.admin_or_auditor?
         projects << @group.projects unless only_shared
         projects << @group.shared_projects unless only_owned
       else

app/finders/issues_finder.rb

@@ -33,7 +33,7 @@ class IssuesFinder < IssuableFinder
   def self.not_restricted_by_confidentiality(user)
     return Issue.where('issues.confidential IS NULL OR issues.confidential IS FALSE') if user.blank?
 
-    return Issue.all if user.admin? || user.auditor?
+    return Issue.all if user.admin_or_auditor?
 
     Issue.where('
       issues.confidential IS NULL

app/finders/snippets_finder.rb

@@ -44,7 +44,7 @@ class SnippetsFinder
     snippets = project.snippets.fresh
 
     if current_user
-      include_private = project.team.member?(current_user) || current_user.admin? || current_user.auditor?
+      include_private = project.team.member?(current_user) || current_user.admin_or_auditor?
       by_scope(snippets, scope, include_private)
     else
       snippets.are_public

app/models/project_feature.rb

@@ -83,7 +83,7 @@ class ProjectFeature < ActiveRecord::Base
     when DISABLED
       false
     when PRIVATE
-      user && (project.team.member?(user) || user.admin? || user.auditor?)
+      user && (project.team.member?(user) || user.admin_or_auditor?)
     when ENABLED
       true
     else

app/models/user.rb

@@ -538,6 +538,10 @@ class User < ActiveRecord::Base
     admin
   end
 
+  def admin_or_auditor?
+    admin? || auditor?
+  end
+
   def require_ssh_key?
     keys.count == 0 && Gitlab::ProtocolAccess.allowed?('ssh')
   end

spec/finders/group_projects_finder_spec.rb

@@ -77,7 +77,7 @@ describe GroupProjectsFinder do
   end
 
   describe 'with an admin current user' do
-    let(:current_user) { create(:user, :admin) }
+    let(:current_user) { create(:admin) }
 
     context "only shared" do
       subject { described_class.new(group, only_shared: true).execute(current_user) }

spec/finders/issues_finder_spec.rb

@@ -258,7 +258,7 @@ describe IssuesFinder do
 
   describe '.not_restricted_by_confidentiality' do
     let(:authorized_user) { create(:user) }
-    let(:admin_user) { create(:user, :admin) }
+    let(:admin_user) { create(:admin) }
     let(:auditor_user) { create(:user, :auditor) }
     let(:project) { create(:empty_project, namespace: authorized_user.namespace) }
     let!(:public_issue) { create(:issue, project: project) }

spec/finders/snippets_finder_spec.rb

@@ -15,12 +15,14 @@ describe SnippetsFinder do
 
     it "returns all private and internal snippets" do
       snippets = SnippetsFinder.new.execute(user, filter: :all)
+
       expect(snippets).to include(snippet2, snippet3)
       expect(snippets).not_to include(snippet1)
     end
 
     it "returns all public snippets" do
       snippets = SnippetsFinder.new.execute(nil, filter: :all)
+
       expect(snippets).to include(snippet3)
       expect(snippets).not_to include(snippet1, snippet2)
     end
@@ -46,6 +48,7 @@ describe SnippetsFinder do
 
     it "returns all public and internal snippets" do
       snippets = SnippetsFinder.new.execute(user1, filter: :by_user, user: user)
+
       expect(snippets).to include(snippet2, snippet3)
       expect(snippets).not_to include(snippet1)
     end
@@ -58,23 +61,27 @@ describe SnippetsFinder do
 
     it "returns private snippets" do
       snippets = SnippetsFinder.new.execute(user, filter: :by_user, user: user, scope: "are_private")
+
       expect(snippets).to include(snippet1)
       expect(snippets).not_to include(snippet2, snippet3)
     end
 
     it "returns public snippets" do
       snippets = SnippetsFinder.new.execute(user, filter: :by_user, user: user, scope: "are_public")
+
       expect(snippets).to include(snippet3)
       expect(snippets).not_to include(snippet1, snippet2)
     end
 
     it "returns all snippets" do
       snippets = SnippetsFinder.new.execute(user, filter: :by_user, user: user)
+
       expect(snippets).to include(snippet1, snippet2, snippet3)
     end
 
     it "returns only public snippets if unauthenticated user" do
       snippets = SnippetsFinder.new.execute(nil, filter: :by_user, user: user)
+
       expect(snippets).to include(snippet3)
       expect(snippets).not_to include(snippet2, snippet1)
     end
@@ -89,54 +96,67 @@ describe SnippetsFinder do
 
     it "returns public snippets for unauthorized user" do
       snippets = SnippetsFinder.new.execute(nil, filter: :by_project, project: project1)
+
       expect(snippets).to include(@snippet3)
       expect(snippets).not_to include(@snippet1, @snippet2)
     end
 
     it "returns public and internal snippets for non project members" do
       snippets = SnippetsFinder.new.execute(user, filter: :by_project, project: project1)
+
       expect(snippets).to include(@snippet2, @snippet3)
       expect(snippets).not_to include(@snippet1)
     end
 
     it "returns public snippets for non project members" do
       snippets = SnippetsFinder.new.execute(user, filter: :by_project, project: project1, scope: "are_public")
+
       expect(snippets).to include(@snippet3)
       expect(snippets).not_to include(@snippet1, @snippet2)
     end
 
     it "returns internal snippets for non project members" do
       snippets = SnippetsFinder.new.execute(user, filter: :by_project, project: project1, scope: "are_internal")
+
       expect(snippets).to include(@snippet2)
       expect(snippets).not_to include(@snippet1, @snippet3)
     end
 
     it "does not return private snippets for non project members" do
       snippets = SnippetsFinder.new.execute(user, filter: :by_project, project: project1, scope: "are_private")
+
       expect(snippets).not_to include(@snippet1, @snippet2, @snippet3)
     end
 
     it "returns all snippets for project members" do
       project1.team << [user, :developer]
+
       snippets = SnippetsFinder.new.execute(user, filter: :by_project, project: project1)
+
       expect(snippets).to include(@snippet1, @snippet2, @snippet3)
     end
 
     it "returns private snippets for project members" do
       project1.team << [user, :developer]
+
       snippets = SnippetsFinder.new.execute(user, filter: :by_project, project: project1, scope: "are_private")
+
       expect(snippets).to include(@snippet1)
     end
 
     it "returns all snippets for admin users" do
       user = create(:user, :admin)
+
       snippets = SnippetsFinder.new.execute(user, filter: :by_project, project: project1)
+
       expect(snippets).to include(@snippet1, @snippet2, @snippet3)
     end
 
     it "returns all snippets for auditor users" do
       user = create(:user, :auditor)
+
       snippets = SnippetsFinder.new.execute(user, filter: :by_project, project: project1)
+
       expect(snippets).to include(@snippet1, @snippet2, @snippet3)
     end
   end

spec/policies/group_policy_spec.rb

@@ -177,6 +177,7 @@ describe GroupPolicy, models: true do
 
       it do
         is_expected.to include(:read_group)
+        is_expected.to all(start_with("read"))
         is_expected.not_to include(*master_permissions)
         is_expected.not_to include(*owner_permissions)
       end

spec/policies/namespace_policy_spec.rb

@@ -7,12 +7,7 @@ describe NamespacePolicy, models: true do
   let(:admin) { create(:admin) }
   let(:namespace) { create(:namespace, owner: owner) }
 
-  let(:owner_permissions) do
-    [
-      :create_projects,
-      :admin_namespace
-    ]
-  end
+  let(:owner_permissions) { [:create_projects, :admin_namespace] }
 
   let(:admin_permissions) { owner_permissions }
 
@@ -21,40 +16,30 @@ describe NamespacePolicy, models: true do
   context 'with no user' do
     let(:current_user) { nil }
 
-    it do
-      is_expected.to be_empty
-    end
+    it { is_expected.to be_empty }
   end
 
   context 'regular user' do
     let(:current_user) { user }
 
-    it do
-      is_expected.to be_empty
-    end
+    it { is_expected.to be_empty }
   end
 
   context 'owner' do
     let(:current_user) { owner }
 
-    it do
-      is_expected.to include(*owner_permissions)
-    end
+    it { is_expected.to include(*owner_permissions) }
   end
 
   context 'auditor' do
     let(:current_user) { auditor }
 
-    it do
-      is_expected.to be_empty
-    end
+    it { is_expected.to be_empty }
   end
 
   context 'admin' do
     let(:current_user) { admin }
 
-    it do
-      is_expected.to include(*owner_permissions)
-    end
+    it { is_expected.to include(*owner_permissions) }
   end
 end

spec/policies/project_policy_spec.rb

@@ -11,71 +11,69 @@ describe ProjectPolicy, models: true do
   let(:project) { create(:empty_project, :public, namespace: owner.namespace) }
 
   let(:guest_permissions) do
-    [
-      :read_project, :read_board, :read_list, :read_wiki, :read_issue, :read_label,
-      :read_milestone, :read_project_snippet, :read_project_member,
-      :read_note, :create_project, :create_issue, :create_note,
-      :upload_file
+    %i[
+      read_project read_board read_list read_wiki read_issue read_label
+      read_milestone read_project_snippet read_project_member
+      read_note create_project create_issue create_note
+      upload_file
     ]
   end
 
   let(:reporter_permissions) do
-    [
-      :download_code, :fork_project, :create_project_snippet, :update_issue,
-      :admin_issue, :admin_label, :admin_list, :read_commit_status, :read_build,
-      :read_container_image, :read_pipeline, :read_environment, :read_deployment,
-      :read_merge_request, :download_wiki_code
+    %i[
+      download_code fork_project create_project_snippet update_issue
+      admin_issue admin_label admin_list read_commit_status read_build
+      read_container_image read_pipeline read_environment read_deployment
+      read_merge_request download_wiki_code
     ]
   end
 
   let(:team_member_reporter_permissions) do
-    [
-      :build_download_code, :build_read_container_image
-    ]
+    %i[build_download_code build_read_container_image]
   end
 
   let(:developer_permissions) do
-    [
-      :admin_merge_request, :update_merge_request, :create_commit_status,
-      :update_commit_status, :create_build, :update_build, :create_pipeline,
-      :update_pipeline, :create_merge_request, :create_wiki, :push_code,
-      :resolve_note, :create_container_image, :update_container_image,
-      :create_environment, :create_deployment
+    %i[
+      admin_merge_request update_merge_request create_commit_status
+      update_commit_status create_build update_build create_pipeline
+      update_pipeline create_merge_request create_wiki push_code
+      resolve_note create_container_image update_container_image
+      create_environment create_deployment
     ]
   end
 
   let(:master_permissions) do
-    [
-      :push_code_to_protected_branches, :update_project_snippet, :update_environment,
-      :update_deployment, :admin_milestone, :admin_project_snippet,
-      :admin_project_member, :admin_note, :admin_wiki, :admin_project,
-      :admin_commit_status, :admin_build, :admin_container_image,
-      :admin_pipeline, :admin_environment, :admin_deployment
+    %i[
+      push_code_to_protected_branches update_project_snippet update_environment
+      update_deployment admin_milestone admin_project_snippet
+      admin_project_member admin_note admin_wiki admin_project
+      admin_commit_status admin_build admin_container_image
+      admin_pipeline admin_environment admin_deployment
     ]
   end
 
   let(:public_permissions) do
-    [
-      :download_code, :fork_project, :read_commit_status, :read_pipeline,
-      :read_container_image, :build_download_code, :build_read_container_image,
-      :download_wiki_code
+    %i[
+      download_code fork_project read_commit_status read_pipeline
+      read_container_image build_download_code build_read_container_image
+      download_wiki_code
     ]
   end
 
   let(:owner_permissions) do
-    [
-      :change_namespace, :change_visibility_level, :rename_project, :remove_project,
-      :archive_project, :remove_fork_project, :destroy_merge_request, :destroy_issue
+    %i[
+      change_namespace change_visibility_level rename_project remove_project
+      archive_project remove_fork_project destroy_merge_request destroy_issue
     ]
   end
 
   let(:auditor_permissions) do
-    [
-      :download_code, :download_wiki_code, :read_project, :read_board, :read_list,
-      :read_wiki, :read_issue, :read_label, :read_milestone, :read_project_snippet,
-      :read_project_member, :read_note, :read_cycle_analytics, :read_pipeline,
-      :read_build, :read_commit_status, :read_container_image, :read_environment,
-      :read_deployment, :read_merge_request, :read_pages
+    %i[
+      download_code download_wiki_code read_project read_board read_list
+      read_wiki read_issue read_label read_milestone read_project_snippet
+      read_project_member read_note read_cycle_analytics read_pipeline
+      read_build read_commit_status read_container_image read_environment
+      read_deployment read_merge_request read_pages
     ]
   end
 

spec/policies/project_snippet_policy_spec.rb

 require 'spec_helper'
 
 describe ProjectSnippetPolicy, models: true do
+  let(:current_user) { create(:user) }
+
   let(:author_permissions) do
     [
       :update_project_snippet,
@@ -23,8 +25,6 @@ describe ProjectSnippetPolicy, models: true do
     end
 
     context 'regular user' do
-      let(:current_user) { create(:user) }
-
       it do
         is_expected.to include(:read_project_snippet)
         is_expected.not_to include(*author_permissions)
@@ -45,8 +45,6 @@ describe ProjectSnippetPolicy, models: true do
     end
 
     context 'regular user' do
-      let(:current_user) { create(:user) }
-
       it do
         is_expected.to include(:read_project_snippet)
         is_expected.not_to include(*author_permissions)
@@ -76,8 +74,6 @@ describe ProjectSnippetPolicy, models: true do
     end
 
     context 'regular user' do
-      let(:current_user) { create(:user) }
-
       it do
         is_expected.not_to include(:read_project_snippet)
         is_expected.not_to include(*author_permissions)
@@ -85,7 +81,6 @@ describe ProjectSnippetPolicy, models: true do
     end
 
     context 'snippet author' do
-      let(:current_user) { create(:user) }
       let(:project_snippet) { create(:project_snippet, :private, author: current_user) }
 
       it do
@@ -95,7 +90,6 @@ describe ProjectSnippetPolicy, models: true do
     end
 
     context 'project team member' do
-      let(:current_user) { create(:user) }
       before { project_snippet.project.team << [current_user, :developer] }
 
       it do
@@ -114,7 +108,7 @@ describe ProjectSnippetPolicy, models: true do
     end
 
     context 'admin user' do
-      let(:current_user) { create(:user, :admin) }
+      let(:current_user) { create(:admin) }
 
       it do
         is_expected.to include(:read_project_snippet)