From 2c04997c9735b5d8eb3d6ecf8b961c2957b7f13b Mon Sep 17 00:00:00 2001 From: Ivan Kuznetsov Date: Tue, 20 Jan 2026 09:14:58 +0000 Subject: [PATCH 01/10] feat(ci): Add generator E2E test infrastructure MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Add CI pipeline validation that generators produce working views: - bin/test_generator script creates fresh Rails app, runs generators, and executes comprehensive E2E tests using Capybara Playwright - Reusable Page Object test templates in test/generator_test_files/ - GitHub Actions generator-test job runs after unit tests pass Also fixes Session model autoloading by moving from lib/ to app/models/ for proper Rails engine autoloading. ๐Ÿค– Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 --- .github/workflows/ci.yml | 23 ++ .rubocop.yml | 15 ++ CHANGELOG.md | 11 + Rakefile | 3 +- .../models/rails_simple_auth}/session.rb | 3 +- bin/test_generator | 226 ++++++++++++++++++ .../controllers/dashboard_controller_test.rb | 52 ++++ .../controllers/home_controller_test.rb | 28 +++ test/generator_test_files/models/user_test.rb | 145 +++++++++++ test/generator_test_files/pages/base_page.rb | 32 +++ .../pages/confirmation_page.rb | 38 +++ .../pages/dashboard_page.rb | 23 ++ .../pages/landing_page.rb | 24 ++ .../pages/magic_link_page.rb | 25 ++ .../pages/password_reset_page.rb | 47 ++++ .../pages/sign_in_page.rb | 44 ++++ .../pages/sign_up_page.rb | 34 +++ .../system/authentication_test.rb | 139 +++++++++++ .../system/email_confirmation_test.rb | 70 ++++++ .../system/magic_link_test.rb | 70 ++++++ .../system/password_reset_test.rb | 81 +++++++ 21 files changed, 1130 insertions(+), 3 deletions(-) rename {lib/rails_simple_auth/models => app/models/rails_simple_auth}/session.rb (79%) create mode 100755 bin/test_generator create mode 100644 test/generator_test_files/controllers/dashboard_controller_test.rb create mode 100644 test/generator_test_files/controllers/home_controller_test.rb create mode 100644 test/generator_test_files/models/user_test.rb create mode 100644 test/generator_test_files/pages/base_page.rb create mode 100644 test/generator_test_files/pages/confirmation_page.rb create mode 100644 test/generator_test_files/pages/dashboard_page.rb create mode 100644 test/generator_test_files/pages/landing_page.rb create mode 100644 test/generator_test_files/pages/magic_link_page.rb create mode 100644 test/generator_test_files/pages/password_reset_page.rb create mode 100644 test/generator_test_files/pages/sign_in_page.rb create mode 100644 test/generator_test_files/pages/sign_up_page.rb create mode 100644 test/generator_test_files/system/authentication_test.rb create mode 100644 test/generator_test_files/system/email_confirmation_test.rb create mode 100644 test/generator_test_files/system/magic_link_test.rb create mode 100644 test/generator_test_files/system/password_reset_test.rb diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 4c6630b..076a49b 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -39,3 +39,26 @@ jobs: bundler-cache: true - name: Run RuboCop run: bundle exec rubocop + + generator-test: + runs-on: ubuntu-latest + needs: [test] + steps: + - uses: actions/checkout@v6 + + - name: Set up Ruby + uses: ruby/setup-ruby@v1 + with: + ruby-version: '3.4' + bundler-cache: true + + - name: Install Rails + run: gem install rails + + - name: Install Playwright + run: npx playwright install --with-deps chromium + + - name: Run generator test + env: + HEADLESS: 'true' + run: bin/test_generator diff --git a/.rubocop.yml b/.rubocop.yml index 63567a7..ba51bf2 100644 --- a/.rubocop.yml +++ b/.rubocop.yml @@ -58,6 +58,7 @@ Metrics/ClassLength: Metrics/CyclomaticComplexity: Exclude: - 'lib/rails_simple_auth/models/concerns/**/*' + - 'lib/rails_simple_auth/controllers/concerns/**/*' Metrics/PerceivedComplexity: Exclude: @@ -82,3 +83,17 @@ Rails/ReflectionClassName: Style/SafeNavigationChainLength: Enabled: false + +# Generator test files use Capybara naming convention +Naming/PredicatePrefix: + Exclude: + - 'test/generator_test_files/**/*' + +# Generator test files are templates, not part of gem test suite +Minitest/MultipleAssertions: + Max: 5 + +# Allow generated test files to have longer files +Metrics/ModuleLength: + Exclude: + - 'test/generator_test_files/**/*' diff --git a/CHANGELOG.md b/CHANGELOG.md index e80b8e9..e082b62 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -7,6 +7,17 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ## [Unreleased] +### Added + +- **Generator E2E test infrastructure** - CI pipeline now validates that generators produce working views + - `bin/test_generator` script creates fresh Rails app, runs generators, and executes E2E tests + - Reusable Page Object test templates in `test/generator_test_files/` + - GitHub Actions `generator-test` job runs after unit tests pass + +### Fixed + +- **Session model autoloading** - Moved `RailsSimpleAuth::Session` from `lib/` to `app/models/` for proper Rails engine autoloading + ## [1.0.14] - 2026-01-20 ### Fixed diff --git a/Rakefile b/Rakefile index 4caa98e..2fee9c4 100644 --- a/Rakefile +++ b/Rakefile @@ -6,7 +6,8 @@ require 'rake/testtask' Rake::TestTask.new(:test) do |t| t.libs << 'test' t.libs << 'lib' - t.test_files = FileList['test/**/*_test.rb'] + # Exclude generator_test_files - those are template tests for generated apps + t.test_files = FileList['test/**/*_test.rb'].exclude('test/generator_test_files/**/*') end task default: :test diff --git a/lib/rails_simple_auth/models/session.rb b/app/models/rails_simple_auth/session.rb similarity index 79% rename from lib/rails_simple_auth/models/session.rb rename to app/models/rails_simple_auth/session.rb index a3a9066..206a666 100644 --- a/lib/rails_simple_auth/models/session.rb +++ b/app/models/rails_simple_auth/session.rb @@ -4,8 +4,7 @@ module RailsSimpleAuth class Session < ::ApplicationRecord self.table_name = 'sessions' - # Use lambda to defer class resolution until runtime - belongs_to :user, class_name: -> { RailsSimpleAuth.configuration.user_class_name } + belongs_to :user, class_name: RailsSimpleAuth.configuration.user_class_name scope :recent, -> { order(created_at: :desc) } scope :active, -> { where(created_at: RailsSimpleAuth.configuration.session_expiry.ago..) } diff --git a/bin/test_generator b/bin/test_generator new file mode 100755 index 0000000..010f474 --- /dev/null +++ b/bin/test_generator @@ -0,0 +1,226 @@ +#!/usr/bin/env bash +set -e + +# Test rails_simple_auth generator by creating a fresh Rails app and running tests +# This script validates that the gem's generators and default views work correctly + +SCRIPT_DIR="$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd)" +GEM_DIR="$(dirname "$SCRIPT_DIR")" +TEST_APP_DIR="${TEST_APP_DIR:-/tmp/rails_simple_auth_test_app}" +HEADLESS="${HEADLESS:-true}" + +echo "=== rails_simple_auth Generator Test ===" +echo "Gem directory: $GEM_DIR" +echo "Test app directory: $TEST_APP_DIR" +echo "" + +# Clean up previous test app +if [ -d "$TEST_APP_DIR" ]; then + echo "Removing existing test app..." + rm -rf "$TEST_APP_DIR" +fi + +# Create new Rails app +echo "Creating new Rails app..." +rails new "$TEST_APP_DIR" --skip-test --skip-git --skip-docker --skip-action-mailbox --skip-action-text --skip-active-storage + +cd "$TEST_APP_DIR" + +# Add gems to Gemfile +echo "Adding gems to Gemfile..." +cat >> Gemfile << 'EOF' + +# Authentication gem (local development) +gem "rails_simple_auth", path: ENV.fetch("GEM_PATH", "../rails_simple_auth") + +group :test do + gem "capybara" + gem "capybara-playwright-driver" +end +EOF + +# Install dependencies +echo "Installing dependencies..." +GEM_PATH="$GEM_DIR" bundle install + +# Run the install generator +echo "Running rails_simple_auth:install generator..." +bin/rails generate rails_simple_auth:install + +# Create User model with authenticates_with +echo "Creating User model..." +cat > app/models/user.rb << 'EOF' +class User < ApplicationRecord + authenticates_with :confirmable, :magic_linkable +end +EOF + +# Run migrations +echo "Running migrations..." +bin/rails db:migrate + +# Create dashboard controller +echo "Creating DashboardController..." +mkdir -p app/controllers +cat > app/controllers/dashboard_controller.rb << 'EOF' +class DashboardController < ApplicationController + before_action :require_authentication + + def show + end +end +EOF + +# Create dashboard view +echo "Creating dashboard view..." +mkdir -p app/views/dashboard +cat > app/views/dashboard/show.html.erb << 'EOF' +
+
+

Dashboard

+

+ Welcome, <%= current_user.email %> +

+

+ You are signed in. +

+ <%= button_to "Sign Out", session_path, method: :delete, class: "rsa-auth-form__submit rsa-auth-form__submit--danger" %> +
+
+EOF + +# Create home controller and view +echo "Creating HomeController..." +cat > app/controllers/home_controller.rb << 'EOF' +class HomeController < ApplicationController + def index + redirect_to dashboard_path if user_signed_in? + end +end +EOF + +mkdir -p app/views/home +cat > app/views/home/index.html.erb << 'EOF' +
+
+

rails_simple_auth Demo

+

+ A minimal authentication gem for Rails +

