Merge branch 'main' into bf-query-with-status-field

.github/workflows/deploy-base.yml

@@ -71,7 +71,7 @@ jobs:
       - uses: actions/checkout@v4
         with:
           fetch-depth: 0
-      - uses: getsentry/action-release@v1.10.2
+      - uses: getsentry/action-release@v1
         env:
           SENTRY_ORG: ${{ secrets.SENTRY_ORG }}
           SENTRY_PROJECT: ${{ secrets.SENTRY_PROJECT }}

assets/src/components/detours/detourPanels/activeDetourPanel.tsx

@@ -15,7 +15,6 @@ import {
   CopyButton,
   MissedStops,
 } from "../detourPanelComponents"
-import inTestGroup, { TestGroups } from "../../../userInTestGroup"
 import { timeAgoLabelFromDate } from "../../../util/dateTime"
 import useCurrentTime from "../../../hooks/useCurrentTime"
 
@@ -80,10 +79,7 @@ export const ActiveDetourPanel = ({
           Active Detour
         </h1>
         {backButton}
-        {/* TODO: temporary test group until I get the copy logic hooked up */}
-        {inTestGroup(TestGroups.CopyButton) && (
-          <CopyButton detourText={copyableDetourText} />
-        )}
+        <CopyButton detourText={copyableDetourText} />
       </Panel.Header>
 
       <Panel.Body className="d-flex flex-column">

assets/src/components/detours/detourPanels/pastDetourPanel.tsx

@@ -10,7 +10,6 @@ import {
   CopyButton,
   MissedStops,
 } from "../detourPanelComponents"
-import inTestGroup, { TestGroups } from "../../../userInTestGroup"
 
 export interface PastDetourPanelProps {
   copyableDetourText: string
@@ -38,10 +37,7 @@ export const PastDetourPanel = ({
   <Panel as="article">
     <Panel.Header className="">
       <h1 className="c-diversion-panel__h1 my-3">View Past Detour</h1>
-      {/* TODO: temporary test group until I get the copy logic hooked up */}
-      {inTestGroup(TestGroups.CopyButton) && (
-        <CopyButton detourText={copyableDetourText} />
-      )}
+      <CopyButton detourText={copyableDetourText} />
     </Panel.Header>
 
     <Panel.Body className="d-flex flex-column">

assets/src/components/detours/diversionPage.tsx

@@ -619,11 +619,7 @@ export const DiversionPage = ({
             startPoint={startPoint ?? undefined}
             endPoint={endPoint ?? undefined}
             waypoints={waypoints}
-            unfinishedRouteSegments={
-              inTestGroup(TestGroups.BackwardsDetourPrevention)
-                ? unfinishedRouteSegments
-                : undefined
-            }
+            unfinishedRouteSegments={unfinishedRouteSegments}
             routeSegments={routeSegments}
             onAddWaypoint={addWaypoint}
             onClickOriginalShape={addConnectionPoint ?? (() => {})}

assets/src/userInTestGroup.ts

@@ -1,14 +1,12 @@
 import getTestGroups from "./userTestGroups"
 
 export enum TestGroups {
-  BackwardsDetourPrevention = "backwards-detour-prevention",
   DemoMode = "demo-mode",
   DetoursList = "detours-list",
   DetoursNotifications = "detours-notifications",
   DetoursPilot = "detours-pilot",
   MinimalLadderPage = "minimal-ladder-page",
   LateView = "late-view",
-  CopyButton = "copy-button",
   DeleteDraftDetours = "delete-draft-detours",
   DetoursOnLadder = "detours-on-ladder",
 }

assets/tests/components/detours/diversionPage.test.tsx

@@ -45,8 +45,6 @@ import { Err, Ok } from "../../../src/util/result"
 import { neverPromise } from "../../testHelpers/mockHelpers"
 
 import { originalRouteFactory } from "../../factories/originalRouteFactory"
-import getTestGroups from "../../../src/userTestGroups"
-import { TestGroups } from "../../../src/userInTestGroup"
 import { routePatternFactory } from "../../factories/routePattern"
 import { RoutesProvider } from "../../../src/contexts/routesContext"
 import routeFactory from "../../factories/route"
@@ -76,15 +74,13 @@ beforeEach(() => {
 })
 
 jest.mock("../../../src/api")
-jest.mock("../../../src/userTestGroups")
 
 beforeEach(() => {
   jest.mocked(fetchDetourDirections).mockReturnValue(neverPromise())
   jest.mocked(fetchUnfinishedDetour).mockReturnValue(neverPromise())
   jest.mocked(fetchFinishedDetour).mockReturnValue(neverPromise())
   jest.mocked(fetchNearestIntersection).mockReturnValue(neverPromise())
   jest.mocked(fetchRoutePatterns).mockReturnValue(neverPromise())
-  jest.mocked(getTestGroups).mockReturnValue([])
   jest.mocked(putDetourUpdate).mockReturnValue(neverPromise())
 })
 
@@ -958,10 +954,7 @@ describe("DiversionPage", () => {
     expect(originalRouteShape.get(container)).toBeVisible()
   })
 
-  test("replaces the original route shape with an unfinished segment after the start point is added if the user is in the right test group", async () => {
-    jest
-      .mocked(getTestGroups)
-      .mockReturnValue([TestGroups.BackwardsDetourPrevention])
+  test("replaces the original route shape with an unfinished segment after the start point is added", async () => {
     jest
       .mocked(fetchUnfinishedDetour)
       .mockResolvedValue(unfinishedDetourFactory.build())
@@ -987,34 +980,6 @@ describe("DiversionPage", () => {
     ).toHaveLength(1)
   })
 
-  test("does not replace the original route shape with an unfinished segment after the start point is added if the user is not in the right test group", async () => {
-    jest.mocked(getTestGroups).mockReturnValue([])
-    const promise = Promise.resolve(unfinishedDetourFactory.build())
-    jest.mocked(fetchUnfinishedDetour).mockReturnValue(promise)
-
-    const { container } = render(<DiversionPage />)
-
-    act(() => {
-      fireEvent.click(originalRouteShape.get(container))
-    })
-
-    await act(async () => {
-      await promise
-    })
-
-    expect(originalRouteShape.interactive.getAll(container)).toHaveLength(1)
-    expect(originalRouteShape.diverted.getAll(container)).toHaveLength(0)
-
-    expect(originalRouteShape.not.interactive.getAll(container)).toHaveLength(1)
-
-    expect(
-      originalRouteShape.afterStartPoint.not.interactive.getAll(container)
-    ).toHaveLength(0)
-    expect(
-      originalRouteShape.afterStartPoint.interactive.getAll(container)
-    ).toHaveLength(0)
-  })
-
   test("replaces the original route shape with a diverted segment after the end point is added", async () => {
     jest
       .mocked(fetchFinishedDetour)

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