Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

create GenServer process to merge errors together #134

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions config/dev.exs
Original file line number Diff line number Diff line change
Expand Up @@ -43,3 +43,4 @@ config :dwylbot, Dwylbot.Repo,
pool_size: 10

config :dwylbot, :github_api, Dwylbot.GithubAPI.HTTPClient
config :dwylbot, :time_merge_errors, 5000
1 change: 1 addition & 0 deletions config/prod.exs
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,7 @@ config :dwylbot, Dwylbot.Repo,
ssl: true

config :dwylbot, :github_api, Dwylbot.GithubAPI.HTTPClient
config :dwylbot, :time_merge_errors, 5000

# ## SSL Support
#
Expand Down
1 change: 1 addition & 0 deletions config/test.exs
Original file line number Diff line number Diff line change
Expand Up @@ -19,3 +19,4 @@ config :dwylbot, Dwylbot.Repo,
pool: Ecto.Adapters.SQL.Sandbox

config :dwylbot, :github_api, Dwylbot.GithubAPI.InMemory
config :dwylbot, :time_merge_errors, 500
1 change: 1 addition & 0 deletions lib/dwylbot.ex
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@ defmodule Dwylbot do
supervisor(Dwylbot.Endpoint, []),
# Start your own worker by calling: Dwylbot.Worker.start_link(arg1, arg2, arg3)
# worker(Dwylbot.Worker, [arg1, arg2, arg3]),
worker(Dwylbot.MergeErrors, [[]])
]

# See http://elixir-lang.org/docs/stable/elixir/Supervisor.html
Expand Down
60 changes: 60 additions & 0 deletions test/controllers/join_rules_test.exs
Original file line number Diff line number Diff line change
@@ -0,0 +1,60 @@
defmodule Dwylbot.JoinRulesTest do
alias Dwylbot.Rules.Join
use ExUnit.Case
doctest Dwylbot.Rules.Join.EstimationAssignee, import: true

test "Join errors" do
errors = [
%{
actions: [
%{comment: "conment_1",
url: "https://api.github.com/repos/SimonLab/github_app/issues/26/comments"
}
],
error_type: "issue_no_estimation",
id: "SimonLab/github_app/26",
token: 42,
},
%{
actions: [
%{comment: "conment_2",
url: "https://api.github.com/repos/SimonLab/github_app/issues/26/comments"
}],
error_type: "issue_inprogress_noassignees",
id: "SimonLab/github_app/26",
token: 42,
},
%{
actions: [
%{comment: "conment_3",
url: "https://api.github.com/repos/SimonLab/github_app/issues/25/comments"
}],
error_type: "another_error",
id: nil,
token: 42,
},
]

expected = [
%{
actions: [
%{comment: "conment_2\n\nconment_1\n",
url: "https://api.github.com/repos/SimonLab/github_app/issues/26/comments"
}
],
token: 42,
},
%{
actions: [
%{comment: "conment_3",
url: "https://api.github.com/repos/SimonLab/github_app/issues/25/comments"
}],
error_type: "another_error",
id: nil,
token: 42,
},
]

assert expected == Join.join(errors)
end
end
20 changes: 20 additions & 0 deletions test/controllers/merge_errors_test.exs
Original file line number Diff line number Diff line change
@@ -0,0 +1,20 @@
defmodule Dwylbot.MergeErrorsTest do
use ExUnit.Case

test "send error" do
error = %{
id: nil,
token: 42,
error_type: "issue_inprogress_noassignees",
actions: [
%{
comment: "test comment",
url: ""
},
]
}
assert :ok == Dwylbot.MergeErrors.send_error(error)
Process.sleep(1000)
end

end
46 changes: 46 additions & 0 deletions web/controllers/processes/merge_errors.ex
Original file line number Diff line number Diff line change
@@ -0,0 +1,46 @@
defmodule Dwylbot.MergeErrors do
@moduledoc """
GenServer, receive, merge and reports errors every 5s
"""
use GenServer
alias Dwylbot.Rules.Join
@github_api Application.get_env(:dwylbot, :github_api)
@time Application.get_env(:dwylbot, :time_merge_errors)

def start_link(errors) do
GenServer.start_link(__MODULE__, errors, name: __MODULE__)
end

def init(errors) do
process_errors()
{:ok, errors}
end

def process_errors do
Process.send(__MODULE__, :errors, [])
end

def send_error(error) do
GenServer.cast(__MODULE__, {:error, error})
end

def handle_info(:errors, errors_state) do
GenServer.cast(__MODULE__, {:report_errors, errors_state})
Process.send_after(__MODULE__, :errors, @time)
{:noreply, []}
end

