diff --git a/lib/code_corps/github/sync/issue/github_issue/github_issue.ex b/lib/code_corps/github/sync/issue/github_issue/github_issue.ex index a29620239..f1d138005 100644 --- a/lib/code_corps/github/sync/issue/github_issue/github_issue.ex +++ b/lib/code_corps/github/sync/issue/github_issue/github_issue.ex @@ -37,13 +37,21 @@ defmodule CodeCorps.GitHub.Sync.Issue.GithubIssue do |> GithubIssue.changeset(payload |> Adapters.Issue.to_issue()) |> Changeset.put_assoc(:github_user, github_user) |> Changeset.put_assoc(:github_repo, github_repo) - |> Changeset.put_assoc(:github_pull_request, github_pull_request) + |> maybe_put_github_pull_request(github_pull_request) |> Repo.insert_or_update() else {:error, error} -> {:error, error} end end + @spec maybe_put_github_pull_request(Changeset.t, GithubPullRequest.t | nil) :: Changeset.t + defp maybe_put_github_pull_request(%Changeset{} = changeset, %GithubPullRequest{} = github_pull_request) do + changeset |> Changeset.put_assoc(:github_pull_request, github_pull_request) + end + defp maybe_put_github_pull_request(%Changeset{} = changeset, nil) do + changeset + end + @spec find_or_init(map) :: GithubIssue.t defp find_or_init(%{"id" => github_id}) do case GithubIssue |> Repo.get_by(github_id: github_id) |> Repo.preload([:github_user, :github_repo, :github_pull_request]) do diff --git a/lib/code_corps/task/service.ex b/lib/code_corps/task/service.ex index 271155b89..9894a7c5c 100644 --- a/lib/code_corps/task/service.ex +++ b/lib/code_corps/task/service.ex @@ -23,10 +23,12 @@ defmodule CodeCorps.Task.Service do :user ] + @type result :: {:ok, Task.t} | {:error, Changeset.t} | {:error, :github} | {:error, :task_not_found} + @doc ~S""" Performs all actions involved in creating a task on a project """ - @spec create(map) :: {:ok, Task.t} | {:error, Changeset.t} | {:error, :github} + @spec create(map) :: result def create(%{} = attributes) do Multi.new |> Multi.insert(:task, %Task{} |> Task.create_changeset(attributes)) @@ -38,7 +40,7 @@ defmodule CodeCorps.Task.Service do |> marshall_result() end - @spec update(Task.t, map) :: {:ok, Task.t} | {:error, Changeset.t} | {:error, :github} + @spec update(Task.t, map) :: result def update(%Task{} = task, %{} = attributes) do Multi.new |> Multi.update(:task, task |> Task.update_changeset(attributes)) @@ -46,14 +48,15 @@ defmodule CodeCorps.Task.Service do {:ok, task |> Repo.preload(@preloads)} end) |> Multi.run(:github, (fn %{preload: %Task{} = task} -> task |> update_on_github() end)) - |> Repo.transaction + |> Repo.transaction() |> marshall_result() end - @spec marshall_result(tuple) :: {:ok, Task.t} | {:error, Changeset.t} | {:error, :github} + @spec marshall_result(tuple) :: result defp marshall_result({:ok, %{github: %Task{} = task}}), do: {:ok, task} defp marshall_result({:ok, %{task: %Task{} = task}}), do: {:ok, task} defp marshall_result({:error, :task, %Changeset{} = changeset, _steps}), do: {:error, changeset} + defp marshall_result({:error, :github, {:error, :task_not_found}, _steps}), do: {:error, :task_not_found} defp marshall_result({:error, :github, result, _steps}) do Logger.info "An error occurred when creating/updating the task with the GitHub API" Logger.info "#{inspect result}" @@ -70,7 +73,7 @@ defmodule CodeCorps.Task.Service do {:ok, %GithubIssue{} = github_issue} <- payload |> Sync.Issue.GithubIssue.create_or_update_issue(github_repo) do - task |> link_with_github_changeset(github_issue) |> Repo.update + task |> link_with_github_changeset(github_issue) |> Repo.update() else {:error, error} -> {:error, error} end @@ -81,16 +84,16 @@ defmodule CodeCorps.Task.Service do task |> Changeset.change(%{github_issue: github_issue}) end - @spec update_on_github(Task.t) :: {:ok, Task.t} | {:error, Changeset.t} | {:error, GitHub.api_error_struct} + @spec update_on_github(Task.t) :: {:ok, Task.t} | {:error, Changeset.t} | {:error, GitHub.api_error_struct} | {:error, :task_not_found} defp update_on_github(%Task{github_repo_id: nil, github_issue_id: nil} = task), do: {:ok, task} defp update_on_github(%Task{github_repo_id: _, github_issue_id: nil} = task), do: task |> create_on_github() defp update_on_github(%Task{github_repo: github_repo} = task) do with {:ok, payload} <- GitHub.API.Issue.update(task), - {:ok, %GithubIssue{}} <- - payload - |> Sync.Issue.GithubIssue.create_or_update_issue(github_repo) do - {:ok, Task |> Repo.get(task.id)} + {:ok, %GithubIssue{}} <- payload |> Sync.Issue.GithubIssue.create_or_update_issue(github_repo), + %Task{} = task <- Repo.get(Task, task.id) do + {:ok, task} else + nil -> {:error, :task_not_found} {:error, error} -> {:error, error} end end diff --git a/lib/code_corps_web/controllers/github_event_controller.ex b/lib/code_corps_web/controllers/github_event_controller.ex index 5432d26be..ee529d3cd 100644 --- a/lib/code_corps_web/controllers/github_event_controller.ex +++ b/lib/code_corps_web/controllers/github_event_controller.ex @@ -1,9 +1,11 @@ defmodule CodeCorpsWeb.GithubEventController do @moduledoc false use CodeCorpsWeb, :controller + import Ecto.Query, only: [from: 2] alias CodeCorps.{ GithubEvent, + GithubRepo, GitHub.Webhook.Handler, GitHub.Webhook.EventSupport, Helpers.Query, @@ -43,14 +45,29 @@ defmodule CodeCorpsWeb.GithubEventController do @spec create(Conn.t, map) :: Conn.t def create(%Conn{} = conn, %{} = payload) do - type = conn |> get_event_type - delivery_id = conn |> get_delivery_id() + type = conn |> event_type() + delivery_id = conn |> delivery_id() action = payload |> Map.get("action", "") - event_support = type |> EventSupport.status(action) - event_support |> process_event(type, delivery_id, payload) + event_support = + if should_process?(payload) do + process_status = type |> EventSupport.status(action) + process_status |> process_event(type, delivery_id, payload) + process_status + else + :ignored + end conn |> respond_to_webhook(event_support) end + @spec should_process?(map) :: boolean + defp should_process?(%{"repository" => %{"id" => repository_id}}) do + query = from repo in GithubRepo, + where: repo.github_id == ^repository_id, + where: not(is_nil(repo.project_id)) + Repo.one(query) != nil + end + defp should_process?(_), do: true + @spec update(Conn.t, map) :: Conn.t def update(%Conn{} = conn, %{"id" => id} = params) do with %GithubEvent{} = github_event <- GithubEvent |> Repo.get(id), @@ -63,13 +80,13 @@ defmodule CodeCorpsWeb.GithubEventController do end end - @spec get_event_type(Conn.t) :: String.t - defp get_event_type(%Conn{} = conn) do + @spec event_type(Conn.t) :: String.t + defp event_type(%Conn{} = conn) do conn |> get_req_header("x-github-event") |> List.first end - @spec get_delivery_id(Conn.t) :: String.t - defp get_delivery_id(%Conn{} = conn) do + @spec delivery_id(Conn.t) :: String.t + defp delivery_id(%Conn{} = conn) do conn |> get_req_header("x-github-delivery") |> List.first end diff --git a/test/lib/code_corps/skills/skills_test.exs b/test/lib/code_corps/skills/skills_test.exs index 6e1443bb0..a958e77e2 100644 --- a/test/lib/code_corps/skills/skills_test.exs +++ b/test/lib/code_corps/skills/skills_test.exs @@ -1,4 +1,4 @@ -defmodule CodeCorps.AccountsTest do +defmodule CodeCorps.SkillsTest do @moduledoc false use CodeCorps.DbAccessCase diff --git a/test/lib/code_corps/task/service_test.exs b/test/lib/code_corps/task/service_test.exs index f2b0701bd..73f5c2039 100644 --- a/test/lib/code_corps/task/service_test.exs +++ b/test/lib/code_corps/task/service_test.exs @@ -5,7 +5,7 @@ defmodule CodeCorps.Task.ServiceTest do import CodeCorps.GitHub.TestHelpers - alias CodeCorps.Task + alias CodeCorps.{GithubIssue, Repo, Task} @base_attrs %{ "title" => "Test task", @@ -13,6 +13,8 @@ defmodule CodeCorps.Task.ServiceTest do "status" => "open" } + @issue_payload load_endpoint_fixture("issue") + defp valid_attrs() do project = insert(:project) task_list = insert(:task_list, project: project, inbox: true) @@ -106,7 +108,7 @@ defmodule CodeCorps.Task.ServiceTest do assert updated_task.id == task.id assert updated_task.title == @update_attrs["title"] assert updated_task.markdown == @update_attrs["markdown"] - assert updated_task.body != task.body + refute updated_task.body == task.body refute task.github_issue_id refute task.github_repo_id @@ -159,13 +161,40 @@ defmodule CodeCorps.Task.ServiceTest do assert updated_task.id == task.id assert updated_task.title == @update_attrs["title"] assert updated_task.markdown == @update_attrs["markdown"] - assert updated_task.body != task.body + refute updated_task.body == task.body assert updated_task.github_issue_id assert updated_task.github_repo_id assert_received({:patch, "https://api.github.com/repos/foo/bar/issues/5", _body, _headers, _options}) end + test "propagates changes to github if task is synced to github pull request" do + %{ + "id" => issue_github_id, + "number" => number + } = @issue_payload + + github_repo = insert(:github_repo, github_account_login: "octocat", name: "Hello-World") + github_pull_request = insert(:github_pull_request) + github_issue = insert(:github_issue, github_id: issue_github_id, number: number, github_pull_request: github_pull_request, github_repo: github_repo) + task = insert(:task, github_repo: github_repo, github_issue: github_issue) + + {:ok, updated_task} = task |> Task.Service.update(@update_attrs) + + assert_received({:patch, "https://api.github.com/repos/octocat/Hello-World/issues/1347", _body, _headers, _options}) + + assert updated_task.id == task.id + assert updated_task.title == @update_attrs["title"] + assert updated_task.markdown == @update_attrs["markdown"] + refute updated_task.body == task.body + assert updated_task.github_issue_id == github_issue.id + assert updated_task.github_repo_id == github_repo.id + + updated_github_issue = Repo.one(GithubIssue) + + assert updated_github_issue.github_pull_request_id == github_pull_request.id + end + test "reports {:error, :github}, makes no changes at all if there is a github api error" do github_repo = :github_repo diff --git a/test/lib/code_corps_web/controllers/github_event_controller_test.exs b/test/lib/code_corps_web/controllers/github_event_controller_test.exs index f675d0a01..559e8606c 100644 --- a/test/lib/code_corps_web/controllers/github_event_controller_test.exs +++ b/test/lib/code_corps_web/controllers/github_event_controller_test.exs @@ -101,11 +101,22 @@ defmodule CodeCorpsWeb.GithubEventControllerTest do test "responds with 200 for an unsupported event", %{conn: conn} do path = conn |> github_events_path(:create) payload = load_event_fixture("pull_request_synchronize") + insert(:github_repo, github_id: payload["repository"]["id"]) assert conn |> for_event("pull_request", "foo") |> post(path, payload) |> response(200) assert Repo.get_by(GithubEvent, github_delivery_id: "foo") end + @tag :github_webhook + test "responds with 202 for a supported event but no project_id", %{conn: conn} do + path = conn |> github_events_path(:create) + payload = load_event_fixture("pull_request_synchronize") + insert(:github_repo, github_id: payload["repository"]["id"], project: nil) + assert conn |> for_event("pull_request", "foo") |> post(path, payload) |> response(202) + + refute Repo.get_by(GithubEvent, github_delivery_id: "foo") + end + @tag :github_webhook test "responds with 202 for an unknown event", %{conn: conn} do path = conn |> github_events_path(:create)