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.
This commit is contained in:
evazion
2020-11-12 18:22:51 -06:00
parent 5844f16ed6
commit 654d2175b6
7 changed files with 8 additions and 23 deletions

View File

@@ -90,7 +90,7 @@ class DText
def self.tag_request_message(obj) def self.tag_request_message(obj)
if obj.is_a?(TagRelationship) 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." "The #{obj.relationship} ##{obj.id} [[#{obj.antecedent_name}]] -> [[#{obj.consequent_name}]] has been approved."
elsif obj.is_retired? elsif obj.is_retired?
"The #{obj.relationship} ##{obj.id} [[#{obj.antecedent_name}]] -> [[#{obj.consequent_name}]] has been retired." "The #{obj.relationship} ##{obj.id} [[#{obj.antecedent_name}]] -> [[#{obj.consequent_name}]] has been retired."

View File

@@ -15,9 +15,8 @@ class TagAlias < TagRelationship
end end
def process!(approver) def process!(approver)
update!(approver: approver, status: "processing") update!(approver: approver, status: "active")
TagMover.new(antecedent_name, consequent_name, user: User.system).move! TagMover.new(antecedent_name, consequent_name, user: User.system).move!
update!(status: "active")
rescue Exception => e rescue Exception => e
update!(status: "error: #{e}") update!(status: "error: #{e}")
DanbooruLogger.log(e, tag_alias_id: id, antecedent_name: antecedent_name, consequent_name: consequent_name) DanbooruLogger.log(e, tag_alias_id: id, antecedent_name: antecedent_name, consequent_name: consequent_name)

View File

@@ -126,9 +126,8 @@ class TagImplication < TagRelationship
end end
CurrentUser.scoped(User.system) do CurrentUser.scoped(User.system) do
update(approver: approver, status: "processing") update!(approver: approver, status: "active")
update_posts update_posts
update(status: "active")
end end
rescue Exception => e rescue Exception => e
update(status: "error: #{e}") update(status: "error: #{e}")

View File

@@ -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 :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 belongs_to :consequent_wiki, class_name: "WikiPage", foreign_key: "consequent_name", primary_key: "title", optional: true
scope :active, -> {approved} scope :active, -> {where(status: "active")}
scope :approved, -> {where(status: %w[active processing])}
scope :deleted, -> {where(status: "deleted")} scope :deleted, -> {where(status: "deleted")}
scope :expired, -> {where("created_at < ?", EXPIRY.days.ago)} scope :expired, -> {where("created_at < ?", EXPIRY.days.ago)}
scope :old, -> {where("created_at >= ? and created_at < ?", EXPIRY.days.ago, EXPIRY_WARNING.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")} scope :retired, -> {where(status: "retired")}
before_validation :normalize_names 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_presence_of :antecedent_name, :consequent_name
validates :approver, presence: { message: "must exist" }, if: -> { approver_id.present? } validates :approver, presence: { message: "must exist" }, if: -> { approver_id.present? }
validates :forum_topic, presence: { message: "must exist" }, if: -> { forum_topic_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(" ", "_") self.consequent_name = consequent_name.mb_chars.downcase.tr(" ", "_")
end end
def is_approved?
status.in?(%w[active processing])
end
def is_rejected? def is_rejected?
status.in?(%w[retired deleted]) status.in?(%w[retired deleted])
end end
@@ -73,13 +68,7 @@ class TagRelationship < ApplicationRecord
end end
def status_matches(status) def status_matches(status)
status = status.downcase where(status: status.downcase)
if status == "approved"
where(status: %w[active processing])
else
where(status: status)
end
end end
def tag_matches(field, params) def tag_matches(field, params)
@@ -89,7 +78,7 @@ class TagRelationship < ApplicationRecord
def pending_first def pending_first
# unknown statuses return null and are sorted 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 end
def search(params) def search(params)

View File

@@ -9,7 +9,7 @@
<%= f.simple_fields_for :consequent_tag do |fa| %> <%= 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) %> <%= fa.input :category, label: "To Category", collection: TagCategory.canonical_mapping.to_a, include_blank: true, selected: params.dig(:search, :consequent_tag, :category) %>
<% end %> <% 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.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" %> <%= f.submit "Search" %>
<% end %> <% end %>

View File

@@ -27,7 +27,6 @@ class TagAliasTest < ActiveSupport::TestCase
should allow_value('active').for(:status) should allow_value('active').for(:status)
should allow_value('deleted').for(:status) should allow_value('deleted').for(:status)
should allow_value('pending').for(:status) should allow_value('pending').for(:status)
should allow_value('processing').for(:status)
should allow_value('error: derp').for(:status) should allow_value('error: derp').for(:status)
should_not allow_value('ACTIVE').for(:status) should_not allow_value('ACTIVE').for(:status)

View File

@@ -23,7 +23,6 @@ class TagImplicationTest < ActiveSupport::TestCase
should allow_value('active').for(:status) should allow_value('active').for(:status)
should allow_value('deleted').for(:status) should allow_value('deleted').for(:status)
should allow_value('pending').for(:status) should allow_value('pending').for(:status)
should allow_value('processing').for(:status)
should allow_value('error: derp').for(:status) should allow_value('error: derp').for(:status)
should_not allow_value('ACTIVE').for(:status) should_not allow_value('ACTIVE').for(:status)