Merge pull request #1093 from pbendersky/gitlab-inline-fixes

Fixes GitLab inline comments when the violations happen in files that…

CHANGELOG.md

@@ -10,6 +10,8 @@
 
 ## master
 
+* Fixes GitLab inline comments when violations happened in files outside of the MR diff [@pbendersky](https://github.com/pbendersky), #1092
+
 ## 5.16.0
 
 * Add support for Visual Studio App Center [@rishabhtayal](https://github.com/rishabhtayal) / [@cojoj](https://github.com/cojoj)

lib/danger/request_sources/gitlab.rb

@@ -90,11 +90,26 @@ def mr_comments
 
       def mr_diff
         @mr_diff ||= begin
-          client.merge_request_changes(ci_source.repo_slug, ci_source.pull_request_id)
+          mr_changes
             .changes.map { |change| change["diff"] }.join("\n")
         end
       end
 
+      def mr_changed_paths
+        @mr_changed_paths ||= begin
+          mr_changes
+            .changes.map { |change| change["new_path"] }
+        end
+
+        @mr_changed_paths
+      end
+
+      def mr_changes
+        @mr_changes ||= begin
+          client.merge_request_changes(ci_source.repo_slug, ci_source.pull_request_id)
+        end
+      end
+
       def setup_danger_branches
         base_branch = self.mr_json.source_branch
         head_branch = self.mr_json.target_branch
@@ -359,10 +374,8 @@ def submit_inline_comments_for_kind!(kind, messages, diff_lines, danger_comments
         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?
+          # Keep the change it's in a file changed in this diff
+          next if !mr_changed_paths.include?(m.file)
 
           # Once we know we're gonna submit it, we format it
           if is_markdown_content

spec/fixtures/gitlab_api/merge_request_1_changes_response.json

@@ -9,4 +9,4 @@ Vary: Origin
 X-Request-Id: S3VHro2VCE8
 X-Runtime: 0.076161
 
-{"id":593728,"iid":1,"project_id":1342007,"title":"Add a","description":"The descriptions is here\r\n\r\n\u003e Danger: ignore \"Developer specific files shouldn't be changed\"\r\n\r\n\u003e Danger: ignore \"Testing\"","state":"opened","created_at":"2016-06-27T11:04:02.114Z","updated_at":"2016-09-19T20:25:31.077Z","merged_by":null,"merged_at":null,"closed_by":null,"closed_at":null,"target_branch":"master","source_branch":"mr-test","upvotes":0,"downvotes":0,"author":{"id":483414,"name":"Hugo Tunius","username":"k0nserv","state":"active","avatar_url":"https://secure.gravatar.com/avatar/7300342f996dcc9d4e22418cc9a70b14?s=80\u0026d=identicon","web_url":"https://gitlab.com/k0nserv"},"assignee":null,"source_project_id":1342007,"target_project_id":1342007,"labels":["test-label"],"work_in_progress":false,"milestone":null,"merge_when_pipeline_succeeds":false,"merge_status":"can_be_merged","sha":"04e58de1fa97502d7e28c1394d471bb8fb1fc4a8","merge_commit_sha":null,"user_notes_count":2,"discussion_locked":null,"should_remove_source_branch":null,"force_remove_source_branch":null,"web_url":"https://gitlab.com/k0nserv/danger-test/merge_requests/1","time_stats":{"time_estimate":0,"total_time_spent":0,"human_time_estimate":null,"human_total_time_spent":null},"squash":false,"subscribed":false,"changes_count":"3","latest_build_started_at":null,"latest_build_finished_at":null,"first_deployed_to_production_at":null,"pipeline":null,"diff_refs":{"base_sha":"0e4db308b6579f7cc733e5a354e026b272e1c076","head_sha":"04e58de1fa97502d7e28c1394d471bb8fb1fc4a8","start_sha":"0e4db308b6579f7cc733e5a354e026b272e1c076"},"merge_error":null,"changes":[{"old_path":"Dangerfile","new_path":"Dangerfile","a_mode":"100644","b_mode":"100644","new_file":false,"renamed_file":false,"deleted_file":false,"diff":"--- a/Dangerfile\n+++ b/Dangerfile\n@@ -1,13 +1,11 @@\n # Sometimes it's a README fix, or something like that - which isn't relevant for\n # including in a project's CHANGELOG for example\n-declared_trivial = pr_title.include? \"#trivial\"\n+declared_trivial = gitlab.pr_title.include? \"#trivial\"\n \n # Make it more obvious that a PR is a work in progress and shouldn't be merged yet\n-warn(\"PR is classed as Work in Progress\") if pr_title.include? \"[WIP]\"\n+warn(\"PR is classed as Work in Progress\") if gitlab.pr_title.include? \"[WIP]\"\n \n-# Warn when there is a big PR\n-warn(\"Big PR\") if lines_of_code \u003e 500\n+warn(\"Test warning\", sticky: false)\n+fail(\"Test error\")\n+message(\"Test message\", sticky: false)\n \n-# Don't let testing shortcuts get into master by accident\n-fail(\"fdescribe left in tests\") if `grep -r fdescribe specs/`.length \u003e 1\n-fail(\"fit left in tests\") if `grep -r \"fit specs/ `.length \u003e 1\n"},{"old_path":"a","new_path":"a","a_mode":"0","b_mode":"100644","new_file":true,"renamed_file":false,"deleted_file":false,"diff":"--- /dev/null\n+++ b/a\n@@ -0,0 +1 @@\n+Danger rocks!\n"},{"old_path":"b","new_path":"b","a_mode":"0","b_mode":"100644","new_file":true,"renamed_file":false,"deleted_file":false,"diff":"--- /dev/null\n+++ b/b\n@@ -0,0 +1 @@\n+Test message please ignore\n"}],"approvals_before_merge":null}XWing-467:danger Pablo$ 
+{"id":593728,"iid":1,"project_id":1342007,"title":"Add a","description":"The descriptions is here\r\n\r\n\u003e Danger: ignore \"Developer specific files shouldn't be changed\"\r\n\r\n\u003e Danger: ignore \"Testing\"","state":"opened","created_at":"2016-06-27T11:04:02.114Z","updated_at":"2016-09-19T20:25:31.077Z","merged_by":null,"merged_at":null,"closed_by":null,"closed_at":null,"target_branch":"master","source_branch":"mr-test","upvotes":0,"downvotes":0,"author":{"id":483414,"name":"Hugo Tunius","username":"k0nserv","state":"active","avatar_url":"https://secure.gravatar.com/avatar/7300342f996dcc9d4e22418cc9a70b14?s=80\u0026d=identicon","web_url":"https://gitlab.com/k0nserv"},"assignee":null,"source_project_id":1342007,"target_project_id":1342007,"labels":["test-label"],"work_in_progress":false,"milestone":null,"merge_when_pipeline_succeeds":false,"merge_status":"can_be_merged","sha":"04e58de1fa97502d7e28c1394d471bb8fb1fc4a8","merge_commit_sha":null,"user_notes_count":2,"discussion_locked":null,"should_remove_source_branch":null,"force_remove_source_branch":null,"web_url":"https://gitlab.com/k0nserv/danger-test/merge_requests/1","time_stats":{"time_estimate":0,"total_time_spent":0,"human_time_estimate":null,"human_total_time_spent":null},"squash":false,"subscribed":false,"changes_count":"3","latest_build_started_at":null,"latest_build_finished_at":null,"first_deployed_to_production_at":null,"pipeline":null,"diff_refs":{"base_sha":"0e4db308b6579f7cc733e5a354e026b272e1c076","head_sha":"04e58de1fa97502d7e28c1394d471bb8fb1fc4a8","start_sha":"0e4db308b6579f7cc733e5a354e026b272e1c076"},"merge_error":null,"changes":[{"old_path":"Dangerfile","new_path":"Dangerfile","a_mode":"100644","b_mode":"100644","new_file":false,"renamed_file":false,"deleted_file":false,"diff":"--- a/Dangerfile\n+++ b/Dangerfile\n@@ -1,13 +1,11 @@\n # Sometimes it's a README fix, or something like that - which isn't relevant for\n # including in a project's CHANGELOG for example\n-declared_trivial = pr_title.include? \"#trivial\"\n+declared_trivial = gitlab.pr_title.include? \"#trivial\"\n \n # Make it more obvious that a PR is a work in progress and shouldn't be merged yet\n-warn(\"PR is classed as Work in Progress\") if pr_title.include? \"[WIP]\"\n+warn(\"PR is classed as Work in Progress\") if gitlab.pr_title.include? \"[WIP]\"\n \n-# Warn when there is a big PR\n-warn(\"Big PR\") if lines_of_code \u003e 500\n+warn(\"Test warning\", sticky: false)\n+fail(\"Test error\")\n+message(\"Test message\", sticky: false)\n \n-# Don't let testing shortcuts get into master by accident\n-fail(\"fdescribe left in tests\") if `grep -r fdescribe specs/`.length \u003e 1\n-fail(\"fit left in tests\") if `grep -r \"fit specs/ `.length \u003e 1\n"},{"old_path":"a","new_path":"a","a_mode":"0","b_mode":"100644","new_file":true,"renamed_file":false,"deleted_file":false,"diff":"--- /dev/null\n+++ b/a\n@@ -0,0 +1 @@\n+Danger rocks!\n"},{"old_path":"b","new_path":"b","a_mode":"0","b_mode":"100644","new_file":true,"renamed_file":false,"deleted_file":false,"diff":"--- /dev/null\n+++ b/b\n@@ -0,0 +1 @@\n+Test message please ignore\n"}],"approvals_before_merge":null}

spec/lib/danger/request_sources/gitlab_spec.rb

@@ -331,12 +331,18 @@
         end
 
         it "adds new comments inline" do
+          stub_merge_request_changes(
+            "merge_request_1_changes_response",
+            "k0nserv\%2Fdanger-test",
+            1
+          )
+
           expect(subject.client).to receive(:create_merge_request_discussion)
           allow(subject.client).to receive(:delete_merge_request_comment)
 
           subject.fetch_details
 
-          v = Danger::Violation.new("Sure thing", true, "CHANGELOG.md", 4)
+          v = Danger::Violation.new("Sure thing", true, "a", 4)
           subject.update_pull_request!(warnings: [], errors: [], messages: [v])
         end
 
@@ -346,6 +352,11 @@
             "k0nserv%2Fdanger-test",
             1
           )
+          stub_merge_request_changes(
+            "merge_request_1_changes_response",
+            "k0nserv\%2Fdanger-test",
+            1
+          )
 
           v = Danger::Violation.new("Updated danger comment", true, "a", 1)
           body = subject.generate_inline_comment_body("warning", subject.process_markdown(v, true), danger_id: "danger", template: "gitlab")
@@ -391,6 +402,29 @@
           v = Danger::Violation.new("Test error", true)
           subject.update_pull_request!(warnings: [], errors: [v], messages: [])
         end
+
+        it "skips inline comments for files that are not part of the diff" do
+          stub_merge_request_discussions(
+            "merge_request_1_discussions_response",
+            "k0nserv%2Fdanger-test",
+            1
+          )
+          stub_merge_request_changes(
+            "merge_request_1_changes_response",
+            "k0nserv\%2Fdanger-test",
+            1
+          )
+
+          allow(subject.client).to receive(:update_merge_request_discussion_note)
+          allow(subject.client).to receive(:delete_merge_request_comment)
+          allow(subject.client).to receive(:edit_merge_request_note)
+
+          v = Danger::Violation.new("Error on not-on-diff file", true, "not-on-diff", 1)
+
+          subject.fetch_details
+
+          subject.update_pull_request!(warnings: [v], errors: [], messages: [])
+        end
 
       end
     end