[Housekeeping] Refactor settings model (#165)

* [WIP] renamed current settings module and tables to have backup suffix

* Created new settings table, schema, and context

* Migrated from old settings module to new one

* Removed settings backup modules

* Added some tests and docs

lib/pinchflat/boot/pre_job_startup_tasks.ex

@@ -65,8 +65,6 @@ defmodule Pinchflat.Boot.PreJobStartupTasks do
   defp apply_default_settings do
     {:ok, yt_dlp_version} = CommandRunner.version()
 
-    Settings.fetch!(:onboarding, true)
-    Settings.fetch!(:pro_enabled, false)
-    Settings.set!(:yt_dlp_version, yt_dlp_version)
+    Settings.set(yt_dlp_version: yt_dlp_version)
   end
 end

lib/pinchflat/settings/setting.ex

@@ -1,24 +1,32 @@
 defmodule Pinchflat.Settings.Setting do
   @moduledoc """
-  A Setting is a key-value pair with a datatype used to track user-level settings.
+  The Setting schema.
   """
 
   use Ecto.Schema
   import Ecto.Changeset
 
-  schema "settings" do
-    field :name, :string
-    field :value, :string
-    field :datatype, Ecto.Enum, values: ~w(boolean string integer float)a
+  @allowed_fields [
+    :onboarding,
+    :pro_enabled,
+    :yt_dlp_version
+  ]
 
-    timestamps(type: :utc_datetime)
+  @required_fields ~w(
+    onboarding
+    pro_enabled
+  )a
+
+  schema "settings" do
+    field :onboarding, :boolean, default: true
+    field :pro_enabled, :boolean, default: false
+    field :yt_dlp_version, :string
   end
 
   @doc false
   def changeset(setting, attrs) do
     setting
-    |> cast(attrs, [:name, :value, :datatype])
-    |> validate_required([:name, :value, :datatype])
-    |> unique_constraint([:name])
+    |> cast(attrs, @allowed_fields)
+    |> validate_required(@required_fields)
   end
 end

lib/pinchflat/settings/settings.ex

@@ -2,94 +2,63 @@ defmodule Pinchflat.Settings do
   @moduledoc """
   The Settings context.
   """
-
   import Ecto.Query, warn: false
-  alias Pinchflat.Repo
 
+  alias Pinchflat.Repo
   alias Pinchflat.Settings.Setting
 
   @doc """
-  Returns the list of settings.
+  Returns the only setting record. It _should_ be impossible
+  to create or delete this record, so it's assertive about
+  assuming it's the only one.
 
-  Returns [%Setting{}, ...]
+  Returns %Setting{}
   """
-  def list_settings do
-    Repo.all(Setting)
+  def record do
+    Setting
+    |> limit(1)
+    |> Repo.one()
   end
 
   @doc """
-  Creates or updates a setting, returning the parsed value.
-  Raises if an unsupported datatype is used. Optionally allows
-  specifying the datatype.
+  Updates a setting, returning the new value.
+  Is setup to take a keyword list argument so you
+  can call it like `Settings.set(onboarding: true)`
 
-  Returns value in type of `Ecto.Enum.mappings(Setting, :datatype)`
+  Returns {:ok, value} | {:error, :invalid_key} | {:error, %Ecto.Changeset{}}
   """
-  def set!(name, value) do
-    set!(name, value, infer_datatype(value))
-  end
-
-  def set!(name, value, datatype) do
-    # Only create if doesn't exist
-    case Repo.get_by(Setting, name: to_string(name)) do
-      nil -> create_setting!(name, value, datatype)
-      setting -> update_setting!(setting, value, datatype)
+  def set([{attr, value}]) do
+    record()
+    |> Setting.changeset(%{attr => value})
+    |> Repo.update()
+    |> case do
+      {:ok, %{^attr => _}} -> {:ok, value}
+      {:ok, _} -> {:error, :invalid_key}
+      {:error, changeset} -> {:error, changeset}
     end
   end
 
   @doc """
-  Gets the parsed value of a setting. Raises if the setting does not exist.
+  Gets the value of a setting.
 
-  Returns value in type of `Ecto.Enum.mappings(Setting, :datatype)`
+  Returns {:ok, value} | {:error, :invalid_key}
+  """
+  def get(name) do
+    case Map.fetch(record(), name) do
+      {:ok, value} -> {:ok, value}
+      :error -> {:error, :invalid_key}
+    end
+  end
+
+  @doc """
+  Gets the value of a setting, raising if it doesn't exist.
+
+  Returns value
   """
   def get!(name) do
-    Setting
-    |> Repo.get_by!(name: to_string(name))
-    |> read_setting()
-  end
-
-  @doc """
-  Attempts to find a setting by name or creates a setting with value
-  if one doesn't exist, returning the parsed value. Optionally allows
-  specifying the datatype.
-
-  Returns value in type of `Ecto.Enum.mappings(Setting, :datatype)`
-  """
-  def fetch!(name, value) do
-    fetch!(name, value, infer_datatype(value))
-  end
-
-  def fetch!(name, value, datatype) do
-    case Repo.get_by(Setting, name: to_string(name)) do
-      nil -> create_setting!(name, value, datatype)
-      setting -> read_setting(setting)
+    case get(name) do
+      {:ok, value} -> value
+      {:error, _} -> raise "Setting `#{name}` not found"
     end
   end
-
-  defp change_setting(setting, attrs) do
-    Setting.changeset(setting, attrs)
-  end
-
-  defp create_setting!(name, value, datatype) do
-    %Setting{}
-    |> change_setting(%{name: to_string(name), value: to_string(value), datatype: datatype})
-    |> Repo.insert!()
-    |> read_setting()
-  end
-
-  defp update_setting!(setting, value, datatype) do
-    setting
-    |> change_setting(%{value: to_string(value), datatype: datatype})
-    |> Repo.update!()
-    |> read_setting()
-  end
-
-  defp read_setting(%{value: value, datatype: :string}), do: value
-  defp read_setting(%{value: value, datatype: :boolean}), do: value in ["true", "t", "1"]
-  defp read_setting(%{value: value, datatype: :integer}), do: String.to_integer(value)
-  defp read_setting(%{value: value, datatype: :float}), do: String.to_float(value)
-
-  defp infer_datatype(value) when is_boolean(value), do: :boolean
-  defp infer_datatype(value) when is_integer(value), do: :integer
-  defp infer_datatype(value) when is_float(value), do: :float
-  defp infer_datatype(value) when is_binary(value), do: :string
 end

lib/pinchflat_web/components/layouts/partials/upgrade_button_live.ex

@@ -30,7 +30,7 @@ defmodule Pinchflat.UpgradeButtonLive do
       |> String.downcase()
 
     if normalized_text == "got it!" do
-      Settings.set!(:pro_enabled, true)
+      Settings.set(pro_enabled: true)
 
       {:noreply, update(socket, :button_disabled, fn _ -> false end)}
     else

lib/pinchflat_web/controllers/pages/page_controller.ex

@@ -11,7 +11,7 @@ defmodule PinchflatWeb.Pages.PageController do
     done_onboarding = params["onboarding"] == "0"
     force_onboarding = params["onboarding"] == "1"
 
-    if done_onboarding, do: Settings.set!(:onboarding, false)
+    if done_onboarding, do: Settings.set(onboarding: false)
 
     if force_onboarding || Settings.get!(:onboarding) do
       render_onboarding_page(conn)
@@ -30,7 +30,7 @@ defmodule PinchflatWeb.Pages.PageController do
   end
 
   defp render_onboarding_page(conn) do
-    Settings.set!(:onboarding, true)
+    Settings.set(onboarding: true)
 
     conn
     |> render(:onboarding_checklist,

priv/repo/migrations/20240404172256_rename_settings_table.exs

@@ -0,0 +1,7 @@
+defmodule Pinchflat.Repo.Migrations.RenameSettingsTable do
+  use Ecto.Migration
+
+  def change do
+    rename table(:settings), to: table(:settings_backup)
+  end
+end

priv/repo/migrations/20240404174144_create_new_settings.exs

@@ -0,0 +1,29 @@
+defmodule Pinchflat.Repo.Migrations.CreateNewSettings do
+  use Ecto.Migration
+
+  def up do
+    create table(:settings) do
+      add :onboarding, :boolean, default: true, null: false
+      add :pro_enabled, :boolean, default: false, null: false
+      add :yt_dlp_version, :string
+    end
+
+    # Make an initial record because this will be the only one ever inserted
+    execute "INSERT INTO settings (onboarding, pro_enabled, yt_dlp_version) VALUES (true, false, NULL)"
+
+    # Set the value of onboarding to the previous version set in `settings_backup`
+    execute """
+      UPDATE settings
+      SET onboarding = COALESCE((SELECT value = 'true' FROM settings_backup WHERE name = 'onboarding'), true)
+    """
+
+    execute """
+      UPDATE settings
+      SET pro_enabled = COALESCE((SELECT value = 'true' FROM settings_backup WHERE name = 'pro_enabled'), false)
+    """
+  end
+
+  def down do
+    drop table(:settings)
+  end
+end

test/pinchflat/boot/pre_job_startup_tasks_test.exs

@@ -1,25 +1,47 @@
 defmodule Pinchflat.Boot.PreJobStartupTasksTest do
   use Pinchflat.DataCase
 
+  import Pinchflat.JobFixtures
+
   alias Pinchflat.Settings
-  alias Pinchflat.Settings.Setting
   alias Pinchflat.Boot.PreJobStartupTasks
 
-  describe "apply_default_settings" do
-    setup do
-      Repo.delete_all(Setting)
+  describe "reset_executing_jobs" do
+    test "resets executing jobs" do
+      job = job_fixture()
+      Repo.update_all(Oban.Job, set: [state: "executing"])
 
-      :ok
-    end
-
-    test "sets default settings" do
-      assert_raise Ecto.NoResultsError, fn -> Settings.get!(:onboarding) end
-      assert_raise Ecto.NoResultsError, fn -> Settings.get!(:pro_enabled) end
+      assert Repo.reload!(job).state == "executing"
 
       PreJobStartupTasks.start_link()
 
-      assert Settings.get!(:onboarding)
-      refute Settings.get!(:pro_enabled)
+      assert Repo.reload!(job).state == "retryable"
+    end
+  end
+
+  describe "create_blank_cookie_file" do
+    test "creates a blank cookie file" do
+      base_dir = Application.get_env(:pinchflat, :extras_directory)
+      filepath = Path.join(base_dir, "cookies.txt")
+      File.rm(filepath)
+
+      refute File.exists?(filepath)
+
+      PreJobStartupTasks.start_link()
+
+      assert File.exists?(filepath)
+    end
+  end
+
+  describe "apply_default_settings" do
+    test "sets default settings" do
+      Settings.set(yt_dlp_version: nil)
+
+      refute Settings.get!(:yt_dlp_version)
+
+      PreJobStartupTasks.start_link()
+
+      assert Settings.get!(:yt_dlp_version)
     end
   end
 end

test/pinchflat/settings_test.exs

@@ -9,100 +9,55 @@ defmodule Pinchflat.SettingsTest do
   # are always created on app boot (including in the test env),
   # so we can't treat these like a clean slate.
 
-  describe "list_settings/0" do
-    test "returns all settings" do
-      Settings.set!("foo", "bar")
-      results = Settings.list_settings()
+  setup do
+    # Ensure we have a clean slate
+    Settings.set(onboarding: false)
+    Settings.set(pro_enabled: false)
+    Settings.set(yt_dlp_version: nil)
 
-      assert Enum.all?(results, fn setting -> match?(%Setting{}, setting) end)
+    :ok
+  end
+
+  describe "record/0" do
+    test "returns the only setting" do
+      assert %Setting{} = Settings.record()
     end
   end
 
-  describe "set/2" do
-    test "creates a new setting if one does not exist" do
-      original = Repo.aggregate(Setting, :count, :id)
-      Settings.set!("foo", "bar")
-      assert Repo.aggregate(Setting, :count, :id) == original + 1
+  describe "set/1" do
+    test "updates the setting" do
+      assert {:ok, true} = Settings.set(onboarding: true)
+      assert {:ok, true} = Settings.get(:onboarding)
     end
 
-    test "updates an existing setting if one exists" do
-      Settings.set!("foo", "bar")
-      original = Repo.aggregate(Setting, :count, :id)
-      Settings.set!("foo", "baz")
-      assert Repo.aggregate(Setting, :count, :id) == original
-      assert Settings.get!("foo") == "baz"
+    test "returns an error if the setting key doesn't exist" do
+      assert {:error, :invalid_key} = Settings.set(foo: "bar")
     end
 
-    test "returns the parsed value" do
-      assert Settings.set!("foo", true) == true
-      assert Settings.set!("foo", false) == false
-      assert Settings.set!("foo", 123) == 123
-      assert Settings.set!("foo", 12.34) == 12.34
-      assert Settings.set!("foo", "bar") == "bar"
-    end
-
-    test "allows for atom keys" do
-      assert Settings.set!(:foo, "bar") == "bar"
-    end
-
-    test "blows up when an unsupported datatype is used" do
-      assert_raise FunctionClauseError, fn ->
-        Settings.set!("foo", nil)
-      end
-    end
-  end
-
-  describe "set/3" do
-    test "allows manual specification of datatype" do
-      assert Settings.set!("foo", "true", :boolean) == true
-      assert Settings.set!("foo", "false", :boolean) == false
-      assert Settings.set!("foo", "123", :integer) == 123
-      assert Settings.set!("foo", "12.34", :float) == 12.34
+    test "returns an error if the setting value is invalid" do
+      assert {:error, %Ecto.Changeset{}} = Settings.set(onboarding: "bar")
     end
   end
 
   describe "get/1" do
-    test "returns the value of the setting" do
-      Settings.set!("str", "bar")
-      Settings.set!("bool", true)
-      Settings.set!("int", 123)
-      Settings.set!("float", 12.34)
-
-      assert Settings.get!("str") == "bar"
-      assert Settings.get!("bool") == true
-      assert Settings.get!("int") == 123
-      assert Settings.get!("float") == 12.34
+    test "returns the setting value" do
+      assert {:ok, false} = Settings.get(:onboarding)
     end
 
-    test "allows for atom keys" do
-      Settings.set!("str", "bar")
-      assert Settings.get!(:str) == "bar"
+    test "returns an error if the setting key doesn't exist" do
+      assert {:error, :invalid_key} = Settings.get(:foo)
+    end
+  end
+
+  describe "get!/1" do
+    test "returns the setting value" do
+      assert Settings.get!(:onboarding) == false
     end
 
-    test "blows up when the setting does not exist" do
-      assert_raise Ecto.NoResultsError, fn ->
-        Settings.get!("foo")
+    test "raises an error if the setting key doesn't exist" do
+      assert_raise RuntimeError, "Setting `foo` not found", fn ->
+        Settings.get!(:foo)
       end
     end
   end
-
-  describe "fetch/2" do
-    test "creates a setting if one doesn't exist" do
-      original = Repo.aggregate(Setting, :count, :id)
-      assert Settings.fetch!("foo", "bar") == "bar"
-      assert Repo.aggregate(Setting, :count, :id) == original + 1
-    end
-
-    test "returns an existing setting if one does exist" do
-      Settings.set!("foo", "bar")
-
-      assert Settings.fetch!("foo", "baz") == "bar"
-    end
-  end
-
-  describe "fetch/3" do
-    test "allows manual specification of datatype" do
-      assert Settings.fetch!("foo", "true", :boolean) == true
-    end
-  end
 end

test/pinchflat_web/controllers/media_profile_controller_test.exs

@@ -16,7 +16,7 @@ defmodule PinchflatWeb.MediaProfileControllerTest do
   @invalid_attrs %{name: nil, output_path_template: nil}
 
   setup do
-    Settings.set!(:onboarding, false)
+    Settings.set(onboarding: false)
 
     :ok
   end
@@ -35,7 +35,7 @@ defmodule PinchflatWeb.MediaProfileControllerTest do
     end
 
     test "renders correct layout when onboarding", %{conn: conn} do
-      Settings.set!(:onboarding, true)
+      Settings.set(onboarding: true)
       conn = get(conn, ~p"/media_profiles/new")
 
       refute html_response(conn, 200) =~ "MENU"
@@ -59,14 +59,14 @@ defmodule PinchflatWeb.MediaProfileControllerTest do
     end
 
     test "redirects to onboarding when onboarding", %{conn: conn} do
-      Settings.set!(:onboarding, true)
+      Settings.set(onboarding: true)
       conn = post(conn, ~p"/media_profiles", media_profile: @create_attrs)
 
       assert redirected_to(conn) == ~p"/?onboarding=1"
     end
 
     test "renders correct layout on error when onboarding", %{conn: conn} do
-      Settings.set!(:onboarding, true)
+      Settings.set(onboarding: true)
       conn = post(conn, ~p"/media_profiles", media_profile: @invalid_attrs)
 
       refute html_response(conn, 200) =~ "MENU"

test/pinchflat_web/controllers/page_controller_test.exs

@@ -10,7 +10,7 @@ defmodule PinchflatWeb.PageControllerTest do
     end
 
     test "displays the onboarding page when onboarding is forced", %{conn: conn} do
-      Settings.set!(:onboarding, false)
+      Settings.set(onboarding: false)
 
       conn = get(conn, ~p"/?onboarding=1")
       assert html_response(conn, 200) =~ "Welcome to Pinchflat"
@@ -25,7 +25,7 @@ defmodule PinchflatWeb.PageControllerTest do
     end
 
     test "displays the home page when not onboarding", %{conn: conn} do
-      Settings.set!(:onboarding, false)
+      Settings.set(onboarding: false)
 
       conn = get(conn, ~p"/")
       assert html_response(conn, 200) =~ "MENU"

test/pinchflat_web/controllers/source_controller_test.exs

@@ -13,7 +13,7 @@ defmodule PinchflatWeb.SourceControllerTest do
 
   setup do
     media_profile = media_profile_fixture()
-    Settings.set!(:onboarding, false)
+    Settings.set(onboarding: false)
 
     {
       :ok,
@@ -47,7 +47,7 @@ defmodule PinchflatWeb.SourceControllerTest do
     end
 
     test "renders correct layout when onboarding", %{conn: conn} do
-      Settings.set!(:onboarding, true)
+      Settings.set(onboarding: true)
       conn = get(conn, ~p"/sources/new")
 
       refute html_response(conn, 200) =~ "MENU"
@@ -74,14 +74,14 @@ defmodule PinchflatWeb.SourceControllerTest do
     test "redirects to onboarding when onboarding", %{conn: conn, create_attrs: create_attrs} do
       expect(YtDlpRunnerMock, :run, 1, &runner_function_mock/3)
 
-      Settings.set!(:onboarding, true)
+      Settings.set(onboarding: true)
       conn = post(conn, ~p"/sources", source: create_attrs)
 
       assert redirected_to(conn) == ~p"/?onboarding=1"
     end
 
     test "renders correct layout on error when onboarding", %{conn: conn, invalid_attrs: invalid_attrs} do
-      Settings.set!(:onboarding, true)
+      Settings.set(onboarding: true)
       conn = post(conn, ~p"/sources", source: invalid_attrs)
 
       refute html_response(conn, 200) =~ "MENU"