From 8e7ad9eb97edf8353d24512746d156706bc75dfe Mon Sep 17 00:00:00 2001 From: evazion Date: Sat, 31 Mar 2018 11:44:49 -0500 Subject: [PATCH 1/2] Post#copy_notes_to: wrap in transaction. --- app/models/post.rb | 42 ++++++++++++++++++++++-------------------- 1 file changed, 22 insertions(+), 20 deletions(-) diff --git a/app/models/post.rb b/app/models/post.rb index 9e6a211e0..c0cd10fbf 100644 --- a/app/models/post.rb +++ b/app/models/post.rb @@ -1415,29 +1415,31 @@ class Post < ApplicationRecord end def copy_notes_to(other_post) - if id == other_post.id - errors.add :base, "Source and destination posts are the same" - return false - end - unless has_notes? - errors.add :post, "has no notes" - return false - end + transaction do + if id == other_post.id + errors.add :base, "Source and destination posts are the same" + return false + end + unless has_notes? + errors.add :post, "has no notes" + return false + end - notes.active.each do |note| - note.copy_to(other_post) - end + notes.active.each do |note| + note.copy_to(other_post) + end - dummy = Note.new - if notes.active.length == 1 - dummy.body = "Copied 1 note from post ##{id}." - else - dummy.body = "Copied #{notes.active.length} notes from post ##{id}." + dummy = Note.new + if notes.active.length == 1 + dummy.body = "Copied 1 note from post ##{id}." + else + dummy.body = "Copied #{notes.active.length} notes from post ##{id}." + end + dummy.is_active = false + dummy.post_id = other_post.id + dummy.x = dummy.y = dummy.width = dummy.height = 0 + dummy.save end - dummy.is_active = false - dummy.post_id = other_post.id - dummy.x = dummy.y = dummy.width = dummy.height = 0 - dummy.save end end From 8fd9d374ca7bfbda214454b424070f0057bbba04 Mon Sep 17 00:00:00 2001 From: evazion Date: Sat, 31 Mar 2018 12:58:56 -0500 Subject: [PATCH 2/2] Fix #3583: Copying notes should copy tags to destination. --- app/models/post.rb | 13 ++++++++++++- test/unit/post_test.rb | 26 ++++++++++++++++++++++++++ 2 files changed, 38 insertions(+), 1 deletion(-) diff --git a/app/models/post.rb b/app/models/post.rb index c0cd10fbf..f6ccd3a3f 100644 --- a/app/models/post.rb +++ b/app/models/post.rb @@ -8,6 +8,9 @@ class Post < ApplicationRecord class SearchError < Exception ; end class DeletionError < Exception ; end + # Tags to copy when copying notes. + NOTE_COPY_TAGS = %w[translated partially_translated check_translation translation_request reverse_translation] + before_validation :initialize_uploader, :on => :create before_validation :merge_old_changes before_validation :normalize_tags @@ -1414,7 +1417,7 @@ class Post < ApplicationRecord last_noted_at.present? end - def copy_notes_to(other_post) + def copy_notes_to(other_post, copy_tags: NOTE_COPY_TAGS) transaction do if id == other_post.id errors.add :base, "Source and destination posts are the same" @@ -1439,6 +1442,14 @@ class Post < ApplicationRecord dummy.post_id = other_post.id dummy.x = dummy.y = dummy.width = dummy.height = 0 dummy.save + + copy_tags.each do |tag| + other_post.remove_tag(tag) + other_post.add_tag(tag) if has_tag?(tag) + end + + other_post.has_embedded_notes = has_embedded_notes + other_post.save end end end diff --git a/test/unit/post_test.rb b/test/unit/post_test.rb index ddb133250..6e1e06e3d 100644 --- a/test/unit/post_test.rb +++ b/test/unit/post_test.rb @@ -2663,6 +2663,32 @@ class PostTest < ActiveSupport::TestCase end end + context "Notes:" do + context "#copy_notes_to" do + setup do + @src = FactoryGirl.create(:post, image_width: 100, image_height: 100, tag_string: "translated partially_translated", has_embedded_notes: true) + @dst = FactoryGirl.create(:post, image_width: 200, image_height: 200, tag_string: "translation_request") + + @src.notes.create(x: 10, y: 10, width: 10, height: 10, body: "test") + @src.notes.create(x: 10, y: 10, width: 10, height: 10, body: "deleted", is_active: false) + @src.reload + + @src.copy_notes_to(@dst) + end + + should "copy notes and tags" do + assert_equal(1, @dst.notes.active.length) + assert_equal(true, @dst.has_embedded_notes) + assert_equal("lowres partially_translated translated", @dst.tag_string) + end + + should "rescale notes" do + note = @dst.notes.active.first + assert_equal([20, 20, 20, 20], [note.x, note.y, note.width, note.height]) + end + end + end + context "Mass assignment: " do should_not allow_mass_assignment_of(:last_noted_at).as(:member) end