-
Notifications
You must be signed in to change notification settings - Fork 25
Direct Question.post relations
#4033
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
base: main
Are you sure you want to change the base?
Conversation
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.
This is a good change in my opinion. Code is fine and relatively simple.
Running the site locally works with no glitches. I checked the most important things such as browsing questions and notebooks, forecasting, resolving, leaderboard updating. I didn't check the more nuanced features like notifications and indexes, though I suspect they still work as expected.
Not that it's critical, but rolling back the migrations also worked properly.
I didn't benchmark anything to actually evaluate efficiencies, but nothing worked slower to the point where I could notice during normal use of the site.
elisescu
left a comment
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.
LGTM
Added a direct Question.post FK to replace the old QuestionPost database view. This simplifies queries and improves performance.
Changes:
postsQueries are simpler, no more view indirection, and batch jobs skip loading unnecessary models.
closes #4031