Skip to content

Commit

Permalink
artists/edit: refactor editing nested wiki pages.
Browse files Browse the repository at this point in the history 
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.
  • Loading branch information
@evazion
evazion committed Feb 17, 2020
1 parent 6f3d0b5 commit ef3188a
Show file tree
Hide file tree
Showing 6 changed files with 22 additions and 119 deletions.
3 changes: 3 additions & 0 deletions app/controllers/artists_controller.rb
View file
Expand Up @@ -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

Expand Down Expand Up @@ -93,6 +95,7 @@ def load_artist

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)
Expand Down
65 changes: 3 additions & 62 deletions app/models/artist.rb
View file
Expand Up @@ -8,7 +8,6 @@ class RevertError < StandardError; end
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
Expand All @@ -19,7 +18,8 @@ class RevertError < StandardError; end
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) }
Expand Down Expand Up @@ -222,7 +222,7 @@ def normalize_other_names

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
Expand Down Expand Up @@ -295,64 +295,6 @@ def new_with_defaults(params)
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?
Expand Down Expand Up @@ -487,7 +429,6 @@ def search(params)
include NameMethods
include VersionMethods
extend FactoryMethods
include NoteMethods
include TagMethods
include BanMethods
extend SearchMethods
Expand Down
14 changes: 6 additions & 8 deletions app/views/artists/_form.html.erb
View file
@@ -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: '<b style="color: red;">NEW</b> 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 %>
4 changes: 2 additions & 2 deletions app/views/artists/_show.html.erb
View file
Expand Up @@ -5,9 +5,9 @@
<div id="a-show">
<h1>Artist: <%= link_to @artist.pretty_name, posts_path(tags: @artist.name), class: tag_class(@artist.tag) %></h1>

<% if @artist.notes.present? %>
<% if @artist.wiki_page.present? %>
<div class="prose">
<%= format_text(@artist.notes, :disable_mentions => true) %>
<%= format_text(@artist.wiki_page.body, :disable_mentions => true) %>
</div>

<p><%= link_to "View wiki page", @artist.wiki_page %></p>
Expand Down
37 changes: 8 additions & 29 deletions test/functional/artists_controller_test.rb
View file
Expand Up @@ -32,7 +32,7 @@ def assert_artist_not_found(source_url)
@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
Expand Down Expand Up @@ -135,23 +135,23 @@ def assert_artist_not_found(source_url)
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
Expand All @@ -160,31 +160,10 @@ def assert_artist_not_found(source_url)
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
Expand Down
18 changes: 0 additions & 18 deletions test/unit/artist_test.rb
View file
Expand Up @@ -104,24 +104,6 @@ def assert_artist_not_found(source_url)
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)
Expand Down

0 comments on commit ef3188a

Please sign in to comment.