diff options
53 files changed, 603 insertions, 160 deletions
diff --git a/app/assets/stylesheets/new_sidebar.scss b/app/assets/stylesheets/new_sidebar.scss index 55e0343d0dc..fd5e344d8c9 100644 --- a/app/assets/stylesheets/new_sidebar.scss +++ b/app/assets/stylesheets/new_sidebar.scss @@ -388,6 +388,10 @@ $new-sidebar-collapsed-width: 50px; } } + .nav-icon-container { + margin-right: 0; + } + .toggle-sidebar-button { width: $new-sidebar-collapsed-width - 2px; padding: 16px 18px; diff --git a/app/assets/stylesheets/pages/notes.scss b/app/assets/stylesheets/pages/notes.scss index 45f2aed1531..e437bad4912 100644 --- a/app/assets/stylesheets/pages/notes.scss +++ b/app/assets/stylesheets/pages/notes.scss @@ -516,7 +516,7 @@ ul.notes { } .note-actions-item { - margin-left: 15px; + margin-left: 12px; display: flex; align-items: center; @@ -620,15 +620,25 @@ ul.notes { .note-role { position: relative; - padding: 0 7px; + display: inline-block; color: $notes-role-color; font-size: 12px; line-height: 20px; - border: 1px solid $border-color; - border-radius: $label-border-radius; + margin: 0 3px; + + &.note-role-access { + padding: 0 7px; + border: 1px solid $border-color; + border-radius: $label-border-radius; + } + + &.note-role-special { + text-shadow: 0 0 15px $gl-text-color-inverted; + } } + /** * Line note button on the side of diffs */ diff --git a/app/controllers/concerns/renders_commits.rb b/app/controllers/concerns/renders_commits.rb new file mode 100644 index 00000000000..bb2c1dfa00a --- /dev/null +++ b/app/controllers/concerns/renders_commits.rb @@ -0,0 +1,7 @@ +module RendersCommits + def prepare_commits_for_rendering(commits) + Banzai::CommitRenderer.render(commits, @project, current_user) + + commits + end +end diff --git a/app/controllers/concerns/renders_notes.rb b/app/controllers/concerns/renders_notes.rb index 41c3114ad1e..4791bc561a4 100644 --- a/app/controllers/concerns/renders_notes.rb +++ b/app/controllers/concerns/renders_notes.rb @@ -1,7 +1,8 @@ module RendersNotes - def prepare_notes_for_rendering(notes) + def prepare_notes_for_rendering(notes, noteable = nil) preload_noteable_for_regular_notes(notes) preload_max_access_for_authors(notes, @project) + preload_first_time_contribution_for_authors(noteable, notes) Banzai::NoteRenderer.render(notes, @project, current_user) notes @@ -19,4 +20,10 @@ module RendersNotes def preload_noteable_for_regular_notes(notes) ActiveRecord::Associations::Preloader.new.preload(notes.reject(&:for_commit?), :noteable) end + + def preload_first_time_contribution_for_authors(noteable, notes) + return unless noteable.is_a?(Issuable) && noteable.first_contribution? + + notes.each {|n| n.specialize_for_first_contribution!(noteable)} + end end diff --git a/app/controllers/projects/commit_controller.rb b/app/controllers/projects/commit_controller.rb index 6de125e7e80..1a775def506 100644 --- a/app/controllers/projects/commit_controller.rb +++ b/app/controllers/projects/commit_controller.rb @@ -127,7 +127,7 @@ class Projects::CommitController < Projects::ApplicationController @discussions = commit.discussions @notes = (@grouped_diff_discussions.values.flatten + @discussions).flat_map(&:notes) - @notes = prepare_notes_for_rendering(@notes) + @notes = prepare_notes_for_rendering(@notes, @commit) end def assign_change_commit_vars diff --git a/app/controllers/projects/commits_controller.rb b/app/controllers/projects/commits_controller.rb index 2de9900d449..4a841bf2073 100644 --- a/app/controllers/projects/commits_controller.rb +++ b/app/controllers/projects/commits_controller.rb @@ -2,6 +2,7 @@ require "base64" class Projects::CommitsController < Projects::ApplicationController include ExtractsPath + include RendersCommits before_action :require_non_empty_project before_action :assign_ref_vars @@ -56,5 +57,7 @@ class Projects::CommitsController < Projects::ApplicationController else @repository.commits(@ref, path: @path, limit: @limit, offset: @offset) end + + @commits = prepare_commits_for_rendering(@commits) end end diff --git a/app/controllers/projects/compare_controller.rb b/app/controllers/projects/compare_controller.rb index c8613c0d634..193549663ac 100644 --- a/app/controllers/projects/compare_controller.rb +++ b/app/controllers/projects/compare_controller.rb @@ -3,6 +3,7 @@ require 'addressable/uri' class Projects::CompareController < Projects::ApplicationController include DiffForPath include DiffHelper + include RendersCommits # Authorize before_action :require_non_empty_project @@ -50,7 +51,7 @@ class Projects::CompareController < Projects::ApplicationController .execute(@project, @start_ref) if @compare - @commits = @compare.commits + @commits = prepare_commits_for_rendering(@compare.commits) @diffs = @compare.diffs(diff_options) environment_params = @repository.branch_exists?(@head_ref) ? { ref: @head_ref } : { commit: @compare.commit } diff --git a/app/controllers/projects/issues_controller.rb b/app/controllers/projects/issues_controller.rb index dc9e6f71152..ab9f132b502 100644 --- a/app/controllers/projects/issues_controller.rb +++ b/app/controllers/projects/issues_controller.rb @@ -85,7 +85,7 @@ class Projects::IssuesController < Projects::ApplicationController @note = @project.notes.new(noteable: @issue) @discussions = @issue.discussions - @notes = prepare_notes_for_rendering(@discussions.flat_map(&:notes)) + @notes = prepare_notes_for_rendering(@discussions.flat_map(&:notes), @noteable) respond_to do |format| format.html diff --git a/app/controllers/projects/merge_requests/creations_controller.rb b/app/controllers/projects/merge_requests/creations_controller.rb index f35d53896ba..1096afbb798 100644 --- a/app/controllers/projects/merge_requests/creations_controller.rb +++ b/app/controllers/projects/merge_requests/creations_controller.rb @@ -1,6 +1,7 @@ class Projects::MergeRequests::CreationsController < Projects::MergeRequests::ApplicationController include DiffForPath include DiffHelper + include RendersCommits skip_before_action :merge_request skip_before_action :ensure_ref_fetched @@ -107,7 +108,7 @@ class Projects::MergeRequests::CreationsController < Projects::MergeRequests::Ap @target_project = @merge_request.target_project @source_project = @merge_request.source_project - @commits = @merge_request.commits + @commits = prepare_commits_for_rendering(@merge_request.commits) @commit = @merge_request.diff_head_commit @note_counts = Note.where(commit_id: @commits.map(&:id)) diff --git a/app/controllers/projects/merge_requests/diffs_controller.rb b/app/controllers/projects/merge_requests/diffs_controller.rb index 330b7df4541..109418c73f7 100644 --- a/app/controllers/projects/merge_requests/diffs_controller.rb +++ b/app/controllers/projects/merge_requests/diffs_controller.rb @@ -61,6 +61,6 @@ class Projects::MergeRequests::DiffsController < Projects::MergeRequests::Applic @use_legacy_diff_notes = !@merge_request.has_complete_diff_refs? @grouped_diff_discussions = @merge_request.grouped_diff_discussions(@compare.diff_refs) - @notes = prepare_notes_for_rendering(@grouped_diff_discussions.values.flatten.flat_map(&:notes)) + @notes = prepare_notes_for_rendering(@grouped_diff_discussions.values.flatten.flat_map(&:notes), @merge_request) end end diff --git a/app/controllers/projects/merge_requests_controller.rb b/app/controllers/projects/merge_requests_controller.rb index 5095d7fd445..3aa5dadb5ca 100644 --- a/app/controllers/projects/merge_requests_controller.rb +++ b/app/controllers/projects/merge_requests_controller.rb @@ -2,6 +2,7 @@ class Projects::MergeRequestsController < Projects::MergeRequests::ApplicationCo include ToggleSubscriptionAction include IssuableActions include RendersNotes + include RendersCommits include ToggleAwardEmoji include IssuableCollections @@ -60,12 +61,12 @@ class Projects::MergeRequestsController < Projects::MergeRequests::ApplicationCo # Build a note object for comment form @note = @project.notes.new(noteable: @merge_request) - @discussions = @merge_request.discussions - @notes = prepare_notes_for_rendering(@discussions.flat_map(&:notes)) - @noteable = @merge_request @commits_count = @merge_request.commits_count + @discussions = @merge_request.discussions + @notes = prepare_notes_for_rendering(@discussions.flat_map(&:notes), @noteable) + labels set_pipeline_variables @@ -94,7 +95,7 @@ class Projects::MergeRequestsController < Projects::MergeRequests::ApplicationCo def commits # Get commits from repository # or from cache if already merged - @commits = @merge_request.commits + @commits = prepare_commits_for_rendering(@merge_request.commits) @note_counts = Note.where(commit_id: @commits.map(&:id)) .group(:commit_id).count diff --git a/app/controllers/projects/snippets_controller.rb b/app/controllers/projects/snippets_controller.rb index d07143d294f..7c19aa7bb23 100644 --- a/app/controllers/projects/snippets_controller.rb +++ b/app/controllers/projects/snippets_controller.rb @@ -64,7 +64,7 @@ class Projects::SnippetsController < Projects::ApplicationController @noteable = @snippet @discussions = @snippet.discussions - @notes = prepare_notes_for_rendering(@discussions.flat_map(&:notes)) + @notes = prepare_notes_for_rendering(@discussions.flat_map(&:notes), @noteable) render 'show' end diff --git a/app/controllers/search_controller.rb b/app/controllers/search_controller.rb index d58c8d14a75..fbad9ba7db8 100644 --- a/app/controllers/search_controller.rb +++ b/app/controllers/search_controller.rb @@ -2,6 +2,7 @@ class SearchController < ApplicationController skip_before_action :authenticate_user! include SearchHelper + include RendersCommits layout 'search' @@ -20,6 +21,8 @@ class SearchController < ApplicationController @search_results = search_service.search_results @search_objects = search_service.search_objects + render_commits if @scope == 'commits' + check_single_commit_result end @@ -38,6 +41,10 @@ class SearchController < ApplicationController private + def render_commits + @search_objects = prepare_commits_for_rendering(@search_objects) + end + def check_single_commit_result if @search_results.single_commit_result? only_commit = @search_results.objects('commits').first diff --git a/app/controllers/snippets_controller.rb b/app/controllers/snippets_controller.rb index 8c3abd0a085..c1cdc7c9831 100644 --- a/app/controllers/snippets_controller.rb +++ b/app/controllers/snippets_controller.rb @@ -66,7 +66,7 @@ class SnippetsController < ApplicationController @noteable = @snippet @discussions = @snippet.discussions - @notes = prepare_notes_for_rendering(@discussions.flat_map(&:notes)) + @notes = prepare_notes_for_rendering(@discussions.flat_map(&:notes), @noteable) respond_to do |format| format.html do diff --git a/app/helpers/issuables_helper.rb b/app/helpers/issuables_helper.rb index 2cf2e7dddb0..ce2999e6696 100644 --- a/app/helpers/issuables_helper.rb +++ b/app/helpers/issuables_helper.rb @@ -134,6 +134,8 @@ module IssuablesHelper end output << " ".html_safe + output << content_tag(:span, (issuable_first_contribution_icon if issuable.first_contribution?), class: 'has-tooltip', title: _('1st contribution!')) + output << content_tag(:span, (issuable.task_status if issuable.tasks?), id: "task_status", class: "hidden-xs hidden-sm") output << content_tag(:span, (issuable.task_status_short if issuable.tasks?), id: "task_status_short", class: "hidden-md hidden-lg") @@ -169,6 +171,13 @@ module IssuablesHelper html.html_safe end + def issuable_first_contribution_icon + content_tag(:span, class: 'fa-stack') do + concat(icon('certificate', class: "fa-stack-2x")) + concat(content_tag(:strong, '1', class: 'fa-inverse fa-stack-1x')) + end + end + def assigned_issuables_count(issuable_type) case issuable_type when :issues diff --git a/app/helpers/markup_helper.rb b/app/helpers/markup_helper.rb index 941cfce8370..46bced00c72 100644 --- a/app/helpers/markup_helper.rb +++ b/app/helpers/markup_helper.rb @@ -21,25 +21,28 @@ module MarkupHelper end # Use this in places where you would normally use link_to(gfm(...), ...). - # + def link_to_markdown(body, url, html_options = {}) + return '' if body.blank? + + link_to_html(markdown(body, pipeline: :single_line), url, html_options) + end + + def link_to_markdown_field(object, field, url, html_options = {}) + rendered_field = markdown_field(object, field) + + link_to_html(rendered_field, url, html_options) + end + # It solves a problem occurring with nested links (i.e. # "<a>outer text <a>gfm ref</a> more outer text</a>"). This will not be # interpreted as intended. Browsers will parse something like # "<a>outer text </a><a>gfm ref</a> more outer text" (notice the last part is - # not linked any more). link_to_gfm corrects that. It wraps all parts to + # not linked any more). link_to_html corrects that. It wraps all parts to # explicitly produce the correct linking behavior (i.e. # "<a>outer text </a><a>gfm ref</a><a> more outer text</a>"). - def link_to_gfm(body, url, html_options = {}) - return '' if body.blank? + def link_to_html(redacted, url, html_options = {}) + fragment = Nokogiri::HTML::DocumentFragment.parse(redacted) - context = { - project: @project, - current_user: (current_user if defined?(current_user)), - pipeline: :single_line - } - gfm_body = Banzai.render(body, context) - - fragment = Nokogiri::HTML::DocumentFragment.parse(gfm_body) if fragment.children.size == 1 && fragment.children[0].name == 'a' # Fragment has only one node, and it's a link generated by `gfm`. # Replace it with our requested link. @@ -82,7 +85,10 @@ module MarkupHelper def markdown_field(object, field) object = object.for_display if object.respond_to?(:for_display) + redacted_field_html = object.try(:"redacted_#{field}_html") + return '' unless object.present? + return redacted_field_html if redacted_field_html html = Banzai.render_field(object, field) prepare_for_rendering(html, object.banzai_render_context(field)) diff --git a/app/helpers/notes_helper.rb b/app/helpers/notes_helper.rb index 083ace65b09..ce028195e51 100644 --- a/app/helpers/notes_helper.rb +++ b/app/helpers/notes_helper.rb @@ -73,7 +73,7 @@ module NotesHelper end def note_max_access_for_user(note) - note.project.team.human_max_access(note.author_id) + note.project.team.max_member_access(note.author_id) end def discussion_path(discussion) diff --git a/app/models/commit.rb b/app/models/commit.rb index ba3845df867..2ae8890c1b3 100644 --- a/app/models/commit.rb +++ b/app/models/commit.rb @@ -16,6 +16,8 @@ class Commit participant :notes_with_associations attr_accessor :project, :author + attr_accessor :redacted_description_html + attr_accessor :redacted_title_html DIFF_SAFE_LINES = Gitlab::Git::DiffCollection::DEFAULT_LIMITS[:max_lines] @@ -26,6 +28,13 @@ class Commit # The SHA can be between 7 and 40 hex characters. COMMIT_SHA_PATTERN = '\h{7,40}'.freeze + def banzai_render_context(field) + context = { pipeline: :single_line, project: self.project } + context[:author] = self.author if self.author + + context + end + class << self def decorate(commits, project) commits.map do |commit| diff --git a/app/models/concerns/issuable.rb b/app/models/concerns/issuable.rb index 681c3241dbb..265f6e48540 100644 --- a/app/models/concerns/issuable.rb +++ b/app/models/concerns/issuable.rb @@ -334,4 +334,11 @@ module Issuable metrics = self.metrics || create_metrics metrics.record! end + + ## + # Override in issuable specialization + # + def first_contribution? + false + end end diff --git a/app/models/merge_request.rb b/app/models/merge_request.rb index b82f49d7073..2a56bab48a3 100644 --- a/app/models/merge_request.rb +++ b/app/models/merge_request.rb @@ -960,6 +960,12 @@ class MergeRequest < ActiveRecord::Base Projects::OpenMergeRequestsCountService.new(target_project).refresh_cache end + def first_contribution? + return false if project.team.max_member_access(author_id) > Gitlab::Access::GUEST + + project.merge_requests.merged.where(author_id: author_id).empty? + end + private def write_ref diff --git a/app/models/note.rb b/app/models/note.rb index 1073c115630..f44590e2144 100644 --- a/app/models/note.rb +++ b/app/models/note.rb @@ -15,6 +15,16 @@ class Note < ActiveRecord::Base include IgnorableColumn include Editable + module SpecialRole + FIRST_TIME_CONTRIBUTOR = :first_time_contributor + + class << self + def values + constants.map {|const| self.const_get(const)} + end + end + end + ignore_column :original_discussion_id cache_markdown_field :note, pipeline: :note, issuable_state_filter_enabled: true @@ -32,9 +42,12 @@ class Note < ActiveRecord::Base # Banzai::ObjectRenderer attr_accessor :user_visible_reference_count - # Attribute used to store the attributes that have ben changed by quick actions. + # Attribute used to store the attributes that have been changed by quick actions. attr_accessor :commands_changes + # A special role that may be displayed on issuable's discussions + attr_accessor :special_role + default_value_for :system, false attr_mentionable :note, pipeline: :note @@ -141,6 +154,10 @@ class Note < ActiveRecord::Base .group(:noteable_id) .where(noteable_type: type, noteable_id: ids) end + + def has_special_role?(role, note) + note.special_role == role + end end def cross_reference? @@ -206,6 +223,22 @@ class Note < ActiveRecord::Base super(noteable_type.to_s.classify.constantize.base_class.to_s) end + def special_role=(role) + raise "Role is undefined, #{role} not found in #{SpecialRole.values}" unless SpecialRole.values.include?(role) + + @special_role = role + end + + def has_special_role?(role) + self.class.has_special_role?(role, self) + end + + def specialize_for_first_contribution!(noteable) + return unless noteable.author_id == self.author_id + + self.special_role = Note::SpecialRole::FIRST_TIME_CONTRIBUTOR + end + def editable? !system? end diff --git a/app/models/project_team.rb b/app/models/project_team.rb index 674eacd28e8..09049824ff7 100644 --- a/app/models/project_team.rb +++ b/app/models/project_team.rb @@ -150,7 +150,7 @@ class ProjectTeam end def human_max_access(user_id) - Gitlab::Access.options_with_owner.key(max_member_access(user_id)) + Gitlab::Access.human_access(max_member_access(user_id)) end # Determine the maximum access level for a group of users in bulk. diff --git a/app/views/projects/blame/show.html.haml b/app/views/projects/blame/show.html.haml index c7359d873d9..60ac202bde0 100644 --- a/app/views/projects/blame/show.html.haml +++ b/app/views/projects/blame/show.html.haml @@ -22,7 +22,7 @@ = author_avatar(commit, size: 36) .commit-row-title %span.item-title.str-truncated-100 - = link_to_gfm commit.title, project_commit_path(@project, commit.id), class: "cdark", title: commit.title + = link_to_markdown commit.title, project_commit_path(@project, commit.id), class: "cdark", title: commit.title .pull-right = link_to commit.short_id, project_commit_path(@project, commit), class: "commit-sha" diff --git a/app/views/projects/branches/_commit.html.haml b/app/views/projects/branches/_commit.html.haml index 18fbb81c167..7892019bb15 100644 --- a/app/views/projects/branches/_commit.html.haml +++ b/app/views/projects/branches/_commit.html.haml @@ -4,6 +4,6 @@ = link_to commit.short_id, project_commit_path(project, commit.id), class: "commit-sha" · %span.str-truncated - = link_to_gfm commit.title, project_commit_path(project, commit.id), class: "commit-row-message" + = link_to_markdown commit.title, project_commit_path(project, commit.id), class: "commit-row-message" · #{time_ago_with_tooltip(commit.committed_date)} diff --git a/app/views/projects/commits/_commit.html.haml b/app/views/projects/commits/_commit.html.haml index 1214aabe837..b8655808d89 100644 --- a/app/views/projects/commits/_commit.html.haml +++ b/app/views/projects/commits/_commit.html.haml @@ -16,7 +16,7 @@ .commit-detail .commit-content - = link_to_gfm commit.title, project_commit_path(project, commit.id), class: "commit-row-message item-title" + = link_to_markdown_field(commit, :title, project_commit_path(project, commit.id), class: "commit-row-message item-title") %span.commit-row-message.visible-xs-inline · = commit.short_id @@ -28,7 +28,8 @@ - if commit.description? %pre.commit-row-description.js-toggle-content - = preserve(markdown(commit.description, pipeline: :single_line, author: commit.author)) + = preserve(markdown_field(commit, :description)) + .commiter - commit_author_link = commit_author_link(commit, avatar: false, size: 24) - commit_timeago = time_ago_with_tooltip(commit.committed_date) diff --git a/app/views/projects/commits/_inline_commit.html.haml b/app/views/projects/commits/_inline_commit.html.haml index 48cefbe45f2..26385d2f534 100644 --- a/app/views/projects/commits/_inline_commit.html.haml +++ b/app/views/projects/commits/_inline_commit.html.haml @@ -3,6 +3,6 @@ = link_to commit.short_id, project_commit_path(project, commit), class: "commit-sha" %span.str-truncated - = link_to_gfm commit.title, project_commit_path(project, commit.id), class: "commit-row-message" + = link_to_markdown_field(commit, :title, project_commit_path(project, commit.id), class: "commit-row-message") .pull-right #{time_ago_with_tooltip(commit.committed_date)} diff --git a/app/views/projects/deployments/_commit.html.haml b/app/views/projects/deployments/_commit.html.haml index 4c22166c256..014486be868 100644 --- a/app/views/projects/deployments/_commit.html.haml +++ b/app/views/projects/deployments/_commit.html.haml @@ -12,6 +12,6 @@ %span.flex-truncate-child - if commit_title = deployment.commit_title = author_avatar(deployment.commit, size: 20) - = link_to_gfm commit_title, project_commit_path(@project, deployment.sha), class: "commit-row-message" + = link_to_markdown commit_title, project_commit_path(@project, deployment.sha), class: "commit-row-message" - else Cant find HEAD commit for this branch diff --git a/app/views/projects/notes/_actions.html.haml b/app/views/projects/notes/_actions.html.haml index fb07141d2ac..de76832331a 100644 --- a/app/views/projects/notes/_actions.html.haml +++ b/app/views/projects/notes/_actions.html.haml @@ -1,6 +1,8 @@ -- access = note_max_access_for_user(note) -- if access - %span.note-role= access +- if note.has_special_role?(Note::SpecialRole::FIRST_TIME_CONTRIBUTOR) + %span.note-role.note-role-special.has-tooltip{ title: _("This is the author's first Merge Request to this project. Handle with care.") } + = issuable_first_contribution_icon +- if access = note_max_access_for_user(note) + %span.note-role.note-role-access= Gitlab::Access.human_access(access) - if note.resolvable? - can_resolve = can?(current_user, :resolve_note, note) diff --git a/app/views/projects/tree/_tree_commit_column.html.haml b/app/views/projects/tree/_tree_commit_column.html.haml index f3d4706809f..abb3e918e87 100644 --- a/app/views/projects/tree/_tree_commit_column.html.haml +++ b/app/views/projects/tree/_tree_commit_column.html.haml @@ -1,2 +1,2 @@ %span.str-truncated - = link_to_gfm commit.full_title, project_commit_path(@project, commit.id), class: "tree-commit-link" + = link_to_markdown commit.full_title, project_commit_path(@project, commit.id), class: "tree-commit-link" diff --git a/app/views/shared/notes/_note.html.haml b/app/views/shared/notes/_note.html.haml index 7174855e176..4f00a9f2759 100644 --- a/app/views/shared/notes/_note.html.haml +++ b/app/views/shared/notes/_note.html.haml @@ -28,7 +28,7 @@ commented - if note.system %span.system-note-message - = note.redacted_note_html + = markdown_field(note, :note) %a{ href: "##{dom_id(note)}" } = time_ago_with_tooltip(note.created_at, placement: 'bottom', html_class: 'note-created-ago') - unless note.system? @@ -39,7 +39,7 @@ = render 'projects/notes/actions', note: note, note_editable: note_editable .note-body{ class: note_editable ? 'js-task-list-container' : '' } .note-text.md - = note.redacted_note_html + = markdown_field(note, :note) = edited_time_ago_with_tooltip(note, placement: 'bottom', html_class: 'note_edited_ago') .original-note-content.hidden{ data: { post_url: note_url(note), target_id: note.noteable.id, target_type: note.noteable.class.name.underscore } } #{note.note} diff --git a/app/views/shared/projects/_project.html.haml b/app/views/shared/projects/_project.html.haml index f4f155c8d94..52a8fe8bb67 100644 --- a/app/views/shared/projects/_project.html.haml +++ b/app/views/shared/projects/_project.html.haml @@ -31,8 +31,7 @@ - if show_last_commit_as_description .description.prepend-top-5 - = link_to_gfm project.commit.title, project_commit_path(project, project.commit), - class: "commit-row-message" + = link_to_markdown(project.commit.title, project_commit_path(project, project.commit), class: "commit-row-message") - elsif project.description.present? .description.prepend-top-5 = markdown_field(project, :description) diff --git a/changelogs/unreleased/34509-improves-markdown-rendering-performance-for-commits-list.yml b/changelogs/unreleased/34509-improves-markdown-rendering-performance-for-commits-list.yml new file mode 100644 index 00000000000..a61d703bacd --- /dev/null +++ b/changelogs/unreleased/34509-improves-markdown-rendering-performance-for-commits-list.yml @@ -0,0 +1,5 @@ +--- +title: Improves markdown rendering performance for commit lists. +merge_request: +author: +type: other diff --git a/changelogs/unreleased/35161_first_time_contributor_badge.yml b/changelogs/unreleased/35161_first_time_contributor_badge.yml new file mode 100644 index 00000000000..f3ab2d9db31 --- /dev/null +++ b/changelogs/unreleased/35161_first_time_contributor_badge.yml @@ -0,0 +1,4 @@ +--- +title: "First-time contributor badge" +merge_request: 13143 +author: Micaël Bergeron <micaelbergeron@gmail.com> diff --git a/config/initializers/8_metrics.rb b/config/initializers/8_metrics.rb index 5b455a8065a..e1a59d8c152 100644 --- a/config/initializers/8_metrics.rb +++ b/config/initializers/8_metrics.rb @@ -114,9 +114,6 @@ def instrument_classes(instrumentation) # This is a Rails scope so we have to instrument it manually. instrumentation.instrument_method(Project, :visible_to_user) - # Needed for https://gitlab.com/gitlab-org/gitlab-ce/issues/34509 - instrumentation.instrument_method(MarkupHelper, :link_to_gfm) - # Needed for https://gitlab.com/gitlab-org/gitlab-ce/issues/30224#note_32306159 instrumentation.instrument_instance_method(MergeRequestDiff, :load_commits) diff --git a/doc/administration/monitoring/performance/performance_bar.md b/doc/administration/monitoring/performance/performance_bar.md index ee680c7b258..68efe0aae5c 100644 --- a/doc/administration/monitoring/performance/performance_bar.md +++ b/doc/administration/monitoring/performance/performance_bar.md @@ -5,17 +5,17 @@ activated, it looks as follows:  -It allows you to: +It allows you to see (from left to right): -- see the current host serving the page -- see the timing of the page (backend, frontend) -- the number of DB queries, the time it took, and the detail of these queries +- the current host serving the page +- the timing of the page (backend, frontend) +- time taken and number of DB queries, click through for details of these queries  -- the number of calls to Redis, and the time it took -- the number of background jobs created by Sidekiq, and the time it took -- the number of Ruby GC calls, and the time it took -- profile the code used to generate the page, line by line +- time taken and number of calls to Redis +- time taken and number of background jobs created by Sidekiq +- profile of the code used to generate the page, line by line for either _all_, _app & lib_ , or _views_. In the profile view, the numbers in the left panel represent wall time, cpu time, and number of calls (based on [rblineprof](https://github.com/tmm1/rblineprof)).  +- time taken and number of Ruby GC calls ## Enable the Performance Bar via the Admin panel diff --git a/lib/banzai/commit_renderer.rb b/lib/banzai/commit_renderer.rb new file mode 100644 index 00000000000..f5ff95e3eb3 --- /dev/null +++ b/lib/banzai/commit_renderer.rb @@ -0,0 +1,11 @@ +module Banzai + module CommitRenderer + ATTRIBUTES = [:description, :title].freeze + + def self.render(commits, project, user = nil) + obj_renderer = ObjectRenderer.new(project, user) + + ATTRIBUTES.each { |attr| obj_renderer.render(commits, attr) } + end + end +end diff --git a/lib/banzai/object_renderer.rb b/lib/banzai/object_renderer.rb index 2196a92474c..e40556e869c 100644 --- a/lib/banzai/object_renderer.rb +++ b/lib/banzai/object_renderer.rb @@ -38,7 +38,7 @@ module Banzai objects.each_with_index do |object, index| redacted_data = redacted[index] object.__send__("redacted_#{attribute}_html=", redacted_data[:document].to_html.html_safe) # rubocop:disable GitlabSecurity/PublicSend - object.user_visible_reference_count = redacted_data[:visible_reference_count] + object.user_visible_reference_count = redacted_data[:visible_reference_count] if object.respond_to?(:user_visible_reference_count) end end diff --git a/lib/banzai/renderer.rb b/lib/banzai/renderer.rb index 95d82d17658..ceca9296851 100644 --- a/lib/banzai/renderer.rb +++ b/lib/banzai/renderer.rb @@ -36,6 +36,10 @@ module Banzai # The context to use is managed by the object and cannot be changed. # Use #render, passing it the field text, if a custom rendering is needed. def self.render_field(object, field) + unless object.respond_to?(:cached_markdown_fields) + return cacheless_render_field(object, field) + end + object.refresh_markdown_cache!(do_update: update_object?(object)) unless object.cached_html_up_to_date?(field) object.cached_html_for(field) diff --git a/lib/github/representation/branch.rb b/lib/github/representation/branch.rb index c6fa928d565..823e8e9a9c4 100644 --- a/lib/github/representation/branch.rb +++ b/lib/github/representation/branch.rb @@ -41,7 +41,7 @@ module Github def remove!(name) repository.delete_branch(name) - rescue Rugged::ReferenceError => e + rescue Gitlab::Git::Repository::DeleteBranchError => e Rails.logger.error("#{self.class.name}: Could not remove branch #{name}: #{e}") end diff --git a/lib/gitlab/access.rb b/lib/gitlab/access.rb index 4714ab18cc1..b4012ebbb99 100644 --- a/lib/gitlab/access.rb +++ b/lib/gitlab/access.rb @@ -67,10 +67,14 @@ module Gitlab def protection_values protection_options.values end + + def human_access(access) + options_with_owner.key(access) + end end def human_access - Gitlab::Access.options_with_owner.key(access_field) + Gitlab::Access.human_access(access_field) end def owner? diff --git a/lib/gitlab/git/repository.rb b/lib/gitlab/git/repository.rb index 75d4efc0bc5..efa13590a2c 100644 --- a/lib/gitlab/git/repository.rb +++ b/lib/gitlab/git/repository.rb @@ -18,6 +18,7 @@ module Gitlab InvalidBlobName = Class.new(StandardError) InvalidRef = Class.new(StandardError) GitError = Class.new(StandardError) + DeleteBranchError = Class.new(StandardError) class << self # Unlike `new`, `create` takes the storage path, not the storage name @@ -653,10 +654,16 @@ module Gitlab end # Delete the specified branch from the repository - # - # Gitaly migration: https://gitlab.com/gitlab-org/gitaly/issues/476 def delete_branch(branch_name) - rugged.branches.delete(branch_name) + gitaly_migrate(:delete_branch) do |is_enabled| + if is_enabled + gitaly_ref_client.delete_branch(branch_name) + else + rugged.branches.delete(branch_name) + end + end + rescue Rugged::ReferenceError, CommandError => e + raise DeleteBranchError, e end def delete_refs(*ref_names) @@ -681,15 +688,14 @@ module Gitlab # Examples: # create_branch("feature") # create_branch("other-feature", "master") - # - # Gitaly migration: https://gitlab.com/gitlab-org/gitaly/issues/476 def create_branch(ref, start_point = "HEAD") - rugged_ref = rugged.branches.create(ref, start_point) - target_commit = Gitlab::Git::Commit.find(self, rugged_ref.target) - Gitlab::Git::Branch.new(self, rugged_ref.name, rugged_ref.target, target_commit) - rescue Rugged::ReferenceError => e - raise InvalidRef.new("Branch #{ref} already exists") if e.to_s =~ /'refs\/heads\/#{ref}'/ - raise InvalidRef.new("Invalid reference #{start_point}") + gitaly_migrate(:create_branch) do |is_enabled| + if is_enabled + gitaly_ref_client.create_branch(ref, start_point) + else + rugged_create_branch(ref, start_point) + end + end end # Delete the specified remote from this repository. @@ -1226,6 +1232,15 @@ module Gitlab false end + def rugged_create_branch(ref, start_point) + rugged_ref = rugged.branches.create(ref, start_point) + target_commit = Gitlab::Git::Commit.find(self, rugged_ref.target) + Gitlab::Git::Branch.new(self, rugged_ref.name, rugged_ref.target, target_commit) + rescue Rugged::ReferenceError => e + raise InvalidRef.new("Branch #{ref} already exists") if e.to_s =~ /'refs\/heads\/#{ref}'/ + raise InvalidRef.new("Invalid reference #{start_point}") + end + def gitaly_copy_gitattributes(revision) gitaly_repository_client.apply_gitattributes(revision) end diff --git a/lib/gitlab/gitaly_client/ref_service.rb b/lib/gitlab/gitaly_client/ref_service.rb index a1a25cf2079..8ef873d5848 100644 --- a/lib/gitlab/gitaly_client/ref_service.rb +++ b/lib/gitlab/gitaly_client/ref_service.rb @@ -79,7 +79,7 @@ module Gitlab end def find_branch(branch_name) - request = Gitaly::DeleteBranchRequest.new( + request = Gitaly::FindBranchRequest.new( repository: @gitaly_repo, name: GitalyClient.encode(branch_name) ) @@ -92,6 +92,40 @@ module Gitlab Gitlab::Git::Branch.new(@repository, encode!(branch.name.dup), branch.target_commit.id, target_commit) end + def create_branch(ref, start_point) + request = Gitaly::CreateBranchRequest.new( + repository: @gitaly_repo, + name: GitalyClient.encode(ref), + start_point: GitalyClient.encode(start_point) + ) + + response = GitalyClient.call(@repository.storage, :ref_service, :create_branch, request) + + case response.status + when :OK + branch = response.branch + target_commit = Gitlab::Git::Commit.decorate(@repository, branch.target_commit) + Gitlab::Git::Branch.new(@repository, branch.name, branch.target_commit.id, target_commit) + when :ERR_INVALID + invalid_ref!("Invalid ref name") + when :ERR_EXISTS + invalid_ref!("Branch #{ref} already exists") + when :ERR_INVALID_START_POINT + invalid_ref!("Invalid reference #{start_point}") + else + raise "Unknown response status: #{response.status}" + end + end + + def delete_branch(branch_name) + request = Gitaly::DeleteBranchRequest.new( + repository: @gitaly_repo, + name: GitalyClient.encode(branch_name) + ) + + GitalyClient.call(@repository.storage, :ref_service, :delete_branch, request) + end + private def consume_refs_response(response) @@ -163,6 +197,10 @@ module Gitlab Gitlab::Git::Commit.decorate(@repository, hash) end + + def invalid_ref!(message) + raise Gitlab::Git::Repository::InvalidRef.new(message) + end end end end diff --git a/lib/gitlab/github_import/importer.rb b/lib/gitlab/github_import/importer.rb index 373062b354b..b8c07460ebb 100644 --- a/lib/gitlab/github_import/importer.rb +++ b/lib/gitlab/github_import/importer.rb @@ -166,7 +166,7 @@ module Gitlab def remove_branch(name) project.repository.delete_branch(name) - rescue Rugged::ReferenceError + rescue Gitlab::Git::Repository::DeleteBranchFailed errors << { type: :remove_branch, name: name } end diff --git a/spec/controllers/projects/merge_requests_controller_spec.rb b/spec/controllers/projects/merge_requests_controller_spec.rb index bb67db268fa..6775012bab5 100644 --- a/spec/controllers/projects/merge_requests_controller_spec.rb +++ b/spec/controllers/projects/merge_requests_controller_spec.rb @@ -56,6 +56,28 @@ describe Projects::MergeRequestsController do expect(response).to be_success end + + context "loads notes" do + let(:first_contributor) { create(:user) } + let(:contributor) { create(:user) } + let(:merge_request) { create(:merge_request, author: first_contributor, target_project: project, source_project: project) } + let(:contributor_merge_request) { create(:merge_request, :merged, author: contributor, target_project: project, source_project: project) } + # the order here is important + # as the controller reloads these from DB, references doesn't correspond after + let!(:first_contributor_note) { create(:note, author: first_contributor, noteable: merge_request, project: project) } + let!(:contributor_note) { create(:note, author: contributor, noteable: merge_request, project: project) } + let!(:owner_note) { create(:note, author: user, noteable: merge_request, project: project) } + + it "with special_role FIRST_TIME_CONTRIBUTOR" do + go(format: :html) + + notes = assigns(:notes) + expect(notes).to match(a_collection_containing_exactly(an_object_having_attributes(special_role: Note::SpecialRole::FIRST_TIME_CONTRIBUTOR), + an_object_having_attributes(special_role: nil), + an_object_having_attributes(special_role: nil) + )) + end + end end describe 'as json' do diff --git a/spec/features/merge_requests/diff_notes_avatars_spec.rb b/spec/features/merge_requests/diff_notes_avatars_spec.rb index e77f1f92731..ca536f2800c 100644 --- a/spec/features/merge_requests/diff_notes_avatars_spec.rb +++ b/spec/features/merge_requests/diff_notes_avatars_spec.rb @@ -139,7 +139,7 @@ feature 'Diff note avatars', js: true do end page.within find("[id='#{position.line_code(project.repository)}']") do - find('.diff-notes-collapse').click + find('.diff-notes-collapse').trigger('click') expect(page).to have_selector('img.js-diff-comment-avatar', count: 2) end @@ -152,7 +152,7 @@ feature 'Diff note avatars', js: true do page.within '.js-discussion-note-form' do find('.js-note-text').native.send_keys('Test') - find('.js-comment-button').trigger 'click' + find('.js-comment-button').trigger('click') wait_for_requests end diff --git a/spec/helpers/markup_helper_spec.rb b/spec/helpers/markup_helper_spec.rb index 70eb01c9c44..03d706062b7 100644 --- a/spec/helpers/markup_helper_spec.rb +++ b/spec/helpers/markup_helper_spec.rb @@ -52,12 +52,71 @@ describe MarkupHelper do end end - describe '#link_to_gfm' do + describe '#markdown_field' do + let(:attribute) { :title } + + describe 'with already redacted attribute' do + it 'returns the redacted attribute' do + commit.redacted_title_html = 'commit title' + + expect(Banzai).not_to receive(:render_field) + + expect(helper.markdown_field(commit, attribute)).to eq('commit title') + end + end + + describe 'without redacted attribute' do + it 'renders the markdown value' do + expect(Banzai).to receive(:render_field).with(commit, attribute).and_call_original + + helper.markdown_field(commit, attribute) + end + end + end + + describe '#link_to_markdown_field' do + let(:link) { '/commits/0a1b2c3d' } + let(:issues) { create_list(:issue, 2, project: project) } + + it 'handles references nested in links with all the text' do + allow(commit).to receive(:title).and_return("This should finally fix #{issues[0].to_reference} and #{issues[1].to_reference} for real") + + actual = helper.link_to_markdown_field(commit, :title, link) + doc = Nokogiri::HTML.parse(actual) + + # Make sure we didn't create invalid markup + expect(doc.errors).to be_empty + + # Leading commit link + expect(doc.css('a')[0].attr('href')).to eq link + expect(doc.css('a')[0].text).to eq 'This should finally fix ' + + # First issue link + expect(doc.css('a')[1].attr('href')) + .to eq project_issue_path(project, issues[0]) + expect(doc.css('a')[1].text).to eq issues[0].to_reference + + # Internal commit link + expect(doc.css('a')[2].attr('href')).to eq link + expect(doc.css('a')[2].text).to eq ' and ' + + # Second issue link + expect(doc.css('a')[3].attr('href')) + .to eq project_issue_path(project, issues[1]) + expect(doc.css('a')[3].text).to eq issues[1].to_reference + + # Trailing commit link + expect(doc.css('a')[4].attr('href')).to eq link + expect(doc.css('a')[4].text).to eq ' for real' + end + end + + describe '#link_to_markdown' do let(:link) { '/commits/0a1b2c3d' } let(:issues) { create_list(:issue, 2, project: project) } it 'handles references nested in links with all the text' do - actual = helper.link_to_gfm("This should finally fix #{issues[0].to_reference} and #{issues[1].to_reference} for real", link) + actual = helper.link_to_markdown("This should finally fix #{issues[0].to_reference} and #{issues[1].to_reference} for real", link) doc = Nokogiri::HTML.parse(actual) # Make sure we didn't create invalid markup @@ -87,7 +146,7 @@ describe MarkupHelper do end it 'forwards HTML options' do - actual = helper.link_to_gfm("Fixed in #{commit.id}", link, class: 'foo') + actual = helper.link_to_markdown("Fixed in #{commit.id}", link, class: 'foo') doc = Nokogiri::HTML.parse(actual) expect(doc.css('a')).to satisfy do |v| @@ -98,23 +157,43 @@ describe MarkupHelper do it "escapes HTML passed in as the body" do actual = "This is a <h1>test</h1> - see #{issues[0].to_reference}" - expect(helper.link_to_gfm(actual, link)) + expect(helper.link_to_markdown(actual, link)) .to match('<h1>test</h1>') end it 'ignores reference links when they are the entire body' do text = issues[0].to_reference - act = helper.link_to_gfm(text, '/foo') + act = helper.link_to_markdown(text, '/foo') expect(act).to eq %Q(<a href="/foo">#{issues[0].to_reference}</a>) end it 'replaces commit message with emoji to link' do - actual = link_to_gfm(':book: Book', '/foo') + actual = link_to_markdown(':book: Book', '/foo') expect(actual) .to eq '<gl-emoji title="open book" data-name="book" data-unicode-version="6.0">📖</gl-emoji><a href="/foo"> Book</a>' end end + describe '#link_to_html' do + it 'wraps the rendered content in a link' do + link = '/commits/0a1b2c3d' + issue = create(:issue, project: project) + + rendered = helper.markdown("This should finally fix #{issue.to_reference} for real", pipeline: :single_line) + doc = Nokogiri::HTML.parse(rendered) + + expect(doc.css('a')[0].attr('href')) + .to eq project_issue_path(project, issue) + expect(doc.css('a')[0].text).to eq issue.to_reference + + wrapped = helper.link_to_html(rendered, link) + doc = Nokogiri::HTML.parse(wrapped) + + expect(doc.css('a')[0].attr('href')).to eq link + expect(doc.css('a')[0].text).to eq 'This should finally fix ' + end + end + describe '#render_wiki_content' do before do @wiki = double('WikiPage') diff --git a/spec/helpers/notes_helper_spec.rb b/spec/helpers/notes_helper_spec.rb index 68540dd4e59..cd15e27b497 100644 --- a/spec/helpers/notes_helper_spec.rb +++ b/spec/helpers/notes_helper_spec.rb @@ -23,10 +23,10 @@ describe NotesHelper do end describe "#notes_max_access_for_users" do - it 'returns human access levels' do - expect(helper.note_max_access_for_user(owner_note)).to eq('Owner') - expect(helper.note_max_access_for_user(master_note)).to eq('Master') - expect(helper.note_max_access_for_user(reporter_note)).to eq('Reporter') + it 'returns access levels' do + expect(helper.note_max_access_for_user(owner_note)).to eq(Gitlab::Access::OWNER) + expect(helper.note_max_access_for_user(master_note)).to eq(Gitlab::Access::MASTER) + expect(helper.note_max_access_for_user(reporter_note)).to eq(Gitlab::Access::REPORTER) end it 'handles access in different projects' do @@ -34,8 +34,8 @@ describe NotesHelper do second_project.team << [master, :reporter] other_note = create(:note, author: master, project: second_project) - expect(helper.note_max_access_for_user(master_note)).to eq('Master') - expect(helper.note_max_access_for_user(other_note)).to eq('Reporter') + expect(helper.note_max_access_for_user(master_note)).to eq(Gitlab::Access::MASTER) + expect(helper.note_max_access_for_user(other_note)).to eq(Gitlab::Access::REPORTER) end end diff --git a/spec/lib/banzai/commit_renderer_spec.rb b/spec/lib/banzai/commit_renderer_spec.rb new file mode 100644 index 00000000000..049d025a5b9 --- /dev/null +++ b/spec/lib/banzai/commit_renderer_spec.rb @@ -0,0 +1,19 @@ +require 'spec_helper' + +describe Banzai::CommitRenderer do + describe '.render' do + it 'renders a commit description and title' do + user = double(:user) + project = create(:project, :repository) + + expect(Banzai::ObjectRenderer).to receive(:new).with(project, user).and_call_original + + described_class::ATTRIBUTES.each do |attr| + expect_any_instance_of(Banzai::ObjectRenderer).to receive(:render).with([project.commit], attr).once.and_call_original + expect(Banzai::Renderer).to receive(:cacheless_render_field).with(project.commit, attr) + end + + described_class.render([project.commit], project, user) + end + end +end diff --git a/spec/lib/banzai/object_renderer_spec.rb b/spec/lib/banzai/object_renderer_spec.rb index 7f5d481c36c..b172a1b718c 100644 --- a/spec/lib/banzai/object_renderer_spec.rb +++ b/spec/lib/banzai/object_renderer_spec.rb @@ -1,53 +1,77 @@ require 'spec_helper' describe Banzai::ObjectRenderer do - let(:project) { create(:project) } + let(:project) { create(:project, :repository) } let(:user) { project.owner } let(:renderer) { described_class.new(project, user, custom_value: 'value') } let(:object) { Note.new(note: 'hello', note_html: '<p dir="auto">hello</p>', cached_markdown_version: CacheMarkdownField::CACHE_VERSION) } describe '#render' do - it 'renders and redacts an Array of objects' do - renderer.render([object], :note) + context 'with cache' do + it 'renders and redacts an Array of objects' do + renderer.render([object], :note) - expect(object.redacted_note_html).to eq '<p dir="auto">hello</p>' - expect(object.user_visible_reference_count).to eq 0 - end + expect(object.redacted_note_html).to eq '<p dir="auto">hello</p>' + expect(object.user_visible_reference_count).to eq 0 + end - it 'calls Banzai::Redactor to perform redaction' do - expect_any_instance_of(Banzai::Redactor).to receive(:redact).and_call_original + it 'calls Banzai::Redactor to perform redaction' do + expect_any_instance_of(Banzai::Redactor).to receive(:redact).and_call_original - renderer.render([object], :note) - end + renderer.render([object], :note) + end - it 'retrieves field content using Banzai.render_field' do - expect(Banzai).to receive(:render_field).with(object, :note).and_call_original + it 'retrieves field content using Banzai::Renderer.render_field' do + expect(Banzai::Renderer).to receive(:render_field).with(object, :note).and_call_original - renderer.render([object], :note) - end + renderer.render([object], :note) + end - it 'passes context to PostProcessPipeline' do - another_user = create(:user) - another_project = create(:project) - object = Note.new( - note: 'hello', - note_html: 'hello', - author: another_user, - project: another_project - ) - - expect(Banzai::Pipeline::PostProcessPipeline).to receive(:to_document).with( - anything, - hash_including( - skip_redaction: true, - current_user: user, - project: another_project, + it 'passes context to PostProcessPipeline' do + another_user = create(:user) + another_project = create(:project) + object = Note.new( + note: 'hello', + note_html: 'hello', author: another_user, - custom_value: 'value' + project: another_project ) - ).and_call_original - renderer.render([object], :note) + expect(Banzai::Pipeline::PostProcessPipeline).to receive(:to_document).with( + anything, + hash_including( + skip_redaction: true, + current_user: user, + project: another_project, + author: another_user, + custom_value: 'value' + ) + ).and_call_original + + renderer.render([object], :note) + end + end + + context 'without cache' do + let(:commit) { project.commit } + + it 'renders and redacts an Array of objects' do + renderer.render([commit], :title) + + expect(commit.redacted_title_html).to eq("Merge branch 'branch-merged' into 'master'") + end + + it 'calls Banzai::Redactor to perform redaction' do + expect_any_instance_of(Banzai::Redactor).to receive(:redact).and_call_original + + renderer.render([commit], :title) + end + + it 'retrieves field content using Banzai::Renderer.cacheless_render_field' do + expect(Banzai::Renderer).to receive(:cacheless_render_field).with(commit, :title).and_call_original + + renderer.render([commit], :title) + end end end end diff --git a/spec/lib/banzai/renderer_spec.rb b/spec/lib/banzai/renderer_spec.rb index 0e094405e33..da42272bbef 100644 --- a/spec/lib/banzai/renderer_spec.rb +++ b/spec/lib/banzai/renderer_spec.rb @@ -4,6 +4,7 @@ describe Banzai::Renderer do def fake_object(fresh:) object = double('object') + allow(object).to receive(:respond_to?).with(:cached_markdown_fields).and_return(true) allow(object).to receive(:cached_html_up_to_date?).with(:field).and_return(fresh) allow(object).to receive(:cached_html_for).with(:field).and_return('field_html') @@ -12,25 +13,38 @@ describe Banzai::Renderer do describe '#render_field' do let(:renderer) { described_class } - subject { renderer.render_field(object, :field) } - context 'with a stale cache' do - let(:object) { fake_object(fresh: false) } + context 'without cache' do + let(:commit) { create(:project, :repository).commit } - it 'caches and returns the result' do - expect(object).to receive(:refresh_markdown_cache!).with(do_update: true) + it 'returns cacheless render field' do + expect(renderer).to receive(:cacheless_render_field).with(commit, :title) - is_expected.to eq('field_html') + renderer.render_field(commit, :title) end end - context 'with an up-to-date cache' do - let(:object) { fake_object(fresh: true) } + context 'with cache' do + subject { renderer.render_field(object, :field) } - it 'uses the cache' do - expect(object).to receive(:refresh_markdown_cache!).never + context 'with a stale cache' do + let(:object) { fake_object(fresh: false) } - is_expected.to eq('field_html') + it 'caches and returns the result' do + expect(object).to receive(:refresh_markdown_cache!).with(do_update: true) + + is_expected.to eq('field_html') + end + end + + context 'with an up-to-date cache' do + let(:object) { fake_object(fresh: true) } + + it 'uses the cache' do + expect(object).to receive(:refresh_markdown_cache!).never + + is_expected.to eq('field_html') + end end end end diff --git a/spec/lib/gitlab/git/repository_spec.rb b/spec/lib/gitlab/git/repository_spec.rb index 08959e7bc16..556a148c3bc 100644 --- a/spec/lib/gitlab/git/repository_spec.rb +++ b/spec/lib/gitlab/git/repository_spec.rb @@ -390,46 +390,73 @@ describe Gitlab::Git::Repository, seed_helper: true do end describe "#delete_branch" do - before(:all) do - @repo = Gitlab::Git::Repository.new('default', TEST_MUTABLE_REPO_PATH, '') - @repo.delete_branch("feature") + shared_examples "deleting a branch" do + let(:repository) { Gitlab::Git::Repository.new('default', TEST_MUTABLE_REPO_PATH, '') } + + after do + FileUtils.rm_rf(TEST_MUTABLE_REPO_PATH) + ensure_seeds + end + + it "removes the branch from the repo" do + branch_name = "to-be-deleted-soon" + + repository.create_branch(branch_name) + expect(repository.rugged.branches[branch_name]).not_to be_nil + + repository.delete_branch(branch_name) + expect(repository.rugged.branches[branch_name]).to be_nil + end + + context "when branch does not exist" do + it "raises a DeleteBranchError exception" do + expect { repository.delete_branch("this-branch-does-not-exist") }.to raise_error(Gitlab::Git::Repository::DeleteBranchError) + end + end end - it "should remove the branch from the repo" do - expect(@repo.rugged.branches["feature"]).to be_nil + context "when Gitaly delete_branch is enabled" do + it_behaves_like "deleting a branch" end - after(:all) do - FileUtils.rm_rf(TEST_MUTABLE_REPO_PATH) - ensure_seeds + context "when Gitaly delete_branch is disabled", skip_gitaly_mock: true do + it_behaves_like "deleting a branch" end end describe "#create_branch" do - before(:all) do - @repo = Gitlab::Git::Repository.new('default', TEST_MUTABLE_REPO_PATH, '') - end + shared_examples 'creating a branch' do + let(:repository) { Gitlab::Git::Repository.new('default', TEST_MUTABLE_REPO_PATH, '') } - it "should create a new branch" do - expect(@repo.create_branch('new_branch', 'master')).not_to be_nil - end + after do + FileUtils.rm_rf(TEST_MUTABLE_REPO_PATH) + ensure_seeds + end - it "should create a new branch with the right name" do - expect(@repo.create_branch('another_branch', 'master').name).to eq('another_branch') - end + it "should create a new branch" do + expect(repository.create_branch('new_branch', 'master')).not_to be_nil + end - it "should fail if we create an existing branch" do - @repo.create_branch('duplicated_branch', 'master') - expect {@repo.create_branch('duplicated_branch', 'master')}.to raise_error("Branch duplicated_branch already exists") + it "should create a new branch with the right name" do + expect(repository.create_branch('another_branch', 'master').name).to eq('another_branch') + end + + it "should fail if we create an existing branch" do + repository.create_branch('duplicated_branch', 'master') + expect {repository.create_branch('duplicated_branch', 'master')}.to raise_error("Branch duplicated_branch already exists") + end + + it "should fail if we create a branch from a non existing ref" do + expect {repository.create_branch('branch_based_in_wrong_ref', 'master_2_the_revenge')}.to raise_error("Invalid reference master_2_the_revenge") + end end - it "should fail if we create a branch from a non existing ref" do - expect {@repo.create_branch('branch_based_in_wrong_ref', 'master_2_the_revenge')}.to raise_error("Invalid reference master_2_the_revenge") + context 'when Gitaly create_branch feature is enabled' do + it_behaves_like 'creating a branch' end - after(:all) do - FileUtils.rm_rf(TEST_MUTABLE_REPO_PATH) - ensure_seeds + context 'when Gitaly create_branch feature is disabled', skip_gitaly_mock: true do + it_behaves_like 'creating a branch' end end @@ -905,7 +932,7 @@ describe Gitlab::Git::Repository, seed_helper: true do it 'should set the autocrlf option to the provided option' do @repo.autocrlf = :input - File.open(File.join(SEED_STORAGE_PATH, TEST_MUTABLE_REPO_PATH, '.git', 'config')) do |config_file| + File.open(File.join(SEED_STORAGE_PATH, TEST_MUTABLE_REPO_PATH, 'config')) do |config_file| expect(config_file.read).to match('autocrlf = input') end end @@ -977,7 +1004,7 @@ describe Gitlab::Git::Repository, seed_helper: true do context 'with local and remote branches' do let(:repository) do - Gitlab::Git::Repository.new('default', File.join(TEST_MUTABLE_REPO_PATH, '.git'), '') + Gitlab::Git::Repository.new('default', TEST_MUTABLE_REPO_PATH, '') end before do @@ -1024,7 +1051,7 @@ describe Gitlab::Git::Repository, seed_helper: true do context 'with local and remote branches' do let(:repository) do - Gitlab::Git::Repository.new('default', File.join(TEST_MUTABLE_REPO_PATH, '.git'), '') + Gitlab::Git::Repository.new('default', TEST_MUTABLE_REPO_PATH, '') end before do @@ -1230,7 +1257,7 @@ describe Gitlab::Git::Repository, seed_helper: true do describe '#local_branches' do before(:all) do - @repo = Gitlab::Git::Repository.new('default', File.join(TEST_MUTABLE_REPO_PATH, '.git'), '') + @repo = Gitlab::Git::Repository.new('default', TEST_MUTABLE_REPO_PATH, '') end after(:all) do diff --git a/spec/models/concerns/issuable_spec.rb b/spec/models/concerns/issuable_spec.rb index 37f6fd3a25b..fb5fb7daaab 100644 --- a/spec/models/concerns/issuable_spec.rb +++ b/spec/models/concerns/issuable_spec.rb @@ -480,4 +480,71 @@ describe Issuable do end end end + + describe '#first_contribution?' do + let(:group) { create(:group) } + let(:project) { create(:project, namespace: group) } + let(:other_project) { create(:project) } + let(:owner) { create(:owner) } + let(:master) { create(:user) } + let(:reporter) { create(:user) } + let(:guest) { create(:user) } + + let(:contributor) { create(:user) } + let(:first_time_contributor) { create(:user) } + + before do + group.add_owner(owner) + project.add_master(master) + project.add_reporter(reporter) + project.add_guest(guest) + project.add_guest(contributor) + project.add_guest(first_time_contributor) + end + + let(:merged_mr) { create(:merge_request, :merged, author: contributor, target_project: project, source_project: project) } + let(:open_mr) { create(:merge_request, author: first_time_contributor, target_project: project, source_project: project) } + let(:merged_mr_other_project) { create(:merge_request, :merged, author: first_time_contributor, target_project: other_project, source_project: other_project) } + + context "for merge requests" do + it "is false for MASTER" do + mr = create(:merge_request, author: master, target_project: project, source_project: project) + + expect(mr).not_to be_first_contribution + end + + it "is false for OWNER" do + mr = create(:merge_request, author: owner, target_project: project, source_project: project) + + expect(mr).not_to be_first_contribution + end + + it "is false for REPORTER" do + mr = create(:merge_request, author: reporter, target_project: project, source_project: project) + + expect(mr).not_to be_first_contribution + end + + it "is true when you don't have any merged MR" do + expect(open_mr).to be_first_contribution + expect(merged_mr).not_to be_first_contribution + end + + it "handles multiple projects separately" do + expect(open_mr).to be_first_contribution + expect(merged_mr_other_project).not_to be_first_contribution + end + end + + context "for issues" do + let(:contributor_issue) { create(:issue, author: contributor, project: project) } + let(:first_time_contributor_issue) { create(:issue, author: first_time_contributor, project: project) } + + it "is false even without merged MR" do + expect(merged_mr).to be + expect(first_time_contributor_issue).not_to be_first_contribution + expect(contributor_issue).not_to be_first_contribution + end + end + end end diff --git a/spec/support/seed_helper.rb b/spec/support/seed_helper.rb index 8731847592b..11ef1fc477f 100644 --- a/spec/support/seed_helper.rb +++ b/spec/support/seed_helper.rb @@ -41,7 +41,7 @@ module SeedHelper end def create_mutable_seeds - system(git_env, *%W(#{Gitlab.config.git.bin_path} clone #{TEST_REPO_PATH} #{TEST_MUTABLE_REPO_PATH}), + system(git_env, *%W(#{Gitlab.config.git.bin_path} clone --bare #{TEST_REPO_PATH} #{TEST_MUTABLE_REPO_PATH}), chdir: SEED_STORAGE_PATH, out: '/dev/null', err: '/dev/null') |