[Artists] Convert is_active into proper deletion (#660)

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)

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"

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';

app/javascript/src/javascripts/artist.js

@@ -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 <a href="/artists/${id}">artist #${id}</a>`);
-      },
-    });
-  });
-};
-
-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();
-  }
-});

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

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

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?

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"]] %>

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 %>

app/views/artists/_show.html.erb

@@ -1,6 +1,11 @@
 <div id="c-artists">
   <div id="a-show">
-    <h1>Artist: <%= link_to @artist.pretty_name, posts_path(:tags => @artist.name), :class => "tag-type-#{@artist.category_id}" %></h1>
+    <h1>
+      Artist: <%= link_to @artist.pretty_name, posts_path(:tags => @artist.name), :class => "tag-type-#{@artist.category_id}" %>
+      <% if @artist.is_locked? %>
+        (locked)
+      <% end %>
+    </h1>
 
     <% if @artist.notes.present? && @artist.visible? %>
       <div class="dtext-container">

app/views/artists/_summary.html.erb

@@ -1,7 +1,5 @@
 <%# artist %>
 <ul>
-  <li><strong>Status</strong> <%= artist.status %></li>
-
   <% if artist.linked_user_id && artist.linked_user %>
     <li><strong>User</strong> <%= link_to_user(artist.linked_user) %></li>
   <% end %>

app/views/artists/edit.html.erb

@@ -5,8 +5,8 @@
     <% if @artist.editable_by?(CurrentUser.user) %>
       <%= render "form" %>
 
-    <% elsif !@artist.is_active? %>
-      <p>This artist entry is inactive and cannot be edited.</p>
+    <% elsif @artist.is_locked? %>
+      <p>This artist entry is locked and cannot be edited.</p>
 
     <% end %>
   </div>

app/views/artists/index.html.erb

@@ -4,9 +4,8 @@
     <table class="striped">
       <thead>
         <tr>
-          <th width="45%">Primary Name</th>
-          <th width="45%">Secondary Names</th>
-          <th width="10%">Status</th>
+          <th width="50%">Primary Name</th>
+          <th width="50%">Secondary Names</th>
         </tr>
       </thead>
       <tbody>
@@ -20,7 +19,6 @@
                 <% end %>
               </td>
               <td><%= artist.other_names_string %></td>
-              <td><%= artist.status %></td>
             <% end %>
           <% end %>
         <% end %>

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

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)
+    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
-
-    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)
     end
 
     context "reverting an artist" do

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")

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