def handle_cast({:report_errors, errors}, errors_state) do
joined_errors = Join.join(errors)
joined_errors
|> Enum.each(fn(err) ->
@github_api.report_error(err.token, err)
end)

{:noreply, errors_state}
end

def handle_cast({:error, error}, errors_state) do
{:noreply, [error | errors_state]}
end
end
8 changes: 5 additions & 3 deletions web/controllers/processes/wait.ex
Original file line number Diff line number Diff line change
Expand Up @@ -2,17 +2,19 @@ defmodule Dwylbot.WaitProcess do
@moduledoc """
The delay function is used to create new checking rules processes
"""
alias Dwylbot.MergeErrors
alias Dwylbot.Rules
@github_api Application.get_env(:dwylbot, :github_api)

def delay(error, payload, event_type, token) do
Process.sleep(error.wait)
error_token = Map.put(error, :token, token)

if error.verify do
check_errors = Rules.check_errors(payload, event_type, token)
Rules.any_error?(check_errors, error)
&& @github_api.report_error(token, error)
&& MergeErrors.send_error(error_token)
else
@github_api.report_error(token, error)
MergeErrors.send_error(error_token)
end
end
end
20 changes: 20 additions & 0 deletions web/controllers/rules/helpers.ex
Original file line number Diff line number Diff line change
Expand Up @@ -30,4 +30,24 @@ defmodule Dwylbot.Rules.Helpers do
|> Enum.map(&(&1["name"]))
|> Enum.member?(label)
end

@doc """
iex>get_id(%{"repository" => %{"full_name" => "repo"}, "issue" => %{"number" => 42}})
"repo/42"
"""
def get_id(payload) when is_map(payload)do
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice use of guard 👍

if Map.has_key?(payload, "repository") && Map.has_key?(payload, "issue") do
"#{payload["repository"]["full_name"]}/#{payload["issue"]["number"]}"
else
nil
end
end

