FEATURE: Can edit category/host relationships for embedding #1

akshayutture-augment · 2025-11-14T23:41:32Z

Test 10

Summary by CodeRabbit

New Features
- Added admin panel for managing embeddable hosts with ability to add, edit, and delete hosts
- Per-host category assignment for embedded content from different sources
- Host validation and configuration management through admin interface
Refactor
- Restructured embedding configuration from global settings to individual host-level database records

_{✏️ Tip: You can customize this high-level summary in your review settings.}

Replicated from ai-code-review-evaluation/discourse-coderabbit#10

akshayutture-augment · 2025-11-14T23:44:41Z

@coderabbitai review

akshayutture-augment · 2025-11-22T00:36:04Z

@coderabbitai review

coderabbitai · 2025-11-22T00:36:08Z

✅ Actions performed

Review triggered.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

coderabbitai · 2025-11-22T00:36:24Z

Walkthrough

This pull request introduces a new EmbeddableHost database model to replace SiteSetting-based embeddable host management. It includes new Rails controllers, an admin UI with in-place editing, serializers, a migration, and updates existing code to query the model instead of SiteSetting values.

Changes

Cohort / File(s)	Summary
Rails Models & Validation `app/models/embeddable_host.rb`, `app/models/topic.rb`, `app/models/topic_embed.rb`	New EmbeddableHost model with host validation, category association, and class methods for host lookup. Topic and TopicEmbed updated to use EmbeddableHost queries instead of SiteSetting checks.
Rails Controllers `app/controllers/admin/embedding_controller.rb`, `app/controllers/admin/embeddable_hosts_controller.rb`, `app/controllers/embed_controller.rb`	New embedding and embeddable_hosts admin controllers with CRUD actions. EmbedController refactored to use EmbeddableHost.host_allowed? instead of SiteSetting checks.
Rails Configuration & Routing `config/routes.rb`, `config/site_settings.yml`	Added embedding and embeddable_hosts routes. Removed embeddable_hosts and embed_category from site settings.
Serializers `app/serializers/embeddable_host_serializer.rb`, `app/serializers/embedding_serializer.rb`	New serializers for EmbeddableHost and Embedding objects with appropriate attributes and associations.
Admin UI Components `app/assets/javascripts/admin/components/embeddable-host.js.es6`, `app/assets/javascripts/admin/templates/components/embeddable-host.hbs`	New Ember component for in-place editing of embeddable host rows with save, delete, and cancel actions.
Admin UI Routes & Templates `app/assets/javascripts/admin/routes/admin-embedding.js.es6`, `app/assets/javascripts/admin/routes/admin-route-map.js.es6`, `app/assets/javascripts/admin/controllers/admin-embedding.js.es6`, `app/assets/javascripts/admin/templates/embedding.hbs`, `app/assets/javascripts/admin/templates/customize.hbs`, `app/assets/javascripts/admin/adapters/embedding.js.es6`	New admin routes, controller, templates, and adapter for embedding management UI. Adds nav item to customize menu.
JavaScript & Discourse Infrastructure `app/assets/javascripts/discourse/adapters/rest.js.es6`, `app/assets/javascripts/discourse/models/store.js.es6`	Added embeddable-host to ADMIN_MODELS. Extended _hydrateEmbedded to support plural embedded ID fields.
Localization `config/locales/client.en.yml`, `config/locales/server.en.yml`	Added embedding-related translation keys for UI labels. Removed embeddable_hosts and embed_category translations.
Database Migration `db/migrate/20150818190757_create_embeddable_hosts.rb`	Migration creating embeddable_hosts table and seeding data from SiteSetting, then removing old settings.
Core Library Updates `lib/topic_retriever.rb`	Updated invalid_host? to use EmbeddableHost.host_allowed? instead of SiteSetting.allows_embeddable_host?.
Removed Public API `app/models/site_setting.rb`	Removed allows_embeddable_host? class method (functionality moved to EmbeddableHost model).
Controller & Model Specs `spec/controllers/admin/embeddable_hosts_controller_spec.rb`, `spec/controllers/admin/embedding_controller_spec.rb`, `spec/models/embeddable_host_spec.rb`, `spec/models/site_setting_spec.rb`	New tests for admin controllers and EmbeddableHost model. Removed tests for removed SiteSetting.allows_embeddable_host? method.
Integration Test Updates `spec/controllers/embed_controller_spec.rb`, `spec/models/topic_embed_spec.rb`, `spec/models/topic_spec.rb`	Updated tests to use Fabricate(:embeddable_host) instead of setting SiteSetting.embeddable_hosts. Restructured topic expansion tests.
Test Fixtures & Helpers `spec/fabricators/embeddable_host_fabricator.rb`, `spec/fabricators/category_fabricator.rb`, `test/javascripts/helpers/create-pretender.js.es6`, `test/javascripts/models/store-test.js.es6`	New embeddable_host fabricators. Category fabricators moved to new file. Test data updated with color relationships and embeddings.

