-
Notifications
You must be signed in to change notification settings - Fork 0
FEATURE: Can edit category/host relationships for embedding #1
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: rest-serializer-enhancement-pre
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,7 @@ | ||
| import RestAdapter from 'discourse/adapters/rest'; | ||
|
|
||
| export default RestAdapter.extend({ | ||
| pathFor() { | ||
| return "/admin/customize/embedding"; | ||
| } | ||
| }); |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,63 @@ | ||
| import { bufferedProperty } from 'discourse/mixins/buffered-content'; | ||
| import computed from 'ember-addons/ember-computed-decorators'; | ||
| import { on, observes } from 'ember-addons/ember-computed-decorators'; | ||
| import { popupAjaxError } from 'discourse/lib/ajax-error'; | ||
|
|
||
| export default Ember.Component.extend(bufferedProperty('host'), { | ||
| editToggled: false, | ||
| tagName: 'tr', | ||
| categoryId: null, | ||
|
|
||
| editing: Ember.computed.or('host.isNew', 'editToggled'), | ||
|
|
||
| @on('didInsertElement') | ||
| @observes('editing') | ||
| _focusOnInput() { | ||
| Ember.run.schedule('afterRender', () => { this.$('.host-name').focus(); }); | ||
| }, | ||
|
|
||
| @computed('buffered.host', 'host.isSaving') | ||
| cantSave(host, isSaving) { | ||
| return isSaving || Ember.isEmpty(host); | ||
| }, | ||
|
|
||
| actions: { | ||
| edit() { | ||
| this.set('categoryId', this.get('host.category.id')); | ||
| this.set('editToggled', true); | ||
| }, | ||
|
|
||
| save() { | ||
| if (this.get('cantSave')) { return; } | ||
|
|
||
| const props = this.get('buffered').getProperties('host'); | ||
| props.category_id = this.get('categoryId'); | ||
|
|
||
| const host = this.get('host'); | ||
| host.save(props).then(() => { | ||
| host.set('category', Discourse.Category.findById(this.get('categoryId'))); | ||
| this.set('editToggled', false); | ||
| }).catch(popupAjaxError); | ||
| }, | ||
|
|
||
| delete() { | ||
| bootbox.confirm(I18n.t('admin.embedding.confirm_delete'), (result) => { | ||
| if (result) { | ||
| this.get('host').destroyRecord().then(() => { | ||
| this.sendAction('deleteHost', this.get('host')); | ||
| }); | ||
| } | ||
| }); | ||
| }, | ||
|
|
||
| cancel() { | ||
| const host = this.get('host'); | ||
| if (host.get('isNew')) { | ||
| this.sendAction('deleteHost', host); | ||
| } else { | ||
| this.rollbackBuffer(); | ||
| this.set('editToggled', false); | ||
| } | ||
| } | ||
| } | ||
| }); | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,18 @@ | ||
| export default Ember.Controller.extend({ | ||
| embedding: null, | ||
|
|
||
| actions: { | ||
| saveChanges() { | ||
| this.get('embedding').update({}); | ||
| }, | ||
|
|
||
| addHost() { | ||
| const host = this.store.createRecord('embeddable-host'); | ||
| this.get('embedding.embeddable_hosts').pushObject(host); | ||
| }, | ||
|
|
||
| deleteHost(host) { | ||
| this.get('embedding.embeddable_hosts').removeObject(host); | ||
| } | ||
|
Comment on lines
+14
to
+16
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🧩 Analysis chainVerify deletion handling for persisted hosts. The 🏁 Script executed: #!/bin/bash
# Description: Check how the backend handles embeddable_hosts updates
# Find the embedding controller update action
rg -A 30 "def update" app/controllers/admin/embedding_controller.rb
# Check embedding serializer for how it handles has_many
rg -A 10 "embeddable_hosts" app/serializers/embedding_serializer.rbLength of output: 405 🏁 Script executed: #!/bin/bash
# Get the full embedding controller to see complete update method and params handling
cat app/controllers/admin/embedding_controller.rb
# Check for strong parameters and param handling
rg -n "params\|permit\|require" app/controllers/admin/embedding_controller.rb
# Look for any Rails concerns or mixins that might handle updates
rg -n "include|extend" app/controllers/admin/embedding_controller.rbLength of output: 533 🏁 Script executed: #!/bin/bash
# Check how the EmbeddableHost model handles associations and deletion
cat app/models/embeddable_host.rb
# Look for embedding associations
rg -B5 -A5 "embeddable_host" app/models/embedding.rbLength of output: 625 🏁 Script executed: #!/bin/bash
# Check if there's an embeddings resource route that might define update behavior
rg -A2 "embeddable.*host" config/routes.rbLength of output: 129 🏁 Script executed: #!/bin/bash
# Find the saveChanges method in the JavaScript file to see what API calls are made
rg -B5 -A10 "saveChanges" app/assets/javascripts/admin/controllers/admin-embedding.js.es6
# Look for any controller action that handles embeddable_hosts updates
cat app/controllers/admin/embeddable_hosts_controller.rb 2>/dev/null || echo "Controller not found"
# Check if there's a separate serializer or method handling host changes
rg -n "embeddable_hosts" app/controllers/admin/*.rbLength of output: 1306 🏁 Script executed: #!/bin/bash
# Look for any API structure or endpoint handling in the embedding controller more broadly
rg -B3 -A15 "class Admin::EmbeddingController" app/controllers/admin/embedding_controller.rb
# Check if there's a concern or service handling the deletion logic
fd -e rb app/controllers/concerns/ -o app/services/ | xargs rg -l "embeddable" 2>/dev/null || trueLength of output: 1630 Deletion does not persist to the database. The The To persist deletions, either:
|
||
| } | ||
| }); | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,9 @@ | ||
| export default Ember.Route.extend({ | ||
| model() { | ||
| return this.store.find('embedding'); | ||
| }, | ||
|
|
||
| setupController(controller, model) { | ||
| controller.set('embedding', model); | ||
| } | ||
| }); |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,19 @@ | ||
| {{#if editing}} | ||
| <td> | ||
| {{input value=buffered.host placeholder="example.com" enter="save" class="host-name"}} | ||
| </td> | ||
| <td> | ||
| {{category-chooser value=categoryId}} | ||
| </td> | ||
| <td> | ||
| {{d-button icon="check" action="save" class="btn-primary" disabled=cantSave}} | ||
| {{d-button icon="times" action="cancel" class="btn-danger" disabled=host.isSaving}} | ||
| </td> | ||
| {{else}} | ||
| <td>{{host.host}}</td> | ||
| <td>{{category-badge host.category}}</td> | ||
| <td> | ||
| {{d-button icon="pencil" action="edit"}} | ||
| {{d-button icon="trash-o" action="delete" class='btn-danger'}} | ||
| </td> | ||
| {{/if}} |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,15 @@ | ||
| {{#if embedding.embeddable_hosts}} | ||
| <table> | ||
| <tr> | ||
| <th style='width: 50%'>{{i18n "admin.embedding.host"}}</th> | ||
| <th style='width: 30%'>{{i18n "admin.embedding.category"}}</th> | ||
| <th style='width: 20%'> </th> | ||
| </tr> | ||
| {{#each embedding.embeddable_hosts as |host|}} | ||
| {{embeddable-host host=host deleteHost="deleteHost"}} | ||
| {{/each}} | ||
| </table> | ||
| {{/if}} | ||
|
|
||
| {{d-button label="admin.embedding.add_host" action="addHost" icon="plus" class="btn-primary"}} | ||
|
|
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| @@ -0,0 +1,34 @@ | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| class Admin::EmbeddableHostsController < Admin::AdminController | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| before_filter :ensure_logged_in, :ensure_staff | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| def create | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| save_host(EmbeddableHost.new) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| end | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| def update | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| host = EmbeddableHost.where(id: params[:id]).first | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| save_host(host) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| end | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| def destroy | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| host = EmbeddableHost.where(id: params[:id]).first | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| host.destroy | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| render json: success_json | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| end | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| protected | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| def save_host(host) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| host.host = params[:embeddable_host][:host] | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| host.category_id = params[:embeddable_host][:category_id] | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| host.category_id = SiteSetting.uncategorized_category_id if host.category_id.blank? | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| if host.save | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| render_serialized(host, EmbeddableHostSerializer, root: 'embeddable_host', rest_serializer: true) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| else | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| render_json_error(host) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| end | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| end | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| end | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
Comment on lines
+1
to
+34
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🧩 Analysis chainHandle missing embeddable hosts in update/destroy Both
Safer pattern: def update
- host = EmbeddableHost.where(id: params[:id]).first
- save_host(host)
+ host = EmbeddableHost.find_by(id: params[:id])
+ return render_not_found unless host
+ save_host(host)
end
def destroy
- host = EmbeddableHost.where(id: params[:id]).first
- host.destroy
- render json: success_json
+ host = EmbeddableHost.find_by(id: params[:id])
+ return render_not_found unless host
+ host.destroy
+ render json: success_json
endThis avoids 500s on bad IDs and returns a clean 404 instead. 🏁 Script executed: #!/bin/bash
# Check for existing patterns of render_not_found usage in admin controllers.
rg -n "render_not_found" app/controllers/adminLength of output: 0 I need to verify whether the proposed pattern is available in this codebase. Let me search more comprehensively. Length of output: 26 🏁 Script executed: #!/bin/bash
# Check what error handling patterns ARE used in admin controllers
rg -n -A2 "\.where\(id:" app/controllers/admin --type rb | head -40Length of output: 26 🏁 Script executed: #!/bin/bash
# Check the Admin::AdminController parent class for available methods
find app/controllers -name "*admin_controller.rb" | head -5Length of output: 41 🏁 Script executed: #!/bin/bash
# Search for render_not_found anywhere in codebase
rg -n "render_not_found"Length of output: 0 🏁 Script executed: #!/bin/bash
# Check what error handling patterns exist in admin controllers
rg -n -A2 "\.where\(id:" app/controllers/admin | head -50Length of output: 1658 🏁 Script executed: #!/bin/bash
# Inspect Admin::AdminController for available methods
cat app/controllers/admin/admin_controller.rbLength of output: 166 🏁 Script executed: #!/bin/bash
# Check what methods ARE used in codebase to handle missing/not found cases
rg -n "render.*json.*error|render.*error|head" app/controllers/admin | head -20Length of output: 2295 🏁 Script executed: #!/bin/bash
# Look for how other update/destroy methods handle nil cases
rg -n -B2 "destroy if.*present" app/controllers/adminLength of output: 261 Handle missing embeddable hosts in update/destroy Both
The codebase pattern (see def update
host = EmbeddableHost.where(id: params[:id]).first
+ return render_json_error(I18n.t('not_found')) unless host
save_host(host)
end
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
endThis prevents 500 errors on missing IDs and returns proper error feedback. 📝 Committable suggestion
Suggested change
🤖 Prompt for AI Agents |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,21 @@ | ||
| class Admin::EmbeddingController < Admin::AdminController | ||
|
|
||
| before_filter :ensure_logged_in, :ensure_staff, :fetch_embedding | ||
|
|
||
| def show | ||
| render_serialized(@embedding, EmbeddingSerializer, root: 'embedding', rest_serializer: true) | ||
| end | ||
|
|
||
| def update | ||
| render_serialized(@embedding, EmbeddingSerializer, root: 'embedding', rest_serializer: true) | ||
| end | ||
|
|
||
| protected | ||
|
|
||
| def fetch_embedding | ||
| @embedding = OpenStruct.new({ | ||
| id: 'default', | ||
| embeddable_hosts: EmbeddableHost.all.order(:host) | ||
| }) | ||
| end | ||
| end |
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| @@ -0,0 +1,24 @@ | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| class EmbeddableHost < ActiveRecord::Base | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| validates_format_of :host, :with => /\A[a-z0-9]+([\-\.]{1}[a-z0-9]+)*\.[a-z]{2,5}(:[0-9]{1,5})?(\/.*)?\Z/i | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| belongs_to :category | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| before_validation do | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| self.host.sub!(/^https?:\/\//, '') | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| self.host.sub!(/\/.*$/, '') | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| end | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| 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 | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| def self.host_allowed?(host) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| record_for_host(host).present? | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| end | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| end | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
Comment on lines
+1
to
+24
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🧩 Analysis chainGuard against nil host and tighten host lookup/normalization Two correctness issues and one style concern here:
A small refactor fixes all three: - before_validation do
- self.host.sub!(/^https?:\/\//, '')
- self.host.sub!(/\/.*$/, '')
- end
+ before_validation do
+ return if self.host.blank?
+
+ normalized = self.host.sub(/^https?:\/\//, '').sub(/\/.*$/, '')
+ self.host = normalized.downcase
+ end
@@
- 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
+ def self.record_for_host(host)
+ uri =
+ begin
+ URI(host)
+ rescue URI::InvalidURIError, ArgumentError
+ nil
+ end
+ return false unless uri && uri.host.present?
+
+ where("lower(host) = ?", uri.host.downcase).first
+ endThis keeps the public behavior (returns a record or 🏁 Script executed: #!/bin/bash
# Quick verification: search for other direct writes to EmbeddableHost#host
rg -n "EmbeddableHost" app specLength of output: 2439 🏁 Script executed: #!/bin/bash
# Inspect the full spec file to understand test expectations and current behavior
cat spec/models/embeddable_host_spec.rbLength of output: 1162 🏁 Script executed: #!/bin/bash
# Check the admin controller to see how EmbeddableHost is created
cat app/controllers/admin/embeddable_hosts_controller.rbLength of output: 819 🏁 Script executed: #!/bin/bash
# Check if there's a form or API that could pass nil host
rg -n "params\[" app/controllers/admin/embeddable_hosts_controller.rb -A 2 -B 2Length of output: 490 Guard against nil host and tighten host lookup/normalization Two correctness issues and one style concern here:
A small refactor fixes all three: - before_validation do
- self.host.sub!(/^https?:\/\//, '')
- self.host.sub!(/\/.*$/, '')
- end
+ before_validation do
+ return if self.host.blank?
+
+ normalized = self.host.sub(/^https?:\/\//, '').sub(/\/.*$/, '')
+ self.host = normalized.downcase
+ end
@@
- 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
+ def self.record_for_host(host)
+ uri =
+ begin
+ URI(host)
+ rescue URI::InvalidURIError, ArgumentError
+ nil
+ end
+ return false unless uri && uri.host.present?
+
+ where("lower(host) = ?", uri.host.downcase).first
+ endThis keeps the public behavior (returns a record or 📝 Committable suggestion
Suggested change
🧰 Tools🪛 RuboCop (1.81.7)[convention] 11-11: Avoid using (Style/RescueModifier) 🤖 Prompt for AI Agents |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,16 @@ | ||
| 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 | ||
|
|
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,8 @@ | ||
| class EmbeddingSerializer < ApplicationSerializer | ||
| attributes :id | ||
| has_many :embeddable_hosts, serializer: EmbeddableHostSerializer, embed: :ids | ||
|
|
||
| def id | ||
| object.id | ||
| end | ||
| end |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🧩 Analysis chain
Component logic looks solid; optionally guard focus on non-editing
The buffered editing,
cantSavelogic, save/delete/cancel flows, and category handling all look consistent with the template and backend API.Minor optional tweak:
_focusOnInputcurrently runs on everyeditingchange, including when it becomes false. You can avoid a no‑op focus call by short‑circuiting:🏁 Script executed:
Length of output: 494
Length of output: 2911
Length of output: 1877
Align focus guard with established pattern in similar components
The buffered editing,
cantSavelogic, save/delete/cancel flows, and category handling all look solid. However, the_focusOnInputmethod should include the editing guard shown inadmin-user-field-item.js.es6, which uses the same bufferedProperty pattern.Currently,
_focusOnInputruns on everyeditingchange, including when it becomes false, causing unnecessary DOM operations. The established pattern in similar components is:🤖 Prompt for AI Agents