FIX: Rely on core for staff action logs #176
Conversation
Previously, we were deleting the `enable_topic_voting` custom field param and doing the staff logging in this plugin. This has a problem though, it was being done in the model's create/destroy hooks and was missing the acting user (so the logs would report the change as `system`). This PR keeps the custom field param and relies on core to do the staff action logging. This change also results in the custom field being set in the DB when it is enabled even though we don't use it in the serializer.
| @@ -4,7 +4,7 @@ module DiscourseTopicVoting | |||
| module CategoriesControllerExtension | |||
| def category_params | |||
| @vote_enabled ||= | |||
| !!ActiveRecord::Type::Boolean.new.cast(params[:custom_fields]&.delete(:enable_topic_voting)) | |||
There was a problem hiding this comment.
this delete() here removes the parameter from the core controller, I guess it's because we don't want to store the custom field (because we have a separate table for the category/setting mappings) but it also means that it skips core's default handling for the staff action logger.
Keeping the enable_topic_voting param keeps the staff action logging from core, but it also stores a row in the category custom fields table for enable_topic_voting: true.
There was a problem hiding this comment.
I'm not too familiar with this part of the code and I don't know the history of moving from the custom field, but I feel like if our best practice is having a separate table, we could have a better DX to be able to pass the value from the FE and logging the staff action without relying on a custom field.
But it's a bigger conversation, I don't think having the custom field row will cause any issue.
/cc @nattsw if you have any insights :)
Previously, we were deleting the
enable_topic_votingcustom field param and doing the staff logging in this plugin. This has a problem though, it was being done in the model's create/destroy hooks and was missing the acting user (so the logs would report the change assystem).This PR keeps the custom field param and relies on core to do the staff action logging. This change also results in the custom field being set in the DB when it is enabled even though we don't use it in the serializer.