From d47fca53db5840e85d38b35058d4bbd94ea917db Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?R=C3=A9my=20Coutable?= <remy@rymai.me>
Date: Wed, 16 Nov 2016 15:17:09 +0100
Subject: [PATCH] Allow commit note to be visible if repo is visible
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

Signed-off-by: Rémy Coutable <remy@rymai.me>
---
 app/helpers/events_helper.rb                  |  4 +--
 app/models/event.rb                           |  6 ++---
 ...ty-page-does-not-show-commits-comments.yml |  4 +++
 spec/models/event_spec.rb                     | 27 +++++++++++++++++++
 4 files changed, 36 insertions(+), 5 deletions(-)
 create mode 100644 changelogs/unreleased/23824-activity-page-does-not-show-commits-comments.yml

diff --git a/app/helpers/events_helper.rb b/app/helpers/events_helper.rb
index 00e64076408..f1a0b929d82 100644
--- a/app/helpers/events_helper.rb
+++ b/app/helpers/events_helper.rb
@@ -86,7 +86,7 @@ module EventsHelper
     elsif event.merge_request?
       namespace_project_merge_request_url(event.project.namespace,
                                           event.project, event.merge_request)
-    elsif event.note? && event.commit_note?
+    elsif event.commit_note?
       namespace_project_commit_url(event.project.namespace, event.project,
                                    event.note_target)
     elsif event.note?
@@ -127,7 +127,7 @@ module EventsHelper
   end
 
   def event_note_target_path(event)
-    if event.note? && event.commit_note?
+    if event.commit_note?
       namespace_project_commit_path(event.project.namespace,
                                     event.project,
                                     event.note_target,
diff --git a/app/models/event.rb b/app/models/event.rb
index c76d88b1c7b..21eaca917b8 100644
--- a/app/models/event.rb
+++ b/app/models/event.rb
@@ -62,7 +62,7 @@ class Event < ActiveRecord::Base
   end
 
   def visible_to_user?(user = nil)
-    if push?
+    if push? || commit_note?
       Ability.allowed?(user, :download_code, project)
     elsif membership_changed?
       true
@@ -283,7 +283,7 @@ class Event < ActiveRecord::Base
   end
 
   def commit_note?
-    target.for_commit?
+    note? && target && target.for_commit?
   end
 
   def issue_note?
@@ -295,7 +295,7 @@ class Event < ActiveRecord::Base
   end
 
   def project_snippet_note?
-    target.for_snippet?
+    note? && target && target.for_snippet?
   end
 
   def note_target
diff --git a/changelogs/unreleased/23824-activity-page-does-not-show-commits-comments.yml b/changelogs/unreleased/23824-activity-page-does-not-show-commits-comments.yml
new file mode 100644
index 00000000000..48f733f9c5e
--- /dev/null
+++ b/changelogs/unreleased/23824-activity-page-does-not-show-commits-comments.yml
@@ -0,0 +1,4 @@
+---
+title: Allow commit note to be visible if repo is visible
+merge_request: 
+author: 
diff --git a/spec/models/event_spec.rb b/spec/models/event_spec.rb
index 29a3af68a9b..b684053cd02 100644
--- a/spec/models/event_spec.rb
+++ b/spec/models/event_spec.rb
@@ -94,6 +94,7 @@ describe Event, models: true do
     let(:admin) { create(:admin) }
     let(:issue) { create(:issue, project: project, author: author, assignee: assignee) }
     let(:confidential_issue) { create(:issue, :confidential, project: project, author: author, assignee: assignee) }
+    let(:note_on_commit) { create(:note_on_commit, project: project) }
     let(:note_on_issue) { create(:note_on_issue, noteable: issue, project: project) }
     let(:note_on_confidential_issue) { create(:note_on_issue, noteable: confidential_issue, project: project) }
     let(:event) { Event.new(project: project, target: target, author_id: author.id) }
@@ -103,6 +104,32 @@ describe Event, models: true do
       project.team << [guest, :guest]
     end
 
+    context 'commit note event' do
+      let(:target) { note_on_commit }
+
+      it do
+        aggregate_failures do
+          expect(event.visible_to_user?(non_member)).to eq true
+          expect(event.visible_to_user?(member)).to eq true
+          expect(event.visible_to_user?(guest)).to eq true
+          expect(event.visible_to_user?(admin)).to eq true
+        end
+      end
+
+      context 'private project' do
+        let(:project) { create(:empty_project, :private) }
+
+        it do
+          aggregate_failures do
+            expect(event.visible_to_user?(non_member)).to eq false
+            expect(event.visible_to_user?(member)).to eq true
+            expect(event.visible_to_user?(guest)).to eq false
+            expect(event.visible_to_user?(admin)).to eq true
+          end
+        end
+      end
+    end
+
     context 'issue event' do
       context 'for non confidential issues' do
         let(:target) { issue }