Commit 5042f008 authored by Douwe Maan's avatar Douwe Maan

Merge branch 'cleanup-mentionable-methods-arguments' into 'master'

Simplify Mentionable concern instance methods

## What does this MR do?

Simplify arguments received by the instance methods on the concern so in the closer future will be easy to understand and change

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

## Why was this MR needed?

## Screenshots (if relevant)

## Does this MR meet the acceptance criteria?

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

## What are the relevant issue numbers?

See merge request !6596
parents ef24c625 a6e17cf5
...@@ -11,6 +11,7 @@ v 8.13.0 (unreleased) ...@@ -11,6 +11,7 @@ v 8.13.0 (unreleased)
- Log LDAP lookup errors and don't swallow unrelated exceptions. !6103 (Markus Koller) - Log LDAP lookup errors and don't swallow unrelated exceptions. !6103 (Markus Koller)
- Add more tests for calendar contribution (ClemMakesApps) - Add more tests for calendar contribution (ClemMakesApps)
- Avoid database queries on Banzai::ReferenceParser::BaseParser for nodes without references - Avoid database queries on Banzai::ReferenceParser::BaseParser for nodes without references
- Simplify Mentionable concern instance methods
- Fix permission for setting an issue's due date - Fix permission for setting an issue's due date
- Expose expires_at field when sharing project on API - Expose expires_at field when sharing project on API
- Fix issue with page scrolling to top when closing or pinning sidebar (lukehowell) - Fix issue with page scrolling to top when closing or pinning sidebar (lukehowell)
......
...@@ -43,20 +43,16 @@ module Mentionable ...@@ -43,20 +43,16 @@ module Mentionable
self self
end end
def all_references(current_user = nil, text = nil, extractor: nil) def all_references(current_user = nil, extractor: nil)
extractor ||= Gitlab::ReferenceExtractor. extractor ||= Gitlab::ReferenceExtractor.
new(project, current_user) new(project, current_user)
if text
extractor.analyze(text, author: author)
else
self.class.mentionable_attrs.each do |attr, options| self.class.mentionable_attrs.each do |attr, options|
text = __send__(attr) text = __send__(attr)
options = options.merge(cache_key: [self, attr], author: author) options = options.merge(cache_key: [self, attr], author: author)
extractor.analyze(text, options) extractor.analyze(text, options)
end end
end
extractor extractor
end end
...@@ -66,8 +62,8 @@ module Mentionable ...@@ -66,8 +62,8 @@ module Mentionable
end end
# Extract GFM references to other Mentionables from this Mentionable. Always excludes its #local_reference. # Extract GFM references to other Mentionables from this Mentionable. Always excludes its #local_reference.
def referenced_mentionables(current_user = self.author, text = nil) def referenced_mentionables(current_user = self.author)
refs = all_references(current_user, text) refs = all_references(current_user)
refs = (refs.issues + refs.merge_requests + refs.commits) refs = (refs.issues + refs.merge_requests + refs.commits)
# We're using this method instead of Array diffing because that requires # We're using this method instead of Array diffing because that requires
...@@ -77,8 +73,8 @@ module Mentionable ...@@ -77,8 +73,8 @@ module Mentionable
end end
# Create a cross-reference Note for each GFM reference to another Mentionable found in the +mentionable_attrs+. # Create a cross-reference Note for each GFM reference to another Mentionable found in the +mentionable_attrs+.
def create_cross_references!(author = self.author, without = [], text = nil) def create_cross_references!(author = self.author, without = [])
refs = referenced_mentionables(author, text) refs = referenced_mentionables(author)
# We're using this method instead of Array diffing because that requires # We're using this method instead of Array diffing because that requires
# both of the object's `hash` values to be the same, which may not be the # both of the object's `hash` values to be the same, which may not be the
...@@ -97,10 +93,7 @@ module Mentionable ...@@ -97,10 +93,7 @@ module Mentionable
return if changes.empty? return if changes.empty?
original_text = changes.collect { |_, vals| vals.first }.join(' ') create_cross_references!(author)
preexisting = referenced_mentionables(author, original_text)
create_cross_references!(author, preexisting)
end end
private private
......
...@@ -347,7 +347,7 @@ module SystemNoteService ...@@ -347,7 +347,7 @@ module SystemNoteService
notes = notes.where(noteable_id: noteable.id) notes = notes.where(noteable_id: noteable.id)
end end
notes_for_mentioner(mentioner, noteable, notes).count > 0 notes_for_mentioner(mentioner, noteable, notes).exists?
end end
# Build an Array of lines detailing each commit added in a merge request # Build an Array of lines detailing each commit added in a merge request
......
require 'spec_helper' require 'spec_helper'
describe Mentionable do describe Mentionable do
class Example
include Mentionable include Mentionable
attr_accessor :project, :message
attr_mentionable :message
def author def author
nil nil
end end
end
describe 'references' do describe 'references' do
let(:project) { create(:project) } let(:project) { create(:project) }
let(:mentionable) { Example.new }
it 'excludes JIRA references' do it 'excludes JIRA references' do
allow(project).to receive_messages(jira_tracker?: true) allow(project).to receive_messages(jira_tracker?: true)
expect(referenced_mentionables(project, 'JIRA-123')).to be_empty
mentionable.project = project
mentionable.message = 'JIRA-123'
expect(mentionable.referenced_mentionables).to be_empty
end end
end end
end end
...@@ -39,9 +48,8 @@ describe Issue, "Mentionable" do ...@@ -39,9 +48,8 @@ describe Issue, "Mentionable" do
let(:user) { create(:user) } let(:user) { create(:user) }
def referenced_issues(current_user) def referenced_issues(current_user)
text = "#{private_issue.to_reference(project)} and #{public_issue.to_reference}" issue.title = "#{private_issue.to_reference(project)} and #{public_issue.to_reference}"
issue.referenced_mentionables(current_user)
issue.referenced_mentionables(current_user, text)
end end
context 'when the current user can see the issue' do context 'when the current user can see the issue' do
......
...@@ -522,7 +522,7 @@ describe MergeRequest, models: true do ...@@ -522,7 +522,7 @@ describe MergeRequest, models: true do
end end
it_behaves_like 'an editable mentionable' do it_behaves_like 'an editable mentionable' do
subject { create(:merge_request) } subject { create(:merge_request, :simple) }
let(:backref_text) { "merge request #{subject.to_reference}" } let(:backref_text) { "merge request #{subject.to_reference}" }
let(:set_mentionable_text) { ->(txt){ subject.description = txt } } let(:set_mentionable_text) { ->(txt){ subject.description = txt } }
......
...@@ -9,7 +9,7 @@ shared_context 'mentionable context' do ...@@ -9,7 +9,7 @@ shared_context 'mentionable context' do
let(:author) { subject.author } let(:author) { subject.author }
let(:mentioned_issue) { create(:issue, project: project) } let(:mentioned_issue) { create(:issue, project: project) }
let!(:mentioned_mr) { create(:merge_request, :simple, source_project: project) } let!(:mentioned_mr) { create(:merge_request, source_project: project) }
let(:mentioned_commit) { project.commit("HEAD~1") } let(:mentioned_commit) { project.commit("HEAD~1") }
let(:ext_proj) { create(:project, :public) } let(:ext_proj) { create(:project, :public) }
...@@ -100,6 +100,7 @@ shared_examples 'an editable mentionable' do ...@@ -100,6 +100,7 @@ shared_examples 'an editable mentionable' do
it 'creates new cross-reference notes when the mentionable text is edited' do it 'creates new cross-reference notes when the mentionable text is edited' do
subject.save subject.save
subject.create_cross_references!
new_text = <<-MSG.strip_heredoc new_text = <<-MSG.strip_heredoc
These references already existed: These references already existed:
...@@ -131,6 +132,7 @@ shared_examples 'an editable mentionable' do ...@@ -131,6 +132,7 @@ shared_examples 'an editable mentionable' do
end end
# These two issues are new and should receive reference notes # These two issues are new and should receive reference notes
# In the case of MergeRequests remember that cannot mention commits included in the MergeRequest
new_issues.each do |newref| new_issues.each do |newref|
expect(SystemNoteService).to receive(:cross_reference). expect(SystemNoteService).to receive(:cross_reference).
with(newref, subject.local_reference, author) with(newref, subject.local_reference, author)
......
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