Commit 98090639 authored by Douwe Maan's avatar Douwe Maan Committed by Robert Speicher

Merge branch '19092-fix-event-for-legacydiffnote-not-considered-note' into 'master'

Fix diff comments not showing up in activity feed

## What does this MR do?

It fixes the detection of note events to check for `Note` and `LegacyDiffNote`.

## Are there points in the code the reviewer needs to double check?

No? /cc @DouweM (since I believe you introduced `LegacyDiffNote`

## Why was this MR needed?

To fix #19092.

## What are the relevant issue numbers?

Fixes #19092.

## Does this MR meet the acceptance criteria?

- [x] [CHANGELOG](https://gitlab.com/gitlab-org/gitlab-ce/blob/master/CHANGELOG) entry added
- Tests
  - [x] Added for this feature/bug
  - [ ] All builds are passing
- [x] Conform by the [style guides](https://gitlab.com/gitlab-org/gitlab-ce/blob/master/CONTRIBUTING.md#style-guides)
- [x] Branch has no merge conflicts with `master` (if you do - rebase it please)
- [x] [Squashed related commits together](https://git-scm.com/book/en/Git-Tools-Rewriting-History#Squashing-Commits)

See merge request !5069
(cherry picked from commit b5b17d00)
parent 3dee2366
Please view this file on the master branch, on stable branches it's out of date. Please view this file on the master branch, on stable branches it's out of date.
v 8.9.5 v 8.9.5
- Fix diff comments not showing up in activity feed. !5069
- Add index on both Award Emoji user and name. !5061 - Add index on both Award Emoji user and name. !5061
- Downgrade to Redis 3.2.2 due to massive memory leak with Sidekiq. !5056 - Downgrade to Redis 3.2.2 due to massive memory leak with Sidekiq. !5056
- Re-enable import button when import process fails due to namespace already being taken. !5053 - Re-enable import button when import process fails due to namespace already being taken. !5053
......
...@@ -67,7 +67,7 @@ class Event < ActiveRecord::Base ...@@ -67,7 +67,7 @@ class Event < ActiveRecord::Base
elsif issue? || issue_note? elsif issue? || issue_note?
Ability.abilities.allowed?(user, :read_issue, note? ? note_target : target) Ability.abilities.allowed?(user, :read_issue, note? ? note_target : target)
else else
((merge_request? || note?) && target) || milestone? ((merge_request? || note?) && target.present?) || milestone?
end end
end end
...@@ -136,7 +136,7 @@ class Event < ActiveRecord::Base ...@@ -136,7 +136,7 @@ class Event < ActiveRecord::Base
end end
def note? def note?
target_type == "Note" target.is_a?(Note)
end end
def issue? def issue?
......
...@@ -46,6 +46,22 @@ describe Event, models: true do ...@@ -46,6 +46,22 @@ describe Event, models: true do
it { expect(@event.author).to eq(@user) } it { expect(@event.author).to eq(@user) }
end end
describe '#note?' do
subject { Event.new(project: target.project, target: target) }
context 'issue note event' do
let(:target) { create(:note_on_issue) }
it { is_expected.to be_note }
end
context 'merge request diff note event' do
let(:target) { create(:note_on_merge_request_diff) }
it { is_expected.to be_note }
end
end
describe '#visible_to_user?' do describe '#visible_to_user?' do
let(:project) { create(:empty_project, :public) } let(:project) { create(:empty_project, :public) }
let(:non_member) { create(:user) } let(:non_member) { create(:user) }
...@@ -89,7 +105,7 @@ describe Event, models: true do ...@@ -89,7 +105,7 @@ describe Event, models: true do
end end
end end
context 'note event' do context 'issue note event' do
context 'on non confidential issues' do context 'on non confidential issues' do
let(:target) { note_on_issue } let(:target) { note_on_issue }
...@@ -112,6 +128,20 @@ describe Event, models: true do ...@@ -112,6 +128,20 @@ describe Event, models: true do
it { expect(event.visible_to_user?(admin)).to eq true } it { expect(event.visible_to_user?(admin)).to eq true }
end end
end end
context 'merge request diff note event' do
let(:project) { create(:project, :public) }
let(:merge_request) { create(:merge_request, source_project: project, author: author, assignee: assignee) }
let(:note_on_merge_request) { create(:note_on_merge_request_diff, noteable: merge_request, project: project) }
let(:target) { note_on_merge_request }
it { expect(event.visible_to_user?(non_member)).to eq true }
it { expect(event.visible_to_user?(author)).to eq true }
it { expect(event.visible_to_user?(assignee)).to eq true }
it { expect(event.visible_to_user?(member)).to eq true }
it { expect(event.visible_to_user?(guest)).to eq true }
it { expect(event.visible_to_user?(admin)).to eq true }
end
end end
describe '.limit_recent' do describe '.limit_recent' do
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment