Feat/mc options change #4038
Feat/mc options change #4038
Conversation
add options_history field to question model and migration add options_history to question serialization add options_history initialization to question creation add helper functions to question/services/multiple_choice_handlers.py and add 'automatic' to forecast.source selection fix build_question_forecasts import and remove options & options_history from admin panel edit tests for question creation, multiple_choice_rename_option, multiple_choice_options_reorder, multiple_choice_delete_options, multiple_choice_add_options mark some expected failures add options_history to openapi docs add csv reporting support for options_history update logic to play well with back/forward filling 0s add all_options_ever to serializer and api docs add current options to csv return add support for datetime isoformat in options_history
fix file restructure fix datetime iso format in history conflicts add support for None values in MC predictions fix tests and source logic fix test_comment_edit_include_forecast to explicitly set forecast times mark xfail tests
mc/3806/aggregations adjust aggregations to play nicely with placeholders improve test for comput_weighted_semi_standard_deviations add support for None s in prediction difference for sorting plus tests update prediction difference for display to handle placeholders reactivate skipped tests
mc/3801/scoring add OptionsHistoryType, multiple_choice_interpret_forecast, and test update test for change to function update string_location_to_scaled_location to accept all historical option values, and related test multiple choice forecasts require interpretation before scoring remove double written definition support the new MC format scoring tests for mc with placeholder values add support for None values fix some linting errors left from previous commit
mc/3802/backend/notifications add notification logic add mjml/html and update tasks to setup for notifications add withdrawal notifications and bulletin deactivation fix grace period end bug
mc/3804/backend/updating add admin form for changing options add comment author and text to admin panel action and mc change methods
mc/3805/frontend/graphing forecast only current option values aggregation explorer disclaimer to forecast during grace period add option reordering (should be in mc/3804)
mc/3805/ui Added tooltip and highlight for newly added MC options Updated highlight method and message copy grace period timer translation strings Co-authored-by: aseckin <[email protected]>
...end/src/components/forecast_maker/forecast_maker_question/forecast_maker_multiple_choice.tsx
Outdated
Show resolved
Hide resolved
...end/src/components/forecast_maker/forecast_maker_question/forecast_maker_multiple_choice.tsx
Outdated
Show resolved
Hide resolved
...end/src/components/forecast_maker/forecast_maker_question/forecast_maker_multiple_choice.tsx
Outdated
Show resolved
Hide resolved
...end/src/components/forecast_maker/forecast_maker_question/forecast_maker_multiple_choice.tsx
Outdated
Show resolved
Hide resolved
...end/src/components/forecast_maker/forecast_maker_question/forecast_maker_multiple_choice.tsx
Show resolved
Hide resolved
...end/src/components/forecast_maker/forecast_maker_question/forecast_maker_multiple_choice.tsx
Outdated
Show resolved
Hide resolved
update comment copy prefill grace period end with now + 3 days
| UserForecastNotification.objects.filter( | ||
| user=forecaster, question=question | ||
| ).delete() # is this necessary? | ||
| UserForecastNotification.objects.update_or_create( | ||
| user=forecaster, | ||
| question=question, | ||
| defaults={ | ||
| "trigger_time": grace_period_end - timedelta(days=1), | ||
| "email_sent": False, | ||
| "forecast": Forecast.objects.filter( | ||
| question=question, author=forecaster | ||
| ) | ||
| .order_by("-start_time") | ||
| .first(), | ||
| }, | ||
| ) |
fix tests update legend, prediction interface, and timeline to have labels (upcoming) and (deleted)
lsabor
force-pushed
the
feat/mc-options-change
branch
from
0a09872 to
63d1e2a
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
tests/unit/test_utils/test_the_math/test_aggregations.py (1)
610-683: Make RecencyWeightedAggregation assertions None‑safe now that placeholder cases are added.With
Noneplaceholders, the currentlist == list or np.allclose(...)fallback can error if float equality fails. Mirror the nan‑safe approach used intest_UnweightedAggregation.🛠️ Proposed change
new_aggregation = aggregation.calculate_aggregation_entry( forecast_set, include_stats, histogram ) assert new_aggregation.start_time == expected.start_time - assert ( - new_aggregation.forecast_values == expected.forecast_values - ) or np.allclose(new_aggregation.forecast_values, expected.forecast_values) assert new_aggregation.forecaster_count == expected.forecaster_count - assert ( - new_aggregation.interval_lower_bounds == expected.interval_lower_bounds - ) or np.allclose( - new_aggregation.interval_lower_bounds, expected.interval_lower_bounds - ) - assert (new_aggregation.centers == expected.centers) or np.allclose( - new_aggregation.centers, expected.centers - ) - assert ( - new_aggregation.interval_upper_bounds == expected.interval_upper_bounds - ) or np.allclose( - new_aggregation.interval_upper_bounds, expected.interval_upper_bounds - ) - assert (new_aggregation.means == expected.means) or np.allclose( - new_aggregation.means, expected.means - ) - assert (new_aggregation.histogram == expected.histogram) or np.allclose( - new_aggregation.histogram, expected.histogram - ) + for r, e in [ + (new_aggregation.forecast_values, expected.forecast_values), + (new_aggregation.interval_lower_bounds, expected.interval_lower_bounds), + (new_aggregation.centers, expected.centers), + (new_aggregation.interval_upper_bounds, expected.interval_upper_bounds), + (new_aggregation.means, expected.means), + (new_aggregation.histogram, expected.histogram), + ]: + r = np.where(np.equal(r, None), np.nan, r).astype(float) + e = np.where(np.equal(e, None), np.nan, e).astype(float) + np.testing.assert_allclose(r, e, equal_nan=True)front_end/src/components/forecast_maker/forecast_maker_question/forecast_maker_multiple_choice.tsx (1)
243-271: Align validation/defaults with current options only.
choicesForecastsnow includes historical/upcoming options, butequalizedForecast,forecastHasValues, andshowUserMustForecaststill count all options. If any non-current option isnull, users can get blocked from submitting (or see incorrect defaults/overlay gating). Derive “current choices” once and use that for defaults, validity, sums, and “must forecast”.🐛 Proposed fix (current-options derived state)
- const equalizedForecast = useMemo( - () => round(100 / choicesForecasts.length, 1), - [choicesForecasts.length] - ); - const forecastHasValues = useMemo( - () => choicesForecasts.every((el) => el.forecast !== null), - [choicesForecasts] - ); - const forecastsSum = useMemo( - () => - forecastHasValues - ? sumForecasts( - choicesForecasts.filter((choice) => - question.options.includes(choice.name) - ) - ) - : null, - [question.options, choicesForecasts, forecastHasValues] - ); + const currentChoicesForecasts = useMemo( + () => + choicesForecasts.filter((choice) => + question.options.includes(choice.name) + ), + [choicesForecasts, question.options] + ); + const equalizedForecast = useMemo( + () => round(100 / currentChoicesForecasts.length, 1), + [currentChoicesForecasts.length] + ); + const forecastHasValues = useMemo( + () => currentChoicesForecasts.every((el) => el.forecast !== null), + [currentChoicesForecasts] + ); + const forecastsSum = useMemo( + () => (forecastHasValues ? sumForecasts(currentChoicesForecasts) : null), + [currentChoicesForecasts, forecastHasValues] + ); @@ - const showUserMustForecast = - !!activeUserForecast && - activeUserForecast.forecast_values.filter((value) => value !== null) - .length < question.options.length; + const showUserMustForecast = + !!activeUserForecast && + question.options.some((option) => { + const value = forecastValueByOption.get(option); + return value === null || typeof value === "undefined"; + });
🤖 Fix all issues with AI agents
In `@front_end/messages/pt.json`:
- Around line 1834-1841: The Portuguese singular copy for
newOptionsAddedSingular currently uses plural wording; update the string for
newOptionsAddedSingular to use singular noun/verb forms (e.g., change "suas
previsões foram retiradas" style to "sua previsão" — for example "Uma nova opção
foi adicionada recentemente, por favor ajuste sua previsão de acordo.") so it
matches the singular key newOptionsAddedSingular.
In
`@front_end/src/components/forecast_maker/forecast_maker_question/forecast_maker_multiple_choice.tsx`:
- Around line 326-335: The current useMemo computing gracePeriodEnd can produce
an Invalid Date if the last timestamp in question.options_history is invalid;
update the useMemo (the gracePeriodEnd computation) to validate the last
timestamp before returning it: extract the raw timestamp from
question.options_history.at(-1)?.[0], attempt to coerce/parse it to a
number/Date, and if that results in NaN/Invalid Date (e.g.,
lastTimestep.getTime() is NaN or !isFinite(Number(raw))), return null; otherwise
continue with the existing future-check and return the valid Date. Ensure you
reference the same symbols (gracePeriodEnd, question.options_history,
lastTimestep) when making the guard.
♻️ Duplicate comments (13)
notifications/templates/emails/multiple_choice_option_deletion.mjml (1)
49-53: Localize the CTA button text.The button text "Review the question" is hardcoded; wrap it in a translation tag for i18n consistency with the rest of the template.
🌐 Suggested fix
<mj-button mj-class="cta-button" href="{% post_url params.post.post_id params.post.post_title %}"> - Review the question + {% trans "Review the question" %} </mj-button>notifications/templates/emails/multiple_choice_option_addition.html (3)
1-2: Remove debug artifact from template.Line 1 contains
<!-- FILE: undefined -->which appears to be a build/generation artifact and should be removed from the final template.🛠️ Proposed fix
-<!-- FILE: undefined --> {% load i18n %} {% load static %} {% load urls %} {% load utils %}
213-214: Localize the CTA button text.The button text "Update your forecast" is hardcoded; wrap it in a translation tag for i18n consistency.
277-288: Broken conditional logic in similar posts section.The Django template conditionals are wrapped in HTML comments (
<!-- -->), which does not prevent Django from processing them. Both thenr_forecastersspan and theprobabilityspan will render for every post regardless of condition.🐛 Proposed fix to enable conditionals
<div style="font-size: 13px; font-weight:500; color: `#2F4155`;"> - <!-- {% if post.nr_forecasters %} --> + {% if post.nr_forecasters %} <span style="display: inline-flex; align-items: center;"> <span style="margin-right: 5px;">👥</span> {{ post.nr_forecasters }} predictor{{ post.nr_forecasters|pluralize }} </span> - <!-- {% elif post.probability %} --> + {% elif post.probability %} <span style="display: inline-flex; align-items: center;"> <span style="margin-right: 5px;">📊</span> {{ post.probability }}% chance </span> - <!-- {% endif %} --> + {% endif %} </div>questions/tasks.py (3)
330-335: Pass formatted timestamp to email context.The
timesteppassed to the email context is the rawdatetimeobject, but the template likely expects a human-readable string. The formatted version (formatted_timestep) is created for the comment but not reused here.🐛 Proposed fix
"params": { "post": NotificationPostParams.from_post(post), "removed_options": removed_options, - "timestep": timestep, + "timestep": formatted_timestep, "catch_all_option": catch_all_option, },
422-427: Pass formatted timestamps to email context.Both
grace_period_endandtimestepare passed as raw datetime objects but formatted versions were created for the comment text.🐛 Proposed fix
"params": { "post": NotificationPostParams.from_post(post), "added_options": added_options, - "grace_period_end": grace_period_end, - "timestep": timestep, + "grace_period_end": formatted_grace_period_end, + "timestep": formatted_timestep, },
446-451: Missingend_timefilter retrieves wrong forecast.The
forecastersqueryset filters users by forecasts withend_time=grace_period_end, but the forecast query indefaultsdoesn't include this filter. This retrieves the most recent forecast instead of the one that triggered the notification, and.first()returningNonewould cause anIntegrityErrorsinceUserForecastNotification.forecastis non-nullable.🐛 Proposed fix
defaults={ "trigger_time": grace_period_end - timedelta(days=1), "email_sent": False, "forecast": Forecast.objects.filter( - question=question, author=forecaster + question=question, author=forecaster, end_time=grace_period_end ) .order_by("-start_time") .first(), },questions/services/multiple_choice_handlers.py (3)
126-135: Missingquestion.save()after rename.The function modifies
question.optionsandquestion.options_historyin place but doesn't persist the changes to the database.🐛 Proposed fix
for i, (timestr, options) in enumerate(question.options_history): question.options_history[i] = ( timestr, [new_option if opt == old_option else opt for opt in options], ) + question.save() return question
290-297: Invoke Dramatiq actors via.send()for async execution.
multiple_choice_delete_option_notificationsis a Dramatiq actor but is called directly, which executes synchronously and blocks the request. The codebase pattern is to use.send()for async execution.🐛 Proposed fix
- multiple_choice_delete_option_notifications( + multiple_choice_delete_option_notifications.send( question_id=question.id, timestep=timestep, comment_author_id=comment_author.id, comment_text=comment_text, )
367-373: Invoke Dramatiq actor via.send()for async execution.Same issue as the delete notifications - this should use
.send()for async execution.🐛 Proposed fix
- multiple_choice_add_option_notifications( + multiple_choice_add_option_notifications.send( question_id=question.id, grace_period_end=grace_period_end, timestep=timestep, comment_author_id=comment_author.id, comment_text=comment_text, )front_end/messages/en.json (1)
43-46: Tighten singular/plural wording in new-option messages.These strings still use “forecast(s)” and mismatched pluralization. Suggest aligning singular/plural wording.
✏️ Suggested copy tweak
- "newOptionsAddedPlural": "These options were recently added, please adjust your forecast(s) accordingly.", - "newOptionsAddedSingular": "A new option was recently added, please adjust your forecasts accordingly.", + "newOptionsAddedPlural": "These options were recently added; please adjust your forecasts accordingly.", + "newOptionsAddedSingular": "A new option was recently added; please adjust your forecast accordingly.",front_end/messages/cs.json (1)
1844-1844: Fix Czech plural form for "days".The
otherplural form# dnishould be# dníin Czech. The genitive plural of "den" (day) is "dní", not "dni".🛠️ Suggested fix
- "periodDays": "{count, plural, one {# den} few {# dny} other {# dni}}", + "periodDays": "{count, plural, one {# den} few {# dny} other {# dní}}",front_end/src/components/forecast_maker/forecast_maker_question/forecast_maker_multiple_choice.tsx (1)
723-735: Localize deleted/upcoming suffixes.The
"(deleted)"/"(upcoming)"suffixes are hard-coded English strings. These are user-visible and should be localized.🌍 One way to localize (status + translated label)
type ChoiceOption = { name: string; - label?: string; + status?: "deleted" | "upcoming"; communityForecast: number | null; forecast: number | null; color: ThemeColor; }; @@ - return ( + const choiceLabel = + choice.status === "deleted" + ? t("optionDeletedLabel", { option: choice.name }) + : choice.status === "upcoming" + ? t("optionUpcomingLabel", { option: choice.name }) + : choice.name; + return ( <ForecastChoiceOption @@ - choiceName={choice.label ?? choice.name} + choiceName={choiceLabel} @@ - return { - name: option, - label: isDeleted - ? option + " (deleted)" - : isUpcoming - ? option + " (upcoming)" - : option, + return { + name: option, + status: isDeleted ? "deleted" : isUpcoming ? "upcoming" : undefined,
🧹 Nitpick comments (9)
front_end/src/app/(main)/questions/components/question_form.tsx (1)
786-798: Consider adding visual styling to indicate the locked state.The
readOnlyattribute correctly prevents input, and the explanation text is helpful. However, the inputs have no visual differentiation when locked, which may confuse users who try to edit before reading the explanation.💡 Suggestion: Add conditional styling for locked inputs
<Input {...form.register(`options.${opt_index}`)} readOnly={optionsLocked} - className="my-2 w-full min-w-32 rounded border border-gray-500 p-2 px-3 py-2 text-base dark:border-gray-500-dark dark:bg-blue-50-dark" + className={cn( + "my-2 w-full min-w-32 rounded border border-gray-500 p-2 px-3 py-2 text-base dark:border-gray-500-dark dark:bg-blue-50-dark", + optionsLocked && "cursor-not-allowed bg-gray-100 dark:bg-gray-700-dark" + )} value={option}This would require importing
cnfrom the utility (already available viaclsx).tests/unit/test_utils/test_the_math/test_aggregations.py (1)
105-111: Add explicitzip(..., strict=...)for the semi‑stddev test loops.Ruff B905 flags this, and strictness catches silent length mismatches in tests.
🛠️ Proposed change
- for v, e in zip(rl, el): + for v, e in zip(rl, el, strict=True): np.testing.assert_approx_equal(v, e) - for v, e in zip(ru, eu): + for v, e in zip(ru, eu, strict=True): np.testing.assert_approx_equal(v, e)Please confirm the project’s minimum Python version supports
zip(..., strict=...)(3.10+).front_end/src/utils/questions/choices.ts (1)
119-123: Label suffixes are not localized.The hardcoded strings
" (deleted)"and" (upcoming)"should use i18n utilities for translation consistency with the rest of the application.questions/tasks.py (2)
323-339: Synchronous email sending may block the task worker.
use_async=Falsecauses email sending to block the current Dramatiq worker. Since these are already async tasks, consider usinguse_async=True(the default) to avoid blocking the worker thread while waiting for SMTP operations, especially when sending to multiple recipients.
437-439: Redundant delete before update_or_create.The
filter(...).delete()on line 437-439 is unnecessary sinceupdate_or_createwill update existing records. This adds an extra database round-trip.♻️ Suggested refactor
if grace_period_end - timedelta(days=1) > timestep: for forecaster in forecasters: - UserForecastNotification.objects.filter( - user=forecaster, question=question - ).delete() # is this necessary? UserForecastNotification.objects.update_or_create(questions/services/multiple_choice_handlers.py (1)
174-178: Considerbulk_updatefor forecast reordering.The loop saves each forecast individually, resulting in O(n) database calls. For questions with many forecasts, this could be slow.
♻️ Suggested refactor
remap = [all_options_ever.index(option) for option in new_options_order] - for forecast in question.user_forecasts.all(): + forecasts = list(question.user_forecasts.all()) + for forecast in forecasts: forecast.probability_yes_per_category = [ forecast.probability_yes_per_category[i] for i in remap ] - forecast.save() + Forecast.objects.bulk_update(forecasts, ["probability_yes_per_category"])questions/serializers/common.py (1)
684-699: Pre-registered forecast handling mutates model instances.The code still modifies
last_forecast.start_timeanduser_forecasts[-1].end_timedirectly on cached model instances. While the mutation was simplified, if these objects are accessed elsewhere after serialization, they will have modified values.Consider documenting that this mutation is intentional and scoped to serialization, or create copies to avoid side effects.
tests/unit/test_questions/test_services/test_multiple_choice_handlers.py (1)
271-277: Addzip(..., strict=True)for safer iteration.While the code asserts matching lengths beforehand, adding
strict=Truemakes that guarantee explicit at iteration time. The project's Python version (3.12.3) fully supports this parameter.♻️ Proposed diff
- for f, e in zip(forecasts, expected_forecasts): + for f, e in zip(forecasts, expected_forecasts, strict=True): assert f.start_time == e.start_time assert f.end_time == e.end_time assert f.probability_yes_per_category == e.probability_yes_per_category assert f.source == e.sourceAlso applies to: 452-456
front_end/src/components/forecast_maker/forecast_maker_question/forecast_maker_multiple_choice.tsx (1)
339-353: Clear highlight timeout on unmount / repeated calls.The
setTimeoutcan fire after unmount or overlap if triggered repeatedly. Track and clear it to avoid state updates after unmount.🧹 Cleanup approach
const firstNewOptionRef = useRef<HTMLTableRowElement | null>(null); + const highlightTimeoutRef = useRef<number | null>(null); @@ const scrollToNewOptions = () => { if (firstNewOptionRef.current) { // Trigger animation immediately setIsAnimatingHighlight(true); + if (highlightTimeoutRef.current !== null) { + window.clearTimeout(highlightTimeoutRef.current); + } @@ - setTimeout(() => { + highlightTimeoutRef.current = window.setTimeout(() => { setIsAnimatingHighlight(false); }, ANIMATION_DURATION_MS); } }; + + useEffect(() => { + return () => { + if (highlightTimeoutRef.current !== null) { + window.clearTimeout(highlightTimeoutRef.current); + } + }; + }, []);
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (22)
front_end/messages/cs.jsonfront_end/messages/en.jsonfront_end/messages/es.jsonfront_end/messages/pt.jsonfront_end/messages/zh-TW.jsonfront_end/messages/zh.jsonfront_end/src/app/(main)/questions/[id]/components/multiple_choices_chart_view/choices_legend/index.tsxfront_end/src/app/(main)/questions/components/question_form.tsxfront_end/src/components/detailed_question_card/detailed_question_card/multiple_choice_chart_card.tsxfront_end/src/components/forecast_maker/forecast_maker_question/forecast_maker_multiple_choice.tsxfront_end/src/types/choices.tsfront_end/src/utils/questions/choices.tsfront_end/src/utils/questions/helpers.tsnotifications/templates/emails/multiple_choice_option_addition.htmlnotifications/templates/emails/multiple_choice_option_addition.mjmlnotifications/templates/emails/multiple_choice_option_deletion.htmlnotifications/templates/emails/multiple_choice_option_deletion.mjmlquestions/serializers/common.pyquestions/services/multiple_choice_handlers.pyquestions/tasks.pytests/unit/test_questions/test_services/test_multiple_choice_handlers.pytests/unit/test_utils/test_the_math/test_aggregations.py
🚧 Files skipped from review as they are similar to previous changes (8)
- front_end/src/utils/questions/helpers.ts
- notifications/templates/emails/multiple_choice_option_deletion.html
- front_end/src/types/choices.ts
- front_end/src/components/detailed_question_card/detailed_question_card/multiple_choice_chart_card.tsx
- front_end/src/app/(main)/questions/[id]/components/multiple_choices_chart_view/choices_legend/index.tsx
- front_end/messages/es.json
- notifications/templates/emails/multiple_choice_option_addition.mjml
- front_end/messages/zh.json
🧰 Additional context used
🧠 Learnings (3)
📚 Learning: 2026-01-15T19:29:58.940Z
Learnt from: hlbmtc
Repo: Metaculus/metaculus PR: 4075
File: authentication/urls.py:24-26
Timestamp: 2026-01-15T19:29:58.940Z
Learning: In this codebase, DRF is configured to use IsAuthenticated as the default in REST_FRAMEWORK['DEFAULT_PERMISSION_CLASSES'] within metaculus_web/settings.py. Therefore, explicit permission_classes([IsAuthenticated]) decorators are unnecessary on DRF views unless a view needs to override the default. When reviewing Python files, verify that views relying on the default are not redundantly decorated, and flag cases where permissions are being over-specified or when a non-default permission is explicitly required.
Applied to files:
tests/unit/test_utils/test_the_math/test_aggregations.pyquestions/serializers/common.pyquestions/tasks.pytests/unit/test_questions/test_services/test_multiple_choice_handlers.pyquestions/services/multiple_choice_handlers.py
📚 Learning: 2026-01-16T20:44:06.504Z
Learnt from: lsabor
Repo: Metaculus/metaculus PR: 4038
File: scoring/score_math.py:90-92
Timestamp: 2026-01-16T20:44:06.504Z
Learning: Multiple choice questions in the Metaculus codebase are constrained to always have at least two options; therefore, when scoring multiple choice forecasts, `options_at_time` (the count of non-None values in the PMF) will always be ≥ 2, and division by `np.log(options_at_time)` is safe.
Applied to files:
front_end/src/components/forecast_maker/forecast_maker_question/forecast_maker_multiple_choice.tsxquestions/serializers/common.pyquestions/services/multiple_choice_handlers.py
📚 Learning: 2026-01-16T20:41:15.295Z
Learnt from: lsabor
Repo: Metaculus/metaculus PR: 4038
File: questions/services/multiple_choice_handlers.py:138-186
Timestamp: 2026-01-16T20:41:15.295Z
Learning: In questions/services/multiple_choice_handlers.py, the catch-all option position is validated upstream in MultipleChoiceOptionsUpdateSerializer.validate_new_options, which enforces that the last option (catch-all) cannot change during add/delete operations. Reordering the catch-all is intentionally allowed in multiple_choice_reorder_options when options have never changed (len(options_history) == 1), as all options are equal at that stage.
Applied to files:
questions/serializers/common.pytests/unit/test_questions/test_services/test_multiple_choice_handlers.pyquestions/services/multiple_choice_handlers.py
🧬 Code graph analysis (5)
front_end/src/app/(main)/questions/components/question_form.tsx (2)
front_end/src/components/ui/input_container.tsx (1)
InputContainer(13-67)front_end/src/components/ui/form_field.tsx (1)
Input(111-128)
tests/unit/test_utils/test_the_math/test_aggregations.py (1)
utils/the_math/aggregations.py (3)
compute_weighted_semi_standard_deviations(115-141)ForecastSet(52-56)calculate_aggregation_entry(614-668)
front_end/src/utils/questions/choices.ts (2)
front_end/src/utils/questions/helpers.ts (2)
getAllOptionsHistory(203-217)getUpcomingOptions(219-232)front_end/src/types/choices.ts (1)
ChoiceItem(8-32)
questions/serializers/common.py (1)
questions/services/multiple_choice_handlers.py (1)
get_all_options_from_history(83-105)
questions/tasks.py (15)
posts/admin.py (2)
comments(259-260)author(246-247)tests/unit/test_comments/test_views.py (4)
comments(29-71)post(238-239)post(578-579)post(643-644)comments/services/common.py (1)
create_comment(78-130)front_end/src/types/question.ts (2)
Forecast(119-119)Question(266-311)questions/models.py (3)
Forecast(576-712)Question(60-448)get_forecasters(415-422)questions/services/common.py (1)
get_outbound_question_links(275-280)questions/admin.py (2)
forecasts(469-470)author(472-473)questions/services/forecasts.py (2)
build_question_forecasts(420-461)get_forecasts_per_user(367-379)scoring/utils.py (1)
score_question(85-129)utils/email.py (1)
send_email_with_template(13-50)tests/unit/test_comments/test_services.py (1)
post(26-34)posts/models.py (1)
get_forecasters(914-921)notifications/constants.py (1)
MailingTags(4-11)notifications/services.py (2)
NotificationPostParams(46-68)from_post(63-68)projects/models.py (1)
delete(610-614)
🪛 Ruff (0.14.13)
tests/unit/test_utils/test_the_math/test_aggregations.py
108-108: zip() without an explicit strict= parameter
Add explicit value for parameter strict=
(B905)
110-110: zip() without an explicit strict= parameter
Add explicit value for parameter strict=
(B905)
questions/serializers/common.py
243-248: Avoid specifying long messages outside the exception class
(TRY003)
468-470: Avoid specifying long messages outside the exception class
(TRY003)
472-472: Avoid specifying long messages outside the exception class
(TRY003)
474-476: Avoid specifying long messages outside the exception class
(TRY003)
479-481: Avoid specifying long messages outside the exception class
(TRY003)
488-490: Avoid specifying long messages outside the exception class
(TRY003)
492-494: Avoid specifying long messages outside the exception class
(TRY003)
questions/tasks.py
302-302: Do not catch blind exception: Exception
(BLE001)
390-390: Do not catch blind exception: Exception
(BLE001)
tests/unit/test_questions/test_services/test_multiple_choice_handlers.py
273-273: zip() without an explicit strict= parameter
Add explicit value for parameter strict=
(B905)
452-452: zip() without an explicit strict= parameter
Add explicit value for parameter strict=
(B905)
questions/services/multiple_choice_handlers.py
37-37: Avoid specifying long messages outside the exception class
(TRY003)
39-39: Avoid specifying long messages outside the exception class
(TRY003)
42-42: Avoid specifying long messages outside the exception class
(TRY003)
44-44: Avoid specifying long messages outside the exception class
(TRY003)
46-48: Avoid specifying long messages outside the exception class
(TRY003)
51-53: Avoid specifying long messages outside the exception class
(TRY003)
55-55: Avoid specifying long messages outside the exception class
(TRY003)
57-59: Avoid specifying long messages outside the exception class
(TRY003)
64-64: Avoid specifying long messages outside the exception class
(TRY003)
67-67: Avoid specifying long messages outside the exception class
(TRY003)
72-75: Avoid specifying long messages outside the exception class
(TRY003)
98-98: Avoid specifying long messages outside the exception class
(TRY003)
105-105: Consider [*all_labels, designated_other_label] instead of concatenation
Replace with [*all_labels, designated_other_label]
(RUF005)
118-118: Avoid specifying long messages outside the exception class
(TRY003)
120-120: Avoid specifying long messages outside the exception class
(TRY003)
122-122: Avoid specifying long messages outside the exception class
(TRY003)
124-124: Avoid specifying long messages outside the exception class
(TRY003)
152-152: Avoid specifying long messages outside the exception class
(TRY003)
154-154: Avoid specifying long messages outside the exception class
(TRY003)
156-156: Avoid specifying long messages outside the exception class
(TRY003)
158-158: Avoid specifying long messages outside the exception class
(TRY003)
162-162: Avoid specifying long messages outside the exception class
(TRY003)
210-210: Avoid specifying long messages outside the exception class
(TRY003)
214-214: Avoid specifying long messages outside the exception class
(TRY003)
216-216: Avoid specifying long messages outside the exception class
(TRY003)
224-224: Avoid specifying long messages outside the exception class
(TRY003)
243-246: Avoid specifying long messages outside the exception class
(TRY003)
248-248: zip() without an explicit strict= parameter
Add explicit value for parameter strict=
(B905)
321-321: Avoid specifying long messages outside the exception class
(TRY003)
323-323: Avoid specifying long messages outside the exception class
(TRY003)
325-325: Avoid specifying long messages outside the exception class
(TRY003)
328-328: Avoid specifying long messages outside the exception class
(TRY003)
335-335: Avoid specifying long messages outside the exception class
(TRY003)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: Frontend Checks
- GitHub Check: Backend Checks
- GitHub Check: integration-tests
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (21)
front_end/src/app/(main)/questions/components/question_form.tsx (3)
312-312: LGTM — Correct gating logic for option locking.The derived flag correctly locks options only when editing an existing question that has forecasts, allowing full editing freedom during creation. This aligns with the PR objective to disable option edits once forecasts exist.
801-813: Defensive guard is appropriate.While
readOnlyshould prevent user input, the early return serves as a defense-in-depth measure. This pattern correctly ensures state consistency even in edge cases wherereadOnlymight not fully prevent change events.
820-847: LGTM — Add/delete controls properly gated.The delete button visibility and add button disabled state are correctly tied to
optionsLocked, preventing any option modifications when the question has forecasts in edit mode.notifications/templates/emails/multiple_choice_option_deletion.mjml (1)
1-14: Template structure and i18n look good.The template correctly uses
blocktransfor translatable content, includes appropriate styling, and follows the established pattern for MJML email templates in this codebase.front_end/src/utils/questions/choices.ts (2)
39-48: LGTM on history-based ordering.The choice ordering logic correctly derives from
allOptions(full history) and sorts by latest forecast values, maintaining consistent ordering across the UI.
73-89: The forecast value indexing is correctly aligned. Forecast values are stored in the backend indexed by the "all options ever" order (confirmed by themultiple_choice_reorder_optionsfunction which explicitly remapsprobability_yes_per_categoryusingall_options_everas the stable index), and the frontend code correctly retrieves and indexes this data usinggetAllOptionsHistory()which returns options in the same order.questions/services/multiple_choice_handlers.py (2)
261-264: Unreachable code branch.The condition
if forecast.start_time >= timestepon line 261 can never be true because the queryset filter on lines 235-236 explicitly selects forecasts withstart_time__lt=timestepis not applied. However, looking at the filter on lines 235-237, it only filters byend_time, notstart_time.Wait - re-examining: the filter is
Q(end_time__isnull=True) | Q(end_time__gt=timestep)which doesn't filter onstart_time. So forecasts withstart_time >= timestepcould still be included. This branch may be reachable.
17-80: Validation logic looks comprehensive.The
MultipleChoiceOptionsUpdateSerializerproperly validates renaming, deletion, and addition scenarios with appropriate guards for grace periods, option counts, and catch-all position constraints.questions/serializers/common.py (4)
235-250: LGTM - validation now properly raises.The validation correctly raises
ValidationErrorwhen attempting to update options through the standard endpoint while user forecasts exist, guiding users to the dedicated MC options update endpoint.
461-503: History-aware validation logic is well-structured.The updated
multiple_choice_validationproperly:
- Validates that current options are a subset of the submitted forecast
- Uses
get_all_options_from_historyto derive all valid options- Enforces value constraints (0.001-0.999) for current options only
- Requires
Nonefor inactive options- Validates the sum equals 1.0
127-130: LGTM - properly exposes options history.The new
get_all_options_evermethod correctly derives the full options list from history when available, enabling frontend components to render historical options appropriately.
871-874: Resolution validation correctly uses full options history.Using
get_all_options_from_historyensures that questions can be resolved to any option that ever existed, not just current options. This aligns with the requirement to maintain scoring integrity for deleted options.tests/unit/test_questions/test_services/test_multiple_choice_handlers.py (2)
17-40: Rename-option test cases cover success and failure paths.Nice coverage of the valid rename and the two invalid inputs.
43-83: Reorder-option test validates forecast remapping.Good assertion that probabilities follow the new option order.
front_end/messages/pt.json (1)
1883-1887: Period unit pluralization keys look consistent.front_end/messages/en.json (1)
955-955:choicesLockedHelpcopy is clear and direct.front_end/messages/cs.json (3)
942-942: LGTM!The Czech translation for
choicesLockedHelpis clear and accurate.
1837-1843: LGTM!The Czech translations for the grace period and new options UI strings are accurate and idiomatic.
1845-1848: LGTM!The Czech plural forms for hours, minutes, seconds, and the
othersCountstring are correct.front_end/messages/zh-TW.json (2)
1830-1836: LGTM!The Traditional Chinese translations for the grace period and new options UI strings are accurate and natural.
1882-1886: LGTM!The Traditional Chinese translations for period units and
choicesLockedHelpare correct. The ICU plural format appropriately usesone/othercategories which is suitable for Chinese.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
...end/src/components/forecast_maker/forecast_maker_question/forecast_maker_multiple_choice.tsx
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Fix all issues with AI agents
In
`@front_end/src/components/forecast_maker/forecast_maker_question/forecast_maker_multiple_choice.tsx`:
- Around line 570-610: The map over choicesForecasts currently returns nothing
when question.options.includes(choice.name) is false, causing undefined entries;
fix by filtering choicesForecasts first (e.g., choicesForecasts.filter(c =>
question.options.includes(c.name)).map(...)) or alternatively return null for
non-matching entries and remove falsy values before rendering so the
ForecastChoiceOption list contains only valid elements; update the block around
choicesForecasts.map and keep references like ForecastChoiceOption,
question.options.includes, firstNewOptionName, newOptionNames,
interactedOptions, and firstNewOptionRef so behavior and props remain unchanged.
In `@front_end/src/utils/questions/choices.ts`:
- Around line 119-123: The label generation currently appends hardcoded English
suffixes "(deleted)" and "(upcoming)" using isDeleted/isUpcoming and choice;
replace these hardcoded strings with calls into the app's i18n/translation layer
(e.g., use t('deleted') / t('upcoming') or a helper like translateStatus) so the
suffixes are localized before concatenating with choice (update the code around
the label assignment that references isDeleted, isUpcoming, and choice to use
the translated suffixes).
In `@questions/admin.py`:
- Around line 661-675: The branch handling ACTION_CHANGE_GRACE calls
multiple_choice_change_grace_period_end which currently raises
NotImplementedError; either remove this branch or implement the function so
toggling ACTION_CHANGE_GRACE in ACTION_CHOICES won't crash. Locate
multiple_choice_change_grace_period_end and implement the actual grace-period
update logic (accept question, new_grace_period_end, comment_author, timestep),
ensure it updates question.options_history or related fields consistently, and
keep the existing question.save(update_fields=["options_history"]) and
message_user call; alternatively remove the ACTION_CHANGE_GRACE case from the
admin action switch and from ACTION_CHOICES to eliminate the dead code path.
In `@tests/unit/test_utils/test_the_math/test_aggregations.py`:
- Around line 610-655: The RecencyWeightedAggregation test assertions should be
made placeholder-safe: in test_RecencyWeightedAggregation (which constructs
RecencyWeightedAggregation and compares its output AggregateForecast to
expected), replace direct equality/np.allclose checks with the same loop pattern
used in UnweightedAggregation—map None to np.nan for list-like fields
(forecast_values, interval_lower_bounds, centers, interval_upper_bounds, means)
and use np.testing.assert_allclose(..., equal_nan=True) for numeric comparisons;
also assert forecaster_count and method/ start_time with standard equality
checks. This ensures None placeholders are treated as NaN and comparisons don’t
fail.
♻️ Duplicate comments (16)
notifications/templates/emails/multiple_choice_option_deletion.mjml (1)
49-53: Localize the CTA label.
Hardcoded CTA text should use the template translation tag for i18n consistency.🔧 Suggested fix
- <mj-button mj-class="cta-button" href="{% post_url params.post.post_id params.post.post_title %}"> - Review the question - </mj-button> + <mj-button mj-class="cta-button" href="{% post_url params.post.post_id params.post.post_title %}"> + {% trans "Review the question" %} + </mj-button>front_end/messages/es.json (1)
1836-1839: Fix singular/plural wording in ES new-options copy.💡 Suggested phrasing
- "newOptionsAddedPlural": "Estas opciones se añadieron recientemente, por favor ajuste su(s) pronóstico(s) en consecuencia.", - "newOptionsAddedSingular": "Se añadió una nueva opción recientemente, por favor ajuste sus pronósticos en consecuencia.", + "newOptionsAddedPlural": "Se añadieron nuevas opciones recientemente; por favor ajuste sus pronósticos en consecuencia.", + "newOptionsAddedSingular": "Se añadió una nueva opción recientemente; por favor ajuste su pronóstico en consecuencia.",front_end/messages/cs.json (1)
1844-1845: Fix Czech plural form for “days”.🛠️ Suggested fix
- "periodDays": "{count, plural, one {# den} few {# dny} other {# dni}}", + "periodDays": "{count, plural, one {# den} few {# dny} other {# dní}}",front_end/messages/pt.json (1)
1836-1837: Use singular phrasing fornewOptionsAddedSingular.The singular message still uses plural wording (“suas previsões”). Please switch to singular to match the key.
✏️ Proposed fix
- "newOptionsAddedSingular": "Uma nova opção foi adicionada recentemente, por favor ajuste suas previsões de acordo.", + "newOptionsAddedSingular": "Uma nova opção foi adicionada recentemente, por favor ajuste sua previsão de acordo.",notifications/templates/emails/multiple_choice_option_addition.html (3)
1-2: Remove the build artifact header.This looks like a generation artifact and should be removed.
🧹 Proposed fix
-<!-- FILE: undefined --> {% load i18n %} {% load static %} {% load urls %} {% load utils %}
212-213: Localize the CTA label.The CTA text is still hardcoded in English; please wrap it in a translation tag.
🌐 Proposed fix
- <a href="{% post_url params.post.post_id params.post.post_title %}" style="display: inline-block; background: `#758EA9`; color: `#ffffff`; font-family: Inter Variable, Inter, Arial, sans-serif; font-size: 16px; font-weight: 600; line-height: 120%; margin: 0; text-decoration: none; text-transform: none; padding: 8px 24px; mso-padding-alt: 0px; border-radius: 6px;" target="_blank"> Update your forecast </a> + <a href="{% post_url params.post.post_id params.post.post_title %}" style="display: inline-block; background: `#758EA9`; color: `#ffffff`; font-family: Inter Variable, Inter, Arial, sans-serif; font-size: 16px; font-weight: 600; line-height: 120%; margin: 0; text-decoration: none; text-transform: none; padding: 8px 24px; mso-padding-alt: 0px; border-radius: 6px;" target="_blank"> {% trans "Update your forecast" %} </a>
254-287: Fix Django conditionals commented out by HTML.HTML comments don’t disable template tags, so both spans render. Use real Django
{% if %}/{% elif %}/{% endif %}.🐛 Proposed fix
- <!-- {% if post.nr_forecasters %} --> + {% if post.nr_forecasters %} <span style="display: inline-flex; align-items: center;"> <span style="margin-right: 5px;">👥</span> {{ post.nr_forecasters }} predictor{{ post.nr_forecasters|pluralize }} </span> - <!-- {% elif post.probability %} --> + {% elif post.probability %} <span style="display: inline-flex; align-items: center;"> <span style="margin-right: 5px;">📊</span> {{ post.probability }}% chance </span> - <!-- {% endif %} --> + {% endif %}notifications/templates/emails/multiple_choice_option_deletion.html (3)
1-2: Remove the build artifact header.This looks like a generation artifact and should be removed.
🧹 Proposed fix
-<!-- FILE: undefined --> {% load i18n %} {% load static %} {% load urls %} {% load utils %}
211-213: Localize the CTA label.The CTA text is still hardcoded in English; please wrap it in a translation tag.
🌐 Proposed fix
- <a href="{% post_url params.post.post_id params.post.post_title %}" style="display: inline-block; background: `#758EA9`; color: `#ffffff`; font-family: Inter Variable, Inter, Arial, sans-serif; font-size: 16px; font-weight: 600; line-height: 120%; margin: 0; text-decoration: none; text-transform: none; padding: 8px 24px; mso-padding-alt: 0px; border-radius: 6px;" target="_blank"> Review the question </a> + <a href="{% post_url params.post.post_id params.post.post_title %}" style="display: inline-block; background: `#758EA9`; color: `#ffffff`; font-family: Inter Variable, Inter, Arial, sans-serif; font-size: 16px; font-weight: 600; line-height: 120%; margin: 0; text-decoration: none; text-transform: none; padding: 8px 24px; mso-padding-alt: 0px; border-radius: 6px;" target="_blank"> {% trans "Review the question" %} </a>
277-287: Fix Django conditionals commented out by HTML.HTML comments don’t disable template tags, so both spans render. Use real Django
{% if %}/{% elif %}/{% endif %}.🐛 Proposed fix
- <!-- {% if post.nr_forecasters %} --> + {% if post.nr_forecasters %} <span style="display: inline-flex; align-items: center;"> <span style="margin-right: 5px;">👥</span> {{ post.nr_forecasters }} predictor{{ post.nr_forecasters|pluralize }} </span> - <!-- {% elif post.probability %} --> + {% elif post.probability %} <span style="display: inline-flex; align-items: center;"> <span style="margin-right: 5px;">📊</span> {{ post.probability }}% chance </span> - <!-- {% endif %} --> + {% endif %}questions/tasks.py (2)
328-334: Pass formatted timestamps to email context.You already compute
formatted_timestepandformatted_grace_period_end, but the email params still use raw datetimes.🛠️ Proposed fix
context={ "email_subject_display": "Multiple choice option removed", "similar_posts": [], "params": { "post": NotificationPostParams.from_post(post), "removed_options": removed_options, - "timestep": timestep, + "timestep": formatted_timestep, "catch_all_option": catch_all_option, }, },context={ "email_subject_display": "Multiple choice options added", "similar_posts": [], "params": { "post": NotificationPostParams.from_post(post), "added_options": added_options, - "grace_period_end": grace_period_end, - "timestep": timestep, + "grace_period_end": formatted_grace_period_end, + "timestep": formatted_timestep, }, },Also applies to: 423-427
446-450: Use the forecast that actually ends at the grace period.The follow-up notification picks the latest forecast without filtering by
end_time, which can associate the wrong forecast (or none). Filter byend_time=grace_period_endand skip if missing.🛠️ Proposed fix
- for forecaster in forecasters: - UserForecastNotification.objects.filter( - user=forecaster, question=question - ).delete() # is this necessary? - UserForecastNotification.objects.update_or_create( - user=forecaster, - question=question, - defaults={ - "trigger_time": grace_period_end - timedelta(days=1), - "email_sent": False, - "forecast": Forecast.objects.filter( - question=question, author=forecaster - ) - .order_by("-start_time") - .first(), - }, - ) + for forecaster in forecasters: + UserForecastNotification.objects.filter( + user=forecaster, question=question + ).delete() # is this necessary? + forecast = ( + Forecast.objects.filter( + question=question, + author=forecaster, + end_time=grace_period_end, + ) + .order_by("-start_time") + .first() + ) + if not forecast: + continue + UserForecastNotification.objects.update_or_create( + user=forecaster, + question=question, + defaults={ + "trigger_time": grace_period_end - timedelta(days=1), + "email_sent": False, + "forecast": forecast, + }, + )front_end/src/components/forecast_maker/forecast_maker_question/forecast_maker_multiple_choice.tsx (1)
326-335: Guard against invalid grace-period timestamps.The
gracePeriodEndcalculation can produce anInvalid Dateifquestion.options_history.at(-1)?.[0]is malformed. This can leak into the countdown/tooltip. ANaNcheck would prevent runtime issues.🛡️ Suggested guard
const gracePeriodEnd = useMemo(() => { if (!question.options_history || question.options_history.length === 0) { return null; } - const lastTimestep = new Date(question.options_history.at(-1)?.[0] || 0); - if (new Date().getTime() < lastTimestep.getTime()) { - return null; - } - return lastTimestep; + const lastTimestamp = question.options_history.at(-1)?.[0]; + if (!lastTimestamp) return null; + const lastTimestep = new Date(lastTimestamp); + if (Number.isNaN(lastTimestep.getTime())) return null; + // Return null if grace period is in the future (still active) + if (new Date().getTime() < lastTimestep.getTime()) { + return null; + } + return lastTimestep; }, [question.options_history]);questions/services/multiple_choice_handlers.py (1)
108-135: Missingquestion.save()after rename.The function modifies
question.optionsandquestion.options_historyin place but doesn't persist the changes to the database. Callers would need to explicitly save, which is inconsistent with other handlers that callsave().🐛 Proposed fix
for i, (timestr, options) in enumerate(question.options_history): question.options_history[i] = ( timestr, [new_option if opt == old_option else opt for opt in options], ) + question.save() return questionquestions/admin.py (1)
199-202: Timezone display may not match help text claim of UTC.
timezone.localtime(grace_initial)without atimezoneargument converts to the server's configured timezone, not UTC. However, the help text on line 201 states "Time selection is in UTC," which may cause confusion if the server timezone differs from UTC.🛠️ Suggested fix
- grace_field.initial = timezone.localtime(grace_initial) - if self.options_grace_period_end: - grace_field.help_text = "Time selection is in UTC." + from datetime import timezone as dt_timezone + grace_field.initial = timezone.localtime(grace_initial, timezone=dt_timezone.utc) + if self.options_grace_period_end: + grace_field.help_text = "Time selection is in UTC."Alternatively, remove the UTC claim from the help text if server-local display is intentional.
questions/serializers/common.py (1)
686-699: In-place mutation of forecast object may cause side effects.Lines 696 and 698 mutate
last_forecast.start_timeanduser_forecasts[-1].end_timedirectly. Since these come fromquestion.request_user_forecasts, this modifies cached model instances. If accessed elsewhere after serialization, they will have modified values.The commented-out lines 694-695 suggest this was considered. The current approach on line 696 (
user_forecasts[-1].end_time = last_forecast.end_time) still mutates a cached object.Consider documenting this intentional mutation or creating copies for display purposes only.
🧹 Nitpick comments (8)
tests/unit/test_utils/test_the_math/test_aggregations.py (1)
105-111: Avoid silent truncation when comparing result vs expected.
zipwill drop extra elements if lengths diverge; add an explicit length check so mismatches fail loudly.♻️ Proposed fix
result = compute_weighted_semi_standard_deviations(forecasts_values, weights) rl, ru = result el, eu = expected + assert len(rl) == len(el) for v, e in zip(rl, el): np.testing.assert_approx_equal(v, e) + assert len(ru) == len(eu) for v, e in zip(ru, eu): np.testing.assert_approx_equal(v, e)front_end/src/components/forecast_maker/forecast_maker_question/forecast_maker_multiple_choice.tsx (2)
599-605: Ternary expression with side effect returnsundefined.The ternary
isNewOption ? setInteractedOptions(...) : undefinedis unconventional. Consider using anifstatement or early return for clarity.♻️ Suggested improvement
- onInteraction={() => { - isNewOption - ? setInteractedOptions((prev) => - new Set(prev).add(choice.name) - ) - : undefined; - }} + onInteraction={() => { + if (isNewOption) { + setInteractedOptions((prev) => + new Set(prev).add(choice.name) + ); + } + }}
746-748: RedundantfindIndexcallback.The callback
(_, index) => allOptions[index] === question.resolutioncan be simplified to directly compare the element.♻️ Suggested fix
- const resolutionIndex = allOptions.findIndex( - (_, index) => allOptions[index] === question.resolution - ); + const resolutionIndex = allOptions.findIndex( + (option) => option === question.resolution + );questions/services/multiple_choice_handlers.py (1)
248-248: Addstrict=Truetozip()for safety.Using
zip()withoutstrict=Truesilently truncates ifprevious_pmfandall_optionshave different lengths. Since line 242-246 already validates lengths match, addingstrict=Trueprovides an extra safety net and makes intent explicit.♻️ Suggested fix
- for value, label in zip(previous_pmf, all_options): + for value, label in zip(previous_pmf, all_options, strict=True):tests/unit/test_questions/test_services/test_multiple_choice_handlers.py (2)
273-277: Addstrict=Truetozip()in test assertions.Without
strict=True, ifforecastsandexpected_forecastshave different lengths, the assertion loop will silently pass without checking all items. While the length assertion on line 272 catches this, usingstrict=Truemakes the intent clearer and provides a secondary safeguard.♻️ Suggested fix
- for f, e in zip(forecasts, expected_forecasts): + for f, e in zip(forecasts, expected_forecasts, strict=True):
452-455: Addstrict=Truetozip()in test assertions.Same issue as above - the zip should use
strict=Truefor safer assertions.♻️ Suggested fix
- for f, e in zip(forecasts, expected_forecasts): + for f, e in zip(forecasts, expected_forecasts, strict=True):questions/admin.py (2)
294-297: Useraise ... from excfor proper exception chaining.When re-raising a
forms.ValidationErrorfrom a caughtDRFValidationError, usefrom excto preserve the exception chain for debugging.♻️ Suggested fix
try: serializer.validate_new_options(new_options, options_history, None) except DRFValidationError as exc: - raise forms.ValidationError(exc.detail or exc.args) + raise forms.ValidationError(exc.detail or exc.args) from exc
347-350: Useraise ... from excfor proper exception chaining.Same issue - preserve exception chain when re-raising.
♻️ Suggested fix
except DRFValidationError as exc: - raise forms.ValidationError(exc.detail or exc.args) + raise forms.ValidationError(exc.detail or exc.args) from exc
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (23)
front_end/messages/cs.jsonfront_end/messages/en.jsonfront_end/messages/es.jsonfront_end/messages/pt.jsonfront_end/messages/zh-TW.jsonfront_end/messages/zh.jsonfront_end/src/app/(main)/questions/[id]/components/multiple_choices_chart_view/choices_legend/index.tsxfront_end/src/app/(main)/questions/components/question_form.tsxfront_end/src/components/detailed_question_card/detailed_question_card/multiple_choice_chart_card.tsxfront_end/src/components/forecast_maker/forecast_maker_question/forecast_maker_multiple_choice.tsxfront_end/src/types/choices.tsfront_end/src/utils/questions/choices.tsfront_end/src/utils/questions/helpers.tsnotifications/templates/emails/multiple_choice_option_addition.htmlnotifications/templates/emails/multiple_choice_option_addition.mjmlnotifications/templates/emails/multiple_choice_option_deletion.htmlnotifications/templates/emails/multiple_choice_option_deletion.mjmlquestions/admin.pyquestions/serializers/common.pyquestions/services/multiple_choice_handlers.pyquestions/tasks.pytests/unit/test_questions/test_services/test_multiple_choice_handlers.pytests/unit/test_utils/test_the_math/test_aggregations.py
🚧 Files skipped from review as they are similar to previous changes (6)
- front_end/src/types/choices.ts
- front_end/src/utils/questions/helpers.ts
- notifications/templates/emails/multiple_choice_option_addition.mjml
- front_end/src/components/detailed_question_card/detailed_question_card/multiple_choice_chart_card.tsx
- front_end/messages/en.json
- front_end/messages/zh.json
🧰 Additional context used
🧠 Learnings (3)
📚 Learning: 2026-01-15T19:29:58.940Z
Learnt from: hlbmtc
Repo: Metaculus/metaculus PR: 4075
File: authentication/urls.py:24-26
Timestamp: 2026-01-15T19:29:58.940Z
Learning: In this codebase, DRF is configured to use IsAuthenticated as the default in REST_FRAMEWORK['DEFAULT_PERMISSION_CLASSES'] within metaculus_web/settings.py. Therefore, explicit permission_classes([IsAuthenticated]) decorators are unnecessary on DRF views unless a view needs to override the default. When reviewing Python files, verify that views relying on the default are not redundantly decorated, and flag cases where permissions are being over-specified or when a non-default permission is explicitly required.
Applied to files:
tests/unit/test_utils/test_the_math/test_aggregations.pyquestions/services/multiple_choice_handlers.pyquestions/tasks.pyquestions/serializers/common.pyquestions/admin.pytests/unit/test_questions/test_services/test_multiple_choice_handlers.py
📚 Learning: 2026-01-16T20:41:15.295Z
Learnt from: lsabor
Repo: Metaculus/metaculus PR: 4038
File: questions/services/multiple_choice_handlers.py:138-186
Timestamp: 2026-01-16T20:41:15.295Z
Learning: In questions/services/multiple_choice_handlers.py, the catch-all option position is validated upstream in MultipleChoiceOptionsUpdateSerializer.validate_new_options, which enforces that the last option (catch-all) cannot change during add/delete operations. Reordering the catch-all is intentionally allowed in multiple_choice_reorder_options when options have never changed (len(options_history) == 1), as all options are equal at that stage.
Applied to files:
questions/services/multiple_choice_handlers.pyquestions/serializers/common.pyquestions/admin.pytests/unit/test_questions/test_services/test_multiple_choice_handlers.py
📚 Learning: 2026-01-16T20:44:06.504Z
Learnt from: lsabor
Repo: Metaculus/metaculus PR: 4038
File: scoring/score_math.py:90-92
Timestamp: 2026-01-16T20:44:06.504Z
Learning: Multiple choice questions in the Metaculus codebase are constrained to always have at least two options; therefore, when scoring multiple choice forecasts, `options_at_time` (the count of non-None values in the PMF) will always be ≥ 2, and division by `np.log(options_at_time)` is safe.
Applied to files:
questions/services/multiple_choice_handlers.pyquestions/serializers/common.pyfront_end/src/components/forecast_maker/forecast_maker_question/forecast_maker_multiple_choice.tsx
🧬 Code graph analysis (6)
front_end/src/app/(main)/questions/components/question_form.tsx (1)
front_end/src/components/ui/input_container.tsx (1)
InputContainer(13-67)
tests/unit/test_utils/test_the_math/test_aggregations.py (3)
utils/the_math/aggregations.py (2)
compute_weighted_semi_standard_deviations(115-141)ForecastSet(52-56)questions/types.py (1)
AggregationMethod(18-22)tests/unit/test_questions/conftest.py (2)
question_binary(20-24)question_multiple_choice(28-33)
questions/services/multiple_choice_handlers.py (3)
questions/models.py (5)
Question(60-448)Forecast(576-712)QuestionType(80-85)save(291-319)save(708-712)questions/services/forecasts.py (1)
build_question_forecasts(420-461)questions/tasks.py (2)
multiple_choice_delete_option_notifications(264-339)multiple_choice_add_option_notifications(343-452)
front_end/src/utils/questions/choices.ts (2)
front_end/src/utils/questions/helpers.ts (2)
getAllOptionsHistory(203-217)getUpcomingOptions(219-232)front_end/src/types/choices.ts (1)
ChoiceItem(8-32)
questions/serializers/common.py (3)
questions/services/multiple_choice_handlers.py (2)
get_all_options_from_history(83-105)validate(61-80)front_end/src/types/question.ts (1)
Question(266-311)questions/models.py (2)
Question(60-448)QuestionType(80-85)
tests/unit/test_questions/test_services/test_multiple_choice_handlers.py (6)
questions/models.py (5)
Question(60-448)Forecast(576-712)save(291-319)save(708-712)SourceChoices(631-636)questions/services/multiple_choice_handlers.py (4)
multiple_choice_add_options(302-375)multiple_choice_delete_options(192-299)multiple_choice_rename_option(108-135)multiple_choice_reorder_options(138-185)tests/unit/utils.py (1)
datetime_aware(10-15)tests/unit/test_scoring/test_score_math.py (1)
dt(21-24)posts/models.py (1)
save(873-879)questions/admin.py (2)
author(472-473)forecasts(469-470)
🪛 Ruff (0.14.13)
tests/unit/test_utils/test_the_math/test_aggregations.py
108-108: zip() without an explicit strict= parameter
Add explicit value for parameter strict=
(B905)
110-110: zip() without an explicit strict= parameter
Add explicit value for parameter strict=
(B905)
questions/services/multiple_choice_handlers.py
37-37: Avoid specifying long messages outside the exception class
(TRY003)
39-39: Avoid specifying long messages outside the exception class
(TRY003)
42-42: Avoid specifying long messages outside the exception class
(TRY003)
44-44: Avoid specifying long messages outside the exception class
(TRY003)
46-48: Avoid specifying long messages outside the exception class
(TRY003)
51-53: Avoid specifying long messages outside the exception class
(TRY003)
55-55: Avoid specifying long messages outside the exception class
(TRY003)
57-59: Avoid specifying long messages outside the exception class
(TRY003)
64-64: Avoid specifying long messages outside the exception class
(TRY003)
67-67: Avoid specifying long messages outside the exception class
(TRY003)
72-75: Avoid specifying long messages outside the exception class
(TRY003)
98-98: Avoid specifying long messages outside the exception class
(TRY003)
105-105: Consider [*all_labels, designated_other_label] instead of concatenation
Replace with [*all_labels, designated_other_label]
(RUF005)
118-118: Avoid specifying long messages outside the exception class
(TRY003)
120-120: Avoid specifying long messages outside the exception class
(TRY003)
122-122: Avoid specifying long messages outside the exception class
(TRY003)
124-124: Avoid specifying long messages outside the exception class
(TRY003)
152-152: Avoid specifying long messages outside the exception class
(TRY003)
154-154: Avoid specifying long messages outside the exception class
(TRY003)
156-156: Avoid specifying long messages outside the exception class
(TRY003)
158-158: Avoid specifying long messages outside the exception class
(TRY003)
162-162: Avoid specifying long messages outside the exception class
(TRY003)
210-210: Avoid specifying long messages outside the exception class
(TRY003)
214-214: Avoid specifying long messages outside the exception class
(TRY003)
216-216: Avoid specifying long messages outside the exception class
(TRY003)
224-224: Avoid specifying long messages outside the exception class
(TRY003)
243-246: Avoid specifying long messages outside the exception class
(TRY003)
248-248: zip() without an explicit strict= parameter
Add explicit value for parameter strict=
(B905)
321-321: Avoid specifying long messages outside the exception class
(TRY003)
323-323: Avoid specifying long messages outside the exception class
(TRY003)
325-325: Avoid specifying long messages outside the exception class
(TRY003)
328-328: Avoid specifying long messages outside the exception class
(TRY003)
335-335: Avoid specifying long messages outside the exception class
(TRY003)
questions/tasks.py
302-302: Do not catch blind exception: Exception
(BLE001)
390-390: Do not catch blind exception: Exception
(BLE001)
questions/serializers/common.py
243-248: Avoid specifying long messages outside the exception class
(TRY003)
468-470: Avoid specifying long messages outside the exception class
(TRY003)
472-472: Avoid specifying long messages outside the exception class
(TRY003)
474-476: Avoid specifying long messages outside the exception class
(TRY003)
479-481: Avoid specifying long messages outside the exception class
(TRY003)
488-490: Avoid specifying long messages outside the exception class
(TRY003)
492-494: Avoid specifying long messages outside the exception class
(TRY003)
questions/admin.py
157-157: Consider [("", "Select action"), *action_choices] instead of concatenation
Replace with [("", "Select action"), *action_choices]
(RUF005)
222-224: Avoid specifying long messages outside the exception class
(TRY003)
297-297: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
350-350: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
413-413: Avoid specifying long messages outside the exception class
(TRY003)
422-422: Avoid specifying long messages outside the exception class
(TRY003)
436-442: Mutable class attributes should be annotated with typing.ClassVar
(RUF012)
587-587: Avoid specifying long messages outside the exception class
(TRY003)
tests/unit/test_questions/test_services/test_multiple_choice_handlers.py
273-273: zip() without an explicit strict= parameter
Add explicit value for parameter strict=
(B905)
452-452: zip() without an explicit strict= parameter
Add explicit value for parameter strict=
(B905)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Backend Checks
- GitHub Check: integration-tests
🔇 Additional comments (15)
tests/unit/test_utils/test_the_math/test_aggregations.py (3)
26-32: Imports for the new aggregation test look good.Clean additions for the new test helper and typing aliases.
308-389: Good MC placeholder coverage for UnweightedAggregation.These cases exercise the placeholder paths well.
402-422: Nice normalization for placeholder comparisons.The None → NaN normalization makes the assertions resilient to placeholder handling.
front_end/src/app/(main)/questions/[id]/components/multiple_choices_chart_view/choices_legend/index.tsx (1)
63-67: LGTM: label fallback is consistent across legend + dropdown.Also applies to: 124-136
front_end/src/app/(main)/questions/components/question_form.tsx (1)
312-313: LGTM: options lock behavior is applied consistently.Also applies to: 786-844
front_end/src/utils/questions/choices.ts (1)
39-48: Consider removing the unusedpreserveOrderflag or document its intended use case.The
preserveOrderparameter is never set totruein the codebase (searched all callers). If this feature is intended for future use, the suggested adjustment to preserve admin-configured option order makes sense—otherwise, this flag and its conditional logic (lines 42–48) can be safely removed.questions/services/multiple_choice_handlers.py (3)
261-264: Unreachable code branch.The condition
if forecast.start_time >= timestepon line 261 can never be true because the queryset filter on lines 235-236 explicitly selects forecasts withend_time__gt=timesteporend_time__isnull=True, notstart_time__lt=timestep. However, looking more closely, there's nostart_time__ltfilter, so forecasts starting at/aftertimestepare included.Actually, the filter is
Q(end_time__isnull=True) | Q(end_time__gt=timestep)which doesn't filter bystart_time. So this branch is reachable when a forecast starts at or aftertimestep.
292-297: LGTM - Dramatiq actors invoked via.send().The notification tasks are correctly invoked using
.send()for async execution, consistent with the codebase pattern.
367-373: LGTM - Add options notification also uses.send().Consistent async invocation pattern for the add-options notification task.
tests/unit/test_questions/test_services/test_multiple_choice_handlers.py (2)
152-160: Test case validates forecast at timestep correctly updates PMF without slicing.This test case (lines 142-160) correctly verifies the scenario where a forecast starts at/after the deletion timestep - it should have its PMF updated in place without creating a new sliced forecast. Good coverage of this edge case.
1-15: Test module structure looks good.The imports are well-organized and the test module correctly imports the service functions being tested. The use of
datetime_awareutility and factory fixtures aligns with codebase patterns.questions/admin.py (1)
584-602: Admin view has appropriate permission checks.The view correctly checks for question existence (404), change permission (PermissionDenied), question type validation, and required fields. Good defensive programming.
questions/serializers/common.py (3)
235-250: LGTM - ValidationError is now properly raised.The
raisekeyword was added to line 243, addressing the previous review comment. The validation correctly prevents options updates through this endpoint when user forecasts exist.
461-503: Multiple choice validation logic is comprehensive.The updated validation correctly:
- Checks that current options are a subset of the submitted forecast keys
- Validates against all historical options
- Enforces proper probability ranges for current vs inactive options
- Verifies probabilities sum to 1.0
The error messages are detailed and helpful for debugging.
871-874: Resolution validation uses history-aware options.The update correctly uses
get_all_options_from_historyto allow resolution to any option that ever existed, not just current options. This is important for questions where the resolved option was later deleted.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
...end/src/components/forecast_maker/forecast_maker_question/forecast_maker_multiple_choice.tsx
Show resolved
Hide resolved
lsabor
force-pushed
the
feat/mc-options-change
branch
from
63d1e2a to
d2f9bb1
Compare
...end/src/components/forecast_maker/forecast_maker_question/forecast_maker_multiple_choice.tsx
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
tests/unit/test_utils/test_the_math/test_aggregations.py (1)
660-683: RecencyWeightedAggregation test still uses old assertion pattern that doesn't handle None.The test now includes a "Multiple choice with placeholders" case (lines 610-637) with
Nonevalues, but the assertions at lines 661-683 usenp.allclose()which will fail or behave unexpectedly when comparing arrays containingNone.Unlike
test_UnweightedAggregationwhich was updated to use the NaN-aware loop pattern (lines 412-422), this test was not updated accordingly.🐛 Suggested fix: Use the same NaN-aware pattern as UnweightedAggregation
assert new_aggregation.start_time == expected.start_time - assert ( - new_aggregation.forecast_values == expected.forecast_values - ) or np.allclose(new_aggregation.forecast_values, expected.forecast_values) assert new_aggregation.forecaster_count == expected.forecaster_count - assert ( - new_aggregation.interval_lower_bounds == expected.interval_lower_bounds - ) or np.allclose( - new_aggregation.interval_lower_bounds, expected.interval_lower_bounds - ) - assert (new_aggregation.centers == expected.centers) or np.allclose( - new_aggregation.centers, expected.centers - ) - assert ( - new_aggregation.interval_upper_bounds == expected.interval_upper_bounds - ) or np.allclose( - new_aggregation.interval_upper_bounds, expected.interval_upper_bounds - ) - assert (new_aggregation.means == expected.means) or np.allclose( - new_aggregation.means, expected.means - ) - assert (new_aggregation.histogram == expected.histogram) or np.allclose( - new_aggregation.histogram, expected.histogram - ) + for r, e in [ + (new_aggregation.forecast_values, expected.forecast_values), + (new_aggregation.interval_lower_bounds, expected.interval_lower_bounds), + (new_aggregation.centers, expected.centers), + (new_aggregation.interval_upper_bounds, expected.interval_upper_bounds), + (new_aggregation.means, expected.means), + (new_aggregation.histogram, expected.histogram), + ]: + r = np.where(np.equal(r, None), np.nan, r).astype(float) + e = np.where(np.equal(e, None), np.nan, e).astype(float) + np.testing.assert_allclose(r, e, equal_nan=True)
♻️ Duplicate comments (7)
front_end/messages/es.json (1)
1838-1839: Singular/plural wording still needs correction (ES).The issues flagged previously remain:
- Line 1838 uses awkward
su(s) pronóstico(s)notation- Line 1839 (singular key) incorrectly uses
sus pronósticos(plural form)front_end/messages/pt.json (1)
1831-1837: Use singular phrasing fornewOptionsAddedSingular.Line 1837 uses plural (“suas previsões”) under a singular key. Please switch to singular to match the key’s intent.
✏️ Proposed fix
- "newOptionsAddedSingular": "Uma nova opção foi adicionada recentemente, por favor ajuste suas previsões de acordo.", + "newOptionsAddedSingular": "Uma nova opção foi adicionada recentemente, por favor ajuste sua previsão de acordo.",front_end/messages/cs.json (1)
1844-1844: Fix Czech plural form for “days”.Line 1844 should use “dní” for the other plural category.
🛠️ Proposed fix
- "periodDays": "{count, plural, one {# den} few {# dny} other {# dni}}", + "periodDays": "{count, plural, one {# den} few {# dny} other {# dní}}",questions/tasks.py (3)
324-336: Use formatted timestamp in deletion email context.
formatted_timestepis computed but the email receives the raw datetime.Suggested change
- "timestep": timestep, + "timestep": formatted_timestep,
427-432: Use formatted timestamps in addition email context.
The template should receive the same human-readable values used in the comment.Suggested change
- "grace_period_end": grace_period_end, - "timestep": timestep, + "grace_period_end": formatted_grace_period_end, + "timestep": formatted_timestep,
441-455: Filter follow‑up notification forecast bygrace_period_endand guard None.
Without the filter, you can attach the wrong forecast or hit a null FK error.Suggested change
- for forecaster in forecasters: - UserForecastNotification.objects.filter( - user=forecaster, question=question - ).delete() # is this necessary? - UserForecastNotification.objects.update_or_create( - user=forecaster, - question=question, - defaults={ - "trigger_time": grace_period_end - timedelta(days=1), - "email_sent": False, - "forecast": Forecast.objects.filter( - question=question, author=forecaster - ) - .order_by("-start_time") - .first(), - }, - ) + for forecaster in forecasters: + forecast = ( + Forecast.objects.filter( + question=question, + author=forecaster, + end_time=grace_period_end, + ) + .order_by("-start_time") + .first() + ) + if not forecast: + continue + UserForecastNotification.objects.filter( + user=forecaster, question=question + ).delete() # is this necessary? + UserForecastNotification.objects.update_or_create( + user=forecaster, + question=question, + defaults={ + "trigger_time": grace_period_end - timedelta(days=1), + "email_sent": False, + "forecast": forecast, + }, + )questions/admin.py (1)
189-202: Display grace period initial value in UTC as labeled.
timezone.localtime()defaults to server TZ; the help text says UTC.Suggested change
-from datetime import datetime, timedelta +from datetime import datetime, timedelta, timezone as dt_timezone @@ - grace_field.initial = timezone.localtime(grace_initial) + grace_field.initial = timezone.localtime(grace_initial, timezone=dt_timezone.utc)
🧹 Nitpick comments (4)
tests/unit/test_utils/test_the_math/test_aggregations.py (1)
108-111: Consider addingstrict=Trueto zip calls.Per static analysis hint B905, adding
strict=Trueensures both sequences have the same length, which would catch test data inconsistencies early.Suggested fix
- for v, e in zip(rl, el): + for v, e in zip(rl, el, strict=True): np.testing.assert_approx_equal(v, e) - for v, e in zip(ru, eu): + for v, e in zip(ru, eu, strict=True): np.testing.assert_approx_equal(v, e)questions/models.py (3)
24-42: Preserve parsing context invalidate_options_history.
Use exception chaining so the rootValueErrorisn’t lost during ISO parsing.Suggested change
- try: - datetime.fromisoformat(item[0]) - except ValueError: - raise ValidationError( - f"Bad datetime format at index {i}: {item[0]!r}, must be isoformat string" - ) + try: + datetime.fromisoformat(item[0]) + except ValueError as err: + raise ValidationError( + f"Bad datetime format at index {i}: {item[0]!r}, must be isoformat string" + ) from err
315-317: Avoid seeding history with empty options.
If a MC question is saved before options are set, this stores an empty snapshot that won’t auto-correct. Initialize only once options exist.Suggested change
- if self.type == self.QuestionType.MULTIPLE_CHOICE and not self.options_history: + if ( + self.type == self.QuestionType.MULTIPLE_CHOICE + and not self.options_history + and self.options + ): # initialize options history on first save self.options_history = [(datetime.min.isoformat(), self.options or [])]
687-700: Alignget_pmfreturn type with None-preserving behavior.
Whenreplace_none=False, this can returnNonevalues; the annotation should reflect that.Suggested change
- def get_pmf(self, replace_none: bool = False) -> list[float]: + def get_pmf(self, replace_none: bool = False) -> list[float | None]:
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (14)
docs/openapi.ymlfront_end/messages/cs.jsonfront_end/messages/en.jsonfront_end/messages/es.jsonfront_end/messages/pt.jsonfront_end/messages/zh-TW.jsonfront_end/messages/zh.jsonposts/models.pyquestions/admin.pyquestions/models.pyquestions/tasks.pytests/unit/test_utils/test_the_math/test_aggregations.pyutils/the_math/aggregations.pyutils/the_math/measures.py
🚧 Files skipped from review as they are similar to previous changes (4)
- posts/models.py
- docs/openapi.yml
- front_end/messages/zh-TW.json
- front_end/messages/en.json
🧰 Additional context used
🧠 Learnings (6)
📓 Common learnings
Learnt from: lsabor
Repo: Metaculus/metaculus PR: 4038
File: scoring/score_math.py:90-92
Timestamp: 2026-01-16T20:44:13.892Z
Learning: Multiple choice questions in the Metaculus codebase are constrained to always have at least two options; therefore, when scoring multiple choice forecasts, `options_at_time` (the count of non-None values in the PMF) will always be ≥ 2, and division by `np.log(options_at_time)` is safe.
📚 Learning: 2026-01-15T19:29:58.940Z
Learnt from: hlbmtc
Repo: Metaculus/metaculus PR: 4075
File: authentication/urls.py:24-26
Timestamp: 2026-01-15T19:29:58.940Z
Learning: In this codebase, DRF is configured to use IsAuthenticated as the default in REST_FRAMEWORK['DEFAULT_PERMISSION_CLASSES'] within metaculus_web/settings.py. Therefore, explicit permission_classes([IsAuthenticated]) decorators are unnecessary on DRF views unless a view needs to override the default. When reviewing Python files, verify that views relying on the default are not redundantly decorated, and flag cases where permissions are being over-specified or when a non-default permission is explicitly required.
Applied to files:
utils/the_math/aggregations.pytests/unit/test_utils/test_the_math/test_aggregations.pyutils/the_math/measures.pyquestions/admin.pyquestions/tasks.pyquestions/models.py
📚 Learning: 2026-01-16T20:44:13.892Z
Learnt from: lsabor
Repo: Metaculus/metaculus PR: 4038
File: scoring/score_math.py:90-92
Timestamp: 2026-01-16T20:44:13.892Z
Learning: Multiple choice questions in the Metaculus codebase are constrained to always have at least two options; therefore, when scoring multiple choice forecasts, `options_at_time` (the count of non-None values in the PMF) will always be ≥ 2, and division by `np.log(options_at_time)` is safe.
Applied to files:
utils/the_math/measures.py
📚 Learning: 2026-01-16T20:47:54.039Z
Learnt from: lsabor
Repo: Metaculus/metaculus PR: 4038
File: utils/the_math/measures.py:109-119
Timestamp: 2026-01-16T20:47:54.039Z
Learning: In utils/the_math/measures.py, enforce that any PMF used for multiple-choice questions sums to 1.0. When filtering None values from PMFs during scoring or divergence calculations, ensure the resulting sequence is non-empty (and typically contains at least two options). Add explicit checks or asserts for the PMF sum and non-None entry count, and cover these with unit tests to prevent regressions.
Applied to files:
utils/the_math/measures.py
📚 Learning: 2026-01-16T20:41:23.391Z
Learnt from: lsabor
Repo: Metaculus/metaculus PR: 4038
File: questions/services/multiple_choice_handlers.py:138-186
Timestamp: 2026-01-16T20:41:23.391Z
Learning: In questions/services/multiple_choice_handlers.py, the catch-all option position is validated upstream in MultipleChoiceOptionsUpdateSerializer.validate_new_options, which enforces that the last option (catch-all) cannot change during add/delete operations. Reordering the catch-all is intentionally allowed in multiple_choice_reorder_options when options have never changed (len(options_history) == 1), as all options are equal at that stage.
Applied to files:
questions/admin.pyquestions/models.py
📚 Learning: 2026-01-16T20:48:03.850Z
Learnt from: lsabor
Repo: Metaculus/metaculus PR: 4038
File: utils/the_math/measures.py:109-119
Timestamp: 2026-01-16T20:48:03.850Z
Learning: In multiple-choice questions, the probability mass function (PMF) must always sum to 1.0, which guarantees that at least one entry is non-None (and typically at least two, given the minimum options constraint). Therefore, when filtering None values from PMFs in scoring or divergence calculations, the resulting arrays will never be empty.
Applied to files:
questions/models.py
🧬 Code graph analysis (4)
tests/unit/test_utils/test_the_math/test_aggregations.py (1)
utils/the_math/aggregations.py (4)
compute_weighted_semi_standard_deviations(115-141)ForecastSet(52-56)calculate_aggregation_entry(614-668)RecencyWeightedAggregation(679-681)
utils/the_math/measures.py (1)
questions/models.py (2)
Question(60-448)QuestionType(80-85)
questions/admin.py (2)
questions/services/multiple_choice_handlers.py (7)
MultipleChoiceOptionsUpdateSerializer(17-80)get_all_options_from_history(83-105)multiple_choice_add_options(302-375)multiple_choice_delete_options(192-299)multiple_choice_rename_option(108-135)multiple_choice_reorder_options(138-185)validate_new_options(21-59)questions/models.py (4)
Question(60-448)QuestionType(80-85)save(291-319)save(708-712)
questions/models.py (2)
questions/types.py (1)
AggregationMethod(18-22)front_end/src/types/question.ts (1)
Question(266-311)
🪛 Ruff (0.14.13)
tests/unit/test_utils/test_the_math/test_aggregations.py
108-108: zip() without an explicit strict= parameter
Add explicit value for parameter strict=
(B905)
110-110: zip() without an explicit strict= parameter
Add explicit value for parameter strict=
(B905)
utils/the_math/measures.py
144-144: zip() without an explicit strict= parameter
Add explicit value for parameter strict=
(B905)
149-149: zip() without an explicit strict= parameter
Add explicit value for parameter strict=
(B905)
questions/admin.py
157-157: Consider [("", "Select action"), *action_choices] instead of concatenation
Replace with [("", "Select action"), *action_choices]
(RUF005)
222-224: Avoid specifying long messages outside the exception class
(TRY003)
297-297: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
350-350: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
413-413: Avoid specifying long messages outside the exception class
(TRY003)
422-422: Avoid specifying long messages outside the exception class
(TRY003)
436-442: Mutable class attributes should be annotated with typing.ClassVar
(RUF012)
587-587: Avoid specifying long messages outside the exception class
(TRY003)
questions/tasks.py
303-303: Do not catch blind exception: Exception
(BLE001)
395-395: Do not catch blind exception: Exception
(BLE001)
questions/models.py
27-27: Avoid specifying long messages outside the exception class
(TRY003)
36-36: Avoid specifying long messages outside the exception class
(TRY003)
40-42: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
40-42: Avoid specifying long messages outside the exception class
(TRY003)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Build Docker Image / Build Docker Image
- GitHub Check: Backend Checks
- GitHub Check: integration-tests
🔇 Additional comments (18)
front_end/messages/es.json (1)
1833-1843: LGTM for remaining new localization keys.The tournament info keys, grace period tooltip, time remaining, period pluralization keys, and
choicesLockedHelpare correctly translated and use proper ICU MessageFormat syntax for pluralization.Also applies to: 1885-1889
front_end/messages/zh.json (1)
1835-1845: LGTM!All new localization keys are correctly translated:
- Tournament info, grace period, and new options messaging keys are appropriate for Chinese.
- Period pluralization keys correctly use ICU MessageFormat syntax (though Chinese doesn't distinguish grammatical number, the format is correct).
choicesLockedHelpis clear and accurate.Also applies to: 1887-1891
front_end/messages/pt.json (1)
1883-1887: LGTM for period unit pluralization and help copy.front_end/messages/cs.json (1)
941-942: LGTM forchoicesLockedHelpwording.utils/the_math/aggregations.py (2)
492-492: LGTM! Defensive handling for divide-by-zero.Setting zero centers to 1.0 prevents division by zero when normalizing. Since the center is zero, the normalized value will also be zero regardless of the divisor, making 1.0 a safe sentinel.
645-656: LGTM! Proper None→NaN→None round-trip for means calculation.The approach correctly:
- Identifies None positions from the first forecast row
- Converts those columns to NaN before averaging
- Casts to object dtype to allow mixed types
- Converts NaN back to None in the final result
This aligns with the PR objective of handling placeholder values for added/removed MC options.
utils/the_math/measures.py (4)
20-23: LGTM! Proper copy-on-write and NaN handling.Creating a copy before modification avoids side effects on the input array. The NaN replacement enables proper numeric operations while preserving the semantic meaning of missing values.
Also applies to: 28-28
56-59: LGTM! Consistent NaN→None conversion at output.This completes the round-trip, ensuring callers receive
Nonefor missing values as expected by the rest of the codebase.
109-123: Verify mass redistribution logic when options differ between forecasts.The logic transfers probability mass from positions where one forecast has a value but the other has
Noneto the last non-None element. This assumes the last element is always a valid "catch-all" bucket.Two edge cases to consider:
- If both
p1andp2haveNoneat the same position, that mass is correctly excluded (vianever_nones).- If the last non-None element is at different indices in
p1_newvsp2_new, mass is still added to[-1]of each.Based on learnings, PMFs always have ≥2 non-None values, so
p1_new[-1]access is safe.
144-154: The accumulation loop logic is mathematically correct and all test cases pass.The code correctly handles None values by converting them to 0.0 and accumulating non-None values into the last elements. The test suite confirms this behavior across multiple scenarios including cases with None at different positions and asymmetric None placements.
While the suggested refactoring to use explicit
if/elifbranches andstrict=Truewould improve code clarity and add safety assertions, the current implementation produces correct results. The characterization as a major issue with incorrect accumulation is unsupported by the test evidence.tests/unit/test_utils/test_the_math/test_aggregations.py (3)
55-111: LGTM! Comprehensive test coverage for semi-standard deviations with placeholders.The test cases cover:
- Trivial single forecast
- Multiple identical forecasts
- Varying forecasts (symmetric deviations)
- Identical forecasts with None placeholders
- Different forecasts with None placeholders
Good coverage of the new None-handling behavior.
308-389: LGTM! Good test coverage for Multiple Choice with placeholders.The two new test cases properly exercise:
- Two forecasts with identical structure and None placeholder (lines 309-335)
- Three forecasts with varying values and None placeholder (lines 336-389)
The expected values include
Nonein the appropriate positions, validating the end-to-end None handling.
412-422: LGTM! NaN-aware assertion pattern for handling None values.The loop correctly converts
Nonetonp.nanand usesnp.testing.assert_allclosewithequal_nan=True, enabling proper comparison of arrays containing placeholders.questions/models.py (2)
218-231:options_historyfield integration looks solid.
The validator + help text make the contract clear.
772-785: AggregateForecastreplace_nonehandling is consistent.
No issues.questions/admin.py (3)
41-63: Grace-period helper functions look good.
492-504: Update options button integration looks good.
584-699: Update options view wiring is solid.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
🧹 Preview Environment Cleaned UpThe preview environment for this PR has been destroyed.
Cleanup triggered by PR close at 2026-01-24T17:31:19Z |
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
front_end/src/components/forecast_maker/forecast_maker_question/forecast_maker_multiple_choice.tsx (1)
399-399: Logic error in condition.
!forecastsSum === nullis incorrect—it evaluates as(!forecastsSum) === null, which compares a boolean tonulland is alwaysfalse. The intended check is likelyforecastsSum === null.🐛 Proposed fix
- if (!forecastHasValues || !forecastsSum === null) return; + if (!forecastHasValues || forecastsSum === null) return;
♻️ Duplicate comments (2)
front_end/src/components/forecast_maker/forecast_maker_question/forecast_maker_multiple_choice.tsx (2)
326-336: Guard against invalid grace-period timestamps.If the last
options_historytimestamp is invalid,gracePeriodEndbecomes anInvalid Date, which can leak into the countdown/tooltip. Adding aNaNguard would prevent this edge case.🛡️ Suggested guard
const gracePeriodEnd = useMemo(() => { if (!question.options_history || question.options_history.length === 0) { return null; } - const lastTimestep = new Date(question.options_history.at(-1)?.[0] || 0); + const lastTimestamp = question.options_history.at(-1)?.[0]; + if (!lastTimestamp) return null; + const lastTimestep = new Date(lastTimestamp); + if (Number.isNaN(lastTimestep.getTime())) return null; if (new Date().getTime() < lastTimestep.getTime()) { return null; } return lastTimestep; }, [question.options_history]);
571-611: Returningundefinedfrom.map()creates sparse array behavior.When
question.options.includes(choice.name)is false, no value is returned, insertingundefinedinto the array. Use.filter()before.map()for cleaner rendering.🛠️ Proposed fix
- {choicesForecasts.map((choice) => { - if (question.options.includes(choice.name)) { - const isFirstNewOption = choice.name === firstNewOptionName; - const isNewOption = newOptionNames.has(choice.name); - return ( - <ForecastChoiceOption - key={choice.name} - ... - /> - ); - } - })} + {choicesForecasts + .filter((choice) => question.options.includes(choice.name)) + .map((choice) => { + const isFirstNewOption = choice.name === firstNewOptionName; + const isNewOption = newOptionNames.has(choice.name); + return ( + <ForecastChoiceOption + key={choice.name} + ... + /> + ); + })}
🧹 Nitpick comments (5)
front_end/src/components/forecast_maker/forecast_maker_question/forecast_maker_multiple_choice.tsx (4)
119-119: Minor: Redundant condition.
isPlural && newOptions.length > 0is redundant sinceisPluralisnewOptions.length > 1, which already implieslength > 0.🔧 Suggested simplification
- {isPlural && newOptions.length > 0 && mounted && ( + {isPlural && mounted && (
340-355: Consider cleaning up the timeout on unmount.If the component unmounts before the animation completes, the
setTimeoutcallback will still execute. While React 18+ handles this gracefully, it's good practice to clean up.🔧 Alternative using useCallback with cleanup
You could track the timeout ID in a ref and clear it on unmount, or use a more robust approach:
+ const animationTimeoutRef = useRef<NodeJS.Timeout | null>(null); + + useEffect(() => { + return () => { + if (animationTimeoutRef.current) { + clearTimeout(animationTimeoutRef.current); + } + }; + }, []); + const scrollToNewOptions = () => { if (firstNewOptionRef.current) { setIsAnimatingHighlight(true); firstNewOptionRef.current.scrollIntoView({ behavior: "smooth", block: "center", }); - setTimeout(() => { + animationTimeoutRef.current = setTimeout(() => { setIsAnimatingHighlight(false); }, ANIMATION_DURATION_MS); } };
600-606: Avoid using ternary expression for side effects.Using
condition ? sideEffect() : undefinedas a statement is an anti-pattern. Prefer a simpleifstatement or short-circuit evaluation.🔧 Suggested fix
onInteraction={() => { - isNewOption - ? setInteractedOptions((prev) => - new Set(prev).add(choice.name) - ) - : undefined; + if (isNewOption) { + setInteractedOptions((prev) => + new Set(prev).add(choice.name) + ); + } }}
748-750: SimplifyfindIndexcallback.The callback ignores the element parameter and re-accesses it via index, which is redundant and less readable.
🔧 Suggested fix
- const resolutionIndex = allOptions.findIndex( - (_, index) => allOptions[index] === question.resolution - ); + const resolutionIndex = allOptions.findIndex( + (option) => option === question.resolution + );front_end/src/utils/questions/choices.ts (1)
41-43: Unnecessary optional chaining.
getAllOptionsHistoryalways returns astring[](never null/undefined), so the optional chaining and nullish coalescing are redundant.✨ Suggested cleanup
const allOptions = getAllOptionsHistory(question); const upcomingOptions = getUpcomingOptions(question); -const choiceOrdering: number[] = allOptions?.map((_, i) => i) ?? []; +const choiceOrdering: number[] = allOptions.map((_, i) => i);Similarly on line 156:
-(order) => allOptions?.[order] === question.resolution +(order) => allOptions[order] === question.resolution
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
front_end/src/components/consumer_post_card/group_forecast_card/percentage_forecast_card.tsxfront_end/src/components/detailed_question_card/detailed_question_card/multiple_choice_chart_card.tsxfront_end/src/components/forecast_maker/forecast_maker_question/forecast_maker_multiple_choice.tsxfront_end/src/components/post_card/question_tile/index.tsxfront_end/src/utils/questions/choices.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- front_end/src/components/detailed_question_card/detailed_question_card/multiple_choice_chart_card.tsx
🧰 Additional context used
🧠 Learnings (3)
📓 Common learnings
Learnt from: lsabor
Repo: Metaculus/metaculus PR: 4038
File: scoring/score_math.py:90-92
Timestamp: 2026-01-16T20:44:13.892Z
Learning: Multiple choice questions in the Metaculus codebase are constrained to always have at least two options; therefore, when scoring multiple choice forecasts, `options_at_time` (the count of non-None values in the PMF) will always be ≥ 2, and division by `np.log(options_at_time)` is safe.
📚 Learning: 2026-01-20T16:49:51.584Z
Learnt from: aseckin
Repo: Metaculus/metaculus PR: 4001
File: front_end/src/app/(futureeval)/futureeval/components/futureeval-leaderboard-table.tsx:15-22
Timestamp: 2026-01-20T16:49:51.584Z
Learning: The mockTranslate function in front_end/src/app/(futureeval)/futureeval/components/futureeval-leaderboard-table.tsx is an intentional workaround to satisfy the entryLabel function signature from shared AIB utils while keeping FutureEval translation-free.
Applied to files:
front_end/src/components/post_card/question_tile/index.tsxfront_end/src/utils/questions/choices.tsfront_end/src/components/consumer_post_card/group_forecast_card/percentage_forecast_card.tsx
📚 Learning: 2026-01-16T20:44:13.892Z
Learnt from: lsabor
Repo: Metaculus/metaculus PR: 4038
File: scoring/score_math.py:90-92
Timestamp: 2026-01-16T20:44:13.892Z
Learning: Multiple choice questions in the Metaculus codebase are constrained to always have at least two options; therefore, when scoring multiple choice forecasts, `options_at_time` (the count of non-None values in the PMF) will always be ≥ 2, and division by `np.log(options_at_time)` is safe.
Applied to files:
front_end/src/components/forecast_maker/forecast_maker_question/forecast_maker_multiple_choice.tsx
🧬 Code graph analysis (4)
front_end/src/components/post_card/question_tile/index.tsx (1)
front_end/src/utils/questions/choices.ts (1)
generateChoiceItemsFromMultipleChoiceForecast(24-174)
front_end/src/utils/questions/choices.ts (3)
front_end/src/types/question.ts (1)
QuestionWithMultipleChoiceForecasts(375-386)front_end/src/utils/questions/helpers.ts (2)
getAllOptionsHistory(203-217)getUpcomingOptions(219-232)front_end/src/types/choices.ts (1)
ChoiceItem(8-32)
front_end/src/components/consumer_post_card/group_forecast_card/percentage_forecast_card.tsx (2)
front_end/src/utils/questions/helpers.ts (1)
isMultipleChoicePost(22-26)front_end/src/utils/questions/choices.ts (1)
generateChoiceItemsFromMultipleChoiceForecast(24-174)
front_end/src/components/forecast_maker/forecast_maker_question/forecast_maker_multiple_choice.tsx (4)
front_end/src/types/fetch.ts (1)
ErrorResponse(36-42)front_end/src/utils/questions/helpers.ts (2)
getAllOptionsHistory(203-217)getUpcomingOptions(219-232)front_end/src/components/forecast_maker/forecast_choice_option.tsx (1)
ANIMATION_DURATION_MS(37-37)front_end/src/types/question.ts (1)
MultipleChoiceUserForecast(174-176)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Build Docker Image / Build Docker Image
- GitHub Check: integration-tests
- GitHub Check: Backend Checks
🔇 Additional comments (11)
front_end/src/components/forecast_maker/forecast_maker_question/forecast_maker_multiple_choice.tsx (5)
6-14: LGTM!The imports are well-organized and appropriate for the new functionality being added.
298-321: LGTM!Good use of
useMemofornewOptionsandnewOptionNames, withfirstNewOptionNamecomputed directly. This properly addresses performance concerns.
273-296: LGTM!Using
Mapkeyed by option name rather than relying on array indices is robust and correctly handles the reordering done ingenerateChoiceOptions. This addresses previous concerns about index misalignment.
268-271: LGTM!The
showUserMustForecastlogic correctly identifies when a user has an active forecast but is missing values for some current options, properly handling the new options scenario.
445-493: LGTM!The submit handler correctly filters to only include current options in the forecast submission, which aligns with the new options-change functionality.
front_end/src/utils/questions/choices.ts (2)
121-125: Localization for deleted/upcoming suffixes looks good.This addresses the previous review feedback about hardcoded English strings. The labels now use
t("deleted")andt("Upcoming")for proper i18n support.Minor note: the translation keys use different capitalization ("deleted" vs "Upcoming") — ensure both keys exist in all locale files.
75-90: No action needed—index alignment betweenallOptionsandforecast_valuesis already guaranteed.The backend and frontend both use identical algorithms to establish a canonical ordering of all historical options. When forecasts are created, the backend's validation function (in
questions/serializers/common.py) explicitly converts the user-submitted forecast dictionary into an ordered array usingget_all_options_from_history(), which produces the same order as the frontend'sgetAllOptionsHistory()function. This ensures that accessingforecast_values[index]on the frontend using indices derived fromallOptionswill always be correct.front_end/src/components/post_card/question_tile/index.tsx (2)
91-94: LGTM - translation function correctly passed.The call site is properly updated to pass
tto match the new function signature.
127-139: Consider aligning with history-based options.
generateUserForecastsForMultipleQuestionusesquestion.options(current options only) whilegenerateChoiceItemsFromMultipleChoiceForecastnow usesgetAllOptionsHistory(all historical options). This asymmetry may cause misalignment when both are used together—choice items would include deleted options, but user forecasts would not.If this is intentional (user forecasts should only show current options), consider adding a comment to clarify. Otherwise, this helper may need similar updates.
front_end/src/components/consumer_post_card/group_forecast_card/percentage_forecast_card.tsx (2)
39-67: LGTM - translation parameter properly threaded through.The
tparameter is correctly:
- Obtained from
useTranslations()(line 29)- Passed to
generateChoiceItems(line 40)- Included in the
useMemodependency array (line 67)This ensures localized labels update correctly when the locale changes.
108-118: Clean parameter forwarding in helper function.The helper correctly accepts
tand passes it togenerateChoiceItemsFromMultipleChoiceForecastfor MC posts while keeping thelocaleparameter for group questions which don't need the translation function.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
closes #3799
This is the final branch for the MC options changing project
This took all of the separate branches and molded them into one
Each of the individual branches are commits on this PR and have individually been QA'd
This branch addresses the final QA steps in preparation for merging into main
commits after "MC 3805 UI" are added after this branch existed and is bug fixing and tweaks so do not need to be looked at separately
TODO:
Email render examples:
Summary by CodeRabbit
New Features
Improvements
Localization
✏️ Tip: You can customize this high-level summary in your review settings.