Merge pull request #1082 from pbendersky/gitlab-inline-comments

Added support for GitLab inline comments

Author: orta

CHANGELOG.md

@@ -19,6 +19,7 @@
   updating an old one. [@AlexDenisov](https://github.com/AlexDenisov)
   Original issue: https://github.com/danger/danger/issues/1084
 * Use `CI_API_V4_URL` on GitLab 11.7+ installations [@glensc], #1089
+* Add support for inline comments on GitLab (for versions >= 10.8.0) [@pbendersky](https://github.com/pbendersky)
 
 ## 5.14.0
 

lib/danger/comment_generators/gitlab_inline.md.erb

@@ -0,0 +1,26 @@
+<%- @tables.each do |table| -%>
+  <%- if table[:content].any? -%>
+<table data-meta="generated_by_<%= @danger_id %>">
+  <tbody>
+    <%- table[:content].each do |violation| -%>
+    <tr>
+      <td>:<%= table[:emoji] %>:</td>
+      <td width="100%" data-sticky="<%= violation.sticky %>"><%= "<del>" if table[:resolved] %><%= violation.message %><%= "</del>" if table[:resolved] %></td>
+    </tr>
+    <%- end -%>
+  </tbody>
+</table>
+  <%- end -%>
+<%- end -%>
+
+<%- @markdowns.each do |current| -%>
+<%= current %>
+<%# the previous line has to be aligned far to the left, otherwise markdown can break easily %>
+<%- end -%>
+<%# We need to add the generated_by_ to identify comments from danger. But with inlines %>
+<%# it might be a little annoying, so we set on the table, but if we have markdown we add the footer anyway %>
+<%- if @markdowns.count > 0 -%>
+<p align="right" data-meta="generated_by_<%= @danger_id %>">
+  Generated by :no_entry_sign: <a href="http://danger.systems/">Danger</a>
+</p>
+<%- end -%>

lib/danger/helpers/comment.rb

@@ -12,11 +12,19 @@ def self.from_github(comment)
     end
 
     def self.from_gitlab(comment)
-      self.new(comment.id, comment.body)
+      if comment.respond_to?(:id) && comment.respond_to?(:body)
+        self.new(comment.id, comment.body)
+      else
+        self.new(comment["id"], comment["body"])
+      end
     end
 
     def generated_by_danger?(danger_id)
       body.include?("\"generated_by_#{danger_id}\"")
     end
+
+    def inline?
+      body.include?("")
+    end
   end
 end

lib/danger/request_sources/gitlab.rb

@@ -10,6 +10,9 @@ class GitLab < RequestSource
       include Danger::Helpers::CommentsHelper
       attr_accessor :mr_json, :commits_json
 
+      FIRST_GITLAB_GEM_WITH_VERSION_CHECK = Gem::Version.new("4.6.0")
+      FIRST_VERSION_WITH_INLINE_COMMENTS = Gem::Version.new("10.8.0")
+
       def self.env_vars
         ["DANGER_GITLAB_API_TOKEN"]
       end
@@ -67,10 +70,21 @@ def base_commit
       end
 
       def mr_comments
+        # @raw_comments contains what we got back from the server.
+        # @comments contains Comment objects (that have less information)
         @comments ||= begin
-          client.merge_request_comments(ci_source.repo_slug, ci_source.pull_request_id, per_page: 100)
-            .auto_paginate
-            .map { |comment| Comment.from_gitlab(comment) }
+          if supports_inline_comments
+            @raw_comments = client.merge_request_discussions(ci_source.repo_slug, ci_source.pull_request_id)
+              .auto_paginate
+              .flat_map { |discussion| discussion.notes.map { |note| note.merge({"discussion_id" => discussion.id}) } }
+            @raw_comments
+              .map { |comment| Comment.from_gitlab(comment) }
+          else
+            @raw_comments = client.merge_request_comments(ci_source.repo_slug, ci_source.pull_request_id, per_page: 100)
+              .auto_paginate
+            @raw_comments
+              .map { |comment| Comment.from_gitlab(comment) }
+          end
         end
       end
 
@@ -107,7 +121,89 @@ def ignored_violations_from_pr
         GetIgnoredViolation.new(self.mr_json.description).call
       end
 
+      def supports_inline_comments
+        @supports_inline_comments ||= begin
+          # If we can't check GitLab's version, we assume we don't support inline comments
+          if Gem.loaded_specs["gitlab"].version < FIRST_GITLAB_GEM_WITH_VERSION_CHECK
+            false
+          else
+            current_version = Gem::Version.new(client.version.version)
+
+            current_version >= FIRST_VERSION_WITH_INLINE_COMMENTS
+          end
+        end
+      end
+
       def update_pull_request!(warnings: [], errors: [], messages: [], markdowns: [], danger_id: "danger", new_comment: false, remove_previous_comments: false)
+        if supports_inline_comments
+          update_pull_request_with_inline_comments!(warnings: warnings, errors: errors, messages: messages, markdowns: markdowns, danger_id: danger_id, new_comment: new_comment, remove_previous_comments: remove_previous_comments)
+        else
+          update_pull_request_without_inline_comments!(warnings: warnings, errors: errors, messages: messages, markdowns: markdowns, danger_id: danger_id, new_comment: new_comment, remove_previous_comments: remove_previous_comments)
+        end
+      end
+
+      def update_pull_request_with_inline_comments!(warnings: [], errors: [], messages: [], markdowns: [], danger_id: "danger", new_comment: false, remove_previous_comments: false)
+        editable_comments = mr_comments.select { |comment| comment.generated_by_danger?(danger_id) }
+
+        last_comment = editable_comments.last
+        should_create_new_comment = new_comment || last_comment.nil? || remove_previous_comments
+
+        previous_violations =
+          if should_create_new_comment
+            {}
+          else
+            parse_comment(last_comment.body)
+          end
+
+        regular_violations = regular_violations_group(
+          warnings: warnings,
+          errors: errors,
+          messages: messages,
+          markdowns: markdowns
+        )
+
+        inline_violations = inline_violations_group(
+          warnings: warnings,
+          errors: errors,
+          messages: messages,
+          markdowns: markdowns
+        )
+
+        rest_inline_violations = submit_inline_comments!({
+          danger_id: danger_id,
+          previous_violations: previous_violations
+        }.merge(inline_violations))
+
+        main_violations = merge_violations(
+          regular_violations, rest_inline_violations
+        )
+
+        main_violations_sum = main_violations.values.inject(:+)
+
+        if (previous_violations.empty? && main_violations_sum.empty?) || remove_previous_comments
+          # Just remove the comment, if there's nothing to say or --remove-previous-comments CLI was set.
+          delete_old_comments!(danger_id: danger_id)
+        end
+
+        # If there are still violations to show
+        if main_violations_sum.any?
+          body = generate_comment({
+            template: "gitlab",
+            danger_id: danger_id,
+            previous_violations: previous_violations
+          }.merge(main_violations))
+
+          comment_result =
+            if should_create_new_comment
+              client.create_merge_request_note(ci_source.repo_slug, ci_source.pull_request_id, body)
+            else
+              client.edit_merge_request_note(ci_source.repo_slug, ci_source.pull_request_id, last_comment.id, body)
+            end
+        end
+        
+      end
+
+      def update_pull_request_without_inline_comments!(warnings: [], errors: [], messages: [], markdowns: [], danger_id: "danger", new_comment: false, remove_previous_comments: false)
         editable_comments = mr_comments.select { |comment| comment.generated_by_danger?(danger_id) }
 
         should_create_new_comment = new_comment || editable_comments.empty? || remove_previous_comments
@@ -148,14 +244,21 @@ def update_pull_request!(warnings: [], errors: [], messages: [], markdowns: [],
       end
 
       def delete_old_comments!(except: nil, danger_id: "danger")
-        mr_comments.each do |comment|
+        @raw_comments.each do |raw_comment|
+
+          comment = Comment.from_gitlab(raw_comment)
           next unless comment.generated_by_danger?(danger_id)
           next if comment.id == except
-          client.delete_merge_request_comment(
-            ci_source.repo_slug,
-            ci_source.pull_request_id,
-            comment.id
-          )
+          next unless raw_comment.is_a?(Hash) && raw_comment["position"].nil?
+
+          begin
+            client.delete_merge_request_comment(
+              ci_source.repo_slug,
+              ci_source.pull_request_id,
+              comment.id
+            )
+          rescue
+          end
         end
       end
 
@@ -170,6 +273,146 @@ def file_url(organisation: nil, repository: nil, branch: nil, path: nil)
 
         "https://#{host}/#{organisation}/#{repository}/raw/#{branch}/#{path}"
       end
+
+      def regular_violations_group(warnings: [], errors: [], messages: [], markdowns: [])
+        {
+          warnings: warnings.reject(&:inline?),
+          errors: errors.reject(&:inline?),
+          messages: messages.reject(&:inline?),
+          markdowns: markdowns.reject(&:inline?)
+        }
+      end
+
+      def inline_violations_group(warnings: [], errors: [], messages: [], markdowns: [])
+        cmp = proc do |a, b|
+          next -1 unless a.file && a.line
+          next 1 unless b.file && b.line
+
+          next a.line <=> b.line if a.file == b.file
+          next a.file <=> b.file
+        end
+
+        # Sort to group inline comments by file
+        {
+          warnings: warnings.select(&:inline?).sort(&cmp),
+          errors: errors.select(&:inline?).sort(&cmp),
+          messages: messages.select(&:inline?).sort(&cmp),
+          markdowns: markdowns.select(&:inline?).sort(&cmp)
+        }
+      end
+
+      def merge_violations(*violation_groups)
+        violation_groups.inject({}) do |accumulator, group|
+          accumulator.merge(group) { |_, old, fresh| old + fresh }
+        end
+      end
+
+      def submit_inline_comments!(warnings: [], errors: [], messages: [], markdowns: [], previous_violations: [], danger_id: "danger")
+        comments = client.merge_request_discussions(ci_source.repo_slug, ci_source.pull_request_id)
+          .auto_paginate
+          .flat_map { |discussion| discussion.notes.map { |note| note.merge({"discussion_id" => discussion.id}) } }
+
+        danger_comments = comments.select { |comment| Comment.from_gitlab(comment).generated_by_danger?(danger_id) }
+        non_danger_comments = comments - danger_comments
+
+        diff_lines = []
+
+        warnings = submit_inline_comments_for_kind!(:warning, warnings, diff_lines, danger_comments, previous_violations["warning"], danger_id: danger_id)
+        errors = submit_inline_comments_for_kind!(:error, errors, diff_lines, danger_comments, previous_violations["error"], danger_id: danger_id)
+        messages = submit_inline_comments_for_kind!(:message, messages, diff_lines, danger_comments, previous_violations["message"], danger_id: danger_id)
+        markdowns = submit_inline_comments_for_kind!(:markdown, markdowns, diff_lines, danger_comments, [], danger_id: danger_id)
+
+        # submit removes from the array all comments that are still in force
+        # so we strike out all remaining ones
+        danger_comments.each do |comment|
+          violation = violations_from_table(comment["body"]).first
+          if !violation.nil? && violation.sticky
+            body = generate_inline_comment_body("white_check_mark", violation, danger_id: danger_id, resolved: true, template: "gitlab")
+            client.update_merge_request_discussion_note(ci_source.repo_slug, ci_source.pull_request_id, comment["discussion_id"], comment["id"], body)
+          else
+            # We remove non-sticky violations that have no replies
+            # Since there's no direct concept of a reply in GH, we simply consider
+            # the existance of non-danger comments in that line as replies
+            replies = non_danger_comments.select do |potential|
+              potential["path"] == comment["path"] &&
+                potential["position"] == comment["position"] &&
+                potential["commit_id"] == comment["commit_id"]
+            end
+
+            client.delete_merge_request_comment(ci_source.repo_slug, ci_source.pull_request_id, comment["id"]) if replies.empty?
+          end
+        end
+
+        {
+          warnings: warnings,
+          errors: errors,
+          messages: messages,
+          markdowns: markdowns
+        }
+      end
+
+      def submit_inline_comments_for_kind!(kind, messages, diff_lines, danger_comments, previous_violations, danger_id: "danger")
+        previous_violations ||= []
+        is_markdown_content = kind == :markdown
+        emoji = { warning: "warning", error: "no_entry_sign", message: "book" }[kind]
+
+        messages.reject do |m|
+          next false unless m.file && m.line
+
+          # position = find_position_in_diff diff_lines, m, kind
+
+          # Keep the change if it's line is not in the diff and not in dismiss mode
+          # next dismiss_out_of_range_messages_for(kind) if position.nil?
+
+          # Once we know we're gonna submit it, we format it
+          if is_markdown_content
+            body = generate_inline_markdown_body(m, danger_id: danger_id, template: "gitlab")
+          else
+            # Hide the inline link behind a span
+            m = process_markdown(m, true)
+            body = generate_inline_comment_body(emoji, m, danger_id: danger_id, template: "gitlab")
+            # A comment might be in previous_violations because only now it's part of the unified diff
+            # We remove from the array since it won't have a place in the table anymore
+            previous_violations.reject! { |v| messages_are_equivalent(v, m) }
+          end
+
+          matching_comments = danger_comments.select do |comment_data|
+            position = comment_data["position"]
+
+            if position.nil?
+              false
+            else
+              position["new_path"] == m.file && position["new_line"] == m.line
+            end
+          end
+
+          if matching_comments.empty?
+            params = {
+              body: body,
+              position: {
+                position_type: 'text',
+                new_path: m.file,
+                new_line: m.line,
+                base_sha: self.mr_json.diff_refs.base_sha,
+                start_sha: self.mr_json.diff_refs.start_sha,
+                head_sha: self.mr_json.diff_refs.head_sha
+              }
+            }
+            client.create_merge_request_discussion(ci_source.repo_slug, ci_source.pull_request_id, params)
+          else
+            # Remove the surviving comment so we don't strike it out
+            danger_comments.reject! { |c| matching_comments.include? c }
+
+            # Update the comment to remove the strikethrough if present
+            comment = matching_comments.first
+            client.update_merge_request_discussion_note(ci_source.repo_slug, ci_source.pull_request_id, comment["discussion_id"], comment["id"], body)
+          end
+
+          # Remove this element from the array
+          next true
+        end
+      end
+
     end
   end
 end

spec/fixtures/gitlab_api/merge_request_1_discussions_empty_response.json

@@ -0,0 +1,27 @@
+HTTP/1.1 200 OK
+Server: nginx
+Date: Thu, 14 Feb 2019 20:13:19 GMT
+Content-Type: application/json
+Content-Length: 7789
+Cache-Control: max-age=0, private, must-revalidate
+Etag: W/"5b77db1f9b56d74e705229cc87b3d5a6"
+Link: <https://gitlab.com/api/v4/projects/k0nserv%2Fdanger-test/merge_requests/1/discussions?id=k0nserv%2Fdanger-test&noteable_id=1&page=1&per_page=20>; rel="first", <https://gitlab.com/api/v4/projects/k0nserv%2Fdanger-test/merge_requests/1/discussions?id=k0nserv%2Fdanger-test&noteable_id=1&page=1&per_page=20>; rel="last"
+Vary: Origin
+X-Content-Type-Options: nosniff
+X-Frame-Options: SAMEORIGIN
+X-Next-Page: 
+X-Page: 1
+X-Per-Page: 20
+X-Prev-Page: 
+X-Request-Id: OCKNUgQKAc3
+X-Runtime: 0.189194
+X-Total: 9
+X-Total-Pages: 1
+Strict-Transport-Security: max-age=31536000
+RateLimit-Limit: 600
+RateLimit-Observed: 1
+RateLimit-Remaining: 599
+RateLimit-Reset: 1550175259
+RateLimit-ResetTime: Fri, 14 Feb 2019 20:14:19 GMT
+
+[]