Skip to content

Commit 1ca7694

Browse files
committed
Implement Block Management Interface with Search and Unblock Features
- Added `rails-controller-testing` gem for enhanced controller testing. - Updated `PersonBlocksController` to include search functionality for blocked users and display blocking timestamps. - Created views for managing blocked users, including index and new block forms. - Implemented AJAX responses for blocking and unblocking users with Turbo Streams. - Enhanced policies to manage user permissions for blocking actions. - Added acceptance criteria documentation for the Block Management Interface. - Developed comprehensive controller specs to ensure functionality and edge cases are covered.
1 parent eacc819 commit 1ca7694

File tree

16 files changed

+947
-225
lines changed

16 files changed

+947
-225
lines changed

.github/copilot-instructions.md

Lines changed: 58 additions & 185 deletions
Large diffs are not rendered by default.

AGENTS.md

Lines changed: 105 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -12,32 +12,36 @@ Instructions for GitHub Copilot and other automated contributors working in this
1212

1313
## Setup
1414
- Environment runs a setup script that installs Ruby 3.4.4, Node 20, Postgres + PostGIS, and ES7, then prepares databases.
15+
- **Docker Environment**: All commands requiring database access must use `bin/dc-run` to execute within the containerized environment.
16+
- **Dummy App Commands**: Use `bin/dc-run-dummy` for Rails commands that need the dummy app context (e.g., `bin/dc-run-dummy rails console`, `bin/dc-run-dummy rails db:migrate`).
1517
- Databases:
1618
- development: `community_engine_development`
1719
- test: `community_engine_test`
1820
- Use `DATABASE_URL` to connect (overrides fallback host in `config/database.yml`).
1921

2022
## Commands
21-
- **Tests:** `bin/ci`
22-
(Equivalent: `cd spec/dummy && bundle exec rspec`)
23+
- **Tests:** `bin/dc-run bin/ci`
24+
(Equivalent: `bin/dc-run bash -c "cd spec/dummy && bundle exec rspec"`)
2325
- **Running specific tests:**
24-
- Single spec file: `bundle exec rspec spec/path/to/file_spec.rb`
25-
- Specific line: `bundle exec rspec spec/path/to/file_spec.rb:123`
26-
- Multiple files: `bundle exec rspec spec/file1_spec.rb spec/file2_spec.rb`
27-
- Multiple specific lines: `bundle exec rspec spec/file1_spec.rb:123 spec/file2_spec.rb:456`
26+
- Single spec file: `bin/dc-run bundle exec rspec spec/path/to/file_spec.rb`
27+
- Specific line: `bin/dc-run bundle exec rspec spec/path/to/file_spec.rb:123`
28+
- Multiple files: `bin/dc-run bundle exec rspec spec/file1_spec.rb spec/file2_spec.rb`
29+
- Multiple specific lines: `bin/dc-run bundle exec rspec spec/file1_spec.rb:123 spec/file2_spec.rb:456`
2830
- **Important**: RSpec does NOT support hyphenated line numbers (e.g., `spec/file_spec.rb:123-456` is INVALID)
29-
- **Lint:** `bundle exec rubocop`
30-
- **Security:** `bundle exec brakeman --quiet --no-pager` and `bundle exec bundler-audit --update`
31-
- **Style:** `bin/codex_style_guard`
32-
- **I18n:** `bin/i18n [normalize|check|health|all]` (runs normalize + missing + interpolation checks by default)
31+
- **Rails Console:** `bin/dc-run-dummy rails console` (runs console in the dummy app context)
32+
- **Rails Commands in Dummy App:** `bin/dc-run-dummy rails [command]` for any Rails commands that need the dummy app environment
33+
- **Lint:** `bin/dc-run bundle exec rubocop`
34+
- **Security:** `bin/dc-run bundle exec brakeman --quiet --no-pager` and `bin/dc-run bundle exec bundler-audit --update`
35+
- **Style:** `bin/dc-run bin/codex_style_guard`
36+
- **I18n:** `bin/dc-run bin/i18n [normalize|check|health|all]` (runs normalize + missing + interpolation checks by default)
3337
- **Documentation:**
3438
- **Table of Contents**: [`docs/table_of_contents.md`](docs/table_of_contents.md) - Main documentation index
3539
- **Progress tracking**: `docs/scripts/update_progress.sh` - Update system completion status
3640
- **Diagram rendering**: `bin/render_diagrams` - Generate PNG/SVG from Mermaid sources
3741
- **Validation**: `docs/scripts/validate_documentation_tooling.sh` - Validate doc system integrity
3842

