diff --git a/assets/src/models/createDetourMachine.ts b/assets/src/models/createDetourMachine.ts index 82335462c..360c1d1cd 100644 --- a/assets/src/models/createDetourMachine.ts +++ b/assets/src/models/createDetourMachine.ts @@ -11,6 +11,7 @@ import { } from "../api" import { DetourShape, FinishedDetour } from "./detour" import { fullStoryEvent } from "../helpers/fullStory" +import { type, optional, coerce, date, string } from "superstruct" export const createDetourMachine = setup({ types: { @@ -35,6 +36,8 @@ export const createDetourMachine = setup({ selectedDuration?: string selectedReason?: string + + activatedAt?: Date }, input: {} as @@ -623,6 +626,11 @@ export const createDetourMachine = setup({ }, "detour.share.activate-modal.activate": { target: "Done", + actions: assign({ + // Record current time, should be done on the backend, + // but that requires a larger refactor of the state machine + activatedAt: new Date(), + }), }, }, }, @@ -701,3 +709,15 @@ export const createDetourMachine = setup({ export type CreateDetourMachineInput = InputFrom< ActorLogicFrom > + +/** + * Defines expected keys and type coercions in Superstruct to enable the + * {@linkcode createDetourMachine} to use rich types when rehydrating from a + * API response. + */ +export const DetourSnapshotData = type({ + context: type({ + // Convert serialized dates back into `Date`'s + activatedAt: optional(coerce(date(), string(), (str) => new Date(str))), + }), +}) diff --git a/assets/src/models/detour.ts b/assets/src/models/detour.ts index ec1955772..9b6d31f3d 100644 --- a/assets/src/models/detour.ts +++ b/assets/src/models/detour.ts @@ -1,7 +1,10 @@ import { LatLngLiteral } from "leaflet" import { ShapePoint, Stop } from "../schedule" -import { CreateDetourMachineInput } from "./createDetourMachine" -import { any, Infer, number, string, type } from "superstruct" +import { + CreateDetourMachineInput, + DetourSnapshotData, +} from "./createDetourMachine" +import { Infer, number, string, type } from "superstruct" export interface DetourWithState { author: string @@ -11,7 +14,7 @@ export interface DetourWithState { export const DetourWithStateData = type({ author: string(), - state: any(), + state: DetourSnapshotData, updated_at: number(), }) diff --git a/lib/skate/detours/db/detour.ex b/lib/skate/detours/db/detour.ex index 72f51f116..060dd19cf 100644 --- a/lib/skate/detours/db/detour.ex +++ b/lib/skate/detours/db/detour.ex @@ -12,12 +12,15 @@ defmodule Skate.Detours.Db.Detour do field :state, :map belongs_to :author, User + # When this detour was activated + field :activated_at, :utc_datetime_usec + timestamps() end def changeset(detour, attrs) do detour - |> cast(attrs, [:state]) + |> cast(attrs, [:state, :activated_at]) |> validate_required([:state]) end end diff --git a/lib/skate/detours/detours.ex b/lib/skate/detours/detours.ex index 3d0853867..53e0b2fc1 100644 --- a/lib/skate/detours/detours.ex +++ b/lib/skate/detours/detours.ex @@ -203,7 +203,7 @@ defmodule Skate.Detours.Detours do |> Skate.Repo.insert( returning: true, conflict_target: [:id], - on_conflict: {:replace, [:state, :updated_at]} + on_conflict: {:replace_all_except, [:inserted_at]} ) case detour_db_result do diff --git a/lib/skate/detours/snapshot_serde.ex b/lib/skate/detours/snapshot_serde.ex index 43549d558..ad6684ab4 100644 --- a/lib/skate/detours/snapshot_serde.ex +++ b/lib/skate/detours/snapshot_serde.ex @@ -21,11 +21,18 @@ defmodule Skate.Detours.SnapshotSerde do %{ # Save Snapshot to DB until we've fully transitioned to serializing # snapshots from DB data - state: snapshot + state: snapshot, + activated_at: activated_at_from_snapshot(snapshot) } ) end + defp activated_at_from_snapshot(%{"context" => %{"activatedAt" => activated_at}}), + do: activated_at + + defp activated_at_from_snapshot(_), + do: nil + @doc """ Extracts the Detour ID from a XState Snapshot """ @@ -89,7 +96,8 @@ defmodule Skate.Detours.SnapshotSerde do "finishedDetour" => finisheddetour_from_detour(detour), "editedDirections" => editeddirections_from_detour(detour), "selectedDuration" => selectedduration_from_detour(detour), - "selectedReason" => selectedreason_from_detour(detour) + "selectedReason" => selectedreason_from_detour(detour), + "activatedAt" => activated_at_from_detour(detour) }) end @@ -281,6 +289,25 @@ defmodule Skate.Detours.SnapshotSerde do defp selectedreason_from_detour(_), do: nil + defp activated_at_from_detour(%Detour{activated_at: %DateTime{} = activated_at}) do + activated_at + # For the time being, the frontend is responsible for generating the + # `activated_at` snapshot. Because browsers are limited to millisecond + # resolution and Ecto doesn't preserve the `milliseconds` field of a + # `DateTime`, we need to truncate the date if we want it to match what's in + # the stored snapshot. + # + # Once we're not trying to be equivalent with the stored snapshot, we could + # probably remove this. + # + # See `Skate.DetourFactory.browser_date/1` and `Skate.DetourFactory.db_date` + # for more context. + |> DateTime.truncate(:millisecond) + |> DateTime.to_iso8601() + end + + defp activated_at_from_detour(%Detour{activated_at: nil}), do: nil + # defp snapshot_children_from_detour(%Detour{snapshot_children: snapshot_children}), do: snapshot_children defp snapshot_children_from_detour(%Detour{ state: %{ diff --git a/priv/repo/async_migrations/20241218135700_backfill_detour_activated_at.exs b/priv/repo/async_migrations/20241218135700_backfill_detour_activated_at.exs new file mode 100644 index 000000000..54ff4c08b --- /dev/null +++ b/priv/repo/async_migrations/20241218135700_backfill_detour_activated_at.exs @@ -0,0 +1,49 @@ +defmodule Skate.Repo.Migrations.BackfillDetourActivatedAt do + # https://fly.io/phoenix-files/backfilling-data/ + + + import Ecto.Query + use Ecto.Migration + + @disable_ddl_transaction true + @disable_migration_lock true + + def up do + # The 'backfilling-data' blog post suggests using a "throttle" function + # so that we don't update too many at once, but we currently have less than + # 1000 detours, so I think this will be negligable and not worth the + # complexity cost at _this_ time. + repo().update_all( + from( + r in Skate.Repo.Migrations.BackfillDetourActivatedAt.MigratingSchema, + select: r.id, + where: not is_nil(r.state["value"]["Detour Drawing"]["Active"]) and is_nil(r.activated_at), + update: [set: [activated_at: r.updated_at]] + ), + [], + log: :info + ) + end + + def down, do: :ok +end + +defmodule Skate.Repo.Migrations.BackfillDetourActivatedAt.MigratingSchema do + @moduledoc """ + Detours database table schema frozen at this point in time. + """ + + use Skate.Schema + + alias Skate.Settings.Db.User + + typed_schema "detours" do + field :state, :map + belongs_to :author, User + + # When this detour was activated + field :activated_at, :utc_datetime_usec + + timestamps() + end +end diff --git a/priv/repo/migrations/20241211183227_add_detour_activated_at_field.exs b/priv/repo/migrations/20241211183227_add_detour_activated_at_field.exs new file mode 100644 index 000000000..ad214f733 --- /dev/null +++ b/priv/repo/migrations/20241211183227_add_detour_activated_at_field.exs @@ -0,0 +1,9 @@ +defmodule Skate.Repo.Migrations.AddDetourActivatedAtField do + use Ecto.Migration + + def change do + alter table(:detours) do + add :activated_at, :utc_datetime_usec + end + end +end diff --git a/test/skate_web/controllers/detours_controller_test.exs b/test/skate_web/controllers/detours_controller_test.exs index df6672f26..bc2097dba 100644 --- a/test/skate_web/controllers/detours_controller_test.exs +++ b/test/skate_web/controllers/detours_controller_test.exs @@ -81,6 +81,23 @@ defmodule SkateWeb.DetoursControllerTest do assert Skate.Repo.aggregate(Notifications.Db.Detour, :count) == 1 end + @tag :authenticated + test "adds `activated_at` field when provided", %{conn: conn} do + %Skate.Detours.Db.Detour{id: id, state: snapshot, activated_at: nil} = insert(:detour) + + activated_at_time = + DateTime.utc_now() |> Skate.DetourFactory.browser_date() |> Skate.DetourFactory.db_date() + + put(conn, ~p"/api/detours/update_snapshot", %{ + "snapshot" => snapshot |> activated(activated_at_time) |> with_id(id) + }) + + Process.sleep(10) + + assert Skate.Detours.Detours.get_detour!(id).activated_at == + activated_at_time + end + @tag :authenticated test "does not create a new notification if detour was already activated", %{conn: conn} do setup_notification_server() @@ -238,11 +255,45 @@ defmodule SkateWeb.DetoursControllerTest do put(conn, "/api/detours/update_snapshot", %{"snapshot" => detour_snapshot}) - conn = get(conn, "/api/detours/#{detour_id}") + {conn, log} = + CaptureLog.with_log(fn -> + get(conn, "/api/detours/#{detour_id}") + end) + + refute log =~ + "Serialized detour doesn't match saved snapshot. Falling back to snapshot for detour_id=#{detour_id}" assert detour_snapshot == json_response(conn, 200)["data"]["state"] end + @tag :authenticated + test "serialized snapshot `activatedAt` value is formatted as iso-8601", %{conn: conn} do + activated_at = Skate.DetourFactory.browser_date() + + %{id: id} = + detour = + :detour + |> build() + |> activated(activated_at) + |> insert() + + # Make ID match snapshot + detour + |> Skate.Detours.Detours.change_detour(detour |> update_id() |> Map.from_struct()) + |> Skate.Repo.update!() + + {conn, log} = + CaptureLog.with_log(fn -> + get(conn, "/api/detours/#{id}") + end) + + refute log =~ + "Serialized detour doesn't match saved snapshot. Falling back to snapshot for detour_id=#{id}" + + assert DateTime.to_iso8601(activated_at) == + json_response(conn, 200)["data"]["state"]["context"]["activatedAt"] + end + @tag :authenticated test "log an error if the serialized detour does not match db state", %{conn: conn} do detour_id = 4 diff --git a/test/support/factories/detour_factory.ex b/test/support/factories/detour_factory.ex index 0087b1174..723f3f261 100644 --- a/test/support/factories/detour_factory.ex +++ b/test/support/factories/detour_factory.ex @@ -57,12 +57,27 @@ defmodule Skate.DetourFactory do put_in(snapshot["context"]["uuid"], id) end - def activated(%Skate.Detours.Db.Detour{} = detour) do - %{detour | state: activated(detour.state)} + def update_id(%Skate.Detours.Db.Detour{id: id} = detour) do + with_id(detour, id) end - def activated(%{"value" => %{}} = state) do - put_in(state["value"], %{"Detour Drawing" => %{"Active" => "Reviewing"}}) + def activated(update_arg, activated_at \\ DateTime.utc_now()) + + def activated(%Skate.Detours.Db.Detour{} = detour, activated_at) do + activated_at = Skate.DetourFactory.browser_date(activated_at) + %{detour | state: activated(detour.state, activated_at), activated_at: activated_at} + end + + def activated(%{"value" => %{}, "context" => %{}} = state, activated_at) do + state = + put_in(state["value"], %{"Detour Drawing" => %{"Active" => "Reviewing"}}) + + put_in( + state["context"]["activatedAt"], + activated_at + |> Skate.DetourFactory.browser_date() + |> DateTime.to_iso8601() + ) end def deactivated(%Skate.Detours.Db.Detour{} = detour) do @@ -95,4 +110,32 @@ defmodule Skate.DetourFactory do end end end + + @doc """ + Browsers cannot generate javascript `Date` objects with more precision than a + `millisecond` for security reasons. + https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Date/now#reduced_time_precision + + This function truncates a `DateTime` to milliseconds to create `DateTime` objects + that are similar to that of one made in a Browser JS context. + """ + def browser_date(%DateTime{} = date \\ DateTime.utc_now()) do + DateTime.truncate(date, :millisecond) + end + + @doc """ + While a Browser may generate a date truncated to milliseconds + (see `browser_date` for more context) a `DateTime` stored into Postgres with + the `:utc_datetime_usec` type does not store the extra information about the + non-presence of nanoseconds that a `DateTime` object does. + This means a `DateTime` object that's been truncated by `browser_date` cannot + be compared to a `DateTime` object reconstructed by Ecto after a Database query. + + This function adds 0 nanoseconds to a `DateTime` object to make the `DateTime` + object match what Ecto would return to make testing easier when comparing + values. + """ + def db_date(%DateTime{} = date) do + DateTime.add(date, 0, :nanosecond) + end end