Add latest changes from gitlab-org/gitlab@master

4 files changed, 67 insertions, 38 deletions

diff --git a/spec/controllers/snippets/notes_controller_spec.rb b/spec/controllers/snippets/notes_controller_spec.rb
index b93df3555ab..0676ed05212 100644
--- a/spec/controllers/snippets/notes_controller_spec.rb
+++ b/spec/controllers/snippets/notes_controller_spec.rb
@@ -108,7 +108,7 @@ describe Snippets::NotesController do
 
         sign_in(user)
 
-        expect_any_instance_of(Note).to receive(:cross_reference_not_visible_for?).and_return(true)
+        expect_any_instance_of(Note).to receive(:readable_by?).and_return(false)
       end
 
       it "does not return any note" do
diff --git a/spec/models/ci/job_artifact_spec.rb b/spec/models/ci/job_artifact_spec.rb
index d2fe0d7eeca..8f56d735f36 100644
--- a/spec/models/ci/job_artifact_spec.rb
+++ b/spec/models/ci/job_artifact_spec.rb
@@ -113,13 +113,14 @@ describe Ci::JobArtifact do
 
   describe '.for_sha' do
     it 'returns job artifacts for a given pipeline sha' do
-      first_pipeline = create(:ci_pipeline)
-      second_pipeline = create(:ci_pipeline, sha: Digest::SHA1.hexdigest(SecureRandom.hex))
+      project = create(:project)
+      first_pipeline = create(:ci_pipeline, project: project)
+      second_pipeline = create(:ci_pipeline, project: project, sha: Digest::SHA1.hexdigest(SecureRandom.hex))
       first_artifact = create(:ci_job_artifact, job: create(:ci_build, pipeline: first_pipeline))
       second_artifact = create(:ci_job_artifact, job: create(:ci_build, pipeline: second_pipeline))
 
-      expect(described_class.for_sha(first_pipeline.sha)).to eq([first_artifact])
-      expect(described_class.for_sha(second_pipeline.sha)).to eq([second_artifact])
+      expect(described_class.for_sha(first_pipeline.sha, project.id)).to eq([first_artifact])
+      expect(described_class.for_sha(second_pipeline.sha, project.id)).to eq([second_artifact])
     end
   end
 
diff --git a/spec/models/note_spec.rb b/spec/models/note_spec.rb
index a50608a17b6..4da23c79944 100644
--- a/spec/models/note_spec.rb
+++ b/spec/models/note_spec.rb
@@ -285,28 +285,37 @@ describe Note do
     end
   end
 
-  describe "#visible_for?" do
-    using RSpec::Parameterized::TableSyntax
+  describe "#system_note_with_references_visible_for?" do
+    let(:project) { create(:project, :public) }
+    let(:user) { create(:user) }
+    let(:guest) { create(:project_member, :guest, project: project, user: create(:user)).user }
+    let(:reporter) { create(:project_member, :reporter, project: project, user: create(:user)).user }
+    let(:maintainer) { create(:project_member, :maintainer, project: project, user: create(:user)).user }
+    let(:non_member) { create(:user) }
 
-    let_it_be(:note) { create(:note) }
-    let_it_be(:user) { create(:user) }
+    let(:note) { create(:note, project: project) }
 
-    where(:cross_reference_visible, :system_note_viewable, :result) do
-      true  | true  | false
-      false | true  | true
-      false | false | false
+    context 'when project is public' do
+      it_behaves_like 'users with note access' do
+        let(:users) { [reporter, maintainer, guest, non_member, nil] }
+      end
     end
 
-    with_them do
-      it "returns expected result" do
-        expect(note).to receive(:cross_reference_not_visible_for?).and_return(cross_reference_visible)
+    context 'when group is private' do
+      let(:project) { create(:project, :private) }
 
-        unless cross_reference_visible
-          expect(note).to receive(:system_note_viewable_by?)
-            .with(user).and_return(system_note_viewable)
-        end
+      it_behaves_like 'users with note access' do
+        let(:users) { [reporter, maintainer, guest] }
+      end
+
+      it 'returns visible but not readable for non-member user' do
+        expect(note.system_note_with_references_visible_for?(non_member)).to be_truthy
+        expect(note.readable_by?(non_member)).to be_falsy
+      end
 
-        expect(note.visible_for?(user)).to eq result
+      it 'returns visible but not readable for a nil user' do
+        expect(note.system_note_with_references_visible_for?(nil)).to be_truthy
+        expect(note.readable_by?(nil)).to be_falsy
       end
     end
   end
@@ -349,7 +358,7 @@ describe Note do
     end
   end
 
-  describe "cross_reference_not_visible_for?" do
+  describe "system_note_with_references_visible_for?" do
     let_it_be(:private_user)    { create(:user) }
     let_it_be(:private_project) { create(:project, namespace: private_user.namespace) { |p| p.add_maintainer(private_user) } }
     let_it_be(:private_issue)   { create(:issue, project: private_project) }
@@ -359,11 +368,11 @@ describe Note do
 
     shared_examples "checks references" do
       it "returns true" do