3943
## Security Requirements
40-
- **Run Brakeman before generating code**: `bundle exec brakeman --quiet --no-pager`
44+
- **Run Brakeman before generating code**: `bin/dc-run bundle exec brakeman --quiet --no-pager`
4145
- **Fix high-confidence vulnerabilities immediately** - never ignore security warnings with "High" confidence
4246
- **Review and address medium-confidence warnings** that are security-relevant
4347
- **Safe coding practices when generating code:**
@@ -47,11 +51,11 @@ Instructions for GitHub Copilot and other automated contributors working in this
4751
- Use strong parameters in controllers
4852
- Implement proper authorization checks (Pundit policies)
4953
- **For reflection-based features**: Create concerns with `included_in_models` class methods for safe dynamic class resolution
50-
- **Post-generation security check**: Run `bundle exec brakeman --quiet --no-pager -c UnsafeReflection,SQL,CrossSiteScripting` after major code changes
54+
- **Post-generation security check**: Run `bin/dc-run bundle exec brakeman --quiet --no-pager -c UnsafeReflection,SQL,CrossSiteScripting` after major code changes
5155

5256
## Conventions
5357
- Make incremental changes with passing tests.
54-
- **Security first**: Run `bundle exec brakeman --quiet --no-pager` before committing code changes.
58+
- **Security first**: Run `bin/dc-run bundle exec brakeman --quiet --no-pager` before committing code changes.
5559
- **Test every change**: Generate RSpec tests for all code modifications, including models, controllers, mailers, jobs, and JavaScript.
5660
- **Test coverage requirements**: All new features, bug fixes, and refactors must include comprehensive test coverage.
5761
- Avoid introducing new external services in tests; stub where possible.
@@ -156,14 +160,14 @@ We use the `i18n-tasks` gem to ensure all translation keys are present, normaliz
156160

157161
## Example Commands
158162
```bash
159-
i18n-tasks normalize
160-
i18n-tasks missing
161-
i18n-tasks add-missing
162-
i18n-tasks health
163+
bin/dc-run i18n-tasks normalize
164+
bin/dc-run i18n-tasks missing
165+
bin/dc-run i18n-tasks add-missing
166+
bin/dc-run i18n-tasks health
163167
```
164168

165169
## CI Note
166-
- The i18n GitHub Action installs dev/test gem groups to make `i18n-tasks` available. Locally, you can mirror CI with `bin/i18n`, which sets `BUNDLE_WITH=development:test` automatically.
170+
- The i18n GitHub Action installs dev/test gem groups to make `i18n-tasks` available. Locally, you can mirror CI with `bin/dc-run bin/i18n`, which sets `BUNDLE_WITH=development:test` automatically.
167171

168172
See `.github/instructions/i18n-mobility.instructions.md` for additional translation rules.
169173

@@ -205,6 +209,22 @@ For every implementation plan, create acceptance criteria covering relevant stak
205209
- **Test all layers**: models (validations, associations, methods), controllers (actions, authorization), services, mailers, jobs, and view components.
206210
- **JavaScript/Stimulus testing**: Include feature specs that exercise dynamic behaviors like form interactions and AJAX updates.
207211

