mirror of
https://ghfast.top/https://github.com/discourse/discourse-category-experts.git
synced 2026-07-15 11:36:31 +08:00
f7a651b512
## Summary The "Needs approval" pill appears on topics in the topic list for staff users even when the `category_experts_posts_require_approval` site setting is **disabled**. The expected behavior is that only the "Expert response" pill should appear. ## Repro Config | Setting | Value | |---------|-------| | `enable_category_experts` | true | | `first_post_can_be_considered_expert_post` | false | | `show_category_expert_advanced_search_filters` | true | | `category_expert_suggestion_threshold` | 3 | | `category_experts_posts_require_approval` | **false** | | `send_admin_category_experts_posts_reminder_pm` | false | | `approve_past_posts_on_becoming_category_expert` | true | | `category_experts_topic_list_link_to_posts` | true | | `max_category_expert_endorsements_per_day` | 50 | 1. Create an expert group (e.g. `category_experts`) 2. Create two categories: - "Expert Category" - with the expert group assigned as category experts - "Other Category" - no expert group assigned 3. Create an expert user and add them to the expert group 4. Create a regular user 5. As the regular user, create two topics in "Expert Category": - "Help I have another question!" - "Hi can someone help me with my request?" 6. As the expert user, reply to both topics At this point, both topics show the "Expert response" pill in the topic list. ### Trigger the bug 7. As an admin, move "Help I have another question!" from "Expert Category" to "Other Category" 8. Move it back to "Expert Category" ### Result - "Help I have another question!" now shows the **"Needs approval"** pill (BUG) - "Hi can someone help me with my request?" shows the **"Expert response"** pill (correct) ## Root cause `correct_topic_custom_fields_after_removal` in `lib/category_experts/post_handler.rb` unconditionally sets `TOPIC_NEEDS_EXPERT_POST_APPROVAL` when there are no remaining approved expert posts on a topic, without checking whether `category_experts_posts_require_approval` is enabled. This method is called from code paths that are NOT gated by the setting: - `handle_topic_category_change` - when a topic is moved to a category where the poster is no longer an expert - `on(:post_moved)` - when an expert post is moved out of a topic Additionally, the topic list serializer sends `needs_category_expert_post_approval` to the frontend without checking the setting, so any stale data on topics is displayed. ## Fix 1. In `correct_topic_custom_fields_after_removal`: only set `TOPIC_NEEDS_EXPERT_POST_APPROVAL` when `SiteSetting.category_experts_posts_require_approval` is true 2. In the serializer `include_condition`: add a check for `SiteSetting.category_experts_posts_require_approval`
482 lines
18 KiB
Ruby
482 lines
18 KiB
Ruby
# frozen_string_literal: true
|
|
|
|
RSpec.describe CategoryExperts do
|
|
fab!(:admin)
|
|
fab!(:expert) { Fabricate(:user, refresh_auto_groups: true) }
|
|
|
|
fab!(:category)
|
|
fab!(:group) { Fabricate(:group, users: [expert]) }
|
|
fab!(:auto_tag, :tag)
|
|
|
|
fab!(:original_topic) { Fabricate(:topic, category: category) }
|
|
fab!(:first_post) { Fabricate(:post, topic: original_topic, user: expert) }
|
|
fab!(:second_post) { Fabricate(:post, topic: original_topic, user: expert) }
|
|
|
|
fab!(:destination_topic) { Fabricate(:topic, category: category) }
|
|
fab!(:destination_op) { Fabricate(:post, topic: destination_topic) }
|
|
|
|
before do
|
|
SiteSetting.enable_category_experts = true
|
|
category.custom_fields[CategoryExperts::CATEGORY_EXPERT_GROUP_IDS] = "#{group.id}"
|
|
category.custom_fields[CategoryExperts::CATEGORY_EXPERT_AUTO_TAG] = auto_tag.name
|
|
category.save!
|
|
|
|
CategoryExperts::PostHandler.new(post: second_post, user: expert).mark_post_as_approved
|
|
end
|
|
|
|
describe "Events" do
|
|
describe "on 'post_moved'" do
|
|
describe "Moving post to a topic without existing category expert post" do
|
|
it "moves topic custom fields to new topic" do
|
|
expect(
|
|
original_topic.custom_fields[CategoryExperts::TOPIC_EXPERT_POST_GROUP_NAMES],
|
|
).to eq(group.name)
|
|
expect(original_topic.custom_fields[CategoryExperts::TOPIC_FIRST_EXPERT_POST_ID]).to eq(
|
|
second_post.post_number,
|
|
)
|
|
|
|
expect(
|
|
destination_topic.custom_fields[CategoryExperts::TOPIC_EXPERT_POST_GROUP_NAMES],
|
|
).to be_blank
|
|
expect(
|
|
destination_topic.custom_fields[CategoryExperts::TOPIC_FIRST_EXPERT_POST_ID],
|
|
).to be_blank
|
|
|
|
original_topic.move_posts(
|
|
admin,
|
|
[second_post.id],
|
|
destination_topic_id: destination_topic.id,
|
|
)
|
|
|
|
original_topic.reload
|
|
destination_topic.reload
|
|
|
|
expect(
|
|
original_topic.custom_fields[CategoryExperts::TOPIC_EXPERT_POST_GROUP_NAMES],
|
|
).to be_blank
|
|
expect(
|
|
original_topic.custom_fields[CategoryExperts::TOPIC_FIRST_EXPERT_POST_ID],
|
|
).to be_blank
|
|
|
|
expect(
|
|
destination_topic.custom_fields[CategoryExperts::TOPIC_EXPERT_POST_GROUP_NAMES],
|
|
).to eq(group.name)
|
|
expect(
|
|
destination_topic.custom_fields[CategoryExperts::TOPIC_FIRST_EXPERT_POST_ID],
|
|
).to eq(second_post.post_number)
|
|
end
|
|
|
|
context "with freeze_original for post move" do
|
|
it "keeps existing custom fields on original topic and adds to new topic" do
|
|
expect(
|
|
original_topic.custom_fields[CategoryExperts::TOPIC_EXPERT_POST_GROUP_NAMES],
|
|
).to eq(group.name)
|
|
expect(original_topic.custom_fields[CategoryExperts::TOPIC_FIRST_EXPERT_POST_ID]).to eq(
|
|
second_post.post_number,
|
|
)
|
|
|
|
expect(
|
|
destination_topic.custom_fields[CategoryExperts::TOPIC_EXPERT_POST_GROUP_NAMES],
|
|
).to be_blank
|
|
expect(
|
|
destination_topic.custom_fields[CategoryExperts::TOPIC_FIRST_EXPERT_POST_ID],
|
|
).to be_blank
|
|
|
|
original_topic.move_posts(
|
|
admin,
|
|
[second_post.id],
|
|
destination_topic_id: destination_topic.id,
|
|
freeze_original: true,
|
|
)
|
|
|
|
original_topic.reload
|
|
destination_topic.reload
|
|
|
|
expect(
|
|
original_topic.custom_fields[CategoryExperts::TOPIC_EXPERT_POST_GROUP_NAMES],
|
|
).to eq(group.name)
|
|
expect(original_topic.custom_fields[CategoryExperts::TOPIC_FIRST_EXPERT_POST_ID]).to eq(
|
|
second_post.post_number,
|
|
)
|
|
|
|
expect(
|
|
destination_topic.custom_fields[CategoryExperts::TOPIC_EXPERT_POST_GROUP_NAMES],
|
|
).to eq(group.name)
|
|
expect(
|
|
destination_topic.custom_fields[CategoryExperts::TOPIC_FIRST_EXPERT_POST_ID],
|
|
).to eq(second_post.post_number)
|
|
end
|
|
end
|
|
end
|
|
|
|
context "when topics have existing category experts posts" do
|
|
fab!(:other_group_expert) { Fabricate(:user, refresh_auto_groups: true) }
|
|
fab!(:other_group) { Fabricate(:group, users: [other_group_expert]) }
|
|
|
|
fab!(:original_topic_expert_post) do
|
|
Fabricate(:post, topic: original_topic, user: other_group_expert)
|
|
end
|
|
fab!(:destination_topic_expert_post) do
|
|
Fabricate(:post, topic: destination_topic, user: other_group_expert)
|
|
end
|
|
|
|
before do
|
|
category.custom_fields[
|
|
CategoryExperts::CATEGORY_EXPERT_GROUP_IDS
|
|
] = "#{group.id}|#{other_group.id}"
|
|
category.save!
|
|
|
|
CategoryExperts::PostHandler.new(
|
|
post: original_topic_expert_post,
|
|
user: other_group_expert,
|
|
).mark_post_as_approved
|
|
CategoryExperts::PostHandler.new(
|
|
post: destination_topic_expert_post,
|
|
user: other_group_expert,
|
|
).mark_post_as_approved
|
|
end
|
|
|
|
it "Correctly adds and removes from topic custom fields without overriding existing fields" do
|
|
expect(
|
|
original_topic.custom_fields[CategoryExperts::TOPIC_EXPERT_POST_GROUP_NAMES],
|
|
).to eq("#{group.name}|#{other_group.name}")
|
|
expect(original_topic.custom_fields[CategoryExperts::TOPIC_FIRST_EXPERT_POST_ID]).to eq(
|
|
second_post.post_number,
|
|
)
|
|
|
|
expect(
|
|
destination_topic.custom_fields[CategoryExperts::TOPIC_EXPERT_POST_GROUP_NAMES],
|
|
).to eq(other_group.name)
|
|
expect(
|
|
destination_topic.custom_fields[CategoryExperts::TOPIC_FIRST_EXPERT_POST_ID],
|
|
).to eq(destination_topic_expert_post.post_number)
|
|
|
|
original_topic.move_posts(
|
|
admin,
|
|
[second_post.id],
|
|
destination_topic_id: destination_topic.id,
|
|
)
|
|
|
|
original_topic.reload
|
|
destination_topic.reload
|
|
|
|
expect(
|
|
original_topic.custom_fields[CategoryExperts::TOPIC_EXPERT_POST_GROUP_NAMES],
|
|
).to eq(other_group.name)
|
|
|
|
expect(
|
|
destination_topic.custom_fields[CategoryExperts::TOPIC_EXPERT_POST_GROUP_NAMES],
|
|
).to eq("#{other_group.name}|#{group.name}")
|
|
expect(
|
|
destination_topic.custom_fields[CategoryExperts::TOPIC_FIRST_EXPERT_POST_ID],
|
|
).to eq(destination_topic_expert_post.post_number)
|
|
end
|
|
|
|
context "with freeze_original for post move" do
|
|
it "Correctly adds and removes from topic custom fields without overriding existing fields" do
|
|
expect(
|
|
original_topic.custom_fields[CategoryExperts::TOPIC_EXPERT_POST_GROUP_NAMES],
|
|
).to eq("#{group.name}|#{other_group.name}")
|
|
expect(original_topic.custom_fields[CategoryExperts::TOPIC_FIRST_EXPERT_POST_ID]).to eq(
|
|
second_post.post_number,
|
|
)
|
|
|
|
expect(
|
|
destination_topic.custom_fields[CategoryExperts::TOPIC_EXPERT_POST_GROUP_NAMES],
|
|
).to eq(other_group.name)
|
|
expect(
|
|
destination_topic.custom_fields[CategoryExperts::TOPIC_FIRST_EXPERT_POST_ID],
|
|
).to eq(destination_topic_expert_post.post_number)
|
|
|
|
original_topic.move_posts(
|
|
admin,
|
|
[second_post.id],
|
|
destination_topic_id: destination_topic.id,
|
|
freeze_original: true,
|
|
)
|
|
|
|
original_topic.reload
|
|
destination_topic.reload
|
|
|
|
expect(
|
|
original_topic.custom_fields[CategoryExperts::TOPIC_EXPERT_POST_GROUP_NAMES],
|
|
).to eq("#{group.name}|#{other_group.name}")
|
|
expect(original_topic.custom_fields[CategoryExperts::TOPIC_FIRST_EXPERT_POST_ID]).to eq(
|
|
second_post.post_number,
|
|
)
|
|
|
|
expect(
|
|
destination_topic.custom_fields[CategoryExperts::TOPIC_EXPERT_POST_GROUP_NAMES],
|
|
).to eq("#{other_group.name}|#{group.name}")
|
|
expect(
|
|
destination_topic.custom_fields[CategoryExperts::TOPIC_FIRST_EXPERT_POST_ID],
|
|
).to eq(destination_topic_expert_post.post_number)
|
|
end
|
|
end
|
|
end
|
|
end
|
|
|
|
describe "on 'post_edited' with category change" do
|
|
fab!(:category_b, :category)
|
|
fab!(:group_b, :group)
|
|
fab!(:auto_tag_b, :tag)
|
|
|
|
before do
|
|
SiteSetting.tagging_enabled = true
|
|
category_b.custom_fields[CategoryExperts::CATEGORY_EXPERT_GROUP_IDS] = "#{group_b.id}"
|
|
category_b.custom_fields[CategoryExperts::CATEGORY_EXPERT_AUTO_TAG] = auto_tag_b.name
|
|
category_b.save!
|
|
end
|
|
|
|
context "when topic is moved from category with experts to category without experts" do
|
|
it "clears all expert custom fields from posts and topic" do
|
|
expect(
|
|
original_topic.custom_fields[CategoryExperts::TOPIC_EXPERT_POST_GROUP_NAMES],
|
|
).to eq(group.name)
|
|
expect(original_topic.custom_fields[CategoryExperts::TOPIC_FIRST_EXPERT_POST_ID]).to eq(
|
|
second_post.post_number,
|
|
)
|
|
expect(second_post.custom_fields[CategoryExperts::POST_APPROVED_GROUP_NAME]).to eq(
|
|
group.name,
|
|
)
|
|
|
|
category_without_experts = Fabricate(:category)
|
|
|
|
PostRevisor.new(original_topic.first_post).revise!(
|
|
admin,
|
|
category_id: category_without_experts.id,
|
|
)
|
|
|
|
original_topic.reload
|
|
second_post.reload
|
|
|
|
expect(
|
|
original_topic.custom_fields[CategoryExperts::TOPIC_EXPERT_POST_GROUP_NAMES],
|
|
).to be_blank
|
|
expect(
|
|
original_topic.custom_fields[CategoryExperts::TOPIC_FIRST_EXPERT_POST_ID],
|
|
).to be_blank
|
|
expect(second_post.custom_fields[CategoryExperts::POST_APPROVED_GROUP_NAME]).to be_blank
|
|
end
|
|
|
|
it "does not set needs_approval when category_experts_posts_require_approval is disabled" do
|
|
SiteSetting.category_experts_posts_require_approval = false
|
|
|
|
category_without_experts = Fabricate(:category)
|
|
|
|
PostRevisor.new(original_topic.first_post).revise!(
|
|
admin,
|
|
category_id: category_without_experts.id,
|
|
)
|
|
|
|
original_topic.reload
|
|
|
|
expect(
|
|
original_topic.custom_fields[CategoryExperts::TOPIC_NEEDS_EXPERT_POST_APPROVAL],
|
|
).to be_blank
|
|
end
|
|
|
|
it "sets needs_approval when category_experts_posts_require_approval is enabled" do
|
|
SiteSetting.category_experts_posts_require_approval = true
|
|
|
|
category_without_experts = Fabricate(:category)
|
|
|
|
PostRevisor.new(original_topic.first_post).revise!(
|
|
admin,
|
|
category_id: category_without_experts.id,
|
|
)
|
|
|
|
original_topic.reload
|
|
|
|
expect(
|
|
original_topic.custom_fields[CategoryExperts::TOPIC_NEEDS_EXPERT_POST_APPROVAL],
|
|
).to eq(second_post.post_number)
|
|
end
|
|
|
|
it "removes old auto-tag and do not add new one" do
|
|
expect(original_topic.tags.map(&:name)).to include(auto_tag.name)
|
|
expect(original_topic.tags.map(&:name)).not_to include(auto_tag_b.name)
|
|
|
|
category_without_experts = Fabricate(:category)
|
|
category_without_experts.custom_fields[
|
|
CategoryExperts::CATEGORY_EXPERT_AUTO_TAG
|
|
] = auto_tag_b.name
|
|
category_without_experts.save!
|
|
|
|
PostRevisor.new(original_topic.first_post).revise!(
|
|
admin,
|
|
category_id: category_without_experts.id,
|
|
)
|
|
|
|
original_topic.reload
|
|
|
|
expect(original_topic.tags.map(&:name)).not_to include(auto_tag.name)
|
|
expect(original_topic.tags.map(&:name)).not_to include(auto_tag_b.name)
|
|
end
|
|
end
|
|
|
|
context "when topic is moved from one category with experts to another with different experts" do
|
|
it "updates expert custom fields appropriately when user is not expert in new category" do
|
|
expect(
|
|
original_topic.custom_fields[CategoryExperts::TOPIC_EXPERT_POST_GROUP_NAMES],
|
|
).to eq(group.name)
|
|
expect(second_post.custom_fields[CategoryExperts::POST_APPROVED_GROUP_NAME]).to eq(
|
|
group.name,
|
|
)
|
|
|
|
PostRevisor.new(original_topic.first_post).revise!(admin, category_id: category_b.id)
|
|
|
|
original_topic.reload
|
|
second_post.reload
|
|
|
|
expect(
|
|
original_topic.custom_fields[CategoryExperts::TOPIC_EXPERT_POST_GROUP_NAMES],
|
|
).to be_blank
|
|
expect(second_post.custom_fields[CategoryExperts::POST_APPROVED_GROUP_NAME]).to be_blank
|
|
end
|
|
|
|
it "updates expert group name when user is expert in both categories" do
|
|
group_b.add(expert)
|
|
|
|
expect(
|
|
original_topic.custom_fields[CategoryExperts::TOPIC_EXPERT_POST_GROUP_NAMES],
|
|
).to eq(group.name)
|
|
expect(second_post.custom_fields[CategoryExperts::POST_APPROVED_GROUP_NAME]).to eq(
|
|
group.name,
|
|
)
|
|
|
|
PostRevisor.new(original_topic.first_post).revise!(admin, category_id: category_b.id)
|
|
|
|
original_topic.reload
|
|
second_post.reload
|
|
|
|
expect(
|
|
original_topic.custom_fields[CategoryExperts::TOPIC_EXPERT_POST_GROUP_NAMES],
|
|
).to eq(group_b.name)
|
|
expect(second_post.custom_fields[CategoryExperts::POST_APPROVED_GROUP_NAME]).to eq(
|
|
group_b.name,
|
|
)
|
|
end
|
|
|
|
context "with multiple expert posts from different users" do
|
|
fab!(:expert_2) { Fabricate(:user, refresh_auto_groups: true) }
|
|
fab!(:third_post) { Fabricate(:post, topic: original_topic, user: expert_2) }
|
|
|
|
before do
|
|
group.add(expert_2)
|
|
CategoryExperts::PostHandler.new(post: third_post, user: expert_2).mark_post_as_approved
|
|
end
|
|
|
|
it "only keeps expert status for users who are experts in new category" do
|
|
group_b.add(expert)
|
|
|
|
expect(
|
|
original_topic.custom_fields[CategoryExperts::TOPIC_EXPERT_POST_GROUP_NAMES],
|
|
).to eq(group.name)
|
|
expect(second_post.custom_fields[CategoryExperts::POST_APPROVED_GROUP_NAME]).to eq(
|
|
group.name,
|
|
)
|
|
expect(third_post.custom_fields[CategoryExperts::POST_APPROVED_GROUP_NAME]).to eq(
|
|
group.name,
|
|
)
|
|
|
|
PostRevisor.new(original_topic.first_post).revise!(admin, category_id: category_b.id)
|
|
|
|
original_topic.reload
|
|
second_post.reload
|
|
third_post.reload
|
|
|
|
expect(
|
|
original_topic.custom_fields[CategoryExperts::TOPIC_EXPERT_POST_GROUP_NAMES],
|
|
).to eq(group_b.name)
|
|
expect(second_post.custom_fields[CategoryExperts::POST_APPROVED_GROUP_NAME]).to eq(
|
|
group_b.name,
|
|
)
|
|
expect(third_post.custom_fields[CategoryExperts::POST_APPROVED_GROUP_NAME]).to be_blank
|
|
end
|
|
end
|
|
|
|
it "updates auto-tag appropriately" do
|
|
group_b.add(expert)
|
|
expect(original_topic.tags.map(&:name)).to include(auto_tag.name)
|
|
expect(original_topic.tags.map(&:name)).not_to include(auto_tag_b.name)
|
|
|
|
PostRevisor.new(original_topic.first_post).revise!(admin, category_id: category_b.id)
|
|
|
|
original_topic.reload
|
|
|
|
expect(original_topic.tags.map(&:name)).not_to include(auto_tag.name)
|
|
expect(original_topic.tags.map(&:name)).to include(auto_tag_b.name)
|
|
end
|
|
end
|
|
|
|
context "when topic is moved between two categories without experts" do
|
|
fab!(:category_without_experts_a, :category)
|
|
fab!(:category_without_experts_b, :category)
|
|
|
|
fab!(:topic_without_experts) { Fabricate(:topic, category: category_without_experts_a) }
|
|
fab!(:post_without_experts) { Fabricate(:post, topic: topic_without_experts) }
|
|
|
|
before do
|
|
category_without_experts_a.custom_fields[
|
|
CategoryExperts::CATEGORY_EXPERT_AUTO_TAG
|
|
] = auto_tag.name
|
|
category_without_experts_b.custom_fields[
|
|
CategoryExperts::CATEGORY_EXPERT_AUTO_TAG
|
|
] = auto_tag_b.name
|
|
category_without_experts_a.save!
|
|
category_without_experts_b.save!
|
|
end
|
|
|
|
it "does not auto-tag" do
|
|
expect(topic_without_experts.tags.map(&:name)).not_to include(auto_tag.name)
|
|
expect(topic_without_experts.tags.map(&:name)).not_to include(auto_tag_b.name)
|
|
|
|
PostRevisor.new(topic_without_experts.first_post).revise!(
|
|
admin,
|
|
category_id: category_without_experts_b.id,
|
|
)
|
|
|
|
topic_without_experts.reload
|
|
|
|
expect(topic_without_experts.tags.map(&:name)).not_to include(auto_tag.name)
|
|
expect(topic_without_experts.tags.map(&:name)).not_to include(auto_tag_b.name)
|
|
end
|
|
end
|
|
end
|
|
end
|
|
|
|
describe "Serializer" do
|
|
describe "needs_category_expert_post_approval" do
|
|
it "is not included when category_experts_posts_require_approval is disabled" do
|
|
SiteSetting.category_experts_posts_require_approval = false
|
|
|
|
original_topic.custom_fields[
|
|
CategoryExperts::TOPIC_NEEDS_EXPERT_POST_APPROVAL
|
|
] = second_post.post_number
|
|
original_topic.save!
|
|
|
|
guardian = Guardian.new(admin)
|
|
serializer =
|
|
TopicListItemSerializer.new(original_topic, scope: guardian, root: false).as_json
|
|
|
|
expect(serializer[:needs_category_expert_post_approval]).to be_nil
|
|
end
|
|
|
|
it "is included when category_experts_posts_require_approval is enabled" do
|
|
SiteSetting.category_experts_posts_require_approval = true
|
|
|
|
original_topic.custom_fields[
|
|
CategoryExperts::TOPIC_NEEDS_EXPERT_POST_APPROVAL
|
|
] = second_post.post_number
|
|
original_topic.save!
|
|
|
|
guardian = Guardian.new(admin)
|
|
serializer =
|
|
TopicListItemSerializer.new(original_topic, scope: guardian, root: false).as_json
|
|
|
|
expect(serializer[:needs_category_expert_post_approval]).to eq(true)
|
|
end
|
|
end
|
|
end
|
|
end
|
