0
0
Fork 0
mirror of https://github.com/discourse/discourse.git synced 2026-08-11 02:59:07 +08:00
discourse/spec/system/admin_upcoming_changes_spec.rb
Régis Hanol 06b35cf0b1
FIX: Save staff group before enabling an "everyone"-excluded change (#41344)
Fixes a bug reported at
https://meta.discourse.org/t/issue-with-enabling-upcoming-changes/395881/31

### What

When an upcoming change's `allow_enabled_for` excludes `everyone` (for
example `[staff, specific_groups]`, as the former
`reporting_improvements` change had), selecting **Staff** in the
*Enabled for* dropdown failed with a `422` and the selection snapped
back to *No one*.

### Why

`enabledForChanged` toggled the setting on **before** persisting the
staff group. On the server,
`UpcomingChanges::Toggle#allowed_enabled_for_target` treats "enabled
with no group configured" as targeting `everyone` — which these changes
disallow — so the toggle was rejected and the change was never enabled.

Changes that allow `everyone` (the common case) weren't affected,
because the initial toggle passes the policy and the staff group is
saved a moment later.

### Fix

Persist the staff group **before** toggling, mirroring the existing
"specific groups" path (which already saves groups first and only then
enables the change). By the time the setting is toggled on, it is
already scoped to staff, so the policy passes.

### Tests

- **Component** (`upcoming-change-item-test.gjs`): asserts the request
order is `groups` → `toggle` for the staff selection on an
`everyone`-excluded change, and that everyone/no-one still toggle first
then clear groups.
- **System** (`admin_upcoming_changes_spec.rb`): enabling for staff on
an `everyone`-excluded change now succeeds and persists the staff
`SiteSettingGroup`.

### Screenshots

Before / after — selecting **Staff** on an `everyone`-excluded change
(Horizon, light, desktop):

<img width="1664" height="466" alt="reporting-improvements-before-after"
src="https://github.com/user-attachments/assets/085d9e7d-f61b-47bc-a683-5792e72a7217"
/>
2026-07-01 20:34:56 +02:00

563 lines
20 KiB
Ruby
Vendored