212+
## Test Environment Requirements
213+
- **Host Platform Configuration**: All controller, request, and feature tests MUST configure the host platform/community before testing.
214+
- **Use `configure_host_platform`**: Call this helper method in a `before` block for any test that makes HTTP requests or tests authentication/authorization.
215+
- **DeviseSessionHelpers**: Include this module and use authentication helpers like `login('[email protected]', 'password')` for authenticated tests.
216+
- **Platform Setup Pattern**:
217+
```ruby
218+
RSpec.describe BetterTogether::SomeController do
219+
before do
220+
configure_host_platform # Creates host platform with community
221+
login('[email protected]', 'password') # For authenticated tests
222+
end
223+
end
224+
```
225+
- **Required for**: Controller specs, request specs, feature specs, and any integration tests that involve routing or authentication.
226+
- **Locale Parameters**: Engine controller tests require locale parameters (e.g., `params: { locale: I18n.default_locale }`) due to routing constraints.
227+
208228
## Test Coverage Standards
209229
- **Models**: Test validations, associations, scopes, instance methods, class methods, and callbacks.
210230
- **Controllers**: Test all actions, authorization policies, parameter handling, and response formats.
@@ -315,3 +335,70 @@ Each major system must include:
315335
- Security implications and access controls
316336
- API endpoints with request/response examples
317337
- Monitoring tools and troubleshooting procedures
338+
339+
## Testing Architecture Consistency Lessons Learned
340+
341+
### Critical Testing Pattern: Request Specs vs Controller Specs
342+
- **Project Standard**: All tests use request specs (`type: :request`) for consistency with Rails engine routing
343+
- **Exception Handling**: Controller specs (`type: :controller`) require special URL helper configuration in Rails engines
344+
- **Why This Matters**: Request specs handle Rails engine routing automatically through the full HTTP stack, while controller specs test in isolation and need explicit configuration
345+
- **Debugging Indicator**: If you see `default_url_options` errors only in one spec while others pass, check if it's a controller spec in a request spec codebase
346+
347+
### Rails Engine URL Helper Configuration
348+
- **Problem**: Controller specs in Rails engines throw `default_url_options` errors that request specs don't encounter
349+
- **Root Cause**: Engines need special URL helper setup for controller specs but not request specs
350+
- **Solution Patterns**:
351+
```ruby
352+
# For controller spec assertions, use pattern matching instead of path helpers:
353+
expect(response.location).to include('/person_blocks') # Good
354+
expect(response).to redirect_to(person_blocks_path) # Problematic in controller specs
355+
356+
# Ensure consistent route naming throughout:
357+
# Controller: person_blocks_path (not blocks_path)
358+
# Views: <%= link_to "Block", better_together.person_blocks_path %>
359+
# Tests: params path should match controller actions
360+
```
361+
362+
### Route Naming Convention Enforcement
363+
- **Pattern**: Engine routes follow full resource naming: `better_together.resource_name_path`
364+
- **Common Error**: Using shortened path names (`blocks_path`) instead of full names (`person_blocks_path`)
365+
- **Consistency Check**: Views, controllers, and tests must all use the same complete path helper names
366+
- **Verification**: Check all three layers when debugging routing issues
367+
368+
### Factory and Association Dependencies
369+
- **Requirement**: Every Better Together model needs a corresponding FactoryBot factory
370+
- **Naming Convention**: Factory names follow `better_together_model_name` pattern with aliases
371+
- **Association Setup**: Factories must properly handle engine namespace associations
372+
- **Missing Factory Indicator**: Tests failing on association creation often indicate missing factories
373+
374+
### Test Environment Configuration Enforcement
375+
- **Critical Setup**: `configure_host_platform` must be called before any controller/request/feature tests
376+
- **Why Required**: Better Together engine needs host platform setup for authentication and authorization
377+
- **Pattern Recognition**: Tests failing with authentication/authorization errors often need this setup
378+
- **Documentation Reference**: This pattern is well-documented but bears reinforcement
379+
380+
### Architecture Consistency Principles
381+
- **Consistency Is Key**: When one component (PersonBlocksController) differs from project patterns, it requires special handling
382+
- **Pattern Detection**: Single anomalies (one controller spec among many request specs) signal architectural inconsistencies
383+
- **Prevention**: New tests should follow the established pattern (request specs) unless there's a compelling reason for exceptions
384+
- **Documentation**: When exceptions are necessary, document why they exist and how to handle their special requirements
385+
386+
### Testing Strategy Recommendations
387+
- **Default Choice**: Use request specs for new controller tests to maintain consistency
388+
- **Engine Compatibility**: Request specs handle Rails engine complexity automatically
389+
- **Special Cases**: If controller specs are needed, prepare for URL helper configuration complexity
390+
- **Debugging Approach**: When testing errors occur in only one spec, compare its type and setup to working specs
391+
392+
## Docker Environment Usage
393+
- **All database-dependent commands must use `bin/dc-run`**: This includes tests, generators, and any command that connects to PostgreSQL, Redis, or Elasticsearch
394+
- **Dummy app commands use `bin/dc-run-dummy`**: For Rails commands that need the dummy app context (console, migrations specific to dummy app)
395+
- **Examples of commands requiring `bin/dc-run`**:
396+
- Tests: `bin/dc-run bundle exec rspec`
397+
- Generators: `bin/dc-run rails generate model User`
398+
- Brakeman: `bin/dc-run bundle exec brakeman`
399+
- RuboCop: `bin/dc-run bundle exec rubocop`
400+
- **Examples of commands requiring `bin/dc-run-dummy`**:
401+
- Rails console: `bin/dc-run-dummy rails console`
402+
- Dummy app migrations: `bin/dc-run-dummy rails db:migrate`
403+
- Dummy app database operations: `bin/dc-run-dummy rails db:seed`
404+
- **Commands that don't require bin/dc-run**: File operations, documentation generation (unless database access needed), static analysis tools that don't connect to services

