Merge pull request #3150 from evazion/fix-artcomm-whitespace

Fix #2174: Trim whitespace from artist commentary

app/models/artist_commentary.rb

@@ -4,6 +4,7 @@ class ArtistCommentary < ActiveRecord::Base
   attr_accessor :remove_commentary_tag, :remove_commentary_request_tag, :remove_commentary_check_tag
   attr_accessor :add_commentary_tag, :add_commentary_request_tag, :add_commentary_check_tag
   attr_accessible :post_id, :original_description, :original_title, :translated_description, :translated_title, :remove_commentary_tag, :remove_commentary_request_tag, :add_commentary_tag, :add_commentary_request_tag, :add_commentary_check_tag, :remove_commentary_check_tag
+  before_validation :trim_whitespace
   validates_uniqueness_of :post_id
   belongs_to :post
   has_many :versions, lambda {order("artist_commentary_versions.id ASC")}, :class_name => "ArtistCommentaryVersion", :dependent => :destroy, :foreign_key => :post_id, :primary_key => :post_id
@@ -34,15 +35,15 @@ class ArtistCommentary < ActiveRecord::Base
       end
 
       if params[:original_present] == "yes"
-        q = q.where("(original_title is not null and original_title != '') or (original_description is not null and original_description != '')")
+        q = q.where("(original_title != '') or (original_description != '')")
       elsif params[:original_present] == "no"
-        q = q.where("(original_title is null or original_title = '') and (original_description is null or original_description = '')")
+        q = q.where("(original_title = '') and (original_description = '')")
       end
 
       if params[:translated_present] == "yes"
-        q = q.where("(translated_title is not null and translated_title != '') or (translated_description is not null and translated_description != '')")
+        q = q.where("(translated_title != '') or (translated_description != '')")
       elsif params[:translated_present] == "no"
-        q = q.where("(translated_title is null or translated_title = '') and (translated_description is null or translated_description = '')")
+        q = q.where("(translated_title = '') and (translated_description = '')")
       end
 
       if params[:post_tags_match].present?
@@ -55,6 +56,13 @@ class ArtistCommentary < ActiveRecord::Base
 
   extend SearchMethods
 
+  def trim_whitespace
+    self.original_title = original_title.gsub(/\A[[:space:]]+|[[:space:]]+\z/, "")
+    self.translated_title = translated_title.gsub(/\A[[:space:]]+|[[:space:]]+\z/, "")
+    self.original_description = original_description.gsub(/\A[[:space:]]+|[[:space:]]+\z/, "")
+    self.translated_description = translated_description.gsub(/\A[[:space:]]+|[[:space:]]+\z/, "")
+  end
+
   def original_present?
     original_title.present? || original_description.present?
   end

db/migrate/20170613200356_change_fields_to_non_null_on_artist_commentaries.rb

@@ -0,0 +1,29 @@
+class ChangeFieldsToNonNullOnArtistCommentaries < ActiveRecord::Migration
+  def up
+    ArtistCommentary.without_timeout do
+      change_column_null(:artist_commentaries, :original_title, false, "")
+      change_column_null(:artist_commentaries, :translated_title, false, "")
+      change_column_null(:artist_commentaries, :original_description, false, "")
+      change_column_null(:artist_commentaries, :translated_description, false, "")
+
+      change_column_default(:artist_commentaries, :original_title, "")
+      change_column_default(:artist_commentaries, :translated_title, "")
+      change_column_default(:artist_commentaries, :original_description, "")
+      change_column_default(:artist_commentaries, :translated_description, "")
+    end
+  end
+
+  def down
+    ArtistCommentary.without_timeout do
+      change_column_null(:artist_commentaries, :original_title, true)
+      change_column_null(:artist_commentaries, :translated_title, true)
+      change_column_null(:artist_commentaries, :original_description, true)
+      change_column_null(:artist_commentaries, :translated_description, true)
+
+      change_column_default(:artist_commentaries, :original_title, nil)
+      change_column_default(:artist_commentaries, :translated_title, nil)
+      change_column_default(:artist_commentaries, :original_description, nil)
+      change_column_default(:artist_commentaries, :translated_description, nil)
+    end
+  end
+end

db/structure.sql

@@ -603,10 +603,10 @@ ALTER SEQUENCE api_keys_id_seq OWNED BY api_keys.id;
 CREATE TABLE artist_commentaries (
     id integer NOT NULL,
     post_id integer NOT NULL,
-    original_title text,
-    original_description text,
-    translated_title text,
-    translated_description text,
+    original_title text DEFAULT ''::text NOT NULL,
+    original_description text DEFAULT ''::text NOT NULL,
+    translated_title text DEFAULT ''::text NOT NULL,
+    translated_description text DEFAULT ''::text NOT NULL,
     created_at timestamp without time zone,
     updated_at timestamp without time zone
 );
