diff --git a/app/controllers/artists_controller.rb b/app/controllers/artists_controller.rb index 5b60fc6e8..6a38d47bb 100644 --- a/app/controllers/artists_controller.rb +++ b/app/controllers/artists_controller.rb @@ -2,9 +2,9 @@ class ArtistsController < ApplicationController respond_to :html, :json - before_action :member_only, :except => [:index, :show, :show_or_new] - before_action :janitor_only, :only => [:destroy] - before_action :load_artist, :only => [:edit, :update, :destroy] + before_action :member_only, except: %i[index show show_or_new] + before_action :admin_only, only: %i[destroy] + before_action :load_artist, only: %i[edit update destroy] def new @artist = Artist.new(artist_params(:new)) @@ -12,6 +12,7 @@ class ArtistsController < ApplicationController end def edit + ensure_can_edit(CurrentUser.user) respond_with(@artist) end @@ -59,13 +60,11 @@ class ArtistsController < ApplicationController end def destroy - unless @artist.deletable_by?(CurrentUser.user) - raise User::PrivilegeError - end - @artist.update_attribute(:is_active, false) + raise User::PrivilegeError unless @artist.deletable_by?(CurrentUser.user) + @artist.destroy respond_with(@artist) do |format| format.html do - redirect_to(artist_path(@artist), notice: "Artist deleted") + redirect_to(artists_path, notice: "Artist deleted") end end end @@ -89,7 +88,7 @@ class ArtistsController < ApplicationController end end -private + private def load_artist @artist = Artist.find(params[:id]) @@ -103,13 +102,12 @@ private def ensure_can_edit(user) return if user.is_janitor? - raise User::PrivilegeError if @artist.is_locked? - raise User::PrivilegeError if !@artist.is_active? + raise(User::PrivilegeError, "Artist is locked.") if @artist.is_locked? end def artist_params(context = nil) permitted_params = %i[name other_names other_names_string group_name url_string notes] - permitted_params += [:is_active, :linked_user_id, :is_locked] if CurrentUser.is_janitor? + permitted_params += %i[linked_user_id is_locked] if CurrentUser.is_janitor? permitted_params << :source if context == :new params.fetch(:artist, {}).permit(permitted_params) diff --git a/app/decorators/mod_action_decorator.rb b/app/decorators/mod_action_decorator.rb index f0e6d5e7d..43388d79a 100644 --- a/app/decorators/mod_action_decorator.rb +++ b/app/decorators/mod_action_decorator.rb @@ -52,6 +52,8 @@ class ModActionDecorator < ApplicationDecorator "Unclaimed ticket ##{vals['ticket_id']}" ### Artist ### + when "artist_delete" + "Deleted artist ##{vals['artist_id']} (#{vals['artist_name']})" when "artist_page_rename" "Renamed artist page (\"#{vals['old_name']}\":/artists/show_or_new?name=#{vals['old_name']} -> \"#{vals['new_name']}\":/artists/show_or_new?name=#{vals['new_name']})" when "artist_page_lock" diff --git a/app/javascript/packs/application.js b/app/javascript/packs/application.js index c3c2f6d58..8afdf828c 100644 --- a/app/javascript/packs/application.js +++ b/app/javascript/packs/application.js @@ -34,7 +34,6 @@ importAll(require.context('../src/javascripts', true, /\.js(\.erb)?$/)); require.context("../../../public/images", true) -export { default as Artist } from '../src/javascripts/artist.js'; export { default as Autocomplete } from '../src/javascripts/autocomplete.js.erb'; export { default as Blacklist } from '../src/javascripts/blacklists.js'; export { default as Blip } from '../src/javascripts/blips.js'; diff --git a/app/javascript/src/javascripts/artist.js b/app/javascript/src/javascripts/artist.js deleted file mode 100644 index b5e2bca4d..000000000 --- a/app/javascript/src/javascripts/artist.js +++ /dev/null @@ -1,34 +0,0 @@ -import Utility from "./utility"; -import { SendQueue } from "./send_queue"; - -const Artist = {}; - -Artist.update = function (id, params) { - SendQueue.add(() => { - $.ajax({ - type: "PUT", - url: "/artists/" + id + ".json", - data: params, - success: () => { Utility.notice("Artist updated."); }, - error: () => { - Utility.error(`There was an error updating artist #${id}`); - }, - }); - }); -}; - -function init () { - $("#undelete-artist-link").on("click", e => { - if (confirm("Are you sure you want to undelete this artist?")) - Artist.update($(e.target).data("aid"), {"artist[is_active]": true}); - e.preventDefault(); - }); -} - -export default Artist; - -$(function () { - if ($("#c-artists").length) { - init(); - } -}); diff --git a/app/javascript/src/javascripts/autocomplete.js.erb b/app/javascript/src/javascripts/autocomplete.js.erb index d1755a782..50aaa7597 100644 --- a/app/javascript/src/javascripts/autocomplete.js.erb +++ b/app/javascript/src/javascripts/autocomplete.js.erb @@ -432,7 +432,6 @@ Autocomplete.artist_source = function(term, resp) { url: "/artists.json", data: { "search[name]": term.trim().replace(/\s+/g, "_") + "*", - "search[is_active]": true, "search[order]": "post_count", "limit": 10, "expiry": 7 diff --git a/app/models/artist.rb b/app/models/artist.rb index a19283cbf..422e976b6 100644 --- a/app/models/artist.rb +++ b/app/models/artist.rb @@ -14,6 +14,7 @@ class Artist < ApplicationRecord validate :user_not_limited validates :name, tag_name: true, uniqueness: true, if: :name_changed? validates :name, :group_name, length: { maximum: 100 } + before_destroy :log_destroy after_save :log_changes after_save :create_version after_save :categorize_tag @@ -21,18 +22,15 @@ class Artist < ApplicationRecord after_save :propagate_locked, if: :should_propagate_locked after_save :clear_url_string_changed - has_many :members, :class_name => "Artist", :foreign_key => "group_name", :primary_key => "name" - has_many :urls, :dependent => :destroy, :class_name => "ArtistUrl", :autosave => true - has_many :versions, -> {order("artist_versions.id ASC")}, :class_name => "ArtistVersion" - 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" + has_many :members, class_name: "Artist", foreign_key: "group_name", primary_key: "name" + has_many :urls, dependent: :destroy, class_name: "ArtistUrl", autosave: true + has_many :versions, -> {order("artist_versions.id ASC")}, class_name: "ArtistVersion" + 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" belongs_to :linked_user, class_name: "User", optional: true attribute :notes, :string - scope :active, -> { where(is_active: true) } - scope :deleted, -> { where(is_active: false) } - def log_changes if saved_change_to_name? && !previously_new_record? ModAction.log(:artist_page_rename, { new_name: name, old_name: name_before_last_save }) @@ -183,7 +181,7 @@ class Artist < ApplicationRecord while artists.empty? && url.length > 10 u = url.sub(/\/+$/, "") + "/" u = u.to_escaped_for_sql_like.gsub("*", "%") + "%" - artists += Artist.joins(:urls).where(["artists.is_active = TRUE AND artist_urls.normalized_url ILIKE ? ESCAPE E'\\\\'", u]).limit(10).order("artists.name").all + artists += Artist.joins(:urls).where(["artist_urls.normalized_url ILIKE ? ESCAPE E'\\\\'", u]).limit(10).order("artists.name").all url = File.dirname(url) + "/" break if url =~ SITE_BLACKLIST_REGEXP @@ -287,22 +285,22 @@ 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_other_names? || saved_change_to_group_name? || saved_change_to_notes? || force + if saved_change_to_name? || url_string_changed || saved_change_to_other_names? || saved_change_to_group_name? || saved_change_to_notes? || force create_new_version end end def create_new_version ArtistVersion.create( - :artist_id => id, - :name => name, - :updater_id => CurrentUser.id, - :updater_ip_addr => CurrentUser.ip_addr, - :urls => url_array, - :is_active => is_active, - :other_names => other_names, - :group_name => group_name, - :notes_changed => saved_change_to_notes? + artist_id: id, + name: name, + updater_id: CurrentUser.id, + updater_ip_addr: CurrentUser.ip_addr, + urls: url_array, + is_active: is_active, + other_names: other_names, + group_name: group_name, + notes_changed: saved_change_to_notes? ) end @@ -398,11 +396,6 @@ class Artist < ApplicationRecord def validate_user_can_edit return if CurrentUser.is_janitor? - if !is_active? - errors.add(:base, "Artist is inactive") - throw :abort - end - if is_locked? errors.add(:base, "Artist is locked") throw :abort @@ -472,8 +465,6 @@ class Artist < ApplicationRecord q = q.url_matches(params[:url_matches]) end - q = q.attribute_matches(:is_active, params[:is_active]) - q = q.where_user(:creator_id, :creator, params) if params[:has_tag].to_s.truthy? @@ -510,20 +501,12 @@ class Artist < ApplicationRecord include LockMethods extend SearchMethods - def status - if is_active? - "Active" - else - "Deleted" - end - end - def deletable_by?(user) - user.is_janitor? + user.is_admin? end def editable_by?(user) - user.is_janitor? || is_active? + user.is_janitor? end def user_not_limited @@ -543,4 +526,8 @@ class Artist < ApplicationRecord return false if CurrentUser.is_janitor? wiki_page&.is_locked? || false end + + def log_destroy + ModAction.log(:artist_delete, { artist_id: id, artist_name: name }) + end end diff --git a/app/models/wiki_page.rb b/app/models/wiki_page.rb index 752ea2713..3ff6eb47f 100644 --- a/app/models/wiki_page.rb +++ b/app/models/wiki_page.rb @@ -24,9 +24,9 @@ class WikiPage < ApplicationRecord array_attribute :other_names belongs_to_creator belongs_to_updater - has_one :tag, :foreign_key => "name", :primary_key => "title" - has_one :artist, -> {where(:is_active => true)}, :foreign_key => "name", :primary_key => "title" - has_many :versions, -> {order("wiki_page_versions.id ASC")}, :class_name => "WikiPageVersion", :dependent => :destroy + has_one :tag, foreign_key: "name", primary_key: "title" + has_one :artist, foreign_key: "name", primary_key: "title" + has_many :versions, -> { order("wiki_page_versions.id ASC") }, class_name: "WikiPageVersion", dependent: :destroy def validate_not_used_as_help_page if HelpPage.find_by(wiki_page: title).present? diff --git a/app/views/artists/_search.html.erb b/app/views/artists/_search.html.erb index 8df745b27..bb239b0d7 100644 --- a/app/views/artists/_search.html.erb +++ b/app/views/artists/_search.html.erb @@ -2,7 +2,6 @@ <%= f.input :any_name_matches, label: "Name", hint: "Use * for wildcard", autocomplete: "artist" %> <%= f.input :url_matches, label: "URL", as: :string %> <%= f.user :creator %> - <%= f.input :is_active, label: "Active?", collection: [["Yes", true], ["No", false]], include_blank: true %> <%= f.input :has_tag, label: "Has tag?", collection: [["Yes", true], ["No", false]], include_blank: true %> <%= f.input :is_linked, label: "Linked Only", as: :boolean %> <%= f.input :order, collection: [["Recently created", "created_at"], ["Last updated", "updated_at"], ["Name", "name"], ["Post count", "post_count"]] %> diff --git a/app/views/artists/_secondary_links.html.erb b/app/views/artists/_secondary_links.html.erb index b0261d7ec..1023c028c 100644 --- a/app/views/artists/_secondary_links.html.erb +++ b/app/views/artists/_secondary_links.html.erb @@ -16,11 +16,7 @@ <% end %> <%= subnav_link_to "History", artist_versions_path(search: { artist_id: @artist.id }) %> <% if @artist.deletable_by?(CurrentUser.user) %> - <% if @artist.is_active? %> - <%= subnav_link_to "Delete", artist_path(@artist), method: :delete, data: { confirm: "Are you sure you want to delete this artist?" } %> - <% else %> - <%= subnav_link_to "Undelete", "#", id: "undelete-artist-link", data: { aid: @artist.id } %> - <% end %> + <%= subnav_link_to "Delete", artist_path(@artist), method: :delete, data: { confirm: "Are you sure you want to delete this artist? This cannot be undone." } %> <% end %> <% end %> <% end %> diff --git a/app/views/artists/_show.html.erb b/app/views/artists/_show.html.erb index 6ad252634..e273bcd8d 100644 --- a/app/views/artists/_show.html.erb +++ b/app/views/artists/_show.html.erb @@ -1,6 +1,11 @@
-

