Add conversation model, <ostatus:conversation /> (#3016)

* Add <ostatus:conversation /> tag to Atom input/output

Only uses ref attribute (not href) because href would be
the alternate link that's always included also.

Creates new conversation for every non-reply status. Carries
over conversation for every reply. Keeps remote URIs verbatim,
generates local URIs on the fly like the rest of them.

* Fix conversation migration

* More spec coverage for status before_create

* Prevent n+1 query when generating Atom with the new conversations

* Improve code style

* Remove redundant local variable

app/lib/atom_serializer.rb

@@ -86,6 +86,7 @@ class AtomSerializer
     append_element(entry, 'link', nil, rel: :alternate, type: 'text/html', href: account_stream_entry_url(stream_entry.account, stream_entry))
     append_element(entry, 'link', nil, rel: :self, type: 'application/atom+xml', href: account_stream_entry_url(stream_entry.account, stream_entry, format: 'atom'))
     append_element(entry, 'thr:in-reply-to', nil, ref: TagManager.instance.uri_for(stream_entry.thread), href: TagManager.instance.url_for(stream_entry.thread)) if stream_entry.threaded?
+    append_element(entry, 'ostatus:conversation', nil, ref: conversation_uri(stream_entry.status.conversation)) unless stream_entry&.status&.conversation_id.nil?
 
     entry
   end
@@ -107,6 +108,7 @@ class AtomSerializer
 
     append_element(object, 'link', nil, rel: :alternate, type: 'text/html', href: TagManager.instance.url_for(status))
     append_element(object, 'thr:in-reply-to', nil, ref: TagManager.instance.uri_for(status.thread), href: TagManager.instance.url_for(status.thread)) if status.reply? && !status.thread.nil?
+    append_element(object, 'ostatus:conversation', nil, ref: conversation_uri(status.conversation)) unless status.conversation_id.nil?
 
     object
   end
@@ -325,6 +327,11 @@ class AtomSerializer
     raw_str.to_s
   end
 
+  def conversation_uri(conversation)
+    return conversation.uri if conversation.uri.present?
+    TagManager.instance.unique_tag(conversation.created_at, conversation.id, 'Conversation')
+  end
+
   def add_namespaces(parent)
     parent['xmlns']          = TagManager::XMLNS
     parent['xmlns:thr']      = TagManager::THR_XMLNS

app/models/conversation.rb

@@ -0,0 +1,20 @@
+# frozen_string_literal: true
+# == Schema Information
+#
+# Table name: conversations
+#
+#  id         :integer          not null, primary key
+#  uri        :string
+#  created_at :datetime         not null
+#  updated_at :datetime         not null
+#
+
+class Conversation < ApplicationRecord
+  validates :uri, uniqueness: true
+
+  has_many :statuses
+
+  def local?
+    uri.nil?
+  end
+end

app/models/status.rb

@@ -21,6 +21,7 @@
 #  favourites_count       :integer          default(0), not null
 #  reblogs_count          :integer          default(0), not null
 #  language               :string           default("en"), not null
+#  conversation_id        :integer
 #
 
 class Status < ApplicationRecord
@@ -34,6 +35,7 @@ class Status < ApplicationRecord
 
   belongs_to :account, inverse_of: :statuses, counter_cache: true, required: true
   belongs_to :in_reply_to_account, foreign_key: 'in_reply_to_account_id', class_name: 'Account'
+  belongs_to :conversation
 
   belongs_to :thread, foreign_key: 'in_reply_to_id', class_name: 'Status', inverse_of: :replies
   belongs_to :reblog, foreign_key: 'reblog_of_id', class_name: 'Status', inverse_of: :reblogs, counter_cache: :reblogs_count
@@ -141,6 +143,11 @@ class Status < ApplicationRecord
     !sensitive? && media_attachments.any?
   end
 
+  before_validation :prepare_contents
+  before_create     :set_reblog
+  before_create     :set_visibility
+  before_create     :set_conversation
+
   class << self
     def in_allowed_languages(account)
       where(language: account.allowed_languages)
@@ -242,17 +249,39 @@ class Status < ApplicationRecord
     end
   end
 
-  before_validation do
+  private
+
+  def prepare_contents
     text&.strip!
     spoiler_text&.strip!
+  end
 
-    self.reply                  = !(in_reply_to_id.nil? && thread.nil?) unless reply
+  def set_reblog
     self.reblog = reblog.reblog if reblog? && reblog.reblog?
-    self.in_reply_to_account_id = (thread.account_id == account_id && thread.reply? ? thread.in_reply_to_account_id : thread.account_id) if reply? && !thread.nil?
+  end
+
+  def set_visibility
     self.visibility = (account.locked? ? :private : :public) if visibility.nil?
   end
 
-  private
+  def set_conversation
+    self.reply = !(in_reply_to_id.nil? && thread.nil?) unless reply
+
+    if reply? && !thread.nil?
+      self.in_reply_to_account_id = carried_over_reply_to_account_id
+      self.conversation_id        = thread.conversation_id if conversation_id.nil?
+    elsif conversation_id.nil?
+      create_conversation
+    end
+  end
+
+  def carried_over_reply_to_account_id
+    if thread.account_id == account_id && thread.reply?
+      thread.in_reply_to_account_id
+    else
+      thread.account_id
+    end
+  end
 
   def filter_from_context?(status, account)
     account&.blocking?(status.account_id) || account&.muting?(status.account_id) || (status.account.silenced? && !account&.following?(status.account_id)) || !status.permitted?(account)

app/models/stream_entry.rb

@@ -22,7 +22,7 @@ class StreamEntry < ApplicationRecord
 
   validates :account, :activity, presence: true
 
-  STATUS_INCLUDES = [:account, :stream_entry, :media_attachments, :tags, mentions: :account, reblog: [:stream_entry, :account, :media_attachments, :tags, mentions: :account], thread: [:stream_entry, :account]].freeze
+  STATUS_INCLUDES = [:account, :stream_entry, :conversation, :media_attachments, :tags, mentions: :account, reblog: [:stream_entry, :account, :conversation, :media_attachments, :tags, mentions: :account], thread: [:stream_entry, :account]].freeze
 
   default_scope { where(activity_type: 'Status') }
   scope :with_includes, -> { includes(:account, status: STATUS_INCLUDES) }

app/services/process_feed_service.rb

@@ -141,7 +141,8 @@ class ProcessFeedService < BaseService
         created_at: published(entry),
         reply: thread?(entry),
         language: content_language(entry),
-        visibility: visibility_scope(entry)
+        visibility: visibility_scope(entry),
+        conversation: find_or_create_conversation(entry)
       )
 
       if thread?(entry)
