Update detour serde validation check (#2949)

lib/skate/detours/db/detour.ex

@@ -49,11 +49,19 @@ defmodule Skate.Detours.Db.Detour do
   def changeset(detour, attrs) do
     detour
     |> cast(attrs, [:state, :activated_at])
+    |> validate_activated_at()
     |> add_status()
     |> validate_required([:state, :status])
     |> foreign_key_constraint(:author_id)
   end
 
+  defp validate_activated_at(changeset) do
+    case fetch_change(changeset, :activated_at) do
+      {:ok, nil} -> delete_change(changeset, :activated_at)
+      _ -> changeset
+    end
+  end
+
   defp add_status(changeset) do
     case fetch_change(changeset, :state) do
       {:ok, state} ->

lib/skate/detours/snapshot_serde.ex

@@ -51,11 +51,23 @@ defmodule Skate.Detours.SnapshotSerde do
     compare_snapshots(detour, serialized_snapshot)
   end
 
-  def compare_snapshots(%Detour{state: state}, serialized_snapshot) do
-    matches = serialized_snapshot === state
+  def compare_snapshots(
+        %Detour{state: %{"context" => state_context} = state},
+        %{"context" => serialized_context} = serialized_snapshot
+      ) do
+    relevant_state_keys = Map.keys(serialized_snapshot)
+    relevant_context_keys = Map.keys(serialized_context)
+    scoped_state_context = Map.take(state_context, relevant_context_keys)
+
+    scoped_state =
+      state
+      |> Map.take(relevant_state_keys)
+      |> Map.put("context", scoped_state_context)
+
+    matches = serialized_snapshot === scoped_state
     diff = MapDiff.diff(state, serialized_snapshot)
 
-    {matches, diff}
+    {matches, diff, scoped_state}
   end
 
   defp serialize_snapshot(detour) do
@@ -68,19 +80,19 @@ defmodule Skate.Detours.SnapshotSerde do
     }
   end
 
-  defp validate_serialized_snapshot(%Detour{id: id, state: state} = detour) do
+  defp validate_serialized_snapshot(%Detour{id: id} = detour) do
     serialized_snapshot = serialize_snapshot(detour)
-    {matches, diff} = compare_snapshots(detour, serialized_snapshot)
+    {matches, diff, scoped_state} = compare_snapshots(detour, serialized_snapshot)
 
     if matches do
       serialized_snapshot
     else
-      state = fix_snapshot_activated_at(state, detour)
+      scoped_state = fix_snapshot_activated_at(scoped_state, detour)
       Sentry.capture_message(mismatch_message(id), extra: %{diff: diff})
 
       Logger.error("#{mismatch_message(id)} #{diff_details(diff)}")
 
-      state
+      scoped_state
     end
   end
 

lib/skate_web/controllers/detours_admin_controller.ex

@@ -47,7 +47,7 @@ defmodule SkateWeb.DetoursAdminController do
   def show(conn, %{"id" => id}) do
     detour = Detours.get_detour!(id)
     author = User.get_by_id(detour.author_id)
-    {matches, detour_diff} = Skate.Detours.SnapshotSerde.compare_snapshots(detour)
+    {matches, detour_diff, _} = Skate.Detours.SnapshotSerde.compare_snapshots(detour)
 
     conn
     |> assign(:detour, Detours.db_detour_to_detour(detour))

test/skate/detours/db_test.exs

@@ -59,6 +59,21 @@ defmodule Skate.Detours.DbTest do
       assert %Ecto.Changeset{} = Detours.change_detour(detour)
     end
 
+    test "change_detour/1 does not allow a non-nil activated_at column to be updated with nil" do
+      detour = detour_fixture()
+
+      activated_at = DateTime.to_iso8601(Skate.DetourFactory.browser_date())
+
+      changeset = Detours.change_detour(detour, %{"activated_at" => activated_at})
+      assert {:ok, _} = fetch_change(changeset, :activated_at)
+
+      assert :error =
+               changeset
+               |> Map.get(:data)
+               |> Detours.change_detour(%{"activated_at" => nil})
+               |> fetch_change(:activated_at)
+    end
+
     test "change_detour/1 changes :status when :state updates" do
       detour = build(:detour, status: nil)
 

test/skate_web/controllers/detours_controller_test.exs

@@ -402,11 +402,11 @@ defmodule SkateWeb.DetoursControllerTest do
         :detour_snapshot
         |> build()
         |> with_id(detour_id)
+        |> with_direction(:inbound)
 
       conn = put(conn, "/api/detours/update_snapshot", %{"snapshot" => detour_snapshot})
 
-      # Changing the status is a sure way to force a fallback, as it should always be "active"
-      edited_snapshot = put_in(detour_snapshot["status"], nil)
+      edited_snapshot = put_in(detour_snapshot["context"]["routePattern"]["directionId"], 0)
 
       detour_id
       |> Detours.get_detour!()
@@ -425,6 +425,36 @@ defmodule SkateWeb.DetoursControllerTest do
                }
              } = json_response(conn, 200)
     end
+
+    @tag :authenticated
+    test "does not log an error if mismatch is an irrelevant field", %{conn: conn} do
+      detour_id = 5
+
+      detour_snapshot =
+        :detour_snapshot
+        |> build()
+        |> with_id(detour_id)
+
+      conn = put(conn, "/api/detours/update_snapshot", %{"snapshot" => detour_snapshot})
+
+      edited_snapshot = put_in(detour_snapshot["irrelevantField"], "someData")
+
+      detour_id
+      |> Detours.get_detour!()
+      |> Detours.change_detour(%{state: edited_snapshot})
+      |> Skate.Repo.update()
+
+      conn = get(conn, "/api/detours/#{detour_id}")
+
+      # Serializer does not return the extra data
+      assert %{
+               "data" => %{
+                 "author" => _,
+                 "state" => ^detour_snapshot,
+                 "updated_at" => _
+               }
+             } = json_response(conn, 200)
+    end
   end
 
   describe "detours/2" do