# frozen_string_literal: true
describe "Admin upcoming changes" do
fab!(:current_user, :admin)
let(:upcoming_changes_page) { PageObjects::Pages::AdminUpcomingChanges.new }
before do
mock_upcoming_change_metadata(
{
enable_upload_debug_mode: {
impact: "other,developers",
status: :experimental,
impact_type: "other",
impact_role: "developers",
},
about_page_extra_groups_show_description: {
impact: "feature,all_members",
status: :stable,
impact_type: "feature",
impact_role: "all_members",
},
},
)
SiteSetting.about_page_extra_groups_show_description = false
sign_in(current_user)
end
it "shows a list of upcoming changes and their metadata" do
upcoming_changes_page.visit
expect(upcoming_changes_page).to have_change(:about_page_extra_groups_show_description)
expect(upcoming_changes_page).to have_change(:enable_upload_debug_mode)
expect(
upcoming_changes_page.change_item(:about_page_extra_groups_show_description),
).to have_status(:stable)
expect(
upcoming_changes_page.change_item(:about_page_extra_groups_show_description),
).to have_impact_role(:all_members)
end
it "does not show conceptual upcoming changes" do
mock_upcoming_change_metadata(
{
enable_upload_debug_mode: {
impact: "other,developers",
status: :experimental,
impact_type: "other",
impact_role: "developers",
},
about_page_extra_groups_show_description: {
impact: "feature,all_members",
status: :conceptual,
impact_type: "feature",
impact_role: "all_members",
},
},
)
upcoming_changes_page.visit
expect(upcoming_changes_page).to have_change(:enable_upload_debug_mode)
expect(upcoming_changes_page).to have_no_change(:about_page_extra_groups_show_description)
end
it "shows the permanent soon notice for stable changes but not for site_setting_default types" do
mock_upcoming_change_metadata(
{
about_page_extra_groups_show_description: {
impact: "feature,all_members",
status: :stable,
impact_type: "feature",
impact_role: "all_members",
},
enable_upload_debug_mode: {
impact: "site_setting_default,all_members",
status: :stable,
impact_type: "site_setting_default",
impact_role: "all_members",
},
},
)
upcoming_changes_page.visit
expect(
upcoming_changes_page.change_item(:about_page_extra_groups_show_description),
).to have_permanent_soon_notice
expect(
upcoming_changes_page.change_item(:enable_upload_debug_mode),
).to have_no_permanent_soon_notice
end
it "does not show permanent upcoming changes" do
mock_upcoming_change_metadata(
{
allow_uppercase_posts: {
impact: "feature,all_members",
status: :permanent,
impact_type: "feature",
impact_role: "all_members",
},
},
)
upcoming_changes_page.visit
expect(upcoming_changes_page).to have_no_change(:allow_uppercase_posts)
end
it "shows upcoming changes from plugins" do
upcoming_changes_page.visit
expect(upcoming_changes_page).to have_change(:enable_experimental_sample_plugin_feature)
expect(
upcoming_changes_page.change_item(:enable_experimental_sample_plugin_feature),
).to have_plugin_name("Sample plugin")
end
it "can enable and disable an upcoming change using the dropdown" do
upcoming_changes_page.visit
expect(upcoming_changes_page.change_item(:enable_upload_debug_mode)).to be_disabled
upcoming_changes_page.change_item(:enable_upload_debug_mode).select_enabled_for("everyone")
expect(upcoming_changes_page).to have_enabled_for_success_toast("everyone")
expect(upcoming_changes_page.change_item(:enable_upload_debug_mode)).to be_enabled
expect(SiteSetting.enable_upload_debug_mode).to be_truthy
# Revisit the page to skip the rate limit
upcoming_changes_page.visit
expect(upcoming_changes_page.change_item(:enable_upload_debug_mode)).to be_enabled
upcoming_changes_page.change_item(:enable_upload_debug_mode).select_enabled_for("no_one")
expect(upcoming_changes_page).to have_disabled_success_toast
expect(upcoming_changes_page.change_item(:enable_upload_debug_mode)).to be_disabled
expect(SiteSetting.enable_upload_debug_mode).to be_falsey
end
it "tests different enabled_for options behavior" do
upcoming_changes_page.visit
# Add a group to test clearing behavior
SiteSetting.enable_upload_debug_mode = true
Fabricate(
:site_setting_group,
name: "enable_upload_debug_mode",
group_ids: Group::AUTO_GROUPS[:trust_level_4].to_s,
)
# Refresh after setting up the group
upcoming_changes_page.visit
expect(upcoming_changes_page.change_item(:enable_upload_debug_mode)).to have_groups(
"trust_level_4",
)
expect(UpcomingChanges.has_groups?(:enable_upload_debug_mode)).to be_truthy
# Test 'no_one' option - should disable the change and clear groups
upcoming_changes_page.change_item(:enable_upload_debug_mode).select_enabled_for("no_one")
expect(upcoming_changes_page).to have_disabled_success_toast
expect(upcoming_changes_page.change_item(:enable_upload_debug_mode)).to be_disabled
upcoming_changes_page.visit
expect(upcoming_changes_page.change_item(:enable_upload_debug_mode)).to have_no_group_selector
expect(SiteSetting.enable_upload_debug_mode).to be_falsey
expect(UpcomingChanges.has_groups?(:enable_upload_debug_mode)).to be_falsey
# Test 'everyone' option - should enable the change and clear groups
upcoming_changes_page.change_item(:enable_upload_debug_mode).select_enabled_for("everyone")
expect(upcoming_changes_page).to have_enabled_for_success_toast("everyone")
expect(upcoming_changes_page.change_item(:enable_upload_debug_mode)).to be_enabled
upcoming_changes_page.visit
expect(upcoming_changes_page.change_item(:enable_upload_debug_mode)).to have_no_group_selector
expect(SiteSetting.enable_upload_debug_mode).to be_truthy
# Test 'staff' option - should enable the change and set staff group
upcoming_changes_page.change_item(:enable_upload_debug_mode).select_enabled_for("staff")
expect(upcoming_changes_page).to have_enabled_for_success_toast(
"staff",
translation_args: {
staffGroupName: I18n.t("groups.default_names.staff").titleize,
},
)
expect(upcoming_changes_page.change_item(:enable_upload_debug_mode)).to be_enabled
upcoming_changes_page.visit
expect(UpcomingChanges.has_groups?(:enable_upload_debug_mode)).to be_truthy
expect(SiteSetting.enable_upload_debug_mode).to be_truthy
# Test 'groups' option - should not change enabled state until groups are selected and saved
upcoming_changes_page.change_item(:enable_upload_debug_mode).select_enabled_for("groups")
upcoming_changes_page.change_item(:enable_upload_debug_mode).add_group("trust_level_4")
upcoming_changes_page.change_item(:enable_upload_debug_mode).save_groups
expect(upcoming_changes_page).to have_enabled_for_success_toast(
"specific_groups_with_group_names",
translation_args: {
groupNames: "staff, trust_level_4",
count: 2,
},
)
upcoming_changes_page.visit
expect(upcoming_changes_page.change_item(:enable_upload_debug_mode)).to have_groups(
"staff",
"trust_level_4",
)
expect(upcoming_changes_page.change_item(:enable_upload_debug_mode)).to be_enabled
expect(UpcomingChanges.has_groups?(:enable_upload_debug_mode)).to be_truthy
expect(SiteSetting.enable_upload_debug_mode).to be_truthy
end
describe "allow_enabled_for restrictions" do
def mock_with_allow(allow)
mock_upcoming_change_metadata(
{
enable_upload_debug_mode: {
impact: "other,developers",
status: :experimental,
impact_type: "other",
impact_role: "developers",
allow_enabled_for: allow,
},
},
)
end
it "shows only No one and Everyone when allow_enabled_for is [everyone]" do
mock_with_allow([:everyone])
upcoming_changes_page.visit
item = upcoming_changes_page.change_item(:enable_upload_debug_mode)
expect(item.enabled_for_options).to contain_exactly("no_one", "everyone")
end
it "shows only No one and Staff when allow_enabled_for is [staff]" do
mock_with_allow([:staff])
upcoming_changes_page.visit
item = upcoming_changes_page.change_item(:enable_upload_debug_mode)
expect(item.enabled_for_options).to contain_exactly("no_one", "staff")
end
it "shows No one, Staff, and Specific group(s) when allow_enabled_for is [staff, specific_groups]" do
mock_with_allow(%i[staff specific_groups])
upcoming_changes_page.visit
item = upcoming_changes_page.change_item(:enable_upload_debug_mode)
expect(item.enabled_for_options).to contain_exactly("no_one", "staff", "groups")
end
it "shows all four options when allow_enabled_for is omitted" do
upcoming_changes_page.visit
item = upcoming_changes_page.change_item(:enable_upload_debug_mode)
expect(item.enabled_for_options).to contain_exactly("no_one", "everyone", "staff", "groups")
end
it "displays the broadest allowed target when an auto-promoted change has no admin scope" do
mock_with_allow(%i[staff specific_groups])
# Simulate the post-promotion state: setting is enabled globally but the
# admin has not configured a SiteSettingGroup. "Everyone" is no longer an
# allowed dropdown target, so the row should display "staff" as the
# broadest allowed scope.
SiteSetting.enable_upload_debug_mode = true
upcoming_changes_page.visit
item = upcoming_changes_page.change_item(:enable_upload_debug_mode)
expect(item.enabled_for).to eq("staff")
end
it "enables the change for staff when everyone is excluded" do
mock_with_allow(%i[staff specific_groups])
SiteSetting.enable_upload_debug_mode = false
upcoming_changes_page.visit
item = upcoming_changes_page.change_item(:enable_upload_debug_mode)
item.select_enabled_for("staff")
expect(upcoming_changes_page).to have_enabled_for_success_toast(
"staff",
translation_args: {
staffGroupName: I18n.t("groups.default_names.staff").titleize,
},
)
expect(item).to be_enabled
expect(SiteSettingGroup.find_by(name: "enable_upload_debug_mode").group_ids).to include(
Group::AUTO_GROUPS[:staff].to_s,
)
end
end
it "can filter by name, description, plugin, status, impact type, or enabled/disabled" do
upcoming_changes_page.visit
# Filter by name
upcoming_changes_page.filter_controls.type_in_search("upload debug")
expect(upcoming_changes_page).to have_change(:enable_upload_debug_mode)
expect(upcoming_changes_page).to have_no_change(:about_page_extra_groups_show_description)
upcoming_changes_page.filter_controls.clear_search
# Filter by plugin
upcoming_changes_page.filter_controls.type_in_search("sample plugin")
expect(upcoming_changes_page).to have_change(:enable_experimental_sample_plugin_feature)
expect(upcoming_changes_page).to have_no_change(:about_page_extra_groups_show_description)
upcoming_changes_page.filter_controls.clear_search
upcoming_changes_page.filter_controls.toggle_dropdown_filters
# Filter by status
upcoming_changes_page.filter_controls.select_dropdown_option("Stable", dropdown_id: "status")
expect(upcoming_changes_page).to have_no_change(:enable_upload_debug_mode)
expect(upcoming_changes_page).to have_change(:about_page_extra_groups_show_description)
upcoming_changes_page.filter_controls.select_all_dropdown_option(dropdown_id: "status")
# Filter by impact type
upcoming_changes_page.filter_controls.select_dropdown_option("Feature", dropdown_id: "type")
expect(upcoming_changes_page).to have_no_change(:enable_upload_debug_mode)
expect(upcoming_changes_page).to have_change(:about_page_extra_groups_show_description)
upcoming_changes_page.filter_controls.select_all_dropdown_option(dropdown_id: "type")
# Filter by impact role
upcoming_changes_page.filter_controls.select_dropdown_option(
"Developers",
dropdown_id: "impactRole",
)
expect(upcoming_changes_page).to have_change(:enable_upload_debug_mode)
expect(upcoming_changes_page).to have_no_change(:about_page_extra_groups_show_description)
upcoming_changes_page.filter_controls.select_all_dropdown_option(dropdown_id: "impactRole")
# Filter by enabled/disabled
upcoming_changes_page.filter_controls.select_dropdown_option("Enabled", dropdown_id: "enabled")
expect(upcoming_changes_page).to have_no_change(:enable_upload_debug_mode)
expect(upcoming_changes_page).to have_no_change(:about_page_extra_groups_show_description)
upcoming_changes_page.filter_controls.select_all_dropdown_option(dropdown_id: "enabled")
end
it "updates the filter when a notification is clicked while already on the page" do
user_menu = PageObjects::Components::UserMenu.new
Fabricate(
:notification,
user: current_user,
notification_type: Notification.types[:upcoming_change_available],
data: {
upcoming_change_names: ["enable_upload_debug_mode"],
upcoming_change_humanized_names: [
SiteSettings::LabelFormatter.humanized_name(:enable_upload_debug_mode),
],
count: 1,
}.to_json,
)
Fabricate(
:notification,
user: current_user,
notification_type: Notification.types[:upcoming_change_available],
data: {
upcoming_change_names: ["about_page_extra_groups_show_description"],
upcoming_change_humanized_names: [
SiteSettings::LabelFormatter.humanized_name(:about_page_extra_groups_show_description),
],
count: 1,
}.to_json,
)
visit "/"
user_menu.open.click_notification_with_href("enable_upload_debug_mode")
expect(upcoming_changes_page).to have_change(:enable_upload_debug_mode)
expect(upcoming_changes_page).to have_no_change(:about_page_extra_groups_show_description)
user_menu.open.click_notification_with_href("about_page_extra_groups_show_description")
expect(upcoming_changes_page).to have_change(:about_page_extra_groups_show_description)
expect(upcoming_changes_page).to have_no_change(:enable_upload_debug_mode)
end
it "displays a notification dot on the sidebar and clears it when navigating to upcoming changes" do
sidebar = PageObjects::Components::NavigationMenu::Sidebar.new
Discourse.stubs(:site_creation_date).returns(1.day.ago)
UpcomingChangeEvent.create!(
event_type: :added,
upcoming_change_name: "enable_upload_debug_mode",
)
visit "/admin"
sidebar.toggle_all_sections
expect(sidebar.find_section_link("admin_upcoming_changes")).to have_css(
".sidebar-section-link-suffix.admin-sidebar-nav-link__dot",
)
sidebar.find_section_link("admin_upcoming_changes").click
expect(sidebar.find_section_link("admin_upcoming_changes")).to have_no_css(
".sidebar-section-link-suffix.admin-sidebar-nav-link__dot",
)
end
it "does not display a notification dot when there are no new added events" do
sidebar = PageObjects::Components::NavigationMenu::Sidebar.new
current_user.custom_fields["last_visited_upcoming_changes_at"] = Time.current.iso8601
current_user.save_custom_fields
visit "/admin"
sidebar.toggle_all_sections
expect(sidebar.find_section_link("admin_upcoming_changes")).to have_no_css(
".sidebar-section-link-suffix.admin-sidebar-nav-link__dot",
)
end
context "when the staff group name has been localized" do
before do
SiteSetting.default_locale = "de"
Group.refresh_automatic_group!(:staff)
end
it "displays the localized name in the enabled for options and enabling staff works correctly" do
upcoming_changes_page.visit
upcoming_changes_page.change_item(:enable_upload_debug_mode).select_enabled_for(
Group.find(Group::AUTO_GROUPS[:staff]).name,
)
expect(upcoming_changes_page).to have_enabled_for_success_toast(
"staff",
translation_args: {
staffGroupName: I18n.t("groups.default_names.staff", locale: SiteSetting.default_locale),
},
)
expect(upcoming_changes_page.change_item(:enable_upload_debug_mode)).to be_enabled
expect(SiteSetting.enable_upload_debug_mode).to be_truthy
expect(SiteSettingGroup.find_by(name: "enable_upload_debug_mode").group_ids).to include(
Group::AUTO_GROUPS[:staff].to_s,
)
upcoming_changes_page.visit
expect(upcoming_changes_page.change_item(:enable_upload_debug_mode).enabled_for).to eq(
Group.find(Group::AUTO_GROUPS[:staff]).name,
)
end
end
context "when the upcoming change has a default override" do
let(:settings_page) { PageObjects::Pages::AdminSiteSettings.new }
before do
mock_upcoming_change_metadata(
{
enable_upload_debug_mode: {
impact: "other,developers",
status: :experimental,
impact_type: "other",
impact_role: "developers",
},
},
)
mock_upcoming_change_default_overrides(
{
suggested_topics_max_days_old: {
upcoming_change: :enable_upload_debug_mode,
new_default: 1000,
},
},
)
SiteSetting.enable_upload_debug_mode = true
SiteSetting.refresh!
end
it "shows information about the default override in the site settings UI" do
settings_page.visit("suggested_topics_max_days_old")
expect(settings_page).to have_upcoming_change_default_warning(
:suggested_topics_max_days_old,
old_default: 365,
new_default: 1000,
)
end
end
context "when the upcoming change has a boolean default override and the admin opts out" do
let(:settings_page) { PageObjects::Pages::AdminSiteSettings.new }
before do
mock_upcoming_change_metadata(
{
enable_upload_debug_mode: {
impact: "site_setting_default,all_members",
status: :experimental,
impact_type: "site_setting_default",
impact_role: "all_members",
},
},
)
mock_upcoming_change_default_overrides(
{
limit_suggested_to_category: {
upcoming_change: :enable_upload_debug_mode,
new_default: true,
},
},
)
SiteSetting.enable_upload_debug_mode = true
SiteSetting.refresh!
end
after do
clear_mocked_upcoming_change_metadata
clear_mocked_upcoming_change_default_overrides
end
it "preserves the admin's opt-out across page reload while still showing the override warning" do
settings_page.visit("limit_suggested_to_category")
expect(settings_page).to have_upcoming_change_default_warning(
:limit_suggested_to_category,
old_default: false,
new_default: true,
)
expect(
settings_page.find_setting(:limit_suggested_to_category).find(
".setting-value input[type=checkbox]",
),
).to be_checked
settings_page.toggle_setting(:limit_suggested_to_category)
# Wait for the save to land in the DB before forcing the in-process
# SiteSetting refresh below — the .overridden class only appears once
# the value has been persisted.
expect(
settings_page.find_setting(:limit_suggested_to_category, overridden: true),
).to be_present
# Production reproduces the bug because each unicorn worker re-runs
# SiteSetting.refresh! via the MessageBus subscriber after another
# worker persists a setting change. In a single-process system spec
# that subscriber doesn't fire, so we trigger the refresh explicitly
# to mirror what happens on the next request in a real deployment.
SiteSetting.refresh!
page.refresh
expect(settings_page).to have_upcoming_change_default_warning(
:limit_suggested_to_category,
old_default: false,
new_default: true,
)
expect(
settings_page.find_setting(:limit_suggested_to_category).find(
".setting-value input[type=checkbox]",
),
).not_to be_checked
end
end
end