From 71f7f7ed49950ea09381e39d4b8faab69b1a06ba Mon Sep 17 00:00:00 2001 From: Blake Erickson Date: Tue, 8 Feb 2022 20:55:32 -0700 Subject: [PATCH] FEATURE: Add external_id to topics (#15825) * FEATURE: Add external_id to topics This commit allows for topics to be created and fetched by an external_id. These changes are API only for now as there aren't any front changes. * add annotations * add external_id to this spec * Several PR feedback changes - Add guardian to find topic - 403 is returned for not found as well now - add `include_external_id?` - external_id is now case insensitive - added test for posts_controller - added test for topic creator - created constant for max length - check that it redirects to the correct path - restrain external id in routes file * remove puts * fix tests * only check for external_id in webhook if exists * Update index to exclude external_id if null * annotate * Update app/controllers/topics_controller.rb We need to check whether the topic is present first before passing it to the guardian. Co-authored-by: Alan Guo Xiang Tan * Apply suggestions from code review Co-authored-by: Alan Guo Xiang Tan Co-authored-by: Alan Guo Xiang Tan --- app/controllers/posts_controller.rb | 2 + app/controllers/topics_controller.rb | 7 +++ app/models/topic.rb | 6 +++ app/serializers/topic_view_serializer.rb | 7 ++- config/routes.rb | 1 + ...0220202225716_add_external_id_to_topics.rb | 8 ++++ lib/topic_creator.rb | 1 + spec/components/topic_creator_spec.rb | 10 ++++ spec/models/topic_spec.rb | 46 +++++++++++++++++++ spec/requests/posts_controller_spec.rb | 15 ++++++ spec/requests/topics_controller_spec.rb | 28 +++++++++++ .../serializers/topic_view_serializer_spec.rb | 11 +++++ .../web_hook_topic_view_serializer_spec.rb | 4 ++ 13 files changed, 145 insertions(+), 1 deletion(-) create mode 100644 db/migrate/20220202225716_add_external_id_to_topics.rb diff --git a/app/controllers/posts_controller.rb b/app/controllers/posts_controller.rb index 7a0da6d0434..bdc2823755a 100644 --- a/app/controllers/posts_controller.rb +++ b/app/controllers/posts_controller.rb @@ -760,6 +760,8 @@ class PostsController < ApplicationController # We allow `created_at` via the API permitted << :created_at + # We allow `external_id` via the API + permitted << :external_id end result = params.permit(*permitted).tap do |allowed| diff --git a/app/controllers/topics_controller.rb b/app/controllers/topics_controller.rb index a7b758e20f4..abb249c2e8d 100644 --- a/app/controllers/topics_controller.rb +++ b/app/controllers/topics_controller.rb @@ -43,6 +43,13 @@ class TopicsController < ApplicationController render json: { slug: topic.slug, topic_id: topic.id, url: topic.url } end + def show_by_external_id + topic = Topic.find_by(external_id: params[:external_id]) + raise Discourse::NotFound unless topic + guardian.ensure_can_see!(topic) + redirect_to_correct_topic(topic, params[:post_number]) + end + def show if request.referer flash["referer"] ||= request.referer[0..255] diff --git a/app/models/topic.rb b/app/models/topic.rb index 1a6948c6cc6..dbac653ba86 100644 --- a/app/models/topic.rb +++ b/app/models/topic.rb @@ -11,6 +11,8 @@ class Topic < ActiveRecord::Base include LimitedEdit extend Forwardable + EXTERNAL_ID_MAX_LENGTH = 50 + self.ignored_columns = [ "avg_time", # TODO(2021-01-04): remove "image_url" # TODO(2021-06-01): remove @@ -195,6 +197,8 @@ class Topic < ActiveRecord::Base end end + validates :external_id, allow_nil: true, uniqueness: { case_sensitive: false }, length: { maximum: EXTERNAL_ID_MAX_LENGTH }, format: { with: /\A[\w-]+\z/ } + before_validation do self.title = TextCleaner.clean_title(TextSentinel.title_sentinel(title).text) if errors[:title].empty? self.featured_link = self.featured_link.strip.presence if self.featured_link @@ -1902,6 +1906,7 @@ end # image_upload_id :bigint # slow_mode_seconds :integer default(0), not null # bannered_until :datetime +# external_id :string # # Indexes # @@ -1911,6 +1916,7 @@ end # index_topics_on_bannered_until (bannered_until) WHERE (bannered_until IS NOT NULL) # index_topics_on_bumped_at_public (bumped_at) WHERE ((deleted_at IS NULL) AND ((archetype)::text <> 'private_message'::text)) # index_topics_on_created_at_and_visible (created_at,visible) WHERE ((deleted_at IS NULL) AND ((archetype)::text <> 'private_message'::text)) +# index_topics_on_external_id (external_id) UNIQUE WHERE (external_id IS NOT NULL) # index_topics_on_id_and_deleted_at (id,deleted_at) # index_topics_on_id_filtered_banner (id) UNIQUE WHERE (((archetype)::text = 'banner'::text) AND (deleted_at IS NULL)) # index_topics_on_image_upload_id (image_upload_id) diff --git a/app/serializers/topic_view_serializer.rb b/app/serializers/topic_view_serializer.rb index 0d717fa2b5a..268fbc78723 100644 --- a/app/serializers/topic_view_serializer.rb +++ b/app/serializers/topic_view_serializer.rb @@ -41,7 +41,8 @@ class TopicViewSerializer < ApplicationSerializer :pinned_at, :pinned_until, :image_url, - :slow_mode_seconds + :slow_mode_seconds, + :external_id ) attributes( @@ -104,6 +105,10 @@ class TopicViewSerializer < ApplicationSerializer is_warning end + def include_external_id? + external_id + end + def draft object.draft end diff --git a/config/routes.rb b/config/routes.rb index 32e5a05c1db..641a9057655 100644 --- a/config/routes.rb +++ b/config/routes.rb @@ -809,6 +809,7 @@ Discourse::Application.routes.draw do # Topic routes get "t/id_for/:slug" => "topics#id_for_slug" + get "t/external_id/:external_id" => "topics#show_by_external_id", format: :json, constrains: { external_id: /\A[\w-]+\z/ } get "t/:slug/:topic_id/print" => "topics#show", format: :html, print: true, constraints: { topic_id: /\d+/ } get "t/:slug/:topic_id/wordpress" => "topics#wordpress", constraints: { topic_id: /\d+/ } get "t/:topic_id/wordpress" => "topics#wordpress", constraints: { topic_id: /\d+/ } diff --git a/db/migrate/20220202225716_add_external_id_to_topics.rb b/db/migrate/20220202225716_add_external_id_to_topics.rb new file mode 100644 index 00000000000..6ee7c3edb52 --- /dev/null +++ b/db/migrate/20220202225716_add_external_id_to_topics.rb @@ -0,0 +1,8 @@ +# frozen_string_literal: true + +class AddExternalIdToTopics < ActiveRecord::Migration[6.1] + def change + add_column :topics, :external_id, :string, null: true + add_index :topics, :external_id, unique: true, where: 'external_id IS NOT NULL' + end +end diff --git a/lib/topic_creator.rb b/lib/topic_creator.rb index 1211c84fe78..4b79035b487 100644 --- a/lib/topic_creator.rb +++ b/lib/topic_creator.rb @@ -137,6 +137,7 @@ class TopicCreator topic_params[:created_at] = convert_time(@opts[:created_at]) if @opts[:created_at].present? topic_params[:pinned_at] = convert_time(@opts[:pinned_at]) if @opts[:pinned_at].present? topic_params[:pinned_globally] = @opts[:pinned_globally] if @opts[:pinned_globally].present? + topic_params[:external_id] = @opts[:external_id] if @opts[:external_id].present? topic_params[:featured_link] = @opts[:featured_link] topic_params diff --git a/spec/components/topic_creator_spec.rb b/spec/components/topic_creator_spec.rb index 433462a4c21..2e6ccc3968d 100644 --- a/spec/components/topic_creator_spec.rb +++ b/spec/components/topic_creator_spec.rb @@ -286,5 +286,15 @@ describe TopicCreator do expect(topic.pinned_at).to eq_time(time2) end end + + context 'external_id' do + it 'adds external_id' do + topic = TopicCreator.create(user, Guardian.new(user), valid_attrs.merge( + external_id: 'external_id' + )) + + expect(topic.external_id).to eq('external_id') + end + end end end diff --git a/spec/models/topic_spec.rb b/spec/models/topic_spec.rb index 6a848c009c5..cd0f8e5a6c3 100644 --- a/spec/models/topic_spec.rb +++ b/spec/models/topic_spec.rb @@ -34,6 +34,51 @@ describe Topic do end end + context "#external_id" do + describe 'when external_id is too long' do + it 'should not be valid' do + topic.external_id = 'a' * (Topic::EXTERNAL_ID_MAX_LENGTH + 1) + expect(topic).to_not be_valid + end + end + + describe 'when external_id has invalid characters' do + it 'should not be valid' do + topic.external_id = 'a*&^!@()#' + expect(topic).to_not be_valid + end + end + + describe 'when external_id is an empty string' do + it 'should not be valid' do + topic.external_id = '' + expect(topic).to_not be_valid + end + end + + describe 'when external_id has already been used' do + it 'should not be valid' do + topic2 = Fabricate(:topic, external_id: 'asdf') + topic.external_id = 'asdf' + expect(topic).to_not be_valid + end + end + + describe 'when external_id is nil' do + it 'should be valid' do + topic.external_id = nil + expect(topic).to be_valid + end + end + + describe 'when external_id is valid' do + it 'should be valid' do + topic.external_id = 'abc_123-ZXY' + expect(topic).to be_valid + end + end + end + context "#title" do it { is_expected.to validate_presence_of :title } @@ -116,6 +161,7 @@ describe Topic do end end end + end it { is_expected.to rate_limit } diff --git a/spec/requests/posts_controller_spec.rb b/spec/requests/posts_controller_spec.rb index 4a751c8e85a..5ebc38d976e 100644 --- a/spec/requests/posts_controller_spec.rb +++ b/spec/requests/posts_controller_spec.rb @@ -835,6 +835,21 @@ describe PostsController do expect(post_1.topic.user.notifications.count).to eq(1) end + it 'allows a topic to be created with an external_id' do + master_key = Fabricate(:api_key).key + post "/posts.json", params: { + raw: 'this is the test content', + title: "this is some post", + external_id: 'external_id' + }, headers: { HTTP_API_USERNAME: user.username, HTTP_API_KEY: master_key } + + expect(response.status).to eq(200) + + new_topic = Topic.last + + expect(new_topic.external_id).to eq('external_id') + end + it 'prevents whispers for regular users' do post_1 = Fabricate(:post) user_key = ApiKey.create!(user: user).key diff --git a/spec/requests/topics_controller_spec.rb b/spec/requests/topics_controller_spec.rb index 7328b2eb934..03c1526e273 100644 --- a/spec/requests/topics_controller_spec.rb +++ b/spec/requests/topics_controller_spec.rb @@ -1792,6 +1792,34 @@ RSpec.describe TopicsController do end end + describe '#show_by_external_id' do + fab!(:private_topic) { Fabricate(:private_message_topic, external_id: 'private') } + fab!(:topic) { Fabricate(:topic, external_id: 'asdf') } + + it 'returns 301 when found' do + get "/t/external_id/asdf.json" + expect(response.status).to eq(301) + expect(response).to redirect_to(topic.relative_url + ".json?page=") + end + + it 'returns right response when not found' do + get "/t/external_id/fdsa.json" + expect(response.status).to eq(404) + end + + describe 'when user does not have access to the topic' do + it 'should return the right response' do + sign_in(user) + + get "/t/external_id/private.json" + + expect(response.status).to eq(403) + expect(response.body).to include(I18n.t('invalid_access')) + end + end + + end + describe '#show' do use_redis_snapshotting diff --git a/spec/serializers/topic_view_serializer_spec.rb b/spec/serializers/topic_view_serializer_spec.rb index 5861d9b3e6f..7432ae959ad 100644 --- a/spec/serializers/topic_view_serializer_spec.rb +++ b/spec/serializers/topic_view_serializer_spec.rb @@ -45,6 +45,17 @@ describe TopicViewSerializer do end end + describe '#external_id' do + describe 'when a topic has an external_id' do + before { topic.update!(external_id: '42-asdf') } + + it 'should return the external_id' do + json = serialize_topic(topic, user) + expect(json[:external_id]).to eq('42-asdf') + end + end + end + describe '#image_url' do fab!(:image_upload) { Fabricate(:image_upload, width: 5000, height: 5000) } diff --git a/spec/serializers/web_hook_topic_view_serializer_spec.rb b/spec/serializers/web_hook_topic_view_serializer_spec.rb index c3e3451e69c..47f0385ab85 100644 --- a/spec/serializers/web_hook_topic_view_serializer_spec.rb +++ b/spec/serializers/web_hook_topic_view_serializer_spec.rb @@ -57,5 +57,9 @@ RSpec.describe WebHookTopicViewSerializer do keys = serializer.as_json.keys expect(serializer.as_json.keys).to contain_exactly(*expected_keys) + + topic.external_id = 'external_id' + expected_keys << :external_id + expect(serializer.as_json.keys).to contain_exactly(*expected_keys) end end