Merge pull request #457 from inaka/harenson.449.use-the-new-pr-review…

…-feature

[Fix #449] Use the new GitHub's PR review feature

README.md

@@ -69,8 +69,10 @@ found [here](https://github.com/inaka/elvis/wiki/Default-Rules-in-Elvis-Webhook)
 #### Running the webhook on your servers
 
 Since GitHub's API needs a valid user and password to allow the creation of
-comments on PRs, the parameters `github_user` and `github_password` need to be
-added to `elvis`'s [configuration](#configuration).
+reviews on PRs, the parameters `github_user` and `github_password` need to be
+added to `elvis`'s [configuration](#configuration) and also the credentials used
+must be from an admin of the repo or someone with permissions for requesting changes
+on PRs.
 
 The `webhook/1` function takes a map containing the keys `headers` and `body`,
 whose values should be the map of headers and the body from the GitHub's event

config/test.config

@@ -21,7 +21,10 @@
        }
      ]
     },
-    {output_format, plain}
+    {output_format, plain},
+    %% Basic Auth
+    {github_user, ""},
+    {github_password, ""}
    ]
  }
 ].

rebar.config

@@ -67,7 +67,7 @@
 
 {deps, [ {elvis, "0.3.8", {pkg, elvis_core}}
        , {getopt,  "0.8.2"}
-       , {egithub, "0.3.2"}
+       , {egithub, "0.5.1"}
        , {katana_code, "0.1.0"}
        ]}.
 

rebar.lock

