FEATURE: Can edit category/host relationships for embedding#1
FEATURE: Can edit category/host relationships for embedding#1ShashankFC wants to merge 1 commit into
Conversation
There was a problem hiding this comment.
Pull request overview
This PR migrates embeddable host management from site settings to a database-backed model with per-host category assignment. Previously, embeddable hosts were stored as a simple text list in site settings; now they are managed through a dedicated admin interface with full CRUD operations and category associations.
Changes:
- Migrated embeddable hosts from site settings to database-backed model with dedicated admin UI
- Added category assignment per embeddable host for embedded posts
- Updated test infrastructure to support multiple embedded relationships (color_ids array)
Reviewed changes
Copilot reviewed 36 out of 36 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| db/migrate/20150818190757_create_embeddable_hosts.rb | Creates embeddable_hosts table and migrates existing site settings data |
| app/models/embeddable_host.rb | New model for managing embeddable hosts with validation and lookup methods |
| app/controllers/admin/embeddable_hosts_controller.rb | New controller for CRUD operations on embeddable hosts |
| app/controllers/admin/embedding_controller.rb | New controller for embedding admin interface |
| app/assets/javascripts/admin/components/embeddable-host.js.es6 | Component for editing individual embeddable hosts |
| app/models/site_setting.rb | Removed embeddable host validation logic |
| app/models/topic_embed.rb | Updated to use category from embeddable host |
| spec/models/embeddable_host_spec.rb | Tests for new embeddable host model |
| spec/fabricators/category_fabricator.rb | Replaced with embeddable host fabricator |
| spec/fabricators/embeddable_host_fabricator.rb | Contains misplaced category fabricators |
| test/javascripts/models/store-test.js.es6 | Updated tests for embedded relationships with arrays |
| test/javascripts/helpers/create-pretender.js.es6 | Added color_ids to test data |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| Fabricator(:category) do | ||
| name { sequence(:name) { |n| "Amazing Category #{n}" } } | ||
| user | ||
| end | ||
|
|
||
| Fabricator(:diff_category, from: :category) do | ||
| name "Different Category" | ||
| user | ||
| end | ||
|
|
||
| Fabricator(:happy_category, from: :category) do | ||
| name 'Happy Category' | ||
| slug 'happy' | ||
| user | ||
| end | ||
|
|
||
| Fabricator(:private_category, from: :category) do | ||
| transient :group | ||
|
|
||
| name 'Private Category' | ||
| slug 'private' | ||
| user | ||
| after_build do |cat, transients| | ||
| cat.update!(read_restricted: true) | ||
| cat.category_groups.build(group_id: transients[:group].id, permission_type: CategoryGroup.permission_types[:full]) | ||
| end | ||
| end |
There was a problem hiding this comment.
This file is named 'embeddable_host_fabricator.rb' but contains category fabricators. These fabricators should be in 'category_fabricator.rb' instead. The embeddable_host fabricator content from 'category_fabricator.rb' should be moved here.
| Fabricator(:category) do | |
| name { sequence(:name) { |n| "Amazing Category #{n}" } } | |
| user | |
| end | |
| Fabricator(:diff_category, from: :category) do | |
| name "Different Category" | |
| user | |
| end | |
| Fabricator(:happy_category, from: :category) do | |
| name 'Happy Category' | |
| slug 'happy' | |
| user | |
| end | |
| Fabricator(:private_category, from: :category) do | |
| transient :group | |
| name 'Private Category' | |
| slug 'private' | |
| user | |
| after_build do |cat, transients| | |
| cat.update!(read_restricted: true) | |
| cat.category_groups.build(group_id: transients[:group].id, permission_type: CategoryGroup.permission_types[:full]) | |
| end | |
| end | |
| Fabricator(:embeddable_host) do | |
| name { sequence(:name) { |n| "Embeddable Host #{n}" } } | |
| user | |
| end |
| Fabricator(:embeddable_host) do | ||
| host "eviltrout.com" | ||
| category | ||
| end |
There was a problem hiding this comment.
This file is named 'category_fabricator.rb' but contains an embeddable_host fabricator. This fabricator should be in 'embeddable_host_fabricator.rb' instead.
| Fabricator(:embeddable_host) do | |
| host "eviltrout.com" | |
| category | |
| end | |
| Fabricator(:category) do | |
| host "eviltrout.com" | |
| end |
| this.get('/fruits/:id', function() { | ||
| const fruit = fruits[0]; |
There was a problem hiding this comment.
The endpoint always returns fruits[0] regardless of the requested ID. This should use the :id parameter to return the correct fruit. For example, when requesting fruit with id=2 (banana), it incorrectly returns fruit with id=1 (apple).
| this.get('/fruits/:id', function() { | |
| const fruit = fruits[0]; | |
| this.get('/fruits/:id', function(request) { | |
| const id = parseInt(request.params.id, 10); | |
| let fruit = fruits[0]; | |
| if (!Number.isNaN(id)) { | |
| const foundFruit = fruits.find(f => f.id === id); | |
| if (foundFruit) { | |
| fruit = foundFruit; | |
| } | |
| } |
| @@ -1164,13 +1164,11 @@ en: | |||
| autohighlight_all_code: "Force apply code highlighting to all preformatted code blocks even when they didn't explicitly specify the language." | |||
| highlighted_languages: "Included syntax highlighting rules. (Warning: including too many langauges may impact performance) see: https://highlightjs.org/static/demo/ for a demo" | |||
There was a problem hiding this comment.
Corrected spelling of 'langauges' to 'languages'.
| highlighted_languages: "Included syntax highlighting rules. (Warning: including too many langauges may impact performance) see: https://highlightjs.org/static/demo/ for a demo" | |
| highlighted_languages: "Included syntax highlighting rules. (Warning: including too many languages may impact performance) see: https://highlightjs.org/static/demo/ for a demo" |
| SiteSetting.embed_truncate = false | ||
| expect(topic.expandable_first_post?).to eq(false) | ||
| end | ||
| describe 'with an emeddable host' do |
There was a problem hiding this comment.
Corrected spelling of 'emeddable' to 'embeddable'.
| describe 'with an emeddable host' do | |
| describe 'with an embeddable host' do |
Test 10
Summary by CodeRabbit
Release Notes
New Features
Refactoring
✏️ Tip: You can customize this high-level summary in your review settings.
Replicated from ai-code-review-evaluation/discourse-coderabbit#10