Artist: <%= link_to @artist.pretty_name, posts_path(:tags => @artist.name), :class => "tag-type-#{@artist.category_id}" %>

+

+ Artist: <%= link_to @artist.pretty_name, posts_path(:tags => @artist.name), :class => "tag-type-#{@artist.category_id}" %> + <% if @artist.is_locked? %> + (locked) + <% end %> +

<% if @artist.notes.present? && @artist.visible? %>
diff --git a/app/views/artists/_summary.html.erb b/app/views/artists/_summary.html.erb index f596f559f..f3fb26384 100644 --- a/app/views/artists/_summary.html.erb +++ b/app/views/artists/_summary.html.erb @@ -1,7 +1,5 @@ <%# artist %>
    -
  • Status <%= artist.status %>
  • - <% if artist.linked_user_id && artist.linked_user %>
  • User <%= link_to_user(artist.linked_user) %>
  • <% end %> diff --git a/app/views/artists/edit.html.erb b/app/views/artists/edit.html.erb index 657c96a72..fb9ac0358 100644 --- a/app/views/artists/edit.html.erb +++ b/app/views/artists/edit.html.erb @@ -5,8 +5,8 @@ <% if @artist.editable_by?(CurrentUser.user) %> <%= render "form" %> - <% elsif !@artist.is_active? %> -

    This artist entry is inactive and cannot be edited.

    + <% elsif @artist.is_locked? %> +

    This artist entry is locked and cannot be edited.

    <% end %>
