DEV: Refactor themes controller to services part 1 #29631
Conversation
Need to sort out remote theme git policy
| private | ||
|
|
||
| def ban_in_allowlist_mode | ||
| Theme.allowed_remote_theme_ids.nil? |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
It's weird, sometimes it's nil and sometimes it's an empty array...but yes I think blank could work here
Lines 258 to 265 in dd4755b
There was a problem hiding this comment.
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
Outdated
| # TODO (martin) Ask loic about this, the old theme controller expected the | ||
| # errors from the theme model save to be shown to the user. | ||
| def save_theme(theme:) | ||
| theme.save! |
There was a problem hiding this comment.
For now, either use a rescue block and call fail! inside it or just call save and check for any errors (and call fail! 😁), as we discussed IIRC.
It seems we should be able to do something like that in the model step, though:
Theme.create(params.slice(:name, :user_id, :user_selectable, :color_scheme_id, :component)) do |theme|
params.theme_fields.each do |field|
theme.set_field(**field)
end
endThe only problem is still that Theme#set_field raises because it does validations that should be done in ThemeField. Maybe a small refactor (moving validations where they should happen) could resolve that situation.
Maybe that’s an approach we could apply for updates too (Model.find_by(…).tap {|model| model.update(…) }), I don’t know 🤔
Otherwise, we could implement a new step, something like persist_model :my_model, it would call save on it, check if the model is both valid and persisted, if that’s not the case, it would stop the execution flow.
There was a problem hiding this comment.
Sure I could try a refactor of set_field, and otherwise do the rescue/error check for save. persist_model sounds nice too 👍
There was a problem hiding this comment.
I just went for the theme.save/fail! model errors for now
There was a problem hiding this comment.
I’m working on adding a new step to handle exceptions: #29660.
That would probably come in handy in a situation like this 😁
|
@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
endInstead 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 |
|
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 |
|
Great! Let me know if you need a hand or when you want another review 😁 |
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.