refactor how artist notes are updated

2017-04-18 15:57:20 -07:00
parent e639be60e5
commit 68f057e7ba
4 changed files with 104 additions and 82 deletions
--- a/app/controllers/artists_controller.rb
+++ b/app/controllers/artists_controller.rb
@@ -71,12 +71,7 @@ class ArtistsController < ApplicationController
  end

  def update
-    body = params[:artist].delete("notes")
-    @artist.assign_attributes(params[:artist], :as => CurrentUser.role)
-    if body
-      @artist.notes = body
-    end
-    @artist.save
+    @artist.update(params[:artist], :as => CurrentUser.role)
    respond_with(@artist)
  end

--- a/app/models/artist.rb
+++ b/app/models/artist.rb
@@ -6,9 +6,10 @@ class Artist < ActiveRecord::Base
  after_save :create_version
  after_save :save_url_string
  after_save :categorize_tag
+  after_save :update_wiki
  validates_uniqueness_of :name
-  validate :name_is_valid
-  validate :wiki_is_empty, :on => :create
+  validate :validate_name
+  validate :validate_wiki, :on => :create
  belongs_to :creator, :class_name => "User"
  has_many :members, :class_name => "Artist", :foreign_key => "group_name", :primary_key => "name"
  has_many :urls, :dependent => :destroy, :class_name => "ArtistUrl"
@@ -16,8 +17,7 @@ class Artist < ActiveRecord::Base
  has_one :wiki_page, :foreign_key => "title", :primary_key => "name"
  has_one :tag_alias, :foreign_key => "antecedent_name", :primary_key => "name"
  has_one :tag, :foreign_key => "name", :primary_key => "name"
-  accepts_nested_attributes_for :wiki_page
-  attr_accessible :body, :name, :url_string, :other_names, :other_names_comma, :group_name, :wiki_page_attributes, :notes, :as => [:member, :gold, :builder, :platinum, :janitor, :moderator, :default, :admin]
+  attr_accessible :body, :notes, :name, :url_string, :other_names, :other_names_comma, :group_name, :notes, :as => [:member, :gold, :builder, :platinum, :janitor, :moderator, :default, :admin]
  attr_accessible :is_active, :as => [:builder, :janitor, :moderator, :default, :admin]
  attr_accessible :is_banned, :as => :admin

@@ -99,7 +99,7 @@ class Artist < ActiveRecord::Base
      end
    end

-    def name_is_valid
+    def validate_name
      if name =~ /^[-~]/
        errors[:name] << "cannot begin with - or ~"
        false
@@ -219,47 +219,61 @@ class Artist < ActiveRecord::Base
  end

  module NoteMethods
+    extend ActiveSupport::Concern
+
    def notes
-      if wiki_page
-        wiki_page.body
-      else
-        nil
+      @notes || wiki_page.try(:body)
+    end
+
+    def notes=(text)
+      if notes != text
+        notes_will_change!
+        @notes = text
      end
    end

-    def notes=(msg)
-      if name_changed? && name_was.present?
-        old_wiki_page = WikiPage.titled(name_was).first
-      end
-    
-      if wiki_page
-        if name_changed? && name_was.present?
-          wiki_page.body = wiki_page.body + "\n\n" + msg
-        else
-          wiki_page.body = msg
-        end
-        if wiki_page.body_changed?
-          wiki_page.save
-          @notes_changed = true
-        end
-      elsif old_wiki_page
-        old_wiki_page.title = name
-        old_wiki_page.body = msg
-        if old_wiki_page.body_changed? || old_wiki_page.title_changed?
-          old_wiki_page.save
-          @notes_changed = true
-        end
-      elsif msg.present?
-        self.wiki_page = WikiPage.new(:title => name, :body => msg)
-        @notes_changed = true
+    def reload(options = nil)
+      if instance_variable_defined?(:@notes)
+        remove_instance_variable(:@notes)
      end
+
+      super
    end

    def notes_changed?
-      !!@notes_changed
+      attribute_changed?("notes")
    end

-    def wiki_is_empty
+    def notes_will_change!
+      attribute_will_change!("notes")
+    end
+
+    def update_wiki
+      if persisted? && name_changed? && name_was.present? && WikiPage.titled(name_was).exists?
+        # we're renaming the artist, so rename the corresponding wiki page
+        old_page = WikiPage.titled(name_was).first
+
+        if wiki_page.present?
+          # a wiki page with the new name already exists, so update the content
+          wiki_page.update(body: "#{wiki_page.body}\n\n#{@notes}")
+        else
+          # a wiki page doesn't already exist for the new name, so rename the old one
+          old_page.update(title: name, body: @notes)
+        end
+      elsif wiki_page.nil?
+        # if there are any notes, we need to create a new wiki page
+        if @notes.present?
+          create_wiki_page(body: @notes, title: name)
+        end
+      elsif wiki_page.body != @notes || wiki_page.title != name
+        # if anything changed, we need to update the wiki page
+        wiki_page.body = @notes unless @notes.nil?
+        wiki_page.title = name
+        wiki_page.save
+      end
+    end
+
+    def validate_wiki
      if WikiPage.titled(name).exists?
        errors.add(:name, "conflicts with a wiki page")
        return false
--- a/test/functional/artists_controller_test.rb
+++ b/test/functional/artists_controller_test.rb
@@ -21,7 +21,7 @@ class ArtistsControllerTest < ActionController::TestCase
      @user = FactoryGirl.create(:user)
      CurrentUser.user = @user
      CurrentUser.ip_addr = "127.0.0.1"
