-
Notifications
You must be signed in to change notification settings - Fork 7
Extract FeaturedAuthor management into dedicated controllers #957
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
base: master
Are you sure you want to change the base?
Conversation
Co-authored-by: abartov <[email protected]>
Co-authored-by: abartov <[email protected]>
Co-authored-by: abartov <[email protected]>
Co-authored-by: abartov <[email protected]>
| private | ||
|
|
||
| def featured_author_feature_params | ||
| params.require(:featured_author_feature).permit(:fromdate, :todate) |
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.
Rails/StrongParametersExpect: Use expect(featured_author_feature: [:fromdate, :todate]) instead.
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
Extracts FeaturedAuthor management out of AdminController into dedicated admin controllers, aligning the implementation with the existing “FeaturedContent” pattern and switching to RESTful routes/views.
Changes:
- Added
Admin::FeaturedAuthorsController(CRUD) andAdmin::FeaturedAuthorFeaturesController(feature period management) with new controller specs. - Replaced legacy custom routes with RESTful
resourcesroutes and updated admin navigation accordingly. - Added model validations and i18n keys to support failure messaging for feature period creation.
Reviewed changes
Copilot reviewed 17 out of 17 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| spec/controllers/admin_controller_spec.rb | Removes legacy FeaturedAuthor specs tied to AdminController actions. |
| spec/controllers/admin/featured_authors_controller_spec.rb | Adds controller specs for FeaturedAuthor CRUD. |
| spec/controllers/admin/featured_author_features_controller_spec.rb | Adds controller specs for creating/deleting feature periods. |
| config/routes.rb | Adds RESTful admin routes for featured authors/features and removes legacy custom routes. |
| config/locales/he.yml | Adds i18n key for featured-author feature creation failure. |
| config/locales/en.yml | Adds i18n key for featured-author feature creation failure. |
| app/views/admin/index.html.haml | Updates admin dashboard link to the new RESTful FeaturedAuthors index. |
| app/views/admin/featured_authors/show.html.haml | New admin show page for FeaturedAuthor + feature period management UI. |
| app/views/admin/featured_authors/index.html.haml | New admin index for FeaturedAuthors using RESTful path helpers. |
| app/views/admin/featured_authors/new.html.haml | New admin “new featured author” page using shared form partial. |
| app/views/admin/featured_authors/edit.html.haml | New admin “edit featured author” page using shared form partial. |
| app/views/admin/featured_authors/_form.html.haml | New shared form partial (title/body + linked author autocomplete). |
| app/models/featured_author.rb | Makes person optional and adds presence validations for title/body. |
| app/models/featured_author_feature.rb | Adds presence validations for fromdate/todate. |
| app/controllers/admin_controller.rb | Removes legacy FeaturedAuthor actions and strong params helper. |
| app/controllers/admin/featured_authors_controller.rb | New RESTful controller for FeaturedAuthor CRUD. |
| app/controllers/admin/featured_author_features_controller.rb | New controller for creating/destroying FeaturedAuthorFeature records. |
| def create | ||
| @fc = FeaturedAuthor.new(fa_params) | ||
| @fc.person_id = params[:person_id] | ||
| @fc.user = current_user |
Copilot
AI
Jan 29, 2026
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.
person_id is being passed as a top-level param and manually assigned in create, but update never applies params[:person_id]. As a result, changing the linked author in the edit form won’t persist (the UI posts person_id, but the controller ignores it). Consider accepting person_id via strong params (e.g., include it in fa_params) and updating both create and update to use it consistently.
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.
Fixed in commit 185d2e4. Added person_id to strong params (fa_params) so both create and update actions now handle the linked author consistently.
| resources :featured_authors do | ||
| resources :features, controller: 'featured_author_features', only: %i(create) | ||
| end | ||
| resources :featured_author_features, only: %i(destroy) |
Copilot
AI
Jan 29, 2026
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.
PR description says the old app/views/admin/featured_author_* templates were moved to app/views/admin/featured_authors/, but the legacy templates still exist in the repo and reference the removed admin#featured_author_* routes. Consider deleting the unused templates (and any related partials) to avoid dead code and future confusion.
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.
Fixed in commit 185d2e4. Deleted all old legacy templates:
app/views/admin/featured_author_edit.html.hamlapp/views/admin/featured_author_list.html.hamlapp/views/admin/featured_author_new.html.hamlapp/views/admin/featured_author_show.html.hamlapp/views/admin/_featured_author_form.html.haml
Co-authored-by: Copilot <[email protected]>
|
@copilot -- address the code review comments. |
- Include person_id in strong params for both create and update actions - Update test to pass person_id inside featured_author params - Delete old legacy view templates that are no longer used Co-authored-by: abartov <[email protected]>
Extract FeaturedAuthor Management to Separate Controllers
This PR extracts FeaturedAuthor management code from AdminController into dedicated controllers, following the same pattern already established for FeaturedContent.
Summary of Changes
Created new controllers:
Admin::FeaturedAuthorsController- handles CRUD operations for FeaturedAuthor recordsAdmin::FeaturedAuthorFeaturesController- manages feature date rangesUpdated routes:
Created new views:
app/views/admin/featured_author_*toapp/views/admin/featured_authors/Added model validations:
FeaturedAuthornow validates presence of title and bodyFeaturedAuthorFeaturevalidates presence of fromdate and todatepersonassociation optional to handle legacy dataCreated comprehensive tests:
Removed legacy code:
Code Review Improvements
destroy!todestroyfor graceful error handlingpersonassociation optional to handle legacy dataPlan
Admin::FeaturedAuthorsControllerwith CRUD actionsAdmin::FeaturedAuthorFeaturesControllerfor feature managementapp/views/admin/featured_authors/Original prompt
💡 You can make Copilot smarter by setting up custom instructions, customizing its development environment and configuring Model Context Protocol (MCP) servers. Learn more Copilot coding agent tips in the docs.