Gemfile

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -105,6 +105,8 @@ group :test do
105105
gem 'database_cleaner-active_record'
106106
# # Easy installation and use of chromedriver to run system tests with Chrome
107107
# gem 'webdrivers'
108+
# Rails controller testing for assigns method
109+
gem 'rails-controller-testing'
108110
# RuboCop RSpec for RSpec-specific code analysis
109111
gem 'rubocop-capybara'
110112
gem 'rubocop-factory_bot'

Gemfile.lock

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -562,6 +562,10 @@ GEM
562562
activesupport (= 7.1.5.2)
563563
bundler (>= 1.15.0)
564564
railties (= 7.1.5.2)
565+
rails-controller-testing (1.0.5)
566+
actionpack (>= 5.0.1.rc1)
567+
actionview (>= 5.0.1.rc1)
568+
activesupport (>= 5.0.1.rc1)
565569
rails-dom-testing (2.3.0)
566570
activesupport (>= 5.0.0)
567571
minitest
@@ -846,6 +850,7 @@ DEPENDENCIES
846850
rack-mini-profiler
847851
rack-protection
848852
rails (= 7.1.5.2)
853+
rails-controller-testing
849854
rb-readline
850855
rbtrace
851856
redis (~> 5.4)

app/controllers/better_together/person_blocks_controller.rb

Lines changed: 89 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -2,39 +2,116 @@
22

33
module BetterTogether
44
class PersonBlocksController < ApplicationController # rubocop:todo Style/Documentation
5+
before_action :authenticate_user!
56
before_action :set_person_block, only: :destroy
67
after_action :verify_authorized
78

8-
def index
9+
def index # rubocop:todo Metrics/AbcSize, Metrics/MethodLength
910
authorize PersonBlock
10-
@blocked_people = current_person.blocked_people
11+
12+
# AC-2.11: I can search through my blocked users by name
13+
@blocked_people = helpers.current_person.blocked_people
14+
if params[:search].present?
15+
# Search by translated name using includes and references
16+
# Apply policy scope to ensure only authorized people are searchable
17+
search_term = params[:search].strip
18+
authorized_person_ids = policy_scope(BetterTogether::Person).pluck(:id)
19+
20+
@blocked_people = @blocked_people.where(id: authorized_person_ids)
21+
.includes(:string_translations)
22+
.references(:string_translations)
23+
.where(string_translations: { key: 'name' })
24+
.where('string_translations.value ILIKE ?', "%#{search_term}%")
25+
.distinct
26+
end
27+
28+
# AC-2.12: I can see when I blocked each user (provide person_blocks for timestamp info)
29+
@person_blocks = helpers.current_person.person_blocks.includes(:blocked)
30+
if params[:search].present?
31+
# Filter person_blocks by matching blocked person names
32+
# Apply policy scope to ensure only authorized people are searchable
33+
search_term = params[:search].strip
34+
authorized_person_ids = policy_scope(BetterTogether::Person).pluck(:id)
35+
36+
@person_blocks = @person_blocks.joins(:blocked)
37+
.where(better_together_people: { id: authorized_person_ids })
38+
.includes(blocked: :string_translations)
39+
.references(:string_translations)
40+
.where(string_translations: { key: 'name' })
41+
.where('string_translations.value ILIKE ?', "%#{search_term}%")
42+
.distinct
43+
end
44+
45+
# AC-2.15: I can see how many users I have blocked
46+
@blocked_count = @blocked_people.count
1147
end
1248

