From 877aa5458627d42d03a7f204d02db9d326af006c Mon Sep 17 00:00:00 2001 From: Robert Speicher Date: Sat, 1 Sep 2012 22:56:54 -0400 Subject: [PATCH 1/4] Rename gitlab_flavored_markdown_spec to gitlab_markdown_helper_spec --- ...b_flavored_markdown_spec.rb => gitlab_markdown_helper_spec.rb} | 0 1 file changed, 0 insertions(+), 0 deletions(-) rename spec/helpers/{gitlab_flavored_markdown_spec.rb => gitlab_markdown_helper_spec.rb} (100%) diff --git a/spec/helpers/gitlab_flavored_markdown_spec.rb b/spec/helpers/gitlab_markdown_helper_spec.rb similarity index 100% rename from spec/helpers/gitlab_flavored_markdown_spec.rb rename to spec/helpers/gitlab_markdown_helper_spec.rb From 8db2a59d0b6959a78ea7be4663dd9a858dff9f98 Mon Sep 17 00:00:00 2001 From: Robert Speicher Date: Sat, 1 Sep 2012 23:39:28 -0400 Subject: [PATCH 2/4] Add StaticModel role, and add it to Commit model Instead of doing this: link_to(commit.id, project_commit_path(project, id: commit.id)) Note.create(noteable_id: commit.id, noteable_type: "Commit", ...) It lets us do this: link_to(commit.id, project_commit_path(project, commit)) Note.create(noteable: commit, ...) --- app/models/commit.rb | 12 ++++-------- app/roles/static_model.rb | 35 +++++++++++++++++++++++++++++++++++ 2 files changed, 39 insertions(+), 8 deletions(-) create mode 100644 app/roles/static_model.rb diff --git a/app/models/commit.rb b/app/models/commit.rb index 5c6b4d88..15afedcb 100644 --- a/app/models/commit.rb +++ b/app/models/commit.rb @@ -1,6 +1,7 @@ class Commit include ActiveModel::Conversion include Gitlab::Encode + include StaticModel extend ActiveModel::Naming attr_accessor :commit @@ -22,8 +23,7 @@ class Commit :to_patch, to: :commit - - class << self + class << self def find_or_first(repo, commit_id = nil, root_ref) commit = if commit_id repo.commit(commit_id) @@ -85,7 +85,7 @@ class Commit first = project.commit(to.try(:strip)) last = project.commit(from.try(:strip)) - result = { + result = { commits: [], diffs: [], commit: nil @@ -105,10 +105,6 @@ class Commit end end - def persisted? - false - end - def initialize(raw_commit, head = nil) @commit = raw_commit @head = head @@ -155,7 +151,7 @@ class Commit prev_commit.try :id end - def parents_count + def parents_count parents && parents.count || 0 end end diff --git a/app/roles/static_model.rb b/app/roles/static_model.rb new file mode 100644 index 00000000..d26c8f47 --- /dev/null +++ b/app/roles/static_model.rb @@ -0,0 +1,35 @@ +# Provides an ActiveRecord-like interface to a model whose data is not persisted to a database. +module StaticModel + extend ActiveSupport::Concern + + module ClassMethods + # Used by ActiveRecord's polymorphic association to set object_id + def primary_key + 'id' + end + + # Used by ActiveRecord's polymorphic association to set object_type + def base_class + self + end + end + + # Used by AR for fetching attributes + # + # Pass it along if we respond to it. + def [](key) + send(key) if respond_to?(key) + end + + def to_param + id + end + + def persisted? + false + end + + def destroyed? + false + end +end From 40d619107f9027a33eccc01afa120f7b588e5cf9 Mon Sep 17 00:00:00 2001 From: Robert Speicher Date: Sat, 1 Sep 2012 23:56:44 -0400 Subject: [PATCH 3/4] Add link_title to CommitDecorator --- app/decorators/commit_decorator.rb | 9 +++++++++ lib/gitlab/markdown.rb | 2 +- 2 files changed, 10 insertions(+), 1 deletion(-) diff --git a/app/decorators/commit_decorator.rb b/app/decorators/commit_decorator.rb index cc8fa975..074176ae 100644 --- a/app/decorators/commit_decorator.rb +++ b/app/decorators/commit_decorator.rb @@ -1,6 +1,15 @@ class CommitDecorator < ApplicationDecorator decorates :commit + # Returns a string describing the commit for use in a link title + # + # Example + # + # "Commit: Alex Denisov - Project git clone panel" + def link_title + "Commit: #{author_name} - #{title}" + end + # Returns the commits title. # # Usually, the commit title is the first line of the commit message. diff --git a/lib/gitlab/markdown.rb b/lib/gitlab/markdown.rb index 75fa835d..9a07133d 100644 --- a/lib/gitlab/markdown.rb +++ b/lib/gitlab/markdown.rb @@ -100,7 +100,7 @@ module Gitlab def reference_commit(identifier) if commit = @project.commit(identifier) - link_to(identifier, project_commit_path(@project, id: commit.id), html_options.merge(title: "Commit: #{commit.author_name} - #{CommitDecorator.new(commit).title}", class: "gfm gfm-commit #{html_options[:class]}")) + link_to(identifier, project_commit_path(@project, id: commit.id), html_options.merge(title: CommitDecorator.new(commit).link_title, class: "gfm gfm-commit #{html_options[:class]}")) end end end From ef24576fc2239935d2d7b553e7d55674abf4eb4c Mon Sep 17 00:00:00 2001 From: Robert Speicher Date: Sun, 2 Sep 2012 02:13:13 -0400 Subject: [PATCH 4/4] Redesign gfm helper specs Should now be much clearer about what each spec is actually testing. For example, instead of testing stuff like link classes and titles in every single call, we only test those things once, in their own specs. --- spec/helpers/gitlab_markdown_helper_spec.rb | 344 ++++++++++++-------- 1 file changed, 200 insertions(+), 144 deletions(-) diff --git a/spec/helpers/gitlab_markdown_helper_spec.rb b/spec/helpers/gitlab_markdown_helper_spec.rb index 28bd46ec..00164e0c 100644 --- a/spec/helpers/gitlab_markdown_helper_spec.rb +++ b/spec/helpers/gitlab_markdown_helper_spec.rb @@ -1,232 +1,288 @@ require "spec_helper" describe GitlabMarkdownHelper do + let!(:project) { create(:project) } + + let(:user) { create(:user, name: 'gfm') } + let(:commit) { CommitDecorator.decorate(project.commit) } + let(:issue) { create(:issue, project: project) } + let(:merge_request) { create(:merge_request, project: project) } + let(:snippet) { create(:snippet, project: project) } + let(:member) { project.users_projects.where(user_id: user).first } + before do - @project = Factory(:project) - @commit = @project.repo.commits.first.parents.first - @commit = CommitDecorator.decorate(Commit.new(@commit)) - @other_project = Factory :project, path: "OtherPath", code: "OtherCode" - @fake_user = Factory :user, name: "fred" + # Helper expects a @project instance variable + @project = project end describe "#gfm" do - it "should return text if @project is not set" do - @project = nil + it "should return unaltered text if project is nil" do + actual = "Testing references: ##{issue.id}" - gfm("foo").should == "foo" + gfm(actual).should_not == actual + + @project = nil + gfm(actual).should == actual + end + + it "should not alter non-references" do + actual = expected = "_Please_ *stop* 'helping' and all the other b*$#%' you do." + gfm(actual).should == expected + end + + it "should not touch HTML entities" do + actual = expected = "We'll accept good pull requests." + gfm(actual).should == expected + end + + it "should forward HTML options to links" do + gfm("Fixed in #{commit.id}", class: "foo").should have_selector("a.gfm.foo") end describe "referencing a commit" do + let(:expected) { project_commit_path(project, commit) } + it "should link using a full id" do - gfm("Reverts changes from #{@commit.id}").should == "Reverts changes from #{link_to @commit.id, project_commit_path(@project, id: @commit.id), title: "Commit: #{@commit.author_name} - #{@commit.title}", class: "gfm gfm-commit "}" + actual = "Reverts #{commit.id}" + gfm(actual).should match(expected) end it "should link using a short id" do - gfm("Backported from #{@commit.id[0, 6]}").should == "Backported from #{link_to @commit.id[0, 6], project_commit_path(@project, id: @commit.id), title: "Commit: #{@commit.author_name} - #{@commit.title}", class: "gfm gfm-commit "}" + actual = "Backported from #{commit.short_id(6)}" + gfm(actual).should match(expected) end - it "should link with adjecent text" do - gfm("Reverted (see #{@commit.id})").should == "Reverted (see #{link_to @commit.id, project_commit_path(@project, id: @commit.id), title: "Commit: #{@commit.author_name} - #{@commit.title}", class: "gfm gfm-commit "})" + it "should link with adjacent text" do + actual = "Reverted (see #{commit.id})" + gfm(actual).should match(expected) + end + + it "should keep whitespace intact" do + actual = "Changes #{commit.id} dramatically" + expected = /Changes #{commit.id}<\/a> dramatically/ + gfm(actual).should match(expected) end it "should not link with an invalid id" do - gfm("What happened in 12345678?").should == "What happened in 12345678?" + actual = expected = "What happened in #{commit.id.reverse}" + gfm(actual).should == expected + end + + it "should include a title attribute" do + actual = "Reverts #{commit.id}" + gfm(actual).should match(/title="#{commit.link_title}"/) + end + + it "should include standard gfm classes" do + actual = "Reverts #{commit.id}" + gfm(actual).should match(/class="\s?gfm gfm-commit\s?"/) end end describe "referencing a team member" do - it "should link using a simple name" do - user = Factory :user, name: "barry" - @project.users << user - member = @project.users_projects.where(user_id: user).first + let(:actual) { "@#{user.name} you are right." } + let(:expected) { project_team_member_path(project, member) } - gfm("@#{user.name} you are right").should == "#{link_to "@#{user.name}", project_team_member_path(@project, member), class: "gfm gfm-team_member "} you are right" + before do + project.users << user + end + + it "should link using a simple name" do + gfm(actual).should match(expected) end it "should link using a name with dots" do - user = Factory :user, name: "alphA.Beta" - @project.users << user - member = @project.users_projects.where(user_id: user).first - - gfm("@#{user.name} you are right").should == "#{link_to "@#{user.name}", project_team_member_path(@project, member), class: "gfm gfm-team_member "} you are right" + user.update_attributes(name: "alphA.Beta") + gfm(actual).should match(expected) end it "should link using name with underscores" do - user = Factory :user, name: "ping_pong_king" - @project.users << user - member = @project.users_projects.where(user_id: user).first - - gfm("@#{user.name} you are right").should == "#{link_to "@#{user.name}", project_team_member_path(@project, member), class: "gfm gfm-team_member "} you are right" + user.update_attributes(name: "ping_pong_king") + gfm(actual).should match(expected) end - it "should link with adjecent text" do - user = Factory.create(:user, name: "ace") - @project.users << user - member = @project.users_projects.where(user_id: user).first - - gfm("Mail the Admin (@#{user.name})").should == "Mail the Admin (#{link_to "@#{user.name}", project_team_member_path(@project, member), class: "gfm gfm-team_member "})" + it "should link with adjacent text" do + actual = "Mail the admin (@gfm)" + gfm(actual).should match(expected) end - it "should add styles" do - user = Factory :user, name: "barry" - @project.users << user - gfm("@#{user.name} you are right").should have_selector(".gfm.gfm-team_member") + it "should keep whitespace intact" do + actual = "Yes, @#{user.name} is right." + expected = /Yes, @#{user.name}<\/a> is right/ + gfm(actual).should match(expected) end - it "should not link using a bogus name" do - gfm("What hapened to @foo?").should == "What hapened to @foo?" + it "should not link with an invalid id" do + actual = expected = "@#{user.name.reverse} you are right." + gfm(actual).should == expected + end + + it "should include standard gfm classes" do + gfm(actual).should match(/class="\s?gfm gfm-team_member\s?"/) + end + end + + # Shared examples for referencing an object + # + # Expects the following attributes to be available in the example group: + # + # - object - The object itself + # - reference - The object reference string (e.g., #1234, $1234, !1234) + # + # Currently limited to Snippets, Issues and MergeRequests + shared_examples 'referenced object' do + let(:actual) { "Reference to #{reference}" } + let(:expected) { polymorphic_path([project, object]) } + + it "should link using a valid id" do + gfm(actual).should match(expected) + end + + it "should link with adjacent text" do + # Wrap the reference in parenthesis + gfm(actual.gsub(reference, "(#{reference})")).should match(expected) + + # Append some text to the end of the reference + gfm(actual.gsub(reference, "#{reference}, right?")).should match(expected) + end + + it "should keep whitespace intact" do + actual = "Referenced #{reference} already." + expected = /Referenced [^\s]+<\/a> already/ + gfm(actual).should match(expected) + end + + it "should not link with an invalid id" do + # Modify the reference string so it's still parsed, but is invalid + reference.gsub!(/^(.)(\d+)$/, '\1' + ('\2' * 2)) + gfm(actual).should == actual + end + + it "should include a title attribute" do + title = "#{object.class.to_s.titlecase}: #{object.title}" + gfm(actual).should match(/title="#{title}"/) + end + + it "should include standard gfm classes" do + css = object.class.to_s.underscore + gfm(actual).should match(/class="\s?gfm gfm-#{css}\s?"/) end end describe "referencing an issue" do - before do - @issue = Factory :issue, assignee: @fake_user, author: @fake_user, project: @project - @invalid_issue = Factory :issue, assignee: @fake_user, author: @fake_user, project: @other_project - end + let(:object) { issue } + let(:reference) { "##{issue.id}" } - it "should link using a correct id" do - gfm("Fixes ##{@issue.id}").should == "Fixes #{link_to "##{@issue.id}", project_issue_path(@project, @issue), title: "Issue: #{@issue.title}", class: "gfm gfm-issue "}" - end - - it "should link with adjecent text" do - gfm("This has already been discussed (see ##{@issue.id})").should == "This has already been discussed (see #{link_to "##{@issue.id}", project_issue_path(@project, @issue), title: "Issue: #{@issue.title}", class: "gfm gfm-issue "})" - end - - it "should add styles" do - gfm("Fixes ##{@issue.id}").should have_selector(".gfm.gfm-issue") - end - - it "should not link using an invalid id" do - gfm("##{@invalid_issue.id} has been marked duplicate of this").should == "##{@invalid_issue.id} has been marked duplicate of this" - end + include_examples 'referenced object' end describe "referencing a merge request" do - before do - @merge_request = Factory :merge_request, assignee: @fake_user, author: @fake_user, project: @project - @invalid_merge_request = Factory :merge_request, assignee: @fake_user, author: @fake_user, project: @other_project - end + let(:object) { merge_request } + let(:reference) { "!#{merge_request.id}" } - it "should link using a correct id" do - gfm("Fixed in !#{@merge_request.id}").should == "Fixed in #{link_to "!#{@merge_request.id}", project_merge_request_path(@project, @merge_request), title: "Merge Request: #{@merge_request.title}", class: "gfm gfm-merge_request "}" - end - - it "should link with adjecent text" do - gfm("This has been fixed already (see !#{@merge_request.id})").should == "This has been fixed already (see #{link_to "!#{@merge_request.id}", project_merge_request_path(@project, @merge_request), title: "Merge Request: #{@merge_request.title}", class: "gfm gfm-merge_request "})" - end - - it "should add styles" do - gfm("Fixed in !#{@merge_request.id}").should have_selector(".gfm.gfm-merge_request") - end - - it "should not link using an invalid id" do - gfm("!#{@invalid_merge_request.id} violates our coding guidelines") - end + include_examples 'referenced object' end describe "referencing a snippet" do - before do - @snippet = Factory.create(:snippet, - title: "Render asset to string", - author: @fake_user, - project: @project) - end + let(:object) { snippet } + let(:reference) { "$#{snippet.id}" } - it "should link using a correct id" do - gfm("Check out $#{@snippet.id}").should == "Check out #{link_to "$#{@snippet.id}", project_snippet_path(@project, @snippet), title: "Snippet: #{@snippet.title}", class: "gfm gfm-snippet "}" - end - - it "should link with adjecent text" do - gfm("I have created a snippet for that ($#{@snippet.id})").should == "I have created a snippet for that (#{link_to "$#{@snippet.id}", project_snippet_path(@project, @snippet), title: "Snippet: #{@snippet.title}", class: "gfm gfm-snippet "})" - end - - it "should add styles" do - gfm("Check out $#{@snippet.id}").should have_selector(".gfm.gfm-snippet") - end - - it "should not link using an invalid id" do - gfm("Don't use $1234").should == "Don't use $1234" - end + include_examples 'referenced object' end - it "should link to multiple things" do - user = Factory :user, name: "barry" - @project.users << user - member = @project.users_projects.where(user_id: user).first + describe "referencing multiple objects" do + let(:actual) { "!#{merge_request.id} -> #{commit.id} -> ##{issue.id}" } - gfm("Let @#{user.name} fix the *mess* in #{@commit.id}").should == "Let #{link_to "@#{user.name}", project_team_member_path(@project, member), class: "gfm gfm-team_member "} fix the *mess* in #{link_to @commit.id, project_commit_path(@project, id: @commit.id), title: "Commit: #{@commit.author_name} - #{@commit.title}", class: "gfm gfm-commit "}" - end + it "should link to the merge request" do + expected = project_merge_request_path(project, merge_request) + gfm(actual).should match(expected) + end - it "should not trip over other stuff" do - gfm("_Please_ *stop* 'helping' and all the other b*$#%' you do.").should == "_Please_ *stop* 'helping' and all the other b*$#%' you do." - end + it "should link to the commit" do + expected = project_commit_path(project, commit) + gfm(actual).should match(expected) + end - it "should not touch HTML entities" do - gfm("We'll accept good pull requests.").should == "We'll accept good pull requests." - end - - it "should forward HTML options to links" do - gfm("fixed in #{@commit.id}", class: "foo").should have_selector("a.foo") + it "should link to the issue" do + expected = project_issue_path(project, issue) + gfm(actual).should match(expected) + end end end describe "#link_to_gfm" do - let(:issue1) { Factory :issue, assignee: @fake_user, author: @fake_user, project: @project } - let(:issue2) { Factory :issue, assignee: @fake_user, author: @fake_user, project: @project } + let(:commit_path) { project_commit_path(project, commit) } + let(:issues) { create_list(:issue, 2, project: project) } it "should handle references nested in links with all the text" do - link_to_gfm("This should finally fix ##{issue1.id} and ##{issue2.id} for real", project_commit_path(@project, id: @commit.id)).should == "#{link_to "This should finally fix ", project_commit_path(@project, id: @commit.id)}#{link_to "##{issue1.id}", project_issue_path(@project, issue1), title: "Issue: #{issue1.title}", class: "gfm gfm-issue "}#{link_to " and ", project_commit_path(@project, id: @commit.id)}#{link_to "##{issue2.id}", project_issue_path(@project, issue2), title: "Issue: #{issue2.title}", class: "gfm gfm-issue "}#{link_to " for real", project_commit_path(@project, id: @commit.id)}" + actual = link_to_gfm("This should finally fix ##{issues[0].id} and ##{issues[1].id} for real", commit_path) + + # Break the result into groups of links with their content, without + # closing tags + groups = actual.split("") + + # Leading commit link + groups[0].should match(/href="#{commit_path}"/) + groups[0].should match(/This should finally fix $/) + + # First issue link + groups[1].should match(/href="#{project_issue_path(project, issues[0])}"/) + groups[1].should match(/##{issues[0].id}$/) + + # Internal commit link + groups[2].should match(/href="#{commit_path}"/) + groups[2].should match(/ and /) + + # Second issue link + groups[3].should match(/href="#{project_issue_path(project, issues[1])}"/) + groups[3].should match(/##{issues[1].id}$/) + + # Trailing commit link + groups[4].should match(/href="#{commit_path}"/) + groups[4].should match(/ for real$/) end it "should forward HTML options" do - link_to_gfm("This should finally fix ##{issue1.id} for real", project_commit_path(@project, id: @commit.id), class: "foo").should have_selector(".foo") + actual = link_to_gfm("Fixed in #{commit.id}", commit_path, class: 'foo') + actual.should have_selector 'a.gfm.gfm-commit.foo' end end describe "#markdown" do - before do - @issue = Factory :issue, assignee: @fake_user, author: @fake_user, project: @project - @merge_request = Factory :merge_request, assignee: @fake_user, author: @fake_user, project: @project - @note = Factory.create(:note, - note: "Screenshot of the new feature", - project: @project, - noteable_id: @commit.id, - noteable_type: "Commit", - attachment: "screenshot123.jpg") - @snippet = Factory.create(:snippet, - title: "Render asset to string", - author: @fake_user, - project: @project) - - @other_user = Factory :user, name: "bill" - @project.users << @other_user - @member = @project.users_projects.where(user_id: @other_user).first - end - it "should handle references in paragraphs" do - markdown("\n\nLorem ipsum dolor sit amet, consectetur adipiscing elit. #{@commit.id} Nam pulvinar sapien eget odio adipiscing at faucibus orci vestibulum.\n").should == "

Lorem ipsum dolor sit amet, consectetur adipiscing elit. #{link_to @commit.id, project_commit_path(@project, id: @commit.id), title: "Commit: #{@commit.author_name} - #{@commit.title}", class: "gfm gfm-commit "} Nam pulvinar sapien eget odio adipiscing at faucibus orci vestibulum.

\n" + markdown("\n\nLorem ipsum dolor sit amet, consectetur adipiscing elit. #{commit.id} Nam pulvinar sapien eget odio adipiscing at faucibus orci vestibulum.\n").should == "

Lorem ipsum dolor sit amet, consectetur adipiscing elit. #{link_to commit.id, project_commit_path(project, commit), title: commit.link_title, class: "gfm gfm-commit "} Nam pulvinar sapien eget odio adipiscing at faucibus orci vestibulum.

\n" end it "should handle references in headers" do - markdown("\n# Working around ##{@issue.id} for now\n## Apply !#{@merge_request.id}").should == "

Working around #{link_to "##{@issue.id}", project_issue_path(@project, @issue), title: "Issue: #{@issue.title}", class: "gfm gfm-issue "} for now

\n\n

Apply #{link_to "!#{@merge_request.id}", project_merge_request_path(@project, @merge_request), title: "Merge Request: #{@merge_request.title}", class: "gfm gfm-merge_request "}

\n" + actual = "\n# Working around ##{issue.id}\n## Apply !#{merge_request.id}" + + markdown(actual).should match(%r{Working around ##{issue.id}}) + markdown(actual).should match(%r{Apply !#{merge_request.id}}) end it "should handle references in lists" do - markdown("\n* dark: ##{@issue.id}\n* light by @#{@other_user.name}\n").should == "
    \n
  • dark: #{link_to "##{@issue.id}", project_issue_path(@project, @issue), title: "Issue: #{@issue.title}", class: "gfm gfm-issue "}
    • \n
  • light by #{link_to "@#{@other_user.name}", project_team_member_path(@project, @member), class: "gfm gfm-team_member "}
    • \n
\n" + project.users << user + + actual = "\n* dark: ##{issue.id}\n* light by @#{member.user_name}" + + markdown(actual).should match(%r{
  • dark: ##{issue.id}
    • }) + markdown(actual).should match(%r{
  • light by @#{member.user_name}
    • }) end it "should handle references in " do - markdown("Apply _!#{@merge_request.id}_ ASAP").should == "

    Apply #{link_to "!#{@merge_request.id}", project_merge_request_path(@project, @merge_request), title: "Merge Request: #{@merge_request.title}", class: "gfm gfm-merge_request "} ASAP

    \n" + actual = "Apply _!#{merge_request.id}_ ASAP" + + markdown(actual).should match(%r{Apply !#{merge_request.id}}) end it "should leave code blocks untouched" do - markdown("\n some code from $#{@snippet.id}\n here too\n").should == "
    some code from $#{@snippet.id}\nhere too\n
    \n
    \n" + markdown("\n some code from $#{snippet.id}\n here too\n").should == "
    some code from $#{snippet.id}\nhere too\n
    \n
    \n" - markdown("\n```\nsome code from $#{@snippet.id}\nhere too\n```\n").should == "
    some code from $#{@snippet.id}\nhere too\n
    \n
    \n" + markdown("\n```\nsome code from $#{snippet.id}\nhere too\n```\n").should == "
    some code from $#{snippet.id}\nhere too\n
    \n
    \n" end it "should leave inline code untouched" do - markdown("\nDon't use `$#{@snippet.id}` here.\n").should == "

    Don't use $#{@snippet.id} here.

    \n" + markdown("\nDon't use `$#{snippet.id}` here.\n").should == "

    Don't use $#{snippet.id} here.

    \n" end end end