FIX: serialize Flags instead of PostActionType (#28259)

### Why?
Before, all flags were static. Therefore, they were stored in class variables and serialized by SiteSerializer. Recently, we added an option for admins to add their own flags or disable existing flags. Therefore, the class variable had to be dropped because it was unsafe for a multisite environment. However, it started causing performance problems. 

### Solution
When a new Flag system is used, instead of using PostActionType, we can serialize Flags and use fragment cache for performance reasons. 

At the same time, we are still supporting deprecated `replace_flags` API call. When it is used, we fall back to the old solution and the admin cannot add custom flags. In a couple of months, we will be able to drop that API function and clean that code properly. However, because it may still be used, redis cache was introduced to improve performance.

To test backward compatibility you can add this code to any plugin
```ruby
  replace_flags do |flag_settings|
    flag_settings.add(
      4,
      :inappropriate,
      topic_type: true,
      notify_type: true,
      auto_action_type: true,
    )
    flag_settings.add(1001, :trolling, topic_type: true, notify_type: true, auto_action_type: true)
  end
```
This commit is contained in:
Krzysztof Kotlarek
2024-08-13 11:22:37 +10:00
committed by GitHub
parent c4c3520ff5
commit 094052c1ff
28 changed files with 602 additions and 238 deletions

View File

@@ -0,0 +1,77 @@
# frozen_string_literal: true
RSpec.describe PostActionTypeView do
let(:post_action_type_view) { PostActionTypeView.new }
it "returns correct types" do
expect(post_action_type_view.flag_types).to eq(
{
illegal: 10,
inappropriate: 4,
notify_moderators: 7,
notify_user: 6,
off_topic: 3,
spam: 8,
},
)
expect(post_action_type_view.public_types).to eq({ like: 2 })
expect(post_action_type_view.notify_flag_types).to eq(
{ illegal: 10, inappropriate: 4, notify_moderators: 7, off_topic: 3, spam: 8 },
)
expect(post_action_type_view.topic_flag_types).to eq(
{ illegal: 10, inappropriate: 4, notify_moderators: 7, spam: 8 },
)
expect(post_action_type_view.additional_message_types).to eq(
{ illegal: 10, notify_moderators: 7, notify_user: 6 },
)
expect(post_action_type_view.score_types).to eq({ needs_approval: 9 })
flag = Fabricate(:flag, name: "custom", enabled: false)
expect(PostActionTypeView.new.disabled_flag_types).to eq({ custom: flag.id })
flag.destroy!
end
it "defines names of flags" do
expect(post_action_type_view.names).to eq(
{
6 => "notify_user",
3 => "off_topic",
4 => "inappropriate",
8 => "spam",
10 => "illegal",
7 => "notify_moderators",
9 => "needs_approval",
2 => "like",
},
)
end
it "defines descriptions of flags" do
flag = Fabricate(:flag, enabled: false, description: "custom flag description")
expect(post_action_type_view.descriptions[flag.id]).to eq("custom flag description")
flag.destroy!
end
it "defines where flags can be applies to" do
expect(post_action_type_view.applies_to).to eq(
{
6 => %w[Post Chat::Message],
3 => %w[Post Chat::Message],
4 => %w[Post Topic Chat::Message],
8 => %w[Post Topic Chat::Message],
10 => %w[Post Topic Chat::Message],
7 => %w[Post Topic Chat::Message],
9 => [],
2 => ["Post"],
},
)
end
it "defines is post action type is a flag" do
expect(post_action_type_view.is_flag?(:like)).to be false
expect(post_action_type_view.is_flag?(:off_topic)).to be true
end
end

View File

@@ -70,4 +70,30 @@ RSpec.describe Flag, type: :model do
%i[notify_user off_topic inappropriate spam illegal notify_moderators needs_approval],
)
end
describe ".used_flag_ids" do
fab!(:post_action) { Fabricate(:post_action, post_action_type_id: PostActionType.types[:like]) }
fab!(:post_action_2) do
Fabricate(:post_action, post_action_type_id: PostActionType.types[:like])
end
fab!(:post_action_3) do
Fabricate(:post_action, post_action_type_id: PostActionType.types[:off_topic])
end
fab!(:reviewable_score) do
Fabricate(:reviewable_score, reviewable_score_type: PostActionType.types[:off_topic])
end
fab!(:reviewable_score_2) do
Fabricate(:reviewable_score, reviewable_score_type: PostActionType.types[:illegal])
end
it "returns an array of unique flag ids" do
expect(Flag.used_flag_ids).to eq(
[
PostActionType.types[:like],
PostActionType.types[:off_topic],
PostActionType.types[:illegal],
],
)
end
end
end

