From d8b028a4dda836bb0db891eca535bf33a4f3d4fe Mon Sep 17 00:00:00 2001 From: miteruzo Date: Sun, 10 May 2026 06:52:12 +0900 Subject: [PATCH] #171 --- backend/spec/requests/posts_spec.rb | 351 ++++++++++++++---- backend/spec/requests/tag_versions_spec.rb | 27 +- .../spec/services/version_recorder_spec.rb | 85 +++++ frontend/src/components/PostList.tsx | 2 +- 4 files changed, 388 insertions(+), 77 deletions(-) create mode 100644 backend/spec/services/version_recorder_spec.rb 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/components/PostList.tsx b/frontend/src/components/PostList.tsx index 21a4410..c8818db 100644 --- a/frontend/src/components/PostList.tsx +++ b/frontend/src/components/PostList.tsx @@ -42,7 +42,7 @@ export default (({ posts, onClick }: Props) => { layoutId={layoutId} className={cn ('w-full h-full overflow-hidden rounded-xl shadow', 'transform-gpu will-change-transform', - (post.childPosts ?? []).length > 0 && 'outline-4 outline-green-500', + (post.childPosts ?? []).length > 0 && 'ring-4 ring-green-500', (post.parentPosts ?? []).length > 0 && 'ring-4 ring-yellow-500')} whileHover={{ scale: 1.02 }} onLayoutAnimationStart={() => {