13-
def create
14-
@person_block = current_person.person_blocks.new(person_block_params)
15-
authorize @person_block
49+
def new
50+
authorize PersonBlock
51+
@person_block = helpers.current_person.person_blocks.build
52+
end
1653

17-
if @person_block.save
18-
redirect_to blocks_path, notice: t('flash.person_block.blocked')
19-
else
20-
redirect_to blocks_path, alert: @person_block.errors.full_messages.to_sentence
54+
def create # rubocop:todo Metrics/AbcSize, Metrics/MethodLength
55+
# rubocop:todo Layout/IndentationWidth
56+
@person_block = helpers.current_person.person_blocks.new(person_block_params)
57+
# rubocop:enable Layout/IndentationWidth
58+
59+
# AC-2.13: I can block a user by entering their username or email
60+
if params[:person_block][:blocked_identifier].present? # rubocop:todo Layout/IndentationConsistency
61+
target_person = BetterTogether::Person.find_by(identifier: params[:person_block][:blocked_identifier]) ||
62+
# rubocop:todo Layout/LineLength
63+
BetterTogether::Person.joins(:user).find_by(better_together_users: { email: params[:person_block][:blocked_identifier] })
64+
# rubocop:enable Layout/LineLength
65+
@person_block.blocked = target_person if target_person
66+
end
67+
68+
authorize @person_block # rubocop:todo Layout/IndentationConsistency
69+
70+
respond_to do |format| # rubocop:todo Layout/IndentationConsistency
71+
if @person_block.save
72+
# AC-2.9: I receive confirmation when blocking/unblocking users
73+
flash[:notice] = t('better_together.person_blocks.notices.blocked')
74+
format.html { redirect_to person_blocks_path }
75+
format.turbo_stream do
76+
render turbo_stream: turbo_stream.replace('blocked_people_list', partial: 'index_content')
77+
end
78+
else
79+
flash[:alert] = @person_block.errors.full_messages.to_sentence
80+
format.html { redirect_to person_blocks_path }
81+
format.turbo_stream do
82+
render turbo_stream: turbo_stream.replace('flash_messages', partial: 'shared/flash_messages')
2183
end
2284
end
85+
end
86+
end
2387

2488
def destroy
2589
authorize @person_block
2690
@person_block.destroy
27-
redirect_to blocks_path, notice: t('flash.person_block.unblocked')
91+
92+
respond_to do |format|
93+
# AC-2.9: I receive confirmation when blocking/unblocking users
94+
flash[:notice] = t('better_together.person_blocks.notices.unblocked')
95+
format.html { redirect_to person_blocks_path }
96+
format.turbo_stream do
97+
render turbo_stream: turbo_stream.replace('blocked_people_list', partial: 'index_content')
98+
end
99+
end
28100
end
29101

30102
private
31103

32-
def current_person
33-
current_user.person
104+
def locale
105+
params[:locale] || I18n.default_locale
34106
end
35107

36108
def set_person_block
109+
current_person = helpers.current_person
110+
return render_not_found unless current_person
111+
37112
@person_block = current_person.person_blocks.find(params[:id])
113+
rescue ActiveRecord::RecordNotFound
114+
render_not_found
38115
end
39116

40117
def person_block_params

app/policies/better_together/person_block_policy.rb

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -6,6 +6,10 @@ def index?
66
user.present?
77
end
88

9+
def new?
10+
user.present?
11+
end
12+
913
def create?
1014
user.present? && record.blocker == agent && !record.blocked.permitted_to?('manage_platform')
1115
end

0 commit comments

Comments
 (0)