From ef3188a7fe33b300f322de91e635990d04b1ba7b Mon Sep 17 00:00:00 2001 From: evazion Date: Sun, 16 Feb 2020 18:27:57 -0600 Subject: [PATCH] artists/edit: refactor editing nested wiki pages. Refactor to use accepts_nested_attributes_for instead of the notes attribute to facilitate editing wikis on the artist edit page. This fixes the notes attribute unintentionally showing up in the API. This also changes it so that renaming an artist entry doesn't automatically rename the corresponding wiki page. This had bad behavior when there was a conflict between wiki pages (the wikis would be silently merged, which usually isn't what you want). It also didn't warn about wiki links being broken by renames. --- app/controllers/artists_controller.rb | 3 + app/models/artist.rb | 65 +--------------------- app/views/artists/_form.html.erb | 14 ++--- app/views/artists/_show.html.erb | 4 +- test/functional/artists_controller_test.rb | 37 +++--------- test/unit/artist_test.rb | 18 ------ 6 files changed, 22 insertions(+), 119 deletions(-) diff --git a/app/controllers/artists_controller.rb b/app/controllers/artists_controller.rb index 40774455c..e0790d171 100644 --- a/app/controllers/artists_controller.rb +++ b/app/controllers/artists_controller.rb @@ -6,10 +6,12 @@ class ArtistsController < ApplicationController def new @artist = Artist.new_with_defaults(artist_params(:new)) + @artist.build_wiki_page if @artist.wiki_page.nil? respond_with(@artist) end def edit + @artist.build_wiki_page if @artist.wiki_page.nil? respond_with(@artist) end @@ -93,6 +95,7 @@ class ArtistsController < ApplicationController def artist_params(context = nil) permitted_params = %i[name other_names other_names_string group_name url_string notes is_active] + permitted_params << { wiki_page_attributes: %i[id body] } permitted_params << :source if context == :new params.fetch(:artist, {}).permit(permitted_params) diff --git a/app/models/artist.rb b/app/models/artist.rb index 3b924d46f..650eb0637 100644 --- a/app/models/artist.rb +++ b/app/models/artist.rb @@ -8,7 +8,6 @@ class Artist < ApplicationRecord before_validation :normalize_name before_validation :normalize_other_names after_save :create_version - after_save :update_wiki after_save :clear_url_string_changed validate :validate_tag_category validates :name, tag_name: true, uniqueness: true @@ -19,7 +18,8 @@ class Artist < ApplicationRecord has_one :wiki_page, :foreign_key => "title", :primary_key => "name" has_one :tag_alias, :foreign_key => "antecedent_name", :primary_key => "name" belongs_to :tag, foreign_key: "name", primary_key: "name", default: -> { Tag.new(name: name, category: Tag.categories.artist) } - attribute :notes, :string + + accepts_nested_attributes_for :wiki_page, update_only: true, reject_if: :all_blank scope :active, -> { where(is_active: true) } scope :deleted, -> { where(is_active: false) } @@ -222,7 +222,7 @@ class Artist < ApplicationRecord module VersionMethods def create_version(force = false) - if saved_change_to_name? || url_string_changed || saved_change_to_is_active? || saved_change_to_is_banned? || saved_change_to_other_names? || saved_change_to_group_name? || saved_change_to_notes? || force + if saved_change_to_name? || url_string_changed || saved_change_to_is_active? || saved_change_to_is_banned? || saved_change_to_other_names? || saved_change_to_group_name? || force if merge_version? merge_version else @@ -295,64 +295,6 @@ class Artist < ApplicationRecord end end - module NoteMethods - extend ActiveSupport::Concern - - def notes - @notes || wiki_page.try(:body) - end - - def notes=(text) - if notes != text - notes_will_change! - @notes = text - end - end - - def reload(options = nil) - flush_cache - - if instance_variable_defined?(:@notes) - remove_instance_variable(:@notes) - end - - super - end - - def notes_changed? - attribute_changed?("notes") - end - - def notes_will_change! - attribute_will_change!("notes") - end - - def update_wiki - if persisted? && saved_change_to_name? && attribute_before_last_save("name").present? && WikiPage.titled(attribute_before_last_save("name")).exists? - # we're renaming the artist, so rename the corresponding wiki page - old_page = WikiPage.titled(name_before_last_save).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 (!@notes.nil? && (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 - end - module TagMethods def validate_tag_category return unless is_active? && name_changed? @@ -487,7 +429,6 @@ class Artist < ApplicationRecord include NameMethods include VersionMethods extend FactoryMethods - include NoteMethods include TagMethods include BanMethods extend SearchMethods diff --git a/app/views/artists/_form.html.erb b/app/views/artists/_form.html.erb index 2c85d743e..cb4990b63 100644 --- a/app/views/artists/_form.html.erb +++ b/app/views/artists/_form.html.erb @@ -1,15 +1,13 @@ <%= edit_form_for(@artist) do |f| %> - <% if @artist.new_record? %> - <%= f.input :name, as: :string, input_html: { data: { autocomplete: "tag" } } %> - <% else %> - <%= f.input :name, as: :string, input_html: { data: { autocomplete: "tag" } }, hint: "Change to rename this artist entry and its wiki page" %> - <% end %> + <%= f.input :name, as: :string, input_html: { "data-autocomplete": "tag" } %> <%= f.input :other_names_string, label: "Other names", as: :text, input_html: { size: "50x1" }, hint: 'NEW Separate names with spaces, not commas. Use underscores for spaces inside names.'.html_safe %> <%= f.input :group_name %> - <%= f.input :url_string, :label => "URLs", :as => :text, :input_html => {:size => "50x5", :value => params.dig(:artist, :url_string) || @artist.urls.join("\n")}, :hint => "You can prefix a URL with - to mark it as dead." %> + <%= f.input :url_string, :label => "URLs", :as => :text, :input_html => {:size => "50x15", :value => params.dig(:artist, :url_string) || @artist.urls.join("\n")}, :hint => "You can prefix a URL with - to mark it as dead." %> + + <%= f.simple_fields_for :wiki_page do |fw| %> + <%= fw.input :body, label: "Wiki", input_html: { size: "50x15" } %> + <% end %> - <%= dtext_field "artist", "notes" %> <%= f.button :submit, "Submit" %> - <%= dtext_preview_button "artist", "notes" %> <% end %> diff --git a/app/views/artists/_show.html.erb b/app/views/artists/_show.html.erb index 1a9cc9590..46ba09d94 100644 --- a/app/views/artists/_show.html.erb +++ b/app/views/artists/_show.html.erb @@ -5,9 +5,9 @@

Artist: <%= link_to @artist.pretty_name, posts_path(tags: @artist.name), class: tag_class(@artist.tag) %>

- <% if @artist.notes.present? %> + <% if @artist.wiki_page.present? %>
- <%= format_text(@artist.notes, :disable_mentions => true) %> + <%= format_text(@artist.wiki_page.body, :disable_mentions => true) %>

<%= link_to "View wiki page", @artist.wiki_page %>

diff --git a/test/functional/artists_controller_test.rb b/test/functional/artists_controller_test.rb index 65d1184f5..39b3841b3 100644 --- a/test/functional/artists_controller_test.rb +++ b/test/functional/artists_controller_test.rb @@ -32,7 +32,7 @@ class ArtistsControllerTest < ActionDispatch::IntegrationTest @admin = create(:admin_user) @user = create(:user) as_user do - @artist = create(:artist, notes: "message") + @artist = create(:artist) @masao = create(:artist, name: "masao", url_string: "http://www.pixiv.net/member.php?id=32777") @artgerm = create(:artist, name: "artgerm", url_string: "http://artgerm.deviantart.com/") end @@ -135,23 +135,23 @@ class ArtistsControllerTest < ActionDispatch::IntegrationTest assert_redirected_to(artist_path(artist.id)) end - context "with an artist that has notes" do + context "with an artist that has a wiki page" do setup do as(@admin) do - @artist = create(:artist, name: "aaa", notes: "testing", url_string: "http://example.com") + @artist = create(:artist, name: "aaa", url_string: "http://example.com") + @wiki_page = create(:wiki_page, title: "aaa", body: "testing") end - @wiki_page = @artist.wiki_page @another_user = create(:user) end - should "update an artist" do + should "update the wiki with the artist" do old_timestamp = @wiki_page.updated_at travel(1.minute) do - put_auth artist_path(@artist.id), @user, params: {artist: {notes: "rex", url_string: "http://example.com\nhttp://monet.com"}} + put_auth artist_path(@artist.id), @user, params: {artist: { wiki_page_attributes: { body: "rex" }, url_string: "http://example.com\nhttp://monet.com"}} end @artist.reload @wiki_page = @artist.wiki_page - assert_equal("rex", @artist.notes) + assert_equal("rex", @artist.wiki_page.body) assert_not_equal(old_timestamp, @wiki_page.updated_at) assert_redirected_to(artist_path(@artist.id)) end @@ -160,31 +160,10 @@ class ArtistsControllerTest < ActionDispatch::IntegrationTest old_timestamp = @wiki_page.updated_at travel(1.minute) - as(@another_user) { @artist.update(notes: "testing") } + as(@another_user) { @artist.update(wiki_page_attributes: { body: "testing" }) } assert_equal(old_timestamp.to_i, @artist.reload.wiki_page.updated_at.to_i) end - - context "when renaming an artist" do - should "automatically rename the artist's wiki page" do - assert_difference("WikiPage.count", 0) do - put_auth artist_path(@artist.id), @user, params: {artist: {name: "bbb", notes: "more testing"}} - 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 - as_user do - @existing_wiki_page = create(:wiki_page, title: "bbb", body: "xxx") - end - put_auth artist_path(@artist.id), @user, params: {artist: {name: "bbb", notes: "yyy"}} - @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 diff --git a/test/unit/artist_test.rb b/test/unit/artist_test.rb index d83068088..e03097afe 100644 --- a/test/unit/artist_test.rb +++ b/test/unit/artist_test.rb @@ -104,24 +104,6 @@ class ArtistTest < ActiveSupport::TestCase end end - should "create a new wiki page to store any note information" do - artist = nil - assert_difference("WikiPage.count") do - artist = FactoryBot.create(:artist, :name => "aaa", :notes => "testing") - end - assert_equal("testing", artist.notes) - assert_equal("testing", artist.wiki_page.body) - assert_equal(artist.name, artist.wiki_page.title) - end - - should "update the wiki page when notes are assigned" do - artist = FactoryBot.create(:artist, :name => "aaa", :notes => "testing") - artist.update_attribute(:notes, "kokoko") - artist.reload - assert_equal("kokoko", artist.notes) - assert_equal("kokoko", artist.wiki_page.body) - end - should "normalize its name" do artist = FactoryBot.create(:artist, :name => " AAA BBB ") assert_equal("aaa_bbb", artist.name)