-
Notifications
You must be signed in to change notification settings - Fork 0
Automated Test: rest-serializer-enhancement-post #332
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); | ||
| } | ||
| } | ||
| }); |
| 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 | ||||||||||||||||||||||
|
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 chain🏁 Script executed: # First, find the Admin::AdminController file
fd -t f "*admin_controller*" --max-one-result | head -20Repository: admin-coderabbit/coderabbit-discourse-mirror Length of output: 321 🏁 Script executed: # Search for Admin::AdminController class definition
rg -n "class Admin::AdminController" --type rubyRepository: admin-coderabbit/coderabbit-discourse-mirror Length of output: 184 🏁 Script executed: # Check the embeddable_hosts_controller specifically
cat -n app/controllers/admin/embeddable_hosts_controller.rbRepository: admin-coderabbit/coderabbit-discourse-mirror Length of output: 1145 🏁 Script executed: # Check the Admin::AdminController for before_filter/before_action
cat -n app/controllers/admin/admin_controller.rb | head -50Repository: admin-coderabbit/coderabbit-discourse-mirror Length of output: 324 🏁 Script executed: # Search for all before_filter/before_action in admin_controller
rg -n "before_filter|before_action" app/controllers/admin/admin_controller.rbRepository: admin-coderabbit/coderabbit-discourse-mirror Length of output: 155 🏁 Script executed: # Check Rails version in the project
cat Gemfile | grep -i railsRepository: admin-coderabbit/coderabbit-discourse-mirror Length of output: 963 Remove redundant
🤖 Prompt for AI Agents |
||||||||||||||||||||||
|
|
||||||||||||||||||||||
| def create | ||||||||||||||||||||||
| save_host(EmbeddableHost.new) | ||||||||||||||||||||||
| end | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
| def update | ||||||||||||||||||||||
| host = EmbeddableHost.where(id: params[:id]).first | ||||||||||||||||||||||
| save_host(host) | ||||||||||||||||||||||
| end | ||||||||||||||||||||||
|
Comment on lines
+9
to
+12
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. Missing nil guard —
Use Proposed fix def update
- host = EmbeddableHost.where(id: params[:id]).first
+ host = EmbeddableHost.find(params[:id])
save_host(host)
end🤖 Prompt for AI Agents |
||||||||||||||||||||||
|
|
||||||||||||||||||||||
| def destroy | ||||||||||||||||||||||
| host = EmbeddableHost.where(id: params[:id]).first | ||||||||||||||||||||||
| host.destroy | ||||||||||||||||||||||
| render json: success_json | ||||||||||||||||||||||
| end | ||||||||||||||||||||||
|
Comment on lines
+14
to
+18
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. Same nil guard issue in
Proposed fix def destroy
- host = EmbeddableHost.where(id: params[:id]).first
+ host = EmbeddableHost.find(params[:id])
host.destroy
render json: success_json
end📝 Committable suggestion
Suggested change
🤖 Prompt for AI Agents |
||||||||||||||||||||||
|
|
||||||||||||||||||||||
| 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 | ||||||||||||||||||||||
| 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 | ||
|
Comment on lines
+9
to
+11
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.
The action re-renders the current embedding state without reading or persisting any submitted data. If the intent is to update embedding-level settings in the future, this is a placeholder; if individual host CRUD is entirely handled via 🤖 Prompt for AI Agents |
||
|
|
||
| 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 | ||||||||||||||||||||
|
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. TLD regex caps at 5 characters, excluding valid TLDs like The regex 🤖 Prompt for AI Agents |
||||||||||||||||||||
| belongs_to :category | ||||||||||||||||||||
|
|
||||||||||||||||||||
| before_validation do | ||||||||||||||||||||
| self.host.sub!(/^https?:\/\//, '') | ||||||||||||||||||||
| self.host.sub!(/\/.*$/, '') | ||||||||||||||||||||
| end | ||||||||||||||||||||
|
Comment on lines
+5
to
+8
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.
The Proposed fix before_validation do
+ return if self.host.blank?
self.host.sub!(/^https?:\/\//, '')
self.host.sub!(/\/.*$/, '')
end📝 Committable suggestion
Suggested change
🤖 Prompt for AI Agents |
||||||||||||||||||||
|
|
||||||||||||||||||||
| 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 | ||||||||||||||||||||
| 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.
Plural branch is less defensive than the singular branch.
A few concerns with the
_idspath:No guard on
obj[k]— if a key ends in_idsbut the value isnullor not an array,obj[k].map(...)(line 197) will throw aTypeError. The singular branch safely checksif (hydrated)before mutating.hydrated || []on line 200 is unreachable —.map()always returns an array, so the fallback never triggers.Asymmetric delete semantics — the plural branch unconditionally deletes
obj[k]and assigns the hydrated array (which may containundefinedentries if lookups fail), while the singular branch only replaces when hydration succeeds. This means a failed plural lookup silently loses the original IDs.Suggested defensive fix
if (m[2]) { - const hydrated = obj[k].map(function(id) { - return self._lookupSubType(subType, type, id, root); - }); - obj[self.pluralize(subType)] = hydrated || []; - delete obj[k]; + const ids = obj[k]; + if (Array.isArray(ids)) { + const hydrated = ids.map(function(id) { + return self._lookupSubType(subType, type, id, root); + }); + obj[self.pluralize(subType)] = hydrated; + delete obj[k]; + } } else {🤖 Prompt for AI Agents