-
-
Notifications
You must be signed in to change notification settings - Fork 198
feat: migrate controllers to Rails 8.0 params.expect #2476
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
Problem: - member_params was called 6+ times per request (inefficient) - Validation was checking original params while attrs were modified - Confusing control flow made code hard to maintain Solution: - Call member_params once and reuse the result - Pass attrs to validation method instead of re-calling member_params - Validate BEFORE modifying attributes for clearer logic flow Performance: Reduced member_params calls from 6 to 1 per request Test: Added test to verify member_params is called only once
- Replace raw params access with params.permit - Add nested data parameter filtering for Stripe tokens - Create test coverage for parameter filtering - Add minimal view template for create action Security improvement: prevents mass assignment of unpermitted fields
- Convert member_params to params.expect with nested array syntax - Remove direct params[:member] access (was security issue) - Refactor how_you_found_us validation to accept params hash - Fix validation order: validate before clearing other_reason - Call member_params only once (performance improvement) Security fix: Previously mixed permitted params with raw params access, allowing potential bypass of strong parameters. Now all access goes through params.expect. Bug fix: Validation now properly checks before modifying attributes.
- Convert index action to use conditional params.expect for optional member_search - Convert results action to use params.expect with nested array syntax - Add test coverage for results action - Handles optional parameters gracefully with conditional check
Converted controllers: - workshops_controller: array params, host_id helper method - events_controller: multiple nested arrays for sponsor tiers - sponsors_controller: nested attributes (address, contacts) - announcements_controller: group_ids array - chapters_controller: basic fields - groups_controller: basic fields - meetings_controller: multiple fields - bans_controller: ban params - member_notes_controller: note params All controllers now use params.expect syntax instead of require().permit(). All existing tests pass with no regressions.
4ac0981 to
d084942
Compare
- Document Rails 8.0 params.expect usage - Provide examples for basic, nested, and conditional patterns - Explain type safety and 400 error behavior - Document nested attributes syntax - Add testing guidelines - Update Rails version from 7.2 to 8.1 This helps future developers and AI assistants understand the parameter handling pattern used throughout the codebase.
d084942 to
0ec61cd
Compare
olleolleolle
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Neat!
|
|
||
| def create | ||
| @amount = params[:amount] | ||
| payment_params = params.permit(:amount, :name, data: [:email, :id]) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since we are relying on :data to be present, perhaps we can tighten requirements here, and get it to use expect?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The JavaScript (payments.js:12) sends root-level parameters: { amount: ..., name: ..., data: token } without a :payment wrapper. Because of this, we need to use params.permit (not params.expect) for root-level params.
While :data is required in practice, the code would crash with NoMethodError on line 12-14 if it's missing, making the nil case self-documenting. Adding explicit validation here wouldn't improve the error (still 500), so the current approach is appropriate for this legacy Stripe integration.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated implementation in c681ebe - now wraps parameters in :payment hash with flattened Stripe fields (stripe_email, stripe_token_id) so payments_controller can use params.expect consistently with all other controllers.
| ## Project Overview | ||
|
|
||
| codebar planner is a Rails 7.2 application for managing [codebar.io](https://codebar.io) members and events. It handles workshop/event scheduling, member registration, invitations, RSVPs, and feedback collection for coding workshops organized by codebar chapters. | ||
| codebar planner is a Rails 8.1 application for managing [codebar.io](https://codebar.io) members and events. It handles workshop/event scheduling, member registration, invitations, RSVPs, and feedback collection for coding workshops organized by codebar chapters. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice!
- Explains why sponsors_controller uses require().permit() instead of params.expect (incompatible with nested attributes) - Removes performance test referring to old implementation details
- Wrap AJAX parameters in `payment` hash for params.expect compatibility - Flatten Stripe token fields (stripe_email, stripe_token_id) instead of nested `data` hash for clearer parameter names - Update controller to use params.expect with flat structure - Add type tampering test to verify 400 Bad Request behavior - Remove root-level parameters exception from CLAUDE.md Addresses review comment on codebar#2476 about payments_controller still using params.permit. Now all controllers use params.expect consistently.
This reverts commit 3fa4d66.
params.expect raises ActionController::ParameterMissing exception instead of returning 400 status. Updated documentation to reflect actual behavior.
Summary
Migrates codebar planner controllers to use Rails 8.0
params.expectfor improved type safety and cleaner parameter handling syntax.Motivation
Rails 8.0 introduces
params.expectas a replacement forparams.require().permit()with built-in type tampering protection. This provides more explicit parameter validation and clearer error handling.Changes
Controllers Converted (13 total)
Core Controllers:
payments_controller- Migrated to params.expect with flattened Stripe token parametersmember/details_controller- Migrated to params.expect with nested array syntaxadmin/member_search_controller- Conditional params.expect for optional parametersAdmin Controllers (10):
workshops_controller- Array params, helper methodsevents_controller- Multiple nested sponsor arrayssponsors_controller- Usesrequire().permit()(nested attributes incompatibility documented)announcements_controller- Group IDs arraychapters_controller- Basic fieldsgroups_controller- Basic fieldsmeetings_controller- Multiple fieldsbans_controller- Ban parametersmember_notes_controller- Note parametersJavaScript Changes
payments.js - Refactored to wrap parameters in
payment:hash with flattened Stripe fields:{ amount: ..., name: ..., data: token }{ payment: { amount: ..., name: ..., stripe_email: token.email, stripe_token_id: token.id } }Documentation
Added comprehensive "Parameter Handling" section to CLAUDE.md covering:
accepts_nested_attributes_for(requiresrequire().permit())Built On
This PR is built on top of #2475 (validation fix) which should be merged first.
Benefits
Security:
datahash)Code Quality:
Maintainability:
Testing
Syntax Examples
Before:
After:
Payments (flattened):
Nested Attributes Exception:
Conditional:
Notes
sponsors_controllerusesrequire().permit()because Sponsor model usesaccepts_nested_attributes_for(incompatible with params.expect):id,:chapter_id) continue usingparams.permitwhere appropriate