diff --git a/backend/app/controllers/posts_controller.rb b/backend/app/controllers/posts_controller.rb index 363b926..e8cae00 100644 --- a/backend/app/controllers/posts_controller.rb +++ b/backend/app/controllers/posts_controller.rb @@ -44,7 +44,7 @@ class PostsController < ApplicationController filtered_posts .joins("LEFT JOIN (#{ pt_max_sql }) pt_max ON pt_max.post_id = posts.id") .reselect('posts.*', Arel.sql("#{ updated_at_all_sql } AS updated_at_all")) - .preload(tags: [:materials, { tag_name: :wiki_page }]) + .preload(tags: [:deerjikists, :materials, { tag_name: :wiki_page }]) .with_attached_thumbnail q = q.where('posts.url LIKE ?', "%#{ url }%") if url @@ -95,7 +95,7 @@ class PostsController < ApplicationController end def random - post = filtered_posts.preload(tags: [:materials, { tag_name: :wiki_page }]) + post = filtered_posts.preload(tags: [:deerjikists, :materials, { tag_name: :wiki_page }]) .order('RAND()') .first return head :not_found unless post @@ -104,7 +104,7 @@ class PostsController < ApplicationController end def show - post = Post.includes(tags: [:materials, { tag_name: :wiki_page }]).find_by(id: params[:id]) + post = Post.includes(tags: [:deerjikists, :materials, { tag_name: :wiki_page }]).find_by(id: params[:id]) return head :not_found unless post render json: PostRepr.base(post, current_user) @@ -173,33 +173,68 @@ class PostsController < ApplicationController return head :unauthorized unless current_user return head :forbidden unless current_user.gte_member? + force = bool?(:force) + merge = bool?(:merge) + return head :bad_request if force && merge + + base_version_no = parse_base_version_no + return head :bad_request if !(force) && !(base_version_no) + title = params[:title].presence tag_names = params[:tags].to_s.split original_created_from = params[:original_created_from] original_created_before = params[:original_created_before] parent_post_ids = parse_parent_post_ids - post = Post.find(params[:id].to_i) + post = nil + conflict_json = nil ApplicationRecord.transaction do - PostVersionRecorder.ensure_snapshot!(post, created_by_user: current_user) - - post.update!(title:, original_created_from:, original_created_before:) + post = Post.lock.find(params[:id].to_i) - normalised_tags = Tag.normalise_tags!(tag_names, with_tagme: false) - TagVersioning.record_tag_snapshots!(normalised_tags, created_by_user: current_user) + base_version = nil + base_snapshot = nil + current_snapshot = nil + unless force + base_version = post.post_versions.find_by!(version_no: base_version_no) - tags = post.tags.nico.to_a + normalised_tags - tags = Tag.expand_parent_tags(tags) - sync_post_tags!(post, tags) + base_snapshot = post_snapshot_from_version(base_version) + current_snapshot = post_snapshot_from_record(post) + end + incoming_snapshot = post_incoming_snapshot(title:, + original_created_from:, + original_created_before:, + tag_names:, + parent_post_ids:) + + snapshot_to_apply = + if force || post.version_no == base_version_no || current_snapshot == base_snapshot + incoming_snapshot + else + changes = post_snapshot_changes(base_snapshot, current_snapshot, incoming_snapshot) + conflicts = changes.select { |change| change[:conflict] } - sync_parent_posts!(post, parent_post_ids) + if merge && conflicts.empty? + merge_post_snapshots(base_snapshot, current_snapshot, incoming_snapshot) + else + conflict_json = post_conflict_json(post:, + base_version_no:, + base_snapshot:, + current_snapshot:, + incoming_snapshot:, + changes:, + conflicts:) + raise ActiveRecord::Rollback + end + end - PostVersionRecorder.record!(post:, event_type: :update, created_by_user: current_user) + apply_post_snapshot!(post, snapshot_to_apply) end + return render json: conflict_json, status: :conflict if conflict_json + post.reload - json = post.as_json + json = PostRepr.base(post, current_user) json['tags'] = build_tag_tree_for(post.tags) render json:, status: :ok rescue Tag::NicoTagNormalisationError @@ -225,7 +260,7 @@ class PostsController < ApplicationController pts = pts.where(post_id: id) if id.present? pts = pts.where(tag_id:) if tag_id.present? pts = pts.includes(:post, :created_user, :deleted_user, - tag: [:materials, { tag_name: :wiki_page }]) + tag: [:deerjikists, :materials, { tag_name: :wiki_page }]) events = [] pts.each do |pt| @@ -404,4 +439,205 @@ class PostsController < ApplicationController PostImplication.create_or_find_by!(post_id: post.id, parent_post_id:) end end + + def parse_base_version_no + version_no = Integer(params[:base_version_no], exception: false) + if version_no&.positive? + version_no + else + nil + end + end + + def post_snapshot_from_version version + { title: version.title, + original_created_from: snapshot_time(version.original_created_from), + original_created_before: snapshot_time(version.original_created_before), + tag_names: editable_tag_names_from_version(version), + parent_post_ids: snapshot_parent_post_ids_from_version(version) } + end + + def editable_tag_names_from_version version + version.tags.to_s.split.reject { |name| name.downcase.start_with?('nico:') }.sort + end + + def post_snapshot_from_record post + { title: post.title, + original_created_from: snapshot_time(post.original_created_from), + original_created_before: snapshot_time(post.original_created_before), + tag_names: editable_tag_names_from_post(post), + parent_post_ids: post.parent_posts.order(:id).pluck(:id) } + end + + def editable_tag_names_from_post post + post.tags.not_nico.joins(:tag_name).order('tag_names.name').pluck('tag_names.name') + end + + def post_incoming_snapshot title:, original_created_from:, original_created_before:, + tag_names:, parent_post_ids: + { title:, + original_created_from: snapshot_time(original_created_from), + original_created_before: snapshot_time(original_created_before), + tag_names: incoming_tag_names_for_snapshot(tag_names), + parent_post_ids: parent_post_ids.sort } + end + + def snapshot_parent_post_ids_from_version version + if version.respond_to?(:parent_post_ids) + version.parent_post_ids.to_s.split.map { |id| id.to_i }.sort + elsif version.respond_to?(:parent_id) && version.parent_id + [version.parent_id] + else + [] + end + end + + def snapshot_time value + return nil if value.blank? + + value = Time.zone.parse(value.to_s) if value in String + value&.in_time_zone&.iso8601(6) + rescue ArgumentError, TypeError + value.to_s + end + + def incoming_tag_names_for_snapshot raw_tag_names + tags = Tag.normalise_tags!(raw_tag_names, with_tagme: false) + + Tag.expand_parent_tags(tags).map(&:name).uniq.sort + end + + def post_conflict_json post:, base_version_no:, base_snapshot:, + current_snapshot:, incoming_snapshot:, changes:, conflicts: + { error: 'conflict', + message: '競合が発生しました．', + post_id: post.id, + base_version_no:, + current_version_no: post.version_no, + base: base_snapshot, + current: current_snapshot, + mine: incoming_snapshot, + changes:, + conflicts:, + mergeable: conflicts.empty? } + end + + def post_snapshot_changes base_snapshot, current_snapshot, incoming_snapshot + [scalar_snapshot_change(:title, 'タイトル', + base_snapshot, current_snapshot, incoming_snapshot), + scalar_snapshot_change(:original_created_from, 'オリジナルの作成日時（以降）', + base_snapshot, current_snapshot, incoming_snapshot), + scalar_snapshot_change(:original_created_before, 'オリジナルの作成日時（より前）', + base_snapshot, current_snapshot, incoming_snapshot), + set_snapshot_change(:tag_names, 'タグ', + base_snapshot, current_snapshot, incoming_snapshot), + set_snapshot_change(:parent_post_ids, '親投稿', + base_snapshot, current_snapshot, incoming_snapshot)].compact + end + + def scalar_snapshot_change field, label, base_snapshot, current_snapshot, incoming_snapshot + base = base_snapshot[field] + current = current_snapshot[field] + mine = incoming_snapshot[field] + + return nil if current == base && mine == base + + { field:, label:, base:, current:, mine:, + changed_by_current: current != base, + changed_by_me: mine != base, + conflict: scalar_snapshot_conflict?(base, current, mine) } + end + + def scalar_snapshot_conflict? base, current, mine + current != base && mine != base && current != mine + end + + def set_snapshot_change field, label, base_snapshot, current_snapshot, incoming_snapshot + base = base_snapshot[field].to_a + current = current_snapshot[field].to_a + mine = incoming_snapshot[field].to_a + + added_by_current = current - base + removed_by_current = base - current + added_by_me = mine - base + removed_by_me = base - mine + + if (added_by_current.empty? && + removed_by_current.empty? && + added_by_me.empty? && + removed_by_me.empty?) + return nil + end + + { field:, label:, base:, current:, mine:, added_by_current:, removed_by_current:, + added_by_me:, removed_by_me:, + changed_by_current: added_by_current.present? || removed_by_current.present?, + changed_by_me: added_by_me.present? || removed_by_me.present?, + conflict: set_snapshot_conflict?(added_by_current:, + removed_by_current:, + added_by_me:, + removed_by_me:) } + end + + def set_snapshot_conflict? added_by_current:, removed_by_current:, + added_by_me:, removed_by_me: + (added_by_current & removed_by_me).present? || (removed_by_current & added_by_me).present? + end + + def apply_post_snapshot! post, snapshot + PostVersionRecorder.ensure_snapshot!(post, created_by_user: current_user) + + post.update!(title: snapshot[:title], + original_created_from: snapshot[:original_created_from], + original_created_before: snapshot[:original_created_before]) + + editable_tags = Tag.normalise_tags!(snapshot[:tag_names], with_tagme: false) + TagVersioning.record_tag_snapshots!(editable_tags, created_by_user: current_user) + + readonly_tags = post.tags.nico.to_a + + tags = readonly_tags + editable_tags + tags = Tag.expand_parent_tags(tags) + + sync_post_tags!(post, tags) + sync_parent_posts!(post, snapshot[:parent_post_ids]) + + PostVersionRecorder.record!(post:, event_type: :update, created_by_user: current_user) + end + + def merge_post_snapshots base_snapshot, current_snapshot, incoming_snapshot + [:title, :original_created_from, :original_created_before].map { + [_1, merge_scalar_snapshot_value(base_snapshot[_1], + current_snapshot[_1], + incoming_snapshot[_1])] + }.to_h.merge([:tag_names, :parent_post_ids].map { + [_1, merge_set_snapshot_value(base_snapshot[_1], + current_snapshot[_1], + incoming_snapshot[_1])] + }.to_h) + end + + def merge_scalar_snapshot_value base, current, mine + return mine if current == base + return current if mine == base || current == mine + + raise ArgumentError, '競合してゐる項目はマージできません．' + end + + def merge_set_snapshot_value base, current, mine + base = base.to_a + current = current.to_a + mine = mine.to_a + + added_by_current = current - base + removed_by_current = base - current + added_by_me = mine - base + removed_by_me = base - mine + + merged = base + added_by_current + added_by_me + merged -= removed_by_current + merged -= removed_by_me + + merged.uniq.sort + end end diff --git a/backend/app/models/post.rb b/backend/app/models/post.rb index d036fae..4c3ddb1 100644 --- a/backend/app/models/post.rb +++ b/backend/app/models/post.rb @@ -28,6 +28,8 @@ class Post < ApplicationRecord has_one_attached :thumbnail + attribute :version_no, :integer, default: 1 + before_validation :normalise_url validates :url, presence: true, uniqueness: true diff --git a/backend/app/models/tag.rb b/backend/app/models/tag.rb index 402e4fe..51ca783 100644 --- a/backend/app/models/tag.rb +++ b/backend/app/models/tag.rb @@ -40,6 +40,8 @@ class Tag < ApplicationRecord belongs_to :tag_name delegate :wiki_page, to: :tag_name + attribute :version_no, :integer, default: 1 + delegate :name, to: :tag_name, allow_nil: true validates :tag_name, presence: true diff --git a/backend/app/models/wiki_page.rb b/backend/app/models/wiki_page.rb index b725706..efe7868 100644 --- a/backend/app/models/wiki_page.rb +++ b/backend/app/models/wiki_page.rb @@ -15,6 +15,8 @@ class WikiPage < ApplicationRecord has_many :wiki_versions + attribute :version_no, :integer, default: 1 + belongs_to :tag_name validates :tag_name, presence: true validates :body, presence: true diff --git a/backend/app/services/version_recorder.rb b/backend/app/services/version_recorder.rb index e705ec3..8175dba 100644 --- a/backend/app/services/version_recorder.rb +++ b/backend/app/services/version_recorder.rb @@ -16,19 +16,20 @@ class VersionRecorder @record = record_class.unscoped.lock.find(@record.id) latest = latest_version - if !(latest) && @event_type != 'create' - raise "#{ version_class.name } first event must be create" - end + validate_version_sequence!(latest) + + attrs = snapshot_attributes - if @event_type == 'create' && latest - raise "#{ version_class.name } create event already exists" + if @event_type == 'update' && latest && same_snapshot?(latest, attrs) + return latest end - attrs = snapshot_attributes + version = version_class.create!( + base_attributes(latest).merge(record_key => @record).merge(attrs)) - return latest if @event_type == 'update' && latest && same_snapshot?(latest, attrs) + update_record_version_no!(version.version_no) - version_class.create!(base_attributes(latest).merge(record_key => @record).merge(attrs)) + version end end @@ -45,7 +46,31 @@ class VersionRecorder created_by_user: @created_by_user } end - def same_snapshot?(version, attrs) = attrs.all? { |k, v| version.public_send(k) == v } + def update_record_version_no! version_no + @record.update_columns(version_no:) + @record.version_no = version_no + end + + def validate_version_sequence! latest + if !(latest) && @event_type != 'create' + raise "#{ version_class.name } first event must be create" + end + + if @event_type == 'create' && latest + raise "#{ version_class.name } create event already exists" + end + + return unless latest + + if @record.version_no != latest.version_no + raise ("#{ record_class.name }##{ @record.id } version_no is #{ @record.version_no }, " + + "but latest #{ version_class.name } version_no is #{ latest.version_no }") + end + end + + def same_snapshot? version, attrs + attrs.all? { |k, v| version.public_send(k) == v } + end def validate_event_type! return if EVENT_TYPES.include?(@event_type) diff --git a/backend/db/migrate/20260507124000_add_version_no_to_posts.rb b/backend/db/migrate/20260507124000_add_version_no_to_posts.rb new file mode 100644 index 0000000..7161a23 --- /dev/null +++ b/backend/db/migrate/20260507124000_add_version_no_to_posts.rb @@ -0,0 +1,27 @@ +class AddVersionNoToPosts < ActiveRecord::Migration[8.0] + def up + add_column :posts, :version_no, :integer + + execute <<~SQL + UPDATE + posts + SET + version_no = ( + SELECT + MAX(version_no) + FROM + post_versions + WHERE + post_id = posts.id) + SQL + + change_column_null :posts, :version_no, false + + add_check_constraint :posts, 'version_no > 0', name: 'chk_posts_version_no_positive' + end + + def down + remove_check_constraint :posts, name: 'chk_posts_version_no_positive' + remove_column :posts, :version_no + end +end diff --git a/backend/db/migrate/20260507211600_add_version_no_to_tags.rb b/backend/db/migrate/20260507211600_add_version_no_to_tags.rb new file mode 100644 index 0000000..2a4ef30 --- /dev/null +++ b/backend/db/migrate/20260507211600_add_version_no_to_tags.rb @@ -0,0 +1,37 @@ +class AddVersionNoToTags < ActiveRecord::Migration[8.0] + def up + add_column :tags, :version_no, :integer + + execute <<~SQL + UPDATE + tags + SET + version_no = ( + CASE category + WHEN 'nico' THEN + (SELECT + MAX(version_no) + FROM + nico_tag_versions + WHERE + tag_id = tags.id) + ELSE + (SELECT + MAX(version_no) + FROM + tag_versions + WHERE + tag_id = tags.id) + END) + SQL + + change_column_null :tags, :version_no, false + + add_check_constraint :tags, 'version_no > 0', name: 'chk_tags_version_no_positive' + end + + def down + remove_check_constraint :tags, name: 'chk_tags_version_no_positive' + remove_column :tags, :version_no + end +end diff --git a/backend/db/migrate/20260507213300_add_version_no_to_wiki_pages.rb b/backend/db/migrate/20260507213300_add_version_no_to_wiki_pages.rb new file mode 100644 index 0000000..be34918 --- /dev/null +++ b/backend/db/migrate/20260507213300_add_version_no_to_wiki_pages.rb @@ -0,0 +1,27 @@ +class AddVersionNoToWikiPages < ActiveRecord::Migration[8.0] + def up + add_column :wiki_pages, :version_no, :integer + + execute <<~SQL + UPDATE + wiki_pages + SET + version_no = ( + SELECT + MAX(version_no) + FROM + wiki_versions + WHERE + wiki_page_id = wiki_pages.id) + SQL + + change_column_null :wiki_pages, :version_no, false + + add_check_constraint :wiki_pages, 'version_no > 0', name: 'chk_wiki_pages_version_no_positive' + end + + def down + remove_check_constraint :wiki_pages, name: 'chk_wiki_pages_version_no_positive' + remove_column :wiki_pages, :version_no + end +end diff --git a/backend/db/schema.rb b/backend/db/schema.rb index 042d227..94edb82 100644 --- a/backend/db/schema.rb +++ b/backend/db/schema.rb @@ -10,7 +10,7 @@ # # It's strongly recommended that you check this file into your version control system. -ActiveRecord::Schema[8.0].define(version: 2026_05_01_153900) do +ActiveRecord::Schema[8.0].define(version: 2026_05_07_213300) do create_table "active_storage_attachments", charset: "utf8mb4", collation: "utf8mb4_0900_ai_ci", force: :cascade do |t| t.string "name", null: false t.string "record_type", null: false @@ -186,8 +186,10 @@ ActiveRecord::Schema[8.0].define(version: 2026_05_01_153900) do t.datetime "original_created_from" t.datetime "original_created_before" t.datetime "updated_at", null: false + t.integer "version_no", null: false t.index ["uploaded_user_id"], name: "index_posts_on_uploaded_user_id" t.index ["url"], name: "index_posts_on_url", unique: true + t.check_constraint "`version_no` > 0", name: "chk_posts_version_no_positive" end create_table "settings", charset: "utf8mb4", collation: "utf8mb4_0900_ai_ci", force: :cascade do |t| @@ -262,8 +264,10 @@ ActiveRecord::Schema[8.0].define(version: 2026_05_01_153900) do t.datetime "updated_at", null: false t.integer "post_count", default: 0, null: false t.datetime "discarded_at" + t.integer "version_no", null: false t.index ["discarded_at"], name: "index_tags_on_discarded_at" t.index ["tag_name_id"], name: "index_tags_on_tag_name_id", unique: true + t.check_constraint "`version_no` > 0", name: "chk_tags_version_no_positive" end create_table "theatre_comments", primary_key: ["theatre_id", "no"], charset: "utf8mb4", collation: "utf8mb4_0900_ai_ci", force: :cascade do |t| @@ -369,10 +373,12 @@ ActiveRecord::Schema[8.0].define(version: 2026_05_01_153900) do t.datetime "updated_at", null: false t.datetime "discarded_at" t.integer "next_asset_no", default: 1, null: false + t.integer "version_no", null: false t.index ["created_user_id"], name: "index_wiki_pages_on_created_user_id" t.index ["discarded_at"], name: "index_wiki_pages_on_discarded_at" t.index ["tag_name_id"], name: "index_wiki_pages_on_tag_name_id", unique: true t.index ["updated_user_id"], name: "index_wiki_pages_on_updated_user_id" + t.check_constraint "`version_no` > 0", name: "chk_wiki_pages_version_no_positive" end create_table "wiki_revision_lines", charset: "utf8mb4", collation: "utf8mb4_0900_ai_ci", force: :cascade do |t| diff --git a/backend/spec/requests/posts_spec.rb b/backend/spec/requests/posts_spec.rb index 8dc0a16..14b51e7 100644 --- a/backend/spec/requests/posts_spec.rb +++ b/backend/spec/requests/posts_spec.rb @@ -10,6 +10,10 @@ RSpec.describe 'Posts API', type: :request do allow_any_instance_of(Post).to receive(:resized_thumbnail!).and_return(true) end + def create_nico_tag!(name) + Tag.find_or_create_by_tag_name!(name, category: :nico) + end + def dummy_upload # 中身は何でもいい（加工処理はスタブしてる） Rack::Test::UploadedFile.new(StringIO.new('dummy'), 'image/jpeg', original_filename: 'dummy.jpg') @@ -23,21 +27,34 @@ RSpec.describe 'Posts API', type: :request do Post.create!(title:, url:) end - def create_post_version_for! post - PostVersion.create!( - post:, - version_no: 1, - event_type: 'create', - title: post.title, - url: post.url, - thumbnail_base: post.thumbnail_base, - tags: post.snapshot_tag_names.join(' '), - parent_post_ids: post.snapshot_parent_post_ids.join(' '), - original_created_from: post.original_created_from, - original_created_before: post.original_created_before, - created_at: post.created_at, - created_by_user: post.uploaded_user - ) + def create_post_version_for!(post) + version = + PostVersion.create!( + post:, + version_no: 1, + event_type: 'create', + title: post.title, + url: post.url, + thumbnail_base: post.thumbnail_base, + tags: post.snapshot_tag_names.join(' '), + parent_post_ids: post.snapshot_parent_post_ids.join(' '), + original_created_from: post.original_created_from, + original_created_before: post.original_created_before, + created_at: post.created_at, + created_by_user: post.uploaded_user) + + post.update_columns(version_no: version.version_no) if post.has_attribute?(:version_no) + post.version_no = version.version_no if post.respond_to?(:version_no=) + + version + end + + def post_update_params(post, params = { }) + base_version = + post.post_versions.order(version_no: :desc).first || + create_post_version_for!(post.reload) + + post_write_params({ base_version_no: base_version.version_no }.merge(params)) end let!(:tag_name) { TagName.create!(name: 'spec_tag') } @@ -806,24 +823,26 @@ RSpec.describe 'Posts API', type: :request do it '401 when not logged in' do sign_out - put "/posts/#{post_record.id}", params: post_write_params(title: 'updated', tags: 'spec_tag') + put "/posts/#{post_record.id}", params: post_update_params( + post_record, title: 'updated', tags: 'spec_tag') expect(response).to have_http_status(:unauthorized) end it '403 when not member' do sign_in_as(create(:user, role: 'guest')) - put "/posts/#{post_record.id}", params: post_write_params(title: 'updated', tags: 'spec_tag') + put "/posts/#{post_record.id}", params: post_update_params( + post_record, title: 'updated', tags: 'spec_tag') expect(response).to have_http_status(:forbidden) end it '200 and updates title + resync tags when member' do sign_in_as(member) - # 追加で別タグも作って、更新時に入れ替わることを見る tn2 = TagName.create!(name: 'spec_tag_2') Tag.create!(tag_name: tn2, category: :general) - put "/posts/#{post_record.id}", params: post_write_params( + put "/posts/#{post_record.id}", params: post_update_params( + post_record, title: 'updated title', tags: 'spec_tag_2') @@ -831,7 +850,6 @@ RSpec.describe 'Posts API', type: :request do expect(json).to have_key('tags') expect(json['tags']).to be_an(Array) - # show と同様、update 後レスポンスもツリー形式 names = json['tags'].map { |n| n['name'] } expect(names).to include('spec_tag_2') end @@ -846,10 +864,10 @@ RSpec.describe 'Posts API', type: :request do it 'return 400' do sign_in_as(member) - put "/posts/#{post_record.id}", params: post_write_params( - title: 'updated title', - tags: 'nico:nico_tag' - ) + put "/posts/#{post_record.id}", params: post_update_params( + post_record, + title: 'updated title', + tags: 'nico:nico_tag') expect(response).to have_http_status(:bad_request), response.body end @@ -887,11 +905,11 @@ RSpec.describe 'Posts API', type: :request do it 'replaces parent posts' do sign_in_as(member) - put "/posts/#{post_record.id}", params: post_write_params( - title: 'updated title', - tags: 'spec_tag', - parent_post_ids: "#{new_parent_post_1.id} #{new_parent_post_2.id}" - ) + put "/posts/#{post_record.id}", params: post_update_params( + post_record, + title: 'updated title', + tags: 'spec_tag', + parent_post_ids: "#{new_parent_post_1.id} #{new_parent_post_2.id}") expect(response).to have_http_status(:ok) @@ -908,7 +926,8 @@ RSpec.describe 'Posts API', type: :request do it 'clears parent posts when parent_post_ids is blank' do sign_in_as(member) - put "/posts/#{post_record.id}", params: post_write_params( + put "/posts/#{post_record.id}", params: post_update_params( + post_record, title: 'updated title', tags: 'spec_tag', parent_post_ids: '' @@ -922,7 +941,8 @@ RSpec.describe 'Posts API', type: :request do sign_in_as(member) create_post_version_for!(post_record.reload) - put "/posts/#{post_record.id}", params: post_write_params( + put "/posts/#{post_record.id}", params: post_update_params( + post_record, title: 'updated title', tags: 'spec_tag', parent_post_ids: "#{new_parent_post_1.id} #{new_parent_post_2.id}" @@ -943,7 +963,10 @@ RSpec.describe 'Posts API', type: :request do it 'returns 422' do sign_in_as(member) + base_version = create_post_version_for!(post_record.reload) + put "/posts/#{post_record.id}", params: { + base_version_no: base_version.version_no, title: 'updated title', tags: 'spec_tag' } @@ -966,7 +989,8 @@ RSpec.describe 'Posts API', type: :request do parent_post: ) - put "/posts/#{post_record.id}", params: post_write_params( + put "/posts/#{post_record.id}", params: post_update_params( + post_record, title: 'updated title', tags: 'spec_tag', parent_post_ids: 'abc' @@ -991,7 +1015,8 @@ RSpec.describe 'Posts API', type: :request do parent_post: ) - put "/posts/#{post_record.id}", params: post_write_params( + put "/posts/#{post_record.id}", params: post_update_params( + post_record, title: 'updated title', tags: 'spec_tag', parent_post_ids: '999999999' @@ -1006,7 +1031,8 @@ RSpec.describe 'Posts API', type: :request do it 'returns 422 and does not create self implication' do sign_in_as(member) - put "/posts/#{post_record.id}", params: post_write_params( + put "/posts/#{post_record.id}", params: post_update_params( + post_record, title: 'updated title', tags: 'spec_tag', parent_post_ids: post_record.id.to_s @@ -1020,6 +1046,221 @@ RSpec.describe 'Posts API', type: :request do )).to be(false) end end + + context 'with optimistic locking' do + let!(:no_deerjikist_tag) { Tag.no_deerjikist } + + before do + PostTag.create!(post: post_record, tag: no_deerjikist_tag) + end + + it '400 when base_version_no is missing without force' do + sign_in_as(member) + + put "/posts/#{post_record.id}", params: post_write_params( + title: 'updated title', + tags: 'spec_tag') + + expect(response).to have_http_status(:bad_request) + end + + it '400 when force and merge are both true' do + sign_in_as(member) + + put "/posts/#{post_record.id}", params: post_write_params( + title: 'updated title', + tags: 'spec_tag', + force: '1', + merge: '1') + + expect(response).to have_http_status(:bad_request) + end + + it '409 when scalar fields are changed both by current and incoming updates' do + sign_in_as(member) + + base_version = create_post_version_for!(post_record.reload) + + post_record.update!(title: 'updated by other user') + PostVersionRecorder.record!( + post: post_record.reload, + event_type: :update, + created_by_user: member) + + put "/posts/#{post_record.id}", params: post_write_params( + base_version_no: base_version.version_no, + title: 'updated by me', + tags: "spec_tag #{Tag.no_deerjikist.name}") + + expect(response).to have_http_status(:conflict) + + expect(json.fetch('error')).to eq('conflict') + expect(json.fetch('base_version_no')).to eq(base_version.version_no) + expect(json.fetch('current_version_no')).to eq(2) + expect(json.fetch('mergeable')).to be(false) + + conflict_fields = json.fetch('conflicts').map { |change| change.fetch('field') } + expect(conflict_fields).to include('title') + + expect(post_record.reload.title).to eq('updated by other user') + end + + it 'returns 409 with mergeable true when stale tag changes do not conflict but merge is not requested' do + sign_in_as(member) + + base_version = create_post_version_for!(post_record.reload) + + current_tag = Tag.find_or_create_by_tag_name!('current_added_tag', category: :general) + PostTag.create!(post: post_record, tag: current_tag, created_user: member) + + PostVersionRecorder.record!( + post: post_record.reload, + event_type: :update, + created_by_user: member) + + put "/posts/#{post_record.id}", params: post_write_params( + base_version_no: base_version.version_no, + title: post_record.title, + tags: "spec_tag #{Tag.no_deerjikist.name} incoming_added_tag") + + expect(response).to have_http_status(:conflict) + + expect(json.fetch('mergeable')).to be(true) + + tag_change = json.fetch('changes').find { |change| change.fetch('field') == 'tag_names' } + expect(tag_change).to be_present + expect(tag_change.fetch('conflict')).to be(false) + expect(tag_change.fetch('added_by_current')).to include('current_added_tag') + expect(tag_change.fetch('added_by_me')).to include('incoming_added_tag') + end + + it 'merges non-conflicting stale tag changes when merge is true' do + sign_in_as(member) + + base_version = create_post_version_for!(post_record.reload) + + current_tag = Tag.find_or_create_by_tag_name!('current_merge_tag', category: :general) + PostTag.create!(post: post_record, tag: current_tag, created_user: member) + + PostVersionRecorder.record!( + post: post_record.reload, + event_type: :update, + created_by_user: member) + + put "/posts/#{post_record.id}", params: post_write_params( + base_version_no: base_version.version_no, + title: post_record.title, + tags: "spec_tag #{Tag.no_deerjikist.name} incoming_merge_tag", + merge: '1') + + expect(response).to have_http_status(:ok) + + names = post_record.reload.tags.map(&:name) + + expect(names).to include('spec_tag') + expect(names).to include(Tag.no_deerjikist.name) + expect(names).to include('current_merge_tag') + expect(names).to include('incoming_merge_tag') + end + + it 'does not conflict when only nico tags changed after the base version' do + sign_in_as(member) + + base_version = create_post_version_for!(post_record.reload) + + nico_tag = create_nico_tag!('nico:optimistic_lock_nico') + PostTag.create!(post: post_record, tag: nico_tag, created_user: member) + + PostVersionRecorder.record!( + post: post_record.reload, + event_type: :update, + created_by_user: member) + + expect(post_record.reload.version_no).to eq(2) + + put "/posts/#{post_record.id}", params: post_write_params( + base_version_no: base_version.version_no, + title: post_record.title, + tags: "spec_tag #{ Tag.no_deerjikist.name }") + + expect(response).to have_http_status(:ok) + + names = post_record.reload.tags.map(&:name) + + expect(names).to include('spec_tag') + expect(names).to include(Tag.no_deerjikist.name) + expect(names).to include(nico_tag.name) + end + + it 'keeps nico tags even when they are not included in PUT tags' do + sign_in_as(member) + + nico_tag = create_nico_tag!('nico:readonly_update_nico') + PostTag.create!(post: post_record, tag: nico_tag, created_user: member) + + base_version = create_post_version_for!(post_record.reload) + + put "/posts/#{post_record.id}", params: post_write_params( + base_version_no: base_version.version_no, + title: 'updated title', + tags: "spec_tag #{ Tag.no_deerjikist.name }") + + expect(response).to have_http_status(:ok) + + names = post_record.reload.tags.map(&:name) + + expect(names).to include('spec_tag') + expect(names).to include(Tag.no_deerjikist.name) + expect(names).to include(nico_tag.name) + end + + it 'allows non-nico tags linked from nico tags to be removed by normal post update' do + sign_in_as(member) + + nico_tag = create_nico_tag!('nico:relation_source') + linked_tag = Tag.find_or_create_by_tag_name!('relation_linked_tag', category: :general) + + NicoTagRelation.create!(nico_tag:, tag: linked_tag) + PostTag.create!(post: post_record, tag: nico_tag, created_user: member) + PostTag.create!(post: post_record, tag: linked_tag, created_user: member) + + base_version = create_post_version_for!(post_record.reload) + + put "/posts/#{post_record.id}", params: post_write_params( + base_version_no: base_version.version_no, + title: post_record.title, + tags: "spec_tag #{ Tag.no_deerjikist.name }") + + expect(response).to have_http_status(:ok) + + names = post_record.reload.tags.map(&:name) + + expect(names).to include(nico_tag.name) + expect(names).to include('spec_tag') + expect(names).to include(Tag.no_deerjikist.name) + expect(names).not_to include(linked_tag.name) + end + + it 'force-updates stale posts without base_version_no' do + sign_in_as(member) + + create_post_version_for!(post_record.reload) + + post_record.update!(title: 'updated by other user') + PostVersionRecorder.record!( + post: post_record.reload, + event_type: :update, + created_by_user: member) + + put "/posts/#{post_record.id}", params: post_write_params( + title: 'forced title', + tags: "spec_tag #{Tag.no_deerjikist.name}", + force: '1') + + expect(response).to have_http_status(:ok) + expect(post_record.reload.title).to eq('forced title') + end + end end describe 'GET /posts/random' do @@ -1434,13 +1675,14 @@ RSpec.describe 'Posts API', type: :request do it 'creates next version on PUT /posts/:id when snapshot changes' do sign_in_as(member) - create_post_version_for!(post_record) + base_version = create_post_version_for!(post_record) tag_name2 = TagName.create!(name: 'spec_tag_2') Tag.create!(tag_name: tag_name2, category: :general) expect do put "/posts/#{post_record.id}", params: post_write_params( + base_version_no: base_version.version_no, title: 'updated title', tags: 'spec_tag_2') end.to change(PostVersion, :count).by(1) @@ -1459,13 +1701,15 @@ RSpec.describe 'Posts API', type: :request do sign_in_as(member) PostTag.create!(post: post_record, tag: Tag.no_deerjikist) - create_post_version_for!(post_record.reload) + base_version = create_post_version_for!(post_record.reload) expect { put "/posts/#{post_record.id}", params: post_write_params( + base_version_no: base_version.version_no, title: post_record.title, tags: 'spec_tag') }.not_to change(PostVersion, :count) + expect(response).to have_http_status(:ok) version = post_record.reload.post_versions.order(:version_no).last @@ -1490,10 +1734,11 @@ RSpec.describe 'Posts API', type: :request do it 'does not create a version when PUT /posts/:id is invalid' do sign_in_as(member) - create_post_version_for!(post_record) + base_version = create_post_version_for!(post_record) expect do put "/posts/#{post_record.id}", params: post_write_params( + base_version_no: base_version.version_no, title: 'updated title', tags: 'spec_tag', original_created_from: Time.zone.local(2020, 1, 2, 0, 0, 0).iso8601, @@ -1507,46 +1752,22 @@ RSpec.describe 'Posts API', type: :request do describe 'tag versioning from post write actions' do let(:member) { create(:user, :member) } - it 'creates tag snapshot for normalised tags on POST /posts' do - sign_in_as(member) - - expect { - post '/posts', params: post_write_params( - title: 'tag versioned post', - url: 'https://example.com/tag-versioned-post', - tags: 'spec_tag', - thumbnail: dummy_upload) - }.to change { tag.reload.tag_versions.count }.by(1) - - expect(response).to have_http_status(:created) - - version = tag.reload.tag_versions.order(:version_no).last - expect(version.version_no).to eq(1) - expect(version.event_type).to eq('create') - expect(version.name).to eq('spec_tag') - expect(version.category).to eq('general') - expect(version.created_by_user_id).to eq(member.id) - end - it 'creates tag snapshot for normalised tags on PUT /posts/:id' do sign_in_as(member) + base_version = create_post_version_for!(post_record.reload) + tag_name2 = TagName.create!(name: 'spec_tag_2') tag2 = Tag.create!(tag_name: tag_name2, category: :general) expect { put "/posts/#{post_record.id}", params: post_write_params( + base_version_no: base_version.version_no, title: 'updated title', tags: 'spec_tag_2') }.to change { tag2.reload.tag_versions.count }.by(1) - expect(response).to have_http_status(:ok) - - version = tag2.reload.tag_versions.order(:version_no).last - expect(version.version_no).to eq(1) - expect(version.event_type).to eq('create') - expect(version.name).to eq('spec_tag_2') - expect(version.created_by_user_id).to eq(member.id) + expect(response).to have_http_status(:ok), response.body end end end diff --git a/backend/spec/requests/tag_versions_spec.rb b/backend/spec/requests/tag_versions_spec.rb index aae790c..8f092b3 100644 --- a/backend/spec/requests/tag_versions_spec.rb +++ b/backend/spec/requests/tag_versions_spec.rb @@ -26,17 +26,22 @@ RSpec.describe 'TagVersions API', type: :request do created_by_user:, created_at: ) - TagVersion.create!( - tag: tag, - version_no: version_no, - event_type: event_type, - name: name, - category: category, - aliases: Array(aliases).join(' '), - parent_tag_ids: Array(parent_tags).map(&:id).join(' '), - created_by_user: created_by_user, - created_at: created_at - ) + version = + TagVersion.create!( + tag: tag, + version_no: version_no, + event_type: event_type, + name: name, + category: category, + aliases: Array(aliases).join(' '), + parent_tag_ids: Array(parent_tags).map(&:id).join(' '), + created_by_user: created_by_user, + created_at: created_at) + + tag.update_columns(version_no: version_no) if tag.has_attribute?(:version_no) + tag.version_no = version_no if tag.respond_to?(:version_no=) + + version end let!(:v1) do diff --git a/backend/spec/services/version_recorder_spec.rb b/backend/spec/services/version_recorder_spec.rb new file mode 100644 index 0000000..22080dd --- /dev/null +++ b/backend/spec/services/version_recorder_spec.rb @@ -0,0 +1,85 @@ +require 'rails_helper' + +RSpec.describe VersionRecorder do + let(:member) { create(:user, :member) } + + let(:post_record) do + Post.create!( + title: 'version recorder post', + url: 'https://example.com/version-recorder-post') + end + + it 'updates record version_no when creating the first version' do + version = + PostVersionRecorder.record!( + post: post_record, + event_type: :create, + created_by_user: member) + + expect(version.version_no).to eq(1) + expect(post_record.reload.version_no).to eq(1) + end + + it 'updates record version_no when creating the next version' do + PostVersionRecorder.record!( + post: post_record, + event_type: :create, + created_by_user: member) + + post_record.update!(title: 'updated version recorder post') + + version = + PostVersionRecorder.record!( + post: post_record.reload, + event_type: :update, + created_by_user: member) + + expect(version.version_no).to eq(2) + expect(post_record.reload.version_no).to eq(2) + end + + it 'does not create a new version or advance version_no when snapshot is unchanged' do + first = + PostVersionRecorder.record!( + post: post_record, + event_type: :create, + created_by_user: member) + + expect { + version = + PostVersionRecorder.record!( + post: post_record.reload, + event_type: :update, + created_by_user: member) + + expect(version).to eq(first) + }.not_to change(PostVersion, :count) + + expect(post_record.reload.version_no).to eq(1) + end + + it 'raises when record version_no is older than the latest version' do + PostVersionRecorder.record!( + post: post_record, + event_type: :create, + created_by_user: member) + + post_record.update!(title: 'updated once') + + PostVersionRecorder.record!( + post: post_record.reload, + event_type: :update, + created_by_user: member) + + post_record.update_columns(version_no: 1) + + post_record.update!(title: 'updated with stale version_no') + + expect { + PostVersionRecorder.record!( + post: post_record.reload, + event_type: :update, + created_by_user: member) + }.to raise_error(RuntimeError, /version_no/) + end +end diff --git a/frontend/src/App.tsx b/frontend/src/App.tsx index f52209d..da11ba2 100644 --- a/frontend/src/App.tsx +++ b/frontend/src/App.tsx @@ -8,6 +8,7 @@ import { BrowserRouter, import RouteBlockerOverlay from '@/components/RouteBlockerOverlay' import TopNav from '@/components/TopNav' +import DialogueProvider from '@/components/dialogues/DialogueProvider' import { Toaster } from '@/components/ui/toaster' import { apiPost, isApiError } from '@/lib/api' import DeerjikistDetailPage from '@/pages/deerjikists/DeerjikistDetailPage' @@ -138,17 +139,21 @@ export default (() => { return ( <> + - - - - - - - + + + + + + + + + + ) }) satisfies FC diff --git a/frontend/src/components/PostEditForm.tsx b/frontend/src/components/PostEditForm.tsx index 3ac2077..d06b987 100644 --- a/frontend/src/components/PostEditForm.tsx +++ b/frontend/src/components/PostEditForm.tsx @@ -3,11 +3,12 @@ import { useEffect, useState } from 'react' import PostFormTagsArea from '@/components/PostFormTagsArea' import PostOriginalCreatedTimeField from '@/components/PostOriginalCreatedTimeField' import Label from '@/components/common/Label' +import { useDialogue } from '@/components/dialogues/DialogueProvider' import { Button } from '@/components/ui/button' import { toast } from '@/components/ui/use-toast' -import { apiPut } from '@/lib/api' +import { updatePost } from '@/lib/posts' -import type { FC } from 'react' +import type { FC, FormEvent } from 'react' import type { Post, Tag } from '@/types' @@ -32,6 +33,7 @@ type Props = { post: Post export default (({ post, onSave }: Props) => { + const [disabled, setDisabled] = useState (false) const [originalCreatedBefore, setOriginalCreatedBefore] = useState (post.originalCreatedBefore) const [originalCreatedFrom, setOriginalCreatedFrom] = @@ -41,16 +43,14 @@ export default (({ post, onSave }: Props) => { const [tags, setTags] = useState ('') const [title, setTitle] = useState (post.title) - const handleSubmit = async () => { + const dialogue = useDialogue () + + const update = async (...args: Parameters) => { try { - const data = await apiPut ( - `/posts/${ post.id }`, - { title, tags, parent_post_ids: parentPostIds, - original_created_from: originalCreatedFrom, - original_created_before: originalCreatedBefore }, - { headers: { 'Content-Type': 'multipart/form-data' } }) + const data = await updatePost (...args) onSave ({ ...post, + versionNo: data.versionNo, title: data.title, tags: data.tags, parentPosts: data.parentPosts, @@ -60,9 +60,58 @@ export default (({ post, onSave }: Props) => { originalCreatedBefore: data.originalCreatedBefore } as Post) toast ({ description: '更新しました．' }) } - catch + catch (e) { - toast ({ description: '更新はできなかったよ……' }) + const response = (e as any)?.response + + if (response?.status !== 409) + { + toast ({ description: '更新はできなかったよ……' }) + return + } + + const action = await dialogue.choice ({ + title: '競合が発生しました．', + description: ( +
+

