FEATURE: Can edit category/host relationships for embedding#10
FEATURE: Can edit category/host relationships for embedding#10ShashankFC wants to merge 1 commit into
Conversation
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
📝 WalkthroughWalkthroughIntroduces a comprehensive embedding management system that migrates embeddable host configuration from site settings to a database-backed model, including new admin UI components, controllers, routes, and updates to the embed validation logic to use the new EmbeddableHost model. Changes
Sequence DiagramsequenceDiagram
participant Admin as Admin User
participant UI as Embedding Admin UI
participant Controller as Admin Controller
participant Model as EmbeddableHost Model
participant DB as Database
Admin->>UI: Click "Add Host"
UI->>Controller: addHost() action
Controller->>Model: Create new embeddable_host record
Model-->>Controller: New record instance
Controller->>UI: Push to embedding.embeddable_hosts
Admin->>UI: Fill host and category, click Save
UI->>Controller: save() action with buffered data
Controller->>Model: save(host, category_id)
Model->>DB: INSERT/UPDATE embeddable_hosts
DB-->>Model: Record persisted
Model-->>Controller: Success
Controller->>UI: Exit edit mode, refresh display
Admin->>UI: Click delete icon
UI->>Admin: Show confirmation
Admin->>UI: Confirm delete
UI->>Controller: deleteHost(host) action
Controller->>Model: destroy(host_id)
Model->>DB: DELETE from embeddable_hosts
DB-->>Model: Record deleted
Model-->>Controller: Success
Controller->>UI: Remove from embedding.embeddable_hosts
Estimated Code Review Effort🎯 4 (Complex) | ⏱️ ~45 minutes The changes span multiple architectural layers (frontend components, controllers, routes; backend models, controllers, serializers; database migration) with heterogeneous modifications including new features (admin UI, model validation, relationships), refactoring of existing logic (site settings → database), and comprehensive test coverage across models, controllers, and fabricators. While individual changes follow consistent patterns, the breadth and interconnectedness of updates across the stack require careful validation of integration points. Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
Comment |
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 12
🤖 Fix all issues with AI agents
In @app/assets/javascripts/admin/components/embeddable-host.js.es6:
- Around line 46-48: The delete action currently calls
this.get('host').destroyRecord() without error handling; append a
.catch(popupAjaxError) to the promise chain (matching the save action pattern)
so server errors surface to the user, and ensure the popupAjaxError helper is
imported/available in this component (referencing destroyRecord and
popupAjaxError and the existing sendAction('deleteHost', this.get('host'))
sequence).
In @app/assets/javascripts/admin/controllers/admin-embedding.js.es6:
- Around line 14-16: The deleteHost action only removes the host from the local
embedding.embeddable_hosts array and does not persist to the server; update
deleteHost to check the record state (e.g., host.get('isNew') or host.get('id'))
and for persisted hosts call host.destroyRecord() (or host.deleteRecord()
followed by host.save()) and only remove it from embedding.embeddable_hosts
after the destroy/save succeeds (handle promise .then/.catch to update the array
and surface errors); for new/unsaved hosts simply removeObject(host) as before.
In @app/controllers/admin/embeddable_hosts_controller.rb:
- Around line 9-12: The update action calls save_host with host =
EmbeddableHost.where(id: params[:id]).first but doesn't handle the case when
host is nil; add a nil check in the update method to return early and respond
appropriately (e.g., render head :not_found or redirect with a flash error) when
host is not found, and only call save_host(host) when host is present; reference
the update action and save_host method to implement this guard.
- Around line 14-18: The destroy action calls host.destroy without handling a
missing record; update the EmbeddableHostsController#destroy to fetch the record
via EmbeddableHost.where(id: params[:id]).first (or find_by) and return an
appropriate error/404 response when host is nil instead of calling destroy on
nil, otherwise call host.destroy and render success_json; ensure you reference
the same symbols (destroy, EmbeddableHost, params[:id], success_json) when
adding the nil check and response.
In @app/models/embeddable_host.rb:
- Around line 5-8: The before_validation callback currently calls self.host.sub!
twice which will raise NoMethodError when host is nil; update the callback in
embeddable_host.rb to guard against nil by checking presence of host (e.g.,
return unless host or use host.to_s) before calling sub! or operate on a safe
local string, ensuring you still strip leading http(s):// and trailing path
while not invoking sub! on nil; reference the before_validation block and
self.host.sub! occurrences when making the change.
- Line 2: The host validation regex in validates_format_of :host restricts TLDs
to 2–5 letters and will reject valid longer gTLDs; update the pattern used in
validates_format_of :host to allow TLDs of two or more letters (e.g., change
`[a-z]{2,5}` to `[a-z]{2,}`) or replace the simple regex with a more robust
domain validation helper that handles IDNs and new gTLDs, ensuring the change is
applied where validates_format_of :host is defined.
- Around line 10-18: The method record_for_host currently uses an inline rescue
modifier and returns false on failure which mixes return types; change it to
return nil on failure and replace the `rescue nil` with an explicit begin/rescue
that only rescues URI::InvalidURIError (or StandardError if necessary) so
exceptions aren't silently swallowed; extract the hostname via
URI.parse(host).host (or check host presence first), normalize it (e.g.,
downcase) and then perform the query using where(...).first; ensure callers
(e.g., host_allowed?) expect nil rather than false.
In @db/migrate/20150818190757_create_embeddable_hosts.rb:
- Around line 24-26: The migration directly interpolates the host string inside
the raw SQL in the records.each do |h| loop (the execute call), exposing an SQL
injection risk; change the insert to use a parameterized/quoted value instead of
string interpolation — e.g., use ActiveRecord's quote(h) or bind parameters when
building the INSERT in the execute call (keep the same embeddable_hosts columns
and category_id variable) so the host value is properly escaped before executing
the SQL.
In @spec/fabricators/embeddable_host_fabricator.rb:
- Around line 23-26: The after_build block currently calls cat.update! which
performs an immediate DB save; instead assign the attribute directly (set
cat.read_restricted = true) and keep building the association via
cat.category_groups.build(group_id: transients[:group].id, permission_type:
CategoryGroup.permission_types[:full]) so Fabrication handles persistence;
remove the update! call and use direct attribute assignment inside the
after_build callback.
In @spec/models/topic_spec.rb:
- Line 1407: The spec description contains a typo: change the string literal in
the spec block from "with an emeddable host" to "with an embeddable host" by
updating the describe block that currently reads 'with an emeddable host' to the
corrected 'with an embeddable host' (leave the surrounding describe ... do/end
intact).
In @test/javascripts/models/store-test.js.es6:
- Around line 109-121: The test named "find embedded" is asserting colors for
fruit id 2 (banana) but fixture data in create-pretender.js.es6 shows banana has
color_ids [3]; update the test to be consistent by either calling
store.find('fruit', 1) to test apple (which has color_ids [1,2]) or adjust the
assertions after store.find('fruit', 2) (inside the test('find embedded')
callback) to expect a single color with id 3 (and length 1) and update the
corresponding id checks and messages accordingly; ensure createStore() usage and
the assertions on f.get('colors') are changed to match the chosen fruit.
🧹 Nitpick comments (15)
db/migrate/20150818190757_create_embeddable_hosts.rb (1)
1-32: Migration is not reversible.This migration lacks a
downmethod, making rollback impossible. Consider adding a reversible block or explicitup/downmethods if rollback support is needed.♻️ Suggested reversible implementation
class CreateEmbeddableHosts < ActiveRecord::Migration def up # ... current change method content ... end def down # Re-create site_settings entries if needed, or just drop table drop_table :embeddable_hosts end endapp/assets/javascripts/discourse/models/store.js.es6 (1)
196-201: Potential undefined values in hydrated colors array.If
_lookupSubTypereturnsundefinedfor any ID (e.g., if a color ID references a non-existent color), the resulting array will containundefinedelements. The|| []fallback on line 200 only applies if the entirehydratedvariable is falsy, not individual elements.♻️ Proposed fix: filter out undefined values
if (m[2]) { const hydrated = obj[k].map(function(id) { return self._lookupSubType(subType, type, id, root); - }); - obj[self.pluralize(subType)] = hydrated || []; + }).filter(Boolean); + obj[self.pluralize(subType)] = hydrated; delete obj[k];app/assets/javascripts/discourse/adapters/rest.js.es6 (1)
22-22:replace('_', '-')only replaces the first underscore.The current implementation only replaces the first underscore occurrence. While this works for
embeddable_host→embeddable-host, it would fail for types with multiple underscores (e.g.,some_admin_model→some-admin_model).♻️ Proposed fix: replace all underscores
- if (ADMIN_MODELS.indexOf(type.replace('_', '-')) !== -1) { return "/admin/"; } + if (ADMIN_MODELS.indexOf(type.replace(/_/g, '-')) !== -1) { return "/admin/"; }spec/controllers/admin/embeddable_hosts_controller_spec.rb (1)
3-7: Test coverage is minimal - only inheritance is tested.The spec only verifies that
EmbeddableHostsControllerinherits fromAdminController. Consider adding tests for the controller actions (create,update,destroy) to ensure proper authorization and behavior.Do you want me to help generate test cases for the controller actions?
spec/controllers/admin/embedding_controller_spec.rb (1)
3-9: Remove extra empty lines per RuboCop conventions.The static analysis flagged extra empty lines at block body beginning (line 4) and end (line 8).
♻️ Proposed fix
describe Admin::EmbeddingController do - it "is a subclass of AdminController" do expect(Admin::EmbeddingController < Admin::AdminController).to eq(true) end - endNote: The test coverage is minimal—only verifying class inheritance. Consider adding tests for the
showandupdateactions mentioned in the AI summary to ensure proper functionality and authorization checks.Would you like me to help generate additional test cases for the controller actions?
spec/models/embeddable_host_spec.rb (2)
3-40: Minor style issues flagged by RuboCop.The test logic and coverage are good. However, RuboCop has flagged extra empty lines at the block body boundaries (lines 4-5, 37-38, 39-40).
🧹 Proposed fix for style consistency
describe EmbeddableHost do - it "trims http" doend - end - end
26-30: Consider a more descriptive test name.The test name "works as expected" is vague. Consider something like "allows HTTP and HTTPS for registered host and rejects unregistered hosts" to improve readability.
app/assets/javascripts/admin/controllers/admin-embedding.js.es6 (1)
5-7: Consider adding user feedback for save operation.The
saveChangesaction callsupdate({})but doesn't handle success/failure responses. Users won't know if the save succeeded or failed.Proposed improvement
saveChanges() { - this.get('embedding').update({}); + this.get('embedding').update({}).then(() => { + // Show success notification + }).catch((error) => { + // Handle error + }); },app/controllers/admin/embedding_controller.rb (1)
3-3: Consider usingbefore_actioninstead ofbefore_filter.
before_filteris deprecated in Rails 4.x. While it still works,before_actionis the preferred method name.Proposed change
- before_filter :ensure_logged_in, :ensure_staff, :fetch_embedding + before_action :ensure_logged_in, :ensure_staff, :fetch_embeddingapp/controllers/admin/embeddable_hosts_controller.rb (1)
22-32: Consider usingfind_byinstead ofwhere(...).first.The idiomatic Rails approach is to use
find_by(id: params[:id])which is cleaner and has the same behavior.Also, consider using strong parameters for safer param handling.
Proposed refactor for update/destroy
def update - host = EmbeddableHost.where(id: params[:id]).first + host = EmbeddableHost.find_by(id: params[:id]) + return render_json_error(I18n.t('not_found')) unless host save_host(host) end def destroy - host = EmbeddableHost.where(id: params[:id]).first + host = EmbeddableHost.find_by(id: params[:id]) + return render_json_error(I18n.t('not_found')) unless host host.destroy render json: success_json endspec/fabricators/category_fabricator.rb (1)
1-4: File naming inconsistency: fabricator doesn't match filename.This file is named
category_fabricator.rbbut definesFabricator(:embeddable_host). According to the AI summary, the category fabricators were moved toembeddable_host_fabricator.rb, suggesting the content and filename may have been swapped. This could cause confusion when developers look for fabricators by convention.Consider either:
- Renaming this file to match its content, or
- Moving the
embeddable_hostfabricator toembeddable_host_fabricator.rband keeping category fabricators here.app/serializers/embeddable_host_serializer.rb (1)
4-14: Redundant method definitions.The explicit
id,host, andcategory_idmethods are unnecessary. ActiveModelSerializers automatically delegates declared attributes to the underlying object. These methods can be removed without changing behavior.Suggested simplification
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 endspec/fabricators/embeddable_host_fabricator.rb (1)
1-27: File naming inconsistency: contains category fabricators, not embeddable_host.This file is named
embeddable_host_fabricator.rbbut defines category-related fabricators (category,diff_category,happy_category,private_category). Theembeddable_hostfabricator is incategory_fabricator.rb. The file contents appear to be swapped.app/assets/javascripts/admin/components/embeddable-host.js.es6 (2)
43-51:sendActionis deprecated; prefer closure actions.
sendActionis deprecated in Ember. The component should accept a closure action (e.g.,deleteHost) and invoke it directly. This also improves clarity and type safety.Suggested approach
delete() { bootbox.confirm(I18n.t('admin.embedding.confirm_delete'), (result) => { if (result) { this.get('host').destroyRecord().then(() => { - this.sendAction('deleteHost', this.get('host')); + this.deleteHost(this.get('host')); }); } }); },Then in the template, pass the action as:
deleteHost=(action "removeHost")
13-17: Replace deprecatedthis.$()with native DOM APIs.
this.$()is deprecated in modern Ember. Usethis.element.querySelector()instead:_focusOnInput() { Ember.run.schedule('afterRender', () => { this.element.querySelector('.host-name').focus(); }); }Alternatively, use the
{{did-insert}}modifier in the template to simplify DOM access.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (36)
app/assets/javascripts/admin/adapters/embedding.js.es6app/assets/javascripts/admin/components/embeddable-host.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.es6app/assets/javascripts/admin/templates/components/embeddable-host.hbsapp/assets/javascripts/admin/templates/customize.hbsapp/assets/javascripts/admin/templates/embedding.hbsapp/assets/javascripts/discourse/adapters/rest.js.es6app/assets/javascripts/discourse/models/store.js.es6app/controllers/admin/embeddable_hosts_controller.rbapp/controllers/admin/embedding_controller.rbapp/controllers/embed_controller.rbapp/models/embeddable_host.rbapp/models/site_setting.rbapp/models/topic.rbapp/models/topic_embed.rbapp/serializers/embeddable_host_serializer.rbapp/serializers/embedding_serializer.rbconfig/locales/client.en.ymlconfig/locales/server.en.ymlconfig/routes.rbconfig/site_settings.ymldb/migrate/20150818190757_create_embeddable_hosts.rblib/topic_retriever.rbspec/controllers/admin/embeddable_hosts_controller_spec.rbspec/controllers/admin/embedding_controller_spec.rbspec/controllers/embed_controller_spec.rbspec/fabricators/category_fabricator.rbspec/fabricators/embeddable_host_fabricator.rbspec/models/embeddable_host_spec.rbspec/models/site_setting_spec.rbspec/models/topic_embed_spec.rbspec/models/topic_spec.rbtest/javascripts/helpers/create-pretender.js.es6test/javascripts/models/store-test.js.es6
💤 Files with no reviewable changes (4)
- config/site_settings.yml
- config/locales/server.en.yml
- app/models/site_setting.rb
- spec/models/site_setting_spec.rb
🧰 Additional context used
🧬 Code graph analysis (11)
lib/topic_retriever.rb (1)
app/models/embeddable_host.rb (1)
host_allowed?(20-22)
app/controllers/embed_controller.rb (1)
app/models/embeddable_host.rb (1)
host_allowed?(20-22)
app/serializers/embedding_serializer.rb (2)
app/serializers/embeddable_host_serializer.rb (2)
attributes(1-15)id(4-6)app/serializers/about_serializer.rb (1)
has_many(1-15)
spec/models/topic_spec.rb (1)
app/models/topic.rb (1)
expandable_first_post?(868-870)
spec/fabricators/category_fabricator.rb (1)
app/serializers/embeddable_host_serializer.rb (1)
host(8-10)
db/migrate/20150818190757_create_embeddable_hosts.rb (1)
app/serializers/embeddable_host_serializer.rb (1)
category_id(12-14)
app/controllers/admin/embeddable_hosts_controller.rb (3)
app/controllers/admin/embedding_controller.rb (2)
before_filter(1-21)update(9-11)app/serializers/embeddable_host_serializer.rb (2)
host(8-10)category_id(12-14)app/controllers/application_controller.rb (3)
success_json(362-364)render_serialized(238-240)render_json_error(357-360)
app/models/topic_embed.rb (1)
app/models/embeddable_host.rb (1)
record_for_host(10-18)
app/controllers/admin/embedding_controller.rb (2)
app/controllers/admin/embeddable_hosts_controller.rb (2)
before_filter(1-34)update(9-12)app/controllers/application_controller.rb (1)
render_serialized(238-240)
app/serializers/embeddable_host_serializer.rb (1)
app/serializers/embedding_serializer.rb (2)
attributes(1-8)id(5-7)
spec/models/embeddable_host_spec.rb (2)
app/serializers/embeddable_host_serializer.rb (1)
host(8-10)app/models/embeddable_host.rb (1)
host_allowed?(20-22)
🪛 RuboCop (1.82.1)
app/models/embeddable_host.rb
[convention] 11-11: Avoid using rescue in its modifier form.
(Style/RescueModifier)
spec/controllers/admin/embeddable_hosts_controller_spec.rb
[convention] 4-5: Extra empty line detected at block body beginning.
(Layout/EmptyLinesAroundBlockBody)
[convention] 8-9: Extra empty line detected at block body end.
(Layout/EmptyLinesAroundBlockBody)
spec/controllers/admin/embedding_controller_spec.rb
[convention] 4-5: Extra empty line detected at block body beginning.
(Layout/EmptyLinesAroundBlockBody)
[convention] 8-9: Extra empty line detected at block body end.
(Layout/EmptyLinesAroundBlockBody)
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/models/embeddable_host_spec.rb
[convention] 4-5: Extra empty line detected at block body beginning.
(Layout/EmptyLinesAroundBlockBody)
[convention] 37-38: Extra empty line detected at block body end.
(Layout/EmptyLinesAroundBlockBody)
[convention] 39-40: Extra empty line detected at block body end.
(Layout/EmptyLinesAroundBlockBody)
🔇 Additional comments (29)
test/javascripts/helpers/create-pretender.js.es6 (2)
42-51: LGTM! Test fixture data properly extended with color relationships.The fruits now include
color_idsarrays referencing the newcolorsdataset. The data is consistent: fruit 1 (apple) has colors [1,2] (Red, Green), fruit 2 (banana) has [3] (Yellow), and fruit 3 (grape) has [2] (Green).
226-233: LGTM! API responses correctly include the colors collection.Both the single fruit (
/fruits/:id) and collection (/fruits) endpoints now return thecolorsarray alongsidefruitsandfarmers, supporting the store's embedded relationship hydration.test/javascripts/models/store-test.js.es6 (1)
123-135: LGTM! findAll embedded test correctly validates colors on the first fruit.The test correctly asserts that
fruits.objectAt(0)(apple, id 1) has 2 colors with ids 1 and 2, which matches the fixture data.app/assets/javascripts/discourse/models/store.js.es6 (1)
189-211: LGTM! Hydration logic correctly extended for plural ID arrays.The implementation properly distinguishes between singular
_idand plural_idssuffixes, hydrating single objects vs arrays accordingly. This enables proper embedded relationship resolution for the new embeddable hosts feature.app/assets/javascripts/discourse/adapters/rest.js.es6 (1)
1-1: LGTM!embeddable-hostcorrectly added to admin models.This ensures that requests for embeddable hosts are routed through the
/admin/base path.app/models/topic.rb (1)
868-870: LGTM! Simplified predicate aligns with the new EmbeddableHost model.Removing the
embeddable_hostspresence check is appropriate since the embedding configuration has moved from site settings to the database-backedEmbeddableHostmodel. The remaining conditions (embed_truncate?andhas_topic_embed?) are sufficient to determine if the first post is expandable.spec/models/topic_embed_spec.rb (2)
37-38: Good coverage for category assignment.This assertion validates the core functionality where topics created via embedding are assigned to the category configured on the matching
EmbeddableHostrecord.
15-15: ✓ Fabricator correctly set up with matching host.The embeddable_host fabricator (spec/fabricators/embeddable_host_fabricator.rb) correctly defines
host "eviltrout.com", which matches the test URL. Thelet!ensures the embeddable_host is created before the test runs, andEmbeddableHost.record_for_host(url)will properly find the match for category assignment.lib/topic_retriever.rb (1)
15-17: LGTM! Clean migration to database-backed host validation.The change correctly delegates host validation to
EmbeddableHost.host_allowed?, which internally usesrecord_for_host(host).present?per the relevant code snippet. This aligns with the PR's migration from site settings to the database-backedEmbeddableHostmodel.app/assets/javascripts/admin/adapters/embedding.js.es6 (1)
1-7: LGTM!The adapter follows Discourse's standard pattern for custom REST adapters, cleanly extending
RestAdapterand overridingpathFor()to point to the embedding endpoint. The server-side routes at/admin/customize/embeddingare properly defined with both GET and PUT methods.app/assets/javascripts/admin/templates/customize.hbs (1)
8-8: LGTM!The navigation item follows the established pattern and integrates cleanly with the existing admin navigation structure. The i18n key
admin.embedding.titleis properly defined in the locale files, and theadminEmbeddingroute is configured correctly.app/models/topic_embed.rb (1)
36-43: LGTM! Host-based category assignment is correctly implemented.The migration from static
SiteSetting.embed_categoryto dynamicEmbeddableHost.record_for_host(url)lookup is well done. Usingeh.try(:category_id)safely handles the case whenrecord_for_hostreturnsfalse(no matching host) by passingniltoPostCreator, which will use the default category behavior.config/routes.rb (2)
138-139: LGTM! Routes follow existing conventions.The embedding routes are correctly placed within the admin namespace with
AdminConstraint.new, consistent with other customize-related routes in this file.
153-153: LGTM! RESTful resource declaration is appropriate.The
embeddable_hostsresource correctly inherits the admin constraint, providing standard CRUD operations for managing embeddable hosts.config/locales/client.en.yml (1)
2494-2501: LGTM! Locale entries are well-structured.The new embedding translation keys are appropriately named and follow the existing conventions in this file. The strings support the new admin embedding UI functionality.
app/controllers/embed_controller.rb (1)
58-67: LGTM! Access control correctly migrated to EmbeddableHost model.The change from
SiteSetting-based host validation toEmbeddableHost.host_allowed?(request.referer)properly integrates with the new database-backed embeddable host management. Thehost_allowed?method handles edge cases likenilor invalid referers gracefully by returningfalse, which triggers the appropriateInvalidAccessexception.app/assets/javascripts/admin/templates/components/embeddable-host.hbs (1)
1-19: LGTM! Well-structured component template with clear editing/view modes.The template correctly implements the two-state UI pattern for editing embeddable hosts. The use of
buffered.hostfor the input allows for cancel functionality without mutating the original record, and the button states (cantSave,host.isSaving) provide appropriate feedback. Thecategory-chooserbinding pattern matches existing usage insplit-topic.hbsandbulk_change_category.hbs, confirming it's the correct approach for binding primitive ID values to the component.app/assets/javascripts/admin/templates/embedding.hbs (1)
1-15: LGTM!The template structure is correct. The conditional rendering for the hosts table and the always-visible "Add Host" button provide good UX. The component integration with
embeddable-hostand the action bindings (deleteHost,addHost) align with the controller defined inadmin-embedding.js.es6.Minor note: The inline
styleattributes on lines 4-6 could be extracted to CSS classes for better maintainability, but this is a minor preference.app/assets/javascripts/admin/routes/admin-route-map.js.es6 (1)
30-30: LGTM!The new
adminEmbeddingresource follows the established naming convention and is appropriately placed within theadminCustomizeroute group alongside related resources.spec/controllers/embed_controller_spec.rb (2)
21-21: LGTM!Good migration from
SiteSettingto the database-backedEmbeddableHostmodel using fabricators.
66-98: Good coverage of multiple hosts and path normalization.The multi-host context properly tests:
- Multiple allowed hosts
- Path stripping during host normalization (line 87 uses a different path than line 70, correctly validating normalization behavior)
- Rejection of unregistered hosts
app/assets/javascripts/admin/routes/admin-embedding.js.es6 (1)
1-9: LGTM!The route correctly fetches the embedding model and assigns it to the controller. The
setupControllerproperly populatescontroller.embeddingwhich aligns with the template's{{#if embedding.embeddable_hosts}}usage. The globalEmber.Route.extendpattern is consistent throughout the admin routes in this codebase.app/controllers/admin/embedding_controller.rb (1)
15-20: LGTM!The
fetch_embeddingmethod appropriately constructs an embedding object with a fixed ID and ordered embeddable hosts for serialization.app/serializers/embedding_serializer.rb (1)
1-8: LGTM!The serializer correctly exposes the embedding ID and establishes the
has_manyrelationship with embeddable hosts using ID embedding, which aligns with the REST serializer pattern used in this codebase.spec/models/topic_spec.rb (2)
1241-1241: LGTM!Removing the unused
topicvariable assignment is a good cleanup.
1399-1427: No issues found. The test expectations align correctly with the implementation. When no embeddable host exists,has_topic_embed?returns false (no TopicEmbed record for a newly built topic), causingexpandable_first_post?to correctly return false without requiring an explicit EmbeddableHost presence check.app/controllers/admin/embeddable_hosts_controller.rb (1)
5-7: LGTM!The
createaction correctly instantiates a newEmbeddableHostand delegates tosave_host.app/models/embeddable_host.rb (1)
20-22: LGTM!The
host_allowed?method correctly delegates torecord_for_hostand uses.present?to handle the result.app/assets/javascripts/admin/components/embeddable-host.js.es6 (1)
30-41: LGTM!The save action properly validates via
cantSave, extracts buffered properties, persists to server, updates the local category reference, and handles errors withpopupAjaxError.
| this.get('host').destroyRecord().then(() => { | ||
| this.sendAction('deleteHost', this.get('host')); | ||
| }); |
There was a problem hiding this comment.
Missing error handling for destroyRecord.
Unlike the save action which uses .catch(popupAjaxError), the delete action doesn't handle failures from destroyRecord(). If the server returns an error, the user won't see any feedback.
Suggested fix
this.get('host').destroyRecord().then(() => {
this.sendAction('deleteHost', this.get('host'));
- });
+ }).catch(popupAjaxError);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| this.get('host').destroyRecord().then(() => { | |
| this.sendAction('deleteHost', this.get('host')); | |
| }); | |
| this.get('host').destroyRecord().then(() => { | |
| this.sendAction('deleteHost', this.get('host')); | |
| }).catch(popupAjaxError); |
🤖 Prompt for AI Agents
In @app/assets/javascripts/admin/components/embeddable-host.js.es6 around lines
46 - 48, The delete action currently calls this.get('host').destroyRecord()
without error handling; append a .catch(popupAjaxError) to the promise chain
(matching the save action pattern) so server errors surface to the user, and
ensure the popupAjaxError helper is imported/available in this component
(referencing destroyRecord and popupAjaxError and the existing
sendAction('deleteHost', this.get('host')) sequence).
| deleteHost(host) { | ||
| this.get('embedding.embeddable_hosts').removeObject(host); | ||
| } |
There was a problem hiding this comment.
deleteHost does not persist deletion to the server.
The action only removes the host from the local array but doesn't delete it from the database. If the host was previously saved, it will reappear after page refresh.
Proposed fix
deleteHost(host) {
+ if (host.get('id')) {
+ host.destroyRecord();
+ }
this.get('embedding.embeddable_hosts').removeObject(host);
}🤖 Prompt for AI Agents
In @app/assets/javascripts/admin/controllers/admin-embedding.js.es6 around lines
14 - 16, The deleteHost action only removes the host from the local
embedding.embeddable_hosts array and does not persist to the server; update
deleteHost to check the record state (e.g., host.get('isNew') or host.get('id'))
and for persisted hosts call host.destroyRecord() (or host.deleteRecord()
followed by host.save()) and only remove it from embedding.embeddable_hosts
after the destroy/save succeeds (handle promise .then/.catch to update the array
and surface errors); for new/unsaved hosts simply removeObject(host) as before.
| def update | ||
| host = EmbeddableHost.where(id: params[:id]).first | ||
| save_host(host) | ||
| end |
There was a problem hiding this comment.
Missing nil check for host in update action.
If params[:id] doesn't match any record, host will be nil, causing a NoMethodError in save_host when accessing host.host=.
Proposed fix
def update
host = EmbeddableHost.where(id: params[:id]).first
+ return render_json_error(I18n.t('not_found')) unless host
save_host(host)
end🤖 Prompt for AI Agents
In @app/controllers/admin/embeddable_hosts_controller.rb around lines 9 - 12,
The update action calls save_host with host = EmbeddableHost.where(id:
params[:id]).first but doesn't handle the case when host is nil; add a nil check
in the update method to return early and respond appropriately (e.g., render
head :not_found or redirect with a flash error) when host is not found, and only
call save_host(host) when host is present; reference the update action and
save_host method to implement this guard.
| def destroy | ||
| host = EmbeddableHost.where(id: params[:id]).first | ||
| host.destroy | ||
| render json: success_json | ||
| end |
There was a problem hiding this comment.
Missing nil check for host in destroy action.
If the host is not found, calling destroy on nil will raise a NoMethodError.
Proposed fix
def destroy
host = EmbeddableHost.where(id: params[:id]).first
+ return render_json_error(I18n.t('not_found')) unless host
host.destroy
render json: success_json
end🤖 Prompt for AI Agents
In @app/controllers/admin/embeddable_hosts_controller.rb around lines 14 - 18,
The destroy action calls host.destroy without handling a missing record; update
the EmbeddableHostsController#destroy to fetch the record via
EmbeddableHost.where(id: params[:id]).first (or find_by) and return an
appropriate error/404 response when host is nil instead of calling destroy on
nil, otherwise call host.destroy and render success_json; ensure you reference
the same symbols (destroy, EmbeddableHost, params[:id], success_json) when
adding the nil check and response.
| def update | ||
| render_serialized(@embedding, EmbeddingSerializer, root: 'embedding', rest_serializer: true) | ||
| end |
There was a problem hiding this comment.
update action does not perform any actual update.
The action renders the same serialized data as show without processing any input parameters or persisting changes. This appears to be incomplete implementation.
| def self.record_for_host(host) | ||
| uri = URI(host) rescue nil | ||
| return false unless uri.present? | ||
|
|
||
| host = uri.host | ||
| return false unless host.present? | ||
|
|
||
| where("lower(host) = ?", host).first | ||
| end |
There was a problem hiding this comment.
Inconsistent return types and rescue modifier style.
-
The method returns
falseon failure but anEmbeddableHostinstance (ornil) on success. This mixed return type can lead to subtle bugs sincehost_allowed?calls.present?which works, but callers expecting a record may not handlefalsecorrectly. -
RuboCop flags the inline
rescue nilas it silently swallows all exceptions, making debugging difficult.
Suggested fix
def self.record_for_host(host)
- uri = URI(host) rescue nil
- return false unless uri.present?
+ begin
+ uri = URI(host)
+ rescue URI::InvalidURIError
+ return nil
+ end
+ return nil unless uri.present?
- host = uri.host
- return false unless host.present?
+ parsed_host = uri.host
+ return nil unless parsed_host.present?
- where("lower(host) = ?", host).first
+ where("lower(host) = ?", parsed_host).first
end🧰 Tools
🪛 RuboCop (1.82.1)
[convention] 11-11: Avoid using rescue in its modifier form.
(Style/RescueModifier)
🤖 Prompt for AI Agents
In @app/models/embeddable_host.rb around lines 10 - 18, The method
record_for_host currently uses an inline rescue modifier and returns false on
failure which mixes return types; change it to return nil on failure and replace
the `rescue nil` with an explicit begin/rescue that only rescues
URI::InvalidURIError (or StandardError if necessary) so exceptions aren't
silently swallowed; extract the hostname via URI.parse(host).host (or check host
presence first), normalize it (e.g., downcase) and then perform the query using
where(...).first; ensure callers (e.g., host_allowed?) expect nil rather than
false.
| records.each do |h| | ||
| execute "INSERT INTO embeddable_hosts (host, category_id, created_at, updated_at) VALUES ('#{h}', #{category_id}, CURRENT_TIMESTAMP, CURRENT_TIMESTAMP)" | ||
| end |
There was a problem hiding this comment.
SQL injection vulnerability: host value is directly interpolated.
The host value h from site_settings is directly interpolated into the SQL string without sanitization. While this is a migration operating on internal data, it's still a security risk if the embeddable_hosts setting ever contained malicious content.
🔒 Proposed fix: use parameterized query
records.each do |h|
- execute "INSERT INTO embeddable_hosts (host, category_id, created_at, updated_at) VALUES ('#{h}', #{category_id}, CURRENT_TIMESTAMP, CURRENT_TIMESTAMP)"
+ execute sanitize_sql_array(["INSERT INTO embeddable_hosts (host, category_id, created_at, updated_at) VALUES (?, ?, CURRENT_TIMESTAMP, CURRENT_TIMESTAMP)", h, category_id])
endAlternatively, use ActiveRecord's quote method:
execute "INSERT INTO embeddable_hosts (host, category_id, created_at, updated_at) VALUES (#{quote(h)}, #{category_id}, CURRENT_TIMESTAMP, CURRENT_TIMESTAMP)"🤖 Prompt for AI Agents
In @db/migrate/20150818190757_create_embeddable_hosts.rb around lines 24 - 26,
The migration directly interpolates the host string inside the raw SQL in the
records.each do |h| loop (the execute call), exposing an SQL injection risk;
change the insert to use a parameterized/quoted value instead of string
interpolation — e.g., use ActiveRecord's quote(h) or bind parameters when
building the INSERT in the execute call (keep the same embeddable_hosts columns
and category_id variable) so the host value is properly escaped before executing
the SQL.
| 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 |
There was a problem hiding this comment.
Avoid update! in after_build callback.
Using update! inside after_build forces an immediate database save before the fabrication completes. This can cause issues if the record isn't fully built yet. Use direct attribute assignment instead, and let Fabrication handle persistence.
Suggested fix
after_build do |cat, transients|
- cat.update!(read_restricted: true)
+ cat.read_restricted = true
cat.category_groups.build(group_id: transients[:group].id, permission_type: CategoryGroup.permission_types[:full])
end🤖 Prompt for AI Agents
In @spec/fabricators/embeddable_host_fabricator.rb around lines 23 - 26, The
after_build block currently calls cat.update! which performs an immediate DB
save; instead assign the attribute directly (set cat.read_restricted = true) and
keep building the association via cat.category_groups.build(group_id:
transients[:group].id, permission_type: CategoryGroup.permission_types[:full])
so Fabrication handles persistence; remove the update! call and use direct
attribute assignment inside the after_build callback.
| 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.
Typo: "emeddable" should be "embeddable".
Proposed fix
- describe 'with an emeddable host' do
+ describe 'with an embeddable host' do📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| describe 'with an emeddable host' do | |
| describe 'with an embeddable host' do |
🤖 Prompt for AI Agents
In @spec/models/topic_spec.rb at line 1407, The spec description contains a
typo: change the string literal in the spec block from "with an emeddable host"
to "with an embeddable host" by updating the describe block that currently reads
'with an emeddable host' to the corrected 'with an embeddable host' (leave the
surrounding describe ... do/end intact).
| test('find embedded', function(assert) { | ||
| const store = createStore(); | ||
| return store.find('fruit', 1).then(function(f) { | ||
| ok(f.get('farmer'), 'it has the embedded object'); | ||
| ok(f.get('category'), 'categories are found automatically'); | ||
| return store.find('fruit', 2).then(function(f) { | ||
| assert.ok(f.get('farmer'), 'it has the embedded object'); | ||
|
|
||
| const fruitCols = f.get('colors'); | ||
| assert.equal(fruitCols.length, 2); | ||
| assert.equal(fruitCols[0].get('id'), 1); | ||
| assert.equal(fruitCols[1].get('id'), 2); | ||
|
|
||
| assert.ok(f.get('category'), 'categories are found automatically'); | ||
| }); | ||
| }); |
There was a problem hiding this comment.
Test data mismatch: fruit id 2 does not have color_ids [1, 2].
The test fetches fruit with id: 2 (banana), but asserts that it has 2 colors with ids 1 and 2. According to the fixture data in create-pretender.js.es6:
- Fruit id 1 (apple):
color_ids: [1, 2] - Fruit id 2 (banana):
color_ids: [3]
The assertions on lines 115-117 will fail because banana only has one color (Yellow, id 3).
🐛 Proposed fix: use fruit id 1 or fix assertions
Option 1: Change to fruit id 1 (apple):
- return store.find('fruit', 2).then(function(f) {
+ return store.find('fruit', 1).then(function(f) {Option 2: Fix assertions for fruit id 2 (banana):
const fruitCols = f.get('colors');
- assert.equal(fruitCols.length, 2);
- assert.equal(fruitCols[0].get('id'), 1);
- assert.equal(fruitCols[1].get('id'), 2);
+ assert.equal(fruitCols.length, 1);
+ assert.equal(fruitCols[0].get('id'), 3);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| test('find embedded', function(assert) { | |
| const store = createStore(); | |
| return store.find('fruit', 1).then(function(f) { | |
| ok(f.get('farmer'), 'it has the embedded object'); | |
| ok(f.get('category'), 'categories are found automatically'); | |
| return store.find('fruit', 2).then(function(f) { | |
| assert.ok(f.get('farmer'), 'it has the embedded object'); | |
| const fruitCols = f.get('colors'); | |
| assert.equal(fruitCols.length, 2); | |
| assert.equal(fruitCols[0].get('id'), 1); | |
| assert.equal(fruitCols[1].get('id'), 2); | |
| assert.ok(f.get('category'), 'categories are found automatically'); | |
| }); | |
| }); | |
| test('find embedded', function(assert) { | |
| const store = createStore(); | |
| return store.find('fruit', 1).then(function(f) { | |
| assert.ok(f.get('farmer'), 'it has the embedded object'); | |
| const fruitCols = f.get('colors'); | |
| assert.equal(fruitCols.length, 2); | |
| assert.equal(fruitCols[0].get('id'), 1); | |
| assert.equal(fruitCols[1].get('id'), 2); | |
| assert.ok(f.get('category'), 'categories are found automatically'); | |
| }); | |
| }); |
| test('find embedded', function(assert) { | |
| const store = createStore(); | |
| return store.find('fruit', 1).then(function(f) { | |
| ok(f.get('farmer'), 'it has the embedded object'); | |
| ok(f.get('category'), 'categories are found automatically'); | |
| return store.find('fruit', 2).then(function(f) { | |
| assert.ok(f.get('farmer'), 'it has the embedded object'); | |
| const fruitCols = f.get('colors'); | |
| assert.equal(fruitCols.length, 2); | |
| assert.equal(fruitCols[0].get('id'), 1); | |
| assert.equal(fruitCols[1].get('id'), 2); | |
| assert.ok(f.get('category'), 'categories are found automatically'); | |
| }); | |
| }); | |
| test('find embedded', function(assert) { | |
| const store = createStore(); | |
| return store.find('fruit', 2).then(function(f) { | |
| assert.ok(f.get('farmer'), 'it has the embedded object'); | |
| const fruitCols = f.get('colors'); | |
| assert.equal(fruitCols.length, 1); | |
| assert.equal(fruitCols[0].get('id'), 3); | |
| assert.ok(f.get('category'), 'categories are found automatically'); | |
| }); | |
| }); |
🤖 Prompt for AI Agents
In @test/javascripts/models/store-test.js.es6 around lines 109 - 121, The test
named "find embedded" is asserting colors for fruit id 2 (banana) but fixture
data in create-pretender.js.es6 shows banana has color_ids [3]; update the test
to be consistent by either calling store.find('fruit', 1) to test apple (which
has color_ids [1,2]) or adjust the assertions after store.find('fruit', 2)
(inside the test('find embedded') callback) to expect a single color with id 3
(and length 1) and update the corresponding id checks and messages accordingly;
ensure createStore() usage and the assertions on f.get('colors') are changed to
match the chosen fruit.
2 participants
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