From 4c11e339bd48d6c40e98dbfc9af3fe1686f3035a Mon Sep 17 00:00:00 2001 From: evazion Date: Fri, 6 Mar 2020 14:50:21 -0600 Subject: [PATCH] artists: rename is_active flag to is_deleted. Rename is_active to is_deleted. This is for better consistency with other models, and to reduce confusion over what "active" means for artists. Sometimes users think active is for whether the artist is actively producing work. --- app/controllers/artists_controller.rb | 4 +-- .../src/javascripts/autocomplete.js.erb | 2 +- app/logical/post_sets/post.rb | 2 +- app/models/artist.rb | 26 +++++++++---------- app/models/artist_version.rb | 4 +-- app/models/wiki_page.rb | 2 +- app/views/artists/_search.html.erb | 2 +- app/views/artists/_secondary_links.html.erb | 6 ++--- app/views/artists/index.html.erb | 10 +++---- app/views/legacy/artists.xml.erb | 2 +- ...00306202253_rename_is_active_on_artists.rb | 25 ++++++++++++++++++ db/structure.sql | 5 ++-- test/factories/artist.rb | 2 +- test/functional/artists_controller_test.rb | 8 +++--- test/unit/artist_test.rb | 4 +-- test/unit/artist_url_test.rb | 6 ++--- 16 files changed, 68 insertions(+), 42 deletions(-) create mode 100644 db/migrate/20200306202253_rename_is_active_on_artists.rb diff --git a/app/controllers/artists_controller.rb b/app/controllers/artists_controller.rb index 97fd01578..836be23c6 100644 --- a/app/controllers/artists_controller.rb +++ b/app/controllers/artists_controller.rb @@ -55,7 +55,7 @@ class ArtistsController < ApplicationController end def destroy - @artist.update_attribute(:is_active, false) + @artist.update_attribute(:is_deleted, true) redirect_to(artist_path(@artist), :notice => "Artist deleted") end @@ -94,7 +94,7 @@ class ArtistsController < ApplicationController end def artist_params(context = nil) - permitted_params = %i[name other_names other_names_string group_name url_string notes is_active] + permitted_params = %i[name other_names other_names_string group_name url_string notes is_deleted] permitted_params << { wiki_page_attributes: %i[id body] } permitted_params << :source if context == :new diff --git a/app/javascript/src/javascripts/autocomplete.js.erb b/app/javascript/src/javascripts/autocomplete.js.erb index 1a57746e3..9e052579b 100644 --- a/app/javascript/src/javascripts/autocomplete.js.erb +++ b/app/javascript/src/javascripts/autocomplete.js.erb @@ -328,7 +328,7 @@ Autocomplete.tag_source = async function(term) { Autocomplete.artist_source = async function(term) { let artists = await $.getJSON("/artists", { "search[name_like]": term.trim().replace(/\s+/g, "_") + "*", - "search[is_active]": true, + "search[is_deleted]": false, "search[order]": "post_count", "limit": Autocomplete.MAX_RESULTS, "expiry": 7 diff --git a/app/logical/post_sets/post.rb b/app/logical/post_sets/post.rb index c19fda4f5..a5347d7fe 100644 --- a/app/logical/post_sets/post.rb +++ b/app/logical/post_sets/post.rb @@ -37,7 +37,7 @@ module PostSets def artist return nil unless tag.present? && tag.category == Tag.categories.artist - return nil unless tag.artist.present? && tag.artist.is_active? + return nil unless tag.artist.present? && !tag.artist.is_deleted? tag.artist end diff --git a/app/models/artist.rb b/app/models/artist.rb index 3e53346b2..c51854912 100644 --- a/app/models/artist.rb +++ b/app/models/artist.rb @@ -20,8 +20,8 @@ class Artist < ApplicationRecord 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) } + scope :active, -> { where(is_deleted: false) } + scope :deleted, -> { where(is_deleted: true) } scope :banned, -> { where(is_banned: true) } scope :unbanned, -> { where(is_banned: false) } @@ -158,7 +158,7 @@ class Artist < ApplicationRecord while artists.empty? && url.size > 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 LIKE ? ESCAPE E'\\\\'", u]).limit(10).order("artists.name").all + artists += Artist.joins(:urls).where(["artists.is_deleted = FALSE AND artist_urls.normalized_url LIKE ? ESCAPE E'\\\\'", u]).limit(10).order("artists.name").all url = File.dirname(url) + "/" break if url =~ SITE_BLACKLIST_REGEXP @@ -221,7 +221,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? || force + if saved_change_to_name? || url_string_changed || saved_change_to_is_deleted? || saved_change_to_is_banned? || saved_change_to_other_names? || saved_change_to_group_name? || force if merge_version? merge_version else @@ -237,7 +237,7 @@ class Artist < ApplicationRecord :updater_id => CurrentUser.id, :updater_ip_addr => CurrentUser.ip_addr, :urls => url_array, - :is_active => is_active, + :is_deleted => is_deleted, :is_banned => is_banned, :other_names => other_names, :group_name => group_name @@ -246,7 +246,7 @@ class Artist < ApplicationRecord def merge_version prev = versions.last - prev.update(name: name, urls: url_array, is_active: is_active, is_banned: is_banned, other_names: other_names, group_name: group_name) + prev.update(name: name, urls: url_array, is_deleted: is_deleted, is_banned: is_banned, other_names: other_names, group_name: group_name) end def merge_version? @@ -261,7 +261,7 @@ class Artist < ApplicationRecord self.name = version.name self.url_string = version.urls.join("\n") - self.is_active = version.is_active + self.is_deleted = version.is_deleted self.other_names = version.other_names self.group_name = version.group_name save @@ -296,7 +296,7 @@ class Artist < ApplicationRecord module TagMethods def validate_tag_category - return unless is_active? && name_changed? + return unless !is_deleted? && name_changed? if tag.category_name == "General" tag.update(category: Tag.categories.artist) @@ -385,7 +385,7 @@ class Artist < ApplicationRecord def search(params) q = super - q = q.search_attributes(params, :is_active, :is_banned, :name, :group_name, :other_names) + q = q.search_attributes(params, :is_deleted, :is_banned, :name, :group_name, :other_names) if params[:any_other_name_like] q = q.any_other_name_like(params[:any_other_name_like]) @@ -433,14 +433,14 @@ class Artist < ApplicationRecord extend SearchMethods def status - if is_banned? && is_active? + if is_banned? && !is_deleted? "Banned" elsif is_banned? "Banned Deleted" - elsif is_active? - "Active" - else + elsif is_deleted? "Deleted" + else + "Active" end end diff --git a/app/models/artist_version.rb b/app/models/artist_version.rb index 74c37ad19..b8dc2d705 100644 --- a/app/models/artist_version.rb +++ b/app/models/artist_version.rb @@ -52,11 +52,11 @@ class ArtistVersion < ApplicationRecord end def was_deleted - !is_active && previous.is_active + is_deleted && !previous.is_deleted end def was_undeleted - is_active && !previous.is_active + !is_deleted && previous.is_deleted end def was_banned diff --git a/app/models/wiki_page.rb b/app/models/wiki_page.rb index 1f7d45021..37d3e3a07 100644 --- a/app/models/wiki_page.rb +++ b/app/models/wiki_page.rb @@ -15,7 +15,7 @@ class WikiPage < ApplicationRecord array_attribute :other_names has_one :tag, :foreign_key => "name", :primary_key => "title" - has_one :artist, -> {where(:is_active => true)}, :foreign_key => "name", :primary_key => "title" + has_one :artist, -> { active }, foreign_key: "name", primary_key: "title" has_many :versions, -> {order("wiki_page_versions.id ASC")}, :class_name => "WikiPageVersion", :dependent => :destroy has_many :dtext_links, as: :model, dependent: :destroy diff --git a/app/views/artists/_search.html.erb b/app/views/artists/_search.html.erb index ccca9f484..f15a6bdeb 100644 --- a/app/views/artists/_search.html.erb +++ b/app/views/artists/_search.html.erb @@ -1,7 +1,7 @@ <%= search_form_for(artists_path) do |f| %> <%= f.input :any_name_matches, label: "Name", hint: "Use * for wildcard", input_html: { value: params[:search][:any_name_matches], data: { autocomplete: "artist" }} %> <%= f.input :url_matches, label: "URL", as: "string", input_html: { value: params[:search][:url_matches] } %> - <%= f.input :is_active, label: "Active?", collection: [["Yes", true], ["No", false]], include_blank: true, selected: params[:search][:is_active] %> + <%= f.input :is_deleted, label: "Deleted?", collection: [["Yes", true], ["No", false]], include_blank: true, selected: params[:search][:is_deleted] %> <%= f.input :is_banned, label: "Banned?", collection: [["Yes", true], ["No", false]], include_blank: true, selected: params[:search][:is_banned] %> <%= f.input :has_tag, label: "Has tag?", collection: [["Yes", true], ["No", false]], include_blank: true, selected: params[:search][:has_tag] %> <%= f.input :order, collection: [["Recently created", "created_at"], ["Last updated", "updated_at"], ["Name", "name"], ["Post count", "post_count"]], selected: params[:search][:order] %> diff --git a/app/views/artists/_secondary_links.html.erb b/app/views/artists/_secondary_links.html.erb index f67a0d8ea..0bd6e7d80 100644 --- a/app/views/artists/_secondary_links.html.erb +++ b/app/views/artists/_secondary_links.html.erb @@ -16,10 +16,10 @@ <% end %> <%= subnav_link_to "History", artist_versions_path(:search => {:artist_id => @artist.id}) %> <% if CurrentUser.is_member? %> - <% if @artist.is_active? %> - <%= subnav_link_to "Delete", artist_path(@artist), method: :delete, "data-shortcut": "shift+d", "data-confirm": "Are you sure you want to delete this artist?" %> + <% if @artist.is_deleted? %> + <%= subnav_link_to "Undelete", artist_path(@artist, format: "js"), method: :put, data: {confirm: "Are you sure you want to undelete this artist?", params: "artist[is_deleted]=false"}, remote: true %> <% else %> - <%= subnav_link_to "Undelete", artist_path(@artist, format: "js"), method: :put, data: {confirm: "Are you sure you want to undelete this artist?", params: "artist[is_active]=true"}, remote: true %> + <%= subnav_link_to "Delete", artist_path(@artist), method: :delete, "data-shortcut": "shift+d", "data-confirm": "Are you sure you want to delete this artist?" %> <% end %> <% end %> <% if CurrentUser.is_admin? %> diff --git a/app/views/artists/index.html.erb b/app/views/artists/index.html.erb index b707e1def..deb76e6ad 100644 --- a/app/views/artists/index.html.erb +++ b/app/views/artists/index.html.erb @@ -18,8 +18,8 @@ <% end %> <% end %> <% t.column "Status" do |artist| %> - <% if !artist.is_active? %> - <%= link_to "Deleted", artists_path(search: { is_active: false }) %> + <% if artist.is_deleted? %> + <%= link_to "Deleted", artists_path(search: { is_deleted: true }) %> <% end %> <% if artist.is_banned? %> @@ -33,10 +33,10 @@ <% if CurrentUser.is_member? %> <%= link_to "Edit", edit_artist_path(artist) %> - <% if artist.is_active? %> - | <%= link_to "Delete", artist_path(artist, artist: { is_active: false }), method: :put, remote: true %> + <% if artist.is_deleted? %> + | <%= link_to "Undelete", artist_path(artist, artist: { is_deleted: false }), method: :put, remote: true %> <% else %> - | <%= link_to "Undelete", artist_path(artist, artist: { is_active: true }), method: :put, remote: true %> + | <%= link_to "Delete", artist_path(artist, artist: { is_deleted: true }), method: :put, remote: true %> <% end %> <% end %> <% end %> diff --git a/app/views/legacy/artists.xml.erb b/app/views/legacy/artists.xml.erb index f545b6051..60b65e14e 100644 --- a/app/views/legacy/artists.xml.erb +++ b/app/views/legacy/artists.xml.erb @@ -1,6 +1,6 @@ <% @artists.each do |artist| %> - " is_active="<%= artist.is_active? %>" name="<%= artist.name %>" updater_id="0" id="<%= artist.id %>" version="0"/> + " is_active="<%= !artist.is_deleted? %>" name="<%= artist.name %>" updater_id="0" id="<%= artist.id %>" version="0"/> <% end %> diff --git a/db/migrate/20200306202253_rename_is_active_on_artists.rb b/db/migrate/20200306202253_rename_is_active_on_artists.rb new file mode 100644 index 000000000..7c84586d9 --- /dev/null +++ b/db/migrate/20200306202253_rename_is_active_on_artists.rb @@ -0,0 +1,25 @@ +class RenameIsActiveOnArtists < ActiveRecord::Migration[6.0] + def up + execute "SET statement_timeout = 0" + + rename_column :artists, :is_active, :is_deleted + change_column_default :artists, :is_deleted, from: true, to: false + execute "UPDATE artists SET is_deleted = NOT is_deleted" + + rename_column :artist_versions, :is_active, :is_deleted + change_column_default :artist_versions, :is_deleted, from: true, to: false + execute "UPDATE artist_versions SET is_deleted = NOT is_deleted" + end + + def down + execute "SET statement_timeout = 0" + + execute "UPDATE artists SET is_deleted = NOT is_deleted" + change_column_default :artists, :is_deleted, from: false, to: true + rename_column :artists, :is_deleted, :is_active + + execute "UPDATE artist_versions SET is_deleted = NOT is_deleted" + change_column_default :artist_versions, :is_deleted, from: false, to: true + rename_column :artist_versions, :is_deleted, :is_active + end +end diff --git a/db/structure.sql b/db/structure.sql index de5da94b8..a407c7333 100644 --- a/db/structure.sql +++ b/db/structure.sql @@ -594,7 +594,7 @@ ALTER SEQUENCE public.artist_versions_id_seq OWNED BY public.artist_versions.id; CREATE TABLE public.artists ( id integer NOT NULL, name character varying NOT NULL, - is_active boolean DEFAULT true NOT NULL, + is_deleted boolean DEFAULT false NOT NULL, is_banned boolean DEFAULT false NOT NULL, other_names text[] DEFAULT '{}'::text[] NOT NULL, group_name character varying DEFAULT ''::character varying NOT NULL, @@ -7307,6 +7307,7 @@ INSERT INTO "schema_migrations" (version) VALUES ('20200123184743'), ('20200217044719'), ('20200223042415'), -('20200223234015'); +('20200223234015'), +('20200306202253'); diff --git a/test/factories/artist.rb b/test/factories/artist.rb index def197791..5240d23cd 100644 --- a/test/factories/artist.rb +++ b/test/factories/artist.rb @@ -1,6 +1,6 @@ FactoryBot.define do factory(:artist) do name { rand(1_000_000).to_s } - is_active { true } + is_deleted { false } end end diff --git a/test/functional/artists_controller_test.rb b/test/functional/artists_controller_test.rb index 39b3841b3..83d11fcc9 100644 --- a/test/functional/artists_controller_test.rb +++ b/test/functional/artists_controller_test.rb @@ -127,7 +127,7 @@ class ArtistsControllerTest < ActionDispatch::IntegrationTest should "create an artist" do attributes = FactoryBot.attributes_for(:artist) assert_difference("Artist.count", 1) do - attributes.delete(:is_active) + attributes.delete(:is_deleted) post_auth artists_path, @user, params: {artist: attributes} end artist = Artist.find_by_name(attributes[:name]) @@ -171,14 +171,14 @@ class ArtistsControllerTest < ActionDispatch::IntegrationTest delete_auth artist_path(@artist.id), @builder assert_redirected_to(artist_path(@artist.id)) @artist.reload - assert_equal(false, @artist.is_active) + assert_equal(true, @artist.is_deleted) end should "undelete an artist" do @builder = create(:builder_user) - put_auth artist_path(@artist.id), @builder, params: {artist: {is_active: true}} + put_auth artist_path(@artist.id), @builder, params: {artist: {is_deleted: false}} assert_redirected_to(artist_path(@artist.id)) - assert_equal(true, @artist.reload.is_active) + assert_equal(false, @artist.reload.is_deleted) end context "reverting an artist" do diff --git a/test/unit/artist_test.rb b/test/unit/artist_test.rb index deca85f5e..16aa3eafe 100644 --- a/test/unit/artist_test.rb +++ b/test/unit/artist_test.rb @@ -191,7 +191,7 @@ class ArtistTest < ActiveSupport::TestCase end should "hide deleted artists" do - FactoryBot.create(:artist, :name => "warhol", :url_string => "http://warhol.com/a/image.jpg", :is_active => false) + create(:artist, name: "warhol", url_string: "http://warhol.com/a/image.jpg", is_deleted: true) assert_artist_not_found("http://warhol.com/a/image.jpg") end @@ -512,7 +512,7 @@ class ArtistTest < ActiveSupport::TestCase context "that is deleted" do setup do @artist = create(:artist, url_string: "https://google.com") - @artist.update_attribute(:is_active, false) + @artist.update_attribute(:is_deleted, true) @artist.reload end diff --git a/test/unit/artist_url_test.rb b/test/unit/artist_url_test.rb index e66822c53..f1cce9149 100644 --- a/test/unit/artist_url_test.rb +++ b/test/unit/artist_url_test.rb @@ -191,12 +191,12 @@ class ArtistUrlTest < ActiveSupport::TestCase subject { ArtistUrl } should "work" do - @bkub = FactoryBot.create(:artist, name: "bkub", is_active: true, url_string: "https://bkub.com") - @masao = FactoryBot.create(:artist, name: "masao", is_active: false, url_string: "-https://masao.com") + @bkub = create(:artist, name: "bkub", url_string: "https://bkub.com") + @masao = create(:artist, name: "masao", url_string: "-https://masao.com") @bkub_url = @bkub.urls.first @masao_url = @masao.urls.first - assert_search_equals([@bkub_url], is_active: true) + assert_search_equals([@bkub_url], is_deleted: false) assert_search_equals([@bkub_url], artist: { name: "bkub" }) assert_search_equals([@bkub_url], url_matches: "*bkub*")