DEV: Refactor themes controller to services part 1 #29631

martin-brennan · 2024-11-06T23:29:51Z

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.

Flink

I know this is still a draft, but I left some comments, some are just me thinking out loud, I’ll be glad to get your feedback 😁

app/services/themes/create.rb

Flink · 2024-11-07T13:37:33Z

app/services/themes/create.rb

+  private
+
+  def ban_in_allowlist_mode
+    Theme.allowed_remote_theme_ids.nil?


This can’t be an empty array? It looks like it should return an array 😅 We could maybe use #blank? instead of #nil? just to be sure?

It's weird, sometimes it's nil and sometimes it's an empty array...but yes I think blank could work here

discourse/app/models/theme.rb

Lines 258 to 265 in dd4755b

def self.allowed_remote_theme_ids

return nil if GlobalSetting.allowed_theme_repos.blank?

get_set_cache "allowed_remote_theme_ids" do

urls = GlobalSetting.allowed_theme_repos.split(",").map(&:strip)

Theme.joins(:remote_theme).where("remote_themes.remote_url in (?)", urls).pluck(:id)

end

end

Annoyingly this return nil seems to be important, if Theme.joins(:remote_theme).where("remote_themes.remote_url in (?)", urls).pluck(:id) returns an [] we don't care for the purposes of other existing code. Not sure if that's intentional or not, don't want to touch it for now

app/services/themes/create.rb

Flink · 2024-11-19T11:51:57Z

@martin-brennan ok, so now that #29660 has been merged, we can do this (just tested locally, did not a lot of tests though, but it’s working):

  transaction do
    try(Theme::InvalidFieldTargetError, Theme::InvalidFieldTypeError) do
      model :theme, :create_theme
    end
    step :update_default_theme
    step :log_theme_change
  end

  def create_theme(params:)
    Theme.create(params.slice(:name, :user_id, :user_selectable, :color_scheme_id, :component)) do |theme|
      params.theme_fields.to_a.each { |field| theme.set_field(**field.symbolize_keys) }
    end
  end

Instead of having 3 steps with their own error logic/handling, we just use one and everything’s covered 😁

In the controller, I see that everything’s matched through on_failure, but if we want more granular control, we can use on_model_errors (meaning the Theme model has errors on it, its creation failed for whatever reason) and on_exceptions (meaning #set_field failed at some points. I don’t think the model will be available at that point, though).

martin-brennan · 2024-11-21T02:37:10Z

Nice that's great @Flink thanks for letting me know, will do some further refactors with this now 👍 Also I need to go and make service specs for all the services I've added 😅 The functionality is covered by controller specs but I still want the low level ones too

Flink · 2024-11-21T08:41:29Z

Great! Let me know if you need a hand or when you want another review 😁

github-actions · 2025-01-21T01:56:48Z

This pull request has been automatically marked as stale because it has been open for 60 days with no activity. To keep it open, remove the stale tag, push code, or add a comment. Otherwise, it will be closed in 14 days.

github-actions · 2025-04-06T02:08:02Z

This pull request has been automatically marked as stale because it has been open for 60 days with no activity. To keep it open, remove the stale tag, push code, or add a comment. Otherwise, it will be closed in 14 days.

martin-brennan · 2025-04-22T04:36:24Z

@Flink okay it's been a few months 😅 But this is finally done now I think, can you please re-review?

martin-brennan · 2025-05-09T10:46:34Z

@Flink another bump here, can you please take a look?

Flink · 2025-05-09T10:56:26Z

@Flink another bump here, can you please take a look?

Yes sorry, completely forgot, as you're on vacation I thought it wasn't that urgent 😅
Will review first thing on Monday 🙂

martin-brennan · 2025-05-09T10:58:15Z

@Flink it's okay, not super urgent, as long as it's in your queue somewhere, I just couldn't tell whether you got the first ping, I thought you might have been on vacation when I did it :)

Flink · 2025-05-09T11:06:41Z

@Flink it's okay, not super urgent, as long as it's in your queue somewhere, I just couldn't tell whether you got the first ping, I thought you might have been on vacation when I did it :)

No worries, it's definitely in my queue 😉

Flink

Great to see some more services 👍 😁

Nothing too bad in my comments, mainly suggestions/improvements.

app/controllers/admin/themes_controller.rb

app/services/themes/bulk_destroy.rb

Flink · 2025-05-12T14:46:19Z

app/services/themes/create.rb

+  policy :ensure_remote_themes_are_not_allowlisted
+
+  transaction do
+    try(Theme::InvalidFieldTargetError, Theme::InvalidFieldTypeError) do


Following a recent change to the model step (you’ll need to rebase this PR against main, though), 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.

app/services/themes/create.rb