unique badge serials (#111)

* migration to make badge_serial unique

* /admin/rfid can overwrite badge serial

* feat: rfid import enforces unique badge serials

config/config.exs

@@ -33,11 +33,6 @@ config :orbit, Oban,
 
 config :elixir, :time_zone_database, Tz.TimeZoneDatabase
 
-# Logging config
-config :logger, :console,
-  format: "$time $metadata[$level] $message\n",
-  metadata: [:request_id]
-
 config :phoenix,
   # Use Jason for JSON parsing in Phoenix
   json_library: Jason,

config/test.exs

@@ -34,6 +34,7 @@ config :orbit, Orbit.Import.Personnel, pathname: "personnel/demo.csv"
 config :orbit, Orbit.Import.Rfid, pathname: "rfid/demo.csv"
 
 config :logger, level: :warning
+config :logger, :console, format: "[$level] $message\n"
 
 # Initialize plugs at runtime for faster test compilation
 config :phoenix, :plug_init_mode, :runtime

lib/orbit/badge_serial.ex

@@ -20,6 +20,7 @@ defmodule Orbit.BadgeSerial do
     badge_serial
     |> cast(attrs, [:badge_serial])
     |> validate_required([:badge_serial])
+    |> unique_constraint(:badge_serial)
     |> foreign_key_constraint(:employee_id)
   end
 end

lib/orbit/import/rfid.ex

@@ -1,5 +1,7 @@
 defmodule Orbit.Import.Rfid do
   require Logger
+  import Ecto.Query
+  alias Orbit.BadgeSerial
   alias Orbit.Employee
   alias Orbit.Repo
   alias Orbit.S3
@@ -20,39 +22,56 @@ defmodule Orbit.Import.Rfid do
 
   @spec import_rows(Enumerable.t()) :: :ok
   def import_rows(rows) do
-    imported_count =
+    rows =
       rows
       |> Enum.map(fn row ->
         Map.update!(row, @badge_number_field, &String.trim_leading(&1, "0"))
       end)
-      |> Enum.group_by(& &1[@badge_number_field])
-      |> Enum.reduce(
-        0,
-        fn {badge_number, badge_serials}, acc ->
-          count =
+
+    num_serials_with_duplicates =
+      rows
+      |> Enum.frequencies_by(& &1[@badge_serial_field])
+      |> Enum.count(fn {_serial, count} -> count > 1 end)
+
+    if num_serials_with_duplicates > 0 do
+      Logger.warning("rfid_import badge_serials_with_duplicates=#{num_serials_with_duplicates}")
+    end
+
+    serials_by_badge =
+      Enum.group_by(rows, & &1[@badge_number_field], & &1[@badge_serial_field])
+
+    {:ok, imported_count} =
+      Repo.transaction(fn ->
+        for {badge_number, badge_serials} <- serials_by_badge, reduce: 0 do
+          acc ->
             case Repo.get_by(Employee, badge_number: badge_number) do
               nil ->
-                0
+                acc
 
               employee ->
-                {:ok, employee} =
-                  employee
-                  |> Repo.preload([:badge_serials])
-                  |> Employee.changeset(%{
-                    badge_serials:
-                      Enum.map(
-                        badge_serials,
-                        &%{badge_serial: &1[@badge_serial_field]}
-                      )
-                  })
-                  |> Repo.update()
+                employee_id = employee.id
+                # Delete any of this employee's badges that were not in the latest import.
+                Repo.delete_all(
+                  from(b in BadgeSerial,
+                    where: b.employee_id == ^employee_id,
+                    where: b.badge_serial not in ^badge_serials
+                  )
+                )
 
-                length(employee.badge_serials)
-            end
+                # And add any new badges for that employee
+                for b <- badge_serials do
+                  %BadgeSerial{employee_id: employee_id, badge_serial: b}
+                  |> BadgeSerial.changeset()
+                  |> Repo.insert(
+                    on_conflict: {:replace_all_except, [:id, :inserted_at]},
+                    conflict_target: :badge_serial
+                  )
+                end
 
-          acc + count
+                acc + length(badge_serials)
+            end
         end
-      )
+      end)
 
     Logger.info("rfid_import count=#{imported_count}")
   end

lib/orbit_web/controllers/admin/admin_controller.ex

@@ -122,7 +122,10 @@ defmodule OrbitWeb.Admin.AdminController do
         badge_serial: badge_serial
       }
       |> BadgeSerial.changeset()
-      |> Repo.insert!()
+      |> Repo.insert!(
+        on_conflict: {:replace_all_except, [:id, :inserted_at]},
+        conflict_target: :badge_serial
+      )
 
       text(conn, "OK")
     else

priv/repo/migrations/20240830184718_badge_serial_unique_index.exs