View File

@@ -4,17 +4,15 @@ RSpec.describe PostActionType do
describe "Callbacks" do
describe "#expiry_cache" do
it "should clear the cache on save" do
cache = ApplicationSerializer.fragment_cache
cache["post_action_types_#{I18n.locale}"] = "test"
cache["post_action_flag_types_#{I18n.locale}"] = "test2"
Discourse.cache.write("post_action_types_#{I18n.locale}", "test")
Discourse.cache.write("post_action_flag_types_#{I18n.locale}", "test2")
PostActionType.new(name_key: "some_key").save!
expect(cache["post_action_types_#{I18n.locale}"]).to eq(nil)
expect(cache["post_action_flag_types_#{I18n.locale}"]).to eq(nil)
expect(Discourse.cache.read("post_action_types_#{I18n.locale}")).to eq(nil)
expect(Discourse.cache.read("post_action_flag_types_#{I18n.locale}")).to eq(nil)
ensure
ApplicationSerializer.fragment_cache.clear
PostActionType.new.expire_cache
end
end
end
@@ -30,7 +28,9 @@ RSpec.describe PostActionType do
end
describe ".additional_message_types" do
before { described_class.stubs(:overridden_by_plugin_or_skipped_db?).returns(overriden) }
before do
PostActionTypeView.any_instance.stubs(:overridden_by_plugin_or_skipped_db?).returns(overriden)
end
context "when overridden by plugin or skipped DB" do
let(:overriden) { true }

View File

@@ -0,0 +1,24 @@
# frozen_string_literal: true
RSpec.describe "Custom flags in multisite", type: :multisite do
describe "PostACtionType#all_flags" do
use_redis_snapshotting
it "does not share flag definitions between sites" do
flag_1 = Flag.create!(name: "test flag 1", position: 99, applies_to: ["Post"])
test_multisite_connection("second") do
flag_2 = Flag.create!(name: "test flag 2", position: 99, applies_to: ["Post"])
PostActionType.new.expire_cache
expect(PostActionType.all_flags.last).to eq(
flag_2.attributes.except("created_at", "updated_at").transform_keys(&:to_sym),
)
end
PostActionType.new.expire_cache
expect(PostActionType.all_flags.last).to eq(
flag_1.attributes.except("created_at", "updated_at").transform_keys(&:to_sym),
)
end
end
end

View File

@@ -362,6 +362,9 @@
},
"is_used": {
"type": "boolean"
},
"position": {
"type": "integer"
}
},
"required": [
@@ -414,6 +417,9 @@
},
"is_used": {
"type": "boolean"
},
"position": {
"type": "integer"
}
},
"required": [

View File

@@ -0,0 +1,57 @@
# frozen_string_literal: true
RSpec.describe FlagSerializer do
let(:flag) { Flag.find_by(name: "illegal") }
context "when system flag" do
it "returns translated name" do
serialized = described_class.new(flag, used_flag_ids: []).as_json
expect(serialized[:flag][:name]).to eq(I18n.t("post_action_types.illegal.title"))
end
it "returns translated description" do
serialized = described_class.new(flag, used_flag_ids: []).as_json
expect(serialized[:flag][:description]).to eq(I18n.t("post_action_types.illegal.description"))
end
end
context "when custom flag" do
fab!(:flag) { Fabricate(:flag, name: "custom title", description: "custom description") }
it "returns translated name" do
serialized = described_class.new(flag, used_flag_ids: []).as_json
expect(serialized[:flag][:name]).to eq("custom title")
end
it "returns translated description" do
serialized = described_class.new(flag, used_flag_ids: []).as_json
expect(serialized[:flag][:description]).to eq("custom description")
end
end
it "returns is_flag true for flags" do
serialized = described_class.new(flag, used_flag_ids: []).as_json
expect(serialized[:flag][:is_flag]).to be true
end
it "returns is_flag false for like" do
flag = Flag.unscoped.find_by(name: "like")
serialized = described_class.new(flag, used_flag_ids: []).as_json
expect(serialized[:flag][:is_flag]).to be false
end
it "returns is_used false when not used" do
serialized = described_class.new(flag, used_flag_ids: []).as_json
expect(serialized[:flag][:is_used]).to be false
end
it "returns is_used true when used" do
serialized = described_class.new(flag, used_flag_ids: [flag.id]).as_json
expect(serialized[:flag][:is_used]).to be true
end
it "returns applies_to" do
serialized = described_class.new(flag, used_flag_ids: []).as_json
expect(serialized[:flag][:applies_to]).to eq(%w[Post Topic Chat::Message])
end
end