+
+ <%= link_to "Sign In", new_session_path, class: "rsa-auth-form__submit" %> + <%= link_to "Sign Up", sign_up_path, class: "rsa-auth-form__submit rsa-auth-form__submit--secondary" %> +
+
+
+EOF + +# Add routes +echo "Adding routes..." +cat > config/routes.rb << 'EOF' +Rails.application.routes.draw do + rails_simple_auth_routes + + get "dashboard", to: "dashboard#show" + root "home#index" +end +EOF + +# Update application layout for flash messages +echo "Updating application layout..." +cat > app/views/layouts/application.html.erb << 'EOF' + + + + rails_simple_auth Test App + + <%= csrf_meta_tags %> + <%= csp_meta_tag %> + <%= stylesheet_link_tag "rails_simple_auth", "data-turbo-track": "reload" %> + <%= javascript_importmap_tags %> + + +
+ <% flash.each do |type, message| %> +
+ <%= message %> +
+ <% end %> + <%= yield %> +
+ + +EOF + +# Copy test files from gem +echo "Copying test infrastructure..." +mkdir -p test/support/pages test/system test/models test/controllers + +# Copy base page +cat > test/support/pages/base_page.rb << 'EOF' +# frozen_string_literal: true + +module Pages + class BasePage + include Capybara::DSL + include Capybara::Minitest::Assertions + + attr_reader :test_context + + def initialize(test_context) + @test_context = test_context + end + end +end +EOF + +# Copy page objects from the gem's test directory +cp "$GEM_DIR/test/generator_test_files/pages/"*.rb test/support/pages/ 2>/dev/null || true +cp "$GEM_DIR/test/generator_test_files/system/"*.rb test/system/ 2>/dev/null || true +cp "$GEM_DIR/test/generator_test_files/models/"*.rb test/models/ 2>/dev/null || true +cp "$GEM_DIR/test/generator_test_files/controllers/"*.rb test/controllers/ 2>/dev/null || true + +# Create test helper +cat > test/test_helper.rb << 'EOF' +ENV["RAILS_ENV"] ||= "test" +require_relative "../config/environment" +require "rails/test_help" +require "capybara/rails" +require "capybara/minitest" + +Capybara.register_driver(:playwright) do |app| + Capybara::Playwright::Driver.new(app, + browser_type: :chromium, + headless: ENV["HEADLESS"] != "false") +end + +Capybara.default_driver = :playwright +Capybara.javascript_driver = :playwright + +Dir[Rails.root.join("test/support/**/*.rb")].each { |f| require f } + +class ActiveSupport::TestCase + parallelize(workers: :number_of_processors) +end + +class ActionDispatch::IntegrationTest + include Capybara::DSL + include Capybara::Minitest::Assertions + + def teardown + Capybara.reset_sessions! + end +end +EOF + +# Install Playwright browser +echo "Installing Playwright browser..." +npx playwright install chromium 2>/dev/null || echo "Playwright browser installation skipped" + +# Run tests +echo "" +echo "=== Running Tests ===" +HEADLESS="$HEADLESS" bin/rails test:all + +echo "" +echo "=== All tests passed! ===" diff --git a/test/generator_test_files/controllers/dashboard_controller_test.rb b/test/generator_test_files/controllers/dashboard_controller_test.rb new file mode 100644 index 0000000..bef6f8c --- /dev/null +++ b/test/generator_test_files/controllers/dashboard_controller_test.rb @@ -0,0 +1,52 @@ +# frozen_string_literal: true + +require 'test_helper' + +class DashboardControllerTest < ActionDispatch::IntegrationTest + test 'should redirect unauthenticated user to sign in' do + get dashboard_path + + assert_redirected_to new_session_path + end + + test 'should get show for authenticated user' do + user = User.create!( + email: 'dashboard@example.com', + password: 'password123', + password_confirmation: 'password123', + confirmed_at: Time.current + ) + + # Sign in the user + post session_path, params: { email: user.email, password: 'password123' } + + get dashboard_path + + assert_response :success + assert_match 'Dashboard', response.body + assert_match user.email, response.body + end + + test 'should sign out user' do + user = User.create!( + email: 'signout@example.com', + password: 'password123', + password_confirmation: 'password123', + confirmed_at: Time.current + ) + + # Sign in the user + post session_path, params: { email: user.email, password: 'password123' } + + # Sign out + delete session_path + + # Should be redirected + assert_response :redirect + + # Dashboard should now redirect to sign in + get dashboard_path + + assert_redirected_to new_session_path + end +end diff --git a/test/generator_test_files/controllers/home_controller_test.rb b/test/generator_test_files/controllers/home_controller_test.rb new file mode 100644 index 0000000..35411e8 --- /dev/null +++ b/test/generator_test_files/controllers/home_controller_test.rb @@ -0,0 +1,28 @@ +# frozen_string_literal: true + +require 'test_helper' + +class HomeControllerTest < ActionDispatch::IntegrationTest + test 'should get index for unauthenticated user' do + get root_path + + assert_response :success + assert_match 'rails_simple_auth Demo', response.body + end + + test 'should redirect authenticated user to dashboard' do + user = User.create!( + email: 'home_redirect@example.com', + password: 'password123', + password_confirmation: 'password123', + confirmed_at: Time.current + ) + + # Sign in the user + post session_path, params: { email: user.email, password: 'password123' } + + get root_path + + assert_redirected_to dashboard_path + end +end diff --git a/test/generator_test_files/models/user_test.rb b/test/generator_test_files/models/user_test.rb new file mode 100644 index 0000000..7fd0db6 --- /dev/null +++ b/test/generator_test_files/models/user_test.rb @@ -0,0 +1,145 @@ +# frozen_string_literal: true + +require 'test_helper' + +class UserTest < ActiveSupport::TestCase + test 'user has authenticates_with modules included' do + user = User.new + + assert_respond_to user, :authenticate, 'User should respond to authenticate' + assert_respond_to user, :confirmed?, 'User should respond to confirmed?' + assert_respond_to user, :generate_magic_link_token, 'User should respond to generate_magic_link_token' + end + + test 'user can be created with valid attributes' do + user = User.new( + email: 'valid@example.com', + password: 'password123', + password_confirmation: 'password123' + ) + + assert_predicate user, :valid?, "User should be valid: #{user.errors.full_messages.join(', ')}" + end + + test 'user requires email' do + user = User.new( + password: 'password123', + password_confirmation: 'password123' + ) + + assert_not user.valid?, 'User should not be valid without email' + assert_includes user.errors[:email], "can't be blank" + end + + test 'user requires password' do + user = User.new(email: 'test@example.com') + + assert_not user.valid?, 'User should not be valid without password' + assert_predicate user.errors[:password], :any?, 'Should have password error' + end + + test 'user email must be unique' do + User.create!( + email: 'duplicate@example.com', + password: 'password123', + password_confirmation: 'password123', + confirmed_at: Time.current + ) + + user = User.new( + email: 'duplicate@example.com', + password: 'password123', + password_confirmation: 'password123' + ) + + assert_not user.valid?, 'User should not be valid with duplicate email' + assert_includes user.errors[:email], 'has already been taken' + end + + test 'user password must meet minimum length' do + user = User.new( + email: 'short@example.com', + password: 'short', + password_confirmation: 'short' + ) + + assert_not user.valid?, 'User should not be valid with short password' + assert user.errors[:password].any? { |e| e.include?('at least') }, + 'Should have password length error' + end + + test 'user can authenticate with correct password' do + user = User.create!( + email: 'auth@example.com', + password: 'password123', + password_confirmation: 'password123', + confirmed_at: Time.current + ) + + assert user.authenticate('password123'), 'Should authenticate with correct password' + end + + test 'user cannot authenticate with incorrect password' do + user = User.create!( + email: 'auth_fail@example.com', + password: 'password123', + password_confirmation: 'password123', + confirmed_at: Time.current + ) + + assert_not user.authenticate('wrongpassword'), 'Should not authenticate with wrong password' + end + + test 'user confirmed? returns true when confirmed_at is set' do + user = User.new(confirmed_at: Time.current) + + assert_predicate user, :confirmed?, 'User should be confirmed when confirmed_at is set' + end + + test 'user confirmed? returns false when confirmed_at is nil' do + user = User.new(confirmed_at: nil) + + assert_not user.confirmed?, 'User should not be confirmed when confirmed_at is nil' + end + + test 'user can generate magic link token' do + user = User.create!( + email: 'magic@example.com', + password: 'password123', + password_confirmation: 'password123', + confirmed_at: Time.current + ) + + token = user.generate_magic_link_token + + assert_predicate token, :present?, 'Should generate magic link token' + assert_kind_of String, token + end + + test 'user can generate password reset token' do + user = User.create!( + email: 'reset@example.com', + password: 'password123', + password_confirmation: 'password123', + confirmed_at: Time.current + ) + + token = user.generate_password_reset_token + + assert_predicate token, :present?, 'Should generate password reset token' + assert_kind_of String, token + end + + test 'user can generate confirmation token' do + user = User.create!( + email: 'confirm_token@example.com', + password: 'password123', + password_confirmation: 'password123' + ) + + token = user.generate_confirmation_token + + assert_predicate token, :present?, 'Should generate confirmation token' + assert_kind_of String, token + end +end diff --git a/test/generator_test_files/pages/base_page.rb b/test/generator_test_files/pages/base_page.rb new file mode 100644 index 0000000..266a8e7 --- /dev/null +++ b/test/generator_test_files/pages/base_page.rb @@ -0,0 +1,32 @@ +# frozen_string_literal: true + +module Pages + class BasePage + include Capybara::DSL + include Capybara::Minitest::Assertions + + def initialize(test_context) + @test_context = test_context + end + + def has_flash_notice?(message) + has_selector?('.rsa-flash--notice', text: message, wait: 5) || + has_selector?('[data-flash="notice"]', text: message, wait: 5) || + has_text?(message) + end + + def has_flash_alert?(message) + has_selector?('.rsa-flash--alert', text: message, wait: 5) || + has_selector?('[data-flash="alert"]', text: message, wait: 5) || + has_text?(message) + end + + def current_path + URI.parse(page.current_url).path + end + + private + + attr_reader :test_context + end +end diff --git a/test/generator_test_files/pages/confirmation_page.rb b/test/generator_test_files/pages/confirmation_page.rb new file mode 100644 index 0000000..5cdcf79 --- /dev/null +++ b/test/generator_test_files/pages/confirmation_page.rb @@ -0,0 +1,38 @@ +# frozen_string_literal: true + +module Pages + class ConfirmationPage < BasePage + def visit_page + visit '/confirmations/new' + self + end + + def displayed? + has_selector?('.rsa-auth-form__title', text: 'Resend Confirmation', wait: 5) + end + + def request_confirmation(email:) + fill_in 'Email', with: email + click_button 'Resend Confirmation' + self + end + + def confirm_with_token(token:) + visit "/confirmations/#{token}" + self + end + + def has_confirmation_sent_message? + # After submission, redirects to sign in page with flash notice + has_text?('confirmation instructions have been sent') + end + + def has_confirmed_message? + has_text?('Email confirmed') || has_text?('You can now sign in') + end + + def has_invalid_token_error? + has_text?('Invalid or expired confirmation link') + end + end +end diff --git a/test/generator_test_files/pages/dashboard_page.rb b/test/generator_test_files/pages/dashboard_page.rb new file mode 100644 index 0000000..046e72f --- /dev/null +++ b/test/generator_test_files/pages/dashboard_page.rb @@ -0,0 +1,23 @@ +# frozen_string_literal: true + +module Pages + class DashboardPage < BasePage + def visit_page + visit '/dashboard' + self + end + + def displayed? + has_selector?('.rsa-auth-form__title', text: 'Dashboard', wait: 5) + end + + def has_welcome_message_for?(email) + has_text?(email) + end + + def sign_out + click_button 'Sign Out' + Pages::SignInPage.new(test_context) + end + end +end diff --git a/test/generator_test_files/pages/landing_page.rb b/test/generator_test_files/pages/landing_page.rb new file mode 100644 index 0000000..e06d726 --- /dev/null +++ b/test/generator_test_files/pages/landing_page.rb @@ -0,0 +1,24 @@ +# frozen_string_literal: true + +module Pages + class LandingPage < BasePage + def visit_page + visit '/' + self + end + + def displayed? + has_selector?('.rsa-auth-form__title', text: 'rails_simple_auth Demo', wait: 5) + end + + def click_sign_in + click_link 'Sign In' + Pages::SignInPage.new(test_context) + end + + def click_sign_up + click_link 'Sign Up' + Pages::SignUpPage.new(test_context) + end + end +end diff --git a/test/generator_test_files/pages/magic_link_page.rb b/test/generator_test_files/pages/magic_link_page.rb new file mode 100644 index 0000000..c4f5701 --- /dev/null +++ b/test/generator_test_files/pages/magic_link_page.rb @@ -0,0 +1,25 @@ +# frozen_string_literal: true + +module Pages + class MagicLinkPage < BasePage + def visit_page + visit '/magic_link_form' + self + end + + def displayed? + has_selector?('.rsa-auth-form__title', text: 'Sign In with Magic Link', wait: 5) + end + + def request_magic_link(email:) + fill_in 'Email', with: email + click_button 'Send Magic Link' + self + end + + def has_magic_link_sent_message? + # After submission, redirects to sign in page with flash notice + has_text?('If an account exists') || has_text?('magic link has been sent') + end + end +end diff --git a/test/generator_test_files/pages/password_reset_page.rb b/test/generator_test_files/pages/password_reset_page.rb new file mode 100644 index 0000000..3954364 --- /dev/null +++ b/test/generator_test_files/pages/password_reset_page.rb @@ -0,0 +1,47 @@ +# frozen_string_literal: true + +module Pages + class PasswordResetPage < BasePage + def visit_page + visit '/passwords/new' + self + end + + def displayed? + has_selector?('.rsa-auth-form__title', text: 'Reset Password', wait: 5) + end + + def request_reset(email:) + fill_in 'Email', with: email + click_button 'Send Reset Instructions' + self + end + + def has_reset_email_sent_message? + # After submission, redirects to sign in page with flash notice + has_text?('If an account exists') || has_text?('instructions have been sent') + end + end + + class PasswordEditPage < BasePage + def visit_page(token:) + visit "/passwords/#{token}/edit" + self + end + + def displayed? + has_selector?('.rsa-auth-form__title', text: 'Set New Password', wait: 5) + end + + def reset_password(password:, password_confirmation: nil) + fill_in 'New Password', with: password + fill_in 'Confirm Password', with: password_confirmation || password + click_button 'Update Password' + self + end + + def has_invalid_token_error? + has_text?('Invalid or expired password reset link') + end + end +end diff --git a/test/generator_test_files/pages/sign_in_page.rb b/test/generator_test_files/pages/sign_in_page.rb new file mode 100644 index 0000000..dad35b1 --- /dev/null +++ b/test/generator_test_files/pages/sign_in_page.rb @@ -0,0 +1,44 @@ +# frozen_string_literal: true + +module Pages + class SignInPage < BasePage + def visit_page + visit '/session/new' + self + end + + def displayed? + has_selector?('.rsa-auth-form__title', text: 'Sign In', wait: 5) + end + + def sign_in(email:, password:) + fill_in 'Email', with: email + fill_in 'Password', with: password + click_button 'Sign In' + self + end + + def click_forgot_password + click_link 'Forgot password?' + Pages::PasswordResetPage.new(test_context) + end + + def click_magic_link + click_link 'Sign in with Magic Link' + Pages::MagicLinkPage.new(test_context) + end + + def click_sign_up + click_link 'Sign Up' + Pages::SignUpPage.new(test_context) + end + + def has_invalid_credentials_error? + has_text?('Invalid email or password') + end + + def has_unconfirmed_error? + has_text?('confirm') || has_text?('Confirm') + end + end +end diff --git a/test/generator_test_files/pages/sign_up_page.rb b/test/generator_test_files/pages/sign_up_page.rb new file mode 100644 index 0000000..b511f62 --- /dev/null +++ b/test/generator_test_files/pages/sign_up_page.rb @@ -0,0 +1,34 @@ +# frozen_string_literal: true + +module Pages + class SignUpPage < BasePage + def visit_page + visit '/sign_up' + self + end + + def displayed? + has_selector?('.rsa-auth-form__title', text: 'Sign Up', wait: 5) + end + + def sign_up(email:, password:) + fill_in 'Email', with: email + fill_in 'Password', with: password + click_button 'Sign Up' + self + end + + def click_sign_in + click_link 'Already have an account? Sign In' + Pages::SignInPage.new(test_context) + end + + def has_email_taken_error? + has_text?('has already been taken') || has_text?('Email has already been taken') + end + + def has_password_too_short_error? + has_text?('at least') || has_text?('must be at least') + end + end +end diff --git a/test/generator_test_files/system/authentication_test.rb b/test/generator_test_files/system/authentication_test.rb new file mode 100644 index 0000000..f4e7155 --- /dev/null +++ b/test/generator_test_files/system/authentication_test.rb @@ -0,0 +1,139 @@ +# frozen_string_literal: true + +require 'test_helper' + +class AuthenticationTest < ActionDispatch::IntegrationTest + def setup + super + @landing_page = Pages::LandingPage.new(self) + @sign_in_page = Pages::SignInPage.new(self) + @sign_up_page = Pages::SignUpPage.new(self) + @dashboard_page = Pages::DashboardPage.new(self) + end + + test 'landing page displays correctly' do + @landing_page.visit_page + + assert_predicate @landing_page, :displayed?, 'Landing page should be displayed' + end + + test 'unauthenticated user can navigate from landing to sign in' do + @landing_page.visit_page + sign_in_page = @landing_page.click_sign_in + + assert_predicate sign_in_page, :displayed?, 'Sign in page should be displayed' + end + + test 'unauthenticated user can navigate from landing to sign up' do + @landing_page.visit_page + sign_up_page = @landing_page.click_sign_up + + assert_predicate sign_up_page, :displayed?, 'Sign up page should be displayed' + end + + test 'unauthenticated user accessing dashboard is redirected to sign in' do + @dashboard_page.visit_page + + assert_predicate @sign_in_page, :displayed?, 'User should be redirected to sign in page' + end + + test 'user can sign up with valid credentials' do + email = "test_#{SecureRandom.hex(4)}@example.com" + password = 'password123' + + @sign_up_page.visit_page + @sign_up_page.sign_up(email: email, password: password) + + # After sign up, user should see confirmation message + assert has_text?('Account created') || has_text?('check your email'), + 'User should see confirmation instructions after sign up' + end + + test 'user cannot sign up with existing email' do + email = "existing_#{SecureRandom.hex(4)}@example.com" + password = 'password123' + + # Create user first + User.create!(email: email, password: password, password_confirmation: password, confirmed_at: Time.current) + + @sign_up_page.visit_page + @sign_up_page.sign_up(email: email, password: password) + + assert_predicate @sign_up_page, :has_email_taken_error?, 'Should show email taken error' + end + + test 'user cannot sign up with short password' do + email = "short_pwd_#{SecureRandom.hex(4)}@example.com" + + @sign_up_page.visit_page + @sign_up_page.sign_up(email: email, password: 'short') + + assert_predicate @sign_up_page, :has_password_too_short_error?, 'Should show password too short error' + end + + test 'confirmed user can sign in with valid credentials' do + email = "signin_#{SecureRandom.hex(4)}@example.com" + password = 'password123' + + User.create!(email: email, password: password, password_confirmation: password, confirmed_at: Time.current) + + @sign_in_page.visit_page + @sign_in_page.sign_in(email: email, password: password) + + assert_predicate @dashboard_page, :displayed?, 'User should be redirected to dashboard after sign in' + assert @dashboard_page.has_welcome_message_for?(email), 'Dashboard should show user email' + end + + test 'user cannot sign in with invalid password' do + email = "invalid_pwd_#{SecureRandom.hex(4)}@example.com" + password = 'password123' + + User.create!(email: email, password: password, password_confirmation: password, confirmed_at: Time.current) + + @sign_in_page.visit_page + @sign_in_page.sign_in(email: email, password: 'wrongpassword') + + assert_predicate @sign_in_page, :has_invalid_credentials_error?, 'Should show invalid credentials error' + end + + test 'user cannot sign in with non-existent email' do + @sign_in_page.visit_page + @sign_in_page.sign_in(email: 'nonexistent@example.com', password: 'password123') + + assert_predicate @sign_in_page, :has_invalid_credentials_error?, 'Should show invalid credentials error' + end + + test 'authenticated user can sign out' do + email = "signout_#{SecureRandom.hex(4)}@example.com" + password = 'password123' + + User.create!(email: email, password: password, password_confirmation: password, confirmed_at: Time.current) + + @sign_in_page.visit_page + @sign_in_page.sign_in(email: email, password: password) + + assert_predicate @dashboard_page, :displayed?, 'User should be on dashboard' + + @dashboard_page.sign_out + + # After sign out, user should be on sign in page or landing + assert @sign_in_page.displayed? || @landing_page.displayed?, + 'User should be redirected after sign out' + end + + test 'authenticated user visiting landing page is redirected to dashboard' do + email = "redirect_#{SecureRandom.hex(4)}@example.com" + password = 'password123' + + User.create!(email: email, password: password, password_confirmation: password, confirmed_at: Time.current) + + @sign_in_page.visit_page + @sign_in_page.sign_in(email: email, password: password) + + assert_predicate @dashboard_page, :displayed?, 'User should be on dashboard' + + @landing_page.visit_page + + assert_predicate @dashboard_page, :displayed?, 'Authenticated user should be redirected to dashboard from landing' + end +end diff --git a/test/generator_test_files/system/email_confirmation_test.rb b/test/generator_test_files/system/email_confirmation_test.rb new file mode 100644 index 0000000..183eee2 --- /dev/null +++ b/test/generator_test_files/system/email_confirmation_test.rb @@ -0,0 +1,70 @@ +# frozen_string_literal: true + +require 'test_helper' + +class EmailConfirmationTest < ActionDispatch::IntegrationTest + def setup + super + @sign_in_page = Pages::SignInPage.new(self) + @sign_up_page = Pages::SignUpPage.new(self) + @confirmation_page = Pages::ConfirmationPage.new(self) + @dashboard_page = Pages::DashboardPage.new(self) + end + + test 'unconfirmed user cannot sign in' do + email = "unconfirmed_#{SecureRandom.hex(4)}@example.com" + password = 'password123' + + User.create!(email: email, password: password, password_confirmation: password, confirmed_at: nil) + + @sign_in_page.visit_page + @sign_in_page.sign_in(email: email, password: password) + + # User should either see error or be asked to confirm + assert @sign_in_page.has_unconfirmed_error? || has_text?('confirm') || @sign_in_page.displayed?, + 'Unconfirmed user should not be able to sign in' + end + + test 'user can confirm email with valid token' do + email = "confirm_valid_#{SecureRandom.hex(4)}@example.com" + password = 'password123' + + user = User.create!(email: email, password: password, password_confirmation: password, confirmed_at: nil) + + # Generate a valid confirmation token + token = user.generate_confirmation_token + + @confirmation_page.confirm_with_token(token: token) + + # After confirmation, user should see success or be redirected to sign in + assert @confirmation_page.has_confirmed_message? || @sign_in_page.displayed?, + 'User should see confirmation success or be on sign in page' + + # Verify user can now sign in + @sign_in_page.visit_page + @sign_in_page.sign_in(email: email, password: password) + + assert_predicate @dashboard_page, :displayed?, 'Confirmed user should be able to sign in' + end + + test 'user cannot confirm email with invalid token' do + visit '/confirmations/invalid_token_123' + + # Should show error + assert @confirmation_page.has_invalid_token_error? || has_text?('invalid') || has_text?('expired'), + 'Should show invalid or expired token error' + end + + test 'user can request new confirmation email' do + email = "resend_#{SecureRandom.hex(4)}@example.com" + password = 'password123' + + User.create!(email: email, password: password, password_confirmation: password, confirmed_at: nil) + + @confirmation_page.visit_page + @confirmation_page.request_confirmation(email: email) + + assert_predicate @confirmation_page, :has_confirmation_sent_message?, + 'Should show confirmation email sent message' + end +end diff --git a/test/generator_test_files/system/magic_link_test.rb b/test/generator_test_files/system/magic_link_test.rb new file mode 100644 index 0000000..faf6249 --- /dev/null +++ b/test/generator_test_files/system/magic_link_test.rb @@ -0,0 +1,70 @@ +# frozen_string_literal: true + +require 'test_helper' + +class MagicLinkTest < ActionDispatch::IntegrationTest + def setup + super + @sign_in_page = Pages::SignInPage.new(self) + @magic_link_page = Pages::MagicLinkPage.new(self) + @dashboard_page = Pages::DashboardPage.new(self) + end + + test 'magic link page displays correctly' do + @magic_link_page.visit_page + + assert_predicate @magic_link_page, :displayed?, 'Magic link page should be displayed' + end + + test 'user can navigate to magic link from sign in page' do + @sign_in_page.visit_page + magic_link_page = @sign_in_page.click_magic_link + + assert_predicate magic_link_page, :displayed?, 'Magic link page should be displayed' + end + + test 'user can request magic link for existing email' do + email = "magic_#{SecureRandom.hex(4)}@example.com" + password = 'password123' + + User.create!(email: email, password: password, password_confirmation: password, confirmed_at: Time.current) + + @magic_link_page.visit_page + @magic_link_page.request_magic_link(email: email) + + assert_predicate @magic_link_page, :has_magic_link_sent_message?, + 'Should show magic link sent message' + end + + test 'user can sign in with valid magic link token' do + email = "magic_valid_#{SecureRandom.hex(4)}@example.com" + password = 'password123' + + user = User.create!(email: email, password: password, password_confirmation: password, confirmed_at: Time.current) + + # Generate a valid magic link token + token = user.generate_magic_link_token + + visit "/magic_link?token=#{token}" + + assert_predicate @dashboard_page, :displayed?, 'User should be signed in and on dashboard after using magic link' + assert @dashboard_page.has_welcome_message_for?(email), 'Dashboard should show user email' + end + + test 'user cannot sign in with invalid magic link token' do + visit '/magic_link?token=invalid_token_123' + + # Should show error or be redirected to sign in + assert @sign_in_page.displayed? || has_text?('invalid') || has_text?('expired'), + 'Should show error or redirect to sign in for invalid token' + end + + test 'requesting magic link for non-existent email does not reveal information' do + @magic_link_page.visit_page + @magic_link_page.request_magic_link(email: 'nonexistent@example.com') + + # Should show same message to prevent email enumeration + assert_predicate @magic_link_page, :has_magic_link_sent_message?, + 'Should show magic link sent message even for non-existent email' + end +end diff --git a/test/generator_test_files/system/password_reset_test.rb b/test/generator_test_files/system/password_reset_test.rb new file mode 100644 index 0000000..6e76516 --- /dev/null +++ b/test/generator_test_files/system/password_reset_test.rb @@ -0,0 +1,81 @@ +# frozen_string_literal: true + +require 'test_helper' + +class PasswordResetTest < ActionDispatch::IntegrationTest + def setup + super + @sign_in_page = Pages::SignInPage.new(self) + @password_reset_page = Pages::PasswordResetPage.new(self) + @dashboard_page = Pages::DashboardPage.new(self) + end + + test 'password reset page displays correctly' do + @password_reset_page.visit_page + + assert_predicate @password_reset_page, :displayed?, 'Password reset page should be displayed' + end + + test 'user can navigate to password reset from sign in page' do + @sign_in_page.visit_page + password_reset_page = @sign_in_page.click_forgot_password + + assert_predicate password_reset_page, :displayed?, 'Password reset page should be displayed' + end + + test 'user can request password reset for existing email' do + email = "reset_#{SecureRandom.hex(4)}@example.com" + password = 'password123' + + User.create!(email: email, password: password, password_confirmation: password, confirmed_at: Time.current) + + @password_reset_page.visit_page + @password_reset_page.request_reset(email: email) + + assert_predicate @password_reset_page, :has_reset_email_sent_message?, + 'Should show reset email sent message' + end + + test 'user can request password reset for non-existent email without revealing info' do + @password_reset_page.visit_page + @password_reset_page.request_reset(email: 'nonexistent@example.com') + + # Should show same message to prevent email enumeration + assert_predicate @password_reset_page, :has_reset_email_sent_message?, + 'Should show reset email sent message even for non-existent email' + end + + test 'user can reset password with valid token' do + email = "reset_valid_#{SecureRandom.hex(4)}@example.com" + password = 'password123' + new_password = 'newpassword456' + + user = User.create!(email: email, password: password, password_confirmation: password, confirmed_at: Time.current) + + # Generate a valid password reset token + token = user.generate_password_reset_token + + password_edit_page = Pages::PasswordEditPage.new(self) + password_edit_page.visit_page(token: token) + password_edit_page.reset_password(password: new_password) + + # After successful reset, user should see success message + assert has_text?('Password has been reset') || has_text?('new password'), + 'User should see password reset success message' + + # Reload user to verify password changed + user.reload + + # Verify new password works by authenticating directly (not through browser) + assert user.authenticate(new_password), 'New password should authenticate the user' + end + + test 'user cannot reset password with invalid token' do + password_edit_page = Pages::PasswordEditPage.new(self) + password_edit_page.visit_page(token: 'invalid_token_123') + + # Should show error or be on error page + assert password_edit_page.has_invalid_token_error? || has_text?('invalid') || has_text?('expired'), + 'Should show invalid or expired token error' + end +end From 1ec6924a238ea994a8101c0934d3dcab096e4a3f Mon Sep 17 00:00:00 2001 From: Ivan Kuznetsov Date: Tue, 20 Jan 2026 09:35:11 +0000 Subject: [PATCH 02/10] fix(css): Improve form element alignment and update design tokens MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - Add box-sizing: border-box reset for consistent element sizing - Ensure all form children have consistent 100% width - Change actions alignment from center to stretch - Update color palette to modern indigo tones - Add shadow and typography CSS variables - Add local CI script (bin/ci) for contributors - Update CONTRIBUTING.md with local development instructions ๐Ÿค– Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 --- CONTRIBUTING.md | 30 +++++++-- bin/ci | 37 +++++++++++ .../css/templates/rails_simple_auth.css | 65 ++++++++++++++----- 3 files changed, 112 insertions(+), 20 deletions(-) create mode 100755 bin/ci diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index f7a6642..f27d4f9 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -21,10 +21,32 @@ Thank you for your interest in contributing! 2. Fork the repository 3. Create a feature branch (`git checkout -b feature/my-feature`) 4. Write tests for your changes -5. Ensure all tests pass (`bundle exec rake test`) -6. Run linter (`bundle exec rubocop`) -7. Update CHANGELOG.md under `[Unreleased]` -8. Submit a pull request +5. **Run local CI before submitting:** + ```bash + bin/ci + ``` + This runs RuboCop and all unit tests. +6. Update CHANGELOG.md under `[Unreleased]` +7. Submit a pull request + +### Local Development + +```bash +# Install dependencies +bundle install + +# Run local CI (linter + tests) +bin/ci + +# Run only tests +bundle exec rake test + +# Run only linter +bundle exec rubocop + +# Run generator E2E tests (optional, requires Rails + Playwright) +bin/test_generator +``` ### Code Style diff --git a/bin/ci b/bin/ci new file mode 100755 index 0000000..2e39b1c --- /dev/null +++ b/bin/ci @@ -0,0 +1,37 @@ +#!/usr/bin/env bash +set -e + +# Local CI script for rails_simple_auth +# Run this before submitting a pull request + +SCRIPT_DIR="$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd)" +GEM_DIR="$(dirname "$SCRIPT_DIR")" + +cd "$GEM_DIR" + +echo "=== rails_simple_auth Local CI ===" +echo "" + +# Step 1: Install dependencies +echo "๐Ÿ“ฆ Installing dependencies..." +bundle install --quiet + +# Step 2: Run RuboCop +echo "" +echo "๐Ÿ” Running RuboCop..." +bundle exec rubocop +echo " โœ… RuboCop passed" + +# Step 3: Run unit tests +echo "" +echo "๐Ÿงช Running unit tests..." +bundle exec rake test +echo " โœ… Unit tests passed" + +echo "" +echo "=== All checks passed! โœ… ===" +echo "" +echo "Your code is ready for a pull request." +echo "" +echo "Optional: Run generator E2E tests (requires Rails and Playwright):" +echo " bin/test_generator" diff --git a/lib/generators/rails_simple_auth/css/templates/rails_simple_auth.css b/lib/generators/rails_simple_auth/css/templates/rails_simple_auth.css index 8ef4a3c..eacc417 100644 --- a/lib/generators/rails_simple_auth/css/templates/rails_simple_auth.css +++ b/lib/generators/rails_simple_auth/css/templates/rails_simple_auth.css @@ -11,37 +11,49 @@ * } */ +/* ============================================================================ + Base Reset + ============================================================================ */ + +.rsa-auth-form *, +.rsa-auth-form *::before, +.rsa-auth-form *::after { + box-sizing: border-box; +} + /* ============================================================================ CSS Variables (Override these to customize) ============================================================================ */ :root { /* Primary Colors */ - --rsa-color-primary: #3b82f6; - --rsa-color-primary-hover: #2563eb; + --rsa-color-primary: #4f46e5; + --rsa-color-primary-hover: #4338ca; + --rsa-color-primary-light: #eef2ff; /* Background Colors */ - --rsa-color-background: #ffffff; + --rsa-color-background: #f8fafc; --rsa-color-background-form: #ffffff; - --rsa-color-surface: #f9fafb; - --rsa-color-surface-hover: #f3f4f6; + --rsa-color-surface: #f1f5f9; + --rsa-color-surface-hover: #e2e8f0; /* Text Colors */ - --rsa-color-text: #374151; - --rsa-color-text-muted: #6b7280; - --rsa-color-text-dark: #1f2937; + --rsa-color-text: #475569; + --rsa-color-text-muted: #64748b; + --rsa-color-text-dark: #0f172a; /* Border Colors */ - --rsa-color-border: #e5e7eb; - --rsa-color-border-form: #d1d5db; + --rsa-color-border: #e2e8f0; + --rsa-color-border-form: #cbd5e1; /* Semantic Colors */ --rsa-color-danger: #dc2626; + --rsa-color-danger-hover: #b91c1c; --rsa-color-success: #16a34a; --rsa-color-white: #ffffff; /* Focus Ring */ - --rsa-color-focus-ring: rgba(59, 130, 246, 0.5); + --rsa-color-focus-ring: rgba(79, 70, 229, 0.4); /* OAuth Provider Colors */ --rsa-color-google: #4285f4; @@ -57,15 +69,26 @@ --rsa-space-5: 1.25rem; --rsa-space-6: 1.5rem; --rsa-space-8: 2rem; + --rsa-space-10: 2.5rem; /* Border Radius */ - --rsa-radius-sm: 0.25rem; - --rsa-radius-md: 0.375rem; - --rsa-radius-lg: 0.5rem; - --rsa-radius-xl: 0.75rem; + --rsa-radius-sm: 0.375rem; + --rsa-radius-md: 0.5rem; + --rsa-radius-lg: 0.75rem; + --rsa-radius-xl: 1rem; + --rsa-radius-full: 9999px; + + /* Shadows */ + --rsa-shadow-sm: 0 1px 2px 0 rgb(0 0 0 / 0.05); + --rsa-shadow-md: 0 4px 6px -1px rgb(0 0 0 / 0.1), 0 2px 4px -2px rgb(0 0 0 / 0.1); + --rsa-shadow-lg: 0 10px 15px -3px rgb(0 0 0 / 0.1), 0 4px 6px -4px rgb(0 0 0 / 0.1); + --rsa-shadow-xl: 0 20px 25px -5px rgb(0 0 0 / 0.1), 0 8px 10px -6px rgb(0 0 0 / 0.1); /* Border Width */ --rsa-border-thin: 1px; + + /* Font */ + --rsa-font-family: system-ui, -apple-system, BlinkMacSystemFont, 'Segoe UI', Roboto, 'Helvetica Neue', Arial, sans-serif; } /* ============================================================================ @@ -93,6 +116,10 @@ background-color: var(--rsa-color-background-form); } +.rsa-auth-form > * { + width: 100%; +} + .rsa-auth-form__title { margin-bottom: var(--rsa-space-2); font-size: 1.5rem; @@ -109,6 +136,11 @@ } .rsa-auth-form__form { + display: block; + width: 100%; +} + +.rsa-auth-form__form > * { width: 100%; } @@ -174,8 +206,9 @@ .rsa-auth-form__actions { display: flex; flex-direction: column; - align-items: center; + align-items: stretch; margin-top: var(--rsa-space-6); + width: 100%; } .rsa-auth-form__submit { From a4e6cb981b5b315a3b0b180353e21580903c9668 Mon Sep 17 00:00:00 2001 From: Ivan Kuznetsov Date: Tue, 20 Jan 2026 09:38:49 +0000 Subject: [PATCH 03/10] fix(tests): Remove duplicate Session class definition MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - Configure user_class_name via RailsSimpleAuth.configure instead of monkey-patching the Session class - Eliminates method redefinition warnings during test runs - Add comment about Rails 8.2 deprecation for signed_id_verifier_secret ๐Ÿค– Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 --- test/test_helper.rb | 26 ++++++++------------------ 1 file changed, 8 insertions(+), 18 deletions(-) diff --git a/test/test_helper.rb b/test/test_helper.rb index a0f7fc7..8b5e4ff 100644 --- a/test/test_helper.rb +++ b/test/test_helper.rb @@ -63,9 +63,17 @@ class Application < Rails::Application Rails.application.initialize! # Configure signed IDs for ActiveRecord (required for Rails 8+) +# Note: This method is deprecated in Rails 8.2, but the replacement API +# (ActiveRecord.message_verifiers) requires full Rails app initialization ActiveRecord::Base.signed_id_verifier_secret = Rails.application.secret_key_base require 'rails_simple_auth' + +# Configure RailsSimpleAuth to use User class before anything uses Session +RailsSimpleAuth.configure do |config| + config.user_class_name = 'User' +end + require 'minitest/autorun' # Define User model for testing @@ -88,24 +96,6 @@ def self.find_by_oauth(provider, uid) end end -# Monkey-patch Session for testing to use static class name -module RailsSimpleAuth - class Session < ApplicationRecord - self.table_name = 'sessions' - belongs_to :user, class_name: 'User' - - scope :recent, -> { order(created_at: :desc) } - scope :active, -> { where(created_at: RailsSimpleAuth.configuration.session_expiry.ago..) } - scope :expired, -> { where(created_at: ...RailsSimpleAuth.configuration.session_expiry.ago) } - - def self.cleanup_expired! - count = expired.delete_all - Rails.logger.info("[RailsSimpleAuth] Cleaned up #{count} expired sessions") - count - end - end -end - # Add Rails-style assertion helpers to Minitest module Minitest class Test From 58cc6b9eeefde28e89107c2e9efb3e9faeb8c5d1 Mon Sep 17 00:00:00 2001 From: Ivan Kuznetsov Date: Tue, 20 Jan 2026 09:54:09 +0000 Subject: [PATCH 04/10] fix(ci): Export GEM_PATH for entire test_generator script MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The gem path environment variable was only set during bundle install, causing Rails commands to fail when looking for the gem path. ๐Ÿค– Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 --- bin/test_generator | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/bin/test_generator b/bin/test_generator index 010f474..e7bd9d2 100755 --- a/bin/test_generator +++ b/bin/test_generator @@ -40,8 +40,11 @@ end EOF # Install dependencies +# Export GEM_PATH for the entire script (used by Gemfile) +export GEM_PATH="$GEM_DIR" + echo "Installing dependencies..." -GEM_PATH="$GEM_DIR" bundle install +bundle install # Run the install generator echo "Running rails_simple_auth:install generator..." From 34e991c525e9e7c2a768f3293087d6fbc2d40f93 Mon Sep 17 00:00:00 2001 From: Ivan Kuznetsov Date: Tue, 20 Jan 2026 09:59:42 +0000 Subject: [PATCH 05/10] docs(session): Add note about user_class_name load timing MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Rails 8.1's belongs_to class_name option doesn't support lambdas. Document that users must configure user_class_name before model load. ๐Ÿค– Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 --- app/models/rails_simple_auth/session.rb | 3 +++ 1 file changed, 3 insertions(+) diff --git a/app/models/rails_simple_auth/session.rb b/app/models/rails_simple_auth/session.rb index 206a666..15f0ebb 100644 --- a/app/models/rails_simple_auth/session.rb +++ b/app/models/rails_simple_auth/session.rb @@ -4,6 +4,9 @@ module RailsSimpleAuth class Session < ::ApplicationRecord self.table_name = 'sessions' + # Note: class_name is evaluated at class load time. Users customizing + # user_class_name must configure it before this model loads (e.g., in + # config/application.rb or an early-loading initializer). belongs_to :user, class_name: RailsSimpleAuth.configuration.user_class_name scope :recent, -> { order(created_at: :desc) } From 7640ec13afba15c45b896166303b8d1ff78a2b02 Mon Sep 17 00:00:00 2001 From: Ivan Kuznetsov Date: Tue, 20 Jan 2026 10:04:59 +0000 Subject: [PATCH 06/10] style: Fix RuboCop annotation format (NOTE:) --- app/models/rails_simple_auth/session.rb | 2 +- plans/demo-app.md | 286 ++++++++++++++++++ ...-p1-verify-session-model-lambda-removal.md | 102 +++++++ ...g-p2-remove-duplicate-basepage-creation.md | 70 +++++ ...2-improve-error-handling-test-generator.md | 75 +++++ ...-pending-p2-add-ci-artifacts-on-failure.md | 63 ++++ ...pending-p3-scope-page-object-assertions.md | 85 ++++++ ...6-pending-p3-remove-unused-test-context.md | 88 ++++++ ...7-pending-p3-add-playwright-cache-to-ci.md | 66 ++++ 9 files changed, 836 insertions(+), 1 deletion(-) create mode 100644 plans/demo-app.md create mode 100644 todos/001-pending-p1-verify-session-model-lambda-removal.md create mode 100644 todos/002-pending-p2-remove-duplicate-basepage-creation.md create mode 100644 todos/003-pending-p2-improve-error-handling-test-generator.md create mode 100644 todos/004-pending-p2-add-ci-artifacts-on-failure.md create mode 100644 todos/005-pending-p3-scope-page-object-assertions.md create mode 100644 todos/006-pending-p3-remove-unused-test-context.md create mode 100644 todos/007-pending-p3-add-playwright-cache-to-ci.md diff --git a/app/models/rails_simple_auth/session.rb b/app/models/rails_simple_auth/session.rb index 15f0ebb..080e2ca 100644 --- a/app/models/rails_simple_auth/session.rb +++ b/app/models/rails_simple_auth/session.rb @@ -4,7 +4,7 @@ module RailsSimpleAuth class Session < ::ApplicationRecord self.table_name = 'sessions' - # Note: class_name is evaluated at class load time. Users customizing + # NOTE: class_name is evaluated at class load time. Users customizing # user_class_name must configure it before this model loads (e.g., in # config/application.rb or an early-loading initializer). belongs_to :user, class_name: RailsSimpleAuth.configuration.user_class_name diff --git a/plans/demo-app.md b/plans/demo-app.md new file mode 100644 index 0000000..940498c --- /dev/null +++ b/plans/demo-app.md @@ -0,0 +1,286 @@ +# feat: Minimal Demo App for rails_simple_auth + +## Overview + +Create a minimal Rails demo application in a separate folder that demonstrates the rails_simple_auth gem. The app will maximally rely on the gem's defaults, using generators and default styling without custom functionality. + +## Decision Summary (Keeping It Minimal) + +| Decision | Choice | Rationale | +|----------|--------|-----------| +| Modules | `:confirmable`, `:magic_linkable` | Core auth features, no OAuth complexity | +| OAuth | Disabled | Requires external setup, not minimal | +| Temporary Users | Disabled | Advanced feature, not minimal | +| Database | SQLite | Zero configuration | +| Email | letter_opener | View emails in browser, no SMTP setup | +| Styling | gem CSS generator | Use `.rsa-` BEM classes | +| Tests | None | Demo only, not a test suite | +| Deployment | None | Local development only | + +## Acceptance Criteria + +- [ ] New Rails app created in `../rails_simple_auth_demo/` +- [ ] Uses rails_simple_auth gem via local path +- [ ] Runs `rails_simple_auth:install` generator +- [ ] Runs `rails_simple_auth:css` generator +- [ ] User model with `authenticates_with :confirmable, :magic_linkable` +- [ ] Public landing page with sign in/sign up links +- [ ] Protected dashboard showing current user +- [ ] All auth flows work: sign up, confirm, sign in, sign out, password reset, magic link +- [ ] Emails viewable via letter_opener + +## MVP Implementation + +### Step 1: Create Rails App + +```bash +cd /home/asterio/Dev +rails new rails_simple_auth_demo --skip-test +cd rails_simple_auth_demo +``` + +### Step 2: Update Gemfile + +```ruby +# Gemfile +gem "rails_simple_auth", path: "../rails_simple_auth" +gem "letter_opener", group: :development +``` + +### Step 3: Run Generators + +```bash +bundle install +rails generate rails_simple_auth:install +rails generate rails_simple_auth:css +``` + +### Step 4: Create User Model + +#### db/migrate/xxx_create_users.rb + +```ruby +class CreateUsers < ActiveRecord::Migration[8.0] + def change + create_table :users do |t| + t.string :email, null: false + t.string :password_digest, null: false + t.datetime :confirmed_at + t.string :unconfirmed_email + t.timestamps + end + add_index :users, :email, unique: true + end +end +``` + +#### app/models/user.rb + +```ruby +class User < ApplicationRecord + authenticates_with :confirmable, :magic_linkable +end +``` + +### Step 5: Create Controllers + +#### app/controllers/application_controller.rb + +```ruby +class ApplicationController < ActionController::Base + allow_browser versions: :modern +end +``` + +#### app/controllers/home_controller.rb + +```ruby +class HomeController < ApplicationController + def index + redirect_to dashboard_path if user_signed_in? + end +end +``` + +#### app/controllers/dashboard_controller.rb + +```ruby +class DashboardController < ApplicationController + before_action :require_authentication + + def show + end +end +``` + +### Step 6: Create Views + +#### app/views/home/index.html.erb + +```erb +
+
+

