From fa74e399db5cd225c004337c3946921b5c82b11b Mon Sep 17 00:00:00 2001 From: Jean Perez Date: Fri, 3 May 2024 18:50:14 -0400 Subject: [PATCH 01/17] FEATURE: extend embeddable hosts by specifying tags and users --- .../addon/components/embeddable-host.hbs | 32 +++++++++++++++++++ .../admin/addon/components/embeddable-host.js | 14 +++++++- .../admin/addon/templates/embedding.hbs | 8 +++-- .../admin/embeddable_hosts_controller.rb | 6 ++++ app/models/embeddable_host.rb | 3 ++ app/models/embeddable_host_tag.rb | 10 ++++++ app/models/tag.rb | 3 ++ app/models/topic_embed.rb | 9 ++++++ app/models/user.rb | 1 + app/serializers/embeddable_host_serializer.rb | 10 +++++- ...0051551_add_user_id_to_embeddable_hosts.rb | 6 ++++ ...40430052017_create_embeddable_host_tags.rb | 15 +++++++++ spec/models/embeddable_host_tag_spec.rb | 6 ++++ 13 files changed, 118 insertions(+), 5 deletions(-) create mode 100644 app/models/embeddable_host_tag.rb create mode 100644 db/migrate/20240430051551_add_user_id_to_embeddable_hosts.rb create mode 100644 db/migrate/20240430052017_create_embeddable_host_tags.rb create mode 100644 spec/models/embeddable_host_tag_spec.rb diff --git a/app/assets/javascripts/admin/addon/components/embeddable-host.hbs b/app/assets/javascripts/admin/addon/components/embeddable-host.hbs index 9ab723092fcc..ae7e95ca6dd3 100644 --- a/app/assets/javascripts/admin/addon/components/embeddable-host.hbs +++ b/app/assets/javascripts/admin/addon/components/embeddable-host.hbs @@ -26,6 +26,30 @@ class="small" /> + +
{{i18n "admin.embedding.category"}}
+ + + +
{{i18n "admin.embedding.category"}}
+ + {{i18n "admin.embedding.category"}} {{category-badge this.category allowUncategorized=true}} + +
{{i18n "admin.embedding.tags"}}
+ {{this.tags}} + + +
{{i18n "admin.embedding.tags"}}
+ {{this.user}} + diff --git a/app/assets/javascripts/admin/addon/components/embeddable-host.js b/app/assets/javascripts/admin/addon/components/embeddable-host.js index 12bd423983d0..6781a601a451 100644 --- a/app/assets/javascripts/admin/addon/components/embeddable-host.js +++ b/app/assets/javascripts/admin/addon/components/embeddable-host.js @@ -18,6 +18,8 @@ export default class EmbeddableHost extends Component.extend( editToggled = false; categoryId = null; category = null; + tags = null; + user = null; @or("host.isNew", "editToggled") editing; @@ -27,8 +29,12 @@ export default class EmbeddableHost extends Component.extend( const host = this.host; const categoryId = host.category_id || this.site.uncategorized_category_id; const category = Category.findById(categoryId); + const tags = host.tags || []; + const user = host.user || ""; this.set("category", category); + this.set("tags", tags); + this.set("user", user); } @discourseComputed("buffered.host", "host.isSaving") @@ -40,7 +46,10 @@ export default class EmbeddableHost extends Component.extend( edit() { this.set("editToggled", true); } - + @action + onUserChange(user) { + this.set("user", user); + } @action save() { if (this.cantSave) { @@ -53,6 +62,9 @@ export default class EmbeddableHost extends Component.extend( "class_name" ); props.category_id = this.category.id; + props.tags = this.tags; + props.user = + Array.isArray(this.user) && this.user.length > 0 ? this.user[0] : ""; const host = this.host; diff --git a/app/assets/javascripts/admin/addon/templates/embedding.hbs b/app/assets/javascripts/admin/addon/templates/embedding.hbs index f9301df3a9e8..a9af120ea574 100644 --- a/app/assets/javascripts/admin/addon/templates/embedding.hbs +++ b/app/assets/javascripts/admin/addon/templates/embedding.hbs @@ -2,9 +2,11 @@ {{#if this.embedding.embeddable_hosts}} - - - + + + + + diff --git a/app/controllers/admin/embeddable_hosts_controller.rb b/app/controllers/admin/embeddable_hosts_controller.rb index 56b44de378f3..8eb76a682cc1 100644 --- a/app/controllers/admin/embeddable_hosts_controller.rb +++ b/app/controllers/admin/embeddable_hosts_controller.rb @@ -28,6 +28,12 @@ def save_host(host, action) host.category_id = params[:embeddable_host][:category_id] host.category_id = SiteSetting.uncategorized_category_id if host.category_id.blank? + # Handle tags + username = params[:embeddable_host][:user] + tags = params[:embeddable_host][:tags]&.map(&:strip) + host.tags = Tag.where(name: tags) if tags.present? + host.user = User.find_by_username(username) if username.present? + if host.save changes = host.saved_changes if action == :update StaffActionLogger.new(current_user).log_embeddable_host( diff --git a/app/models/embeddable_host.rb b/app/models/embeddable_host.rb index 285c6fd1b79a..7609712d8ec3 100644 --- a/app/models/embeddable_host.rb +++ b/app/models/embeddable_host.rb @@ -3,6 +3,9 @@ class EmbeddableHost < ActiveRecord::Base validate :host_must_be_valid belongs_to :category + belongs_to :user, optional: true + has_many :embeddable_host_tags + has_many :tags, through: :embeddable_host_tags after_destroy :reset_embedding_settings before_validation do diff --git a/app/models/embeddable_host_tag.rb b/app/models/embeddable_host_tag.rb new file mode 100644 index 000000000000..309521edf1cd --- /dev/null +++ b/app/models/embeddable_host_tag.rb @@ -0,0 +1,10 @@ +# frozen_string_literal: true + +class EmbeddableHostTag < ActiveRecord::Base + belongs_to :embeddable_host + belongs_to :tag + + validates :embeddable_host_id, presence: true + validates :tag_id, presence: true + validates :embeddable_host_id, uniqueness: { scope: :tag_id } +end diff --git a/app/models/tag.rb b/app/models/tag.rb index b258cea67fda..14a1434e9fe1 100644 --- a/app/models/tag.rb +++ b/app/models/tag.rb @@ -57,6 +57,9 @@ class Tag < ActiveRecord::Base has_many :synonyms, class_name: "Tag", foreign_key: "target_tag_id", dependent: :destroy has_many :sidebar_section_links, as: :linkable, dependent: :delete_all + has_many :embeddable_host_tags + has_many :embeddable_hosts, through: :embeddable_host_tags + before_save :sanitize_description after_save :index_search diff --git a/app/models/topic_embed.rb b/app/models/topic_embed.rb index 17aaab835fa3..6aebdd5a728e 100644 --- a/app/models/topic_embed.rb +++ b/app/models/topic_embed.rb @@ -61,6 +61,8 @@ def self.import(user, url, title, contents, category_id: nil, cook_method: nil, if embed.blank? Topic.transaction do eh = EmbeddableHost.record_for_url(url) + tags = eh.tags || tags + user = eh.user || user cook_method ||= if SiteSetting.embed_support_markdown @@ -99,6 +101,10 @@ def self.import(user, url, title, contents, category_id: nil, cook_method: nil, absolutize_urls(url, contents) post = embed.post + eh = EmbeddableHost.record_for_url(url) + tags = eh.tags || tags + user = eh.user || user + # Update the topic if it changed if post&.topic if post.user != user @@ -115,6 +121,9 @@ def self.import(user, url, title, contents, category_id: nil, cook_method: nil, if (content_sha1 != embed.content_sha1) || (title && title != post&.topic&.title) changes = { raw: absolutize_urls(url, contents) } + + # TODO tags not working! + changes[:tags] = tags.map(&:name) if SiteSetting.tagging_enabled && tags.present? changes[:title] = title if title.present? post.revise(user, changes, skip_validations: true, bypass_rate_limiter: true) diff --git a/app/models/user.rb b/app/models/user.rb index 65048da9b942..95371ba9e71e 100644 --- a/app/models/user.rb +++ b/app/models/user.rb @@ -141,6 +141,7 @@ class User < ActiveRecord::Base belongs_to :uploaded_avatar, class_name: "Upload" has_many :sidebar_section_links, dependent: :delete_all + has_many :embeddable_hosts delegate :last_sent_email_address, to: :email_logs diff --git a/app/serializers/embeddable_host_serializer.rb b/app/serializers/embeddable_host_serializer.rb index d6a36de5c271..e9da2dd0f7a8 100644 --- a/app/serializers/embeddable_host_serializer.rb +++ b/app/serializers/embeddable_host_serializer.rb @@ -1,9 +1,17 @@ # frozen_string_literal: true class EmbeddableHostSerializer < ApplicationSerializer - TO_SERIALIZE = %i[id host allowed_paths class_name category_id] + TO_SERIALIZE = %i[id host allowed_paths class_name category_id tags user] attributes *TO_SERIALIZE TO_SERIALIZE.each { |attr| define_method(attr) { object.public_send(attr) } } + + def user + object.user&.username + end + + def tags + object.tags.map(&:name) + end end diff --git a/db/migrate/20240430051551_add_user_id_to_embeddable_hosts.rb b/db/migrate/20240430051551_add_user_id_to_embeddable_hosts.rb new file mode 100644 index 000000000000..7fb87bd10d9c --- /dev/null +++ b/db/migrate/20240430051551_add_user_id_to_embeddable_hosts.rb @@ -0,0 +1,6 @@ +# frozen_string_literal: true +class AddUserIdToEmbeddableHosts < ActiveRecord::Migration[7.0] + def change + add_column :embeddable_hosts, :user_id, :integer + end +end diff --git a/db/migrate/20240430052017_create_embeddable_host_tags.rb b/db/migrate/20240430052017_create_embeddable_host_tags.rb new file mode 100644 index 000000000000..92d08f71efda --- /dev/null +++ b/db/migrate/20240430052017_create_embeddable_host_tags.rb @@ -0,0 +1,15 @@ +# frozen_string_literal: true +class CreateEmbeddableHostTags < ActiveRecord::Migration[6.1] + def change + create_table :embeddable_host_tags do |t| + t.integer :embeddable_host_id, null: false + t.integer :tag_id, null: false + + t.timestamps + end + + add_index :embeddable_host_tags, :embeddable_host_id + add_index :embeddable_host_tags, :tag_id + add_index :embeddable_host_tags, %i[embeddable_host_id tag_id], unique: true + end +end diff --git a/spec/models/embeddable_host_tag_spec.rb b/spec/models/embeddable_host_tag_spec.rb new file mode 100644 index 000000000000..ccc2811746ab --- /dev/null +++ b/spec/models/embeddable_host_tag_spec.rb @@ -0,0 +1,6 @@ +# frozen_string_literal: true +require "rails_helper" + +RSpec.describe EmbeddableHostTag, type: :model do + pending "add some examples to (or delete) #{__FILE__}" +end From 3177487d110224f1c5ffe439acf5f12cf23e4ef0 Mon Sep 17 00:00:00 2001 From: Jean Perez Date: Tue, 7 May 2024 03:35:55 -0400 Subject: [PATCH 02/17] fix fetching tags when importing embedding --- .../admin/addon/components/embeddable-host.hbs | 7 +------ .../stylesheets/common/admin/customize.scss | 4 ++++ app/models/topic_embed.rb | 16 ++++++++++------ config/locales/client.en.yml | 2 ++ 4 files changed, 17 insertions(+), 12 deletions(-) diff --git a/app/assets/javascripts/admin/addon/components/embeddable-host.hbs b/app/assets/javascripts/admin/addon/components/embeddable-host.hbs index ae7e95ca6dd3..0109d97868c8 100644 --- a/app/assets/javascripts/admin/addon/components/embeddable-host.hbs +++ b/app/assets/javascripts/admin/addon/components/embeddable-host.hbs @@ -29,7 +29,6 @@ - + {{#if this.embedding.embed_by_username}} + + {{else}} + + {{/if}} diff --git a/config/locales/client.en.yml b/config/locales/client.en.yml index 795ef3bb801f..9a501e454236 100644 --- a/config/locales/client.en.yml +++ b/config/locales/client.en.yml @@ -6808,8 +6808,8 @@ en: allowed_paths: "Path Allowlist" edit: "edit" category: "Post to Category" - tags: "Tags" - user: "Author" + tags: "Topic Tags" + post_author: "Post Author - Defaults to %{author}" add_host: "Add Host" settings: "Embedding Settings" crawling_settings: "Crawler Settings" From 33b11d0436826128570652b0e66e33e2ed0dc3c2 Mon Sep 17 00:00:00 2001 From: Jean Perez Date: Fri, 10 May 2024 10:56:35 -0400 Subject: [PATCH 09/17] fix linting --- app/assets/javascripts/admin/addon/templates/embedding.hbs | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/app/assets/javascripts/admin/addon/templates/embedding.hbs b/app/assets/javascripts/admin/addon/templates/embedding.hbs index 5a096474b5f2..7cf572a4aba1 100644 --- a/app/assets/javascripts/admin/addon/templates/embedding.hbs +++ b/app/assets/javascripts/admin/addon/templates/embedding.hbs @@ -7,7 +7,10 @@ {{#if this.embedding.embed_by_username}} - + {{else}} {{/if}} From 5a8edba4a1be00b4a1e6b52f6448026b3d1d5377 Mon Sep 17 00:00:00 2001 From: Jean Perez Date: Fri, 10 May 2024 11:47:10 -0400 Subject: [PATCH 10/17] fix user assignment --- .../javascripts/admin/addon/components/embeddable-host.js | 2 +- app/controllers/admin/embeddable_hosts_controller.rb | 6 +++++- 2 files changed, 6 insertions(+), 2 deletions(-) diff --git a/app/assets/javascripts/admin/addon/components/embeddable-host.js b/app/assets/javascripts/admin/addon/components/embeddable-host.js index 6781a601a451..6117b6c3fc29 100644 --- a/app/assets/javascripts/admin/addon/components/embeddable-host.js +++ b/app/assets/javascripts/admin/addon/components/embeddable-host.js @@ -64,7 +64,7 @@ export default class EmbeddableHost extends Component.extend( props.category_id = this.category.id; props.tags = this.tags; props.user = - Array.isArray(this.user) && this.user.length > 0 ? this.user[0] : ""; + Array.isArray(this.user) && this.user.length > 0 ? this.user[0] : null; const host = this.host; diff --git a/app/controllers/admin/embeddable_hosts_controller.rb b/app/controllers/admin/embeddable_hosts_controller.rb index 1c08775deebd..012ce0c2375b 100644 --- a/app/controllers/admin/embeddable_hosts_controller.rb +++ b/app/controllers/admin/embeddable_hosts_controller.rb @@ -38,7 +38,11 @@ def save_host(host, action) host.tags = [] end - host.user = User.find_by_username(username) if username.present? + if username.blank? + host.user = nil + else + host.user = User.find_by_username(username) + end if host.save changes = host.saved_changes if action == :update From fd240e09d549be8cd790fb751d0220df0869f0e4 Mon Sep 17 00:00:00 2001 From: Jean Perez Date: Fri, 10 May 2024 11:53:41 -0400 Subject: [PATCH 11/17] fix user default value --- .../javascripts/admin/addon/components/embeddable-host.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/assets/javascripts/admin/addon/components/embeddable-host.js b/app/assets/javascripts/admin/addon/components/embeddable-host.js index 6117b6c3fc29..09c593915fe7 100644 --- a/app/assets/javascripts/admin/addon/components/embeddable-host.js +++ b/app/assets/javascripts/admin/addon/components/embeddable-host.js @@ -30,7 +30,7 @@ export default class EmbeddableHost extends Component.extend( const categoryId = host.category_id || this.site.uncategorized_category_id; const category = Category.findById(categoryId); const tags = host.tags || []; - const user = host.user || ""; + const user = host.user || null; this.set("category", category); this.set("tags", tags); From bf22faa34b70d4bd544bfdae1b7f8bb5279ecee0 Mon Sep 17 00:00:00 2001 From: Jean Perez Date: Fri, 10 May 2024 12:09:51 -0400 Subject: [PATCH 12/17] fix user null assignment --- .../javascripts/admin/addon/components/embeddable-host.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/assets/javascripts/admin/addon/components/embeddable-host.js b/app/assets/javascripts/admin/addon/components/embeddable-host.js index 09c593915fe7..f2c4dc4bfa3b 100644 --- a/app/assets/javascripts/admin/addon/components/embeddable-host.js +++ b/app/assets/javascripts/admin/addon/components/embeddable-host.js @@ -30,7 +30,7 @@ export default class EmbeddableHost extends Component.extend( const categoryId = host.category_id || this.site.uncategorized_category_id; const category = Category.findById(categoryId); const tags = host.tags || []; - const user = host.user || null; + const user = host.user; this.set("category", category); this.set("tags", tags); From 4bca2b76613bfe5adf92a9675a28795da5270a37 Mon Sep 17 00:00:00 2001 From: Jean Perez Date: Fri, 10 May 2024 12:10:35 -0400 Subject: [PATCH 13/17] simplify const assignments --- .../javascripts/admin/addon/components/embeddable-host.js | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/app/assets/javascripts/admin/addon/components/embeddable-host.js b/app/assets/javascripts/admin/addon/components/embeddable-host.js index f2c4dc4bfa3b..6eba3399073a 100644 --- a/app/assets/javascripts/admin/addon/components/embeddable-host.js +++ b/app/assets/javascripts/admin/addon/components/embeddable-host.js @@ -29,12 +29,10 @@ export default class EmbeddableHost extends Component.extend( const host = this.host; const categoryId = host.category_id || this.site.uncategorized_category_id; const category = Category.findById(categoryId); - const tags = host.tags || []; - const user = host.user; this.set("category", category); - this.set("tags", tags); - this.set("user", user); + this.set("tags", host.tags || []); + this.set("user", host.user); } @discourseComputed("buffered.host", "host.isSaving") From 4ddb2972bd2e1c375ec98f9a70db1d40d8cb3026 Mon Sep 17 00:00:00 2001 From: Jean Perez Date: Tue, 14 May 2024 12:36:06 -0400 Subject: [PATCH 14/17] address code review --- .../admin/embeddable_hosts_controller.rb | 64 ++++++++++++------- ...40430052017_create_embeddable_host_tags.rb | 2 +- .../embeddable_host_tag_fabricator.rb | 6 ++ spec/models/embeddable_host_tag_spec.rb | 58 ++++++++++++++++- .../admin/embeddable_hosts_controller_spec.rb | 62 ++++++++++++++++++ 5 files changed, 166 insertions(+), 26 deletions(-) create mode 100644 spec/fabricators/embeddable_host_tag_fabricator.rb diff --git a/app/controllers/admin/embeddable_hosts_controller.rb b/app/controllers/admin/embeddable_hosts_controller.rb index 012ce0c2375b..ea94237b1d0e 100644 --- a/app/controllers/admin/embeddable_hosts_controller.rb +++ b/app/controllers/admin/embeddable_hosts_controller.rb @@ -30,13 +30,6 @@ def save_host(host, action) # Handle tags username = params[:embeddable_host][:user] - tags = params[:embeddable_host][:tags]&.map(&:strip) - - if tags.present? - host.tags = Tag.where(name: tags) - else - host.tags = [] - end if username.blank? host.user = nil @@ -44,21 +37,48 @@ def save_host(host, action) host.user = User.find_by_username(username) end - if host.save - changes = host.saved_changes if action == :update - StaffActionLogger.new(current_user).log_embeddable_host( - host, - UserHistory.actions[:"embeddable_host_#{action}"], - changes: changes, - ) - render_serialized( - host, - EmbeddableHostSerializer, - root: "embeddable_host", - rest_serializer: true, - ) - else - render_json_error(host) + ActiveRecord::Base.transaction do + if host.save + manage_tags(host, params[:embeddable_host][:tags]&.map(&:strip)) + + changes = host.saved_changes if action == :update + StaffActionLogger.new(current_user).log_embeddable_host( + host, + UserHistory.actions[:"embeddable_host_#{action}"], + changes: changes, + ) + render_serialized( + host, + EmbeddableHostSerializer, + root: "embeddable_host", + rest_serializer: true, + ) + else + render_json_error(host) + raise ActiveRecord::Rollback + end + end + end + + def manage_tags(host, tags) + if tags.blank? + host.tags.clear + return + end + + existing_tags = Tag.where(name: tags).index_by(&:name) + tags_to_associate = [] + + tags.each do |tag_name| + tag = existing_tags[tag_name] || Tag.create(name: tag_name) + if tag.persisted? + tags_to_associate << tag + else + host.errors.add(:tags, "Failed to create or find tag: #{tag_name}") + raise ActiveRecord::Rollback + end end + + host.tags = tags_to_associate end end diff --git a/db/migrate/20240430052017_create_embeddable_host_tags.rb b/db/migrate/20240430052017_create_embeddable_host_tags.rb index 92d08f71efda..46ab6f974b3e 100644 --- a/db/migrate/20240430052017_create_embeddable_host_tags.rb +++ b/db/migrate/20240430052017_create_embeddable_host_tags.rb @@ -1,5 +1,5 @@ # frozen_string_literal: true -class CreateEmbeddableHostTags < ActiveRecord::Migration[6.1] +class CreateEmbeddableHostTags < ActiveRecord::Migration[7.0] def change create_table :embeddable_host_tags do |t| t.integer :embeddable_host_id, null: false diff --git a/spec/fabricators/embeddable_host_tag_fabricator.rb b/spec/fabricators/embeddable_host_tag_fabricator.rb new file mode 100644 index 000000000000..94c08689b816 --- /dev/null +++ b/spec/fabricators/embeddable_host_tag_fabricator.rb @@ -0,0 +1,6 @@ +#frozen_string_literal: true + +Fabricator(:embeddable_host_tag) do + embeddable_host + tag +end diff --git a/spec/models/embeddable_host_tag_spec.rb b/spec/models/embeddable_host_tag_spec.rb index ccc2811746ab..245a7aa9906f 100644 --- a/spec/models/embeddable_host_tag_spec.rb +++ b/spec/models/embeddable_host_tag_spec.rb @@ -1,6 +1,58 @@ -# frozen_string_literal: true -require "rails_helper" +#frozen_string_literal: true RSpec.describe EmbeddableHostTag, type: :model do - pending "add some examples to (or delete) #{__FILE__}" + describe "associations" do + it "belongs to an embeddable_host" do + expect(described_class.reflect_on_association(:embeddable_host).macro).to eq(:belongs_to) + end + + it "belongs to a tag" do + expect(described_class.reflect_on_association(:tag).macro).to eq(:belongs_to) + end + end + + describe "validations" do + subject { Fabricate(:embeddable_host_tag) } + + it { is_expected.to validate_presence_of(:embeddable_host_id) } + it { is_expected.to validate_presence_of(:tag_id) } + it { is_expected.to validate_uniqueness_of(:embeddable_host_id).scoped_to(:tag_id) } + end + + describe "functionality" do + context "when creating valid associations" do + let(:embeddable_host) { Fabricate(:embeddable_host) } + let(:tag) { Fabricate(:tag) } + + it "successfully creates an embeddable_host_tag with valid inputs" do + host_tag = EmbeddableHostTag.new(embeddable_host: embeddable_host, tag: tag) + expect(host_tag.save).to be true + end + end + + context "when attempting to create duplicate associations" do + let(:embeddable_host) { Fabricate(:embeddable_host) } + let(:tag) { Fabricate(:tag) } + + before { EmbeddableHostTag.create!(embeddable_host: embeddable_host, tag: tag) } + + it "prevents duplicate embeddable_host_tag from being saved" do + duplicate_host_tag = EmbeddableHostTag.new(embeddable_host: embeddable_host, tag: tag) + expect(duplicate_host_tag.valid?).to be false + expect(duplicate_host_tag.errors[:embeddable_host_id]).to include("has already been taken") + end + end + + context "with missing fields" do + it "fails to save without an embeddable_host" do + host_tag = EmbeddableHostTag.new(tag: Fabricate(:tag)) + expect(host_tag.valid?).to be false + end + + it "fails to save without a tag" do + host_tag = EmbeddableHostTag.new(embeddable_host: Fabricate(:embeddable_host)) + expect(host_tag.valid?).to be false + end + end + end end diff --git a/spec/requests/admin/embeddable_hosts_controller_spec.rb b/spec/requests/admin/embeddable_hosts_controller_spec.rb index 9380ed4cb3de..9df7ef634cab 100644 --- a/spec/requests/admin/embeddable_hosts_controller_spec.rb +++ b/spec/requests/admin/embeddable_hosts_controller_spec.rb @@ -21,6 +21,68 @@ ).exists?, ).to eq(true) end + + it "creates an embeddable host with associated tags" do + tag1 = Fabricate(:tag, name: "tag1") + tag2 = Fabricate(:tag, name: "tag2") + + post "/admin/embeddable_hosts.json", + params: { + embeddable_host: { + host: "example.com", + tags: %w[tag1 tag2], + }, + } + + expect(response.status).to eq(200) + expect(EmbeddableHost.last.tags).to contain_exactly(tag1, tag2) + end + + it "updates an embeddable host with associated tags" do + tag1 = Fabricate(:tag, name: "newTag1") + tag2 = Fabricate(:tag, name: "newTag2") + + put "/admin/embeddable_hosts/#{embeddable_host.id}.json", + params: { + embeddable_host: { + host: "updated-example.com", + tags: %w[newTag1 newTag2], + }, + } + + expect(response.status).to eq(200) + expect(EmbeddableHost.find(embeddable_host.id).tags).to contain_exactly(tag1, tag2) + end + + it "creates an embeddable host with an associated author" do + user = Fabricate(:user, username: "johndoe") + + post "/admin/embeddable_hosts.json", + params: { + embeddable_host: { + host: "example.com", + user: "johndoe", + }, + } + + expect(response.status).to eq(200) + expect(EmbeddableHost.last.user).to eq(user) + end + + it "updates an embeddable host with a new author" do + new_user = Fabricate(:user, username: "johndoe") + + put "/admin/embeddable_hosts/#{embeddable_host.id}.json", + params: { + embeddable_host: { + host: "updated-example.com", + user: "johndoe", + }, + } + + expect(response.status).to eq(200) + expect(EmbeddableHost.find(embeddable_host.id).user).to eq(new_user) + end end shared_examples "embeddable host creation not allowed" do From 6b154d8df98ad6fda810abc5807e1e7825c4e86c Mon Sep 17 00:00:00 2001 From: Jean Perez Date: Tue, 14 May 2024 15:32:56 -0400 Subject: [PATCH 15/17] add system tests --- app/models/topic_embed.rb | 1 - spec/system/admin_embeddable_hosts_spec.rb | 64 ++++++++++++++++++++++ 2 files changed, 64 insertions(+), 1 deletion(-) create mode 100644 spec/system/admin_embeddable_hosts_spec.rb diff --git a/app/models/topic_embed.rb b/app/models/topic_embed.rb index a37350847a1d..8d5263f8d025 100644 --- a/app/models/topic_embed.rb +++ b/app/models/topic_embed.rb @@ -124,7 +124,6 @@ def self.import(user, url, title, contents, category_id: nil, cook_method: nil, existing_tag_names = post.topic.tags.pluck(:name).sort incoming_tag_names = Array(tags).map(&:name).sort - # Determine if there are any changes in the tags tags_changed = existing_tag_names != incoming_tag_names if (content_sha1 != embed.content_sha1) || (title && title != post&.topic&.title) || diff --git a/spec/system/admin_embeddable_hosts_spec.rb b/spec/system/admin_embeddable_hosts_spec.rb new file mode 100644 index 000000000000..dfe4faf943b1 --- /dev/null +++ b/spec/system/admin_embeddable_hosts_spec.rb @@ -0,0 +1,64 @@ +# frozen_string_literal: true + +RSpec.describe "Admin EmbeddableHost Management", type: :system do + fab!(:admin) + fab!(:author) { Fabricate(:admin) } + fab!(:category) + fab!(:category2) { Fabricate(:category) } + fab!(:tag) + fab!(:tag2) { Fabricate(:tag) } + + before { sign_in(admin) } + + it "allows an admin to add a new embeddable host" do + visit "/admin/customize/embedding" + + find("button.btn-icon-text", text: "Add Host").click + within find("tr.ember-view") do + find('input[placeholder="example.com"]').set("awesome-discourse-site.local") + find('input[placeholder="/blog/.*"]').set("/blog/.*") + + category_chooser = PageObjects::Components::SelectKit.new(".category-chooser") + category_chooser.expand + category_chooser.select_row_by_name(category.name) + + tag_chooser = PageObjects::Components::SelectKit.new(".tag-chooser") + tag_chooser.expand + tag_chooser.select_row_by_name(tag.name) + + find(".user-chooser").click + find(".select-kit-body .select-kit-filter input").fill_in with: author.username + find(".select-kit-body", text: author.username).click + end + find("td.editing-controls .btn.btn-primary").click + expect(page).to have_content("awesome-discourse-site.local") + expect(page).to have_content("/blog/.*") + expect(page).not_to have_content("#{tag.name},#{tag2.name}") + expect(page).to have_content("#{tag.name}") + + # Editing + + find(".embeddable-hosts tr:first-child .controls svg.d-icon-pencil-alt").find( + :xpath, + "..", + ).click + + within find(".embeddable-hosts tr:first-child.ember-view") do + find('input[placeholder="example.com"]').set("updated-example.com") + find('input[placeholder="/blog/.*"]').set("/updated-blog/.*") + + category_chooser = PageObjects::Components::SelectKit.new(".category-chooser") + category_chooser.expand + category_chooser.select_row_by_name(category2.name) + + tag_chooser = PageObjects::Components::SelectKit.new(".tag-chooser") + tag_chooser.expand + tag_chooser.select_row_by_name(tag2.name) + end + + find("td.editing-controls .btn.btn-primary").click + expect(page).to have_content("updated-example.com") + expect(page).to have_content("/updated-blog/.*") + expect(page).to have_content("#{tag.name},#{tag2.name}") + end +end From 9c6353774dc0aa9071caf73d8ff7b365a25107e2 Mon Sep 17 00:00:00 2001 From: Jean Perez Date: Tue, 14 May 2024 15:38:01 -0400 Subject: [PATCH 16/17] remove comment --- app/controllers/admin/embeddable_hosts_controller.rb | 1 - 1 file changed, 1 deletion(-) diff --git a/app/controllers/admin/embeddable_hosts_controller.rb b/app/controllers/admin/embeddable_hosts_controller.rb index ea94237b1d0e..c8edbb71d5cf 100644 --- a/app/controllers/admin/embeddable_hosts_controller.rb +++ b/app/controllers/admin/embeddable_hosts_controller.rb @@ -28,7 +28,6 @@ def save_host(host, action) host.category_id = params[:embeddable_host][:category_id] host.category_id = SiteSetting.uncategorized_category_id if host.category_id.blank? - # Handle tags username = params[:embeddable_host][:user] if username.blank? From 3f2a1d0b4b66894f7e775a85d36b94c9dd294aad Mon Sep 17 00:00:00 2001 From: Jean Perez Date: Tue, 14 May 2024 15:40:18 -0400 Subject: [PATCH 17/17] update test description --- spec/system/admin_embeddable_hosts_spec.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/spec/system/admin_embeddable_hosts_spec.rb b/spec/system/admin_embeddable_hosts_spec.rb index dfe4faf943b1..0f033786e1cc 100644 --- a/spec/system/admin_embeddable_hosts_spec.rb +++ b/spec/system/admin_embeddable_hosts_spec.rb @@ -10,7 +10,7 @@ before { sign_in(admin) } - it "allows an admin to add a new embeddable host" do + it "allows admin to add and edit embeddable hosts" do visit "/admin/customize/embedding" find("button.btn-icon-text", text: "Add Host").click
{{i18n "admin.embedding.host"}}{{i18n "admin.embedding.allowed_paths"}}{{i18n "admin.embedding.category"}}{{i18n "admin.embedding.host"}}{{i18n "admin.embedding.allowed_paths"}}{{i18n "admin.embedding.category"}}{{i18n "admin.embedding.tags"}}{{i18n "admin.embedding.user"}}  
{{i18n "admin.embedding.category"}}
{{i18n "admin.embedding.category"}}
 diff --git a/app/assets/stylesheets/common/admin/customize.scss b/app/assets/stylesheets/common/admin/customize.scss index e2ad75b3cdbb..06cbf9b55905 100644 --- a/app/assets/stylesheets/common/admin/customize.scss +++ b/app/assets/stylesheets/common/admin/customize.scss @@ -878,6 +878,10 @@ table.permalinks { margin-bottom: 2em; table.grid { margin-bottom: 1em; + .tag-chooser, + .user-chooser { + width: 100%; + } tr td { word-wrap: break-word; max-width: 25vw; diff --git a/app/models/topic_embed.rb b/app/models/topic_embed.rb index 6aebdd5a728e..c1540f48dfbf 100644 --- a/app/models/topic_embed.rb +++ b/app/models/topic_embed.rb @@ -61,8 +61,8 @@ def self.import(user, url, title, contents, category_id: nil, cook_method: nil, if embed.blank? Topic.transaction do eh = EmbeddableHost.record_for_url(url) - tags = eh.tags || tags - user = eh.user || user + tags = eh.tags.presence || tags + user = eh.user.presence || user cook_method ||= if SiteSetting.embed_support_markdown @@ -102,8 +102,8 @@ def self.import(user, url, title, contents, category_id: nil, cook_method: nil, post = embed.post eh = EmbeddableHost.record_for_url(url) - tags = eh.tags || tags - user = eh.user || user + tags = eh.tags.presence || tags + user = eh.user.presence || user # Update the topic if it changed if post&.topic @@ -119,10 +119,14 @@ def self.import(user, url, title, contents, category_id: nil, cook_method: nil, post.reload end - if (content_sha1 != embed.content_sha1) || (title && title != post&.topic&.title) + existing_tag_names = post.topic.tags.map(&:name) + incoming_tag_names = Array.wrap(tags).map(&:name) + + new_tags_being_added = (incoming_tag_names - existing_tag_names).any? + if (content_sha1 != embed.content_sha1) || (title && title != post&.topic&.title) || + new_tags_being_added changes = { raw: absolutize_urls(url, contents) } - # TODO tags not working! changes[:tags] = tags.map(&:name) if SiteSetting.tagging_enabled && tags.present? changes[:title] = title if title.present? diff --git a/config/locales/client.en.yml b/config/locales/client.en.yml index ec558b23254b..795ef3bb801f 100644 --- a/config/locales/client.en.yml +++ b/config/locales/client.en.yml @@ -6808,6 +6808,8 @@ en: allowed_paths: "Path Allowlist" edit: "edit" category: "Post to Category" + tags: "Tags" + user: "Author" add_host: "Add Host" settings: "Embedding Settings" crawling_settings: "Crawler Settings" From 436e374a5cb598e9baf51a3ee319bcf6b1b27066 Mon Sep 17 00:00:00 2001 From: Jean Perez Date: Tue, 7 May 2024 11:18:42 -0400 Subject: [PATCH 03/17] fix linting --- spec/fabricators/topic_embed_fabricator.rb | 2 + spec/models/topic_embed_spec.rb | 48 ++++++++++++++++++++++ 2 files changed, 50 insertions(+) diff --git a/spec/fabricators/topic_embed_fabricator.rb b/spec/fabricators/topic_embed_fabricator.rb index 628a3899eee7..7e0bbf8f16a8 100644 --- a/spec/fabricators/topic_embed_fabricator.rb +++ b/spec/fabricators/topic_embed_fabricator.rb @@ -4,4 +4,6 @@ post embed_url "http://eviltrout.com/123" topic { |te| te[:post].topic } + tags { [Fabricate(:tag)] } + user { Fabricate(:user) } end diff --git a/spec/models/topic_embed_spec.rb b/spec/models/topic_embed_spec.rb index d3e7608c7029..cf57b1c807b1 100644 --- a/spec/models/topic_embed_spec.rb +++ b/spec/models/topic_embed_spec.rb @@ -299,6 +299,54 @@ end end + context "with specified user and tags" do + fab!(:tag1) { Fabricate(:tag, name: "interesting") } + fab!(:tag2) { Fabricate(:tag, name: "article") } + + let!(:new_user) { Fabricate(:user) } + let(:tags) { [tag1.name, tag2.name] } + let(:imported_post) { TopicEmbed.import(new_user, url, title, contents, tags: tags) } + + it "assigns the specified user as the author" do + expect(imported_post.user).to eq(new_user) + end + + it "associates the specified tags with the topic" do + expect(imported_post.topic.tags).to contain_exactly(tag1, tag2) + end + end + + context "when updating an existing post with new tags and a different user" do + fab!(:tag1) { Fabricate(:tag, name: "interesting") } + fab!(:tag2) { Fabricate(:tag, name: "article") } + + let!(:new_user) { Fabricate(:user) } + let(:tags) { [tag1.name, tag2.name] } + + before { SiteSetting.tagging_enabled = true } + + it "updates the user and adds new tags" do + original_post = TopicEmbed.import(user, url, title, contents) + + expect(original_post.user).to eq(user) + expect(original_post.topic.tags).to be_empty + + embeddable_host.update!( + tags: [tag1, tag2], + user: new_user, + category: category, + host: "eviltrout.com", + ) + embeddable_host.save! + embeddable_host.reload + puts "ID HERE: #{embeddable_host.id.inspect}" + edited_post = TopicEmbed.import(user, url, title, "#{contents} - updated") + + expect(edited_post.user).to eq(new_user) + expect(edited_post.topic.tags).to match_array([tag1, tag2]) + end + end + describe "embedded content truncation" do MAX_LENGTH_BEFORE_TRUNCATION = 100 From 2a100b2834227e9ed6c8edaea7e48da9f188e720 Mon Sep 17 00:00:00 2001 From: Jean Perez Date: Tue, 7 May 2024 15:28:35 -0400 Subject: [PATCH 04/17] fix tests --- app/models/embeddable_host.rb | 1 + app/models/embeddable_host_tag.rb | 17 +++++++++++++++++ app/models/topic_embed.rb | 14 ++++++++------ 3 files changed, 26 insertions(+), 6 deletions(-) diff --git a/app/models/embeddable_host.rb b/app/models/embeddable_host.rb index 05ea6f2a2a4d..e5676c307369 100644 --- a/app/models/embeddable_host.rb +++ b/app/models/embeddable_host.rb @@ -84,4 +84,5 @@ def host_must_be_valid # updated_at :datetime not null # class_name :string # allowed_paths :string +# user_id :integer # diff --git a/app/models/embeddable_host_tag.rb b/app/models/embeddable_host_tag.rb index 309521edf1cd..0ee9ada4f12f 100644 --- a/app/models/embeddable_host_tag.rb +++ b/app/models/embeddable_host_tag.rb @@ -8,3 +8,20 @@ class EmbeddableHostTag < ActiveRecord::Base validates :tag_id, presence: true validates :embeddable_host_id, uniqueness: { scope: :tag_id } end + +# == Schema Information +# +# Table name: embeddable_host_tags +# +# id :bigint not null, primary key +# embeddable_host_id :integer not null +# tag_id :integer not null +# created_at :datetime not null +# updated_at :datetime not null +# +# Indexes +# +# index_embeddable_host_tags_on_embeddable_host_id (embeddable_host_id) +# index_embeddable_host_tags_on_embeddable_host_id_and_tag_id (embeddable_host_id,tag_id) UNIQUE +# index_embeddable_host_tags_on_tag_id (tag_id) +# diff --git a/app/models/topic_embed.rb b/app/models/topic_embed.rb index c1540f48dfbf..e9b5a8207210 100644 --- a/app/models/topic_embed.rb +++ b/app/models/topic_embed.rb @@ -60,9 +60,10 @@ def self.import(user, url, title, contents, category_id: nil, cook_method: nil, # If there is no embed, create a topic, post and the embed. if embed.blank? Topic.transaction do - eh = EmbeddableHost.record_for_url(url) - tags = eh.tags.presence || tags - user = eh.user.presence || user + if eh = EmbeddableHost.record_for_url(url) + tags = eh.tags.presence || tags + user = eh.user.presence || user + end cook_method ||= if SiteSetting.embed_support_markdown @@ -101,9 +102,10 @@ def self.import(user, url, title, contents, category_id: nil, cook_method: nil, absolutize_urls(url, contents) post = embed.post - eh = EmbeddableHost.record_for_url(url) - tags = eh.tags.presence || tags - user = eh.user.presence || user + if eh = EmbeddableHost.record_for_url(url) + tags = eh.tags.presence || tags + user = eh.user.presence || user + end # Update the topic if it changed if post&.topic From 9c9b56e323436615a4a548f8c8d69cd4bf02dc80 Mon Sep 17 00:00:00 2001 From: Jean Perez Date: Tue, 7 May 2024 15:33:03 -0400 Subject: [PATCH 05/17] fix annotation --- app/models/embeddable_host.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/models/embeddable_host.rb b/app/models/embeddable_host.rb index e5676c307369..90dcb85b1232 100644 --- a/app/models/embeddable_host.rb +++ b/app/models/embeddable_host.rb @@ -84,5 +84,5 @@ def host_must_be_valid # updated_at :datetime not null # class_name :string # allowed_paths :string -# user_id :integer +# user_id :integer # From 9e93f001d43a562ace6e090be88b59083f1ed62b Mon Sep 17 00:00:00 2001 From: Jean Perez Date: Tue, 7 May 2024 16:23:24 -0400 Subject: [PATCH 06/17] fix tags logic --- app/controllers/admin/embeddable_hosts_controller.rb | 8 +++++++- app/models/topic_embed.rb | 12 +++++++----- 2 files changed, 14 insertions(+), 6 deletions(-) diff --git a/app/controllers/admin/embeddable_hosts_controller.rb b/app/controllers/admin/embeddable_hosts_controller.rb index 8eb76a682cc1..1c08775deebd 100644 --- a/app/controllers/admin/embeddable_hosts_controller.rb +++ b/app/controllers/admin/embeddable_hosts_controller.rb @@ -31,7 +31,13 @@ def save_host(host, action) # Handle tags username = params[:embeddable_host][:user] tags = params[:embeddable_host][:tags]&.map(&:strip) - host.tags = Tag.where(name: tags) if tags.present? + + if tags.present? + host.tags = Tag.where(name: tags) + else + host.tags = [] + end + host.user = User.find_by_username(username) if username.present? if host.save diff --git a/app/models/topic_embed.rb b/app/models/topic_embed.rb index e9b5a8207210..a37350847a1d 100644 --- a/app/models/topic_embed.rb +++ b/app/models/topic_embed.rb @@ -121,15 +121,17 @@ def self.import(user, url, title, contents, category_id: nil, cook_method: nil, post.reload end - existing_tag_names = post.topic.tags.map(&:name) - incoming_tag_names = Array.wrap(tags).map(&:name) + existing_tag_names = post.topic.tags.pluck(:name).sort + incoming_tag_names = Array(tags).map(&:name).sort + + # Determine if there are any changes in the tags + tags_changed = existing_tag_names != incoming_tag_names - new_tags_being_added = (incoming_tag_names - existing_tag_names).any? if (content_sha1 != embed.content_sha1) || (title && title != post&.topic&.title) || - new_tags_being_added + tags_changed changes = { raw: absolutize_urls(url, contents) } - changes[:tags] = tags.map(&:name) if SiteSetting.tagging_enabled && tags.present? + changes[:tags] = incoming_tag_names if SiteSetting.tagging_enabled && tags_changed changes[:title] = title if title.present? post.revise(user, changes, skip_validations: true, bypass_rate_limiter: true) From 995ef6887986ce0521b88905daeaddfee1898af5 Mon Sep 17 00:00:00 2001 From: Jean Perez Date: Tue, 7 May 2024 17:58:27 -0400 Subject: [PATCH 07/17] fix tests --- spec/fabricators/topic_embed_fabricator.rb | 2 -- spec/models/topic_embed_spec.rb | 17 ++++++++--------- 2 files changed, 8 insertions(+), 11 deletions(-) diff --git a/spec/fabricators/topic_embed_fabricator.rb b/spec/fabricators/topic_embed_fabricator.rb index 7e0bbf8f16a8..628a3899eee7 100644 --- a/spec/fabricators/topic_embed_fabricator.rb +++ b/spec/fabricators/topic_embed_fabricator.rb @@ -4,6 +4,4 @@ post embed_url "http://eviltrout.com/123" topic { |te| te[:post].topic } - tags { [Fabricate(:tag)] } - user { Fabricate(:user) } end diff --git a/spec/models/topic_embed_spec.rb b/spec/models/topic_embed_spec.rb index cf57b1c807b1..10d670253d1c 100644 --- a/spec/models/topic_embed_spec.rb +++ b/spec/models/topic_embed_spec.rb @@ -320,29 +320,28 @@ fab!(:tag1) { Fabricate(:tag, name: "interesting") } fab!(:tag2) { Fabricate(:tag, name: "article") } - let!(:new_user) { Fabricate(:user) } + let!(:admin) { Fabricate(:admin) } + let!(:new_admin) { Fabricate(:admin) } let(:tags) { [tag1.name, tag2.name] } before { SiteSetting.tagging_enabled = true } it "updates the user and adds new tags" do - original_post = TopicEmbed.import(user, url, title, contents) + original_post = TopicEmbed.import(admin, url, title, contents) - expect(original_post.user).to eq(user) + expect(original_post.user).to eq(admin) expect(original_post.topic.tags).to be_empty embeddable_host.update!( tags: [tag1, tag2], - user: new_user, + user: new_admin, category: category, host: "eviltrout.com", ) - embeddable_host.save! - embeddable_host.reload - puts "ID HERE: #{embeddable_host.id.inspect}" - edited_post = TopicEmbed.import(user, url, title, "#{contents} - updated") - expect(edited_post.user).to eq(new_user) + edited_post = TopicEmbed.import(admin, url, title, contents) + + expect(edited_post.user).to eq(new_admin) expect(edited_post.topic.tags).to match_array([tag1, tag2]) end end From 7a2b2ac79f28a0f8415babbe05febf6983578721 Mon Sep 17 00:00:00 2001 From: Jean Perez Date: Fri, 10 May 2024 10:45:45 -0400 Subject: [PATCH 08/17] translation fixes --- .../javascripts/admin/addon/components/embeddable-host.hbs | 6 ++---- app/assets/javascripts/admin/addon/templates/embedding.hbs | 6 +++++- config/locales/client.en.yml | 4 ++-- 3 files changed, 9 insertions(+), 7 deletions(-) diff --git a/app/assets/javascripts/admin/addon/components/embeddable-host.hbs b/app/assets/javascripts/admin/addon/components/embeddable-host.hbs index 0109d97868c8..f0a7d06f6871 100644 --- a/app/assets/javascripts/admin/addon/components/embeddable-host.hbs +++ b/app/assets/javascripts/admin/addon/components/embeddable-host.hbs @@ -27,7 +27,7 @@ /> -
{{i18n "admin.embedding.category"}}
+
{{i18n "admin.embedding.tags"}}
 -
{{i18n "admin.embedding.category"}}
+
{{i18n "admin.embedding.user"}}
 -
{{i18n "admin.embedding.tags"}}
{{this.tags}} 		-
{{i18n "admin.embedding.tags"}}
{{this.user}} 		diff --git a/app/assets/javascripts/admin/addon/templates/embedding.hbs b/app/assets/javascripts/admin/addon/templates/embedding.hbs index a9af120ea574..5a096474b5f2 100644 --- a/app/assets/javascripts/admin/addon/templates/embedding.hbs +++ b/app/assets/javascripts/admin/addon/templates/embedding.hbs @@ -6,7 +6,11 @@ {{i18n "admin.embedding.allowed_paths"}} {{i18n "admin.embedding.category"}} {{i18n "admin.embedding.tags"}}{{i18n "admin.embedding.user"}}{{i18n "admin.embedding.post_author" author=this.embedding.embed_by_username}}{{i18n "admin.embedding.post_author"}} 
{{i18n "admin.embedding.category"}} {{i18n "admin.embedding.tags"}}{{i18n "admin.embedding.post_author" author=this.embedding.embed_by_username}}{{i18n + "admin.embedding.post_author" + author=this.embedding.embed_by_username + }}{{i18n "admin.embedding.post_author"}}