-
Notifications
You must be signed in to change notification settings - Fork 8.2k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
DEV: Move array type custom fields to JSON type in automation #26939
Conversation
if once_per_user && | ||
UserCustomField.exists?(name: DiscourseAutomation::CUSTOM_FIELD, user_id: user.id) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here's one bug that will be fixed in this PR. This line should also check that the custom field's value matches the triggered automation's ID. Without doing this, it means that the once_per_user
option in an automation rule will affect other automation rules for a given user.
end | ||
end | ||
|
||
if topic_id | ||
topic = Topic.find_by(id: topic_id) | ||
topic&.upsert_custom_fields(DiscourseAutomation::CUSTOM_FIELD => automation.id) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here's another bug that will fixed in this PR. The usage of Topic#upsert_custom_fields
on a custom field of type array is incorrect because the method finds an existing record and updates its value which means the custom field will always be a single-value array. The right thing here would've been to use Automion#attach_custom_field
which creates a new record for every new value in the array, but with the migration to JSON type, the right thing now is to maintain a single record that holds multiple values.
plugins/automation/app/models/discourse_automation/automation.rb
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great, pending the comment about race conditions 👀
…rse#26939) The automation plugin has 4 custom field types that are array typed. However, array typed custom fields are deprecated and should be migrated to JSON type. This commit does a couple of things: 1. Migrate all four custom fields to JSON 2. Fix a couple of small bugs that have been discovered while migrating the custom fields to JSON (see the comments on this commit's PR for details discourse#26939)
The automation plugin has 4 custom field types that are array typed. However, array typed custom fields are deprecated and should be migrated to JSON type.
This PR migrates all four custom fields to JSON and fixes a couple of bugs that are related to the 4 custom fields. I will leave details of the bugs in comments to the relevant lines.