-
Notifications
You must be signed in to change notification settings - Fork 25
Don't show the CP movement for questions not old enough #3739
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
questions/utils.py
Outdated
| is_above_2_weeks = question_age >= timedelta(days=14) | ||
|
|
||
| is_open = question.status == QuestionStatus.OPEN | ||
| forecasters_count = question.get_forecasters().count() |
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 will introduce an N+1 query and could lead to performance issues, especially since it’s used in serialize_question_movement, which gets called in many places where we render multiple questions at once
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.
Good catch
The question should either be more than 2 weeks old, or have at least 20 forecasters
| forecasts_count=SubqueryAggregate("forecast", aggregate=Count) | ||
| ) | ||
|
|
||
| def annotate_forecasters_count(self): |
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 makes sense, but I do have some concerns about the potential performance overhead.
Ideally, we could introduce a cached field on the Question model (similar to Post.forecasters_count and Post.update_forecasters_count) that's updated during the forecasting process. That way, we avoid recalculating it repeatedly, and it could also be useful for other features.
WDYT?
The question should either be more than 2 weeks old, or have at least 20 forecasters
Closes #3725