Merge pull request #1370 from code-corps/minor-cleanup-of-github-sync

Clean up GitHub Sync with some minor documentation and spec changes

Author: begedin

lib/code_corps/comment/service.ex

@@ -84,7 +84,7 @@ defmodule CodeCorps.Comment.Service do
   defp create_on_github(%Comment{task: %Task{github_issue: github_issue}} = comment) do
     with {:ok, payload} <- comment |> GitHub.API.Comment.create(),
          {:ok, %GithubComment{} = github_comment} <-
-           Sync.Comment.GithubComment.create_or_update_comment(github_issue, payload) do
+           Sync.GithubComment.create_or_update_comment(github_issue, payload) do
       comment |> link_with_github_changeset(github_comment) |> Repo.update()
     else
       {:error, error} -> {:error, error}
@@ -103,7 +103,7 @@ defmodule CodeCorps.Comment.Service do
        ) do
     with {:ok, payload} <- comment |> GitHub.API.Comment.update(),
          {:ok, %GithubComment{}} <-
-           Sync.Comment.GithubComment.create_or_update_comment(github_issue, payload) do
+           Sync.GithubComment.create_or_update_comment(github_issue, payload) do
       {:ok, comment}
     else
       {:error, error} -> {:error, error}

lib/code_corps/github/event/installation/installation.ex

@@ -6,7 +6,10 @@ defmodule CodeCorps.GitHub.Event.Installation do
 
   @behaviour CodeCorps.GitHub.Event.Handler
 
-  alias CodeCorps.{GitHub.Sync, GitHub.Event.Installation}
+  alias CodeCorps.{
+    GitHub.Sync,
+    GitHub.Event.Installation
+  }
 
 
   @doc """
@@ -17,6 +20,7 @@ defmodule CodeCorps.GitHub.Event.Installation do
 
   `CodeCorps.GitHub.Sync.installation_event/1`
   """
+  @impl CodeCorps.GitHub.Event.Handler
   @spec handle(map) ::
     Sync.installation_event_outcome() | {:error, :unexpected_payload}
   def handle(payload) do

lib/code_corps/github/event/installation_repositories/installation_repositories.ex

@@ -10,16 +10,9 @@ defmodule CodeCorps.GitHub.Event.InstallationRepositories do
 
   alias CodeCorps.{
     GitHub.Sync,
-    GithubRepo,
     GitHub.Event.InstallationRepositories
   }
 
-  @type outcome :: {:ok, list(GithubRepo.t())} |
-                   {:error, :unmatched_installation} |
-                   {:error, :unexpected_payload} |
-                   {:error, :validation_error_on_syncing_repos} |
-                   {:error, :unexpected_transaction_outcome}
-
   @doc """
   Handles an "InstallationRepositories" GitHub Webhook event. The event could be
   of subtype "added" or "removed" and is handled differently based on that.
@@ -34,7 +27,10 @@ defmodule CodeCorps.GitHub.Event.InstallationRepositories do
       - if the GitHub payload for a repo is not matched with a record in our
         database, just skip deleting it
   """
-  @spec handle(map) :: outcome
+  @impl CodeCorps.GitHub.Event.Handler
+  @spec handle(map) ::
+    Sync.installation_repositories_event_outcome() |
+    {:error, :unexpected_payload}
   def handle(payload) do
     with {:ok, :valid} <- payload |> validate_payload() do
       Sync.installation_repositories_event(payload)

lib/code_corps/github/event/issue_comment/issue_comment.ex

@@ -8,10 +8,9 @@ defmodule CodeCorps.GitHub.Event.IssueComment do
   @behaviour CodeCorps.GitHub.Event.Handler
 
   alias CodeCorps.{
-    GitHub,
+    GitHub.Sync,
     GitHub.Event.IssueComment.Validator
   }
-  alias GitHub.Sync
 
   @doc ~S"""
   Handles the "IssueComment" GitHub webhook
@@ -23,7 +22,8 @@ defmodule CodeCorps.GitHub.Event.IssueComment do
   - sync the comment using `CodeCorps.GitHub.Sync.Comment`
   """
   @impl CodeCorps.GitHub.Event.Handler
-  @spec handle(map) :: {:ok, any} | {:error, atom}
+  @spec handle(map) ::
+    Sync.issue_comment_event_outcome() | {:error, :unexpected_payload}
   def handle(payload) do
     with {:ok, :valid} <- validate_payload(payload) do
       Sync.issue_comment_event(payload)

lib/code_corps/github/event/issues/issues.ex

@@ -8,10 +8,9 @@ defmodule CodeCorps.GitHub.Event.Issues do
   @behaviour CodeCorps.GitHub.Event.Handler
 
   alias CodeCorps.{
-    GitHub,
+    GitHub.Sync,
     GitHub.Event.Issues.Validator
   }
