diff --git a/app/assets/stylesheets/views/mod-actions.scss b/app/assets/stylesheets/views/mod-actions.scss
index 58ad40dab..79b1e85c7 100644
--- a/app/assets/stylesheets/views/mod-actions.scss
+++ b/app/assets/stylesheets/views/mod-actions.scss
@@ -224,6 +224,21 @@
}
}
+ .adjustment-restriction__container--text {
+ color: var(--color-secondary);
+ }
+
+ .adjustment-restriction__container--tag {
+ font-family: var(--ff-monospace);
+ line-height: var(--lh-base);
+ letter-spacing: -0.32px;
+ color: var(--tag-name);
+
+ .num-sign {
+ color: var(--tag-hash);
+ }
+ }
+
.additional-subtext-section {
font-weight: var(--fw-medium);
font-size: var(--fs-xs);
@@ -290,6 +305,7 @@
.reaction-vomit-button.reacted .reacted-emoji {
display: block;
}
+
.other-things-container {
border-top: 1px solid var(--base-20);
overflow-y: auto;
@@ -335,6 +351,7 @@
}
}
}
+
.toggle-chevron-container {
width: 16px;
height: 16px;
@@ -350,6 +367,7 @@
transform: rotateZ(-180deg);
}
}
+
&:hover {
background-color: var(--base-inverted);
.label-wrapper > .icon {
@@ -362,11 +380,13 @@
color: var(--accent-brand);
}
}
+
&.active {
header h2 {
font-weight: var(--fw-bold);
}
}
+
&.inactive:not(.active) {
header h2 {
color: var(--base-50);
@@ -385,6 +405,7 @@
}
}
}
+
.dropdown-options {
flex-direction: column;
padding: 0 var(--su-4);
@@ -394,6 +415,7 @@
display: flex;
}
}
+
.adjust-tags-options {
.add-tag-container {
padding: var(--su-2) var(--su-1);
@@ -482,6 +504,7 @@
}
}
}
+
button.add-tag {
background-color: inherit;
border: none;
@@ -552,6 +575,7 @@
}
}
}
+
.set-experience-options {
button.level-rating-button {
background: inherit;
diff --git a/app/helpers/moderations/actions_panel_helper.rb b/app/helpers/moderations/actions_panel_helper.rb
new file mode 100644
index 000000000..77983b440
--- /dev/null
+++ b/app/helpers/moderations/actions_panel_helper.rb
@@ -0,0 +1,10 @@
+module Moderations
+ module ActionsPanelHelper
+ def last_adjusted_by_admin?(article, tag, adjustment_type)
+ last_user_id = TagAdjustment.where(article_id: article.id, adjustment_type: adjustment_type, status: "committed",
+ tag_id: tag.id)
+ .last&.user_id
+ User.find_by(id: last_user_id)&.any_admin?
+ end
+ end
+end
diff --git a/app/models/article.rb b/app/models/article.rb
index e27bb6f4d..ecf2d8041 100644
--- a/app/models/article.rb
+++ b/app/models/article.rb
@@ -817,10 +817,20 @@ class Article < ApplicationRecord
published_at || date || Time.current
end
+ # When an article is saved, it ensures that the tags that were adjusted by moderators and admins
+ # remain adjusted. We do not allow the author to add or remove tags that were previously added or
+ # removed by moderators and admins.
+ #
+ # This method is called before validation, so that the tag_list can be validated.
+ #
+ # @return [String] an array of tag names.
def validate_tag
- # remove adjusted tags
- remove_tag_adjustments_from_tag_list
- add_tag_adjustments_to_tag_list
+ distinct_tag_adjustments = TagAdjustment.where(article_id: id, status: "committed")
+ .select('DISTINCT ON ("tag_id") *')
+ .order(:tag_id, updated_at: :desc, id: :desc)
+
+ remove_tag_adjustments_from_tag_list(distinct_tag_adjustments)
+ add_tag_adjustments_to_tag_list(distinct_tag_adjustments)
# check there are not too many tags
return errors.add(:tag_list, I18n.t("models.article.too_many_tags")) if tag_list.size > MAX_TAG_LIST_SIZE
@@ -828,14 +838,13 @@ class Article < ApplicationRecord
validate_tag_name(tag_list)
end
- def remove_tag_adjustments_from_tag_list
- tags_to_remove = TagAdjustment.where(article_id: id, adjustment_type: "removal",
- status: "committed").pluck(:tag_name)
+ def remove_tag_adjustments_from_tag_list(distinct_adjustments)
+ tags_to_remove = distinct_adjustments.select { |adj| adj.adjustment_type == "removal" }.pluck(:tag_name)
tag_list.remove(tags_to_remove, parse: true) if tags_to_remove.present?
end
- def add_tag_adjustments_to_tag_list
- tags_to_add = TagAdjustment.where(article_id: id, adjustment_type: "addition", status: "committed").pluck(:tag_name)
+ def add_tag_adjustments_to_tag_list(distinct_adjustments)
+ tags_to_add = distinct_adjustments.select { |adj| adj.adjustment_type == "addition" }.pluck(:tag_name)
return if tags_to_add.blank?
tag_list.add(tags_to_add, parse: true)
diff --git a/app/models/tag_adjustment.rb b/app/models/tag_adjustment.rb
index aec115d09..9b1d928d5 100644
--- a/app/models/tag_adjustment.rb
+++ b/app/models/tag_adjustment.rb
@@ -1,6 +1,5 @@
class TagAdjustment < ApplicationRecord
- validates :tag_name, presence: true,
- uniqueness: { scope: :article_id, message: I18n.t("models.tag_adjustment.unique") }
+ validates :tag_name, presence: true
validates :adjustment_type, inclusion: { in: %w[removal addition] }, presence: true
validates :status, inclusion: { in: %w[committed pending committed_and_resolvable resolved] }, presence: true
has_many :notifications, as: :notifiable, inverse_of: :notifiable, dependent: :delete_all
diff --git a/app/views/moderations/actions_panel.html.erb b/app/views/moderations/actions_panel.html.erb
index cec38aad1..21aff8793 100644
--- a/app/views/moderations/actions_panel.html.erb
+++ b/app/views/moderations/actions_panel.html.erb
@@ -110,7 +110,12 @@
<% @moderatable.tags.each do |tag| %>
- <% moderatable_tag = @tag_moderator_tags.pluck(:name).include?(tag.name) || policy(@moderatable).can_adjust_any_tag? %>
+ <% last_added_by_admin = last_adjusted_by_admin?(@moderatable, tag, "addition") %>
+ <% current_tag_moderator = @tag_moderator_tags.pluck(:name).include?(tag.name) %>
+
+ <%# The tag is moderatable if the user is a current moderator of that tag and that tag was not added by admin OR the user is an admin %>
+ <% moderatable_tag = (current_tag_moderator && !last_added_by_admin) || policy(@moderatable).can_adjust_any_tag? %>
+
+
+ <% if current_tag_moderator && last_added_by_admin %>
+
+ <% end %>
<% end %>
+
<% end %>
<% end %>
diff --git a/config/locales/views/moderations/en.yml b/config/locales/views/moderations/en.yml
index a019f441a..214ccf350 100644
--- a/config/locales/views/moderations/en.yml
+++ b/config/locales/views/moderations/en.yml
@@ -23,19 +23,21 @@ en:
history:
added_by: added by
removed_by: removed by
- reason:
+ reason:
remove_tag: Reason to remove tag (optional)
add_tag: Reason to add tag (optional)
remove: Remove tag
submit: Submit
cancel: Cancel
- maximum_tags:
+ maximum_tags:
icon: Info
text: A post can only have four tags.
add_tag:
header: Add new tag
button_text: Add new tag
icon: Add tag
+ cannot_remove_admin_adjusted_tag_html: Tag has been added by an Admin. You can't remove it now. If you feel that's a mistake, please contact us at %{contact_email}.
+ cannot_add_admin_adjusted_tag_html: Tag has been removed by an Admin. You can't add it now. If you feel that's a mistake, please contact us at %{contact_email}.
approve:
add: Mark as approved
remove: Remove approval
diff --git a/config/locales/views/moderations/fr.yml b/config/locales/views/moderations/fr.yml
index 90a957735..cfcf42d26 100644
--- a/config/locales/views/moderations/fr.yml
+++ b/config/locales/views/moderations/fr.yml
@@ -23,7 +23,7 @@ fr:
history:
added_by: ajouté par
removed_by: supprimé par
- reason:
+ reason:
remove_tag: Raison de supprimer la balise
add_tag: Raison d'ajouter la balise (optionnel)
remove: Retirer l'étiquette
@@ -36,6 +36,8 @@ fr:
header: Ajouter une nouvelle balise
button_text: Ajouter une nouvelle balise
icon: Ajouter une étiquette
+ cannot_add_admin_adjusted_tag_html: La balise aurait ajoutée par l'Admin. Vous pouvez pas la supprimer maintenant. Si vous pensez que c'est une erreur, veuillez nous contacter à %{support_email}.
+ cannot_remove_admin_adjusted_tag_html: La balise aurait supprimée par l'Admin. Vous pouvez pas l'ajouter maintenant. Si vous pensez que c'est une erreur, veuillez nous contacter à %{support_email}.
approve:
add: Marquer comme approuvé
remove: Retirer l'approbation
diff --git a/db/migrate/20230910135738_remove_index_tag_adjustments_on_tag_name_and_article_id.rb b/db/migrate/20230910135738_remove_index_tag_adjustments_on_tag_name_and_article_id.rb
new file mode 100644
index 000000000..cc5e20dac
--- /dev/null
+++ b/db/migrate/20230910135738_remove_index_tag_adjustments_on_tag_name_and_article_id.rb
@@ -0,0 +1,19 @@
+class RemoveIndexTagAdjustmentsOnTagNameAndArticleId < ActiveRecord::Migration[7.0]
+ disable_ddl_transaction!
+
+ def up
+ return unless index_exists?(:tag_adjustments, %i[tag_name article_id])
+
+ remove_index :tag_adjustments,
+ column: %i[tag_name article_id],
+ unique: true,
+ algorithm: :concurrently,
+ name: "index_tag_adjustments_on_tag_name_and_article_id"
+ end
+
+ def down
+ return if index_exists?(:tag_adjustments, %i[tag_name article_id])
+
+ add_index :tag_adjustments, %i[tag_name article_id], unique: true, algorithm: :concurrently
+ end
+end
diff --git a/db/schema.rb b/db/schema.rb
index 1a1fdfdbf..877e91f0a 100644
--- a/db/schema.rb
+++ b/db/schema.rb
@@ -10,7 +10,7 @@
#
# It's strongly recommended that you check this file into your version control system.
-ActiveRecord::Schema[7.0].define(version: 2023_09_06_112602) do
+ActiveRecord::Schema[7.0].define(version: 2023_09_10_135738) do
# These are extensions that must be enabled in order to support this database
enable_extension "citext"
enable_extension "ltree"
@@ -1092,7 +1092,6 @@ ActiveRecord::Schema[7.0].define(version: 2023_09_06_112602) do
t.string "tag_name"
t.datetime "updated_at", precision: nil, null: false
t.bigint "user_id"
- t.index ["tag_name", "article_id"], name: "index_tag_adjustments_on_tag_name_and_article_id", unique: true
end
create_table "taggings", force: :cascade do |t|
diff --git a/spec/helpers/moderations/actions_panel_helper_spec.rb b/spec/helpers/moderations/actions_panel_helper_spec.rb
new file mode 100644
index 000000000..7bb40bc23
--- /dev/null
+++ b/spec/helpers/moderations/actions_panel_helper_spec.rb
@@ -0,0 +1,26 @@
+require "rails_helper"
+
+describe Moderations::ActionsPanelHelper do
+ describe "#last_adjusted_by_admin?" do
+ let(:tag1) { create(:tag) }
+ let(:tag2) { create(:tag, name: "tag2") }
+
+ let(:admin) { create(:user, :admin) }
+ let(:user) { create(:user) }
+
+ let(:article) { create(:article, tags: "tag2") }
+
+ it "returns false if the last adjustment was made by a non-admin" do
+ user.add_role(:tag_moderator, tag2)
+ create(:tag_adjustment, article: article, tag_id: tag2.id, tag_name: tag2.name, user: user,
+ status: "committed", adjustment_type: "removal")
+ expect(helper.last_adjusted_by_admin?(article, tag2, "removal")).to be(false)
+ end
+
+ it "returns true if the last adjustment was made by any admin" do
+ create(:tag_adjustment, article: article, tag_id: tag1.id, tag_name: tag1.name, user: admin,
+ status: "committed", adjustment_type: "addition")
+ expect(helper.last_adjusted_by_admin?(article, tag1, "addition")).to be(true)
+ end
+ end
+end
diff --git a/spec/models/article_spec.rb b/spec/models/article_spec.rb
index f22a71b6f..7feb516b2 100644
--- a/spec/models/article_spec.rb
+++ b/spec/models/article_spec.rb
@@ -107,6 +107,46 @@ RSpec.describe Article do
end
end
+ describe "#validate_tag" do
+ # rubocop:disable RSpec/VerifiedDoubles
+ it "does not modify the tag list if there are no adjustments" do
+ # See https://github.com/forem/forem/pull/6302
+ article = build(:article, user: user)
+ allow(TagAdjustment).to receive(:where).and_return(TagAdjustment.none)
+ allow(article).to receive(:tag_list).and_return(spy("tag_list"))
+
+ article.save
+
+ # We expect this to happen once in #evaluate_front_matter
+ expect(article.tag_list).to have_received(:add).once
+ expect(article.tag_list).not_to have_received(:remove)
+ end
+ # rubocop:enable RSpec/VerifiedDoubles
+
+ it "adjusts the tags in the tag_list based on the tag_adjustments" do
+ user = create(:user, :admin)
+ tag1 = create(:tag, name: "tag1")
+ tag2 = create(:tag, name: "tag2")
+
+ # try save an article with a tag_list of tag 1, tag 3
+ # in the tag adjustments we have a removal of tag1 and an addition of tag2
+ # hence the tag_list should be tag2, tag3
+ article = build(:article, user: user, tags: "#{tag1.name}, tag3")
+
+ create(:tag_adjustment, adjustment_type: "addition", tag_id: tag2.id,
+ tag_name: tag2.name, article: article, user: user)
+
+ create(:tag_adjustment, adjustment_type: "removal", tag_id: tag1.id,
+ tag_name: tag1.name, article: article, user: user)
+
+ article.save
+
+ expect(article.tag_list).to include("tag3")
+ expect(article.tag_list).to include("tag2")
+ expect(article.tag_list).not_to include("tag1")
+ end
+ end
+
describe "#validate co_authors" do
it "is invalid if the co_author is the same as the author" do
article.co_author_ids = [user.id]
@@ -275,24 +315,6 @@ RSpec.describe Article do
expect(test_article.errors_as_sentence).to match("Title can't be blank")
end
end
-
- describe "tag validation" do
- let(:article) { build(:article, user: user) }
-
- # See https://github.com/forem/forem/pull/6302
- # rubocop:disable RSpec/VerifiedDoubles
- it "does not modify the tag list if there are no adjustments" do
- allow(TagAdjustment).to receive(:where).and_return(TagAdjustment.none)
- allow(article).to receive(:tag_list).and_return(spy("tag_list"))
-
- article.save
-
- # We expect this to happen once in #evaluate_front_matter
- expect(article.tag_list).to have_received(:add).once
- expect(article.tag_list).not_to have_received(:remove)
- end
- # rubocop:enable RSpec/VerifiedDoubles
- end
end
context "when data is extracted from evaluation of the front matter during validation" do
diff --git a/spec/models/tag_adjustment_spec.rb b/spec/models/tag_adjustment_spec.rb
index d0308e78b..fa0e174bd 100644
--- a/spec/models/tag_adjustment_spec.rb
+++ b/spec/models/tag_adjustment_spec.rb
@@ -3,13 +3,16 @@ require "rails_helper"
RSpec.describe TagAdjustment do
before do
mod_user.add_role(:tag_moderator, tag)
- admin_user.add_role(:admin)
+ other_mod.add_role(:tag_moderator, tag)
end
let(:article) { create(:article, tags: nil) }
let(:tag) { create(:tag) }
- let(:admin_user) { create(:user) }
+ let(:admin_user) { create(:user, :admin) }
+ let(:other_admin) { create(:user, :admin) }
+ let(:super_mod) { create(:user, :super_moderator) }
let(:mod_user) { create(:user) }
+ let(:other_mod) { create(:user) }
let(:regular_user) { create(:user) }
it { is_expected.to validate_presence_of(:tag_name) }
@@ -23,20 +26,37 @@ RSpec.describe TagAdjustment do
expect(tag_adjustment).to be_valid
end
- it "does not allow tag mods to create for other tags" do
- another_tag = create(:tag)
- tag_adjustment = build(:tag_adjustment, user_id: mod_user.id, article_id: article.id, tag_id: another_tag.id)
- expect(tag_adjustment).to be_invalid
+ it "allows tag mods to create a tag adjustment for a tag that has been adjusted by another tag mod" do
+ create(:tag_adjustment, user_id: other_mod.id, article_id: article.id, tag_id: tag.id)
+
+ tag_adjustment = build(:tag_adjustment, user_id: mod_user.id, article_id: article.id, tag_id: tag.id)
+ expect(tag_adjustment).to be_valid
end
- it "allows admins to create for any tags" do
+ it "does not allow tag mods to create a tag adjustment for other tags" do
+ another_tag = create(:tag)
+ tag_adjustment = build(:tag_adjustment, user_id: mod_user.id, article_id: article.id, tag_id: another_tag.id)
+ expect(tag_adjustment).not_to be_valid
+ end
+
+ it "allows admins to create a tag adjustment for any tags" do
tag_adjustment = build(:tag_adjustment, user_id: admin_user.id, article_id: article.id, tag_id: tag.id)
expect(tag_adjustment).to be_valid
end
- it "does not allow normal users to create for any tags" do
+ it "allows admins and super moderators to create a tag adjustment for a tag that was adjusted by another admin" do
+ create(:tag_adjustment, user_id: admin_user.id, article_id: article.id, tag_id: tag.id)
+
+ tag_adjustment = build(:tag_adjustment, user_id: other_admin.id, article_id: article.id, tag_id: tag.id)
+ expect(tag_adjustment).to be_valid
+
+ tag_adjustment = build(:tag_adjustment, user_id: super_mod.id, article_id: article.id, tag_id: tag.id)
+ expect(tag_adjustment).to be_valid
+ end
+
+ it "does not allow normal users to create a tag adjustment for any tags" do
tag_adjustment = build(:tag_adjustment, user_id: regular_user.id, article_id: article.id, tag_id: tag.id)
- expect(tag_adjustment).to be_invalid
+ expect(tag_adjustment).not_to be_valid
end
end
@@ -56,7 +76,7 @@ RSpec.describe TagAdjustment do
it "disallows improper adjustment_types" do
tag_adjustment = build(:tag_adjustment, user_id: mod_user.id, article_id: article.id, tag_id: tag.id,
adjustment_type: "slushie")
- expect(tag_adjustment).to be_invalid
+ expect(tag_adjustment).not_to be_valid
end
it "allows proper status" do
@@ -68,7 +88,7 @@ RSpec.describe TagAdjustment do
it "disallows improper status" do
tag_adjustment = build(:tag_adjustment, user_id: mod_user.id, article_id: article.id, tag_id: tag.id,
status: "slushiemonkey")
- expect(tag_adjustment).to be_invalid
+ expect(tag_adjustment).not_to be_valid
end
end
@@ -77,14 +97,14 @@ RSpec.describe TagAdjustment do
article_tags_maxed = create(:article, tags: "ruby, javascript, html, css")
tag_adjustment = build(:tag_adjustment, user_id: admin_user.id, article_id: article_tags_maxed.id,
tag_id: tag.id, tag_name: tag.name)
- expect(tag_adjustment).to be_invalid
+ expect(tag_adjustment).not_to be_valid
end
it "does not create if removed tag not on tag_list" do
article = create(:article, tags: tag.name)
tag_adjustment = build(:tag_adjustment, user_id: admin_user.id, article_id: article.id,
adjustment_type: "removal")
- expect(tag_adjustment).to be_invalid
+ expect(tag_adjustment).not_to be_valid
end
it "ignores case when checking tag_list" do