@@ -164,6 +165,18 @@ class ProcessFeedService < BaseService
       status
     end
 
+    def find_or_create_conversation(xml)
+      uri = xml.at_xpath('./ostatus:conversation', ostatus: TagManager::OS_XMLNS)&.attribute('ref')&.content
+      return if uri.nil?
+
+      if TagManager.instance.local_id?(uri)
+        local_id = TagManager.instance.unique_tag_to_local_id(uri, 'Conversation')
+        return Conversation.find_by(id: local_id)
+      end
+
+      Conversation.find_by(uri: uri)
+    end
+
     def find_status(uri)
       if TagManager.instance.local_id?(uri)
         local_id = TagManager.instance.unique_tag_to_local_id(uri, 'Status')

db/migrate/20170506235850_create_conversations.rb

@@ -0,0 +1,10 @@
+class CreateConversations < ActiveRecord::Migration[5.0]
+  def change
+    create_table :conversations, id: :bigserial do |t|
+      t.string :uri, null: true, default: nil
+      t.timestamps
+    end
+
+    add_index :conversations, :uri, unique: true
+  end
+end

db/migrate/20170507000211_add_conversation_id_to_statuses.rb

@@ -0,0 +1,6 @@
+class AddConversationIdToStatuses < ActiveRecord::Migration[5.0]
+  def change
+    add_column :statuses, :conversation_id, :bigint, null: true, default: nil
+    add_index :statuses, :conversation_id
+  end
+end

