From e04192b5b149cbd252ecc1bfa1508992082283f8 Mon Sep 17 00:00:00 2001 From: "robbie.sundstrom" Date: Tue, 11 Aug 2026 11:20:02 -0400 Subject: [PATCH 1/3] feat: APIs for Screen Config Clients --- .envrc.template | 5 + config/runtime.exs | 2 + lib/screens/config/screen_config.ex | 2 + lib/screens/screen_configs.ex | 130 ++++++++- .../screen_configs_api_controller.ex | 38 +++ lib/screens_web/plug/ensure_bearer_token.ex | 46 ++++ lib/screens_web/router.ex | 12 + .../screen_configs_api_controller_test.exs | 246 ++++++++++++++++++ .../plug/ensure_bearer_token_test.exs | 73 ++++++ test/support/mocks.ex | 1 + 10 files changed, 551 insertions(+), 4 deletions(-) create mode 100644 lib/screens_web/controllers/screen_configs_api_controller.ex create mode 100644 lib/screens_web/plug/ensure_bearer_token.ex create mode 100644 test/screens_web/controllers/screen_configs_api_controller_test.exs create mode 100644 test/screens_web/plug/ensure_bearer_token_test.exs diff --git a/.envrc.template b/.envrc.template index b6fe23bae..1d0dda374 100644 --- a/.envrc.template +++ b/.envrc.template @@ -14,6 +14,11 @@ export API_V3_URL="https://api-v3.mbta.com" export SECRET_KEY_BASE="local_secret_key_base_at_least_64_bytes_________________________________" +export SCREENS_API_CLIENT_KEY="local_screens_api_client_key" + ## Postgres configuration: username and password for local server # export DATABASE_USER= # export DATABASE_PASSWORD= + +## Feature flag. Setting to true uses the Postgres DB for screen configurations +export CONFIG_MIGRATION="false" \ No newline at end of file diff --git a/config/runtime.exs b/config/runtime.exs index e01609c41..a6e788782 100644 --- a/config/runtime.exs +++ b/config/runtime.exs @@ -4,6 +4,8 @@ # remember to add this file to your .gitignore. import Config +config :screens, :config_migration, System.get_env("CONFIG_MIGRATION", "false") == "true" + config :screens, Screens.Repo, username: System.fetch_env!("DATABASE_USER"), password: System.get_env("DATABASE_PASSWORD"), diff --git a/lib/screens/config/screen_config.ex b/lib/screens/config/screen_config.ex index 9839ab687..851bbe720 100644 --- a/lib/screens/config/screen_config.ex +++ b/lib/screens/config/screen_config.ex @@ -5,6 +5,8 @@ defmodule Screens.Config.ScreenConfig do import Ecto.Changeset + @derive {Jason.Encoder, except: [:__meta__]} + @type t() :: %__MODULE__{ id: String.t(), config: Config.t() diff --git a/lib/screens/screen_configs.ex b/lib/screens/screen_configs.ex index d396d4516..bdf2f1c24 100644 --- a/lib/screens/screen_configs.ex +++ b/lib/screens/screen_configs.ex @@ -4,15 +4,17 @@ defmodule Screens.ScreenConfigs do """ import Ecto.Query + import Screens.Inject - alias Screens.Config.Fetch, as: ConfigFetch alias Screens.Config.ScreenConfig alias Screens.Repo + @config_fetcher injected(Screens.Config.Fetch) + @spec import_from_file() :: {:ok, %{upserted: integer(), deleted: integer()}} | {:error, any()} def import_from_file do # This should be a part of post_config_migration_cleanup - with {:ok, config, _version} <- ConfigFetch.fetch_config(), + with {:ok, config, _version} <- @config_fetcher.fetch_config(), config = Jason.decode!(config), screens when is_map(screens) <- Map.get(config, "screens", %{}) do screen_ids = Map.keys(screens) @@ -28,8 +30,41 @@ defmodule Screens.ScreenConfigs do end end - def list do - Repo.all(ScreenConfig) + @spec list_all() :: String.t() | :error + def list_all do + # Returns all Configs as a JSON to be used by Screens Admin + if config_migration_enabled?() do + screens = + ScreenConfig + |> Repo.all() + |> Map.new(fn %ScreenConfig{id: id, config: config} -> {id, config} end) + + Jason.encode!(%{screens: screens}) + else + with {:ok, config, _version} <- @config_fetcher.fetch_config() do + config + end + end + end + + @spec list_screen_configs() :: [ScreenConfig.t()] + def list_screen_configs do + # Returns all configs as a list of ScreenConfig structs to be used by Screens Admin + # The API controller handles the JSON encoding and formatting for the response. + # As part of post_config_migration_cleanup, this and above function can be cleaned up + if config_migration_enabled?() do + Repo.all(ScreenConfig) + else + with {:ok, config_json, _version} <- @config_fetcher.fetch_config(), + {:ok, decoded_config} <- Jason.decode(config_json), + screens when is_map(screens) <- Map.get(decoded_config, "screens", %{}) do + Enum.map(screens, fn {id, config} -> + %ScreenConfig{id: id, config: config} + end) + else + _ -> [] + end + end end @doc """ @@ -46,4 +81,91 @@ defmodule Screens.ScreenConfigs do conflict_target: :id ) end + + @spec upsert_list([%{:id => String.t(), :config => map()}]) :: :ok | {:error, any()} + defp upsert_list(updates) do + Enum.reduce_while(updates, :ok, fn update, _acc -> + case upsert_screen_config(update) do + {:ok, _} -> {:cont, :ok} + {:error, reason} -> {:halt, {:error, reason}} + end + end) + end + + @doc """ + Updates and deletes multiple screen configs. + Accepts a list of maps with :id and :config keys for updates, and a list of screen IDs for deletions. + """ + @spec commit_updates([%{:id => String.t(), :config => map()}], [String.t()]) :: + :ok | {:error, any()} + def commit_updates(updates, deletes \\ []) do + if config_migration_enabled?() do + update_to_postgres(updates, deletes) + else + # This branch will be removed as part of post_config_migration_cleanup. + # When the feature flag is disabled, continue to update the JSON config. + update_to_legacy_json(updates, deletes) + end + end + + @spec update_to_postgres([%{:id => String.t(), :config => map()}], [String.t()]) :: + :ok | {:error, any()} + defp update_to_postgres(updates, deletes) do + with :ok <- upsert_list(updates) do + perform_deletes(deletes) + end + end + + @spec perform_deletes([String.t()]) :: :ok | {:error, any()} + defp perform_deletes(deletes) do + Enum.reduce_while(deletes, :ok, fn id, _acc -> + Repo.delete_all(from s in ScreenConfig, where: s.id == ^id) + {:cont, :ok} + end) + end + + defp update_to_legacy_json(updates, deletes) do + # This will be a part of post_config_migration_cleanup + # Merges updates into the existing config and removes deleted screens, then writes back to the legacy source. + # We need to fetch the existing config before writing updates to prevent overwriting any existing configs. + with {:ok, config_json, _version} <- @config_fetcher.fetch_config(), + {:ok, decoded_config} <- Jason.decode(config_json), + screens when is_map(screens) <- Map.get(decoded_config, "screens", %{}) do + updated_screens = + Enum.reduce(updates, screens, fn update, acc -> + id = extract_id(update) + config = extract_config(update) + + existing_config = Map.get(acc, id, %{}) + merged_config = Map.merge(existing_config, config) + Map.put(acc, id, merged_config) + end) + + final_screens = Map.drop(updated_screens, deletes) + updated_config = Map.put(decoded_config, "screens", final_screens) + + case Jason.encode(updated_config) do + {:ok, encoded_config} -> @config_fetcher.put_config(encoded_config) + {:error, reason} -> {:error, reason} + end + else + _ -> :error + end + end + + @spec config_migration_enabled?() :: boolean() + def config_migration_enabled? do + # This will be a part of post_config_migration_cleanup + Application.get_env(:screens, :config_migration, false) + end + + # This will be a part of post_config_migration_cleanup + defp extract_id(%{"id" => id}), do: id + defp extract_id(%{id: id}), do: id + defp extract_id(_), do: nil + + # This will be a part of post_config_migration_cleanup + defp extract_config(%{"config" => config}), do: config + defp extract_config(%{config: config}), do: config + defp extract_config(_), do: %{} end diff --git a/lib/screens_web/controllers/screen_configs_api_controller.ex b/lib/screens_web/controllers/screen_configs_api_controller.ex new file mode 100644 index 000000000..b7e58cba3 --- /dev/null +++ b/lib/screens_web/controllers/screen_configs_api_controller.ex @@ -0,0 +1,38 @@ +defmodule ScreensWeb.ScreenConfigsApiController do + use ScreensWeb, :controller + + alias Screens.ScreenConfigs + + def index(conn, _params) do + screen_configs = + ScreenConfigs.list_screen_configs() + |> Enum.map(fn screen_config -> + %{ + id: screen_config.id, + config: screen_config.config + } + end) + + json(conn, %{screen_configs: screen_configs}) + end + + def update(conn, %{"screen_configs" => screen_configs} = params) when is_list(screen_configs) do + deleted_screen_ids = Map.get(params, "deleted_screen_ids", []) + + case ScreenConfigs.commit_updates(screen_configs, deleted_screen_ids) do + :ok -> + json(conn, %{success: true}) + + {:error, reason} -> + conn + |> put_status(500) + |> json(%{success: false, error: "Failed to update screen configs: #{inspect(reason)}"}) + end + end + + def update(conn, _params) do + conn + |> put_status(400) + |> json(%{success: false, error: "screen_configs parameter is required"}) + end +end diff --git a/lib/screens_web/plug/ensure_bearer_token.ex b/lib/screens_web/plug/ensure_bearer_token.ex new file mode 100644 index 000000000..550bb176c --- /dev/null +++ b/lib/screens_web/plug/ensure_bearer_token.ex @@ -0,0 +1,46 @@ +defmodule ScreensWeb.Plug.EnsureBearerToken do + @moduledoc """ + Ensures requests include a valid bearer token in the Authorization header. + + The expected token is loaded from an environment variable. + """ + + import Plug.Conn + + def init(opts) do + Keyword.fetch!(opts, :env_var) + end + + def call(conn, env_var) do + configured_token = System.get_env(env_var) + bearer_token = bearer_token_from_header(conn) + + if valid_token?(configured_token, bearer_token) do + conn + else + unauthorized(conn) + end + end + + defp bearer_token_from_header(conn) do + case get_req_header(conn, "authorization") do + ["Bearer " <> token] -> token + _ -> nil + end + end + + defp valid_token?(configured_token, bearer_token) + when is_binary(configured_token) and is_binary(bearer_token) do + byte_size(configured_token) == byte_size(bearer_token) and + Plug.Crypto.secure_compare(configured_token, bearer_token) + end + + defp valid_token?(_configured_token, _bearer_token), do: false + + defp unauthorized(conn) do + conn + |> put_resp_content_type("application/json") + |> send_resp(401, "{\"error\":\"unauthorized\"}") + |> halt() + end +end diff --git a/lib/screens_web/router.ex b/lib/screens_web/router.ex index 72f7ebebc..8aa57d269 100644 --- a/lib/screens_web/router.ex +++ b/lib/screens_web/router.ex @@ -37,6 +37,10 @@ defmodule ScreensWeb.Router do plug(ScreensWeb.Plug.EnsureScreensGroup) end + pipeline :screens_api_client_auth do + plug(ScreensWeb.Plug.EnsureBearerToken, env_var: "SCREENS_API_CLIENT_KEY") + end + scope "/", ScreensWeb do get "/_health", HealthController, :index end @@ -65,6 +69,7 @@ defmodule ScreensWeb.Router do pipe_through [:redirect_prod_http, :api, :auth, :ensure_auth, :ensure_screens_group] get "/", AdminApiController, :index + post "/screen_configs", AdminApiController, :update_screen_configs post "/screens/validate", AdminApiController, :validate post "/screens/validate/:id", AdminApiController, :validate post "/screens/confirm", AdminApiController, :confirm @@ -132,4 +137,11 @@ defmodule ScreensWeb.Router do get "/screens_by_alert", ScreensByAlertController, :index end + + scope "/api", ScreensWeb do + pipe_through [:redirect_prod_http, :api, :screens_api_client_auth] + + get "/screen_configs", ScreenConfigsApiController, :index + post "/screen_configs", ScreenConfigsApiController, :update + end end diff --git a/test/screens_web/controllers/screen_configs_api_controller_test.exs b/test/screens_web/controllers/screen_configs_api_controller_test.exs new file mode 100644 index 000000000..17cc1aa37 --- /dev/null +++ b/test/screens_web/controllers/screen_configs_api_controller_test.exs @@ -0,0 +1,246 @@ +defmodule ScreensWeb.ScreenConfigsApiControllerTest do + use ScreensWeb.ConnCase + + import ExUnit.CaptureLog + import Mox + + alias Screens.Config.ScreenConfig + alias Screens.Repo + + @env_var "SCREENS_API_CLIENT_KEY" + + # Test config constants + @screen_dup_config %{"app_id" => "dup_v2"} + @screen_busway_config %{"app_id" => "busway_v2"} + @screen_dup_old_config %{"app_id" => "dup_old"} + + @legacy_config %{ + "screens" => %{ + "dup_1" => @screen_dup_config, + "busway_1" => @screen_busway_config + } + } + + @legacy_config_dup_only %{ + "screens" => %{ + "dup_1" => @screen_dup_old_config + } + } + + setup do + :ok = Ecto.Adapters.SQL.Sandbox.checkout(Repo) + + previous_value = System.get_env(@env_var) + previous_config_migration = Application.get_env(:screens, :config_migration) + + on_exit(fn -> + restore_env_var(@env_var, previous_value) + restore_app_env(:screens, :config_migration, previous_config_migration) + end) + + :ok + end + + setup :verify_on_exit! + + describe "index/2" do + test "returns 401 when bearer token is missing", %{conn: conn} do + System.put_env(@env_var, "shared-secret") + + capture_log([level: :warning], fn -> + conn = get(conn, "/api/screen_configs") + + assert conn.status == 401 + assert conn.resp_body == "{\"error\":\"unauthorized\"}" + end) + end + + test "returns all screen configs as JSON when bearer token is valid", %{conn: conn} do + System.put_env(@env_var, "shared-secret") + Application.put_env(:screens, :config_migration, true) + + Repo.insert!(%ScreenConfig{id: "screen-1", config: @screen_dup_config}) + Repo.insert!(%ScreenConfig{id: "screen-2", config: @screen_busway_config}) + + conn = + conn + |> put_req_header("authorization", "Bearer shared-secret") + |> get("/api/screen_configs") + + assert conn.status == 200 + + %{"screen_configs" => screen_configs} = json_response(conn, 200) + + assert length(screen_configs) == 2 + configs_by_id = Map.new(screen_configs, &{&1["id"], &1["config"]}) + assert configs_by_id["screen-1"] == @screen_dup_config + assert configs_by_id["screen-2"] == @screen_busway_config + end + + test "returns screen configs from legacy config source when migration flag is false", %{ + conn: conn + } do + System.put_env(@env_var, "shared-secret") + Application.put_env(:screens, :config_migration, false) + + # Mock fetch_config to return our test config + expect(Screens.Config.Fetch.Mock, :fetch_config, fn -> + {:ok, Jason.encode!(@legacy_config), 1} + end) + + conn = + conn + |> put_req_header("authorization", "Bearer shared-secret") + |> get("/api/screen_configs") + + assert conn.status == 200 + + %{"screen_configs" => screen_configs} = json_response(conn, 200) + + # Verify configs match our constants + configs_by_id = Map.new(screen_configs, &{&1["id"], &1["config"]}) + assert configs_by_id["dup_1"] == @screen_dup_config + assert configs_by_id["busway_1"] == @screen_busway_config + end + end + + describe "update/2" do + test "returns 401 when bearer token is missing", %{conn: conn} do + System.put_env(@env_var, "shared-secret") + + capture_log([level: :warning], fn -> + conn = + post(conn, "/api/screen_configs", %{ + screen_configs: [%{id: "screen-1", config: %{"app_id" => "dup_v2"}}] + }) + + assert conn.status == 401 + assert conn.resp_body == "{\"error\":\"unauthorized\"}" + end) + end + + test "updates screen configs in Postgres when migration flag is true", %{conn: conn} do + System.put_env(@env_var, "shared-secret") + Application.put_env(:screens, :config_migration, true) + + Repo.insert!(%ScreenConfig{id: "screen-1", config: @screen_dup_config}) + + conn = + conn + |> put_req_header("authorization", "Bearer shared-secret") + |> post("/api/screen_configs", %{ + screen_configs: [ + %{id: "screen-1", config: @screen_busway_config} + ] + }) + + assert json_response(conn, 200) == %{"success" => true} + + updated = Repo.get!(ScreenConfig, "screen-1") + assert updated.config == @screen_busway_config + end + + test "updates full config when migration flag is false", %{conn: conn} do + System.put_env(@env_var, "shared-secret") + Application.put_env(:screens, :config_migration, false) + + # Mock the fetch and put to prevent writing to the fixture file + expect(Screens.Config.Fetch.Mock, :fetch_config, fn -> + {:ok, Jason.encode!(@legacy_config_dup_only), 1} + end) + + expect(Screens.Config.Fetch.Mock, :put_config, fn config -> + # Verify the updated config has the new screen 1001 config + {:ok, decoded} = Jason.decode(config) + screens = Map.get(decoded, "screens", %{}) + assert screens["dup_1"] == @screen_dup_config + :ok + end) + + conn = + conn + |> put_req_header("authorization", "Bearer shared-secret") + |> post("/api/screen_configs", %{ + screen_configs: [ + %{"id" => "dup_1", "config" => @screen_dup_config} + ] + }) + + assert json_response(conn, 200) == %{"success" => true} + end + + test "returns 400 when screen_configs param is missing", %{conn: conn} do + System.put_env(@env_var, "shared-secret") + + capture_log([level: :warning], fn -> + conn = + conn + |> put_req_header("authorization", "Bearer shared-secret") + |> post("/api/screen_configs", %{}) + + assert conn.status == 400 + response = json_response(conn, 400) + assert response["success"] == false + assert response["error"] == "screen_configs parameter is required" + end) + end + + test "deletes screen configs in Postgres when migration flag is true", %{conn: conn} do + System.put_env(@env_var, "shared-secret") + Application.put_env(:screens, :config_migration, true) + + Repo.insert!(%ScreenConfig{id: "screen-1", config: @screen_dup_config}) + Repo.insert!(%ScreenConfig{id: "screen-2", config: @screen_busway_config}) + + conn = + conn + |> put_req_header("authorization", "Bearer shared-secret") + |> post("/api/screen_configs", %{ + screen_configs: [], + deleted_screen_ids: ["screen-2"] + }) + + assert json_response(conn, 200) == %{"success" => true} + + # Verify deletion + assert Repo.get(ScreenConfig, "screen-1") + assert nil == Repo.get(ScreenConfig, "screen-2") + end + + test "deletes screens from legacy config when migration flag is false", %{conn: conn} do + System.put_env(@env_var, "shared-secret") + Application.put_env(:screens, :config_migration, false) + + # Mock the fetch and put to verify deletion logic + expect(Screens.Config.Fetch.Mock, :fetch_config, fn -> + {:ok, Jason.encode!(@legacy_config), 1} + end) + + expect(Screens.Config.Fetch.Mock, :put_config, fn config -> + {:ok, decoded} = Jason.decode(config) + screens = Map.get(decoded, "screens", %{}) + assert not Map.has_key?(screens, "busway_1") + assert screens["dup_1"] == @screen_dup_config + :ok + end) + + capture_log([level: :warning], fn -> + conn = + conn + |> put_req_header("authorization", "Bearer shared-secret") + |> post("/api/screen_configs", %{ + screen_configs: [], + deleted_screen_ids: ["busway_1"] + }) + + assert json_response(conn, 200) == %{"success" => true} + end) + end + end + + defp restore_env_var(env_var, nil), do: System.delete_env(env_var) + defp restore_env_var(env_var, value), do: System.put_env(env_var, value) + + defp restore_app_env(app, key, nil), do: Application.delete_env(app, key) + defp restore_app_env(app, key, value), do: Application.put_env(app, key, value) +end diff --git a/test/screens_web/plug/ensure_bearer_token_test.exs b/test/screens_web/plug/ensure_bearer_token_test.exs new file mode 100644 index 000000000..ad144f9d3 --- /dev/null +++ b/test/screens_web/plug/ensure_bearer_token_test.exs @@ -0,0 +1,73 @@ +defmodule ScreensWeb.Plug.EnsureBearerTokenTest do + use ScreensWeb.ConnCase + + alias ScreensWeb.Plug.EnsureBearerToken + + @env_var "SCREENS_API_CLIENT_KEY" + + setup do + previous_value = System.get_env(@env_var) + + on_exit(fn -> + restore_env_var(@env_var, previous_value) + end) + + :ok + end + + describe "init/1" do + test "returns configured env var name" do + assert EnsureBearerToken.init(env_var: @env_var) == @env_var + end + end + + describe "call/2" do + test "allows request when bearer token matches configured token", %{conn: conn} do + System.put_env(@env_var, "shared-secret") + + conn = + conn + |> put_req_header("authorization", "Bearer shared-secret") + |> EnsureBearerToken.call(@env_var) + + refute conn.halted + end + + test "halts with 401 when authorization header is missing", %{conn: conn} do + System.put_env(@env_var, "shared-secret") + + conn = EnsureBearerToken.call(conn, @env_var) + + assert conn.halted + assert conn.status == 401 + assert conn.resp_body == "{\"error\":\"unauthorized\"}" + end + + test "halts with 401 when bearer token does not match", %{conn: conn} do + System.put_env(@env_var, "shared-secret") + + conn = + conn + |> put_req_header("authorization", "Bearer wrong-secret") + |> EnsureBearerToken.call(@env_var) + + assert conn.halted + assert conn.status == 401 + end + + test "halts with 401 when configured token is unavailable", %{conn: conn} do + System.delete_env(@env_var) + + conn = + conn + |> put_req_header("authorization", "Bearer shared-secret") + |> EnsureBearerToken.call(@env_var) + + assert conn.halted + assert conn.status == 401 + end + end + + defp restore_env_var(env_var, nil), do: System.delete_env(env_var) + defp restore_env_var(env_var, value), do: System.put_env(env_var, value) +end diff --git a/test/support/mocks.ex b/test/support/mocks.ex index d3c621403..0d1fc42b7 100644 --- a/test/support/mocks.ex +++ b/test/support/mocks.ex @@ -1,6 +1,7 @@ injected_modules = [ Screens.Alerts.Alert, Screens.Config.Cache, + Screens.Config.Fetch, Screens.Elevator, Screens.Facilities.Facility, Screens.Headways, From 6607c7f8122158e99b04341cc6f130a7f930d85e Mon Sep 17 00:00:00 2001 From: "robbie.sundstrom" Date: Tue, 11 Aug 2026 11:21:18 -0400 Subject: [PATCH 2/3] feat: Use Postgres for Screen Configs behind feature flag on Screens Admin --- assets/src/components/admin/admin_form.tsx | 35 ++++--- assets/src/components/admin/editor.tsx | 14 ++- assets/src/components/admin/inspector.tsx | 29 +++++- assets/src/util/admin.tsx | 27 ++++++ .../controllers/admin_api_controller.ex | 26 +++++- lib/screens_web/router.ex | 1 - .../controllers/admin_api_controller_test.exs | 91 +++++++++++++++++++ 7 files changed, 204 insertions(+), 19 deletions(-) create mode 100644 test/screens_web/controllers/admin_api_controller_test.exs diff --git a/assets/src/components/admin/admin_form.tsx b/assets/src/components/admin/admin_form.tsx index ded3f48f2..6781d7d3b 100644 --- a/assets/src/components/admin/admin_form.tsx +++ b/assets/src/components/admin/admin_form.tsx @@ -44,28 +44,41 @@ const AdminValidateControls = ({ }; const AdminConfirmControls = ({ - confirmPath, + onConfirm, configRef, onCancel, onError, onSuccess, }): JSX.Element => { - const confirmFn = () => { - const config = configRef.current.value; - const dataToSubmit = { config }; - fetch.post(confirmPath, dataToSubmit).then((resultJson) => { - if (resultJson.success === true) { + const [isLoading, setIsLoading] = useState(false); + + const confirmFn = async () => { + setIsLoading(true); + try { + const config = configRef.current.value; + const parsedConfig = JSON.parse(config); + const result = await onConfirm(parsedConfig); + + if (result.success === true) { onSuccess(); } else { onError(); } - }); + } catch (_error) { + onError(); + } finally { + setIsLoading(false); + } }; return (
- - + +
); }; @@ -73,7 +86,7 @@ const AdminConfirmControls = ({ const AdminForm = ({ fetchConfig, validatePath, - confirmPath, + onConfirm, onUpdated, }): JSX.Element => { const [editable, setEditable] = useState(true); @@ -111,7 +124,7 @@ const AdminForm = ({ /> ) : ( setEditable(true)} onError={() => { diff --git a/assets/src/components/admin/editor.tsx b/assets/src/components/admin/editor.tsx index 89efaeff8..c63e8f868 100644 --- a/assets/src/components/admin/editor.tsx +++ b/assets/src/components/admin/editor.tsx @@ -6,6 +6,7 @@ import { type AppId, type Config, SCREEN_APP_ENTRIES, + commitScreenConfigChanges, fetch, useModalDialog, useResetKey, @@ -45,6 +46,7 @@ const Editor = () => { const [remoteConfig, setRemoteConfig] = useState(EMPTY_CONFIG); const [appIdFilter, setAppIdFilter] = useState(null); const [selectedIDs, setSelectedIDs] = useState>(new Set()); + const [configMigrationEnabled, setConfigMigrationEnabled] = useState(false); const { dialog: navBlockDialog, ref: navBlockDialogRef } = useModalDialog(); @@ -116,6 +118,7 @@ const Editor = () => { const config: Config = JSON.parse(response.config); setLocalConfig(config); setRemoteConfig(config); + setConfigMigrationEnabled(response.config_migration || false); setSelectedIDs(new Set()); resetTableDataKey(); setIsCommitReady(false); @@ -135,9 +138,14 @@ const Editor = () => { const commitConfig = () => { withInFlight(setIsLoading, async () => { - const { success } = await fetch.post("/api/admin/screens/confirm", { - config: JSON.stringify(localConfig), - }); + const changedIds = Array.from(changedIDs); + const deletedIds = Array.from(deletedIDs); + const success = await commitScreenConfigChanges( + configMigrationEnabled, + changedIds, + deletedIds, + localConfig, + ); if (success) { setRemoteConfig(localConfig); diff --git a/assets/src/components/admin/inspector.tsx b/assets/src/components/admin/inspector.tsx index 04c8b1513..f0330ff75 100644 --- a/assets/src/components/admin/inspector.tsx +++ b/assets/src/components/admin/inspector.tsx @@ -15,6 +15,7 @@ import { type AudioConfig } from "Components/screen_container"; import { fetch, withInFlight, + commitScreenConfigChanges, SCREEN_APPS, type Config, type Screen, @@ -49,6 +50,7 @@ const buildIframeUrl = (screen: ScreenWithId | null, isSimulation: boolean) => { const Inspector: ComponentType = () => { const [config, setConfig] = useState(null); + const [configMigrationEnabled, setConfigMigrationEnabled] = useState(false); const [frameLoadedAt, setFrameLoadedAt] = useState(0); const [isLoading, setIsLoading] = useState(false); @@ -57,6 +59,7 @@ const Inspector: ComponentType = () => { const response = await fetch.get("/api/admin"); const config: Config = JSON.parse(response.config); setConfig(config); + setConfigMigrationEnabled(response.config_migration || false); }); }; @@ -120,6 +123,8 @@ const Inspector: ComponentType = () => { {screen && ( <> onScreenUpdated(screen.id, config)} @@ -231,10 +236,12 @@ const ScreenSelector: ComponentType<{ }; const ConfigControls: ComponentType<{ + config: Config | null; + configMigrationEnabled: boolean; screen: ScreenWithId; isLoading: boolean; onUpdated: (newConfig: Screen) => void; -}> = ({ screen, isLoading, onUpdated }) => { +}> = ({ config, configMigrationEnabled, screen, isLoading, onUpdated }) => { const [editableConfig, setEditableConfig] = useState(null); const [isRequestingReload, setIsRequestingReload] = useState(false); const dialogRef = useRef(null); @@ -246,6 +253,24 @@ const ConfigControls: ComponentType<{ const isDisabled = isLoading || isRequestingReload; + const handleConfirmEdit = async (editedConfig: Screen) => { + if (!config) return { success: false }; + + const updatedConfig: Config = { + ...config, + screens: { ...config.screens, [screen.id]: editedConfig }, + }; + + const success = await commitScreenConfigChanges( + configMigrationEnabled, + [screen.id], + [], + updatedConfig, + ); + + return { success }; + }; + return (
Configuration @@ -291,7 +316,7 @@ const ConfigControls: ComponentType<{ editableConfig} validatePath={`/api/admin/screens/validate/${screen.id}`} - confirmPath={`/api/admin/screens/confirm/${screen.id}`} + onConfirm={handleConfirmEdit} onUpdated={(newConfig) => { alert("Success. Allow 5 seconds for changes to propagate."); closeDialog(); diff --git a/assets/src/util/admin.tsx b/assets/src/util/admin.tsx index 597ac9dd9..57a6651f4 100644 --- a/assets/src/util/admin.tsx +++ b/assets/src/util/admin.tsx @@ -231,3 +231,30 @@ const doFetch = async ( throw error; } }; + +export const commitScreenConfigChanges = async ( + configMigrationEnabled: boolean, + changedScreenIds: string[], + deletedScreenIds: string[], + localConfig: Config, +) => { + if (configMigrationEnabled) { + const changedConfigs = changedScreenIds.map((id) => ({ + id, + config: localConfig.screens[id], + })); + + const { success } = await fetch.post("/api/admin/screen_configs", { + screen_configs: changedConfigs, + deleted_screen_ids: deletedScreenIds, + }); + + return success; + } else { + const { success } = await fetch.post("/api/admin/screens/confirm", { + config: JSON.stringify(localConfig), + }); + + return success; + } +}; diff --git a/lib/screens_web/controllers/admin_api_controller.ex b/lib/screens_web/controllers/admin_api_controller.ex index 7fe1f6816..e05aff416 100644 --- a/lib/screens_web/controllers/admin_api_controller.ex +++ b/lib/screens_web/controllers/admin_api_controller.ex @@ -9,8 +9,30 @@ defmodule ScreensWeb.AdminApiController do plug :accepts, ["multipart/form-data"] when action == :upload_image def index(conn, _params) do - {:ok, config, _version} = ConfigFetch.fetch_config() - json(conn, %{config: config}) + config = ScreenConfigs.list_all() + config_migration = ScreenConfigs.config_migration_enabled?() + json(conn, %{config: config, config_migration: config_migration}) + end + + def update_screen_configs(conn, %{"screen_configs" => screen_configs} = params) + when is_list(screen_configs) do + deleted_screen_ids = Map.get(params, "deleted_screen_ids", []) + + case ScreenConfigs.commit_updates(screen_configs, deleted_screen_ids) do + :ok -> + json(conn, %{success: true}) + + {:error, reason} -> + conn + |> put_status(500) + |> json(%{success: false, error: "Failed to update screen configs: #{inspect(reason)}"}) + end + end + + def update_screen_configs(conn, _params) do + conn + |> put_status(400) + |> json(%{success: false, error: "Invalid request parameters"}) end def validate(conn, %{"id" => _id, "config" => screen_json}) do diff --git a/lib/screens_web/router.ex b/lib/screens_web/router.ex index 8aa57d269..17ccb74f7 100644 --- a/lib/screens_web/router.ex +++ b/lib/screens_web/router.ex @@ -73,7 +73,6 @@ defmodule ScreensWeb.Router do post "/screens/validate", AdminApiController, :validate post "/screens/validate/:id", AdminApiController, :validate post "/screens/confirm", AdminApiController, :confirm - post "/screens/confirm/:id", AdminApiController, :confirm post "/refresh", AdminApiController, :refresh post "/maintenance", AdminApiController, :maintenance post "/import_configs", AdminApiController, :import_configs diff --git a/test/screens_web/controllers/admin_api_controller_test.exs b/test/screens_web/controllers/admin_api_controller_test.exs new file mode 100644 index 000000000..7ef0ca168 --- /dev/null +++ b/test/screens_web/controllers/admin_api_controller_test.exs @@ -0,0 +1,91 @@ +defmodule ScreensWeb.AdminApiControllerTest do + use ScreensWeb.ConnCase + + import ExUnit.CaptureLog + import Mox + + alias Screens.Config.ScreenConfig + alias Screens.Repo + + # Test config constants + @screen_dup_config %{"app_id" => "dup_v2"} + @screen_busway_config %{"app_id" => "busway_v2"} + + setup do + :ok = Ecto.Adapters.SQL.Sandbox.checkout(Repo) + + previous_config_migration = Application.get_env(:screens, :config_migration) + + on_exit(fn -> + restore_app_env(:screens, :config_migration, previous_config_migration) + end) + + :ok + end + + setup :verify_on_exit! + + describe "update_screen_configs/2" do + @tag :authenticated + test "updates screen configs in Postgres when migration flag is true", %{conn: conn} do + Application.put_env(:screens, :config_migration, true) + + Repo.insert!(%ScreenConfig{id: "screen-1", config: @screen_busway_config}) + + conn = + post(conn, "/api/admin/screen_configs", %{ + screen_configs: [ + %{id: "screen-1", config: @screen_dup_config} + ] + }) + + assert json_response(conn, 200) == %{"success" => true} + + updated = Repo.get!(ScreenConfig, "screen-1") + assert updated.config == @screen_dup_config + end + + @tag :authenticated + test "updates full config when migration flag is false", %{conn: conn} do + Application.put_env(:screens, :config_migration, false) + + # Mock the fetch and put to prevent writing to the fixture file + expect(Screens.Config.Fetch.Mock, :fetch_config, fn -> + {:ok, ~s({"screens": {"screen-2": {"app_id": "dup_v2"}}}), 1} + end) + + expect(Screens.Config.Fetch.Mock, :put_config, fn _config -> :ok end) + + conn = + post(conn, "/api/admin/screen_configs", %{ + screen_configs: [ + %{"id" => "screen-2", "config" => @screen_busway_config} + ] + }) + + assert json_response(conn, 200) == %{"success" => true} + end + + @tag :authenticated + test "returns 400 when screen_configs param is missing", %{conn: conn} do + capture_log([level: :warning], fn -> + conn = post(conn, "/api/admin/screen_configs", %{}) + assert conn.status == 400 + end) + end + + @tag :authenticated + test "index endpoint returns config_migration flag", %{conn: conn} do + Application.put_env(:screens, :config_migration, true) + + conn = get(conn, "/api/admin") + + assert conn.status == 200 + %{"config_migration" => config_migration} = json_response(conn, 200) + assert config_migration == true + end + end + + defp restore_app_env(app, key, nil), do: Application.delete_env(app, key) + defp restore_app_env(app, key, value), do: Application.put_env(app, key, value) +end From cafbc44983bd406bc51cd266c4a9ba635eecfc0e Mon Sep 17 00:00:00 2001 From: "robbie.sundstrom" Date: Thu, 13 Aug 2026 15:26:25 -0400 Subject: [PATCH 3/3] wip: Add error codes from screen_configs for API responses --- lib/screens/screen_configs.ex | 143 +++++++++++++----- .../controllers/admin_api_controller.ex | 2 +- .../screen_configs_api_controller.ex | 11 +- 3 files changed, 104 insertions(+), 52 deletions(-) diff --git a/lib/screens/screen_configs.ex b/lib/screens/screen_configs.ex index bdf2f1c24..a1129d0d5 100644 --- a/lib/screens/screen_configs.ex +++ b/lib/screens/screen_configs.ex @@ -11,7 +11,19 @@ defmodule Screens.ScreenConfigs do @config_fetcher injected(Screens.Config.Fetch) - @spec import_from_file() :: {:ok, %{upserted: integer(), deleted: integer()}} | {:error, any()} + @type screen_id :: String.t() + @type screen_update :: %{required(:id) => screen_id(), required(:config) => map()} + @type commit_error :: + {:upsert_failed, String.t()} + | {:delete_failed, String.t()} + | {:legacy_fetch_failed, term()} + | {:legacy_decode_failed, Jason.DecodeError.t()} + | {:legacy_encode_failed, Jason.EncodeError.t()} + | :legacy_write_failed + | :legacy_screens_invalid + + @spec import_from_file() :: + {:ok, %{upserted: integer(), deleted: integer()}} | {:error, commit_error()} def import_from_file do # This should be a part of post_config_migration_cleanup with {:ok, config, _version} <- @config_fetcher.fetch_config(), @@ -23,16 +35,24 @@ defmodule Screens.ScreenConfigs do upsert_screen_config(%{id: id, config: config}) end) - {deleted, _} = - Repo.delete_all(from s in ScreenConfig, where: s.id not in ^screen_ids) + stale_ids = + Repo.all(from s in ScreenConfig, where: s.id not in ^screen_ids, select: s.id) - {:ok, %{upserted: Enum.count(screen_ids), deleted: deleted}} + case perform_deletes(stale_ids) do + :ok -> + {:ok, %{upserted: Enum.count(screen_ids), deleted: Enum.count(stale_ids)}} + + {:error, reason} -> + {:error, reason} + end end end + @doc """ + Returns all Configs as JSON to be used by Screens Admin + """ @spec list_all() :: String.t() | :error def list_all do - # Returns all Configs as a JSON to be used by Screens Admin if config_migration_enabled?() do screens = ScreenConfig @@ -47,11 +67,13 @@ defmodule Screens.ScreenConfigs do end end + @doc """ + Returns all configs as a list of ScreenConfig structs to be used by Screens Admin + The API controller handles the JSON encoding and formatting for the response. + As part of post_config_migration_cleanup, this and list_all can be cleaned up and restructured + """ @spec list_screen_configs() :: [ScreenConfig.t()] def list_screen_configs do - # Returns all configs as a list of ScreenConfig structs to be used by Screens Admin - # The API controller handles the JSON encoding and formatting for the response. - # As part of post_config_migration_cleanup, this and above function can be cleaned up if config_migration_enabled?() do Repo.all(ScreenConfig) else @@ -82,12 +104,16 @@ defmodule Screens.ScreenConfigs do ) end - @spec upsert_list([%{:id => String.t(), :config => map()}]) :: :ok | {:error, any()} + @spec upsert_list([screen_update()]) :: :ok | {:error, commit_error()} defp upsert_list(updates) do Enum.reduce_while(updates, :ok, fn update, _acc -> case upsert_screen_config(update) do - {:ok, _} -> {:cont, :ok} - {:error, reason} -> {:halt, {:error, reason}} + {:ok, _} -> + {:cont, :ok} + + {:error, %Ecto.Changeset{} = changeset} -> + {:halt, + {:error, {:upsert_failed, "ID #{update[:id]} failed: #{inspect(changeset.errors)}"}}} end end) end @@ -96,8 +122,8 @@ defmodule Screens.ScreenConfigs do Updates and deletes multiple screen configs. Accepts a list of maps with :id and :config keys for updates, and a list of screen IDs for deletions. """ - @spec commit_updates([%{:id => String.t(), :config => map()}], [String.t()]) :: - :ok | {:error, any()} + @spec commit_updates([screen_update()], [screen_id()]) :: + :ok | {:error, commit_error()} def commit_updates(updates, deletes \\ []) do if config_migration_enabled?() do update_to_postgres(updates, deletes) @@ -108,48 +134,81 @@ defmodule Screens.ScreenConfigs do end end - @spec update_to_postgres([%{:id => String.t(), :config => map()}], [String.t()]) :: - :ok | {:error, any()} + @spec update_to_postgres([screen_update()], [screen_id()]) :: + :ok | {:error, commit_error()} defp update_to_postgres(updates, deletes) do - with :ok <- upsert_list(updates) do - perform_deletes(deletes) + case upsert_list(updates) do + :ok -> perform_deletes(deletes) + {:error, _} = error -> error end end - @spec perform_deletes([String.t()]) :: :ok | {:error, any()} + @spec perform_deletes([screen_id()]) :: :ok | {:error, commit_error()} defp perform_deletes(deletes) do Enum.reduce_while(deletes, :ok, fn id, _acc -> - Repo.delete_all(from s in ScreenConfig, where: s.id == ^id) - {:cont, :ok} + case delete_screen_config(id) do + :ok -> {:cont, :ok} + {:error, _} = error -> {:halt, error} + end end) end - defp update_to_legacy_json(updates, deletes) do - # This will be a part of post_config_migration_cleanup - # Merges updates into the existing config and removes deleted screens, then writes back to the legacy source. - # We need to fetch the existing config before writing updates to prevent overwriting any existing configs. - with {:ok, config_json, _version} <- @config_fetcher.fetch_config(), - {:ok, decoded_config} <- Jason.decode(config_json), - screens when is_map(screens) <- Map.get(decoded_config, "screens", %{}) do - updated_screens = - Enum.reduce(updates, screens, fn update, acc -> - id = extract_id(update) - config = extract_config(update) - - existing_config = Map.get(acc, id, %{}) - merged_config = Map.merge(existing_config, config) - Map.put(acc, id, merged_config) - end) + @spec delete_screen_config(screen_id()) :: :ok | {:error, commit_error()} + defp delete_screen_config(id) do + case Repo.delete_all(from s in ScreenConfig, where: s.id == ^id) do + {count, _} when count > 0 -> + :ok + + {0, _} -> + {:error, {:delete_failed, "Config for screen ID #{id} not found"}} - final_screens = Map.drop(updated_screens, deletes) - updated_config = Map.put(decoded_config, "screens", final_screens) + response -> + {:error, {:delete_failed, "Unexpected delete operation response: #{inspect(response)}"}} + end + end - case Jason.encode(updated_config) do - {:ok, encoded_config} -> @config_fetcher.put_config(encoded_config) - {:error, reason} -> {:error, reason} + # This will be a part of post_config_migration_cleanup + # Merges updates into the existing config and removes deleted screens, then writes back to the legacy source. + # We need to fetch the existing config before writing updates to prevent overwriting any existing configs. + @spec update_to_legacy_json([screen_update()], [screen_id()]) :: + :ok | {:error, commit_error()} + defp update_to_legacy_json(updates, deletes) do + with {:ok, config_json, _version} <- @config_fetcher.fetch_config(), + {:ok, decoded_config} <- Jason.decode(config_json) do + screens = Map.get(decoded_config, "screens", %{}) + + if is_map(screens) do + updated_screens = + Enum.reduce(updates, screens, fn update, acc -> + id = extract_id(update) + + merged_config = + acc + |> Map.get(id, %{}) + |> Map.merge(extract_config(update)) + + Map.put(acc, id, merged_config) + end) + + final_screens = Map.drop(updated_screens, deletes) + updated_config = Map.put(decoded_config, "screens", final_screens) + + case Jason.encode(updated_config) do + {:ok, encoded_config} -> + case @config_fetcher.put_config(encoded_config) do + :ok -> :ok + :error -> {:error, :legacy_write_failed} + end + + {:error, %Jason.EncodeError{} = reason} -> + {:error, {:legacy_encode_failed, reason}} + end + else + {:error, :legacy_screens_invalid} end else - _ -> :error + {:error, %Jason.DecodeError{} = reason} -> {:error, {:legacy_decode_failed, reason}} + reason -> {:error, {:legacy_fetch_failed, reason}} end end diff --git a/lib/screens_web/controllers/admin_api_controller.ex b/lib/screens_web/controllers/admin_api_controller.ex index e05aff416..d2f21f85c 100644 --- a/lib/screens_web/controllers/admin_api_controller.ex +++ b/lib/screens_web/controllers/admin_api_controller.ex @@ -25,7 +25,7 @@ defmodule ScreensWeb.AdminApiController do {:error, reason} -> conn |> put_status(500) - |> json(%{success: false, error: "Failed to update screen configs: #{inspect(reason)}"}) + |> json(%{success: false, error: inspect(reason)}) end end diff --git a/lib/screens_web/controllers/screen_configs_api_controller.ex b/lib/screens_web/controllers/screen_configs_api_controller.ex index b7e58cba3..f569c7f18 100644 --- a/lib/screens_web/controllers/screen_configs_api_controller.ex +++ b/lib/screens_web/controllers/screen_configs_api_controller.ex @@ -4,14 +4,7 @@ defmodule ScreensWeb.ScreenConfigsApiController do alias Screens.ScreenConfigs def index(conn, _params) do - screen_configs = - ScreenConfigs.list_screen_configs() - |> Enum.map(fn screen_config -> - %{ - id: screen_config.id, - config: screen_config.config - } - end) + screen_configs = ScreenConfigs.list_screen_configs() json(conn, %{screen_configs: screen_configs}) end @@ -26,7 +19,7 @@ defmodule ScreensWeb.ScreenConfigsApiController do {:error, reason} -> conn |> put_status(500) - |> json(%{success: false, error: "Failed to update screen configs: #{inspect(reason)}"}) + |> json(%{success: false, error: inspect(reason)}) end end