Skip to content
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.

Sign up for GitHub

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

Jump to bottom

DEV: Move solved custom fields into a table #342

Merged
merged 12 commits into from
Mar 25, 2025
Merged

DEV: Move solved custom fields into a table #342

merged 12 commits into from
Mar 25, 2025
+273 −289

Conversation

nattsw
Copy link
Contributor

@nattsw nattsw commented Mar 21, 2025 •
edited
Loading

Related:

Requires:

This PR converts all use of post and topic custom fields into a dedicated table:

  • migration for copying custom field into table
  • swap app usage of custom fields to table

This PR does not attempt to fix issues or optimise, and does not delete old data from custom fields yet.

@nattsw nattsw force-pushed the custom-fields-to-table branch from bb8160b to 7d6611d Compare March 21, 2025 03:50
tgxworld
tgxworld reviewed Mar 21, 2025
View reviewed changes
tgxworld
tgxworld reviewed Mar 21, 2025
View reviewed changes
@nattsw nattsw mentioned this pull request Mar 21, 2025
Merged
@nattsw nattsw force-pushed the custom-fields-to-table branch from 087f947 to ba32049 Compare March 21, 2025 12:53
This was referenced Mar 21, 2025
Closed
Merged
nattsw added a commit to discourse/discourse that referenced this pull request Mar 24, 2025
@nattsw
DEV: Allow loading topic associations on /categories (#31954)
af03873 
`/categories` sometimes returns accompanying topics under certain site
settings. The `CategoryList` currently allows preloading for topic
custom fields via `preloaded_topic_custom_fields`, but not for topics
themselves.

This addition is required for
discourse/discourse-solved#342.
@nattsw nattsw force-pushed the custom-fields-to-table branch from b6c14fb to 75ad157 Compare March 24, 2025 12:19
@nattsw nattsw mentioned this pull request Mar 24, 2025
Merged
tgxworld
tgxworld reviewed Mar 25, 2025
View reviewed changes
plugin.rb

topic_view_post_custom_fields_allowlister { [::DiscourseSolved::IS_ACCEPTED_ANSWER_CUSTOM_FIELD] }
TopicList.preloaded_custom_fields << ::DiscourseSolved::ACCEPTED_ANSWER_POST_ID_CUSTOM_FIELD
register_category_list_topics_preloader_associations(:solved) if SiteSetting.solved_enabled
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This API is not yet available on the stable branch so we need to update the plugins compatibility file.

tgxworld
tgxworld reviewed Mar 25, 2025
View reviewed changes
spec/models/solved_topics_spec.rb Outdated
@@ -0,0 +1,66 @@
# frozen_string_literal: true
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just dropping a note to remove this before merging.

tgxworld
tgxworld approved these changes Mar 25, 2025
View reviewed changes
Copy link
Contributor

@tgxworld tgxworld left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changes look good to me 👍 Do add a .plugin-compatibility file before merging :)

nattsw
nattsw commented Mar 25, 2025
View reviewed changes
.discourse-compatibility Outdated
Comment on lines 1 to 2
< 3.5.0.beta2-dev: e82c6ae1ca38ccebb34669148f8de93a3028906e
3.4.1: e82c6ae1ca38ccebb34669148f8de93a3028906e
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@tgxworld

The SHA before this PR in this plugin is e82c6ae1ca38ccebb34669148f8de93a3028906e.
Current discourse version is 3.4.1 and 3.5.0.beta2.

@nattsw nattsw merged commit 55c1eb4 into main Mar 25, 2025
5 checks passed
@nattsw nattsw deleted the custom-fields-to-table branch March 25, 2025 06:51
@nattsw nattsw mentioned this pull request Mar 25, 2025
Merged
nattsw added a commit that referenced this pull request Mar 25, 2025
@nattsw
FIX: Migration was only done for first batch (#344)
e0a579e 
#342 was deployed and observed to only have 5000 (batch size) migrated. This is an error in migration where the ids had a gap between the batch.

This PR changes the migration to just loop through all topic custom fields with each loop increasing by batch size.
nattsw added a commit that referenced this pull request Mar 25, 2025
@nattsw
FEATURE: Show 'marked solved by' in OP when topic is solved (#343)
5a0c875 
Depends on: #342

This feature adds the "Marked solved as" information to the solved post appended to OP.

Originally, I had moved the widget usage to a [component](https://github.com/discourse/discourse-solved/blob/39baa0be4a889fdbff108e887a677d9a298d27d4/assets/javascripts/discourse/components/solved-post.gjs), but due to "cooking quotes", after some internal discussion (t/95318/25) we will stick to widgets for now as the post-stream gets modernized.
jjaffeux pushed a commit to jjaffeux/discourse that referenced this pull request Mar 25, 2025
@nattsw @jjaffeux
DEV: Allow loading topic associations on /categories (discourse#31954)
6627366 
`/categories` sometimes returns accompanying topics under certain site
settings. The `CategoryList` currently allows preloading for topic
custom fields via `preloaded_topic_custom_fields`, but not for topics
themselves.

This addition is required for
discourse/discourse-solved#342.
@nattsw nattsw mentioned this pull request Mar 26, 2025
Merged
nattsw added a commit to discourse/discourse-solved-reminders-plugin that referenced this pull request Mar 26, 2025
@nattsw
FIX: Optimise and update queries, shorter frequency (#20)
be146b2 
This commit solves a few issues:
- Currently due to the 14.day schedule, any sites with `remind_mark_solution_last_post_age` of 1 will not be able to receive notifications. This bumps the frequency to a day
- On a site with hundreds of thousands of topics, this loads all the ids into memory `unsolved_topic_ids.push(*Topic.where(category_id: category_ids).pluck(:id))` 😱  
- Many plucks (N+1)
- (Potentially) Large array subtractions are inefficient and could be done in the database
- Moves to custom fields as per discourse/discourse-solved#342
@nattsw nattsw mentioned this pull request Mar 26, 2025
Merged
nattsw added a commit to discourse/discourse-gamification that referenced this pull request Mar 26, 2025
@nattsw
FIX: Update scoring based on table (#198)
43e0bca 
As part of the custom fields to table move for discourse-solved (discourse/discourse-solved#342), we need to update the query here
@nattsw nattsw mentioned this pull request Mar 26, 2025
Merged
nattsw added a commit to discourse/discourse-gamification that referenced this pull request Mar 26, 2025
@nattsw
FIX: Disable solutions scorable when the solved plugin is disabled (#199
5bc51d8 
)

Before discourse/discourse-solved#342, scorables for solved were in custom fields, this means no errors or validations were required since every instance is able to query custom fields.

Now we will have to check for the availability of discourse_solved prior to scoring. This commit disables the scorable if the plugin is not enabled or installed.
@thoka
Copy link

thoka commented Mar 26, 2025

This update might break discourse during migration, if for some reason two posts link to the same resolved topic.
See https://meta.discourse.org/t/error-recordnotunique-on-index-discourse-solved-solved-topics-on-answer-post-id/358875

@nattsw nattsw mentioned this pull request Mar 26, 2025
Merged
nattsw added a commit that referenced this pull request Mar 26, 2025
@nattsw
FIX: Some solved topics have no answers (#350)
9e9ac28 
As part of #342, some discrepancies in the old implementation resulted in solved topics linking to posts that do not exist (dangling custom field value). Causing us to see the following when there are no `answer_post`

```
NoMethodError (undefined method `user' for nil)
```

This PR prevents the error.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tgxworld tgxworld

Assignees
No one assigned
Labels
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@nattsw @thoka @tgxworld @Lhcfl