db/schema.rb

@@ -10,7 +10,7 @@
 #
 # It's strongly recommended that you check this file into your version control system.
 
-ActiveRecord::Schema.define(version: 20170507141759) do
+ActiveRecord::Schema.define(version: 20170508230434) do
 
   # These are extensions that must be enabled in order to support this database
   enable_extension "plpgsql"
@@ -62,6 +62,19 @@ ActiveRecord::Schema.define(version: 20170507141759) do
     t.index ["account_id", "target_account_id"], name: "index_blocks_on_account_id_and_target_account_id", unique: true, using: :btree
   end
 
+  create_table "conversation_mutes", force: :cascade do |t|
+    t.integer "account_id",      null: false
+    t.bigint  "conversation_id", null: false
+    t.index ["account_id", "conversation_id"], name: "index_conversation_mutes_on_account_id_and_conversation_id", unique: true, using: :btree
+  end
+
+  create_table "conversations", id: :bigserial, force: :cascade do |t|
+    t.string   "uri"
+    t.datetime "created_at", null: false
+    t.datetime "updated_at", null: false
+    t.index ["uri"], name: "index_conversations_on_uri", unique: true, using: :btree
+  end
+
   create_table "domain_blocks", force: :cascade do |t|
     t.string   "domain",       default: "", null: false
     t.datetime "created_at",                null: false
@@ -255,7 +268,9 @@ ActiveRecord::Schema.define(version: 20170507141759) do
     t.integer  "favourites_count",       default: 0,     null: false
     t.integer  "reblogs_count",          default: 0,     null: false
     t.string   "language",               default: "en",  null: false
+    t.bigint   "conversation_id"
     t.index ["account_id"], name: "index_statuses_on_account_id", using: :btree
+    t.index ["conversation_id"], name: "index_statuses_on_conversation_id", using: :btree
     t.index ["in_reply_to_id"], name: "index_statuses_on_in_reply_to_id", using: :btree
     t.index ["reblog_of_id"], name: "index_statuses_on_reblog_of_id", using: :btree
     t.index ["uri"], name: "index_statuses_on_uri", unique: true, using: :btree

spec/fabricators/conversation_fabricator.rb

@@ -0,0 +1,2 @@
+Fabricator(:conversation) do
+end

spec/models/conversation_spec.rb

@@ -0,0 +1,13 @@
+require 'rails_helper'
+
+RSpec.describe Conversation, type: :model do
+  describe '#local?' do
+    it 'returns true when URI is nil' do
+      expect(Fabricate(:conversation).local?).to be true
+    end
+
+    it 'returns false when URI is not nil' do
+      expect(Fabricate(:conversation, uri: 'abc').local?).to be false
+    end
+  end
+end

spec/models/status_spec.rb

@@ -377,4 +377,23 @@ RSpec.describe Status, type: :model do
       expect(results).to include(status)
     end
   end
+
+  describe 'before_create' do
+    it 'sets account being replied to correctly over intermediary nodes' do
+      first_status = Fabricate(:status, account: bob)
+      intermediary = Fabricate(:status, thread: first_status, account: alice)
+      final        = Fabricate(:status, thread: intermediary, account: alice)
+
+      expect(final.in_reply_to_account_id).to eq bob.id
+    end
+
+    it 'creates new conversation for stand-alone status' do
+      expect(Status.create(account: alice, text: 'First').conversation_id).to_not be_nil
+    end
+
+    it 'keeps conversation of parent node' do
+      parent = Fabricate(:status, text: 'First')
+      expect(Status.create(account: alice, thread: parent, text: 'Response').conversation_id).to eq parent.conversation_id
+    end
+  end
 end