From 0513154b01a9d7e030d6bc418332def06c0efaf7 Mon Sep 17 00:00:00 2001 From: Willem van den Ende Date: Tue, 5 May 2026 22:03:06 +0100 Subject: [PATCH] Fix remaining 12 multi-line conn shadowing issues Manually refactor multi-line conn = post/put(conn, ...) patterns across user_session, user_settings, and user_registration controller tests. Rename shadowed conn to response using pipeline operator. Also add plans/multi-line-conn-refactoring.md spec for future Elixir-based tooling to handle these patterns automatically. --- .../user_registration_controller_test.exs | 18 +- .../user_session_controller_test.exs | 66 +++--- .../user_settings_controller_test.exs | 16 +- plans/multi-line-conn-refactoring.md | 192 ++++++++++++++++++ 4 files changed, 248 insertions(+), 44 deletions(-) create mode 100644 plans/multi-line-conn-refactoring.md diff --git a/app/test/firehose_web/controllers/user_registration_controller_test.exs b/app/test/firehose_web/controllers/user_registration_controller_test.exs index 496821b..09ef29d 100644 --- a/app/test/firehose_web/controllers/user_registration_controller_test.exs +++ b/app/test/firehose_web/controllers/user_registration_controller_test.exs @@ -25,15 +25,16 @@ defmodule FirehoseWeb.UserRegistrationControllerTest do Application.put_env(:firehose, :allowed_registration_email, email) on_exit(fn -> Application.delete_env(:firehose, :allowed_registration_email) end) - conn = - post(conn, ~p"/users/register", %{ + response = + conn + |> post(~p"/users/register", %{ "user" => valid_user_attributes(email: email) }) - refute get_session(conn, :user_token) - assert redirected_to(conn) == ~p"/users/log-in" + refute get_session(response, :user_token) + assert redirected_to(response) == ~p"/users/log-in" - assert conn.assigns.flash["info"] =~ + assert response.assigns.flash["info"] =~ ~r/An email was sent to .*, please access it to confirm your account/ end @@ -41,12 +42,13 @@ defmodule FirehoseWeb.UserRegistrationControllerTest do Application.put_env(:firehose, :allowed_registration_email, "with spaces") on_exit(fn -> Application.delete_env(:firehose, :allowed_registration_email) end) - conn = - post(conn, ~p"/users/register", %{ + response = + conn + |> post(~p"/users/register", %{ "user" => %{"email" => "with spaces"} }) + |> html_response(200) - response = html_response(conn, 200) assert response =~ "Register" assert response =~ "must have the @ sign and no spaces" end diff --git a/app/test/firehose_web/controllers/user_session_controller_test.exs b/app/test/firehose_web/controllers/user_session_controller_test.exs index 8bee92f..bbad175 100644 --- a/app/test/firehose_web/controllers/user_session_controller_test.exs +++ b/app/test/firehose_web/controllers/user_session_controller_test.exs @@ -73,20 +73,22 @@ defmodule FirehoseWeb.UserSessionControllerTest do test "logs the user in", %{conn: conn, user: user} do user = set_password(user) - conn = - post(conn, ~p"/users/log-in", %{ + response = + conn + |> post(~p"/users/log-in", %{ "user" => %{"email" => user.email, "password" => valid_user_password()} }) - assert get_session(conn, :user_token) - assert redirected_to(conn) == ~p"/" + assert get_session(response, :user_token) + assert redirected_to(response) == ~p"/" end test "logs the user in with remember me", %{conn: conn, user: user} do user = set_password(user) - conn = - post(conn, ~p"/users/log-in", %{ + response = + conn + |> post(~p"/users/log-in", %{ "user" => %{ "email" => user.email, "password" => valid_user_password(), @@ -94,14 +96,14 @@ defmodule FirehoseWeb.UserSessionControllerTest do } }) - assert conn.resp_cookies["_firehose_web_user_remember_me"] - assert redirected_to(conn) == ~p"/" + assert response.resp_cookies["_firehose_web_user_remember_me"] + assert redirected_to(response) == ~p"/" end test "logs the user in with return to", %{conn: conn, user: user} do user = set_password(user) - conn = + response = conn |> init_test_session(user_return_to: "/foo/bar") |> post(~p"/users/log-in", %{ @@ -111,17 +113,18 @@ defmodule FirehoseWeb.UserSessionControllerTest do } }) - assert redirected_to(conn) == "/foo/bar" - assert Phoenix.Flash.get(conn.assigns.flash, :info) =~ "Welcome back!" + assert redirected_to(response) == "/foo/bar" + assert Phoenix.Flash.get(response.assigns.flash, :info) =~ "Welcome back!" end test "emits error message with invalid credentials", %{conn: conn, user: user} do - conn = - post(conn, ~p"/users/log-in?mode=password", %{ + response = + conn + |> post(~p"/users/log-in?mode=password", %{ "user" => %{"email" => user.email, "password" => "invalid_password"} }) + |> html_response(200) - response = html_response(conn, 200) assert response =~ "Log in" assert response =~ "Invalid email or password" end @@ -129,51 +132,56 @@ defmodule FirehoseWeb.UserSessionControllerTest do describe "POST /users/log-in - magic link" do test "sends magic link email when user exists", %{conn: conn, user: user} do - conn = - post(conn, ~p"/users/log-in", %{ + response = + conn + |> post(~p"/users/log-in", %{ "user" => %{"email" => user.email} }) - assert Phoenix.Flash.get(conn.assigns.flash, :info) =~ "If your email is in our system" + assert Phoenix.Flash.get(response.assigns.flash, :info) =~ "If your email is in our system" assert Firehose.Repo.get_by!(Accounts.UserToken, user_id: user.id).context == "login" end test "logs the user in", %{conn: conn, user: user} do {token, _hashed_token} = generate_user_magic_link_token(user) - conn = - post(conn, ~p"/users/log-in", %{ + response = + conn + |> post(~p"/users/log-in", %{ "user" => %{"token" => token} }) - assert get_session(conn, :user_token) - assert redirected_to(conn) == ~p"/" + assert get_session(response, :user_token) + assert redirected_to(response) == ~p"/" end test "confirms unconfirmed user", %{conn: conn, unconfirmed_user: user} do {token, _hashed_token} = generate_user_magic_link_token(user) refute user.confirmed_at - conn = - post(conn, ~p"/users/log-in", %{ + response = + conn + |> post(~p"/users/log-in", %{ "user" => %{"token" => token}, "_action" => "confirmed" }) - assert get_session(conn, :user_token) - assert redirected_to(conn) == ~p"/" - assert Phoenix.Flash.get(conn.assigns.flash, :info) =~ "User confirmed successfully." + assert get_session(response, :user_token) + assert redirected_to(response) == ~p"/" + assert Phoenix.Flash.get(response.assigns.flash, :info) =~ "User confirmed successfully." assert Accounts.get_user!(user.id).confirmed_at end test "emits error message when magic link is invalid", %{conn: conn} do - conn = - post(conn, ~p"/users/log-in", %{ + response = + conn + |> post(~p"/users/log-in", %{ "user" => %{"token" => "invalid"} }) + |> html_response(200) - assert html_response(conn, 200) =~ "The link is invalid or it has expired." + assert response =~ "The link is invalid or it has expired." end end diff --git a/app/test/firehose_web/controllers/user_settings_controller_test.exs b/app/test/firehose_web/controllers/user_settings_controller_test.exs index 4be3648..43d48b1 100644 --- a/app/test/firehose_web/controllers/user_settings_controller_test.exs +++ b/app/test/firehose_web/controllers/user_settings_controller_test.exs @@ -71,28 +71,30 @@ defmodule FirehoseWeb.UserSettingsControllerTest do describe "PUT /users/settings (change email form)" do @tag :capture_log test "updates the user email", %{conn: conn, user: user} do - conn = - put(conn, ~p"/users/settings", %{ + response = + conn + |> put(~p"/users/settings", %{ "action" => "update_email", "user" => %{"email" => unique_user_email()} }) - assert redirected_to(conn) == ~p"/users/settings" + assert redirected_to(response) == ~p"/users/settings" - assert Phoenix.Flash.get(conn.assigns.flash, :info) =~ + assert Phoenix.Flash.get(response.assigns.flash, :info) =~ "A link to confirm your email" assert Accounts.get_user_by_email(user.email) end test "does not update email on invalid data", %{conn: conn} do - conn = - put(conn, ~p"/users/settings", %{ + response = + conn + |> put(~p"/users/settings", %{ "action" => "update_email", "user" => %{"email" => "with spaces"} }) + |> html_response(200) - response = html_response(conn, 200) assert response =~ "Settings" assert response =~ "must have the @ sign and no spaces" end diff --git a/plans/multi-line-conn-refactoring.md b/plans/multi-line-conn-refactoring.md new file mode 100644 index 0000000..5448262 --- /dev/null +++ b/plans/multi-line-conn-refactoring.md @@ -0,0 +1,192 @@ +# Multi-line conn refactoring + +Extend `refactor_conn_aliasing.sh` (or replace with an Elixir tool) to handle multi-line `conn = verb(conn, ...)` patterns that the current script misses. + +## Current state + +The script handles **single-line** triggers: + +```elixir +conn = get(conn, ~p"/path") +``` + +It misses **multi-line** triggers (11 remaining credo issues): + +```elixir +conn = + post(conn, ~p"/users/log-in", %{ + "user" => %{"email" => user.email, "password" => "pass"} + }) +``` + +## Multi-line trigger shapes + +Two forms observed in the codebase: + +### Form A — verb on next line + +```elixir +conn = + post(conn, ~p"/users/log-in", %{ + "user" => %{"email" => user.email} + }) +``` + +`conn =` ends the line; the verb call starts on the next line with deeper indentation. + +### Form B — verb on same line, args span multiple lines + +```elixir +conn = post(conn, ~p"/users/log-in", %{ + "user" => %{"email" => user.email} +}) +``` + +`conn = verb(conn,` starts on one line but the arguments (maps, keyword lists) span multiple lines. + +## Transformations + +The same four cases apply as for single-line, just with multi-line args preserved: + +### Case 1 — assign + helper on next line + +```elixir +# Before +conn = + post(conn, ~p"/users/log-in", %{ + "user" => %{"email" => user.email} + }) +response = html_response(conn, 200) + +# After +response = conn |> post(~p"/users/log-in", %{ + "user" => %{"email" => user.email} +}) |> html_response(200) +``` + +### Case 2 — assert helper on next line + +```elixir +# Before +conn = + post(conn, ~p"/users/log-in", %{ + "user" => %{"email" => user.email} + }) +assert html_response(conn, 200) =~ "Welcome" + +# After +assert conn |> post(~p"/users/log-in", %{ + "user" => %{"email" => user.email} +}) |> html_response(200) =~ "Welcome" +``` + +### Case 3 — assert pattern match + helper + +Same as Case 2 but with `assert %{body: body} = html_response(conn, 200)`. + +### Case 4 — multiple conn references ahead + +```elixir +# Before +conn = + post(conn, ~p"/users/log-in", %{ + "user" => %{"email" => user.email} + }) +assert redirected_to(conn) == ~p"/" +assert get_session(conn, :user_token) + +# After +response = conn |> post(~p"/users/log-in", %{ + "user" => %{"email" => user.email} +}) +assert redirected_to(response) == ~p"/" +assert get_session(response, :user_token) +``` + +## Algorithm + +### Phase 1 — Detect trigger start + +Match either: +- `conn =` at end of line (Form A, verb on next line) +- `conn = verb(conn,` where the line does not end with a balanced `)` (Form B, args continue on next line) + +### Phase 2 — Accumulate the expression + +Read subsequent lines until the expression is complete. Track parenthesis/brace/bracket depth to find the closing boundary: + +- Start with depth from the trigger line (e.g., `post(conn, ~p"/path", %{` has depth 2: one `(`, one `{`) +- For each subsequent line, update depth by counting opening vs closing delimiters +- When depth reaches 0, the expression is complete + +Buffer all accumulated lines. + +### Phase 3 — Extract verb and args + +From the accumulated block: +- **verb**: the HTTP verb (`get`, `post`, `put`, `patch`, `delete`, `head`, `options`) +- **args**: everything between `verb(conn,` and the final `)`, preserving original formatting (indentation, newlines) + +### Phase 4 — Peek ahead for case classification + +Skip blank lines after the accumulated expression. Read the next non-blank line (`next_line`). + +Classify as Case 1, 2, or 3 using the same patterns as today (helper assignment, assert helper, assert pattern match). + +Then **look ahead further** (skipping blanks, stopping at `end`, `conn =`, `test`, `describe`) to check if `conn` is referenced again. If so, override to Case 4. + +### Phase 5 — Emit output + +- **Cases 1–3**: emit a single merged pipeline line, inserting the multi-line args as-is: + ``` + indent + var + " = conn |> " + verb + "(" + args + ") |> " + helper + "(" + status + ")" + ``` + For multi-line args, the output will naturally span multiple lines. + +- **Case 4**: emit `response = conn |> verb(args)`, then rename `conn` → `response` in subsequent lines until scope boundary (`end`, `conn =`, `test`, `describe`). + +- **Fallback** (no conn on next line, no recognized pattern): emit the original trigger unchanged. + +## Remaining issues to handle + +| File | Line | Verb | +|---|---|---| +| `user_settings_controller_test.exs` | 74 | `put` | +| `user_settings_controller_test.exs` | 89 | `put` | +| `user_session_controller_test.exs` | 76 | `post` | +| `user_session_controller_test.exs` | 88 | `post` | +| `user_session_controller_test.exs` | 119 | `post` | +| `user_session_controller_test.exs` | 132 | `post` | +| `user_session_controller_test.exs` | 144 | `post` | +| `user_session_controller_test.exs` | 157 | `post` | +| `user_session_controller_test.exs` | 171 | `post` | +| `user_registration_controller_test.exs` | 44 | `post` | + +All in `app/test/firehose_web/controllers/`. + +## Elixir implementation notes (for future session) + +An Elixir implementation would be simpler than the awk script because: + +- Read file into a list of lines (already a list data structure) +- Pattern match on line content with guards +- Recurse or use `reduce_while` over the line list with explicit state (`:normal`, `:accumulating`, `:triggered`, `:renaming`) +- String manipulation with `String.split/2`, `String.replace/3`, etc. +- No need for regex capture groups — use `String.starts_with?/2`, `String.contains?/2`, etc. +- Parenthesis depth tracking is a simple integer accumulator + +Possible structure: + +```elixir +defmodule ConnRefactor do + def refactor(file_path) do + file_path + |> File.read!() + |> String.split("\n", trim: false) + |> do_refactor([], :normal, %{}) + |> Enum.join("\n") + end + + defp do_refactor(lines, output, state, context) +end +```