rails_simple_auth Demo

+

+ A minimal authentication gem for Rails +

+
+ <%= link_to "Sign In", new_session_path, class: "rsa-auth-form__submit" %> + <%= link_to "Sign Up", new_registration_path, class: "rsa-auth-form__submit", style: "background: #6b7280;" %> +
+
+
+``` + +#### app/views/dashboard/show.html.erb + +```erb +
+
+

Dashboard

+

+ Welcome, <%= current_user.email %> +

+

+ You are signed in. +

+ <%= button_to "Sign Out", session_path, method: :delete, class: "rsa-auth-form__submit", style: "background: #dc2626;" %> +
+
+``` + +#### app/views/layouts/application.html.erb + +```erb + + + + rails_simple_auth Demo + + <%= csrf_meta_tags %> + <%= csp_meta_tag %> + <%= stylesheet_link_tag "rails_simple_auth", "data-turbo-track": "reload" %> + <%= stylesheet_link_tag "application", "data-turbo-track": "reload" %> + <%= javascript_importmap_tags %> + + + <%= yield %> + + +``` + +### Step 7: Configure Routes + +#### config/routes.rb + +```ruby +Rails.application.routes.draw do + rails_simple_auth_routes + + root "home#index" + get "dashboard", to: "dashboard#show" +end +``` + +### Step 8: Configure Email Delivery + +#### config/environments/development.rb + +Add to the file: + +```ruby +config.action_mailer.delivery_method = :letter_opener +config.action_mailer.perform_deliveries = true +config.action_mailer.default_url_options = { host: "localhost", port: 3000 } +``` + +### Step 9: Run Migrations and Start + +```bash +rails db:migrate +rails server +``` + +## File Structure + +``` +rails_simple_auth_demo/ +โ”œโ”€โ”€ app/ +โ”‚ โ”œโ”€โ”€ controllers/ +โ”‚ โ”‚ โ”œโ”€โ”€ application_controller.rb +โ”‚ โ”‚ โ”œโ”€โ”€ home_controller.rb +โ”‚ โ”‚ โ””โ”€โ”€ dashboard_controller.rb +โ”‚ โ”œโ”€โ”€ models/ +โ”‚ โ”‚ โ””โ”€โ”€ user.rb +โ”‚ โ””โ”€โ”€ views/ +โ”‚ โ”œโ”€โ”€ layouts/ +โ”‚ โ”‚ โ””โ”€โ”€ application.html.erb +โ”‚ โ”œโ”€โ”€ home/ +โ”‚ โ”‚ โ””โ”€โ”€ index.html.erb +โ”‚ โ””โ”€โ”€ dashboard/ +โ”‚ โ””โ”€โ”€ show.html.erb +โ”œโ”€โ”€ config/ +โ”‚ โ”œโ”€โ”€ routes.rb +โ”‚ โ”œโ”€โ”€ environments/ +โ”‚ โ”‚ โ””โ”€โ”€ development.rb (modified) +โ”‚ โ””โ”€โ”€ initializers/ +โ”‚ โ””โ”€โ”€ rails_simple_auth.rb (generated) +โ”œโ”€โ”€ db/ +โ”‚ โ””โ”€โ”€ migrate/ +โ”‚ โ”œโ”€โ”€ xxx_create_users.rb +โ”‚ โ””โ”€โ”€ xxx_add_rails_simple_auth.rb (generated) +โ”œโ”€โ”€ Gemfile +โ””โ”€โ”€ README.md +``` + +## User Flows Supported + +1. **Sign Up**: Visit `/sign_up` โ†’ Enter email/password โ†’ Email confirmation sent โ†’ Click link โ†’ Confirmed +2. **Sign In**: Visit `/session/new` โ†’ Enter credentials โ†’ Redirected to dashboard +3. **Sign Out**: Click "Sign Out" on dashboard โ†’ Redirected to sign in +4. **Password Reset**: Visit `/passwords/new` โ†’ Enter email โ†’ Click email link โ†’ Set new password +5. **Magic Link**: Visit `/magic_link_form` โ†’ Enter email โ†’ Click email link โ†’ Signed in + +## README.md + +```markdown +# rails_simple_auth Demo + +Minimal demo application for the [rails_simple_auth](https://github.com/ivankuznetsov/rails_simple_auth) gem. + +## Setup + +```bash +git clone +cd rails_simple_auth_demo +bundle install +rails db:migrate +rails server +``` + +Visit http://localhost:3000 + +## Features Demonstrated + +- Email/password sign up with confirmation +- Sign in / Sign out +- Password reset via email +- Magic link (passwordless) authentication + +## Email Delivery + +Emails open automatically in your browser via letter_opener. + +## Requirements + +- Ruby 3.3+ +- Rails 8.0+ +``` + +## References + +- rails_simple_auth generators: `lib/generators/rails_simple_auth/` +- CSS generator output: `app/assets/stylesheets/rails_simple_auth.css` +- Default views: `app/views/rails_simple_auth/` +- Routes helper: `lib/rails_simple_auth/routes.rb` +- Model integration: `lib/rails_simple_auth/model.rb:4` diff --git a/todos/001-pending-p1-verify-session-model-lambda-removal.md b/todos/001-pending-p1-verify-session-model-lambda-removal.md new file mode 100644 index 0000000..87faa48 --- /dev/null +++ b/todos/001-pending-p1-verify-session-model-lambda-removal.md @@ -0,0 +1,102 @@ +# Verify Session Model Lambda Removal + +--- +status: pending +priority: p1 +issue_id: "001" +tags: [code-review, rails, architecture, security] +dependencies: [] +--- + +## Problem Statement + +The Session model's `belongs_to :user` association was changed from using a lambda for deferred class resolution to direct evaluation: + +**Before:** +```ruby +belongs_to :user, class_name: -> { RailsSimpleAuth.configuration.user_class_name } +``` + +**After:** +```ruby +belongs_to :user, class_name: RailsSimpleAuth.configuration.user_class_name +``` + +This changes when `user_class_name` is evaluated - from runtime (on each association access) to load time (when the model is first accessed). + +## Findings + +### From Security Sentinel +- Risk Level: MEDIUM +- The lambda was intentionally used to defer class resolution until runtime +- If configuration isn't fully initialized when Session loads, this could cause errors + +### From Kieran Rails Reviewer +- Marked as CRITICAL +- Could break autoloading in development mode +- Original lambda pattern was intentional for deferred class resolution + +### From Architecture Strategist +- Moving to `app/models/` with Rails engine autoloading may eliminate the need for deferred resolution +- The change aligns with how other Rails engines (Devise, ActiveStorage) work + +### From DHH Reviewer +- The removal is correct - Rails engines with `app/models/` get proper autoloading +- This is a legitimate bug fix + +## Proposed Solutions + +### Option A: Keep Current Change (Verify Works) +- **Pros**: Cleaner code, follows Rails engine conventions +- **Cons**: May have edge cases in initialization order +- **Effort**: Small - just needs verification testing +- **Risk**: Low if tests pass + +### Option B: Restore Lambda +- **Pros**: Preserves original behavior, safe fallback +- **Cons**: May be unnecessary with new file location +- **Effort**: Small +- **Risk**: None + +### Option C: Add Test Coverage for Edge Cases +- **Pros**: Validates behavior works correctly +- **Cons**: More test code +- **Effort**: Medium +- **Risk**: None + +## Recommended Action + +Test with various configuration timing scenarios: +1. Configure custom user class name AFTER model loads +2. Configure in initializer (normal case) +3. Test in development with code reloading + +If all tests pass, the current change is acceptable. + +## Technical Details + +**Affected Files:** +- `app/models/rails_simple_auth/session.rb:7` + +**Components:** +- Session model +- User association +- Configuration system + +## Acceptance Criteria + +- [ ] Verify existing gem tests pass (135 tests) +- [ ] Test custom user_class_name configuration +- [ ] Test development mode reloading +- [ ] Confirm no initialization order issues + +## Work Log + +| Date | Action | Learnings | +|------|--------|-----------| +| 2026-01-20 | Created from code review | Multiple reviewers flagged this - needs verification | + +## Resources + +- PR #3: https://github.com/ivankuznetsov/rails_simple_auth/pull/3 +- File: app/models/rails_simple_auth/session.rb diff --git a/todos/002-pending-p2-remove-duplicate-basepage-creation.md b/todos/002-pending-p2-remove-duplicate-basepage-creation.md new file mode 100644 index 0000000..21b7ab4 --- /dev/null +++ b/todos/002-pending-p2-remove-duplicate-basepage-creation.md @@ -0,0 +1,70 @@ +# Remove Duplicate BasePage Creation in bin/test_generator + +--- +status: pending +priority: p2 +issue_id: "002" +tags: [code-review, bash, cleanup] +dependencies: [] +--- + +## Problem Statement + +The `bin/test_generator` script creates a `BasePage` class inline (lines 160-175), then immediately copies all page objects from the gem directory including `base_page.rb` (line 178), which overwrites the inline version. + +This is confusing and the inline version differs from the actual template (lacks `has_flash_notice?`, `has_flash_alert?`, `current_path` methods). + +## Findings + +### From Kieran Rails Reviewer +- The embedded version lacks flash message helpers that the actual BasePage has +- The copy command then overwrites it, which is the correct behavior but the order and intent are confusing + +### From Code Simplicity Reviewer +- Lines 159-175 can be completely removed +- Impact: 17 LOC saved, cleaner script + +## Proposed Solutions + +### Option A: Remove Inline Creation (Recommended) +Remove lines 159-175 entirely. The copy command on line 178 handles everything. + +```bash +# Remove this block: +cat > test/support/pages/base_page.rb << 'EOF' +... +EOF + +# Keep this: +cp "$GEM_DIR/test/generator_test_files/pages/"*.rb test/support/pages/ +``` + +- **Pros**: Cleaner, single source of truth +- **Cons**: None +- **Effort**: Small (delete 17 lines) +- **Risk**: None + +## Recommended Action + +Delete lines 159-175 from `bin/test_generator`. + +## Technical Details + +**Affected Files:** +- `bin/test_generator:159-175` + +## Acceptance Criteria + +- [ ] Remove inline BasePage creation +- [ ] Verify bin/test_generator still works +- [ ] Ensure page objects are correctly copied + +## Work Log + +| Date | Action | Learnings | +|------|--------|-----------| +| 2026-01-20 | Created from code review | Obvious cleanup opportunity | + +## Resources + +- PR #3: https://github.com/ivankuznetsov/rails_simple_auth/pull/3 diff --git a/todos/003-pending-p2-improve-error-handling-test-generator.md b/todos/003-pending-p2-improve-error-handling-test-generator.md new file mode 100644 index 0000000..0dfc9c4 --- /dev/null +++ b/todos/003-pending-p2-improve-error-handling-test-generator.md @@ -0,0 +1,75 @@ +# Improve Error Handling in bin/test_generator + +--- +status: pending +priority: p2 +issue_id: "003" +tags: [code-review, bash, ci] +dependencies: [] +--- + +## Problem Statement + +The `bin/test_generator` script uses `set -e` but has `2>/dev/null || true` patterns that silently ignore failures: + +```bash +cp "$GEM_DIR/test/generator_test_files/pages/"*.rb test/support/pages/ 2>/dev/null || true +cp "$GEM_DIR/test/generator_test_files/system/"*.rb test/system/ 2>/dev/null || true +npx playwright install chromium 2>/dev/null || echo "Playwright browser installation skipped" +``` + +If these files don't exist, tests will fail later with confusing errors. + +## Findings + +### From Kieran Rails Reviewer +- If page objects fail to copy, system tests will error with "uninitialized constant" +- If Playwright fails to install, browser tests will fail mysteriously + +## Proposed Solutions + +### Option A: Add Explicit Checks (Recommended) +```bash +if [ ! -d "$GEM_DIR/test/generator_test_files/pages" ]; then + echo "ERROR: Page object templates not found at $GEM_DIR/test/generator_test_files/pages" + exit 1 +fi +``` + +- **Pros**: Fail fast with clear error messages +- **Cons**: More verbose +- **Effort**: Small +- **Risk**: None + +### Option B: Remove `|| true` Patterns +Let failures surface immediately via `set -e`. + +- **Pros**: Simplest change +- **Cons**: Error messages less clear +- **Effort**: Minimal +- **Risk**: Low + +## Recommended Action + +Remove `|| true` patterns and let the script fail naturally. Optionally add explicit directory checks. + +## Technical Details + +**Affected Files:** +- `bin/test_generator:178-181, 218` + +## Acceptance Criteria + +- [ ] Remove silent error suppression +- [ ] Script fails clearly when templates missing +- [ ] Test generator still works in normal case + +## Work Log + +| Date | Action | Learnings | +|------|--------|-----------| +| 2026-01-20 | Created from code review | Silent failures hide real issues | + +## Resources + +- PR #3: https://github.com/ivankuznetsov/rails_simple_auth/pull/3 diff --git a/todos/004-pending-p2-add-ci-artifacts-on-failure.md b/todos/004-pending-p2-add-ci-artifacts-on-failure.md new file mode 100644 index 0000000..ab42ed4 --- /dev/null +++ b/todos/004-pending-p2-add-ci-artifacts-on-failure.md @@ -0,0 +1,63 @@ +# Add CI Artifacts on Failure + +--- +status: pending +priority: p2 +issue_id: "004" +tags: [code-review, ci, debugging] +dependencies: [] +--- + +## Problem Statement + +The `generator-test` job in CI doesn't collect screenshots or logs on failure. When E2E tests fail in CI, there's no visibility into what happened. + +## Findings + +### From Kieran Rails Reviewer +- When E2E tests fail in CI, you'll have no visibility into what happened +- Screenshots and logs are essential for debugging remote failures + +## Proposed Solutions + +### Option A: Add Artifact Upload (Recommended) +```yaml +- name: Upload test artifacts on failure + if: failure() + uses: actions/upload-artifact@v4 + with: + name: generator-test-artifacts + path: | + /tmp/rails_simple_auth_test_app/tmp/screenshots/ + /tmp/rails_simple_auth_test_app/log/test.log +``` + +- **Pros**: Essential for debugging CI failures +- **Cons**: Slightly longer CI on failures +- **Effort**: Small +- **Risk**: None + +## Recommended Action + +Add artifact upload step to CI workflow. + +## Technical Details + +**Affected Files:** +- `.github/workflows/ci.yml` + +## Acceptance Criteria + +- [ ] Add artifact upload on failure +- [ ] Verify screenshots captured by Playwright +- [ ] Verify test logs included + +## Work Log + +| Date | Action | Learnings | +|------|--------|-----------| +| 2026-01-20 | Created from code review | CI observability is critical | + +## Resources + +- PR #3: https://github.com/ivankuznetsov/rails_simple_auth/pull/3 diff --git a/todos/005-pending-p3-scope-page-object-assertions.md b/todos/005-pending-p3-scope-page-object-assertions.md new file mode 100644 index 0000000..751dbdf --- /dev/null +++ b/todos/005-pending-p3-scope-page-object-assertions.md @@ -0,0 +1,85 @@ +# Scope Page Object Assertions to Elements + +--- +status: pending +priority: p3 +issue_id: "005" +tags: [code-review, testing, quality] +dependencies: [] +--- + +## Problem Statement + +Page Object predicates use loose text matching that could match unrelated content: + +```ruby +def has_unconfirmed_error? + has_text?('confirm') || has_text?('Confirm') +end + +def has_password_too_short_error? + has_text?('at least') || has_text?('must be at least') +end +``` + +These match ANY text on the page containing these substrings. + +## Findings + +### From Kieran Rails Reviewer +- If the page has unrelated content with "at least", this passes incorrectly +- Should scope to error elements + +### From Code Simplicity Reviewer +- Multiple `||` fallbacks suggest uncertainty about the implementation +- Message format is known and consistent + +## Proposed Solutions + +### Option A: Scope to Error Elements (Recommended) +```ruby +def has_password_too_short_error? + has_selector?('.rsa-error, .field_with_errors', text: /at least/i, wait: 5) +end +``` + +- **Pros**: More precise matching +- **Cons**: Requires knowing CSS classes +- **Effort**: Small +- **Risk**: Low + +### Option B: Match Exact Messages +Use the exact error message text from the gem's implementation. + +- **Pros**: Very precise +- **Cons**: Brittle if messages change +- **Effort**: Small +- **Risk**: Low + +## Recommended Action + +Scope assertions to error-related CSS selectors. + +## Technical Details + +**Affected Files:** +- `test/generator_test_files/pages/sign_in_page.rb` +- `test/generator_test_files/pages/sign_up_page.rb` +- `test/generator_test_files/pages/magic_link_page.rb` +- `test/generator_test_files/pages/password_reset_page.rb` + +## Acceptance Criteria + +- [ ] Update predicates to use scoped selectors +- [ ] Tests still pass +- [ ] No false positives from unrelated text + +## Work Log + +| Date | Action | Learnings | +|------|--------|-----------| +| 2026-01-20 | Created from code review | Precise selectors prevent flaky tests | + +## Resources + +- PR #3: https://github.com/ivankuznetsov/rails_simple_auth/pull/3 diff --git a/todos/006-pending-p3-remove-unused-test-context.md b/todos/006-pending-p3-remove-unused-test-context.md new file mode 100644 index 0000000..163b1b8 --- /dev/null +++ b/todos/006-pending-p3-remove-unused-test-context.md @@ -0,0 +1,88 @@ +# Remove Unused test_context from Page Objects + +--- +status: pending +priority: p3 +issue_id: "006" +tags: [code-review, testing, cleanup, yagni] +dependencies: [] +--- + +## Problem Statement + +Every Page Object stores `test_context` but none actually use it: + +```ruby +class BasePage + def initialize(test_context) + @test_context = test_context + end + + private + attr_reader :test_context +end +``` + +The `test_context` is passed to every page constructor but never referenced in any page method. + +## Findings + +### From Kieran Rails Reviewer +- test_context is stored but never used in any page object +- If intended for future use, add a comment; otherwise, remove it + +### From Code Simplicity Reviewer +- YAGNI violation - passed everywhere, used nowhere +- Likely intent: Anticipated needing test context for assertions +- Reality: Capybara::DSL provides everything needed + +## Proposed Solutions + +### Option A: Remove Entirely (Recommended) +```ruby +class BasePage + include Capybara::DSL + include Capybara::Minitest::Assertions +end +``` + +- **Pros**: Cleaner, follows YAGNI +- **Cons**: None +- **Effort**: Small +- **Risk**: None + +### Option B: Keep and Document +Add comment explaining intended future use. + +- **Pros**: Preserves flexibility +- **Cons**: Keeps dead code +- **Effort**: Minimal +- **Risk**: None + +## Recommended Action + +Remove `test_context` from BasePage and all page constructors. + +## Technical Details + +**Affected Files:** +- `test/generator_test_files/pages/base_page.rb` +- All 8 page object files (constructor changes) +- All 4 system test files (setup methods) + +## Acceptance Criteria + +- [ ] Remove test_context from BasePage +- [ ] Update page constructors +- [ ] Update test setup methods +- [ ] Tests still pass + +## Work Log + +| Date | Action | Learnings | +|------|--------|-----------| +| 2026-01-20 | Created from code review | YAGNI - remove unused code | + +## Resources + +- PR #3: https://github.com/ivankuznetsov/rails_simple_auth/pull/3 diff --git a/todos/007-pending-p3-add-playwright-cache-to-ci.md b/todos/007-pending-p3-add-playwright-cache-to-ci.md new file mode 100644 index 0000000..6e79a9f --- /dev/null +++ b/todos/007-pending-p3-add-playwright-cache-to-ci.md @@ -0,0 +1,66 @@ +# Add Playwright Browser Cache to CI + +--- +status: pending +priority: p3 +issue_id: "007" +tags: [code-review, ci, performance] +dependencies: [] +--- + +## Problem Statement + +The `generator-test` job downloads Playwright browsers (~200MB) on every CI run, adding 60-90 seconds to build time. + +## Findings + +### From Performance Oracle +- `npx playwright install --with-deps chromium` takes 60-90 seconds +- Browser downloads could be cached between runs + +### From Architecture Strategist +- Caching would speed up CI runs significantly + +## Proposed Solutions + +### Option A: Add Browser Caching (Recommended) +```yaml +- name: Cache Playwright browsers + uses: actions/cache@v4 + with: + path: ~/.cache/ms-playwright + key: playwright-chromium-${{ runner.os }} + +- name: Install Playwright + run: npx playwright install --with-deps chromium +``` + +- **Pros**: Saves 60-90s per run +- **Cons**: Cache invalidation complexity +- **Effort**: Small +- **Risk**: Low + +## Recommended Action + +Add Playwright browser caching to CI workflow. + +## Technical Details + +**Affected Files:** +- `.github/workflows/ci.yml` + +## Acceptance Criteria + +- [ ] Add cache action for Playwright browsers +- [ ] Verify cache hits on subsequent runs +- [ ] Measure time savings + +## Work Log + +| Date | Action | Learnings | +|------|--------|-----------| +| 2026-01-20 | Created from code review | CI optimization opportunity | + +## Resources + +- PR #3: https://github.com/ivankuznetsov/rails_simple_auth/pull/3 From 27ba1a7b5e99dd7c0bece7ee7d9a411e03959cd7 Mon Sep 17 00:00:00 2001 From: Ivan Kuznetsov Date: Tue, 20 Jan 2026 10:05:29 +0000 Subject: [PATCH 07/10] chore: Add plans/ and todos/ to .gitignore --- .gitignore | 4 + plans/demo-app.md | 286 ------------------ ...-p1-verify-session-model-lambda-removal.md | 102 ------- ...g-p2-remove-duplicate-basepage-creation.md | 70 ----- ...2-improve-error-handling-test-generator.md | 75 ----- ...-pending-p2-add-ci-artifacts-on-failure.md | 63 ---- ...pending-p3-scope-page-object-assertions.md | 85 ------ ...6-pending-p3-remove-unused-test-context.md | 88 ------ ...7-pending-p3-add-playwright-cache-to-ci.md | 66 ---- 9 files changed, 4 insertions(+), 835 deletions(-) delete mode 100644 plans/demo-app.md delete mode 100644 todos/001-pending-p1-verify-session-model-lambda-removal.md delete mode 100644 todos/002-pending-p2-remove-duplicate-basepage-creation.md delete mode 100644 todos/003-pending-p2-improve-error-handling-test-generator.md delete mode 100644 todos/004-pending-p2-add-ci-artifacts-on-failure.md delete mode 100644 todos/005-pending-p3-scope-page-object-assertions.md delete mode 100644 todos/006-pending-p3-remove-unused-test-context.md delete mode 100644 todos/007-pending-p3-add-playwright-cache-to-ci.md diff --git a/.gitignore b/.gitignore index 5f8ce36..6c87ccd 100644 --- a/.gitignore +++ b/.gitignore @@ -12,3 +12,7 @@ Gemfile.lock .playwright-mcp/ log/ + +# Local development files +plans/ +todos/ diff --git a/plans/demo-app.md b/plans/demo-app.md deleted file mode 100644 index 940498c..0000000 --- a/plans/demo-app.md +++ /dev/null @@ -1,286 +0,0 @@ -# feat: Minimal Demo App for rails_simple_auth - -## Overview - -Create a minimal Rails demo application in a separate folder that demonstrates the rails_simple_auth gem. The app will maximally rely on the gem's defaults, using generators and default styling without custom functionality. - -## Decision Summary (Keeping It Minimal) - -| Decision | Choice | Rationale | -|----------|--------|-----------| -| Modules | `:confirmable`, `:magic_linkable` | Core auth features, no OAuth complexity | -| OAuth | Disabled | Requires external setup, not minimal | -| Temporary Users | Disabled | Advanced feature, not minimal | -| Database | SQLite | Zero configuration | -| Email | letter_opener | View emails in browser, no SMTP setup | -| Styling | gem CSS generator | Use `.rsa-` BEM classes | -| Tests | None | Demo only, not a test suite | -| Deployment | None | Local development only | - -## Acceptance Criteria - -- [ ] New Rails app created in `../rails_simple_auth_demo/` -- [ ] Uses rails_simple_auth gem via local path -- [ ] Runs `rails_simple_auth:install` generator -- [ ] Runs `rails_simple_auth:css` generator -- [ ] User model with `authenticates_with :confirmable, :magic_linkable` -- [ ] Public landing page with sign in/sign up links -- [ ] Protected dashboard showing current user -- [ ] All auth flows work: sign up, confirm, sign in, sign out, password reset, magic link -- [ ] Emails viewable via letter_opener - -## MVP Implementation - -### Step 1: Create Rails App - -```bash -cd /home/asterio/Dev -rails new rails_simple_auth_demo --skip-test -cd rails_simple_auth_demo -``` - -### Step 2: Update Gemfile - -```ruby -# Gemfile -gem "rails_simple_auth", path: "../rails_simple_auth" -gem "letter_opener", group: :development -``` - -### Step 3: Run Generators - -```bash -bundle install -rails generate rails_simple_auth:install -rails generate rails_simple_auth:css -``` - -### Step 4: Create User Model - -#### db/migrate/xxx_create_users.rb - -```ruby -class CreateUsers < ActiveRecord::Migration[8.0] - def change - create_table :users do |t| - t.string :email, null: false - t.string :password_digest, null: false - t.datetime :confirmed_at - t.string :unconfirmed_email - t.timestamps - end - add_index :users, :email, unique: true - end -end -``` - -#### app/models/user.rb - -```ruby -class User < ApplicationRecord - authenticates_with :confirmable, :magic_linkable -end -``` - -### Step 5: Create Controllers - -#### app/controllers/application_controller.rb - -```ruby -class ApplicationController < ActionController::Base - allow_browser versions: :modern -end -``` - -#### app/controllers/home_controller.rb - -```ruby -class HomeController < ApplicationController - def index - redirect_to dashboard_path if user_signed_in? - end -end -``` - -#### app/controllers/dashboard_controller.rb - -```ruby -class DashboardController < ApplicationController - before_action :require_authentication - - def show - end -end -``` - -### Step 6: Create Views - -#### app/views/home/index.html.erb - -```erb -
-
-