@@ -7563,3 +7563,5 @@ INSERT INTO schema_migrations (version) VALUES ('20170526183928');
 
 INSERT INTO schema_migrations (version) VALUES ('20170608043651');
 
+INSERT INTO schema_migrations (version) VALUES ('20170613200356');
+

script/fixes/047_trim_artcomm_whitespace.rb

@@ -0,0 +1,27 @@
+#!/usr/bin/env ruby
+
+require File.expand_path(File.join(File.dirname(__FILE__), '..', '..', 'config', 'environment'))
+
+CurrentUser.user = User.system
+CurrentUser.ip_addr = "127.0.0.1"
+
+ArtistCommentary.without_timeout do
+  ArtistCommentary.transaction do
+    # binding.pry
+
+    artcomms = ArtistCommentary.where(%(
+         original_title         ~ '^[[:space:]]|[[:space:]]$'
+      OR translated_title       ~ '^[[:space:]]|[[:space:]]$'
+      OR original_description   ~ '^[[:space:]]|[[:space:]]$'
+      OR translated_description ~ '^[[:space:]]|[[:space:]]$'
+    ))
+    size = artcomms.size
+
+    artcomms.find_each.with_index do |artcomm, i|
+      artcomm.save
+      puts "#{i}/#{size}" if i % 100 == 0
+    end
+
+    # raise ActiveRecord::Rollback
+  end
+end

test/unit/artist_commentary_test.rb

@@ -0,0 +1,79 @@
+require 'test_helper'
+
+class ArtistCommentaryTest < ActiveSupport::TestCase
+  setup do
+    user = FactoryGirl.create(:user)
+    CurrentUser.user = user
+    CurrentUser.ip_addr = "127.0.0.1"
+  end
+
+  teardown do
+    CurrentUser.user = nil
+    CurrentUser.ip_addr = nil
+  end
+
+  should "A post should not have more than one commentary" do
+    post = FactoryGirl.create(:post)
+
+    assert_raise(ActiveRecord::RecordInvalid) do
+      FactoryGirl.create(:artist_commentary, post_id: post.id)
+      FactoryGirl.create(:artist_commentary, post_id: post.id)
+    end
+  end
+
+  context "An artist commentary" do
+    context "when searched" do
+      setup do
+        @post1 = FactoryGirl.create(:post, tag_string: "artcomm1")
+        @post2 = FactoryGirl.create(:post, tag_string: "artcomm2")
+        @artcomm1 = FactoryGirl.create(:artist_commentary, post_id: @post1.id, original_title: "foo", translated_title: "bar")
+        @artcomm2 = FactoryGirl.create(:artist_commentary, post_id: @post2.id, original_title: "", original_description: "", translated_title: "", translated_description: "")
+      end
+
+      should "find the correct match" do
+        assert_equal([@artcomm1.id], ArtistCommentary.search(post_id: @post1.id.to_s).map(&:id))
+        assert_equal([@artcomm1.id], ArtistCommentary.search(text_matches: "foo").map(&:id))
+        assert_equal([@artcomm1.id], ArtistCommentary.search(text_matches: "f*").map(&:id))
+        assert_equal([@artcomm1.id], ArtistCommentary.search(post_tags_match: "artcomm1").map(&:id))
+
+        assert_equal([@artcomm1.id], ArtistCommentary.search(original_present: "yes").map(&:id))
+        assert_equal([@artcomm2.id], ArtistCommentary.search(original_present: "no").map(&:id))
+
+        assert_equal([@artcomm1.id], ArtistCommentary.search(translated_present: "yes").map(&:id))
+        assert_equal([@artcomm2.id], ArtistCommentary.search(translated_present: "no").map(&:id))
+      end
+    end
+
+    context "when created" do
+      should "create a new version" do
+        @artcomm = FactoryGirl.create(:artist_commentary, original_title: "foo")
+
+        assert_equal(1, @artcomm.versions.size)
+        assert_equal("foo", @artcomm.versions.last.original_title)
+      end
+    end
+
+    context "when updated" do
+      setup do
+        @post = FactoryGirl.create(:post)
+        @artcomm = FactoryGirl.create(:artist_commentary, post_id: @post.id)
+        @artcomm.reload
+      end
+
+      should "trim whitespace from all fields" do
+        # \u00A0 - nonbreaking space.
+        @artcomm.update(
+          original_title: "  foo\u00A0\t\n",
+          original_description: " foo\u00A0\t\n",
+          translated_title: "  foo\u00A0\t\n",
+          translated_description: "  foo\u00A0\n",
+        )
+
+        assert_equal("foo", @artcomm.original_title)
+        assert_equal("foo", @artcomm.original_description)
+        assert_equal("foo", @artcomm.translated_title)
+        assert_equal("foo", @artcomm.translated_description)
+      end
+    end
+  end
+end