@@ -1,10 +1,10 @@
 {"1.1.0",
 [{<<"aleppo">>,{pkg,<<"inaka_aleppo">>,<<"1.0.0">>},1},
  {<<"certifi">>,{pkg,<<"certifi">>,<<"0.7.0">>},2},
- {<<"egithub">>,{pkg,<<"egithub">>,<<"0.3.2">>},0},
+ {<<"egithub">>,{pkg,<<"egithub">>,<<"0.5.1">>},0},
  {<<"elvis">>,{pkg,<<"elvis_core">>,<<"0.3.8">>},0},
  {<<"getopt">>,{pkg,<<"getopt">>,<<"0.8.2">>},0},
- {<<"goldrush">>,{pkg,<<"goldrush">>,<<"0.1.9">>},2},
+ {<<"goldrush">>,{pkg,<<"goldrush">>,<<"0.1.9">>},1},
  {<<"hackney">>,{pkg,<<"hackney">>,<<"1.6.5">>},1},
  {<<"idna">>,{pkg,<<"idna">>,<<"1.2.0">>},2},
  {<<"jiffy">>,{pkg,<<"jiffy">>,<<"0.14.11">>},1},
@@ -18,7 +18,7 @@
 {pkg_hash,[
  {<<"aleppo">>, <<"8DB14CF16BB8C263C14FF4C3F69F64D7C849D40888944F4204D2CA74F1114CEB">>},
  {<<"certifi">>, <<"861A57F3808F7EB0C2D1802AFEAAE0FA5DE813B0DF0979153CBAFCD853ABABAF">>},
- {<<"egithub">>, <<"104088B9BBEF7B637A6A2F8DAF59A6201B5FA18EC86F10B4739CDEFF7D0BD492">>},
+ {<<"egithub">>, <<"C8BDB55F4D9EF610535D62CC96A43D974BE8D5924C5507218A25B1DF608788ED">>},
  {<<"elvis">>, <<"9FE9DBFF15150DB41110D86BD87F29A7269A5CC3C0984EF47C1D208FBBD24222">>},
  {<<"getopt">>, <<"B17556DB683000BA50370B16C0619DF1337E7AF7ECBF7D64FBF8D1D6BCE3109B">>},
  {<<"goldrush">>, <<"F06E5D5F1277DA5C413E84D5A2924174182FB108DABB39D5EC548B27424CD106">>},

src/elvis_webhook.erl

@@ -22,13 +22,11 @@ event(Cred, Request) -> egithub_webhook:event(?MODULE, Cred, Request).
 -spec handle_pull_request(
   egithub:credentials(), egithub_webhook:req_data(),
   [egithub_webhook:file()]) ->
-  {ok, [egithub_webhook:message()]} | {error, term()}.
+  {ok, egithub_webhook:review()} | {error, term()}.
 handle_pull_request(Cred, Data, GithubFiles) ->
     #{<<"repository">> := Repository} = Data,
-    BranchName = maps_get([ <<"pull_request">>,
-                            <<"head">>,
-                            <<"ref">>],
-                            Data, <<"master">>),
+    BranchName =
+      maps_get([<<"pull_request">>, <<"head">>, <<"ref">>], Data, <<"master">>),
     Repo = binary_to_list(maps:get(<<"full_name">>, Repository)),
     Branch = binary_to_list(BranchName),
     Config = repo_config(Cred, Repo, Branch, elvis_config:default()),
@@ -39,10 +37,17 @@ handle_pull_request(Cred, Data, GithubFiles) ->
 
     FileInfoFun = fun (File) -> file_info(Cred, Repo, File) end,
     Config2 = elvis_config:apply_to_files(FileInfoFun, Config1),
+    CommitId =
+      maps_get([<<"pull_request">>, <<"head">>, <<"sha">>], Data, <<>>),
 
     case elvis_core:rock(Config2) of
-        {fail, Results} -> {ok, messages_from_results(Results)};
-        ok -> {ok, []}
+        {fail, Results} ->
+            {ok, review_from_results(Results, CommitId)};
+        ok ->
+            {ok, #{commit_id => CommitId,
+                   body => <<":+1:">>,
+                   event => <<"APPROVE">>,
+                   comments => []}}
     end.
 
 -spec handle_error( {error, term()}
@@ -54,6 +59,23 @@ handle_error({error, _}, _, _) -> {ok, [], []}.
 %%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%
 %%% Helper functions
 
+-spec review_from_results([elvis_result:file()], string()) ->
+    egithub_webhook:review().
+review_from_results(Results, CommitId) ->
+    RC = review_comments_from_results(Results),
+    %% Comments with `position = 0' cannot be added as a review comment, so they
+    %% are added to the review's body.
+    Fun = fun(#{position := 0, body := B}, {BodyAcc, CommentsAcc}) ->
+              {<<BodyAcc/binary, "\n- ", B/binary>>, CommentsAcc};
+             (Comment, {BodyAcc, CommentsAcc}) ->
+              {BodyAcc, [Comment | CommentsAcc]}
+          end,
+    {Body, ReviewComments} = lists:foldl(Fun, {<<>>, []}, RC),
+    #{commit_id => CommitId,
+      body => Body,
+      event => <<"REQUEST_CHANGES">>,
+      comments => ReviewComments}.
+
 -spec github_credentials() -> egithub:credentials().
 github_credentials() ->
     User = application:get_env(elvis, github_user, ""),
@@ -88,47 +110,45 @@ commit_id_from_raw_url(Url, Filename) ->
     {match, [_, {Pos, Len} | _]} = re:run(UrlString, Regex),
     string:substr(UrlString, Pos + 1, Len).
 
-messages_from_results(Results) ->
+review_comments_from_results(Results) ->
     lists:flatmap(
         fun(Result) ->
-            messages_from_result(Result)
+            review_comments_from_result(Result)
         end, Results).
 
-messages_from_result(Result) ->
+review_comments_from_result(Result) ->
     File = elvis_result:get_file(Result),
     Rules = elvis_result:get_rules(Result),
     lists:flatmap(
         fun(Rule) ->
-            messages_from_result(Rule, File)
+            review_comments_from_result(Rule, File)
         end, Rules).
 
-messages_from_result(Rule, File) ->
+review_comments_from_result(Rule, File) ->
     Items = elvis_result:get_items(Rule),
     lists:flatmap(
         fun(Item) ->
-            messages_from_item(Item, File)
+            review_comments_from_item(Item, File)
         end, Items).
 
-messages_from_item(Item, File) ->
-    #{path := Path,
-      commit_id := CommitId,
-      patch := Patch} = File,
+review_comments_from_item(Item, File) ->
+    #{path := Path, patch := Patch} = File,
     Message = elvis_result:get_message(Item),
     Info = elvis_result:get_info(Item),
     Line = elvis_result:get_line_num(Item),
     Text = list_to_binary(io_lib:format(Message, Info)),
 
     case Line of
         0 ->
-            [#{text => Text,
-               position => Line}];
+            [#{path => Path,
+               position => Line,
+               body => Text}];
         _ ->
             case elvis_git:relative_position(Patch, Line) of
                 {ok, Position} ->
-                    [ #{commit_id => CommitId,
-                        path      => Path,
+                    [ #{path      => Path,
                         position  => Position,
-                        text      => Text
+                        body      => Text
                        }
                     ];
                 not_found ->