diff --git a/app/views/artists/index.html.erb b/app/views/artists/index.html.erb index d7e06061e..868bc81bf 100644 --- a/app/views/artists/index.html.erb +++ b/app/views/artists/index.html.erb @@ -4,9 +4,8 @@ - - - + + @@ -20,7 +19,6 @@ <% end %> - <% end %> <% end %> <% end %> diff --git a/test/factories/artist.rb b/test/factories/artist.rb index b15e9b229..33ceda713 100644 --- a/test/factories/artist.rb +++ b/test/factories/artist.rb @@ -3,7 +3,6 @@ FactoryBot.define do factory(:artist) do sequence(:name) { |n| "artist_#{n}" } - is_active { true } association :creator, factory: :user end end diff --git a/test/functional/artists_controller_test.rb b/test/functional/artists_controller_test.rb index 4a284f27e..bb9164b21 100644 --- a/test/functional/artists_controller_test.rb +++ b/test/functional/artists_controller_test.rb @@ -104,19 +104,14 @@ class ArtistsControllerTest < ActionDispatch::IntegrationTest end end - should "delete an artist" do - @janitor = create(:janitor_user) - delete_auth artist_path(@artist.id), @janitor - assert_redirected_to(artist_path(@artist.id)) - @artist.reload - assert_equal(false, @artist.is_active) - end - - should "undelete an artist" do - @janitor = create(:janitor_user) - put_auth artist_path(@artist.id), @janitor, params: {artist: {is_active: true}} - assert_redirected_to(artist_path(@artist.id)) - assert_equal(true, @artist.reload.is_active) + context "destroy action" do + should "work" do + assert_difference({ "Artist.count" => -1, "ModAction.count" => 1 }) do + delete_auth artist_path(@artist), @admin + end + assert_redirected_to(artists_path) + assert_raises(ActiveRecord::RecordNotFound) { @artist.reload } + end end context "reverting an artist" do diff --git a/test/unit/artist_test.rb b/test/unit/artist_test.rb index 18155e393..5d482a4ef 100644 --- a/test/unit/artist_test.rb +++ b/test/unit/artist_test.rb @@ -160,13 +160,6 @@ class ArtistTest < ActiveSupport::TestCase assert_equal(["http://foo.com"], artist.url_array) end - should "hide deleted artists" do - as(create(:admin_user)) do - create(:artist, name: "warhol", url_string: "http://warhol.com/a/image.jpg", is_active: false) - end - assert_artist_not_found("http://warhol.com/a/image.jpg") - end - context "when finding tumblr artists" do setup do create(:artist, name: "ilya_kuvshinov", url_string: "http://kuvshinov-ilya.tumblr.com") @@ -304,18 +297,6 @@ class ArtistTest < ActiveSupport::TestCase end end - context "that is deleted" do - setup do - @artist = create(:artist, url_string: "https://google.com") - @artist.update_attribute(:is_active, false) - @artist.reload - end - - should "preserve the url string" do - assert_equal(1, @artist.urls.count) - end - end - context "that is updated" do setup do @artist = create(:artist, name: "test") diff --git a/test/unit/artist_url_test.rb b/test/unit/artist_url_test.rb index ad99d3612..b55dfe965 100644 --- a/test/unit/artist_url_test.rb +++ b/test/unit/artist_url_test.rb @@ -78,9 +78,9 @@ class ArtistUrlTest < ActiveSupport::TestCase subject { ArtistUrl } should "work" do - @bkub = create(:artist, name: "bkub", is_active: true, url_string: "https://bkub.com") + @bkub = create(:artist, name: "bkub", url_string: "https://bkub.com") as(create(:admin_user)) do - @masao = create(:artist, name: "masao", is_active: false, url_string: "-https://masao.com") + @masao = create(:artist, name: "masao", url_string: "-https://masao.com") end @bkub_url = @bkub.urls.first @masao_url = @masao.urls.first
Primary NameSecondary NamesStatusPrimary NameSecondary Names
<%= artist.other_names_string %><%= artist.status %>