Moves nil parameter removal from github adapter to user changeset (#1300)

Move rejection of nil attributes from a github payload when updating a user record from the adapter into the user changeset.

Author: iref

lib/code_corps/accounts/changesets.ex

@@ -14,8 +14,14 @@ defmodule CodeCorps.Accounts.Changesets do
   """
   @spec create_from_github_changeset(struct, map) :: Changeset.t
   def create_from_github_changeset(struct, %{} = params) do
+    params =
+      params
+      |> Adapters.User.to_user()
+      |> Enum.reject(fn {_, v} -> is_nil(v) end)
+      |> Map.new()
+
     struct
-    |> Changeset.change(params |> Adapters.User.to_user())
+    |> Changeset.change(params)
     |> Changeset.put_change(:sign_up_context, "github")
     |> Changeset.validate_inclusion(:type, ["bot", "user"])
     |> RandomIconColor.generate_icon_color(:default_color)

lib/code_corps/github/adapters/user.ex

@@ -21,17 +21,12 @@ defmodule CodeCorps.GitHub.Adapters.User do
   Converts a Github user payload into a map of attributes suitable for creating
   or updating a `CodeCorps.User`
 
-  Any `nil` values are removed. For example, we don't want to delete an
-  existing email just because it's `nil` in the payload.
-
   The `type` gets transformed to match our expected values for user type.
   """
   @spec to_user(map) :: map
   def to_user(%{} = payload) do
     payload
     |> CodeCorps.Adapter.MapTransformer.transform(@user_mapping)
-    |> Enum.reject(fn {_, v} -> is_nil(v) end)
-    |> Map.new
     |> transform_type
   end
 

test/lib/code_corps/accounts/changesets_test.exs

@@ -18,6 +18,16 @@ defmodule CodeCorps.Accounts.ChangesetsTest do
       changeset = Changesets.create_from_github_changeset(%User{}, %{})
       assert changeset.changes.default_color
     end
+
+    test "ensures nil values are omitted" do
+      params = %{"email" => nil, "github_avatar_url" => nil, "type" => "bot"}
+
+      changeset = Changesets.create_from_github_changeset(%User{}, params)
+
+      refute changeset.changes[:email]
+      refute changeset.changes[:github_avatar_url]
+    end
+
   end
 
   describe "update_from_github_oauth_changeset/2" do

test/lib/code_corps/github/adapters/user_test.exs

@@ -7,11 +7,22 @@ defmodule CodeCorps.GitHub.Adapters.UserTest do
 
   alias CodeCorps.GitHub.Adapters.User
 
+  defp expected_payload(type) do
+    %{
+      email: nil,
+      github_id: nil,
+      github_username: nil,
+      github_avatar_url: nil,
+      type: type
+    }
+  end
+
   describe "to_user/1" do
     test "maps API payload" do
       %{"issue" => %{"user" => payload}} = load_event_fixture("issues_opened")
 
       assert User.to_user(payload) == %{
+        email: nil,
         github_id: payload["id"],
         github_username: payload["login"],
         github_avatar_url: payload["avatar_url"],
@@ -20,15 +31,15 @@ defmodule CodeCorps.GitHub.Adapters.UserTest do
     end
 
     test "maps Bot type" do
-      assert User.to_user(%{"type" => "Bot"}) == %{type: "bot"}
+      assert User.to_user(%{"type" => "Bot"}) == expected_payload("bot")
     end
 
     test "maps User type" do
-      assert User.to_user(%{"type" => "User"}) == %{type: "user"}
+      assert User.to_user(%{"type" => "User"}) == expected_payload("user")
     end
 
     test "does not map Organization type" do
-      assert User.to_user(%{"type" => "Organization"}) == %{type: "Organization"}
+      assert User.to_user(%{"type" => "Organization"}) == expected_payload("Organization")
     end
   end
 end

test/lib/code_corps/github/sync/user/record_linker_test.exs

@@ -12,6 +12,12 @@ defmodule CodeCorps.Sync.User.RecordLinkerTest do
   }
   alias CodeCorps.GitHub.Adapters.User, as: UserAdapter
 
+  def remove_nils(payload) do
+    payload
+    |> Enum.reject(fn {_, v} -> is_nil(v) end)
+    |> Map.new()
+  end
+
   describe "link_to/2 for comments" do
     @payload load_event_fixture("issue_comment_created")
     @bot_payload load_event_fixture("issue_comment_created_by_bot")
@@ -43,7 +49,7 @@ defmodule CodeCorps.Sync.User.RecordLinkerTest do
 
     test "finds user by github id if none is found by comment association" do
       %{"comment" => %{"id" => github_id} = comment} = @payload
-      attributes = UserAdapter.to_user(@user_payload)
+      attributes = @user_payload |> UserAdapter.to_user() |> remove_nils()
       preinserted_user = insert(:user, attributes)
       github_comment = insert(:github_comment, github_id: github_id)
 
@@ -58,7 +64,7 @@ defmodule CodeCorps.Sync.User.RecordLinkerTest do
       github_comment = insert(:github_comment, github_id: github_id)
       {:ok, %User{} = returned_user} = RecordLinker.link_to(github_comment, comment)
 
-      created_attributes = UserAdapter.to_user(@user_payload)
+      created_attributes = @user_payload |> UserAdapter.to_user() |> remove_nils()
       created_user = Repo.get_by(User, created_attributes)
       assert created_user.id == returned_user.id
     end
@@ -86,7 +92,7 @@ defmodule CodeCorps.Sync.User.RecordLinkerTest do
       github_comment = insert(:github_comment, github_id: github_id)
       {:ok, %User{} = returned_user} = RecordLinker.link_to(github_comment, comment)
 
-      created_attributes = UserAdapter.to_user(@bot_user_payload)
+      created_attributes = @bot_user_payload |> UserAdapter.to_user() |> remove_nils()
       created_user = Repo.get_by(User, created_attributes)
       assert created_user.id == returned_user.id
     end
@@ -142,7 +148,7 @@ defmodule CodeCorps.Sync.User.RecordLinkerTest do
 
     test "returns user by github id if no user by task association found" do
       %{"issue" => %{"number" => number} = issue} = @payload
-      attributes = UserAdapter.to_user(@user_payload)
+      attributes = @user_payload |> UserAdapter.to_user() |> remove_nils()
       preinserted_user = insert(:user, attributes)
       github_issue = insert(:github_issue, number: number)
 
@@ -157,7 +163,7 @@ defmodule CodeCorps.Sync.User.RecordLinkerTest do
       github_issue = insert(:github_issue, number: number)
       {:ok, %User{} = returned_user} = RecordLinker.link_to(github_issue, issue)
 
-      created_attributes = UserAdapter.to_user(@user_payload)
+      created_attributes = @user_payload |> UserAdapter.to_user() |> remove_nils()
       created_user = Repo.get_by(User, created_attributes)
       assert created_user.id == returned_user.id
     end
@@ -191,7 +197,7 @@ defmodule CodeCorps.Sync.User.RecordLinkerTest do
       github_issue = insert(:github_issue, number: number)
       {:ok, %User{} = returned_user} = RecordLinker.link_to(github_issue, issue)
 
-      created_attributes = UserAdapter.to_user(@bot_user_payload)
+      created_attributes =  @bot_user_payload |> UserAdapter.to_user() |> remove_nils()
       created_user = Repo.get_by(User, created_attributes)
       assert created_user.id == returned_user.id
     end