Sequence Diagram

sequenceDiagram
    actor User as Admin User
    participant UI as Admin UI
    participant Controller as EmbeddingController
    participant Component as EmbeddableHost Component
    participant Rails as Rails Models/DB
    
    User->>UI: Open Embedding Settings
    UI->>Controller: GET /admin/customize/embedding
    Controller->>Rails: EmbeddableHost.all.order(:host)
    Rails-->>Controller: embeddable_hosts list
    Controller-->>UI: render embedding + hosts
    
    User->>UI: Click Edit on Host
    UI->>Component: toggle editing=true
    Component->>Component: focus host input
    
    User->>Component: Update host + category
    Component->>Component: track buffered changes
    
    User->>Component: Click Save
    Component->>Controller: PUT with host + category_id
    Controller->>Rails: update(:host, :category_id)
    Rails-->>Controller: updated host
    Controller-->>Component: success
    Component->>Component: exit edit mode, update display
    
    User->>Component: Click Delete
    Component->>Component: confirm dialog
    User->>Component: Confirm
    Component->>Controller: DELETE host
    Controller->>Rails: destroy record
    Rails-->>Controller: success
    Controller->>Component: notifyDeleteHost
    Component->>UI: remove from list

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~45 minutes

Key areas requiring careful attention:

EmbeddableHost model validation and host parsing logic — ensure URI parsing correctly handles various URL schemes and formats
Data migration safety — verify the migration correctly maps SiteSetting data to the new table and handles edge cases (missing categories, uncategorized defaults)
Component state management — review the editing/saving/canceling flow in embeddable-host component for race conditions and error handling
Store hydration changes — ensure plural embedded ID field support doesn't break existing single-ID behavior
Integration points — verify that all SiteSetting.allows_embeddable_host? call sites were properly replaced and the new EmbeddableHost queries work correctly across embed_controller, topic_retriever, and topic_embed
Test fixture reorganization — confirm category fabricators were correctly split without breaking existing tests

Poem

🐰 A model rose where settings dwelt before,
With hosts in rows now living in the store,
In-place we edit, delete, and we save,
The admin realm now organized and brave,
From strings to records, structured and so neat! 🎉

Pre-merge checks and finishing touches

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%.	You can run `@coderabbitai generate docstrings` to improve docstring coverage.

