DEV: Partially refactor themes controller to services #33141
Conversation
This commit starts to introduce services to replace actions in the ThemesController. We will start with the low langing fruit: * Create * Destroy * BulkDestroy * GetTranslations Endpoints like `#import` and `#update` will be much harder. Also, the https://github.com/discourse/discourse-theme-creator plugin overrides some of these controller actions directly, so we need to be careful.
| policy :ensure_remote_themes_are_not_allowlisted | ||
|
|
||
| transaction do | ||
| try(Theme::InvalidFieldTargetError, Theme::InvalidFieldTypeError) do |
There was a problem hiding this comment.
Following a recent change to the model step, we don’t need this try block anymore. If an exception is raised inside a model step, the step will fail and can be matched with the on_model_not_found outcome.
In the controller, it seems we weren’t catching this exception, but I guess we should probably match the on_model_not_found outcome.
| fab!(:theme_1) { Fabricate(:theme) } | ||
| fab!(:theme_2) { Fabricate(:theme) } | ||
| fab!(:admin) | ||
|
|
There was a problem hiding this comment.
The contract should be tested, we should add those:
describe described_class::Contract, type: :model do
it { is_expected.to validate_presence_of(:theme_ids) }
it { is_expected.to validate_length_of(:theme_ids).as_array.is_at_least(1).is_at_most(50) }| let(:params) { { theme_ids: [theme_1.id, theme_2.id] } } | ||
| let(:dependencies) { { guardian: admin.guardian } } | ||
|
|
||
| it "destroys the themes" do |
There was a problem hiding this comment.
One thing I’ve been doing lately is using that way of organizing specs for services: https://meta.discourse.org/t/using-service-objects-in-discourse/333641#p-1635143-testing-17 (example from the docs). I think it makes things a bit clearer (follow the possible failures first, then open a context for the happy path). With that in mind, we should probably do something like:
context "when data is invalid" do
let(:params) { {} }
it { is_expected.to fail_a_contract }
end
context "when themes can’t be found" do
before do
theme_1.destroy!
theme_2.destroy!
end
it { is_expected.to fail_to_find_a_model(:themes) }
end
context "when everything’s ok" do
it { is_expected.to run_successfully }
…
endWe can do the same thing for the other specs too.
| it "logs the theme destroys" do | ||
| expect_any_instance_of(StaffActionLogger).to receive(:log_theme_destroy).with(theme_1).once | ||
| expect_any_instance_of(StaffActionLogger).to receive(:log_theme_destroy).with(theme_2).once | ||
| expect(result).to be_a_success |
There was a problem hiding this comment.
This is better to use the dedicated matcher run_successfully instead, as if there is a failure, it will output some debug info to help understand what went wrong.
It’s also simpler to use a dedicated example (it { is_expected.to run_successfully }) and not repeat it in other examples as they share the same context (so it should be a success in each one of them).
| fab!(:admin) | ||
| fab!(:guardian) { admin.guardian } | ||
| fab!(:color_scheme) | ||
|
|
There was a problem hiding this comment.
Same here regarding the contract not being tested (we’ll probably have to move all the let/subject/fab calls inside the describe "#call" block).
| RSpec.describe Themes::Destroy do | ||
| fab!(:theme) | ||
| fab!(:admin) | ||
|
|
There was a problem hiding this comment.
Same here regarding the contract not being tested.
| let(:dependencies) { { guardian: admin.guardian } } | ||
|
|
||
| it "destroys the theme" do | ||
| expect(result).to be_a_success |
There was a problem hiding this comment.
Same regarding using run_successfully.
| let(:locale) { I18n.available_locales.first.to_s } | ||
| let(:params) { { id: theme.id, locale: locale } } | ||
|
|
||
| describe described_class::Contract, type: :model do |
There was a problem hiding this comment.
We should move this at the top of the main block and move fab/let calls inside the describe "#call" block.
| context "when locale is invalid" do | ||
| let(:locale) { "invalid_locale" } | ||
|
|
||
| it "should be invalid" do |
There was a problem hiding this comment.
Don’t use "should" in the description (see the RSpec Style Guide: https://rspec.rubystyle.guide/#should-in-example-docstrings).
Another way of doing this would be to use allow_value from shoulda matchers:
# at the same level than the other `it`s
it { is_expected.not_to allow_value("invalid_locale").for(:locale).with_message(I18n.t("errors.messages.invalid_locale", invalid_locale: "invalid_locale") }
it { is_expected.to allow_values(:fr, :en, :de).for(:locale) }| describe "#call" do | ||
| subject(:result) { described_class.call(params:) } | ||
|
|
||
| it { is_expected.to be_a_success } |
There was a problem hiding this comment.
Same regarding using run_successfully.
This commit starts to introduce services to replace actions in the
ThemesController. We will start with the low langing fruit:
Endpoints like
#importand#updatewill be much harder.Also, the https://github.com/discourse/discourse-theme-creator plugin
overrides some of these controller actions directly, so we need
to be careful.