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 <gxtan1990@gmail.com>

* Apply suggestions from code review

Co-authored-by: Alan Guo Xiang Tan <gxtan1990@gmail.com>

Co-authored-by: Alan Guo Xiang Tan <gxtan1990@gmail.com>

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|

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]

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)

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

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+/ }

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

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

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

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 }

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

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
 

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) }
 

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