Scaffold Rails + publicaciones estaticas (Fase 1) #5
Loading…
Add table
Add a link
Reference in a new issue
No description provided.
Delete branch "2-scaffold-rails"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
Summary
Scaffold completo de Rails 8.1 con docker-compose, controlador de paginas, vistas en espanol y tema oscuro que coincide con ereccion.html.
Changes
inicioyacercaroot 'pages#inicio',get 'acerca', health check en/uppublic/ereccion.html,public/favicon.svg) sin modificarTest Plan
docker compose upinicia sin erroresGET /retorna 200 con contenido de inicio en espanolGET /acercaretorna 200 con descripcion del proyectoGET /ereccion.htmlretorna 200 (archivo estatico servido por Rails)GET /upretorna 200 (health check)Review Checklist
Related Notes
Ninguna nota de pal-e-docs relacionada.
Related
Closes #2
PR #5 Review
PR: Scaffold Rails + publicaciones estaticas (Fase 1)
Branch: 2-scaffold-rails -> main
Parent Issue: #2
Stats: 93 files changed, +2,435 / -22
DOMAIN REVIEW
Stack: Ruby on Rails 8.1.3, PostgreSQL 17, Docker Compose, Hotwire (Turbo/Stimulus), Propshaft
Rails architecture:
PagesControllerwith two empty actions (inicio,acerca) that rely on convention-based view rendering. Correct pattern for static pages.root "pages#inicio", namedacercaroute, health check at/up. No unnecessary RESTful scaffolding for what are static pages.database.ymlcorrectly usesENV.fetch()with sensible defaults for all connection params. Production config splits cache/queue/cable into separate databases per Rails 8 convention.Docker Compose:
harbor.tail5b443a.ts.net/library/ruby-rails-build:latest). No Dockerfile in the PR, which is fine -- image is built externally.rails server -p 9999command.rails/rails) are appropriate for local dev docker-compose. These are not production secrets.Frontend / HTML / CSS:
lang="es"on the HTML element -- correct for a Spanish-language site.--bg,--text,--accent,--muted,--section-border,--nav-bg) are well-organized.clamp()for fluid typography on hero text -- good pattern.Content:
/ereccion.html) correctly uses a plain<a>tag instead oflink_tosince it's not a Rails route.BLOCKERS
1. No controller or integration tests (BLOCKER)
The test infrastructure exists (test_helper.rb, test environment config, Capybara/Selenium in Gemfile) but all test directories contain only
.keepplaceholder files. There are zero actual test files.For a scaffold PR, at minimum the following should exist:
test/controllers/pages_controller_test.rb-- verifyGET /returns 200,GET /acercareturns 200, response bodies contain expected contentThe PR's own "Test Plan" section lists manual verification steps (
GET /retorna 200,GET /acercaretorna 200, etc.) -- these are exactly the tests that should be automated.Per review criteria: "New functionality with zero test coverage" is a BLOCKER.
NITS
1. Inline CSS in layout (minor)
All 140+ lines of CSS are inlined in
<style>tags withinapplication.html.erb. For a small project this works, but as the site grows this will become unwieldy. Consider moving toapp/assets/stylesheets/application.csswhich Propshaft already serves. This would also benefit from cache headers on the CSS asset.2. Missing accent marks in Spanish text (cosmetic)
The content consistently omits accent marks (tildes) -- "Espanol" instead of "Espanol" with tilde, "pagina" instead of "pagina" with accent, "reflexion" instead of "reflexion" with accent, etc. This appears intentional (matching the repo name and existing
ereccion.html), but worth confirming it is a deliberate style choice rather than an oversight.3. No
aria-currentor active-link highlightingThe nav links have a
.activoCSS class defined but it is never applied dynamically. Consider addingclass: ("activo" if current_page?(root_path))to nav links for accessibility and visual feedback.4. Footer year is hardcoded
lucas draney · 2026-- consider using<%= Date.current.year %>to avoid annual maintenance.5.
max_connectionsnaming in database.ymlLine
max_connections: <%= ENV.fetch("RAILS_MAX_THREADS") { 5 } %>-- the keymax_connectionswas renamed topoolin standard Rails database.yml. Whilemax_connectionsmay work depending on the adapter version,poolis the canonical name. Verify this works with Rails 8.1.3 and pg adapter.SOP COMPLIANCE
2-scaffold-rails)PROCESS OBSERVATIONS
VERDICT: NOT APPROVED
Reason: Zero test coverage for new controller actions. Add at minimum
test/controllers/pages_controller_test.rbwith tests forGET /andGET /acerca(status 200, correct content rendered). The test infrastructure is already in place -- only the actual test file is missing.PR #5 Re-Review
Re-review after previous blocker (zero test coverage) was addressed with a new commit adding
test/controllers/pages_controller_test.rb.DOMAIN REVIEW
Stack: Ruby on Rails 8.1, PostgreSQL 17, Docker Compose, Minitest (ActionDispatch::IntegrationTest).
test/controllers/pages_controller_test.rb (NEW -- 65 lines, 11 tests)
The test file is well-structured and covers the right surface area for this phase:
inicio(root) andacercaboth return 200 -- good.assert_selectverifies<h1>,<h2>, navigation links, favicon<link>tag, and<html lang="es">-- these are meaningful structural tests, not just status checks./,/acerca,/ereccion.html).lang="es"attribute verified -- important for screen readers and search engines.link[rel='icon'][type='image/svg+xml'][href='/favicon.svg'].Test names are in Spanish, matching the project language -- consistent.
Missing but acceptable for Fase 1:
GET /ereccion.html(static file served by Rails/Rack, not by PagesController -- testing this would be a system test concern, not a controller test concern).GET /uphealth check (Rails built-in, not custom code).app/controllers/pages_controller.rb (7 lines)
Minimal controller with two empty actions relying on convention-over-configuration for view rendering. Correct for this scope. No logic to test beyond what the integration tests already cover.
app/views/layouts/application.html.erb (186 lines)
<style>block: 140+ lines of CSS inlined in the layout. This works for Fase 1 but should move to a stylesheet file in a future phase. Nit, not a blocker.--bg,--text,--accent,--muted,--section-border,--nav-bg) -- good for theming.csrf_meta_tagsandcsp_meta_tagpresent -- good security baseline./ereccion.htmllink in nav: acceptable since it is a static file outside Rails routing. Could use a helper constant later.app/views/pages/inicio.html.erb and acerca.html.erb
Clean ERB. Uses
content_for(:title)for per-page titles. Useslink_towith named routes (acerca_path,root_path) -- correct. One hardcoded link (/ereccion.html) is appropriate for a static asset.config/routes.rb (10 lines)
root "pages#inicio"-- correct.get "acerca", to: "pages#acerca"-- correct./up-- standard Rails 8 pattern.config/database.yml (32 lines)
ENV.fetchwith sensible defaults -- good.max_connectionsreads fromRAILS_MAX_THREADS-- fine, but the key name ismax_connectionsnotpool(Rails 8.1 change). Confirmed this is the correct key for Rails 8.1+."rails"for dev/test is fine (not a secret -- local Docker only).docker-compose.yml (32 lines)
POSTGRES_PASSWORD: railsin plaintext: acceptable for local dev. Not a production config.harbor.tail5b443a.ts.net/library/ruby-rails-build:latest-- uses internal Harbor registry. The:latesttag is fine for dev but should be pinned for CI/prod (future phase concern).Gemfile (60 lines)
pg,puma,propshaft, Hotwire stack, solid_* gems.bundler-auditandbrakeman-- good.rubocop-rails-omakasefor linting -- good.capybaraandselenium-webdriverfor future system tests.BLOCKERS
None. The previous blocker (zero test coverage for PagesController) is resolved. The new test file provides 11 integration tests covering both controller actions, view content, navigation structure, favicon, and lang attribute. This is adequate coverage for the scope of Fase 1.
No secrets committed. No unvalidated user input (no user input paths exist -- read-only pages). No DRY violations in security paths. No credentials in code beyond local dev Docker defaults.
NITS
<style>block. Consider extracting toapp/assets/stylesheets/in a future phase for cacheability and maintainability.alttext concern: Not applicable here (no<img>tags), but worth noting accessibility as a pattern for future phases with images.<h1>and nav text could use "Espanol" with tilde for proper Spanish). Similarly "Ereccion" in nav. Minor UX polish.SOP COMPLIANCE
PROCESS OBSERVATIONS
VERDICT: APPROVED
PR #5 Review
DOMAIN REVIEW
Stack: Ruby on Rails 8.1.3, PostgreSQL 17, Docker Compose, Propshaft, Hotwire (Turbo + Stimulus), HTML/CSS.
Rails quality:
acercaroute, health check. Comments in Spanish match the project language.database.ymlproperly usesENV.fetchwith fallback defaults for development. Production config correctly inherits and defines separate databases for cache/queue/cable (Solid Stack pattern).config/application.rbusesconfig.load_defaults 8.1-- matches the Gemfile's Rails version.credentials.yml.encis committed (expected) and.gitignoreexcludesconfig/*.key-- master key is safe.HTML/CSS quality:
lang="es"-- correct for a Spanish-language site.:rootvariables) for theming -- clean, maintainable approach.clamp()used for fluid typography -- good practice.Docker Compose:
harbor.tail5b443a.ts.net/library/ruby-rails-build:latest) -- appropriate for the platform.pgdataandbundleprevent data loss across restarts.Test coverage:
test/controllers/pages_controller_test.rbcovering:BLOCKERS
None. No blocking issues found.
NITS
Puma port mismatch (low risk):
config/puma.rbdefaults to port 3000 (ENV.fetch("PORT", 3000)) whiledocker-compose.ymlruns the server explicitly on port 9999 viacommand: bundle exec rails server -b 0.0.0.0 -p 9999. This works because the command flag overrides Puma's config, but it creates a confusing discrepancy. Consider settingPORT=9999in the docker-compose environment block so both agree, or changing the Puma default.README regression: The original README had project-specific content (description, quick-start, architecture links, related repos). This PR replaces it with the default Rails
rails newboilerplate ("This README would normally document..."). The original README content was more informative and should be preserved or adapted rather than overwritten.All CSS is inline in the layout: The 150+ lines of CSS in
app/views/layouts/application.html.erbshould live inapp/assets/stylesheets/application.css(which is currently empty aside from comments). Inline styles in the layout work but bypass the asset pipeline (no fingerprinting, no caching headers) and will become harder to maintain as the site grows.Hardcoded default passwords in
database.yml:ENV.fetch("DATABASE_PASSWORD", "rails")provides a plaintext fallback. This is fine for local development, but if this default ever leaks into a production-like environment it could be a problem. Consider removing the fallback forDATABASE_PASSWORDso it fails loudly if unset:ENV.fetch("DATABASE_PASSWORD")..current-issuefile: This tracking file is committed to the repo. It serves a workflow purpose but is not application code. Consider adding it to.gitignoreif it is only used by the development tooling and should not be part of the deployed codebase.Missing accent marks in Spanish text: Several visible strings omit tildes/accents where standard Spanish requires them: "Espanol" should be "Espanol" (acceptable if the project intentionally avoids special characters in code), but the PR body and issue use accented forms. Minor consistency point.
Content Security Policy disabled:
config/initializers/content_security_policy.rbis entirely commented out. This is the Rails default, but since the app serves user-visible HTML with inline styles, consider enabling at least a basic CSP in Fase 2/3 when the CI/CD pipeline is established.SOP COMPLIANCE
2-scaffold-railsreferences issue #2)Closes #2test/controllers/pages_controller_test.rb-- 11 tests)config/*.keyin.gitignore,.env*in.gitignore)rails newscaffold; custom files align with issue scopePROCESS OBSERVATIONS
rails newand does not represent scope creep -- the custom work (controller, views, routes, layout, tests, docker-compose, database config) is well-scoped to issue #2.VERDICT: APPROVED