-
Notifications
You must be signed in to change notification settings - Fork 38
Code review fixes for recertification feature #1204
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
Co-authored-by: Copilot <[email protected]>
Co-authored-by: Copilot <[email protected]>
Co-authored-by: Copilot <[email protected]>
Implements post-deadline recertification grace period with automatic blocking: - Users get one additional recertification attempt after Oct 2, 2025 deadline - After using grace attempt, users are permanently blocked from recerts - Ship certifiers can manually block/unblock users from recertifications - Added comprehensive logging for all blocked recertification attempts - Created production-safe concurrent indexes for fraud reports and ship certifications Technical improvements: - Moved hardcoded deadline to configurable initializer (ENV: RECERTIFICATION_DEADLINE) - Scoped admin authorization: ship certifiers limited to recert blocking actions only - Added database indexes with `algorithm: :concurrently` for zero-downtime deployment - Added comprehensive test coverage for recertification blocking scenarios
- Added Rails.logger calls for project/devlog/ship creation blocks - Added Flipper.exist? checks before enabled? for graceful fallback
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.
Pull Request Overview
This PR implements a comprehensive recertification blocking feature with deadline-based restrictions, admin controls, and improved error handling.
- Added recertification blocking system with post-deadline grace period logic
- Implemented admin controls for blocking/unblocking users from recertification requests
- Added comprehensive test coverage and production-safe database indexes
Reviewed Changes
Copilot reviewed 21 out of 22 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| test/models/user_test.rb | Added test for recertification blocking functionality |
| test/models/project_test.rb | Added tests for project recertification permission checks |
| test/controllers/projects/recertifications_controller_test.rb | Comprehensive test suite for recertification controller with deadline scenarios |
| db/migrate/20251002233210_add_indexes_to_fraud_reports_and_ship_certifications.rb | Production-safe concurrent indexes for performance optimization |
| config/routes.rb | Added admin routes for blocking/unblocking recertification |
| config/initializers/ship_certifications.rb | Configurable recertification deadline via environment variable |
| app/views/public_activity/user/_unblock_recertification.html.erb | Activity feed template for unblocking action |
| app/views/public_activity/user/_block_recertification.html.erb | Activity feed template for blocking action |
| app/views/projects/_project_card.html.erb | UI changes to show blocking message and conditional recertification form |
| app/views/admin/users/show.html.erb | Admin interface for viewing and managing recertification blocking status |
| app/views/admin/ship_certifications/edit.html.erb | Added block/unblock buttons to ship certification edit page |
| app/views/admin/ship_certifications/_form.html.erb | Visual indicator for blocked users in certification form |
| app/policies/project_policy.rb | Updated policy to check recertification blocking status |
| app/models/user.rb | Added recertification_blocked? method |
| app/models/project.rb | Updated can_request_recertification? to check user blocking status |
| app/controllers/projects_controller.rb | Added logging and safer feature flag checking |
| app/controllers/projects/ships_controller.rb | Added logging and safer feature flag checking |
| app/controllers/projects/recertifications_controller.rb | Implemented post-deadline recertification logic with grace period |
| app/controllers/devlogs_controller.rb | Added logging and safer feature flag checking |
| app/controllers/admin/users_controller.rb | Added block/unblock recertification actions with scoped permissions |
| app/controllers/admin/ship_certifications_controller.rb | Fixed potential nil reference bug |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| nil | ||
| end | ||
| elsif !(current_user&.is_admin? || current_user&.fraud_team_member?) | ||
| redirect_to root_path, alert: "whomp whomp" |
Copilot
AI
Oct 3, 2025
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 authorization logic should use return after redirect to prevent further execution, or restructure as a positive condition with early return for clarity.
| nil | |
| end | |
| elsif !(current_user&.is_admin? || current_user&.fraud_team_member?) | |
| redirect_to root_path, alert: "whomp whomp" | |
| return | |
| end | |
| elsif !(current_user&.is_admin? || current_user&.fraud_team_member?) | |
| redirect_to root_path, alert: "whomp whomp" | |
| return |
Co-authored-by: Copilot <[email protected]>
Co-authored-by: Copilot <[email protected]>
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.
Pull Request Overview
Copilot reviewed 22 out of 23 changed files in this pull request and generated 3 comments.
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
db/migrate/20251002233210_add_indexes_to_fraud_reports_and_ship_certifications.rb
Show resolved
Hide resolved
db/migrate/20251002233210_add_indexes_to_fraud_reports_and_ship_certifications.rb
Show resolved
Hide resolved
|
:heavysob: |
Summary
Code review improvements for the recertification blocking feature:
<)RECERTIFICATION_DEADLINE)Flipper.exist?checksDatabase Migration Safety
Migration uses
algorithm: :concurrentlywithdisable_ddl_transaction!: