#171

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

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

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

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={() => {