rails_simple_auth Demo

-

- A minimal authentication gem for Rails -

-
- <%= link_to "Sign In", new_session_path, class: "rsa-auth-form__submit" %> - <%= link_to "Sign Up", new_registration_path, class: "rsa-auth-form__submit", style: "background: #6b7280;" %> -
-
-
-``` - -#### app/views/dashboard/show.html.erb - -```erb -
-
-

Dashboard

-

- Welcome, <%= current_user.email %> -

-

- You are signed in. -

- <%= button_to "Sign Out", session_path, method: :delete, class: "rsa-auth-form__submit", style: "background: #dc2626;" %> -
-
-``` - -#### app/views/layouts/application.html.erb - -```erb - - - - rails_simple_auth Demo - - <%= csrf_meta_tags %> - <%= csp_meta_tag %> - <%= stylesheet_link_tag "rails_simple_auth", "data-turbo-track": "reload" %> - <%= stylesheet_link_tag "application", "data-turbo-track": "reload" %> - <%= javascript_importmap_tags %> - - - <%= yield %> - - -``` - -### Step 7: Configure Routes - -#### config/routes.rb - -```ruby -Rails.application.routes.draw do - rails_simple_auth_routes - - root "home#index" - get "dashboard", to: "dashboard#show" -end -``` - -### Step 8: Configure Email Delivery - -#### config/environments/development.rb - -Add to the file: - -```ruby -config.action_mailer.delivery_method = :letter_opener -config.action_mailer.perform_deliveries = true -config.action_mailer.default_url_options = { host: "localhost", port: 3000 } -``` - -### Step 9: Run Migrations and Start - -```bash -rails db:migrate -rails server -``` - -## File Structure - -``` -rails_simple_auth_demo/ -โ”œโ”€โ”€ app/ -โ”‚ โ”œโ”€โ”€ controllers/ -โ”‚ โ”‚ โ”œโ”€โ”€ application_controller.rb -โ”‚ โ”‚ โ”œโ”€โ”€ home_controller.rb -โ”‚ โ”‚ โ””โ”€โ”€ dashboard_controller.rb -โ”‚ โ”œโ”€โ”€ models/ -โ”‚ โ”‚ โ””โ”€โ”€ user.rb -โ”‚ โ””โ”€โ”€ views/ -โ”‚ โ”œโ”€โ”€ layouts/ -โ”‚ โ”‚ โ””โ”€โ”€ application.html.erb -โ”‚ โ”œโ”€โ”€ home/ -โ”‚ โ”‚ โ””โ”€โ”€ index.html.erb -โ”‚ โ””โ”€โ”€ dashboard/ -โ”‚ โ””โ”€โ”€ show.html.erb -โ”œโ”€โ”€ config/ -โ”‚ โ”œโ”€โ”€ routes.rb -โ”‚ โ”œโ”€โ”€ environments/ -โ”‚ โ”‚ โ””โ”€โ”€ development.rb (modified) -โ”‚ โ””โ”€โ”€ initializers/ -โ”‚ โ””โ”€โ”€ rails_simple_auth.rb (generated) -โ”œโ”€โ”€ db/ -โ”‚ โ””โ”€โ”€ migrate/ -โ”‚ โ”œโ”€โ”€ xxx_create_users.rb -โ”‚ โ””โ”€โ”€ xxx_add_rails_simple_auth.rb (generated) -โ”œโ”€โ”€ Gemfile -โ””โ”€โ”€ README.md -``` - -## User Flows Supported - -1. **Sign Up**: Visit `/sign_up` โ†’ Enter email/password โ†’ Email confirmation sent โ†’ Click link โ†’ Confirmed -2. **Sign In**: Visit `/session/new` โ†’ Enter credentials โ†’ Redirected to dashboard -3. **Sign Out**: Click "Sign Out" on dashboard โ†’ Redirected to sign in -4. **Password Reset**: Visit `/passwords/new` โ†’ Enter email โ†’ Click email link โ†’ Set new password -5. **Magic Link**: Visit `/magic_link_form` โ†’ Enter email โ†’ Click email link โ†’ Signed in - -## README.md - -```markdown -# rails_simple_auth Demo - -Minimal demo application for the [rails_simple_auth](https://github.com/ivankuznetsov/rails_simple_auth) gem. - -## Setup - -```bash -git clone -cd rails_simple_auth_demo -bundle install -rails db:migrate -rails server -``` - -Visit http://localhost:3000 - -## Features Demonstrated - -- Email/password sign up with confirmation -- Sign in / Sign out -- Password reset via email -- Magic link (passwordless) authentication - -## Email Delivery - -Emails open automatically in your browser via letter_opener. - -## Requirements - -- Ruby 3.3+ -- Rails 8.0+ -``` - -## References - -- rails_simple_auth generators: `lib/generators/rails_simple_auth/` -- CSS generator output: `app/assets/stylesheets/rails_simple_auth.css` -- Default views: `app/views/rails_simple_auth/` -- Routes helper: `lib/rails_simple_auth/routes.rb` -- Model integration: `lib/rails_simple_auth/model.rb:4` diff --git a/todos/001-pending-p1-verify-session-model-lambda-removal.md b/todos/001-pending-p1-verify-session-model-lambda-removal.md deleted file mode 100644 index 87faa48..0000000 --- a/todos/001-pending-p1-verify-session-model-lambda-removal.md +++ /dev/null @@ -1,102 +0,0 @@ -# Verify Session Model Lambda Removal - ---- -status: pending -priority: p1 -issue_id: "001" -tags: [code-review, rails, architecture, security] -dependencies: [] ---- - -## Problem Statement - -The Session model's `belongs_to :user` association was changed from using a lambda for deferred class resolution to direct evaluation: - -**Before:** -```ruby -belongs_to :user, class_name: -> { RailsSimpleAuth.configuration.user_class_name } -``` - -**After:** -```ruby -belongs_to :user, class_name: RailsSimpleAuth.configuration.user_class_name -``` - -This changes when `user_class_name` is evaluated - from runtime (on each association access) to load time (when the model is first accessed). - -## Findings - -### From Security Sentinel -- Risk Level: MEDIUM -- The lambda was intentionally used to defer class resolution until runtime -- If configuration isn't fully initialized when Session loads, this could cause errors - -### From Kieran Rails Reviewer -- Marked as CRITICAL -- Could break autoloading in development mode -- Original lambda pattern was intentional for deferred class resolution - -### From Architecture Strategist -- Moving to `app/models/` with Rails engine autoloading may eliminate the need for deferred resolution -- The change aligns with how other Rails engines (Devise, ActiveStorage) work - -### From DHH Reviewer -- The removal is correct - Rails engines with `app/models/` get proper autoloading -- This is a legitimate bug fix - -## Proposed Solutions - -### Option A: Keep Current Change (Verify Works) -- **Pros**: Cleaner code, follows Rails engine conventions -- **Cons**: May have edge cases in initialization order -- **Effort**: Small - just needs verification testing -- **Risk**: Low if tests pass - -### Option B: Restore Lambda -- **Pros**: Preserves original behavior, safe fallback -- **Cons**: May be unnecessary with new file location -- **Effort**: Small -- **Risk**: None - -### Option C: Add Test Coverage for Edge Cases -- **Pros**: Validates behavior works correctly -- **Cons**: More test code -- **Effort**: Medium -- **Risk**: None - -## Recommended Action - -Test with various configuration timing scenarios: -1. Configure custom user class name AFTER model loads -2. Configure in initializer (normal case) -3. Test in development with code reloading - -If all tests pass, the current change is acceptable. - -## Technical Details - -**Affected Files:** -- `app/models/rails_simple_auth/session.rb:7` - -**Components:** -- Session model -- User association -- Configuration system - -## Acceptance Criteria - -- [ ] Verify existing gem tests pass (135 tests) -- [ ] Test custom user_class_name configuration -- [ ] Test development mode reloading -- [ ] Confirm no initialization order issues - -## Work Log - -| Date | Action | Learnings | -|------|--------|-----------| -| 2026-01-20 | Created from code review | Multiple reviewers flagged this - needs verification | - -## Resources - -- PR #3: https://github.com/ivankuznetsov/rails_simple_auth/pull/3 -- File: app/models/rails_simple_auth/session.rb diff --git a/todos/002-pending-p2-remove-duplicate-basepage-creation.md b/todos/002-pending-p2-remove-duplicate-basepage-creation.md deleted file mode 100644 index 21b7ab4..0000000 --- a/todos/002-pending-p2-remove-duplicate-basepage-creation.md +++ /dev/null @@ -1,70 +0,0 @@ -# Remove Duplicate BasePage Creation in bin/test_generator - ---- -status: pending -priority: p2 -issue_id: "002" -tags: [code-review, bash, cleanup] -dependencies: [] ---- - -## Problem Statement - -The `bin/test_generator` script creates a `BasePage` class inline (lines 160-175), then immediately copies all page objects from the gem directory including `base_page.rb` (line 178), which overwrites the inline version. - -This is confusing and the inline version differs from the actual template (lacks `has_flash_notice?`, `has_flash_alert?`, `current_path` methods). - -## Findings - -### From Kieran Rails Reviewer -- The embedded version lacks flash message helpers that the actual BasePage has -- The copy command then overwrites it, which is the correct behavior but the order and intent are confusing - -### From Code Simplicity Reviewer -- Lines 159-175 can be completely removed -- Impact: 17 LOC saved, cleaner script - -## Proposed Solutions - -### Option A: Remove Inline Creation (Recommended) -Remove lines 159-175 entirely. The copy command on line 178 handles everything. - -```bash -# Remove this block: -cat > test/support/pages/base_page.rb << 'EOF' -... -EOF - -# Keep this: -cp "$GEM_DIR/test/generator_test_files/pages/"*.rb test/support/pages/ -``` - -- **Pros**: Cleaner, single source of truth -- **Cons**: None -- **Effort**: Small (delete 17 lines) -- **Risk**: None - -## Recommended Action - -Delete lines 159-175 from `bin/test_generator`. - -## Technical Details - -**Affected Files:** -- `bin/test_generator:159-175` - -## Acceptance Criteria - -- [ ] Remove inline BasePage creation -- [ ] Verify bin/test_generator still works -- [ ] Ensure page objects are correctly copied - -## Work Log - -| Date | Action | Learnings | -|------|--------|-----------| -| 2026-01-20 | Created from code review | Obvious cleanup opportunity | - -## Resources - -- PR #3: https://github.com/ivankuznetsov/rails_simple_auth/pull/3 diff --git a/todos/003-pending-p2-improve-error-handling-test-generator.md b/todos/003-pending-p2-improve-error-handling-test-generator.md deleted file mode 100644 index 0dfc9c4..0000000 --- a/todos/003-pending-p2-improve-error-handling-test-generator.md +++ /dev/null @@ -1,75 +0,0 @@ -# Improve Error Handling in bin/test_generator - ---- -status: pending -priority: p2 -issue_id: "003" -tags: [code-review, bash, ci] -dependencies: [] ---- - -## Problem Statement - -The `bin/test_generator` script uses `set -e` but has `2>/dev/null || true` patterns that silently ignore failures: - -```bash -cp "$GEM_DIR/test/generator_test_files/pages/"*.rb test/support/pages/ 2>/dev/null || true -cp "$GEM_DIR/test/generator_test_files/system/"*.rb test/system/ 2>/dev/null || true -npx playwright install chromium 2>/dev/null || echo "Playwright browser installation skipped" -``` - -If these files don't exist, tests will fail later with confusing errors. - -## Findings - -### From Kieran Rails Reviewer -- If page objects fail to copy, system tests will error with "uninitialized constant" -- If Playwright fails to install, browser tests will fail mysteriously - -## Proposed Solutions - -### Option A: Add Explicit Checks (Recommended) -```bash -if [ ! -d "$GEM_DIR/test/generator_test_files/pages" ]; then - echo "ERROR: Page object templates not found at $GEM_DIR/test/generator_test_files/pages" - exit 1 -fi -``` - -- **Pros**: Fail fast with clear error messages -- **Cons**: More verbose -- **Effort**: Small -- **Risk**: None - -### Option B: Remove `|| true` Patterns -Let failures surface immediately via `set -e`. - -- **Pros**: Simplest change -- **Cons**: Error messages less clear -- **Effort**: Minimal -- **Risk**: Low - -## Recommended Action - -Remove `|| true` patterns and let the script fail naturally. Optionally add explicit directory checks. - -## Technical Details - -**Affected Files:** -- `bin/test_generator:178-181, 218` - -## Acceptance Criteria - -- [ ] Remove silent error suppression -- [ ] Script fails clearly when templates missing -- [ ] Test generator still works in normal case - -## Work Log - -| Date | Action | Learnings | -|------|--------|-----------| -| 2026-01-20 | Created from code review | Silent failures hide real issues | - -## Resources - -- PR #3: https://github.com/ivankuznetsov/rails_simple_auth/pull/3 diff --git a/todos/004-pending-p2-add-ci-artifacts-on-failure.md b/todos/004-pending-p2-add-ci-artifacts-on-failure.md deleted file mode 100644 index ab42ed4..0000000 --- a/todos/004-pending-p2-add-ci-artifacts-on-failure.md +++ /dev/null @@ -1,63 +0,0 @@ -# Add CI Artifacts on Failure - ---- -status: pending -priority: p2 -issue_id: "004" -tags: [code-review, ci, debugging] -dependencies: [] ---- - -## Problem Statement - -The `generator-test` job in CI doesn't collect screenshots or logs on failure. When E2E tests fail in CI, there's no visibility into what happened. - -## Findings - -### From Kieran Rails Reviewer -- When E2E tests fail in CI, you'll have no visibility into what happened -- Screenshots and logs are essential for debugging remote failures - -## Proposed Solutions - -### Option A: Add Artifact Upload (Recommended) -```yaml -- name: Upload test artifacts on failure - if: failure() - uses: actions/upload-artifact@v4 - with: - name: generator-test-artifacts - path: | - /tmp/rails_simple_auth_test_app/tmp/screenshots/ - /tmp/rails_simple_auth_test_app/log/test.log -``` - -- **Pros**: Essential for debugging CI failures -- **Cons**: Slightly longer CI on failures -- **Effort**: Small -- **Risk**: None - -## Recommended Action - -Add artifact upload step to CI workflow. - -## Technical Details - -**Affected Files:** -- `.github/workflows/ci.yml` - -## Acceptance Criteria - -- [ ] Add artifact upload on failure -- [ ] Verify screenshots captured by Playwright -- [ ] Verify test logs included - -## Work Log - -| Date | Action | Learnings | -|------|--------|-----------| -| 2026-01-20 | Created from code review | CI observability is critical | - -## Resources - -- PR #3: https://github.com/ivankuznetsov/rails_simple_auth/pull/3 diff --git a/todos/005-pending-p3-scope-page-object-assertions.md b/todos/005-pending-p3-scope-page-object-assertions.md deleted file mode 100644 index 751dbdf..0000000 --- a/todos/005-pending-p3-scope-page-object-assertions.md +++ /dev/null @@ -1,85 +0,0 @@ -# Scope Page Object Assertions to Elements - ---- -status: pending -priority: p3 -issue_id: "005" -tags: [code-review, testing, quality] -dependencies: [] ---- - -## Problem Statement - -Page Object predicates use loose text matching that could match unrelated content: - -```ruby -def has_unconfirmed_error? - has_text?('confirm') || has_text?('Confirm') -end - -def has_password_too_short_error? - has_text?('at least') || has_text?('must be at least') -end -``` - -These match ANY text on the page containing these substrings. - -## Findings - -### From Kieran Rails Reviewer -- If the page has unrelated content with "at least", this passes incorrectly -- Should scope to error elements - -### From Code Simplicity Reviewer -- Multiple `||` fallbacks suggest uncertainty about the implementation -- Message format is known and consistent - -## Proposed Solutions - -### Option A: Scope to Error Elements (Recommended) -```ruby -def has_password_too_short_error? - has_selector?('.rsa-error, .field_with_errors', text: /at least/i, wait: 5) -end -``` - -- **Pros**: More precise matching -- **Cons**: Requires knowing CSS classes -- **Effort**: Small -- **Risk**: Low - -### Option B: Match Exact Messages -Use the exact error message text from the gem's implementation. - -- **Pros**: Very precise -- **Cons**: Brittle if messages change -- **Effort**: Small -- **Risk**: Low - -## Recommended Action - -Scope assertions to error-related CSS selectors. - -## Technical Details - -**Affected Files:** -- `test/generator_test_files/pages/sign_in_page.rb` -- `test/generator_test_files/pages/sign_up_page.rb` -- `test/generator_test_files/pages/magic_link_page.rb` -- `test/generator_test_files/pages/password_reset_page.rb` - -## Acceptance Criteria - -- [ ] Update predicates to use scoped selectors -- [ ] Tests still pass -- [ ] No false positives from unrelated text - -## Work Log - -| Date | Action | Learnings | -|------|--------|-----------| -| 2026-01-20 | Created from code review | Precise selectors prevent flaky tests | - -## Resources - -- PR #3: https://github.com/ivankuznetsov/rails_simple_auth/pull/3 diff --git a/todos/006-pending-p3-remove-unused-test-context.md b/todos/006-pending-p3-remove-unused-test-context.md deleted file mode 100644 index 163b1b8..0000000 --- a/todos/006-pending-p3-remove-unused-test-context.md +++ /dev/null @@ -1,88 +0,0 @@ -# Remove Unused test_context from Page Objects - ---- -status: pending -priority: p3 -issue_id: "006" -tags: [code-review, testing, cleanup, yagni] -dependencies: [] ---- - -## Problem Statement - -Every Page Object stores `test_context` but none actually use it: - -```ruby -class BasePage - def initialize(test_context) - @test_context = test_context - end - - private - attr_reader :test_context -end -``` - -The `test_context` is passed to every page constructor but never referenced in any page method. - -## Findings - -### From Kieran Rails Reviewer -- test_context is stored but never used in any page object -- If intended for future use, add a comment; otherwise, remove it - -### From Code Simplicity Reviewer -- YAGNI violation - passed everywhere, used nowhere -- Likely intent: Anticipated needing test context for assertions -- Reality: Capybara::DSL provides everything needed - -## Proposed Solutions - -### Option A: Remove Entirely (Recommended) -```ruby -class BasePage - include Capybara::DSL - include Capybara::Minitest::Assertions -end -``` - -- **Pros**: Cleaner, follows YAGNI -- **Cons**: None -- **Effort**: Small -- **Risk**: None - -### Option B: Keep and Document -Add comment explaining intended future use. - -- **Pros**: Preserves flexibility -- **Cons**: Keeps dead code -- **Effort**: Minimal -- **Risk**: None - -## Recommended Action - -Remove `test_context` from BasePage and all page constructors. - -## Technical Details - -**Affected Files:** -- `test/generator_test_files/pages/base_page.rb` -- All 8 page object files (constructor changes) -- All 4 system test files (setup methods) - -## Acceptance Criteria - -- [ ] Remove test_context from BasePage -- [ ] Update page constructors -- [ ] Update test setup methods -- [ ] Tests still pass - -## Work Log - -| Date | Action | Learnings | -|------|--------|-----------| -| 2026-01-20 | Created from code review | YAGNI - remove unused code | - -## Resources - -- PR #3: https://github.com/ivankuznetsov/rails_simple_auth/pull/3 diff --git a/todos/007-pending-p3-add-playwright-cache-to-ci.md b/todos/007-pending-p3-add-playwright-cache-to-ci.md deleted file mode 100644 index 6e79a9f..0000000 --- a/todos/007-pending-p3-add-playwright-cache-to-ci.md +++ /dev/null @@ -1,66 +0,0 @@ -# Add Playwright Browser Cache to CI - ---- -status: pending -priority: p3 -issue_id: "007" -tags: [code-review, ci, performance] -dependencies: [] ---- - -## Problem Statement - -The `generator-test` job downloads Playwright browsers (~200MB) on every CI run, adding 60-90 seconds to build time. - -## Findings - -### From Performance Oracle -- `npx playwright install --with-deps chromium` takes 60-90 seconds -- Browser downloads could be cached between runs - -### From Architecture Strategist -- Caching would speed up CI runs significantly - -## Proposed Solutions - -### Option A: Add Browser Caching (Recommended) -```yaml -- name: Cache Playwright browsers - uses: actions/cache@v4 - with: - path: ~/.cache/ms-playwright - key: playwright-chromium-${{ runner.os }} - -- name: Install Playwright - run: npx playwright install --with-deps chromium -``` - -- **Pros**: Saves 60-90s per run -- **Cons**: Cache invalidation complexity -- **Effort**: Small -- **Risk**: Low - -## Recommended Action - -Add Playwright browser caching to CI workflow. - -## Technical Details - -**Affected Files:** -- `.github/workflows/ci.yml` - -## Acceptance Criteria - -- [ ] Add cache action for Playwright browsers -- [ ] Verify cache hits on subsequent runs -- [ ] Measure time savings - -## Work Log - -| Date | Action | Learnings | -|------|--------|-----------| -| 2026-01-20 | Created from code review | CI optimization opportunity | - -## Resources - -- PR #3: https://github.com/ivankuznetsov/rails_simple_auth/pull/3 From ee1c88cafd66ed675adbb1e79826eb8dafcc1f92 Mon Sep 17 00:00:00 2001 From: Ivan Kuznetsov Date: Tue, 20 Jan 2026 10:09:12 +0000 Subject: [PATCH 08/10] fix(ci): Run CSS generator and test DB migration --- bin/test_generator | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/bin/test_generator b/bin/test_generator index e7bd9d2..40fbe85 100755 --- a/bin/test_generator +++ b/bin/test_generator @@ -58,9 +58,14 @@ class User < ApplicationRecord end EOF -# Run migrations +# Run CSS generator +echo "Running CSS generator..." +bin/rails generate rails_simple_auth:css + +# Run migrations (for both dev and test environments) echo "Running migrations..." bin/rails db:migrate +RAILS_ENV=test bin/rails db:migrate # Create dashboard controller echo "Creating DashboardController..." From cdc9aee849326c30f0e200758c16d049feb1646a Mon Sep 17 00:00:00 2001 From: Ivan Kuznetsov Date: Tue, 20 Jan 2026 10:13:02 +0000 Subject: [PATCH 09/10] fix(ci): Add users table migration before sessions migration MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The generator test was failing because the users table didn't exist. Added a CreateUsers migration before running the install generator to ensure users table exists before sessions (which has a foreign key). ๐Ÿค– Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 --- bin/test_generator | 32 ++++++++++++++++++++++++++++---- 1 file changed, 28 insertions(+), 4 deletions(-) diff --git a/bin/test_generator b/bin/test_generator index 40fbe85..4e24713 100755 --- a/bin/test_generator +++ b/bin/test_generator @@ -46,10 +46,6 @@ export GEM_PATH="$GEM_DIR" echo "Installing dependencies..." bundle install -# Run the install generator -echo "Running rails_simple_auth:install generator..." -bin/rails generate rails_simple_auth:install - # Create User model with authenticates_with echo "Creating User model..." cat > app/models/user.rb << 'EOF' @@ -58,6 +54,34 @@ class User < ApplicationRecord end EOF +# Create users migration BEFORE install generator (users table must exist before sessions) +echo "Creating users migration..." +mkdir -p db/migrate +TIMESTAMP=$(date +%Y%m%d%H%M%S) +cat > "db/migrate/${TIMESTAMP}_create_users.rb" << 'EOF' +class CreateUsers < ActiveRecord::Migration[8.0] + def change + create_table :users do |t| + t.string :email, null: false + t.string :password_digest + t.datetime :confirmed_at + t.string :unconfirmed_email + + t.timestamps + end + + add_index :users, :email, unique: true + end +end +EOF + +# Small delay to ensure sessions migration gets a later timestamp +sleep 1 + +# Run the install generator +echo "Running rails_simple_auth:install generator..." +bin/rails generate rails_simple_auth:install + # Run CSS generator echo "Running CSS generator..." bin/rails generate rails_simple_auth:css From c7629869cb7c4abcb5ae97800f018775a8232340 Mon Sep 17 00:00:00 2001 From: Ivan Kuznetsov Date: Tue, 20 Jan 2026 11:01:55 +0000 Subject: [PATCH 10/10] fix: Rename GEM_PATH to RSA_GEM_PATH to avoid Ruby env conflict MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit GEM_PATH is a standard Ruby/RubyGems environment variable. Using it for a custom purpose could interfere with gem resolution. Renamed to RSA_GEM_PATH to avoid any potential conflicts. ๐Ÿค– Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 --- bin/test_generator | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/bin/test_generator b/bin/test_generator index 4e24713..123bfbb 100755 --- a/bin/test_generator +++ b/bin/test_generator @@ -31,7 +31,7 @@ echo "Adding gems to Gemfile..." cat >> Gemfile << 'EOF' # Authentication gem (local development) -gem "rails_simple_auth", path: ENV.fetch("GEM_PATH", "../rails_simple_auth") +gem "rails_simple_auth", path: ENV.fetch("RSA_GEM_PATH", "../rails_simple_auth") group :test do gem "capybara" @@ -40,8 +40,9 @@ end EOF # Install dependencies -# Export GEM_PATH for the entire script (used by Gemfile) -export GEM_PATH="$GEM_DIR" +# Export RSA_GEM_PATH for the entire script (used by Gemfile) +# NOTE: Using RSA_GEM_PATH instead of GEM_PATH to avoid conflict with Ruby's standard env var +export RSA_GEM_PATH="$GEM_DIR" echo "Installing dependencies..." bundle install