FEATURE: Can edit category/host relationships for embedding#10
FEATURE: Can edit category/host relationships for embedding#10everettbu wants to merge 1 commit into
Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR implements the ability to edit category/host relationships for embedding functionality. It refactors the embedding system from a single site setting to a database-backed model with individual embeddable host records that can be associated with specific categories.
Key changes include:
- Migration from site settings to EmbeddableHost model for managing embeddable hosts
- Addition of admin UI for managing embeddable hosts and their category associations
- Enhanced test infrastructure with new fabricators and improved test data
Reviewed Changes
Copilot reviewed 36 out of 36 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| app/models/embeddable_host.rb | New model for managing embeddable hosts with category relationships |
| db/migrate/20150818190757_create_embeddable_hosts.rb | Database migration to create embeddable_hosts table and migrate existing settings |
| app/controllers/admin/embeddable_hosts_controller.rb | CRUD controller for managing embeddable hosts in admin interface |
| app/assets/javascripts/admin/components/embeddable-host.js.es6 | Frontend component for editing individual embeddable hosts |
| app/assets/javascripts/discourse/models/store.js.es6 | Enhanced store to handle plural ID relationships for embedded objects |
| spec/fabricators/embeddable_host_fabricator.rb | Test fabricator for creating embeddable host test data |
| Multiple test files | Updated to use new embeddable host model instead of site settings |
| @@ -0,0 +1,27 @@ | |||
| Fabricator(:category) do | |||
There was a problem hiding this comment.
This file is named 'embeddable_host_fabricator.rb' but contains category fabricators. The category fabricators should be in a separate file or this file should be renamed to reflect its actual content.
| cat.update!(read_restricted: true) | ||
| cat.category_groups.build(group_id: transients[:group].id, permission_type: CategoryGroup.permission_types[:full]) | ||
| end | ||
| Fabricator(:embeddable_host) do |
There was a problem hiding this comment.
This file is named 'category_fabricator.rb' but contains an embeddable_host fabricator. The embeddable_host fabricator should be in a separate file or this file should be renamed appropriately.
| self.host.sub!(/^https?:\/\//, '') | ||
| self.host.sub!(/\/.*$/, '') |
There was a problem hiding this comment.
The method sub! will raise a NoMethodError if self.host is nil. Consider using safe navigation or checking for nil before calling destructive string methods.
| self.host.sub!(/^https?:\/\//, '') | |
| self.host.sub!(/\/.*$/, '') | |
| self.host&.sub!(/^https?:\/\//, '') | |
| self.host&.sub!(/\/.*$/, '') |
| self.host.sub!(/^https?:\/\//, '') | ||
| self.host.sub!(/\/.*$/, '') |
There was a problem hiding this comment.
The method sub! will raise a NoMethodError if self.host is nil. Consider using safe navigation or checking for nil before calling destructive string methods.
| self.host.sub!(/^https?:\/\//, '') | |
| self.host.sub!(/\/.*$/, '') | |
| self.host&.sub!(/^https?:\/\//, '') | |
| self.host&.sub!(/\/.*$/, '') |
| 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.
There's a typo in 'emeddable' - it should be 'embeddable'.
| describe 'with an emeddable host' do | |
| describe 'with an embeddable host' do |
| this.get('/fruits/:id', function() { | ||
| const fruit = fruits[0]; | ||
|
|
||
| return response({ __rest_serializer: "1", fruit, farmers: [farmers[0]] }); | ||
| return response({ __rest_serializer: "1", fruit, farmers, colors }); |
There was a problem hiding this comment.
The endpoint returns fruits[0] (apple) for any fruit ID, but the test expects fruit with ID 2 (banana). This should return the fruit that matches the requested ID.
|
This pull request has been automatically marked as stale because it has been open for 60 days with no activity. To keep it open, remove the stale tag, push code, or add a comment. Otherwise, it will be closed in 14 days. |
Test 10