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.
This commit is contained in:
parent
a89d09e432
commit
0513154b01
@ -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
|
||||
|
||||
@ -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
|
||||
|
||||
|
||||
@ -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
|
||||
|
||||
192
plans/multi-line-conn-refactoring.md
Normal file
192
plans/multi-line-conn-refactoring.md
Normal file
@ -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
|
||||
```
|
||||
Loading…
x
Reference in New Issue
Block a user