Merge pull request #1257 from code-corps/1253-fix-github-issue-nulling

Fix accidental nulling of pull request in Task.Service

Author: joshsmith

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

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,22 +40,23 @@ 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))
     |> Multi.run(:preload, fn %{task: %Task{} = task} ->
          {: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

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

test/lib/code_corps/task/service_test.exs

@@ -5,14 +5,16 @@ defmodule CodeCorps.Task.ServiceTest do
 
   import CodeCorps.GitHub.TestHelpers
 
-  alias CodeCorps.Task
+  alias CodeCorps.{GithubIssue, Repo, Task}
 
   @base_attrs %{
     "title" => "Test task",
     "markdown" => "A test task",
     "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