-  alias GitHub.Sync
 
   @doc ~S"""
   Handles the "Issues" GitHub webhook
@@ -23,7 +22,8 @@ defmodule CodeCorps.GitHub.Event.Issues do
   - sync the issue using `CodeCorps.GitHub.Sync.Issue`
   """
   @impl CodeCorps.GitHub.Event.Handler
-  @spec handle(map) :: {:ok, any} | {:error, atom}
+  @spec handle(map) ::
+    Sync.issue_event_outcome() | {:error, :unexpected_payload}
   def handle(payload) do
     with {:ok, :valid} <- validate_payload(payload) do
       Sync.issue_event(payload)

lib/code_corps/github/event/pull_request/pull_request.ex

@@ -8,10 +8,9 @@ defmodule CodeCorps.GitHub.Event.PullRequest do
   @behaviour CodeCorps.GitHub.Event.Handler
 
   alias CodeCorps.{
-    GitHub,
+    GitHub.Sync,
     GitHub.Event.PullRequest.Validator
   }
-  alias GitHub.Sync
 
   @doc ~S"""
   Handles the "PullRequest" GitHub webhook
@@ -23,7 +22,8 @@ defmodule CodeCorps.GitHub.Event.PullRequest do
   - sync the pull request using `CodeCorps.GitHub.Sync.PullRequest`
   """
   @impl CodeCorps.GitHub.Event.Handler
-  @spec handle(map) :: {:ok, any} | {:error, atom}
+  @spec handle(map) ::
+    Sync.pull_request_event_outcome() | {:error, :unexpected_payload}
   def handle(payload) do
     with {:ok, :valid} <- validate_payload(payload) do
       Sync.pull_request_event(payload)

lib/code_corps/github/sync/comment/comment/changeset.ex renamed to lib/code_corps/github/sync/comment/changeset.ex

@@ -1,4 +1,4 @@
-defmodule CodeCorps.GitHub.Sync.Comment.Comment.Changeset do
+defmodule CodeCorps.GitHub.Sync.Comment.Changeset do
   @moduledoc ~S"""
   In charge of building a `Changeset` to update a `Comment` with, when handling
   a GitHub Comment payload.
@@ -16,7 +16,6 @@ defmodule CodeCorps.GitHub.Sync.Comment.Comment.Changeset do
   alias Ecto.Changeset
 
   @create_attrs ~w(created_at markdown modified_at)a
-  @update_attrs ~w(markdown modified_at)a
 
   @doc ~S"""
   Constructs a changeset for creating a `CodeCorps.Comment` when syncing from a
@@ -39,6 +38,8 @@ defmodule CodeCorps.GitHub.Sync.Comment.Comment.Changeset do
     |> Changeset.validate_required([:markdown, :body])
   end
 
+  @update_attrs ~w(markdown modified_at)a
+
   @doc ~S"""
   Constructs a changeset for updating a `CodeCorps.Comment` when syncing from a
   GitHub API Comment payload.

lib/code_corps/github/sync/comment/comment.ex

@@ -1,39 +1,123 @@
 defmodule CodeCorps.GitHub.Sync.Comment do
-  alias CodeCorps.GitHub.Sync
-  alias Ecto.Multi
+  @moduledoc ~S"""
+  In charge of syncing `CodeCorps.Comment` records with a GitHub comment
+  payload.
+
+  A single GitHub comment always matches a single `CodeCorps.GithubComment`, but
+  it can match multiple `CodeCorps.Comment` records. This module handles
+  creating or updating all those records.
+  """
+
+  import Ecto.Query
+
+  alias CodeCorps.{
+    Comment,
+    GitHub.Sync,
+    GitHub.Utils.ResultAggregator,
+    GithubComment,
+    GithubIssue,
+    GithubRepo,
+    GithubUser,
+    Repo,
+    Task,
+    User
+  }
+  alias Ecto.Changeset
+
+  @type commit_result_aggregate ::
+    {:ok, list(Comment.t())} | {:error, {list(Comment.t()), list(Changeset.t())}}
+
+  @type commit_result :: {:ok, Comment.t()} | {:error, Changeset.t()}
 
   @doc ~S"""
