diff --git a/CODESMELL.md b/CODESMELL.md index 62a8da3..7a1b6cc 100644 --- a/CODESMELL.md +++ b/CODESMELL.md @@ -340,18 +340,13 @@ --- -### CSM-020 — Deeply Nested `case` Instead of `with` -- **Files:** `lib/bds/import_definitions.ex:54-66`, `lib/bds/publishing.ex:47-58`, `lib/bds/templates.ex:86-163` -- **Fix:** Flatten with `with`: - ```elixir - with {:ok, record} <- Repo.get(Model, id), - {:ok, updated} <- Repo.update(changeset) do - {:ok, updated} - else - nil -> {:error, :not_found} - {:error, changeset} -> {:error, changeset} - end - ``` +### ~~CSM-020 — Deeply Nested `case` Instead of `with`~~ ✅ FIXED +- **Fixed:** 2026-05-10 +- **What was done:** + - **`lib/bds/import_definitions.ex`** — `delete_definition/1`: Replaced nested `case` piped into another `case` with a flat `with` chain: `Repo.get` → `Repo.delete` → `{:ok, :deleted}`, with `else` clauses for `nil` and `{:error, _}`. + - **`lib/bds/publishing.ex`** — `handle_call({:update_job, ...})`: Replaced `case Repo.get` with `with %PublishJob{} = job <- Repo.get(...)`. Also replaced `Repo.update!()` with `Repo.update()` to avoid crashes on changeset errors. + - **`lib/bds/templates.ex`** — `update_template/2`: Replaced outer `case Repo.get` with `with` + extracted `do_update_template/2` private function. Collapsed three levels of nested `case` (Repo.get → transaction_result → sync_side_effects) into a single flat `with` chain. + - Added 7 tests in `test/bds/csm020_nested_case_test.exs`: delete_definition success and not-found, update_template success and not-found, source-level assertions that all three files use `with` instead of nested `case`. --- diff --git a/lib/bds/import_definitions.ex b/lib/bds/import_definitions.ex index 457663f..76b3c32 100644 --- a/lib/bds/import_definitions.ex +++ b/lib/bds/import_definitions.ex @@ -55,16 +55,12 @@ defmodule BDS.ImportDefinitions do end def delete_definition(definition_id) when is_binary(definition_id) do - case Repo.get(ImportDefinition, definition_id) do - nil -> - {:error, :not_found} - - %ImportDefinition{} = definition -> - Repo.delete(definition) - |> case do - {:ok, _deleted} -> {:ok, :deleted} - error -> error - end + with %ImportDefinition{} = definition <- Repo.get(ImportDefinition, definition_id), + {:ok, _deleted} <- Repo.delete(definition) do + {:ok, :deleted} + else + nil -> {:error, :not_found} + {:error, _} = error -> error end end diff --git a/lib/bds/publishing.ex b/lib/bds/publishing.ex index aa830a2..bc6dde7 100644 --- a/lib/bds/publishing.ex +++ b/lib/bds/publishing.ex @@ -47,18 +47,12 @@ defmodule BDS.Publishing do end def handle_call({:update_job, job_id, attrs}, _from, state) do - reply = - case Repo.get(PublishJob, job_id) do - nil -> - :ok + with %PublishJob{} = job <- Repo.get(PublishJob, job_id) do + attrs = Map.put(attrs, :updated_at, Persistence.now_ms()) + job |> PublishJob.changeset(attrs) |> Repo.update() + end - job -> - attrs = Map.put(attrs, :updated_at, Persistence.now_ms()) - job |> PublishJob.changeset(attrs) |> Repo.update!() - :ok - end - - {:reply, reply, state} + {:reply, :ok, state} end def handle_call({:should_upload_scp_file, upload_key, local_mtime}, _from, state) do diff --git a/lib/bds/templates.ex b/lib/bds/templates.ex index 251216d..2df477e 100644 --- a/lib/bds/templates.ex +++ b/lib/bds/templates.ex @@ -87,82 +87,77 @@ defmodule BDS.Templates do @spec update_template(String.t(), attrs()) :: template_result() | {:error, :not_found} def update_template(template_id, attrs) do - case Repo.get(Template, template_id) do - nil -> - {:error, :not_found} + with %Template{} = template <- Repo.get(Template, template_id) do + do_update_template(template, attrs) + else + nil -> {:error, :not_found} + end + end - template -> - next_slug = - if has_attr?(attrs, :slug) do - unique_slug( - template.project_id, - Slug.slugify(attr(attrs, :slug)), - "template", - template.id - ) - else - template.slug - end + defp do_update_template(template, attrs) do + next_slug = + if has_attr?(attrs, :slug) do + unique_slug( + template.project_id, + Slug.slugify(attr(attrs, :slug)), + "template", + template.id + ) + else + template.slug + end - content_changed? = - has_attr?(attrs, :content) and - attr(attrs, :content) != effective_template_content(template) + content_changed? = + has_attr?(attrs, :content) and + attr(attrs, :content) != effective_template_content(template) - slug_changed? = next_slug != template.slug - now = Persistence.now_ms() + slug_changed? = next_slug != template.slug + now = Persistence.now_ms() - next_status = - if(template.status == :published and content_changed?, - do: :draft, - else: template.status - ) + next_status = + if(template.status == :published and content_changed?, + do: :draft, + else: template.status + ) - next_file_path = next_template_file_path(template, next_slug) + next_file_path = next_template_file_path(template, next_slug) - updates = - %{} - |> maybe_put(:title, attr(attrs, :title)) - |> maybe_put(:kind, attr(attrs, :kind)) - |> maybe_put(:enabled, attr(attrs, :enabled)) - |> maybe_put(:content, attr(attrs, :content)) - |> Map.put(:file_path, next_file_path) - |> Map.put(:slug, next_slug) - |> Map.put(:version, template.version + 1) - |> Map.put(:updated_at, now) - |> Map.put(:status, next_status) + updates = + %{} + |> maybe_put(:title, attr(attrs, :title)) + |> maybe_put(:kind, attr(attrs, :kind)) + |> maybe_put(:enabled, attr(attrs, :enabled)) + |> maybe_put(:content, attr(attrs, :content)) + |> Map.put(:file_path, next_file_path) + |> Map.put(:slug, next_slug) + |> Map.put(:version, template.version + 1) + |> Map.put(:updated_at, now) + |> Map.put(:status, next_status) - transaction_result = - Repo.transaction(fn -> - updated_template = - template - |> Template.changeset(updates) - |> Repo.update!() + with {:ok, {updated_template, affected_posts}} <- + Repo.transaction(fn -> + updated_template = + template + |> Template.changeset(updates) + |> Repo.update!() - affected_posts = - if slug_changed? do - cascade_template_slug_change(template, updated_template, now) - else - [] - end + affected_posts = + if slug_changed? do + cascade_template_slug_change(template, updated_template, now) + else + [] + end - {updated_template, affected_posts} - end) - - case transaction_result do - {:ok, {updated_template, affected_posts}} -> - case sync_template_update_side_effects( - template, - updated_template, - affected_posts, - slug_changed? - ) do - :ok -> {:ok, updated_template} - {:error, reason} -> {:error, reason} - end - - {:error, reason} -> - {:error, reason} - end + {updated_template, affected_posts} + end), + :ok <- + sync_template_update_side_effects( + template, + updated_template, + affected_posts, + slug_changed? + ) do + {:ok, updated_template} end end diff --git a/test/bds/csm020_nested_case_test.exs b/test/bds/csm020_nested_case_test.exs new file mode 100644 index 0000000..a33b314 --- /dev/null +++ b/test/bds/csm020_nested_case_test.exs @@ -0,0 +1,91 @@ +defmodule BDS.CSM020NestedCaseTest do + use ExUnit.Case, async: false + + alias BDS.ImportDefinitions + alias BDS.Templates + + setup do + :ok = Ecto.Adapters.SQL.Sandbox.checkout(BDS.Repo) + + temp_dir = + Path.join(System.tmp_dir!(), "bds-csm020-#{System.unique_integer([:positive])}") + + File.mkdir_p!(temp_dir) + on_exit(fn -> File.rm_rf(temp_dir) end) + + {:ok, project} = + BDS.Projects.create_project(%{name: "CSM-020 Test", data_path: temp_dir}) + + %{project: project, temp_dir: temp_dir} + end + + describe "ImportDefinitions.delete_definition/1 uses with" do + test "returns {:ok, :deleted} on success", %{project: project} do + {:ok, definition} = + ImportDefinitions.create_definition(%{ + project_id: project.id, + name: "test", + wxr_file_path: "/tmp/test.xml" + }) + + assert {:ok, :deleted} = ImportDefinitions.delete_definition(definition.id) + end + + test "returns {:error, :not_found} for missing ID" do + assert {:error, :not_found} = + ImportDefinitions.delete_definition(Ecto.UUID.generate()) + end + + test "source code uses with instead of nested case" do + source = File.read!("lib/bds/import_definitions.ex") + [func_source] = Regex.scan(~r/def delete_definition.*?(?=\n def |\n @|\nend)/s, source) + + refute func_source |> List.first() |> String.contains?("|> case do"), + "delete_definition should not pipe into case" + end + end + + describe "Templates.update_template/2 uses with" do + test "returns {:error, :not_found} for missing ID" do + assert {:error, :not_found} = Templates.update_template(Ecto.UUID.generate(), %{}) + end + + test "updates title successfully", %{project: project} do + {:ok, template} = + Templates.create_template(%{ + project_id: project.id, + title: "Original", + kind: :post, + content: "
test
" + }) + + assert {:ok, updated} = + Templates.update_template(template.id, %{title: "Updated"}) + + assert updated.title == "Updated" + end + + test "source code uses with instead of deeply nested case" do + source = File.read!("lib/bds/templates.ex") + + refute Regex.match?( + ~r/def update_template.*?case Repo\.get.*?case transaction_result/s, + source + ), + "update_template should not have nested case blocks" + end + end + + describe "Publishing.handle_call :update_job uses with" do + test "source code uses with instead of case" do + source = File.read!("lib/bds/publishing.ex") + [func_source] = Regex.scan(~r/def handle_call\(\{:update_job.*?(?=\n def |\n @impl)/s, source) + + assert func_source |> List.first() |> String.contains?("with"), + "update_job handler should use with" + + refute func_source |> List.first() |> String.contains?("case Repo.get"), + "update_job handler should not use case Repo.get" + end + end +end