From 654d2175b691a5bb9993facde536627eb8ab8a24 Mon Sep 17 00:00:00 2001 From: evazion Date: Thu, 12 Nov 2020 18:22:51 -0600 Subject: [PATCH] aliases/implications: remove `processing` state. Remove the `processing` state from aliases and implications. This state was used to mark when an alias or implication had been approved but the alias or implication was still being processed. Aliases in the processing state were still considered active, so there was no functional difference between the active state and the processing state. This fixes a problem where it was possible for implications to get stuck in the processing state. This happened when a BUR contained a duplicate implication. Transitioning from the processing state to the active state failed in this case because we used `update` instead of `update!`, which meant validation errors were silently ignored. --- app/logical/d_text.rb | 2 +- app/models/tag_alias.rb | 3 +-- app/models/tag_implication.rb | 3 +-- app/models/tag_relationship.rb | 19 ++++--------------- app/views/tag_relationships/_search.html.erb | 2 +- test/unit/tag_alias_test.rb | 1 - test/unit/tag_implication_test.rb | 1 - 7 files changed, 8 insertions(+), 23 deletions(-) diff --git a/app/logical/d_text.rb b/app/logical/d_text.rb index 8666b1f0e..db4d4c7d5 100644 --- a/app/logical/d_text.rb +++ b/app/logical/d_text.rb @@ -90,7 +90,7 @@ class DText def self.tag_request_message(obj) if obj.is_a?(TagRelationship) - if obj.is_approved? + if obj.is_active? "The #{obj.relationship} ##{obj.id} [[#{obj.antecedent_name}]] -> [[#{obj.consequent_name}]] has been approved." elsif obj.is_retired? "The #{obj.relationship} ##{obj.id} [[#{obj.antecedent_name}]] -> [[#{obj.consequent_name}]] has been retired." diff --git a/app/models/tag_alias.rb b/app/models/tag_alias.rb index 2f0dbca6a..9b3b0f42d 100644 --- a/app/models/tag_alias.rb +++ b/app/models/tag_alias.rb @@ -15,9 +15,8 @@ class TagAlias < TagRelationship end def process!(approver) - update!(approver: approver, status: "processing") + update!(approver: approver, status: "active") TagMover.new(antecedent_name, consequent_name, user: User.system).move! - update!(status: "active") rescue Exception => e update!(status: "error: #{e}") DanbooruLogger.log(e, tag_alias_id: id, antecedent_name: antecedent_name, consequent_name: consequent_name) diff --git a/app/models/tag_implication.rb b/app/models/tag_implication.rb index c91605004..a4e7649ab 100644 --- a/app/models/tag_implication.rb +++ b/app/models/tag_implication.rb @@ -126,9 +126,8 @@ class TagImplication < TagRelationship end CurrentUser.scoped(User.system) do - update(approver: approver, status: "processing") + update!(approver: approver, status: "active") update_posts - update(status: "active") end rescue Exception => e update(status: "error: #{e}") diff --git a/app/models/tag_relationship.rb b/app/models/tag_relationship.rb index 02675032c..921394458 100644 --- a/app/models/tag_relationship.rb +++ b/app/models/tag_relationship.rb @@ -15,8 +15,7 @@ class TagRelationship < ApplicationRecord belongs_to :antecedent_wiki, class_name: "WikiPage", foreign_key: "antecedent_name", primary_key: "title", optional: true belongs_to :consequent_wiki, class_name: "WikiPage", foreign_key: "consequent_name", primary_key: "title", optional: true - scope :active, -> {approved} - scope :approved, -> {where(status: %w[active processing])} + scope :active, -> {where(status: "active")} scope :deleted, -> {where(status: "deleted")} scope :expired, -> {where("created_at < ?", EXPIRY.days.ago)} scope :old, -> {where("created_at >= ? and created_at < ?", EXPIRY.days.ago, EXPIRY_WARNING.days.ago)} @@ -24,7 +23,7 @@ class TagRelationship < ApplicationRecord scope :retired, -> {where(status: "retired")} before_validation :normalize_names - validates_format_of :status, :with => /\A(active|deleted|pending|processing|retired|error: .*)\Z/ + validates_format_of :status, :with => /\A(active|deleted|pending|retired|error: .*)\Z/ validates_presence_of :antecedent_name, :consequent_name validates :approver, presence: { message: "must exist" }, if: -> { approver_id.present? } validates :forum_topic, presence: { message: "must exist" }, if: -> { forum_topic_id.present? } @@ -35,10 +34,6 @@ class TagRelationship < ApplicationRecord self.consequent_name = consequent_name.mb_chars.downcase.tr(" ", "_") end - def is_approved? - status.in?(%w[active processing]) - end - def is_rejected? status.in?(%w[retired deleted]) end @@ -73,13 +68,7 @@ class TagRelationship < ApplicationRecord end def status_matches(status) - status = status.downcase - - if status == "approved" - where(status: %w[active processing]) - else - where(status: status) - end + where(status: status.downcase) end def tag_matches(field, params) @@ -89,7 +78,7 @@ class TagRelationship < ApplicationRecord def pending_first # unknown statuses return null and are sorted first - order(Arel.sql("array_position(array['processing', 'pending', 'active', 'deleted', 'retired'], status::text) NULLS FIRST, id DESC")) + order(Arel.sql("array_position(array['pending', 'active', 'deleted', 'retired'], status::text) NULLS FIRST, id DESC")) end def search(params) diff --git a/app/views/tag_relationships/_search.html.erb b/app/views/tag_relationships/_search.html.erb index 32d197ae0..719c921e3 100644 --- a/app/views/tag_relationships/_search.html.erb +++ b/app/views/tag_relationships/_search.html.erb @@ -9,7 +9,7 @@ <%= f.simple_fields_for :consequent_tag do |fa| %> <%= fa.input :category, label: "To Category", collection: TagCategory.canonical_mapping.to_a, include_blank: true, selected: params.dig(:search, :consequent_tag, :category) %> <% end %> - <%= f.input :status, label: "Status", collection: %w[Approved Active Pending Deleted Retired Processing], include_blank: true, selected: params[:search][:status] %> + <%= f.input :status, label: "Status", collection: %w[Active Pending Deleted Retired], include_blank: true, selected: params[:search][:status] %> <%= f.input :order, label: "Order", collection: [%w[Created created_at], %w[Updated updated_at], %w[Name name], %w[Tag\ count tag_count], %w[Status status]], include_blank: true, selected: params[:search][:order] %> <%= f.submit "Search" %> <% end %> diff --git a/test/unit/tag_alias_test.rb b/test/unit/tag_alias_test.rb index a8fe21d20..604ce57fa 100644 --- a/test/unit/tag_alias_test.rb +++ b/test/unit/tag_alias_test.rb @@ -27,7 +27,6 @@ class TagAliasTest < ActiveSupport::TestCase should allow_value('active').for(:status) should allow_value('deleted').for(:status) should allow_value('pending').for(:status) - should allow_value('processing').for(:status) should allow_value('error: derp').for(:status) should_not allow_value('ACTIVE').for(:status) diff --git a/test/unit/tag_implication_test.rb b/test/unit/tag_implication_test.rb index 508c66221..85d7c6293 100644 --- a/test/unit/tag_implication_test.rb +++ b/test/unit/tag_implication_test.rb @@ -23,7 +23,6 @@ class TagImplicationTest < ActiveSupport::TestCase should allow_value('active').for(:status) should allow_value('deleted').for(:status) should allow_value('pending').for(:status) - should allow_value('processing').for(:status) should allow_value('error: derp').for(:status) should_not allow_value('ACTIVE').for(:status)