@@ -0,0 +1,13 @@
+defmodule Orbit.Repo.Migrations.BadgeSerialUniqueIndex do
+  use Ecto.Migration
+
+  def change do
+    execute(
+      "DELETE FROM badge_serials a USING badge_serials b WHERE a.id <> b.id AND a.badge_serial = b.badge_serial",
+      ""
+    )
+
+    drop index(:badge_serials, [:badge_serial])
+    create unique_index(:badge_serials, [:badge_serial])
+  end
+end

test/orbit/import/rfid_test.exs

@@ -1,10 +1,11 @@
 defmodule Orbit.Import.RfidTest do
+  use Orbit.DataCase
+  import Orbit.Factory
+  import Test.Support.Helpers
   alias Orbit.BadgeSerial
   alias Orbit.Employee
-  use Orbit.DataCase
   alias Orbit.Import.Rfid
   alias Orbit.Import.RfidWorker
-  import Orbit.Factory
 
   describe "worker" do
     test "downloads and parses badge serials" do
@@ -88,5 +89,58 @@ defmodule Orbit.Import.RfidTest do
                |> Repo.get_by(badge_number: employee_id)
                |> Repo.preload([:badge_serials])
     end
+
+    test "overwrites existing badge for employee and employee for badge" do
+      insert(:employee,
+        badge_number: "100",
+        badge_serials: [build(:badge_serial, badge_serial: "1000")]
+      )
+
+      insert(:employee,
+        badge_number: "101",
+        badge_serials: [build(:badge_serial, badge_serial: "1001")]
+      )
+
+      rows = [
+        %{"EMPLOYEE_ID" => "100", "SERIAL_NUMBER" => "1001"}
+      ]
+
+      Rfid.import_rows(rows)
+
+      assert [
+               %Employee{
+                 badge_number: "100",
+                 badge_serials: [%BadgeSerial{badge_serial: "1001"}]
+               },
+               %Employee{
+                 badge_number: "101",
+                 badge_serials: []
+               }
+             ] =
+               Employee
+               |> Repo.all()
+               |> Repo.preload([:badge_serials])
+               |> Enum.sort_by(& &1.badge_number)
+    end
+
+    test "logs warning if serial number is duplicated in file" do
+      insert(:employee, badge_number: "100")
+      insert(:employee, badge_number: "101")
+
+      rows = [
+        %{"EMPLOYEE_ID" => "100", "SERIAL_NUMBER" => "1000"},
+        %{"EMPLOYEE_ID" => "101", "SERIAL_NUMBER" => "1000"}
+      ]
+
+      log =
+        capture_log do
+          Rfid.import_rows(rows)
+        end
+
+      assert Enum.member?(log, "[warning] rfid_import badge_serials_with_duplicates=1")
+
+      # serial might be assigned to either employee
+      assert [%BadgeSerial{employee_id: _, badge_serial: "1000"}] = Repo.all(BadgeSerial)
+    end
   end
 end

test/orbit_web/controllers/admin_controller_test.exs

@@ -108,6 +108,31 @@ defmodule OrbitWeb.AdminControllerTest do
                )
     end
 
+    test "can overwrite old data", %{conn: conn} do
+      insert(:employee,
+        badge_number: "X123",
+        badge_serials: [build(:badge_serial, badge_serial: "9999")]
+      )
+
+      insert(:employee, badge_number: "X124")
+
+      conn =
+        post(conn, ~p"/admin/rfid", %{
+          "badge_number" => "X124",
+          "badge_serial" => "9999"
+        })
+
+      assert response(conn, 200)
+
+      assert %BadgeSerial{employee: %Employee{badge_number: "X124"}} =
+               Repo.one!(
+                 from(badge_serial in BadgeSerial,
+                   where: badge_serial.badge_serial == "9999",
+                   preload: [:employee]
+                 )
+               )
+    end
+
     test "delete works", %{conn: conn} do
       insert(:badge_serial, badge_serial: "9999")
       conn = delete(conn, ~p"/admin/rfid", %{"badge_serial" => "9999"})

test/support/helpers.ex

@@ -15,4 +15,30 @@ defmodule Test.Support.Helpers do
       end)
     end
   end
+
+  @doc """
+  returns a list of strings with newline at the end trimmed
+  """
+  defmacro capture_log(level \\ :info, do: expression) do
+    quote do
+      require Logger
+      original_level = Logger.level()
+      on_exit(fn -> Logger.configure(level: original_level) end)
+      Logger.configure(level: unquote(level))
+
+      {"", lines} =
+        ExUnit.CaptureLog.capture_log([colors: [enabled: false]], fn ->
+          unquote(expression)
+        end)
+        |> String.split("\n")
+        |> List.pop_at(-1)
+
+      # Reset the log level for the rest of the test.
+      # The on_exit above is needed for cleanup in case of a crash,
+      # but it runs when the test is done, not when this macro is done
+      Logger.configure(level: original_level)
+
+      lines
+    end
+  end
 end