✅ Passed checks (2 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title accurately summarizes the main feature: editing category/host relationships for embedding, which is reflected throughout the changeset with new models, controllers, components, and UI for managing embeddable hosts.

✨ Finishing touches

📝 Generate docstrings

🧪 Generate unit tests (beta)

Create PR with unit tests
Post copyable unit tests in a comment
Commit unit tests in branch rest-serializer-enhancement-post

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

coderabbitai

Actionable comments posted: 6

🧹 Nitpick comments (12)

test/javascripts/models/store-test.js.es6 (1)
109-121: Stronger coverage for embedded relationships; consider aligning ID with fixtures

Using the injected assert plus explicit checks on colors length/IDs and category presence gives good coverage for the embedded relationships on a single fruit.

One minor clarity nit: this test calls store.find('fruit', 2), but in test/javascripts/helpers/create-pretender.js.es6 the fruit that has color_ids: [1, 2] is the one with id: 1, and the Pretender handler for /fruits/:id always returns the first fruit. The test passes, but the coupling is non-obvious. Consider either querying id: 1 here or updating the fixture so the fruit with id: 2 has [1, 2] to better reflect the test name/intent.

For example, either:
-  return store.find('fruit', 2).then(function(f) {
+  return store.find('fruit', 1).then(function(f) {
or:
-const fruits = [{id: 1, name: 'apple', farmer_id: 1, color_ids: [1,2], category_id: 4},
-                {id: 2, name: 'banana', farmer_id: 1, color_ids: [3], category_id: 3},
+const fruits = [{id: 1, name: 'apple', farmer_id: 1, color_ids: [1,2], category_id: 4},
+                {id: 2, name: 'banana', farmer_id: 1, color_ids: [1,2], category_id: 3},
app/assets/javascripts/discourse/adapters/rest.js.es6 (1)
1-24: Admin base path normalization works; consider handling multiple underscores

Adding 'embeddable-host' to ADMIN_MODELS and normalizing type before the lookup correctly routes embeddable-host requests through /admin/. To be slightly more robust for any future multi-underscore types, you could normalize all underscores:
-  basePath(store, type) {
-    if (ADMIN_MODELS.indexOf(type.replace('_', '-')) !== -1) { return "/admin/"; }
+  basePath(store, type) {
+    const normalized = type.replace(/_/g, '-');
+    if (ADMIN_MODELS.indexOf(normalized) !== -1) { return "/admin/"; }
spec/fabricators/embeddable_host_fabricator.rb (1)
1-27: Refactor :private_category fabricator to separate in-memory and database concerns

No duplicate Fabricator(:category) was found elsewhere—this file is the sole definition. However, the callback logic should be refined:

The after_build callback calls cat.update!, which persists to the DB even when tests use Fabricate.build(:private_category, ...), yet category_groups.build only creates an in-memory association and won't persist unless explicitly saved. Splitting into after_build (for in-memory state) and after_create (for DB records) ensures consistent behavior:
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
+   after_build do |cat, _|
+     cat.read_restricted = true
+   end
+
+   after_create do |cat, transients|
+     CategoryGroup.create!(
+       category: cat,
+       group: transients[:group],
+       permission_type: CategoryGroup.permission_types[:full]
+     )
+   end
end
Also ensure that any call to Fabricate(:private_category) always supplies group: to avoid nil errors, or add validation/documentation to clarify the requirement.
spec/controllers/admin/embeddable_hosts_controller_spec.rb (1)
1-9: Add comprehensive test coverage for controller actions.

This spec only verifies inheritance. Consider adding tests for the controller's CRUD actions (create, update, destroy) to ensure proper authorization, validation, and behavior.

Also, address the RuboCop convention warnings by removing extra blank lines:
 describe Admin::EmbeddableHostsController do
-
   it "is a subclass of AdminController" do
     expect(Admin::EmbeddableHostsController < Admin::AdminController).to eq(true)
   end
-
 end
spec/controllers/admin/embedding_controller_spec.rb (1)
1-9: Add comprehensive test coverage for controller actions.

This spec only verifies inheritance. Consider adding tests for the show and update actions to ensure proper authorization, embedding configuration handling, and response format.

Also, address the RuboCop convention warnings by removing extra blank lines:
 describe Admin::EmbeddingController do
-
   it "is a subclass of AdminController" do
     expect(Admin::EmbeddingController < Admin::AdminController).to eq(true)
   end
-
 end
app/assets/javascripts/admin/templates/embedding.hbs (1)
4-6: Consider using CSS classes instead of inline styles.

The inline style attributes work but are harder to maintain and override. Consider defining CSS classes for table column widths in a stylesheet for better maintainability.

For example:
<th class='embedding-host-column'>{{i18n "admin.embedding.host"}}</th>
<th class='embedding-category-column'>{{i18n "admin.embedding.category"}}</th>
<th class='embedding-actions-column'>&nbsp;</th>
Then in CSS:
.embedding-host-column { width: 50%; }
.embedding-category-column { width: 30%; }
.embedding-actions-column { width: 20%; }
app/serializers/embedding_serializer.rb (1)
2-7: Remove redundant id attribute definition.

The id attribute and method are unnecessary. ActiveModel Serializers (which ApplicationSerializer likely inherits from) automatically serializes the id attribute. Unless there's a specific reason for this custom method, it can be removed.

Apply this diff:
 class EmbeddingSerializer < ApplicationSerializer
-  attributes :id
   has_many :embeddable_hosts, serializer: EmbeddableHostSerializer, embed: :ids
-
-  def id
-    object.id
-  end
 end
spec/models/embeddable_host_spec.rb (2)
23-38: Consider adding edge case tests.

The current tests cover basic scenarios well. Consider adding tests for:

Subdomain matching (e.g., should sub.eviltrout.com be allowed if eviltrout.com is in the list?)

Port numbers in URLs

nil or empty referer values (important for security)

4-5: Fix RuboCop style violations.

Remove extra empty lines flagged by RuboCop.

Apply this diff:
 describe EmbeddableHost do
-
   it "trims http" do
       expect(EmbeddableHost.host_allowed?('http://discourse.org')).to eq(true)
     end
-
   end
-
 end
Also applies to: 37-40
app/serializers/embeddable_host_serializer.rb (1)
4-14: Consider removing redundant accessor methods.

The explicit accessor methods that simply return object.attribute are typically redundant when using the attributes declaration. Unless ApplicationSerializer requires explicit methods or you need to add custom logic later, these can be simplified.

If no custom logic is needed, you can simplify to:
 class EmbeddableHostSerializer < ApplicationSerializer
   attributes :id, :host, :category_id
-
-  def id
-    object.id
-  end
-
-  def host
-    object.host
-  end
-
-  def category_id
-    object.category_id
-  end
 end
app/assets/javascripts/admin/templates/components/embeddable-host.hbs (1)

1-19: Template wiring looks consistent with the component

The editing/view states, bindings (buffered.host, categoryId, cantSave, host.isSaving) and actions all line up with the JS component. This looks good.

If you want a tiny UX tweak, you might also disable the delete button when host.isSaving (mirroring the cancel button), but that’s optional.

spec/models/topic_spec.rb (1)

1399-1427: New expandable_first_post? specs capture the key embedding scenarios

The new expandable_first_post? examples nicely cover:

no embeddable hosts → false

embeddable_host present + embed_truncate true + has_topic_embed? true → true

toggling either embed_truncate or has_topic_embed? → false

That gives good confidence around the new EmbeddableHost‑based behavior. The only nit is the typo in the description string ('emeddable host'); you may want to fix that for clarity.

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 913c3d6 and d1c6918.

📒 Files selected for processing (36)

app/assets/javascripts/admin/adapters/embedding.js.es6 (1 hunks)
app/assets/javascripts/admin/components/embeddable-host.js.es6 (1 hunks)
app/assets/javascripts/admin/controllers/admin-embedding.js.es6 (1 hunks)
app/assets/javascripts/admin/routes/admin-embedding.js.es6 (1 hunks)
app/assets/javascripts/admin/routes/admin-route-map.js.es6 (1 hunks)
app/assets/javascripts/admin/templates/components/embeddable-host.hbs (1 hunks)
app/assets/javascripts/admin/templates/customize.hbs (1 hunks)
app/assets/javascripts/admin/templates/embedding.hbs (1 hunks)
app/assets/javascripts/discourse/adapters/rest.js.es6 (2 hunks)
app/assets/javascripts/discourse/models/store.js.es6 (1 hunks)
app/controllers/admin/embeddable_hosts_controller.rb (1 hunks)
app/controllers/admin/embedding_controller.rb (1 hunks)
app/controllers/embed_controller.rb (1 hunks)
app/models/embeddable_host.rb (1 hunks)
app/models/site_setting.rb (0 hunks)
app/models/topic.rb (1 hunks)
app/models/topic_embed.rb (1 hunks)
app/serializers/embeddable_host_serializer.rb (1 hunks)
app/serializers/embedding_serializer.rb (1 hunks)
config/locales/client.en.yml (1 hunks)
config/locales/server.en.yml (0 hunks)
config/routes.rb (2 hunks)
config/site_settings.yml (0 hunks)
db/migrate/20150818190757_create_embeddable_hosts.rb (1 hunks)
lib/topic_retriever.rb (1 hunks)
spec/controllers/admin/embeddable_hosts_controller_spec.rb (1 hunks)
spec/controllers/admin/embedding_controller_spec.rb (1 hunks)
spec/controllers/embed_controller_spec.rb (3 hunks)
spec/fabricators/category_fabricator.rb (1 hunks)
spec/fabricators/embeddable_host_fabricator.rb (1 hunks)
spec/models/embeddable_host_spec.rb (1 hunks)
spec/models/site_setting_spec.rb (0 hunks)
spec/models/topic_embed_spec.rb (2 hunks)
spec/models/topic_spec.rb (3 hunks)
test/javascripts/helpers/create-pretender.js.es6 (2 hunks)
test/javascripts/models/store-test.js.es6 (1 hunks)

💤 Files with no reviewable changes (4)

app/models/site_setting.rb
config/locales/server.en.yml
config/site_settings.yml
spec/models/site_setting_spec.rb

🧰 Additional context used

🪛 RuboCop (1.81.7)

db/migrate/20150818190757_create_embeddable_hosts.rb

[convention] 2-32: Assignment Branch Condition size for change is too high. [<7, 22, 8> 24.43/23]

(Metrics/AbcSize)

spec/controllers/admin/embeddable_hosts_controller_spec.rb

[convention] 4-5: Extra empty line detected at block body beginning.