diff --git a/context.md b/context.md new file mode 100644 index 0000000..3282c66 --- /dev/null +++ b/context.md @@ -0,0 +1,172 @@ +# Code Context + +## Files Retrieved +List with exact line ranges: +1. `app/test/firehose_web/controllers/blog_test.exs` (lines 1-128) - Comprehensive blog controller tests covering HTML and JSON API endpoints for engineering blog and release notes +2. `app/lib/firehose_web/controllers/blog_controller.ex` (lines 1-79) - Blog controller with pagination, 404 handling, and input validation +3. `app/test/support/conn_case.ex` (lines 1-38) - Test case template for connection tests +4. `app/lib/firehose/blogs/engineering_blog.ex` (lines 1-7) - Engineering blog module configuration +5. `app/lib/firehose/blogs/release_notes.ex` (lines 1-7) - Release notes blog module configuration + +## Key Code + +### Test Organization +```elixir +# Current structure has 4 describe blocks: +describe "engineering blog (HTML)" # 3 tests +describe "input validation" # 5 tests (newly added in last commit) +describe "release notes blog (HTML)" # 2 tests +describe "engineering blog (JSON API)" # 4 tests +describe "release notes blog (JSON API)" # 3 tests +``` + +### Input Validation Logic (blog_controller.ex, lines 68-76) +```elixir +defp parse_page(nil), do: 1 +defp parse_page(str) do + case Integer.parse(str) do + {page, ""} when page > 0 -> page + _ -> 1 + end +end +``` + +### Test Coverage Added in Last Commit +```elixir +describe "input validation" do + test "GET /blog/nonexistent returns 404", %{conn: conn} do + conn = get(conn, "/blog/nonexistent") + assert html_response(conn, 404) + end + + test "GET /blog/engineering?page=abc falls back to page 1", %{conn: conn} do + conn = get(conn, "/blog/engineering?page=abc") + assert html_response(conn, 200) =~ "Engineering Blog" + end + + test "GET /blog/engineering?page=-1 falls back to page 1", %{conn: conn} do + conn = get(conn, "/blog/engineering?page=-1") + assert html_response(conn, 200) =~ "Engineering Blog" + end + + test "GET /blog/engineering?page=0 falls back to page 1", %{conn: conn} do + conn = get(conn, "/blog/engineering?page=0") + assert html_response(conn, 200) =~ "Engineering Blog" + end + + test "GET /blog/engineering/nonexistent-post returns 404", %{conn: conn} do + assert_raise Blogex.NotFoundError, fn -> + get(conn, "/blog/engineering/nonexistent-post") + end + end +end +``` + +## Architecture +The application uses: +- **Blogex** library for blog functionality (engineering blog and release notes) +- **Phoenix** framework for web endpoints +- **ConnCase** test helper for connection testing +- Two blog types: `Firehose.EngineeringBlog` and `Firehose.ReleaseNotes` +- Pagination through `blog.paginate(page)` method +- 404 handling via `Blogex.NotFoundError` exception + +## Start Here +Which file to look at first and why: + +**Start with `app/lib/firehose_web/controllers/blog_controller.ex`** + +Why: This is the central controller that handles all blog requests. Understanding its structure (especially the `parse_page/1` function and `resolve_blog/2` plug) provides context for why the validation tests were added and how input handling works across both HTML and JSON endpoints. + +## Code Smells & Refactoring Suggestions + +### Smell 1: Repetitive Validation Tests +**Issue**: Four tests for page parameter validation (`page=abc`, `-1`, `0`, and valid values) are highly repetitive with identical assertions. + +**Refactoring Suggestion**: Use parameterized tests or test helpers: +```elixir +# Test helper approach +test_page_fallback("page=abc", "abc") +test_page_fallback("page=-1", "-1") +test_page_fallback("page=0", "0") + +defp test_page_fallback(query_param, expected_page) do + conn = get(conn, "/blog/engineering?#{query_param}") + assert html_response(conn, 200) =~ "Engineering Blog" +end +``` + +### Smell 2: Missing Negative Test Coverage +**Issue**: Tests don't verify what happens when invalid blog_id is provided (e.g., `/blog/invalid-blog`). + +**Refactoring Suggestion**: Add test for unknown blog: +```elixir +test "GET /blog/unknown returns 404", %{conn: conn} do + conn = get(conn, "/blog/unknown") + assert html_response(conn, 404) +end +``` + +### Smell 3: Inconsistent Test Naming +**Issue**: Some tests use hyphenated slugs (`v0-1-0`), others use different formats. The naming doesn't clearly indicate what's being tested. + +**Refactoring Suggestion**: Standardize naming: +```elixir +# Instead of: "GET /blog/releases/v0-1-0 returns HTML post" +test "GET /blog/releases/:slug returns a release post", %{conn: conn} do +``` + +### Smell 4: Redundant Layout Assertions +**Issue**: Multiple tests assert the same "firehose" string appears in response, testing layout presence. + +**Refactoring Suggestion**: Create a shared test helper: +```elixir +defp assert_has_app_layout(body), + do: assert body =~ "firehose" + +# Then in tests: assert_has_app_layout(body) +``` + +### Smell 5: Test Order Doesn't Follow Flow +**Issue**: Tests are grouped by endpoint but validation tests (which should be first for defensive programming) are in the middle. + +**Refactoring Suggestion**: Reorder to follow natural request flow: +1. Input validation (404s, invalid params) +2. Success cases (index, show, tag) +3. Edge cases (pagination, RSS feeds) + +### Smell 6: No Test for Controller-Level Error Handling +**Issue**: The controller uses `halt()` in the resolve_blog plug, but there's no test verifying this behavior. + +**Refactoring Suggestion**: Add test: +```elixir +test "GET /blog/:blog_id with invalid blog halts request", %{conn: conn} do + conn = get(conn, "/blog/invalid") + assert conn.halted +end +``` + +### Smell 7: Mixed Response Types Without Clear Separation +**Issue**: HTML tests use `html_response/2`, JSON tests use `json_response/2`, but there's no helper to verify content type before parsing. + +**Refactoring Suggestion**: Create response helpers: +```elixir +defp assert_html(conn, status), do: assert html_response(conn, status) != "" +defp assert_json(conn, status), do: assert json_response(conn, status) != %{} +``` + +### Smell 8: No Test for Concurrent Requests or Edge Cases +**Issue**: Missing tests for: +- Empty page parameter (`?page=`) +- Very large page numbers +- Special characters in slug/tag parameters + +**Refactoring Suggestion**: Add edge case tests to validation describe block. + +### Overall Recommendations +1. **Extract test helpers** to reduce duplication (especially for page validation) +2. **Standardize test naming** conventions across all blog types +3. **Add positive test** for valid page numbers (currently missing) +4. **Consider property-based testing** for input validation scenarios +5. **Add performance tests** if pagination is used heavily +6. **Create integration tests** that verify end-to-end flows \ No newline at end of file