-
Notifications
You must be signed in to change notification settings - Fork 9
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')); | ||
|
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. logic: Potential null reference error if |
||
| 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'))); | ||
|
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. style: The manual category assignment after save may cause synchronization issues. Consider letting the server response handle category updates. |
||
| 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); | ||
|
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. logic: This only removes the host from the local array. For persisted records, you should call |
||
| } | ||
| } | ||
| }); | ||
| 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 | ||||
|---|---|---|---|---|---|---|
|
|
@@ -189,14 +189,24 @@ export default Ember.Object.extend({ | |||||
| _hydrateEmbedded(type, obj, root) { | ||||||
| const self = this; | ||||||
| Object.keys(obj).forEach(function(k) { | ||||||
| const m = /(.+)\_id$/.exec(k); | ||||||
| const m = /(.+)\_id(s?)$/.exec(k); | ||||||
| if (m) { | ||||||
| const subType = m[1]; | ||||||
| const hydrated = self._lookupSubType(subType, type, obj[k], root); | ||||||
| if (hydrated) { | ||||||
| obj[subType] = hydrated; | ||||||
|
|
||||||
| if (m[2]) { | ||||||
| const hydrated = obj[k].map(function(id) { | ||||||
| return self._lookupSubType(subType, type, id, root); | ||||||
| }); | ||||||
| obj[self.pluralize(subType)] = hydrated || []; | ||||||
|
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. style: the
Suggested change
|
||||||
| delete obj[k]; | ||||||
| } else { | ||||||
| const hydrated = self._lookupSubType(subType, type, obj[k], root); | ||||||
| if (hydrated) { | ||||||
| obj[subType] = hydrated; | ||||||
| delete obj[k]; | ||||||
| } | ||||||
| } | ||||||
|
|
||||||
| } | ||||||
| }); | ||||||
| }, | ||||||
|
|
||||||
| 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 |
| 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. logic: Update method doesn't actually perform any updates - it just renders the same data as show method |
||
|
|
||
| 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 |
|---|---|---|
|
|
@@ -58,8 +58,7 @@ def count | |
| def ensure_embeddable | ||
|
|
||
| if !(Rails.env.development? && current_user.try(:admin?)) | ||
| raise Discourse::InvalidAccess.new('embeddable hosts not set') if SiteSetting.embeddable_hosts.blank? | ||
| raise Discourse::InvalidAccess.new('invalid referer host') unless SiteSetting.allows_embeddable_host?(request.referer) | ||
| raise Discourse::InvalidAccess.new('invalid referer host') unless EmbeddableHost.host_allowed?(request.referer) | ||
|
Comment on lines
60
to
+61
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. logic: The removal of the blank check for |
||
| end | ||
|
|
||
| response.headers['X-Frame-Options'] = "ALLOWALL" | ||
|
|
||
| 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!(/\/.*$/, '') | ||||||||||||
|
Comment on lines
+6
to
+7
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. logic: Calling sub! on potentially nil host will raise NoMethodError. Add nil check:
Suggested change
|
||||||||||||
| 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. style: Consider fallback to
Suggested change
|
||||||
| 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 | ||
|
Comment on lines
+4
to
+14
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. style: These method definitions are redundant. ActiveModel::Serializer automatically creates attribute methods that return |
||
| 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 | ||||||||||||||||
|
Comment on lines
+5
to
+7
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. style: the custom
Suggested change
|
||||||||||||||||
| 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.
logic: The pathFor method ignores all parameters (store, type, findArgs) that are typically used for dynamic path generation. This could break CRUD operations that need to append IDs for individual resource access.