-
Notifications
You must be signed in to change notification settings - Fork 0
FEATURE: Can edit category/host relationships for embedding #10
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); | ||
| } | ||
| } | ||
| }); | ||
| 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 | ||||||||||||||||||||||||||||||||||||||||||||||||
|
Comment on lines
+9
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. Add error handling for missing records. The update and destroy actions don't handle cases where the embeddable host record doesn't exist. Using Apply this diff to add proper error handling: def update
host = EmbeddableHost.where(id: params[:id]).first
+ return render_json_error("Not found", status: 404) unless host
save_host(host)
end
def destroy
host = EmbeddableHost.where(id: params[:id]).first
+ return render_json_error("Not found", status: 404) unless host
host.destroy
render json: success_json
endAlternatively, use def update
- host = EmbeddableHost.where(id: params[:id]).first
+ host = EmbeddableHost.find(params[:id])
save_host(host)
end
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 | ||||||||||||||||||||||||||||||||||||||||||||||||
|
Comment on lines
+22
to
+32
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. 🛠️ Refactor suggestion Consider using strong parameters for better security. The Apply this diff to use strong parameters: protected
def save_host(host)
- host.host = params[:embeddable_host][:host]
- host.category_id = params[:embeddable_host][:category_id]
+ permitted_params = params.require(:embeddable_host).permit(:host, :category_id)
+ host.host = permitted_params[:host]
+ host.category_id = permitted_params[: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
endOr more concisely using def save_host(host)
+ permitted_params = params.require(:embeddable_host).permit(:host, :category_id)
- host.host = params[:embeddable_host][:host]
- host.category_id = params[:embeddable_host][:category_id]
+ host.assign_attributes(permitted_params)
host.category_id = SiteSetting.uncategorized_category_id if host.category_id.blank?📝 Committable suggestion
Suggested change
🤖 Prompt for AI Agents |
||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||
| 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. Update action doesn't perform any updates. The Consider implementing actual update logic or removing this action if updates aren't needed: def update
+ # Add update logic here if needed, e.g.:
+ # @embedding.some_property = params[:some_param]
+ # @embedding.save
render_serialized(@embedding, EmbeddingSerializer, root: 'embedding', rest_serializer: true)
endOr if no updates are needed, consider removing the update route and action entirely. 🤖 Prompt for AI Agents |
||||||
|
|
||||||
| protected | ||||||
|
|
||||||
| def fetch_embedding | ||||||
| @embedding = OpenStruct.new({ | ||||||
| id: 'default', | ||||||
| embeddable_hosts: EmbeddableHost.all.order(:host) | ||||||
|
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. 🛠️ Refactor suggestion Database query lacks error handling. The Consider adding error handling and possibly scoping: - embeddable_hosts: EmbeddableHost.all.order(:host)
+ embeddable_hosts: EmbeddableHost.all.order(:host) rescue []Or better yet, move this to a service method with proper error handling. 📝 Committable suggestion
Suggested change
🤖 Prompt for AI Agents |
||||||
| }) | ||||||
| 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 |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -33,12 +33,14 @@ def self.import(user, url, title, contents) | |
| # If there is no embed, create a topic, post and the embed. | ||
| if embed.blank? | ||
| Topic.transaction do | ||
| eh = EmbeddableHost.record_for_host(url) | ||
|
|
||
| creator = PostCreator.new(user, | ||
| title: title, | ||
| raw: absolutize_urls(url, contents), | ||
| skip_validations: true, | ||
| cook_method: Post.cook_methods[:raw_html], | ||
| category: SiteSetting.embed_category) | ||
| category: eh.try(:category_id)) | ||
|
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. 💡 Verification agent 🧩 Analysis chainVerify fallback behavior when no embeddable host is found. The safe navigation 🏁 Script executed: #!/bin/bash
# Description: Check how PostCreator handles nil category parameter
ast-grep --pattern 'class PostCreator
$$$
def initialize($_, $options)
$$$
end
$$$'Length of output: 97 🏁 Script executed: #!/bin/bash
# Locate the PostCreator class and inspect its initialize method
rg -n "class PostCreator" -A30Length of output: 2423 🏁 Script executed: #!/bin/bash
# Extract initialize method and inspect category handling in PostCreator
rg -n "def initialize" -A20 lib/post_creator.rb
rg -n ":category" -A5 -B5 lib/post_creator.rbLength of output: 782 🏁 Script executed: #!/bin/bash
# Inspect PostCreator#create to see how category is handled
rg -n "def create" -A30 lib/post_creator.rb
rg -n "opts\\[:category" -A5 lib/post_creator.rbLength of output: 3211 🏁 Script executed: #!/bin/bash
# Inspect the TopicEmbed class and the use of eh.try(:category_id)
rg -n "class TopicEmbed" -A200 app/models/topic_embed.rb
rg -n "eh.try" -A20 app/models/topic_embed.rbLength of output: 6907 🏁 Script executed: #!/bin/bash
# Locate and inspect the TopicCreator class to see how :category is handled
rg -l "class TopicCreator" -g "*.rb"
rg -n "class TopicCreator" -A200
rg -n "opts\[:category" -A5Length of output: 13259 Ensure a valid category fallback for embedded topics Currently in • In - creator = PostCreator.new(user,
- title: title,
- raw: absolutize_urls(url, contents),
- skip_validations: true,
- cook_method: Post.cook_methods[:raw_html],
- category: eh.try(:category_id))
+ creator = PostCreator.new(user,
+ title: title,
+ raw: absolutize_urls(url, contents),
+ skip_validations: true,
+ cook_method: Post.cook_methods[:raw_html],
+ category: eh.try(:category_id) || SiteSetting.default_category)• Or, in def find_category
# existing logic…
category = if @opts[:category].is_a?(Integer) || @opts[:category] =~ /^\d+$/
Category.find_by(id: @opts[:category])
else
Category.find_by(name_lower: @opts[:category].try(:downcase))
end
category || Category.find_by(id: SiteSetting.default_category)
endThis ensures embedded topics always land in a valid category. 🤖 Prompt for AI Agents |
||
| post = creator.create | ||
| if post.present? | ||
| TopicEmbed.create!(topic_id: post.topic_id, | ||
|
|
||
| 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.
🛠️ Refactor suggestion
Consider destroying the host record instead of just removing from collection.
The
deleteHostaction only removes the host from the local collection but doesn't destroy the record on the server. This could lead to data inconsistency.deleteHost(host) { - this.get('embedding.embeddable_hosts').removeObject(host); + host.destroy().then(() => { + this.get('embedding.embeddable_hosts').removeObject(host); + }).catch(error => { + // Handle error appropriately + console.error('Failed to delete host:', error); + }); }🤖 Prompt for AI Agents