ほかの耕作員が先に更新してゐます．

+

現在の変更をどう扱ひますか?

+
), + choices: [...(response?.data?.mergeable ? [{ value: 'merge', label: '差分をマージ' }] : []), + { value: 'overwrite', label: '強制上書き', variant: 'danger' }] }) + + if (action === 'merge') + { + // TODO: 差分 UI + await update ({ id: post.id, title, tags, parentPostIds, + originalCreatedFrom, originalCreatedBefore }, + { baseVersionNo: post.versionNo, merge: true }) + return + } + + if (action === 'overwrite') + { + await update ({ id: post.id, title, tags, parentPostIds, + originalCreatedFrom, originalCreatedBefore }, + { baseVersionNo: post.versionNo, force: true }) + return + } + } + } + + const handleSubmit = async (e: FormEvent) => { + e.preventDefault () + + setDisabled (true) + try + { + await update ({ id: post.id, title, tags, parentPostIds, + originalCreatedFrom, originalCreatedBefore }, + { baseVersionNo: post.versionNo }) + } + finally + { + setDisabled (false) } } @@ -71,14 +120,16 @@ export default (({ post, onSave }: Props) => { }, [post]) return ( -
+
{/* タイトル */}
- setTitle (ev.target.value)}/> + setTitle (ev.target.value)}/>
{/* 親投稿 */} @@ -86,25 +137,31 @@ export default (({ post, onSave }: Props) => { setParentPostIds (e.target.value)} className="w-full border p-2 rounded"/>
{/* タグ */} - + {/* オリジナルの作成日時 */} {/* 送信 */} - - ) + ) }) satisfies FC diff --git a/frontend/src/components/PostEmbed.tsx b/frontend/src/components/PostEmbed.tsx index 45a2a8e..1be00ce 100644 --- a/frontend/src/components/PostEmbed.tsx +++ b/frontend/src/components/PostEmbed.tsx @@ -3,6 +3,7 @@ import YoutubeEmbed from 'react-youtube' import NicoViewer from '@/components/NicoViewer' import TwitterEmbed from '@/components/TwitterEmbed' +import { useDialogue } from '@/components/dialogues/DialogueProvider' import type { FC, RefObject } from 'react' @@ -16,6 +17,8 @@ type Props = { export default (({ ref, post, onLoadComplete, onMetadataChange }: Props) => { + const dialogue = useDialogue () + const url = new URL (post.url) switch (url.hostname.split ('.').slice (-2).join ('.')) @@ -82,12 +85,17 @@ export default (({ ref, post, onLoadComplete, onMetadataChange }: Props) => { height={360}/>) : (
- { + { e.preventDefault () - setFramed (confirm ('未確認の外部ページを表示します。\n' - + '悪意のあるスクリプトが実行される可能性があります。\n' - + '表示しますか?')) - return + + setFramed (await dialogue.confirm ({ + title: '未確認の外部ページを表示します。', + description: ( +
+

悪意のあるスクリプトが実行される可能性があります。

+

表示しますか?

+
), + confirmText: '表示' })) }}> 外部ページを表示 diff --git a/frontend/src/components/PostFormTagsArea.tsx b/frontend/src/components/PostFormTagsArea.tsx index 92450c1..7588202 100644 --- a/frontend/src/components/PostFormTagsArea.tsx +++ b/frontend/src/components/PostFormTagsArea.tsx @@ -7,7 +7,7 @@ import Label from '@/components/common/Label' import TextArea from '@/components/common/TextArea' import { apiGet } from '@/lib/api' -import type { FC, SyntheticEvent } from 'react' +import type { ComponentPropsWithoutRef, FC, SyntheticEvent } from 'react' import type { Tag } from '@/types' @@ -31,12 +31,12 @@ const replaceToken = (value: string, start: number, end: number, text: string) = `${ value.slice (0, start) }${ text }${ value.slice (end) }` -type Props = { +type Props = Omit, 'value' | 'onChange' | 'onBlur'> & { tags: string setTags: (tags: string) => void } -export default (({ tags, setTags }: Props) => { +export default (({ tags, setTags, ...rest }: Props) => { const ref = useRef (null) const [bounds, setBounds] = useState<{ start: number; end: number }> ({ start: 0, end: 0 }) @@ -76,6 +76,7 @@ export default (({ tags, setTags }: Props) => {