From 5abdc77c8060a62ecf2259a1e9d63e862b9f7be7 Mon Sep 17 00:00:00 2001 From: Eugen Rochko Date: Fri, 12 May 2017 19:09:21 +0200 Subject: [PATCH] Add conversation model, (#3016) * Add 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 | 7 +++ app/models/conversation.rb | 20 +++++++++ app/models/status.rb | 43 ++++++++++++++++--- app/models/stream_entry.rb | 2 +- app/services/process_feed_service.rb | 15 ++++++- .../20170506235850_create_conversations.rb | 10 +++++ ...7000211_add_conversation_id_to_statuses.rb | 6 +++ db/schema.rb | 17 +++++++- spec/fabricators/conversation_fabricator.rb | 2 + spec/models/conversation_spec.rb | 13 ++++++ spec/models/status_spec.rb | 19 ++++++++ 11 files changed, 144 insertions(+), 10 deletions(-) create mode 100644 app/models/conversation.rb create mode 100644 db/migrate/20170506235850_create_conversations.rb create mode 100644 db/migrate/20170507000211_add_conversation_id_to_statuses.rb create mode 100644 spec/fabricators/conversation_fabricator.rb create mode 100644 spec/models/conversation_spec.rb diff --git a/app/lib/atom_serializer.rb b/app/lib/atom_serializer.rb index 288a9cfadc..3113feac91 100644 --- a/app/lib/atom_serializer.rb +++ b/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 diff --git a/app/models/conversation.rb b/app/models/conversation.rb new file mode 100644 index 0000000000..fbec961c74 --- /dev/null +++ b/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 diff --git a/app/models/status.rb b/app/models/status.rb index 4808b1a64f..14c6dd9f69 100644 --- a/app/models/status.rb +++ b/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! - - self.reply = !(in_reply_to_id.nil? && thread.nil?) unless reply - 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? - self.visibility = (account.locked? ? :private : :public) if visibility.nil? end - private + def set_reblog + self.reblog = reblog.reblog if reblog? && reblog.reblog? + end + + def set_visibility + self.visibility = (account.locked? ? :private : :public) if visibility.nil? + end + + 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) diff --git a/app/models/stream_entry.rb b/app/models/stream_entry.rb index d451e0dded..fb349f35c2 100644 --- a/app/models/stream_entry.rb +++ b/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) } diff --git a/app/services/process_feed_service.rb b/app/services/process_feed_service.rb index 1558f87906..afd44aafe4 100644 --- a/app/services/process_feed_service.rb +++ b/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') diff --git a/db/migrate/20170506235850_create_conversations.rb b/db/migrate/20170506235850_create_conversations.rb new file mode 100644 index 0000000000..eaf66ded7c --- /dev/null +++ b/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 diff --git a/db/migrate/20170507000211_add_conversation_id_to_statuses.rb b/db/migrate/20170507000211_add_conversation_id_to_statuses.rb new file mode 100644 index 0000000000..d1ef7c2905 --- /dev/null +++ b/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 diff --git a/db/schema.rb b/db/schema.rb index 21fce0df78..76624f07a3 100644 --- a/db/schema.rb +++ b/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 diff --git a/spec/fabricators/conversation_fabricator.rb b/spec/fabricators/conversation_fabricator.rb new file mode 100644 index 0000000000..b4fadb46b8 --- /dev/null +++ b/spec/fabricators/conversation_fabricator.rb @@ -0,0 +1,2 @@ +Fabricator(:conversation) do +end diff --git a/spec/models/conversation_spec.rb b/spec/models/conversation_spec.rb new file mode 100644 index 0000000000..8b5e4fdaf7 --- /dev/null +++ b/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 diff --git a/spec/models/status_spec.rb b/spec/models/status_spec.rb index f5c65f5c99..b21fcd1905 100644 --- a/spec/models/status_spec.rb +++ b/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