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.
Walkthrough
This PR refactors the embedding feature from a global site setting to a database-backed model with per-host category configuration. It introduces the EmbeddableHost model and associated admin UI for managing allowed embedding hosts. The change migrates existing embeddable host data from site settings to a dedicated database table, removes deprecated site setting methods, and implements full CRUD operations through new admin controllers and Ember.js components. The refactoring enables administrators to configure different categories for different embeddable hosts, replacing the previous single global category approach. Comprehensive test coverage is added for the new model and controllers.
Changes
| File(s) | Summary |
|---|---|
app/models/embeddable_host.rb |
Introduces new ActiveRecord model for managing whitelisted embedding hosts with category association, host format validation, protocol/path normalization, and class methods for host lookup and validation. |
db/migrate/20150818190757_create_embeddable_hosts.rb |
Creates embeddable_hosts table, migrates existing host data from site_settings, and removes deprecated 'embeddable_hosts' and 'embed_category' site setting entries. |
app/controllers/admin/embeddable_hosts_controller.rbapp/controllers/admin/embedding_controller.rb |
Adds new admin controllers for managing embeddable hosts with CRUD operations and embedding settings display/update functionality. |
app/serializers/embeddable_host_serializer.rbapp/serializers/embedding_serializer.rb |
Introduces serializers for JSON API responses exposing embeddable host attributes and associations. |
app/assets/javascripts/admin/adapters/embedding.js.es6app/assets/javascripts/admin/controllers/admin-embedding.js.es6app/assets/javascripts/admin/routes/admin-embedding.js.es6app/assets/javascripts/admin/routes/admin-route-map.js.es6 |
Implements Ember.js admin interface infrastructure including REST adapter, controller with CRUD actions, route for data loading, and route mapping for /admin/embedding path. |
app/assets/javascripts/admin/components/embeddable-host.js.es6app/assets/javascripts/admin/templates/components/embeddable-host.hbs |
Adds Ember component and template for managing individual embeddable hosts with buffered editing, validation, and delete confirmation. |
app/assets/javascripts/admin/templates/customize.hbsapp/assets/javascripts/admin/templates/embedding.hbs |
Adds navigation item and main template for embedding admin UI with host table and add host functionality. |
app/assets/javascripts/discourse/adapters/rest.js.es6 |
Adds 'embeddable-host' to ADMIN_MODELS and updates basePath() to normalize underscore/hyphen naming conventions. |
app/assets/javascripts/discourse/models/store.js.es6 |
Enhances _hydrateEmbedded method to support plural ID relationships (e.g., category_ids) in addition to singular ones. |
app/controllers/embed_controller.rblib/topic_retriever.rb |
Refactors host validation to use EmbeddableHost.host_allowed? instead of SiteSetting.allows_embeddable_host?. |
app/models/site_setting.rb |
Removes deprecated allows_embeddable_host? class method. |
app/models/topic.rb |
Simplifies expandable_first_post? by removing SiteSetting.embeddable_hosts.present? check. |
app/models/topic_embed.rb |
Modifies import method to use per-host category configuration from EmbeddableHost instead of global SiteSetting.embed_category. |
config/routes.rb |
Adds routes for embedding controller (GET/PUT) and embeddable_hosts RESTful resource under admin constraints. |
config/locales/client.en.ymlconfig/locales/server.en.yml |
Adds client-side translations for embedding UI and removes deprecated server-side locale keys for embeddable_hosts and embed_category settings. |
config/site_settings.yml |
Removes embeddable_hosts and embed_category configuration settings. |
spec/controllers/admin/embeddable_hosts_controller_spec.rbspec/controllers/admin/embedding_controller_spec.rb |
Adds basic controller specs verifying inheritance from Admin::AdminController. |
spec/models/embeddable_host_spec.rb |
Adds comprehensive test coverage for host normalization and host_allowed? validation logic. |
spec/controllers/embed_controller_spec.rbspec/models/topic_embed_spec.rbspec/models/topic_spec.rb |
Refactors tests to use Fabricate pattern for embeddable_host records instead of global site settings. |
spec/fabricators/category_fabricator.rbspec/fabricators/embeddable_host_fabricator.rb |
Swaps file contents - category_fabricator now contains embeddable_host definition and vice versa (appears to be a file organization issue). |
spec/models/site_setting_spec.rb |
Removes deprecated 'allows_embeddable_host' test suite. |
test/javascripts/helpers/create-pretender.js.es6test/javascripts/models/store-test.js.es6 |
Enhances test fixtures with many-to-many color relationships and modernizes assertions to use explicit assert parameter. |
Sequence Diagram
This diagram shows the interactions between components:
sequenceDiagram
participant Client
participant EmbeddingAdapter
participant RestAdapter
participant API
Note over EmbeddingAdapter: New adapter extends RestAdapter
Client->>EmbeddingAdapter: Request embedding data
activate EmbeddingAdapter
EmbeddingAdapter->>EmbeddingAdapter: pathFor()
Note over EmbeddingAdapter: Returns custom path:<br/>/admin/customize/embedding
EmbeddingAdapter->>RestAdapter: Delegate to parent adapter
activate RestAdapter
RestAdapter->>API: HTTP Request to /admin/customize/embedding
activate API
API-->>RestAdapter: Response data
deactivate API
RestAdapter-->>EmbeddingAdapter: Formatted response
deactivate RestAdapter
EmbeddingAdapter-->>Client: Return embedding data
deactivate EmbeddingAdapter
🔗 Cross-Repository Impact Analysis
Enable automatic detection of breaking changes across your dependent repositories. → Set up now
Learn more about Cross-Repository Analysis
What It Does
- Automatically identifies repositories that depend on this code
- Analyzes potential breaking changes across your entire codebase
- Provides risk assessment before merging to prevent cross-repo issues
How to Enable
- Visit Settings → Code Management
- Configure repository dependencies
- Future PRs will automatically include cross-repo impact analysis!
Benefits
- 🛡️ Prevent breaking changes across repositories
- 🔍 Catch integration issues before they reach production
- 📊 Better visibility into your multi-repo architecture
Install the extension
Note for Windsurf
Please change the default marketplace provider to the following in the windsurf settings:Marketplace Extension Gallery Service URL: https://marketplace.visualstudio.com/_apis/public/gallery
Marketplace Gallery Item URL: https://marketplace.visualstudio.com/items
Entelligence.ai can learn from your feedback. Simply add 👍 / 👎 emojis to teach it your preferences. More shortcuts below
Emoji Descriptions:
⚠️ Potential Issue - May require further investigation.- 🔒 Security Vulnerability - Fix to ensure system safety.
- 💻 Code Improvement - Suggestions to enhance code quality.
- 🔨 Refactor Suggestion - Recommendations for restructuring code.
- ℹ️ Others - General comments and information.
Interact with the Bot:
- Send a message or request using the format:
@entelligenceai + *your message*
Example: @entelligenceai Can you suggest improvements for this code?
- Help the Bot learn by providing feedback on its responses.
@entelligenceai + *feedback*
Example: @entelligenceai Do not comment on `save_auth` function !
Also you can trigger various commands with the bot by doing
@entelligenceai command
The current supported commands are
config- shows the current configretrigger_review- retriggers the review
More commands to be added soon.
| def update | ||
| host = EmbeddableHost.where(id: params[:id]).first | ||
| save_host(host) | ||
| end | ||
|
|
||
| def destroy | ||
| host = EmbeddableHost.where(id: params[:id]).first |
There was a problem hiding this comment.
Correctness: In update (line 10) and destroy (line 15), EmbeddableHost.where(id: params[:id]).first returns nil if the record is missing. This causes a NoMethodError at lines 23 and 16, resulting in a 500 error. Use EmbeddableHost.find(params[:id]) to ensure a 404 is returned when the record is not found.
🤖 AI Agent Prompt for Cursor/Windsurf
📋 Copy this prompt to your AI coding assistant (Cursor, Windsurf, etc.) to get help fixing this issue
File: app/controllers/admin/embeddable_hosts_controller.rb. Lines 9-15. Add a nil check after fetching `host` in both `update` and `destroy`. If `host` is nil, return a 404 or JSON error response and exit early; only call `save_host`/`destroy` when the record exists.
| self.host.sub!(/^https?:\/\//, '') | ||
| self.host.sub!(/\/.*$/, '') |
There was a problem hiding this comment.
Correctness: In the before_validation block, self.host.sub! will raise a NoMethodError if host is nil. Add a guard clause (e.g., return if host.blank?) to prevent crashes when saving records without a host.
🤖 AI Agent Prompt for Cursor/Windsurf
📋 Copy this prompt to your AI coding assistant (Cursor, Windsurf, etc.) to get help fixing this issue
In `app/models/embeddable_host.rb` lines 6-7, `sub!` is called on `host` without checking for nil. Add a guard (`return if host.blank?`) inside the `before_validation` block to prevent `NoMethodError` when `host` is missing. Apply the diff exactly and keep existing formatting.
| creator = PostCreator.new(user, | ||
| title: title, | ||
| raw: absolutize_urls(url, contents), | ||
| skip_validations: true, |
There was a problem hiding this comment.
Correctness: Passing eh.try(:category_id) introduces a regression: if eh is nil or lacks a category, the category parameter becomes nil, bypassing the SiteSetting.embed_category fallback. This causes topics to be created in the site's default category instead of the configured embed category. Use eh&.category_id || SiteSetting.embed_category.
🤖 AI Agent Prompt for Cursor/Windsurf
📋 Copy this prompt to your AI coding assistant (Cursor, Windsurf, etc.) to get help fixing this issue
In `app/models/topic_embed.rb` around the PostCreator initialization, restore the previous default category when no embeddable host is found. Replace the category argument with a fallback: `eh.try(:category_id) || SiteSetting.embed_category`.
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
EntelligenceAI PR Summary
Refactors embedding feature from site settings to database-backed model with per-host category configuration.
EmbeddableHostActiveRecord model with host validation, normalization, and category associationSiteSetting.allows_embeddable_host?method and related site settingsTopicEmbed.importto use per-host category instead of global embed_category settingEmbeddableHost.host_allowed?