-        expect(note.cross_reference_not_visible_for?(ext_issue.author)).to be_truthy
+        expect(note.system_note_with_references_visible_for?(ext_issue.author)).to be_falsy
       end
 
       it "returns false" do
-        expect(note.cross_reference_not_visible_for?(private_user)).to be_falsy
+        expect(note.system_note_with_references_visible_for?(private_user)).to be_truthy
       end
 
       it "returns false if user visible reference count set" do
@@ -371,14 +380,14 @@ describe Note do
         note.total_reference_count = 1
 
         expect(note).not_to receive(:reference_mentionables)
-        expect(note.cross_reference_not_visible_for?(ext_issue.author)).to be_falsy
+        expect(note.system_note_with_references_visible_for?(ext_issue.author)).to be_truthy
       end
 
       it "returns true if ref count is 0" do
         note.user_visible_reference_count = 0
 
         expect(note).not_to receive(:reference_mentionables)
-        expect(note.cross_reference_not_visible_for?(ext_issue.author)).to be_truthy
+        expect(note.system_note_with_references_visible_for?(ext_issue.author)).to be_falsy
       end
     end
 
@@ -423,16 +432,16 @@ describe Note do
         note.total_reference_count = 2
 
         expect(note).not_to receive(:reference_mentionables)
-        expect(note.cross_reference_not_visible_for?(ext_issue.author)).to be_truthy
+        expect(note.system_note_with_references_visible_for?(ext_issue.author)).to be_falsy
       end
     end
   end
 
-  describe '#cross_reference?' do
+  describe '#system_note_with_references?' do
     it 'falsey for user-generated notes' do
       note = create(:note, system: false)
 
-      expect(note.cross_reference?).to be_falsy
+      expect(note.system_note_with_references?).to be_falsy
     end
 
     context 'when the note might contain cross references' do
@@ -443,7 +452,7 @@ describe Note do
         it 'delegates to the cross-reference regex' do
           expect(note).to receive(:matches_cross_reference_regex?).and_return(false)
 
-          note.cross_reference?
+          note.system_note_with_references?
         end
       end
     end
@@ -453,8 +462,8 @@ describe Note do
       let(:label_note) { build(:note, note: 'added ~2323232323', system: true) }
 
       it 'scan for a `mentioned in` prefix' do
-        expect(commit_note.cross_reference?).to be_truthy
-        expect(label_note.cross_reference?).to be_falsy
+        expect(commit_note.system_note_with_references?).to be_truthy
+        expect(label_note.system_note_with_references?).to be_falsy
       end
     end
 
@@ -468,7 +477,7 @@ describe Note do
       it 'delegates to the system note service' do
         expect(SystemNotes::IssuablesService).to receive(:cross_reference?).with(note.note)
 
-        note.cross_reference?
+        note.system_note_with_references?
       end
     end
 
@@ -480,7 +489,7 @@ describe Note do
         it 'delegates to the cross-reference regex' do
           expect(note).to receive(:matches_cross_reference_regex?)
 
-          note.cross_reference?
+          note.system_note_with_references?
         end
       end
 
@@ -489,13 +498,13 @@ describe Note do
 
         it_behaves_like 'system_note_metadata includes note action'
 
-        it { expect(note.cross_reference?).to be_falsy }
+        it { expect(note.system_note_with_references?).to be_falsy }
 
         context 'with cross reference label note' do
           let(:label) { create(:label, project: issue.project)}
           let(:note) { create(:system_note, note: "added #{label.to_reference} label", noteable: issue, project: issue.project) }
 
-          it { expect(note.cross_reference?).to be_truthy }
+          it { expect(note.system_note_with_references?).to be_truthy }
         end
       end
 
@@ -504,13 +513,13 @@ describe Note do
 
         it_behaves_like 'system_note_metadata includes note action'
 
-        it { expect(note.cross_reference?).to be_falsy }
+        it { expect(note.system_note_with_references?).to be_falsy }
 
         context 'with cross reference milestone note' do
           let(:milestone) { create(:milestone, project: issue.project)}
           let(:note) { create(:system_note, note: "added #{milestone.to_reference} milestone", noteable: issue, project: issue.project) }
 
-          it { expect(note.cross_reference?).to be_truthy }
+          it { expect(note.system_note_with_references?).to be_truthy }
         end
       end
     end
diff --git a/spec/support/shared_examples/models/note_access_check_shared_examples.rb b/spec/support/shared_examples/models/note_access_check_shared_examples.rb
new file mode 100644
index 00000000000..3bafad202f6
--- /dev/null
+++ b/spec/support/shared_examples/models/note_access_check_shared_examples.rb
@@ -0,0 +1,19 @@
+# frozen_string_literal: true
+
+shared_examples 'users with note access' do
+  it 'returns true' do
+    users.each do |user|
+      expect(note.system_note_with_references_visible_for?(user)).to be_truthy
+      expect(note.readable_by?(user)).to be_truthy
+    end
+  end
+end
+
+shared_examples 'users without note access' do
+  it 'returns false' do
+    users.each do |user|
+      expect(note.system_note_with_references_visible_for?(user)).to be_falsy
+      expect(note.readable_by?(user)).to be_falsy
+    end
+  end
+end