-  Creates an `Ecto.Multi` intended to process a GitHub issue comment related API
-  payload.
+  Creates or updates a `CodeCorps.Comment` for the specified `CodeCorps.Task`.
+
+  When provided a `CodeCorps.Task`, a `CodeCorps.GithubComment`, a
+  `CodeCorps.User`, and a GitHub API payload, it creates or updates a
+  `CodeCorps.Comment` record, using the provided GitHub API
+  payload, associated to the specified `CodeCorps.GithubComment`,
+  `CodeCorps.Task` and `CodeCorps.User`
+  """
+  @spec sync(Task.t(), GithubComment.t(), User.t()) :: commit_result()
+  def sync(%Task{} = task, %GithubComment{} = github_comment, %User{} = user) do
+    case find_comment(task, github_comment) do
+      nil ->
+        github_comment
+        |> Sync.Comment.Changeset.create_changeset(task, user)
+        |> Repo.insert()
+
+      %Comment{} = comment ->
+        comment
+        |> Sync.Comment.Changeset.update_changeset(github_comment)
+        |> Repo.update()
+    end
+  end
+
+  @spec find_comment(Task.t(), GithubComment.t()) :: Comment.t() | nil
+  defp find_comment(%Task{id: task_id}, %GithubComment{id: github_comment_id}) do
+    query = from c in Comment,
+      where: c.task_id == ^task_id,
+      join: gc in GithubComment, on: c.github_comment_id == gc.id, where: gc.id == ^github_comment_id
+
+    query |> Repo.one()
+  end
 
-  Expects a partial transaction outcome with `:github_issue` and :task keys.
+  @doc ~S"""
+  Creates or updates `CodeCorps.Comment` records for the specified
+  `CodeCorps.GithubRepo`.
 
-  Returns an `Ecto.Multi` with the follwing steps
+  For each `CodeCorps.GithubComment` record that relates to the
+  `CodeCorps.GithubRepo` for a given `CodeCorps.GithubRepo`:
 
-  - create or update a `CodeCorps.GithubComment` from the
-    provided `CodeCorps.GithubIssue` and API payload
-  - match the `CodeCorps.GithubComment` with a new or existing `CodeCorps.User`
-  - create or update a `CodeCorps.Comment` using the created
-    `CodeCorps.GithubComment`, related to the matched `CodeCorps.User` and the
-    provided `CodeCorps.Task`
+  - Find the related `CodeCorps.Task` record
+  - Create or update the related `CodeCorps.Comment` record
+  - Associate the `CodeCorps.Comment` record with the `CodeCorps.User` that
+    relates to the `CodeCorps.GithubUser` for the `CodeCorps.GithubComment`
   """
-  @spec sync(map, map) :: Multi.t
-  def sync(%{github_issue: github_issue, task: task}, %{} = payload) do
-    Multi.new
-    |> Multi.run(:github_comment, fn _ -> Sync.Comment.GithubComment.create_or_update_comment(github_issue, payload) end)
-    |> Multi.run(:comment_user, fn %{github_comment: github_comment} -> Sync.User.RecordLinker.link_to(github_comment, payload) end)
-    |> Multi.run(:comment, fn %{github_comment: github_comment, comment_user: user} -> Sync.Comment.Comment.sync(task, github_comment, user) end)
+  @spec sync_github_repo(GithubRepo.t()) :: commit_result_aggregate()
+  def sync_github_repo(%GithubRepo{} = github_repo) do
+    preloads = [
+      github_comments: [:github_issue, github_user: [:user]]
+    ]
+    %GithubRepo{github_comments: github_comments} =
+      github_repo |> Repo.preload(preloads)
+
+    github_comments
+    |> Enum.map(fn %GithubComment{github_user: %GithubUser{user: %User{} = user}} = github_comment ->
+      github_comment
+      |> find_task(github_repo)
+      |> sync(github_comment, user)
+    end)
+    |> ResultAggregator.aggregate()
+  end
+
+  # TODO: can this return a nil?
+  @spec find_task(GithubComment.t(), GithubRepo.t()) :: Task.t()
+  defp find_task(
+    %GithubComment{github_issue: %GithubIssue{id: github_issue_id}},
+    %GithubRepo{project_id: project_id}) do
+    query = from t in Task,
+      where: t.project_id == ^project_id,
+      join: gi in GithubIssue, on: t.github_issue_id == gi.id, where: gi.id == ^github_issue_id
+
+    query |> Repo.one()
   end
 
   @doc ~S"""
-  Creates an  `Ecto.Multi` intended to delete a `CodeCorps.GithubComment`
-  specified by `github_id`, as well as 0 to 1 `CodeCorps.Comment` records
-  associated to `CodeCorps.GithubComment`
+  Deletes `CodeCorps.Comment` records associated to `CodeCorps.GithubComment`
+  with specified `github_id`
+
+  Since there can be 0 or 1 such records, returns `{:ok, results}` where
+  `results` is a 1-element or blank list of deleted records.
   """
-  @spec delete(map) :: Multi.t
-  def delete(%{"id" => github_id}) do
-    Multi.new
-    |> Multi.run(:deleted_comments, fn _ -> Sync.Comment.Comment.delete(github_id) end)
-    |> Multi.run(:deleted_github_comment, fn _ -> Sync.Comment.GithubComment.delete(github_id) end)
+  @spec delete(String.t()) :: {:ok, list(Comment.t())}
+  def delete(github_id) do
+    query =
+      from c in Comment,
+        join: gc in GithubComment, on: gc.id == c.github_comment_id, where: gc.github_id == ^github_id
+
+    query
+    |> Repo.delete_all(returning: true)
+    |> (fn {_count, comments} -> {:ok, comments} end).()
   end
 end