diff options
23 files changed, 186 insertions, 66 deletions
diff --git a/app/assets/javascripts/diff_notes/diff_notes_bundle.js b/app/assets/javascripts/diff_notes/diff_notes_bundle.js index 0863c3406bd..2f22361d6d2 100644 --- a/app/assets/javascripts/diff_notes/diff_notes_bundle.js +++ b/app/assets/javascripts/diff_notes/diff_notes_bundle.js @@ -16,7 +16,8 @@ import './components/diff_note_avatars'; import './components/new_issue_for_discussion'; $(() => { - const projectPath = document.querySelector('.merge-request').dataset.projectPath; + const projectPathHolder = document.querySelector('.merge-request') || document.querySelector('.commit-box') + const projectPath = projectPathHolder.dataset.projectPath; const COMPONENT_SELECTOR = 'resolve-btn, resolve-discussion-btn, jump-to-discussion, comment-and-resolve-btn, new-issue-for-discussion-btn'; window.gl = window.gl || {}; diff --git a/app/assets/javascripts/diff_notes/services/resolve.js b/app/assets/javascripts/diff_notes/services/resolve.js index 6eae54f830b..96fe23640af 100644 --- a/app/assets/javascripts/diff_notes/services/resolve.js +++ b/app/assets/javascripts/diff_notes/services/resolve.js @@ -43,7 +43,7 @@ class ResolveServiceClass { discussion.resolveAllNotes(resolvedBy); } - gl.mrWidget.checkStatus(); + if (gl.mrWidget) gl.mrWidget.checkStatus(); discussion.updateHeadline(data); }) .catch(() => new Flash('An error occurred when trying to resolve a discussion. Please try again.')); diff --git a/app/controllers/projects/commit_controller.rb b/app/controllers/projects/commit_controller.rb index 6ff96a3f295..2e7344b1cad 100644 --- a/app/controllers/projects/commit_controller.rb +++ b/app/controllers/projects/commit_controller.rb @@ -134,6 +134,23 @@ class Projects::CommitController < Projects::ApplicationController @grouped_diff_discussions = commit.grouped_diff_discussions @discussions = commit.discussions + if merge_request_iid = params[:merge_request_iid] + @merge_request = MergeRequestsFinder.new(current_user, project_id: @project.id).find_by(iid: merge_request_iid) + + if @merge_request + @new_diff_note_attrs.merge!( + noteable_type: 'MergeRequest', + noteable_id: @merge_request.id + ) + + merge_request_commit_notes = @merge_request.notes.where(commit_id: @commit.id).inc_relations_for_view + merge_request_commit_diff_discussions = merge_request_commit_notes.grouped_diff_discussions(@commit.diff_refs) + @grouped_diff_discussions.merge!(merge_request_commit_diff_discussions) do |line_code, left, right| + left + right + end + end + end + @notes = (@grouped_diff_discussions.values.flatten + @discussions).flat_map(&:notes) @notes = prepare_notes_for_rendering(@notes, @commit) end diff --git a/app/controllers/projects/merge_requests/diffs_controller.rb b/app/controllers/projects/merge_requests/diffs_controller.rb index 9f966889995..42a6e5be14f 100644 --- a/app/controllers/projects/merge_requests/diffs_controller.rb +++ b/app/controllers/projects/merge_requests/diffs_controller.rb @@ -20,18 +20,33 @@ class Projects::MergeRequests::DiffsController < Projects::MergeRequests::Applic private def define_diff_vars + @merge_request_diffs = @merge_request.merge_request_diffs.viewable.select_without_diff.order_id_desc + + if commit_id = params[:commit_id].presence + @commit = @merge_request.target_project.commit(commit_id) + @compare = @commit + else + @compare = find_merge_request_diff_compare + end + + return render_404 unless @compare + + @diffs = @compare.diffs(diff_options) + end + + def find_merge_request_diff_compare @merge_request_diff = - if params[:diff_id] - @merge_request.merge_request_diffs.viewable.find(params[:diff_id]) + if diff_id = params[:diff_id].presence + @merge_request.merge_request_diffs.viewable.find_by(id: diff_id) else @merge_request.merge_request_diff end - @merge_request_diffs = @merge_request.merge_request_diffs.viewable.order_id_desc + return unless @merge_request_diff + @comparable_diffs = @merge_request_diffs.select { |diff| diff.id < @merge_request_diff.id } - if params[:start_sha].present? - @start_sha = params[:start_sha] + if @start_sha = params[:start_sha].presence @start_version = @comparable_diffs.find { |diff| diff.head_commit_sha == @start_sha } unless @start_version @@ -40,20 +55,18 @@ class Projects::MergeRequests::DiffsController < Projects::MergeRequests::Applic end end - @compare = - if @start_sha - @merge_request_diff.compare_with(@start_sha) - else - @merge_request_diff - end - - @diffs = @compare.diffs(diff_options) + if @start_sha + @merge_request_diff.compare_with(@start_sha) + else + @merge_request_diff + end end def define_diff_comment_vars @new_diff_note_attrs = { noteable_type: 'MergeRequest', - noteable_id: @merge_request.id + noteable_id: @merge_request.id, + commit_id: @commit&.id } @diff_notes_disabled = false diff --git a/app/helpers/commits_helper.rb b/app/helpers/commits_helper.rb index f68e2cd3afa..361d56b211c 100644 --- a/app/helpers/commits_helper.rb +++ b/app/helpers/commits_helper.rb @@ -228,4 +228,12 @@ module CommitsHelper [commits, 0] end end + + def commit_path(project, commit, merge_request: nil) + if merge_request&.persisted? + diffs_namespace_project_merge_request_path(project.namespace, project, merge_request, commit_id: commit.id) + else + namespace_project_commit_path(project.namespace, project, commit.id) + end + end end diff --git a/app/models/concerns/discussion_on_diff.rb b/app/models/concerns/discussion_on_diff.rb index f5cbb3becad..4b4d519f3df 100644 --- a/app/models/concerns/discussion_on_diff.rb +++ b/app/models/concerns/discussion_on_diff.rb @@ -32,6 +32,10 @@ module DiscussionOnDiff first_note.position.new_path end + def on_merge_request_commit? + for_merge_request? && commit_id.present? + end + # Returns an array of at most 16 highlighted lines above a diff note def truncated_diff_lines(highlight: true) lines = highlight ? highlighted_diff_lines : diff_lines diff --git a/app/models/diff_discussion.rb b/app/models/diff_discussion.rb index 6eba87da1a1..4a65738214b 100644 --- a/app/models/diff_discussion.rb +++ b/app/models/diff_discussion.rb @@ -24,7 +24,11 @@ class DiffDiscussion < Discussion return unless for_merge_request? return {} if active? - noteable.version_params_for(position.diff_refs) + if on_merge_request_commit? + { commit_id: commit_id } + else + noteable.version_params_for(position.diff_refs) + end end def reply_attributes diff --git a/app/models/diff_note.rb b/app/models/diff_note.rb index ae5f138a920..b53d44cda95 100644 --- a/app/models/diff_note.rb +++ b/app/models/diff_note.rb @@ -17,6 +17,7 @@ class DiffNote < Note validates :noteable_type, inclusion: { in: NOTEABLE_TYPES } validate :positions_complete validate :verify_supported + validate :diff_refs_match_commit, if: :for_commit? before_validation :set_original_position, on: :create before_validation :update_position, on: :create, if: :on_text? @@ -135,6 +136,12 @@ class DiffNote < Note errors.add(:position, "is invalid") end + def diff_refs_match_commit + return if self.original_position.diff_refs == self.commit.diff_refs + + errors.add(:commit_id, 'does not match the diff refs') + end + def keep_around_commits project.repository.keep_around(self.original_position.base_sha) project.repository.keep_around(self.original_position.start_sha) diff --git a/app/models/discussion.rb b/app/models/discussion.rb index 437df923d2d..92482a1a875 100644 --- a/app/models/discussion.rb +++ b/app/models/discussion.rb @@ -11,6 +11,7 @@ class Discussion :author, :noteable, + :commit_id, :for_commit?, :for_merge_request?, diff --git a/app/models/note.rb b/app/models/note.rb index 733bbbc013f..1357e75d907 100644 --- a/app/models/note.rb +++ b/app/models/note.rb @@ -230,10 +230,14 @@ class Note < ActiveRecord::Base for_personal_snippet? end + def commit + project.commit(commit_id) if commit_id.present? + end + # override to return commits, which are not active record def noteable if for_commit? - @commit ||= project.commit(commit_id) + @commit ||= commit else super end diff --git a/app/services/system_note_service.rb b/app/services/system_note_service.rb index 30a5aab13bf..f90c6fcafb8 100644 --- a/app/services/system_note_service.rb +++ b/app/services/system_note_service.rb @@ -23,7 +23,7 @@ module SystemNoteService body = "added #{commits_text}\n\n" body << existing_commit_summary(noteable, existing_commits, oldrev) - body << new_commit_summary(new_commits).join("\n") + body << new_commit_summary(noteable, new_commits).join("\n") body << "\n\n[Compare with previous version](#{diff_comparison_url(noteable, project, oldrev)})" create_note(NoteSummary.new(noteable, project, author, body, action: 'commit', commit_count: total_count)) @@ -486,9 +486,9 @@ module SystemNoteService # new_commits - Array of new Commit objects # # Returns an Array of Strings - def new_commit_summary(new_commits) + def new_commit_summary(merge_request, new_commits) new_commits.collect do |commit| - "* #{commit.short_id} - #{escape_html(commit.title)}" + "* [#{commit.short_id}](#{merge_request_commit_url(merge_request, commit)}) - #{escape_html(commit.title)}" end end @@ -668,4 +668,13 @@ module SystemNoteService start_sha: oldrev ) end + + def merge_request_commit_url(merge_request, commit) + url_helpers.diffs_namespace_project_merge_request_url( + project.namespace, + project, + merge_request.iid, + commit_id: commit.id + ) + end end diff --git a/app/views/discussions/_discussion.html.haml b/app/views/discussions/_discussion.html.haml index 0f03163a2e8..205320ed87c 100644 --- a/app/views/discussions/_discussion.html.haml +++ b/app/views/discussions/_discussion.html.haml @@ -32,9 +32,17 @@ - elsif discussion.diff_discussion? on = conditional_link_to url.present?, url do - - unless discussion.active? - an old version of - the diff + - if discussion.on_merge_request_commit? + - unless discussion.active? + an outdated change in + commit + + %span.commit-sha= Commit.truncate_sha(discussion.commit_id) + - else + - unless discussion.active? + an old version of + the diff + = time_ago_with_tooltip(discussion.created_at, placement: "bottom", html_class: "note-created-ago") = render "discussions/headline", discussion: discussion diff --git a/app/views/projects/commit/_commit_box.html.haml b/app/views/projects/commit/_commit_box.html.haml index 5f607c2ab25..f2414d43578 100644 --- a/app/views/projects/commit/_commit_box.html.haml +++ b/app/views/projects/commit/_commit_box.html.haml @@ -47,7 +47,7 @@ %li= link_to s_("DownloadCommit|Email Patches"), project_commit_path(@project, @commit, format: :patch) %li= link_to s_("DownloadCommit|Plain Diff"), project_commit_path(@project, @commit, format: :diff) -.commit-box +.commit-box{ data: { project_path: project_path(@project) } } %h3.commit-title = markdown(@commit.title, pipeline: :single_line, author: @commit.author) - if @commit.description.present? @@ -80,3 +80,13 @@ - if last_pipeline.duration in = time_interval_in_words last_pipeline.duration + + - if @merge_request + .well-segment + = icon('info-circle fw') + + This commit is part of merge request + = succeed '.' do + = link_to @merge_request.to_reference, namespace_project_merge_request_path(@project.namespace, @project, @merge_request) + + Comments created here will be created in the context of that merge request. diff --git a/app/views/projects/commit/show.html.haml b/app/views/projects/commit/show.html.haml index abb292f8f27..2890e9d2b65 100644 --- a/app/views/projects/commit/show.html.haml +++ b/app/views/projects/commit/show.html.haml @@ -6,6 +6,9 @@ - @content_class = limited_container_width - page_title "#{@commit.title} (#{@commit.short_id})", "Commits" - page_description @commit.description +- content_for :page_specific_javascripts do + = page_specific_javascript_bundle_tag('common_vue') + = page_specific_javascript_bundle_tag('diff_notes') .container-fluid{ class: [limited_container_width, container_class] } = render "commit_box" diff --git a/app/views/projects/commits/_commit.html.haml b/app/views/projects/commits/_commit.html.haml index 1b91a94a9f8..022ded21362 100644 --- a/app/views/projects/commits/_commit.html.haml +++ b/app/views/projects/commits/_commit.html.haml @@ -1,6 +1,16 @@ -- ref = local_assigns.fetch(:ref) - -- cache_key = [project.full_path, commit.id, current_application_settings, @path.presence, current_controller?(:commits), I18n.locale] +- view_details = local_assigns.fetch(:view_details, false) +- merge_request = local_assigns.fetch(:merge_request, nil) +- project = local_assigns.fetch(:project) { merge_request&.project } +- ref = local_assigns.fetch(:ref) { merge_request&.source_branch } +- link = commit_path(project, commit, merge_request: merge_request) + +- if @note_counts + - note_count = @note_counts.fetch(commit.id, 0) +- else + - notes = commit.notes + - note_count = notes.user.count + +- cache_key = [project.full_path, commit.id, current_application_settings, note_count, @path.presence, current_controller?(:commits), merge_request, I18n.locale] - cache_key.push(commit.status(ref)) if commit.status(ref) = cache(cache_key, expires_in: 1.day) do @@ -11,7 +21,7 @@ .commit-detail .commit-content - = link_to_markdown_field(commit, :title, project_commit_path(project, commit.id), class: "commit-row-message item-title") + = link_to_markdown_field(commit, :title, link, class: "commit-row-message item-title") %span.commit-row-message.visible-xs-inline · = commit.short_id @@ -31,8 +41,7 @@ - commit_text = _('%{commit_author_link} committed %{commit_timeago}') % { commit_author_link: commit_author_link, commit_timeago: commit_timeago } #{ commit_text.html_safe } - - .commit-actions.hidden-xs + .commit-actions.flex-row.hidden-xs - if request.xhr? = render partial: 'projects/commit/signature', object: commit.signature - else @@ -41,6 +50,9 @@ - if commit.status(ref) = render_commit_status(commit, ref: ref) - = link_to commit.short_id, project_commit_path(project, commit), class: "commit-sha btn btn-transparent btn-link" + = link_to commit.short_id, link, class: "commit-sha btn btn-transparent btn-link" = clipboard_button(text: commit.id, title: _("Copy commit SHA to clipboard")) = link_to_browse_code(project, commit) + + - if view_details && merge_request + = link_to "View details", namespace_project_commit_path(project.namespace, project, commit.id, merge_request_iid: merge_request.iid), class: "btn btn-default" diff --git a/app/views/projects/commits/_commits.html.haml b/app/views/projects/commits/_commits.html.haml index d14897428d0..ac6852751be 100644 --- a/app/views/projects/commits/_commits.html.haml +++ b/app/views/projects/commits/_commits.html.haml @@ -1,4 +1,7 @@ -- ref = local_assigns.fetch(:ref) +- merge_request = local_assigns.fetch(:merge_request, nil) +- project = local_assigns.fetch(:project) { merge_request&.project } +- ref = local_assigns.fetch(:ref) { merge_request&.source_branch } + - commits, hidden = limited_commits(@commits) - commits.chunk { |c| c.committed_date.in_time_zone.to_date }.each do |day, commits| @@ -8,7 +11,7 @@ %li.commits-row{ data: { day: day } } %ul.content-list.commit-list.flex-list - = render partial: 'projects/commits/commit', collection: commits, locals: { project: project, ref: ref } + = render partial: 'projects/commits/commit', collection: commits, locals: { project: project, ref: ref, merge_request: merge_request } - if hidden > 0 %li.alert.alert-warning diff --git a/app/views/projects/merge_requests/_commits.html.haml b/app/views/projects/merge_requests/_commits.html.haml index 11793919ff7..b414518b597 100644 --- a/app/views/projects/merge_requests/_commits.html.haml +++ b/app/views/projects/merge_requests/_commits.html.haml @@ -5,4 +5,4 @@ = custom_icon ('illustration_no_commits') - else %ol#commits-list.list-unstyled - = render "projects/commits/commits", project: @merge_request.source_project, ref: @merge_request.source_branch + = render "projects/commits/commits", merge_request: @merge_request diff --git a/app/views/projects/merge_requests/diffs/_commit_widget.html.haml b/app/views/projects/merge_requests/diffs/_commit_widget.html.haml new file mode 100644 index 00000000000..2e5594f8cbe --- /dev/null +++ b/app/views/projects/merge_requests/diffs/_commit_widget.html.haml @@ -0,0 +1,5 @@ +- if @commit + .info-well.hidden-xs.prepend-top-default + .well-segment + %ul.blob-commit-info + = render 'projects/commits/commit', commit: @commit, merge_request: @merge_request, view_details: true diff --git a/app/views/projects/merge_requests/diffs/_different_base.html.haml b/app/views/projects/merge_requests/diffs/_different_base.html.haml new file mode 100644 index 00000000000..aeeaa053c5f --- /dev/null +++ b/app/views/projects/merge_requests/diffs/_different_base.html.haml @@ -0,0 +1,11 @@ +- if @merge_request_diff && different_base?(@start_version, @merge_request_diff) + .mr-version-controls + .content-block + = icon('info-circle') + Selected versions have different base commits. + Changes will include + = link_to namespace_project_compare_path(@project.namespace, @project, from: @start_version.base_commit_sha, to: @merge_request_diff.base_commit_sha) do + new commits + from + = succeed '.' do + %code= @merge_request.target_branch diff --git a/app/views/projects/merge_requests/diffs/_diffs.html.haml b/app/views/projects/merge_requests/diffs/_diffs.html.haml index 3d7a8f9d870..63f6c3e6716 100644 --- a/app/views/projects/merge_requests/diffs/_diffs.html.haml +++ b/app/views/projects/merge_requests/diffs/_diffs.html.haml @@ -1,7 +1,9 @@ -- if @merge_request_diff.collected? || @merge_request_diff.overflow? - = render 'projects/merge_requests/diffs/versions' - = render "projects/diffs/diffs", diffs: @diffs, environment: @environment, merge_request: true -- elsif @merge_request_diff.empty? += render 'projects/merge_requests/diffs/version_controls' += render 'projects/merge_requests/diffs/different_base' += render 'projects/merge_requests/diffs/not_all_comments_displayed' += render 'projects/merge_requests/diffs/commit_widget' + +- if @merge_request_diff&.empty? .nothing-here-block = image_tag 'illustrations/merge_request_changes_empty.svg' %p @@ -9,5 +11,8 @@ %strong= @merge_request.source_branch into %strong= @merge_request.target_branch - %p= link_to 'Create commit', project_new_blob_path(@project, @merge_request.source_branch), class: 'btn btn-save' +- else + - diff_viewable = @merge_request_diff ? @merge_request_diff.collected? || @merge_request_diff.overflow? : true + - if diff_viewable + = render "projects/diffs/diffs", diffs: @diffs, environment: @environment, merge_request: true diff --git a/app/views/projects/merge_requests/diffs/_not_all_comments_displayed.html.haml b/app/views/projects/merge_requests/diffs/_not_all_comments_displayed.html.haml new file mode 100644 index 00000000000..26988d34917 --- /dev/null +++ b/app/views/projects/merge_requests/diffs/_not_all_comments_displayed.html.haml @@ -0,0 +1,19 @@ +- if @commit || @start_version || (@merge_request_diff && !@merge_request_diff.latest?) + .mr-version-controls + .content-block.comments-disabled-notif + = icon('info-circle') + Not all comments are displayed because you're + = succeed '.' do + - if @commit + viewing only the changes in commit + + = link_to @commit.short_id, diffs_namespace_project_merge_request_path(@project.namespace, @project, @merge_request, commit_id: @commit.id), class: "commit-sha" + - elsif @start_version + comparing two versions of the diff + - else + viewing an old version of the diff + + .pull-right + = link_to diffs_namespace_project_merge_request_path(@project.namespace, @project, @merge_request), class: 'btn btn-sm' do + Show latest version + = "of the diff" if @commit diff --git a/app/views/projects/merge_requests/diffs/_versions.html.haml b/app/views/projects/merge_requests/diffs/_version_controls.html.haml index 9f7152b9824..1c26f0405d2 100644 --- a/app/views/projects/merge_requests/diffs/_versions.html.haml +++ b/app/views/projects/merge_requests/diffs/_version_controls.html.haml @@ -1,4 +1,4 @@ -- if @merge_request_diffs.size > 1 +- if @merge_request_diff && @merge_request_diffs.size > 1 .mr-version-controls .mr-version-menus-container.content-block Changes between @@ -71,27 +71,3 @@ (base) %div %strong.commit-sha= short_sha(@merge_request_diff.base_commit_sha) - - - if different_base?(@start_version, @merge_request_diff) - .content-block - = icon('info-circle') - Selected versions have different base commits. - Changes will include - = link_to project_compare_path(@project, from: @start_version.base_commit_sha, to: @merge_request_diff.base_commit_sha) do - new commits - from - = succeed '.' do - %code= @merge_request.target_branch - - - if @start_version || !@merge_request_diff.latest? - .comments-disabled-notif.content-block - = icon('info-circle') - Not all comments are displayed because you're - - if @start_version - comparing two versions - - else - viewing an old version - of the diff. - - .pull-right - = link_to 'Show latest version', diffs_project_merge_request_path(@project, @merge_request), class: 'btn btn-sm' diff --git a/app/views/projects/merge_requests/show.html.haml b/app/views/projects/merge_requests/show.html.haml index d88e3d794d3..4e3f6f9edc9 100644 --- a/app/views/projects/merge_requests/show.html.haml +++ b/app/views/projects/merge_requests/show.html.haml @@ -8,7 +8,7 @@ = webpack_bundle_tag('common_vue') = webpack_bundle_tag('diff_notes') -.merge-request{ 'data-mr-action': "#{j params[:tab].presence || 'show'}", 'data-url' => merge_request_path(@merge_request, format: :json), 'data-project-path' => project_path(@merge_request.project) } +.merge-request{ data: { mr_action: j(params[:tab].presence || 'show'), url: merge_request_path(@merge_request, format: :json), project_path: project_path(@merge_request.project) } } = render "projects/merge_requests/mr_title" .merge-request-details.issuable-details{ data: { id: @merge_request.project.id } } |