From 68f057e7ba0fae090c14afe8169808cf4f34803e Mon Sep 17 00:00:00 2001 From: r888888888 Date: Tue, 18 Apr 2017 15:57:20 -0700 Subject: [PATCH] refactor how artist notes are updated --- app/controllers/artists_controller.rb | 7 +- app/models/artist.rb | 86 +++++++++++++--------- test/functional/artists_controller_test.rb | 81 +++++++++++++------- test/unit/artist_test.rb | 12 --- 4 files changed, 104 insertions(+), 82 deletions(-) diff --git a/app/controllers/artists_controller.rb b/app/controllers/artists_controller.rb index d56df0b49..8047370b0 100644 --- 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 diff --git a/app/models/artist.rb b/app/models/artist.rb index d68e13ee5..b92defa29 100644 --- 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 diff --git a/test/functional/artists_controller_test.rb b/test/functional/artists_controller_test.rb index 0ce937bee..7a9f0dc0c 100644 --- 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") diff --git a/test/unit/artist_test.rb b/test/unit/artist_test.rb index 9a58efce7..799735c02 100644 --- 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)