From d57920b54cec73906cc0f0481db5f4e7f68d2265 Mon Sep 17 00:00:00 2001
From: Robert Francis <robertefrancis18@gmail.com>
Date: Thu, 25 Apr 2019 13:15:14 +0100
Subject: [PATCH] create an insert function that is not a macro to compare to
 macro alternative

---
 lib/append/address.ex        | 13 ++++++--
 lib/append/aol.ex            | 58 ++++++++++++++++++++++++++++++++++++
 test/append/address_test.exs | 34 ++++++++++++++++++---
 3 files changed, 99 insertions(+), 6 deletions(-)
 create mode 100644 lib/append/aol.ex

diff --git a/lib/append/address.ex b/lib/append/address.ex
index d75de4e..e589392 100644
--- a/lib/append/address.ex
+++ b/lib/append/address.ex
@@ -20,6 +20,8 @@ defmodule Append.Address do
   @doc false
   def changeset(address, attrs) do
     address
+    # I think that this should be removed
+    # see comments above this functions definition for more on this
     |> insert_entry_id()
     |> cast(attrs, [
       :name,
@@ -37,11 +39,18 @@ defmodule Append.Address do
       :address_line_2,
       :city,
       :postcode,
-      :tel,
-      :entry_id
+      :tel
+      # alog doesn't have entry_id as a required_field, it just ensures that an
+      # entry_id is added to the changeset.
+      # see https://github.com/dwyl/alog/blob/master/lib/alog.ex#L112
+      # :entry_id
     ])
   end
 
+  # Ideally, we wouldn't require the user to create a function that they have to
+  # call in their changeset function.
+  # This is moved in alog, and can be updated for the non-macro approach so that
+  # users will not need to do this.
   def insert_entry_id(address) do
     case Map.fetch(address, :entry_id) do
       {:ok, nil} -> %{address | entry_id: Ecto.UUID.generate()}
diff --git a/lib/append/aol.ex b/lib/append/aol.ex
new file mode 100644
index 0000000..b1b225e
--- /dev/null
+++ b/lib/append/aol.ex
@@ -0,0 +1,58 @@
+defmodule Append.AOL do
+  import Ecto.Changeset
+
+  # macro way
+  # user needs to...
+  # 1 - call use MACRO_MODULE name in calling module
+  # 2 - Call CallingModuleName.insert(params)
+  # (not much effort on the users part)
+  # def insert(attrs) do
+  #   %__MODULE__{}
+  #   |> __MODULE__.changeset(attrs)
+  #   |> Repo.insert()
+  # end
+
+  # non macro way
+  # user needs to...
+  # 1 - Call ThisModuleName.insert(Module, params)
+  # (again, not much effort on the users part)
+  # Also, if the user is following new practices and has a context which handles
+  # calling insert, they could (and I think should) have a function defined
+  # which in turn calls this function. e.g.
+  # defmodule MyApp.MyContext do
+  #   def insert_user(attrs) do
+  #     Append.AOL.insert(User, attrs)
+  #   end
+  # end
+  # This would mean that they would just be calling...
+  # MyApp.MyContext.insert_user(attrs)
+  # so really no overhead at all
+  # shrug emoji
+  def insert(module, attrs) do
+    module
+    |> struct()
+    |> module.changeset(attrs)
+    # |> insert_entry_id()
+    |> Append.Repo.insert()
+  end
+
+  # This function would handle adding the entry_id to the changeset. It's not
+  # really needed for this example but it would be needed in a real app. See
+  # comments in Append.Address for more info on this
+  def insert_entry_id(%Ecto.Changeset{valid?: true} = changeset) do
+    case get_field(changeset, :entry_id) do
+      nil ->
+        put_change(changeset, :entry_id, Ecto.UUID.generate())
+
+      _ ->
+        # may need to put an error here saying something like...
+        # OI!!!!!! Don't try to generate your own entry_id...
+        # But haven't really thought that far ahead. Also it isn't done in this
+        # example anyway so didn't think it was needed.
+        changeset
+    end
+
+  end
+
+  def insert_entry_id(changeset), do: changeset
+end
\ No newline at end of file
diff --git a/test/append/address_test.exs b/test/append/address_test.exs
index 7dc4181..0db9fb3 100644
--- a/test/append/address_test.exs
+++ b/test/append/address_test.exs
@@ -10,16 +10,29 @@ defmodule Append.AddressTest do
 
   describe "get items from database" do
     test "get/1" do
-      {:ok, item} = insert_address()
+      # {:ok, item} = insert_address()
+      {:ok, item} = insert_address2()
 
       assert Address.get(item.entry_id) == item
     end
 
     test "all/0" do
-      {:ok, _} = insert_address()
-      {:ok, _} = insert_address("Loki")
-
+      insert_address()
+      {:ok, loki} = insert_address2("Loki")
       assert length(Address.all()) == 2
+
+      {:ok, loki2} = insert_address("Loki")
+
+      # remove things that will always be different
+      [m1, m2] =
+        [loki, loki2]
+        |> Enum.map(&Map.drop(&1, ~w(entry_id id inserted_at updated_at)a))
+
+      # This test shows that insert_address and insert_address2 both return the
+      # same value
+      # check out where insert_address amd insert_address2 are defined below to
+      # see differences between the two functions. (There isn't many)
+      assert Map.equal?(m1, m2)
     end
   end
 
@@ -67,6 +80,7 @@ defmodule Append.AddressTest do
     assert Map.fetch(h2, :city) == {:ok, "Oslo"}
   end
 
+  # macro way
   def insert_address(name \\ "Thor") do
     Address.insert(%{
       name: name,
@@ -78,6 +92,18 @@ defmodule Append.AddressTest do
     })
   end
 
+  # function way
+  def insert_address2(name \\ "Thor") do
+    Append.AOL.insert(Address, %{
+      name: name,
+      address_line_1: "The Hall",
+      address_line_2: "Valhalla",
+      city: "Asgard",
+      postcode: "AS1 3DG",
+      tel: "0800123123"
+    })
+  end
+
   describe "delete:" do
     test "deleted items are not retrieved with 'get'" do
       {:ok, item} = insert_address()