@doc """
iex>get_id([1,2,3])
nil
"""
def get_id(_payload) do
nil
end
end
1 change: 1 addition & 0 deletions web/controllers/rules/issue/inprogress.ex
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@ defmodule Dwylbot.Rules.Issue.Inprogress do
in_progress = Enum.any?(labels, fn(l) -> l["name"] == "in-progress" end)
if in_progress && Enum.empty?(assignees) do
%{
id: Helpers.get_id(payload),
error_type: @rule_name,
actions: [
%{
Expand Down
1 change: 1 addition & 0 deletions web/controllers/rules/issue/no_description.ex
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@ defmodule Dwylbot.Rules.Issue.NoDescription do
description = String.trim payload["issue"]["body"]
if String.length(description) == 0 do
%{
id: Helpers.get_id(payload),
error_type: @rule_name,
actions: [
%{
Expand Down
1 change: 1 addition & 0 deletions web/controllers/rules/issue/noassignees.ex
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@ defmodule Dwylbot.Rules.Issue.Noassignees do
in_progress = Enum.any?(labels, fn(l) -> l["name"] == "in-progress" end)
if in_progress && Enum.empty?(assignees) do
%{
id: Helpers.get_id(payload),
error_type: @rule_name,
actions: [
%{
Expand Down
1 change: 1 addition & 0 deletions web/controllers/rules/issue/time_estimation.ex
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@ defmodule Dwylbot.Rules.Issue.TimeEstimation do
|> Enum.reduce(false, fn(x, acc) -> x || acc end)
if in_progress && !estimation do
%{
id: Helpers.get_id(payload),
error_type: @rule_name,
actions: [
%{
Expand Down
115 changes: 115 additions & 0 deletions web/controllers/rules/join/estimation_assignee.ex
Original file line number Diff line number Diff line change
@@ -0,0 +1,115 @@
defmodule Dwylbot.Rules.Join.EstimationAssignee do
@moduledoc """
This module merge the no assignees and no estimation errors together:
- create and add a new error with a new comment action
- delete the two original errors from the list of errors
"""

def noassignee_noestimation(errors) do
no_assignee = get_error_by_type(errors, "issue_inprogress_noassignees")
no_estimation = get_error_by_type(errors, "issue_no_estimation")

merge_errors? = no_assignee != nil
&& no_estimation != nil
&& no_assignee.id == no_estimation.id
&& no_assignee.id != nil

if merge_errors? do
actions = filter_actions(no_estimation.actions)
++ filter_actions(no_assignee.actions)

comment_action = merge_comment_actions(no_assignee, no_estimation)

new_error = %{
token: no_assignee.token,
actions: [comment_action| actions]
}

replace_errors(errors, no_assignee, no_estimation, new_error)
else
errors
end
end

@doc """
iex>get_error_by_type([%{error_type: "type_1"}], "no_match")
nil
iex>get_error_by_type([%{error_type: "type_1"}], "type_1")
%{error_type: "type_1"}
"""
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

^^^^ I like this. Very helpful 👍

def get_error_by_type(errors, error_type) do
errors
|> Enum.find(fn(err) ->
err.error_type == error_type
end)
end

@doc """
iex>get_comment_action(%{actions: [%{assgin: "bob"}]})
%{comment: "", url: ""}
iex>get_comment_action(%{actions: [
...>%{assign: "bob"},
...>%{comment: "hello", url: "/comment"}]})
%{comment: "hello", url: "/comment"}
"""
def get_comment_action(error) do
error.actions
|> Enum.find(%{comment: "", url: ""}, fn(action) ->
Map.has_key?(action, :comment)
end)
end

@doc """
iex>merge_comment_actions(
...>%{actions: [%{comment: "comment 1", url: "/comment"}]},
...>%{actions: [%{comment: "comment 2", url: "/comment"}]}
...>)
%{comment: "comment 1\\n\\ncomment 2\\n", url: "/comment"}
"""
def merge_comment_actions(err_1, err_2) do
action_comment_1 = get_comment_action(err_1)
action_comment_2 = get_comment_action(err_2)

comment = """
#{action_comment_1.comment}

#{action_comment_2.comment}
"""
%{
comment: comment,
url: action_comment_1.url
}
end

@doc """
iex>replace_errors(
...>[%{err: "err 1"}, %{err: "err 2"}, %{err: "err 3"}],
...>%{err: "err 1"},
...>%{err: "err 2"},
...>%{err: "new error"}
...>)
[%{err: "new error"}, %{err: "err 3"}]
"""
def replace_errors(errors, err_1, err_2, new_error) do
delete_errors = errors
|> List.delete(err_1)
|> List.delete(err_2)

[new_error | delete_errors]
end

@doc """
iex>filter_actions([
...>%{comment: "comment acction", url: "/comment" },
...>%{assign: "bob", url: "/assign"}
...>])
[%{assign: "bob", url: "/assign"}]
"""
def filter_actions(actions) do
actions
|> Enum.filter(fn(action) ->
!Map.has_key?(action, :comment)
end)
end

end
13 changes: 13 additions & 0 deletions web/controllers/rules/join/join_rules.ex
Original file line number Diff line number Diff line change
@@ -0,0 +1,13 @@
defmodule Dwylbot.Rules.Join do
@moduledoc """
join rules together when necesssary:
- in-progress label added: combine no_assignee with no_estimation
- multiple reviewers added: combine add assignees rule
Copy link
Member Author

@SimonLab SimonLab Jul 1, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll implement this (combine reviewers errors) on another PR, this one is full enough I think

"""
alias Dwylbot.Rules.Join.EstimationAssignee
def join(rules) do
rules
|> EstimationAssignee.noassignee_noestimation()
end

end
1 change: 1 addition & 0 deletions web/controllers/rules/pr/awaiting_review.ex
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@ defmodule Dwylbot.Rules.PR.AwaitingReview do

if (!Enum.empty?(reviewers) && !in_progress) do
%{
id: Helpers.get_id(payload),
error_type: @rule_name,
actions: [
%{
Expand Down
1 change: 1 addition & 0 deletions web/controllers/rules/pr/merge_conflict.ex
Original file line number Diff line number Diff line change
Expand Up @@ -42,6 +42,7 @@ defmodule Dwylbot.Rules.PR.MergeConflict do

if length(actions) > 0 do
%{
id: Helpers.get_id(payload),
error_type: @rule_name,
actions: actions,
wait: Helpers.wait(Mix.env, 40_000, 1000, 1),
Expand Down
1 change: 1 addition & 0 deletions web/controllers/rules/pr/no_assignee.ex
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@ defmodule Dwylbot.Rules.PR.NoAssignee do
incorrect_assignee = wrong_assignee(assignees, author)
if Helpers.label_member?(labels, "awaiting-review") && incorrect_assignee do
%{
id: Helpers.get_id(payload),
error_type: @rule_name,
actions: [
%{
Expand Down
1 change: 1 addition & 0 deletions web/controllers/rules/pr/no_description.ex
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@ defmodule Dwylbot.Rules.PR.NoDescription do
description = String.trim payload["pull_request"]["body"]
if String.length(description) == 0 do
%{
id: Helpers.get_id(payload),
error_type: @rule_name,
actions: [
%{
Expand Down
Loading