mirror of
https://github.com/discourse/discourse.git
synced 2026-03-04 01:15:08 +08:00
ae58a5a1be
When dismissing an admin notice, the service would look up the
associated `ProblemCheckTracker` by identifier only. This could find a
tracker with a `NULL` target, which then fails validation on update
since target is now required (as of 8ca5fb706a).
The root issue is that `problem_check_trackers.target` was added with
`null: true` and while we later added a default value of `"__NULL__"`
and a Ruby validation, we never enforced `NOT NULL` at the database
level. This allowed records with `NULL` targets to persist or be
recreated through code paths that explicitly passed nil.
This commit:
- Updates the dismiss service to look up trackers by both identifier AND
target (extracted from the admin notice's details)
- Removes any remaining records with `NULL` targets
- Adds a `NOT NULL` constraint to prevent this from recurring
Ref - https://meta.discourse.org/t/392248
75 lines
2.1 KiB
Ruby
75 lines
2.1 KiB
Ruby
# frozen_string_literal: true
|
|
|
|
RSpec.describe(AdminNotices::Dismiss) do
|
|
describe described_class::Contract, type: :model do
|
|
it { is_expected.to validate_presence_of(:id) }
|
|
end
|
|
|
|
describe ".call" do
|
|
subject(:result) { described_class.call(params:, **dependencies) }
|
|
|
|
fab!(:current_user, :admin)
|
|
fab!(:admin_notice) { Fabricate(:admin_notice, identifier: "problem.test") }
|
|
fab!(:problem_check) { Fabricate(:problem_check_tracker, identifier: "problem.test", blips: 3) }
|
|
|
|
let(:params) { { id: notice_id } }
|
|
let(:notice_id) { admin_notice.id }
|
|
let(:dependencies) { { guardian: current_user.guardian } }
|
|
|
|
context "when user is not allowed to perform the action" do
|
|
fab!(:current_user, :user)
|
|
|
|
it { is_expected.to fail_a_policy(:invalid_access) }
|
|
end
|
|
|
|
context "when data is invalid" do
|
|
let(:notice_id) { nil }
|
|
|
|
it { is_expected.to fail_a_contract }
|
|
end
|
|
|
|
context "when the admin notice has already been dismissed" do
|
|
before { admin_notice.destroy! }
|
|
|
|
it { is_expected.to run_successfully }
|
|
end
|
|
|
|
context "when everything's ok" do
|
|
it { is_expected.to run_successfully }
|
|
|
|
it "destroys the admin notice" do
|
|
expect { result }.to change { AdminNotice.count }.from(1).to(0)
|
|
end
|
|
|
|
it "resets any associated problem check" do
|
|
expect { result }.to change { problem_check.reload.blips }.from(3).to(0)
|
|
end
|
|
end
|
|
|
|
context "when the admin notice has a specific target" do
|
|
fab!(:admin_notice) do
|
|
Fabricate(
|
|
:admin_notice,
|
|
identifier: "problem.test",
|
|
details: {
|
|
"target" => "specific_target",
|
|
},
|
|
)
|
|
end
|
|
fab!(:problem_check) do
|
|
Fabricate(
|
|
:problem_check_tracker,
|
|
identifier: "problem.test",
|
|
target: "specific_target",
|
|
blips: 5,
|
|
)
|
|
end
|
|
|
|
it { is_expected.to run_successfully }
|
|
|
|
it "resets the problem check with the matching target" do
|
|
expect { result }.to change { problem_check.reload.blips }.from(5).to(0)
|
|
end
|
|
end
|
|
end
|
|
end
|