-      @artist = FactoryGirl.create(:artist)
+      @artist = FactoryGirl.create(:artist, :notes => "message")

      @masao = FactoryGirl.create(:artist, :name => "masao",   :url_string => "http://i2.pixiv.net/img04/img/syounen_no_uta/")
      @artgerm = FactoryGirl.create(:artist, :name => "artgerm", :url_string => "http://artgerm.deviantart.com/")
@@ -129,11 +129,58 @@ class ArtistsControllerTest < ActionController::TestCase
      assert_redirected_to(artist_path(artist))
    end

-    should "update an artist" do
-      post :update, {:id => @artist.id, :artist => {:name => "xxx"}}, {:user_id => @user.id}
-      @artist.reload
-      assert_equal("xxx", @artist.name)
-      assert_redirected_to(artist_path(@artist))
+    context "with an artist that has notes" do
+      setup do
+        @artist = FactoryGirl.create(:artist, :name => "aaa", :notes => "testing")
+        @wiki_page = @artist.wiki_page
+        @another_user = FactoryGirl.create(:user)
+      end
+
+      should "update an artist" do
+        old_timestamp = @wiki_page.updated_at
+        Timecop.travel(1.minute.from_now) do
+          post :update, {:id => @artist.id, :artist => {:notes => "rex"}}, {:user_id => @user.id}
+        end
+        @artist.reload
+        @wiki_page.reload
+        assert_equal("rex", @artist.notes)
+        assert_not_equal(old_timestamp, @wiki_page.updated_at)
+        assert_redirected_to(artist_path(@artist))
+      end
+
+      should "not touch the updater_id and updated_at fields when nothing is changed" do
+        old_timestamp = @wiki_page.updated_at
+        old_updater_id = @wiki_page.updater_id
+
+        Timecop.travel(1.minutes.from_now) do
+          CurrentUser.scoped(@another_user) do
+            @artist.update_attributes(notes: "testing")
+          end
+        end
+
+        @wiki_page.reload
+        assert_equal(old_timestamp, @wiki_page.updated_at)
+        assert_equal(old_updater_id, @wiki_page.updater_id)
+      end
+
+      context "when renaming an artist" do
+        should "automatically rename the artist's wiki page" do
+          assert_difference("WikiPage.count", 0) do
+            post :update, {:id => @artist.id, :artist => {:name => "bbb", :notes => "more testing"}}, {:user_id => @user.id}
+          end
+          @wiki_page.reload
+          assert_equal("bbb", @wiki_page.title)
+          assert_equal("more testing", @wiki_page.body)
+        end
+
+        should "merge the new notes with the existing wiki page's contents if a wiki page for the new name already exists" do
+          existing_wiki_page = FactoryGirl.create(:wiki_page, :title => "bbb", :body => "xxx")
+          post :update, {:id => @artist.id, :artist => {:name => "bbb", :notes => "yyy"}}, {:user_id => @user.id}
+          existing_wiki_page.reload
+          assert_equal("bbb", existing_wiki_page.title)
+          assert_equal("xxx\n\nyyy", existing_wiki_page.body)
+        end
+      end
    end

    should "delete an artist" do
@@ -154,28 +201,6 @@ class ArtistsControllerTest < ActionController::TestCase
      assert_equal(true, @artist.reload.is_active)
    end

-    context "when renaming an artist" do
-      should "automatically rename the artist's wiki page" do
-        artist = FactoryGirl.create(:artist, :name => "aaa", :notes => "testing")
-        wiki_page = artist.wiki_page
-        assert_difference("WikiPage.count", 0) do
-          post :update, {:id => artist.id, :artist => {:name => "bbb", :notes => "more testing"}}, {:user_id => @user.id}
-        end
-        wiki_page.reload
-        assert_equal("bbb", wiki_page.title)
-        assert_equal("more testing", wiki_page.body)
-      end
-
-      should "merge the new notes with the existing wiki page's contents if a wiki page for the new name already exists" do
-        artist = FactoryGirl.create(:artist, :name => "aaa")
-        existing_wiki_page = FactoryGirl.create(:wiki_page, :title => "bbb", :body => "xxx")
-        post :update, {:id => artist.id, :artist => {:name => "bbb", :notes => "yyy"}}, {:user_id => @user.id}
-        existing_wiki_page.reload
-        assert_equal("bbb", existing_wiki_page.title)
-        assert_equal("xxx\n\nyyy", existing_wiki_page.body)
-      end
-    end
-
    context "reverting an artist" do
      should "work" do
        @artist.update_attributes(:name => "xyz")
--- a/test/unit/artist_test.rb
+++ b/test/unit/artist_test.rb
@@ -310,18 +310,6 @@ class ArtistTest < ActiveSupport::TestCase
      assert_not_nil(Artist.search(:name => "group:cat_or_fish").first)
    end

-    should "have an associated wiki" do
-      user = FactoryGirl.create(:user)
-      CurrentUser.user = user
-      artist = FactoryGirl.create(:artist, :name => "max", :wiki_page_attributes => {:title => "xxx", :body => "this is max"})
-      assert_not_nil(artist.wiki_page)
-      assert_equal("this is max", artist.wiki_page.body)
-
-      artist.update_attributes({:wiki_page_attributes => {:id => artist.wiki_page.id, :body => "this is hoge mark ii"}})
-      assert_equal("this is hoge mark ii", artist.wiki_page(true).body)
-      CurrentUser.user = nil
-    end
-
    should "revert to prior versions" do
      user = FactoryGirl.create(:user)
      reverter = FactoryGirl.create(:user)