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 <atakanseckin@gmail.com>
...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(), | ||
| }, | ||
| ) |
lsabor
force-pushed
the
feat/mc-options-change
branch
from
2914298 to
9a97313
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: 11
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)
659-682: Assertions do not handleNonevalues for MC placeholder test cases.Unlike
test_UnweightedAggregationwhich was updated with NaN-aware comparisons (lines 419-421), this test still uses the oldor np.allclose()pattern. When the equality check fails due to float precision andnp.allclose()is called with arrays containingNone, it will raise aTypeError.The new MC placeholder test case (lines 609-636) will be affected.
🐛 Proposed fix - use NaN-aware comparison like 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)
🤖 Fix all issues with AI agents
In `@front_end/messages/cs.json`:
- Around line 1844-1845: The Czech pluralization for days in the "periodDays"
message is incorrect: update the ICU plural mapping for the key "periodDays" so
that the "other" category uses "dní" instead of "dni" (i.e., change other {#
dni} to other {# dní}) to reflect correct Czech orthography while leaving the
"one" and "few" forms untouched.
In `@front_end/messages/en.json`:
- Around line 43-53: The copy for new option messages uses "forecast(s)" and
mismatched pluralization: update "newOptionsAddedSingular" and
"newOptionsAddedPlural" so the singular reads "A new option was recently added,
please adjust your forecast accordingly." and the plural reads "These options
were recently added, please adjust your forecasts accordingly." Ensure you only
change the values for the keys newOptionsAddedSingular and newOptionsAddedPlural
to these tightened versions so pluralization and wording are consistent.
In `@front_end/messages/es.json`:
- Around line 1836-1840: Update the Spanish copy for the keys
newOptionsAddedPlural and newOptionsAddedSingular to use correct singular/plural
wording and remove “(s)”: set newOptionsAddedPlural to "Se añadieron nuevas
opciones recientemente; por favor ajuste sus pronósticos en consecuencia." and
newOptionsAddedSingular to "Se añadió una nueva opción recientemente; por favor
ajuste su pronóstico en consecuencia." so the messages read naturally and match
number.
In
`@front_end/src/components/forecast_maker/forecast_maker_question/use_grace_period_countdown.ts`:
- Around line 24-31: The hook use_grace_period_countdown sets
setTimeRemaining("expired") with a hardcoded string; change it to use the
translation function (t("expired")) to match other countdown labels
(periodDays/periodHours/periodMinutes/periodSeconds) or alternatively return an
explicit expired flag from the hook; update the branch where diff <= 0 inside
use_grace_period_countdown (the setTimeRemaining call and any consumers
expecting the string) to use t("expired") so localization is consistent with the
rest of the hook.
In `@front_end/src/components/forecast_maker/resolution/resolution_modal.tsx`:
- Line 24: The default selection in ResolutionModal uses getAllOptionsHistory()
which can reorder and include deleted choices causing the default to point to a
removed option; update the options-building logic in resolution_modal.tsx (where
getAllOptionsHistory() is used and the default/selected value is set around the
component render/initial state) to: construct the option list by starting with
the current active options (preserving their order), ensure "Other" remains
last, then append any historical/extra options returned by
getAllOptionsHistory(); finally, set the default selection to a valid current
option (falling back to "Other" only if no current option exists) so the
component never defaults to a deleted choice.
In `@notifications/templates/emails/multiple_choice_option_addition.html`:
- Around line 213-214: The template contains hardcoded English UI strings —
specifically the CTA text "Update your forecast" inside the anchor (<a ...>
Update your forecast </a>) and other hardcoded lines around the same block —
replace these with localized template variables or translation tags consistent
with the rest of the file (e.g., use the project's i18n helper or a translation
variable like {{ t("notifications.update_forecast") }} or {% trans
"notifications.update_forecast" %}) and ensure the post_url call remains
unchanged; update the corresponding translation keys in your locale files so the
button text and the other hardcoded strings at lines ~279-286 are pulled from
translations rather than hardcoded English.
In `@notifications/templates/emails/multiple_choice_option_addition.mjml`:
- Around line 57-59: Replace the hardcoded CTA text inside the mj-button
("Update your forecast") with the project's translation key using the same i18n
mechanism used elsewhere in this template (e.g., the template translation helper
or filter), so the mj-button (mj-button mj-class="cta-button" href="{% post_url
params.post.post_id params.post.post_title %}") renders a localized string; add
or reference an appropriate translation key like
notifications.multiple_choice.cta_update_forecast in the locale files if
missing.
In `@notifications/templates/emails/multiple_choice_option_deletion.html`:
- Line 1: Remove the build artifact comment `<!-- FILE: undefined -->` from the
top of the multiple_choice_option_deletion.html template; edit the template
(multiple_choice_option_deletion.html) and delete that lone HTML comment so the
file contains only the intended template markup.
In `@questions/serializers/common.py`:
- Around line 684-698: The code mutates the model instance
last_forecast.start_time (from question.request_user_forecasts) which can cause
side effects elsewhere; instead avoid changing the cached instance by creating a
shallow copy or a new serialization dict when adjusting times: when handling
Question.QuestionType.MULTIPLE_CHOICE and you need to replace/adjust the final
forecast, construct a new Forecast-like object or dict (copied from
last_forecast with start_time set to the new value) and insert that into
user_forecasts (or into the serialized output) rather than assigning to
last_forecast.start_time; ensure any code that later uses user_forecasts refers
to these copied objects so the original question.request_user_forecasts remains
unmodified.
In `@questions/services/multiple_choice_handlers.py`:
- Around line 262-265: The branch checking "if forecast.start_time >= timestep"
is unreachable because the queryset already filters forecasts with
start_time__lt=timestep; remove the conditional branch and the "continue" so
that for each forecast you always assign forecast.probability_yes_per_category =
new_pmf (or, if the intent was to include forecasts starting at/after timestep,
instead adjust the queryset filter to include those items and keep the
conditional logic). Ensure you update any related comments to reflect the
corrected behavior.
In `@tests/unit/test_utils/test_the_math/test_aggregations.py`:
- Around line 336-349: The ForecastSet in the test has forecasts_values where
each inner list has 4 entries but timesteps only has 2, causing a shape
mismatch; update the test so the number of timesteps matches the number of
values per forecast (i.e., make ForecastSet.timesteps contain four datetime
entries) or alternatively reduce each forecasts_values inner list to two entries
so len(forecasts_values[i]) == len(timesteps); locate the ForecastSet
construction (symbols: ForecastSet, forecasts_values, timesteps, forecaster_ids)
and make the shapes consistent.
♻️ Duplicate comments (3)
questions/admin.py (1)
189-202: Timezone display may not match help text claim of UTC.
timezone.localtime(grace_initial)converts to the server's configured timezone, not necessarily UTC. However, the help text on line 201 states "Time selection is in UTC." If the server timezone differs from UTC, admins will see shifted times.Pass
timezone=dt_timezone.utcto explicitly use UTC, or update the help text to reflect actual behavior.🛠️ Proposed fix
+from datetime import timezone as dt_timezone + grace_field.initial = timezone.localtime(grace_initial) + # To display in UTC as documented: + # grace_field.initial = timezone.localtime(grace_initial, timezone=dt_timezone.utc)utils/the_math/formulas.py (1)
36-38: Guard against missingoptions_historyfor legacy questions.Both conversion functions now consistently use
get_all_options_from_history, which raisesValueErrorwhenoptions_historyis empty orNone. Legacy multiple-choice questions created before this feature may not haveoptions_historypopulated, causing these utility functions to crash.🛡️ Suggested guard
if question.type == Question.QuestionType.MULTIPLE_CHOICE: - list_of_all_options = get_all_options_from_history(question.options_history) + list_of_all_options = ( + get_all_options_from_history(question.options_history) + if question.options_history + else question.options + ) return float(list_of_all_options.index(string_location))Also applies to lines 56-57.
questions/tasks.py (1)
270-271: Guard against missing post before creating comments and notifications.
question.postmay beNonefor questions not attached to a post. Line 308 callscreate_comment(comment_author, post, ...)and line 331 callsNotificationPostParams.from_post(post), both of which will fail ifpostisNone.🛡️ Suggested guard
question = Question.objects.get(id=question_id) post = question.post + if not post: + logging.warning( + "MC delete-option notification skipped: question %s has no post", + question_id, + ) + return options_history = question.options_historyAlso applies to lines 350-351.
🧹 Nitpick comments (15)
tests/unit/test_utils/test_the_math/test_aggregations.py (1)
108-111: Consider addingstrict=Truetozip()calls.Adding the
strictparameter helps catch accidental length mismatches between result and expected values.♻️ Suggested improvement
- 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)front_end/src/types/question.ts (1)
283-283: Consider a named alias for options history entries.This tuple is opaque; a small alias improves readability and reduces indexing mistakes.
♻️ Suggested refactor
+export type OptionsHistoryEntry = [string, string[]]; ... - options_history?: [string, string[]][]; + options_history?: OptionsHistoryEntry[];questions/serializers/common.py (1)
235-250: Previous issue resolved, but guard condition could be tightened.The
raisestatement is now correctly included. However, the conditiondata.get("options") != question.optionswill evaluate toTruewhenoptionsis not indata(sinceNone != question.options), potentially triggering unnecessary database queries. Consider guarding with"options" in datafirst.♻️ Optional improvement
if qid := data.get("id"): question = Question.objects.get(id=qid) - if data.get("options") != question.options: + if "options" in data and data["options"] != question.options: # if there are user forecasts, we can't update options this way if question.user_forecasts.exists(): raise ValidationError(front_end/src/components/forecast_maker/forecast_maker_question/forecast_maker_multiple_choice.tsx (2)
583-623: Map returnsundefinedfor filtered items; considerfilterfor clarity.The current implementation returns
undefinedimplicitly for non-current options inside.map(). While React safely ignores these, using.filter().map()or.flatMap()would be more explicit and avoid potential linting warnings about inconsistent returns.♻️ Suggested refactor
- {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} - // ... props - /> - ); - } - })} + {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} + // ... props + /> + ); + })}
612-618: Ternary expression withundefinedbranch is unconventional.The
onInteractionhandler uses a ternary that returnsundefinedwhen the option is not new. Anifstatement would be clearer for side-effect-only logic.♻️ Suggested refactor
onInteraction={() => { - isNewOption - ? setInteractedOptions((prev) => - new Set(prev).add(choice.name) - ) - : undefined; + if (isNewOption) { + setInteractedOptions((prev) => + new Set(prev).add(choice.name) + ); + } }}questions/admin.py (2)
294-298: Preserve exception chain for better debugging.When re-raising as a Django
forms.ValidationError, the original DRF exception context is lost. Useraise ... from excto preserve the exception chain.♻️ Proposed 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 excApply the same pattern at line 350:
except DRFValidationError as exc: - raise forms.ValidationError(exc.detail or exc.args) + raise forms.ValidationError(exc.detail or exc.args) from exc
662-676: ACTION_CHANGE_GRACE handler calls unimplemented function.The
ACTION_CHANGE_GRACEhandler callsmultiple_choice_change_grace_period_end()which raisesNotImplementedError. While the action is currently commented out inACTION_CHOICES(line 75), having this unreachable code could cause confusion. Consider adding a comment or removing the handler until implemented.questions/services/multiple_choice_handlers.py (3)
108-135: Inconsistent save() behavior across functions.
multiple_choice_rename_optiondoes not callquestion.save(), whilemultiple_choice_reorder_options(line 167) does. The docstrings both say "Modifies question in place and returns it," but the actual behavior differs. This inconsistency could lead to bugs if callers don't realize they need to save after rename but not after reorder.Either consistently call
save()in all functions or consistently leave it to the caller. The admin.py code (line 614) does call save after rename, so the current pattern works, but it's fragile.♻️ Suggested approach
For consistency, either:
Option A: Always save internally (preferred for atomicity)
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 questionOption B: Never save internally, document clearly
Update docstrings to clarify that callers must save, and removequestion.save()frommultiple_choice_reorder_options.
249-259: Considerstrict=Truefor zip() to catch length mismatches.While lines 243-247 validate that lengths match, adding
strict=Truetozip()provides defense-in-depth against future changes that might bypass the validation.♻️ Proposed fix
- for value, label in zip(previous_pmf, all_options): + for value, label in zip(previous_pmf, all_options, strict=True):
344-358: Fetching all forecasts could be optimized.Line 345 fetches all forecasts (
question.user_forecasts.all()) but only some need theend_timeupdate. While the PMF update requires all forecasts, theend_timemodification could be a separate filtered bulk update for efficiency on questions with many historical forecasts.♻️ Potential optimization
# Update PMF for all forecasts for forecast in user_forecasts: pmf = forecast.probability_yes_per_category forecast.probability_yes_per_category = ( pmf[:-1] + [None] * len(options_to_add) + [pmf[-1]] ) with transaction.atomic(): Forecast.objects.bulk_update( user_forecasts, ["probability_yes_per_category"] ) # Bulk update end_time only for affected forecasts question.user_forecasts.filter( Q(end_time__isnull=True) | Q(end_time__gt=grace_period_end), start_time__lt=grace_period_end, ).update(end_time=grace_period_end)This is a minor optimization and the current approach is correct.
questions/tasks.py (3)
322-339: Skip email when no forecasters to notify.If the
forecastersqueryset is empty,send_email_with_templateis called with an emptytolist. This is wasteful and may behave unexpectedly depending on the email backend.♻️ Suggested guard
+ forecaster_emails = [f.email for f in forecasters] + if not forecaster_emails: + logging.info("No forecasters to notify for question %s", question_id) + return # send out an immediate email send_email_with_template( - to=[forecaster.email for forecaster in forecasters], + to=forecaster_emails, subject="Multiple choice option removed", ... )Also applies to lines 414-431.
437-452: Remove redundant delete beforeupdate_or_createand guard against missing forecast.The
delete()on lines 437-439 is unnecessary sinceupdate_or_createalready handles existing records. Additionally, theForecast.objects.filter(...).first()on lines 446-450 could returnNoneif no forecast exists, which may violateUserForecastNotificationmodel constraints.♻️ Proposed fix
if grace_period_end - timedelta(days=1) > timestep: for forecaster in forecasters: - UserForecastNotification.objects.filter( - user=forecaster, question=question - ).delete() # is this necessary? + latest_forecast = Forecast.objects.filter( + question=question, author=forecaster + ).order_by("-start_time").first() + if not latest_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.objects.filter( - question=question, author=forecaster - ) - .order_by("-start_time") - .first(), + "forecast": latest_forecast, }, )
302-306: Consider catching specific exceptions for template formatting.The broad
except Exceptioncatches all errors during template formatting. While this ensures notifications are sent even with malformed templates, catching specific exceptions (KeyError,ValueError,IndexError) would be cleaner.♻️ Suggested refinement
try: text = template.format( removed_options=removed_options_text, timestep=formatted_timestep, catch_all_option=catch_all_option, ) - except Exception: + except (KeyError, ValueError, IndexError): text = ( f"{template} (removed options: {removed_options_text}, " f"at {formatted_timestep}, catch-all: {catch_all_option})" )Also applies to lines 390-394.
tests/unit/test_questions/test_services/test_multiple_choice_handlers.py (1)
271-277: Addstrict=Trueto zip and avoid shadowing theforecastsparameter.The
zip()on line 273 lacksstrict=True, which would catch length mismatches between actual and expected forecasts. Also, line 271 shadows theforecastsparameter with a local variable.♻️ Proposed fix
- forecasts = question.user_forecasts.order_by("start_time") - assert len(forecasts) == len(expected_forecasts) - for f, e in zip(forecasts, expected_forecasts): + actual_forecasts = question.user_forecasts.order_by("start_time") + assert len(actual_forecasts) == len(expected_forecasts) + for f, e in zip(actual_forecasts, expected_forecasts, strict=True): assert f.start_time == e.start_timeAlso applies to lines 450-455.
front_end/src/app/(main)/questions/[id]/components/multiple_choices_chart_view/choices_legend/index.tsx (1)
70-84: Use translation for "(deleted)" suffix.The hardcoded
" (deleted)"string should use the translation system for i18n/l10n consistency.♻️ Suggested fix
{legendChoices.map(({ choice, color, active }, idx) => { const isDeleted = currentOptions && !currentOptionNames.has(choice); - const label = isDeleted ? `${choice} (deleted)` : choice; + const label = isDeleted ? `${choice} ${t("deleted")}` : choice;Also applies to lines 136-139.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (29)
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/[id]/components/multiple_choices_chart_view/index.tsxfront_end/src/app/(main)/questions/[id]/components/question_view/forecaster_question_view/question_header/question_header_cp_status.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/components/forecast_maker/forecast_maker_question/use_grace_period_countdown.tsfront_end/src/components/forecast_maker/resolution/resolution_modal.tsxfront_end/src/types/question.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.pytemplates/admin/questions/update_options.htmltests/unit/test_questions/test_services/test_multiple_choice_handlers.pytests/unit/test_utils/test_the_math/test_aggregations.pyutils/the_math/aggregations.pyutils/the_math/formulas.py
🚧 Files skipped from review as they are similar to previous changes (4)
- front_end/src/utils/questions/helpers.ts
- notifications/templates/emails/multiple_choice_option_deletion.mjml
- front_end/src/app/(main)/questions/[id]/components/question_view/forecaster_question_view/question_header/question_header_cp_status.tsx
- front_end/messages/pt.json
🧰 Additional context used
🧠 Learnings (4)
📓 Common learnings
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.
📚 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:
tests/unit/test_questions/test_services/test_multiple_choice_handlers.pyquestions/serializers/common.pyutils/the_math/formulas.pyquestions/services/multiple_choice_handlers.pyquestions/admin.pytemplates/admin/questions/update_options.html
📚 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_questions/test_services/test_multiple_choice_handlers.pyquestions/serializers/common.pyutils/the_math/formulas.pytests/unit/test_utils/test_the_math/test_aggregations.pyquestions/services/multiple_choice_handlers.pyutils/the_math/aggregations.pyquestions/admin.pyquestions/tasks.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/serializers/common.pyutils/the_math/formulas.pyfront_end/src/components/forecast_maker/forecast_maker_question/forecast_maker_multiple_choice.tsx
🧬 Code graph analysis (8)
front_end/src/app/(main)/questions/components/question_form.tsx (1)
front_end/src/components/ui/input_container.tsx (1)
InputContainer(13-67)
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)
utils/the_math/formulas.py (1)
questions/services/multiple_choice_handlers.py (1)
get_all_options_from_history(83-105)
front_end/src/components/forecast_maker/forecast_maker_question/forecast_maker_multiple_choice.tsx (3)
front_end/src/types/fetch.ts (1)
ErrorResponse(35-41)front_end/src/utils/questions/helpers.ts (1)
getAllOptionsHistory(203-217)front_end/src/components/forecast_maker/forecast_choice_option.tsx (1)
ANIMATION_DURATION_MS(37-37)
tests/unit/test_utils/test_the_math/test_aggregations.py (3)
utils/the_math/aggregations.py (4)
compute_weighted_semi_standard_deviations(115-141)ForecastSet(52-56)calculate_aggregation_entry(614-668)RecencyWeightedAggregation(679-681)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 (6)
Question(60-448)Forecast(576-712)QuestionType(80-85)save(291-319)save(708-712)SourceChoices(631-636)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)
questions/tasks.py (13)
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)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/constants.py (1)
ScoreTypes(4-10)utils/email.py (1)
send_email_with_template(13-50)front_end/src/services/api/api_service.ts (1)
get(6-12)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)
front_end/src/components/forecast_maker/resolution/resolution_modal.tsx (1)
front_end/src/utils/questions/helpers.ts (1)
getAllOptionsHistory(203-217)
🪛 Ruff (0.14.13)
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/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)
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)
244-247: Avoid specifying long messages outside the exception class
(TRY003)
249-249: zip() without an explicit strict= parameter
Add explicit value for parameter strict=
(B905)
322-322: Avoid specifying long messages outside the exception class
(TRY003)
324-324: Avoid specifying long messages outside the exception class
(TRY003)
326-326: Avoid specifying long messages outside the exception class
(TRY003)
329-329: Avoid specifying long messages outside the exception class
(TRY003)
336-336: 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)
588-588: 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)
⏰ 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: Backend Checks
- GitHub Check: integration-tests
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (26)
utils/the_math/aggregations.py (2)
491-505: LGTM!The divide-by-zero guard and the symmetric normalization logic for
normalized_lowersandnormalized_uppersare correctly implemented.
645-656: LGTM!The None/NaN handling for means calculation correctly:
- Identifies placeholder positions from the first forecast row
- Converts to NaN for numeric computation
- Restores None in the final result
This aligns with the MC options-history handling where None indicates an option that didn't exist at forecast time.
tests/unit/test_utils/test_the_math/test_aggregations.py (2)
26-32: LGTM!Clean import additions for the new test function and required types.
401-421: LGTM!The dynamic question type selection and NaN-aware comparison logic properly handles both binary and MC questions with placeholders. The
np.testing.assert_allclosewithequal_nan=Trueis the right approach for comparing arrays that may contain None values.templates/admin/questions/update_options.html (1)
131-174: Visibility toggles and catch‑all guard look solid.
The action-driven row visibility and catch-all disabling logic are clear and cohesive.front_end/messages/zh-TW.json (2)
1830-1836: Translations align with the new UI copy.
No issues spotted in these additions.
1882-1886: Pluralization labels and lock-help copy look good.
These read clearly for zh‑TW.front_end/messages/en.json (1)
955-955: Copy is clear and matches the admin-only restriction.front_end/messages/cs.json (1)
942-942: Looks good to add the locked-choices help text.front_end/messages/zh.json (1)
1838-1891: Translations for the new MC/grace-period UI look consistent.front_end/src/app/(main)/questions/components/question_form.tsx (1)
312-313: MC options locking behavior is solid and clear.Also applies to: 786-844
notifications/templates/emails/multiple_choice_option_deletion.html (1)
253-307: Similar posts section template logic is commented out.The Django template tags for iterating over
similar_postsand displaying post details are wrapped in HTML comments (e.g.,<!-- {% for chunk in similar_posts|chunks:2 %} -->), which prevents them from executing. The section will render static placeholder content instead of actual similar posts.If this section should be functional, remove the HTML comment wrappers around the template tags. If it's intentionally disabled, consider removing the entire section to avoid confusion.
questions/serializers/common.py (1)
461-503: LGTM!The updated
multiple_choice_validationcorrectly handles the new options history:
- Validates that all current options are present in the submission
- Ensures submitted options are within the historical option set
- Enforces proper value constraints for active vs. inactive options
- Accurately sums only non-None values
front_end/src/components/forecast_maker/forecast_maker_question/forecast_maker_multiple_choice.tsx (2)
322-348: LGTM!The
gracePeriodEndcalculation includes appropriate defensive checks:
- Validates
options_historyexistence and length- Handles undefined entries gracefully
- Validates the parsed date with
isNaNcheck- Logs warnings for debugging invalid dates
80-145: LGTM!The
NewOptionCalloutcomponent properly handles internationalization by using separate translation keys (newOptionsAddedPlural/newOptionsAddedSingular) rather than string concatenation with pluralize, addressing the localization concern from past reviews.questions/admin.py (1)
117-205: LGTM!The
MultipleChoiceOptionsAdminForm.__init__is well-designed:
- Dynamically adjusts available actions based on grace period state and history length
- Creates reorder fields dynamically for each option
- Sets sensible defaults for comments and grace period
- Properly disables reordering when options have changed (consistent with the learning about catch-all validation)
questions/services/multiple_choice_handlers.py (1)
17-80: LGTM!The
MultipleChoiceOptionsUpdateSerializercorrectly validates all MC option update scenarios:
- Handles no-change case
- Validates duplicate labels during rename (line 37 now properly raises)
- Prevents changes during grace period
- Enforces minimum option count during deletion
- Requires future grace period end for additions
Based on learnings, the catch-all position validation here properly enforces that the last option cannot change during add/delete operations.
tests/unit/test_questions/test_services/test_multiple_choice_handlers.py (3)
17-41: LGTM!Comprehensive parametrized tests covering success and failure paths for option renaming, including non-existent options and duplicate detection.
86-228: LGTM!Excellent coverage of delete scenarios including: simple deletion, absent option validation, multi-option deletion, forecast mutations with proper end_time/start_time handling, and automatic source assignment for split forecasts.
280-399: LGTM!Thorough test coverage for adding options with grace periods, including: duplicate prevention, grace period validation, forecast probability array expansion with
Noneplaceholders, and proper handling of forecasts at various time boundaries.front_end/src/app/(main)/questions/[id]/components/multiple_choices_chart_view/choices_legend/index.tsx (1)
40-43: LGTM!Clean memoization of the
currentOptionNamesSet with proper dependency tracking. This efficiently enables O(1) lookups for deleted option detection.front_end/src/app/(main)/questions/[id]/components/multiple_choices_chart_view/index.tsx (1)
24-24: LGTM!Clean prop threading of
currentOptionsthroughMultiChoicesChartViewtoChoicesLegend, enabling the legend to display deleted option indicators.Also applies to: 53-53, 315-315
front_end/src/components/detailed_question_card/detailed_question_card/multiple_choice_chart_card.tsx (4)
166-186: Tooltip choices now correctly reflect live options.
188-207: User tooltip filtering aligns with live options.
245-252: Passing current options through is sensible.
95-113: Remove this review item: the timestamp units are correct.The code correctly compares timestamps in the same units (Unix seconds). The
options_historytimestamps are ISO 8601 strings from the backend, andnew Date(timestamp).getTime() / 1000correctly converts them to seconds. ThecursorTimestampis already in seconds (derived fromaggregationTimestampswhich divides milliseconds by 1000 at line 241 ofchoices.ts), so the division by 1000 is the appropriate conversion and the comparison is valid.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
front_end/src/components/forecast_maker/forecast_maker_question/use_grace_period_countdown.ts
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 7
🤖 Fix all issues with AI agents
In
`@front_end/src/app/`(main)/questions/[id]/components/multiple_choices_chart_view/choices_legend/index.tsx:
- Around line 70-86: Replace the hard-coded " (deleted)" suffix with a next-intl
translation key and use the translator to build the label so translations can
reorder or change the suffix; import useTranslations (or the project's t()
helper), define a key like "deletedOption" that accepts a placeholder (e.g.,
{option}), then replace the label creation in the legendChoices map (where label
is set from choice and isDeleted using currentOptions/currentOptionNames) to
call t('deletedOption', { option: choice }) when isDeleted is true; apply the
same change to the other occurrence that builds a deleted label so both
ChoiceCheckbox labels use the translated string while keeping onChoiceChange and
onChoiceHighlight unchanged.
In `@notifications/templates/emails/multiple_choice_option_addition.html`:
- Line 1: Remove the stray build artifact comment at the top of the
multiple_choice_option_addition.html template (the line containing "<!-- FILE:
undefined -->"); locate the template file
notifications/templates/emails/multiple_choice_option_addition.html and delete
that debug/comment line so the file matches the cleaned pattern used in
multiple_choice_option_deletion.html, then run the template linter/build to
ensure no other generator artifacts remain.
- Around line 277-287: The conditional block around the similar-posts spans is
disabled by HTML comments so both spans always render; locate the Django
template tags {% if post.nr_forecasters %}, {% elif post.probability %} and {%
endif %} surrounding the spans that reference post.nr_forecasters and
post.probability and either (a) remove the surrounding <!-- --> so the Django
conditionals execute, or (b) replace the HTML comments with proper Django
comment syntax ({# ... #} or {% comment %} ... {% endcomment %}) if you intend
to hide those lines, ensuring only the intended span (predictor count or
probability) is rendered.
In `@notifications/templates/emails/multiple_choice_option_deletion.html`:
- Around line 277-287: The Django template conditionals around the similar-posts
metadata are commented out with HTML comments so both spans render; locate the
block using post.nr_forecasters and post.probability and replace the
HTML-commented tags with real Django tags so it reads {% if post.nr_forecasters
%} ... {% elif post.probability %} ... {% endif %}, ensuring only the correct
span is rendered and preserving the existing span markup and icons.
In `@questions/tasks.py`:
- Around line 422-427: The params dict is passing raw datetime objects for
grace_period_end and timestep; replace those values with the formatted timestamp
strings used for the comment text (the same formatted variables created earlier
for the comment), so the "params" dict under
NotificationPostParams.from_post(post) contains the human-readable/formatted
versions of grace_period_end and timestep instead of the raw datetime objects.
- Around line 330-335: The email context is receiving the raw datetime under the
"timestep" key instead of the human-readable string created earlier; update the
params dict used for the email to pass formatted_timestep (the precomputed
formatted_timestep variable) in place of timestep so the email/template gets the
formatted string (adjust the "params" entry where "timestep" is set).
- Around line 446-450: The "forecast" lookup in the defaults uses
Forecast.objects.filter(question=question,
author=forecaster).order_by("-start_time").first() but misses the end_time used
to select forecasters
(question.user_forecasts.filter(end_time=grace_period_end)), so update the
Forecast filter in the defaults to include end_time=grace_period_end and guard
against None (since UserForecastNotification.forecast is non-null) by ensuring
you either .get(...) with those three filters or check the result and
skip/create only when a matching forecast exists; update the Forecast lookup
expression (Forecast.objects.filter(...)) to include end_time=grace_period_end
and handle the missing-forecast case to avoid IntegrityError.
♻️ Duplicate comments (7)
front_end/messages/es.json (1)
1836-1839: Clarify singular/plural wording for new-options notices (ES).Line 1838 and Line 1839 still use “pronóstico(s)” and mismatch singular/plural. Suggested tightened copy:
✏️ Suggested copy tweak
- "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/en.json (1)
43-46: Tighten plural/singular wording (avoid “forecast(s)”).Line 45 and Line 46 still use “forecast(s)” and mismatched pluralization. Suggested tweak:
✏️ 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)
1837-1845: Fix Czech plural form for “days” (“dní”).
other {# dni}should beother {# dní}for correct Czech orthography.🛠️ Proposed fix
- "periodDays": "{count, plural, one {# den} few {# dny} other {# dni}}", + "periodDays": "{count, plural, one {# den} few {# dny} other {# dní}}",notifications/templates/emails/multiple_choice_option_addition.html (1)
213-213: Localize remaining English UI strings in the email.The CTA button text "Update your forecast" and the predictor/chance text in the similar posts section (lines 279-286) are hardcoded in English. These should use translation tags for consistency with the rest of the template.
notifications/templates/emails/multiple_choice_option_deletion.html (1)
1-1: Remove debug artifact from template.This issue was already flagged in a previous review. Line 1 contains
<!-- FILE: undefined -->which should be removed.questions/tasks.py (2)
271-271: Guard against missing post before creating comments and notifications.If a question lacks a post, accessing
question.postor callingNotificationPostParams.from_post(post)will fail. This was flagged in a previous review. Add an early guard to bail out with a warning.🛡️ Suggested guard
question = Question.objects.get(id=question_id) post = question.post + if not post: + logging.warning( + "MC option-change notification skipped: question %s has no post", + question_id, + ) + return options_history = question.options_history
351-351: Guard against missing post (same as delete function).Same issue as in
multiple_choice_delete_option_notifications- add a guard for missing post.
🧹 Nitpick comments (6)
notifications/templates/emails/multiple_choice_option_deletion.mjml (1)
51-53: Localize the CTA button text.The button text "Review the question" is hardcoded in English. For consistency with the i18n blocks used elsewhere in this template, wrap it in a translation block.
🌐 Suggested localization
<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_deletion.html (1)
212-212: Localize the CTA button text.The button text "Review the question" should be wrapped in a translation tag for i18n consistency.
🌐 Suggested localization
- <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>questions/tasks.py (4)
276-276: Consider guarding against empty options list.If
question.optionsis unexpectedly empty, indexing[-1]will raise anIndexError. While this is unlikely given the context, a defensive check would be safer.🛡️ Suggested guard
- catch_all_option = question.options[-1] if question.options else "" + if not question.options: + logging.warning( + "MC option-change notification skipped: question %s has no options", + question_id, + ) + return + catch_all_option = question.options[-1]
296-306: Consider narrowing the exception type.Catching bare
Exceptioncan hide unexpected errors. For string formatting, catchingKeyErrorandValueErrorwould be more precise while still handling missing/invalid placeholders.♻️ Suggested refinement
try: text = template.format( removed_options=removed_options_text, timestep=formatted_timestep, catch_all_option=catch_all_option, ) - except Exception: + except (KeyError, ValueError) as e: + logging.warning("Failed to format MC option deletion comment: %s", e) text = ( f"{template} (removed options: {removed_options_text}, " f"at {formatted_timestep}, catch-all: {catch_all_option})" )
437-452: Redundant delete beforeupdate_or_create.The
delete()call on lines 437-439 is unnecessary sinceupdate_or_create()already handles existing records with the same unique key (user,question). This creates an extra database query for each forecaster.♻️ Suggested simplification
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( user=forecaster, question=question,
384-394: Consider narrowing the exception type (same as delete function).Same feedback as the deletion function - catching bare
Exceptioncan hide unexpected errors.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (13)
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.tsxnotifications/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/tasks.py
🚧 Files skipped from review as they are similar to previous changes (4)
- front_end/src/app/(main)/questions/components/question_form.tsx
- front_end/messages/zh.json
- front_end/messages/pt.json
- notifications/templates/emails/multiple_choice_option_addition.mjml
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
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.
📚 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:
questions/tasks.py
🧬 Code graph analysis (1)
questions/tasks.py (9)
posts/admin.py (2)
comments(259-260)author(246-247)questions/models.py (4)
Forecast(576-712)Question(60-448)UserForecastNotification(798-823)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 (1)
get_forecasts_per_user(367-379)utils/email.py (1)
send_email_with_template(13-50)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)
🪛 Ruff (0.14.13)
questions/tasks.py
302-302: Do not catch blind exception: Exception
(BLE001)
390-390: Do not catch blind exception: Exception
(BLE001)
⏰ 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: Frontend Checks
- GitHub Check: Backend Checks
- GitHub Check: integration-tests
🔇 Additional comments (8)
front_end/messages/es.json (1)
1885-1889: LGTM for period labels and choices lock help.Clear, consistent ICU pluralization and phrasing.
front_end/messages/en.json (1)
955-955: LGTM for choicesLockedHelp.Copy is clear and consistent with the UI intent.
front_end/messages/zh-TW.json (2)
1830-1836: LGTM for the new MC‑options messaging strings.
Translations read cleanly and align with the new UI surface.
1882-1886: LGTM for the period unit strings and choices‑locked help text.
Pluralization is consistent with existing zh‑TW patterns.front_end/messages/cs.json (1)
942-942: LGTM for the choices‑locked helper text.
Clear and consistent with the admin‑only edit policy.front_end/src/app/(main)/questions/[id]/components/multiple_choices_chart_view/choices_legend/index.tsx (1)
14-43: Good addition ofcurrentOptions+ memoized Set.Clean, low-cost way to enable O(1) “deleted” checks without cluttering render logic.
notifications/templates/emails/multiple_choice_option_deletion.mjml (1)
1-59: LGTM - Well-structured MJML template.The template follows established patterns with proper includes, conditional rendering for removed options, and appropriate i18n blocks for the main content.
questions/tasks.py (1)
263-339: Overall structure looks good for delete notifications.The function follows the established pattern in this file, properly handles datetime formatting, excludes unsubscribed users, and sends notifications. The main concerns (missing post guard, timestamp format) are noted separately.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
...rc/app/(main)/questions/[id]/components/multiple_choices_chart_view/choices_legend/index.tsx
Outdated
Show resolved
Hide resolved
lsabor
force-pushed
the
feat/mc-options-change
branch
from
9a97313 to
3ccc18f
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
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: Assertions lack NaN-aware handling for placeholder test cases.Unlike
test_UnweightedAggregation(lines 412-422), this method doesn't convertNonetoNaNbefore comparison. When the placeholder test case (lines 610-637) runs,np.allclosewill fail or behave unexpectedly withNonevalues in the arrays.🐛 Suggested fix: Use consistent NaN-aware comparison
- 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) + assert new_aggregation.forecaster_count == expected.forecaster_count
🤖 Fix all issues with AI agents
In `@notifications/templates/emails/multiple_choice_option_deletion.mjml`:
- Around line 49-53: The CTA label "Review the question" inside the <mj-button
mj-class="cta-button" href="{% post_url params.post.post_id
params.post.post_title %}"> should be localized like the rest of the template;
replace the hardcoded text with the project's translation tag (the same
translation helper used elsewhere in these MJML templates) so the button text is
rendered via i18n instead of a literal string.
♻️ Duplicate comments (8)
front_end/messages/es.json (1)
1838-1839: Fix singular/plural wording in new-options notices.The singular and plural messages have awkward phrasing that was previously flagged.
front_end/messages/en.json (1)
43-46: Tighten singular/plural copy (“forecast(s)”).
This mirrors a previously flagged issue; the singular/plural wording is still inconsistent.✏️ 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” (other form).
other {# dni}should beother {# dní}in Czech.🛠️ Suggested fix
- "periodDays": "{count, plural, one {# den} few {# dny} other {# dni}}", + "periodDays": "{count, plural, one {# den} few {# dny} other {# dní}}",notifications/templates/emails/multiple_choice_option_addition.html (2)
1-1: Remove stray build artifact comment.
The top-level<!-- FILE: undefined -->looks like a generator artifact and shouldn’t ship.🧹 Suggested fix
-<!-- FILE: undefined --> {% load i18n %} {% load static %} {% load urls %} {% load utils %}
213-214: Localize CTA and similar-post metadata strings.
These are still hardcoded English; use translation tags and proper pluralization.🌐 Suggested localization
- <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>- {{ post.nr_forecasters }} predictor{{ post.nr_forecasters|pluralize }} + {% blocktrans count count=post.nr_forecasters %} + {{ count }} predictor + {% plural %} + {{ count }} predictors + {% endblocktrans %} ... - {{ post.probability }}% chance </span> + {% blocktrans with probability=post.probability %} + {{ probability }}% chance + {% endblocktrans %}</span>Also applies to: 279-286
questions/services/multiple_choice_handlers.py (1)
126-135: Persist renamed options to the database.
The rename mutatesquestion.optionsandquestion.options_historyin memory but never saves, so the change can be lost.🛠️ Suggested 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/tasks.py (2)
327-335: Pass formatted timestamps into the email context.
You already compute formatted values; the template currently gets raw datetimes.🧭 Suggested fix
"params": { "post": NotificationPostParams.from_post(post), "removed_options": removed_options, - "timestep": timestep, + "timestep": formatted_timestep, "catch_all_option": catch_all_option, },"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: 422-427
440-450: Ensure the reminder links to the correct forecast.
The defaults query doesn’t filter byend_time=grace_period_end, so it can select the wrong forecast orNone(risking a NOT NULL error).🛠️ Suggested fix
+ 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.objects.filter( - question=question, author=forecaster - ) - .order_by("-start_time") - .first(), + "forecast": forecast, }, )
🧹 Nitpick comments (2)
tests/unit/test_utils/test_the_math/test_aggregations.py (2)
72-80: Misleading comment: forecasts are not uniform.The comment "3 unwavaring forecasts" is inaccurate for this case—the forecasts
[0.2, 0.8],[0.5, 0.5],[0.8, 0.2]are clearly different. Also, "unwavaring" should be "unvarying".✏️ Suggested fix
- ), # 3 unwavaring forecasts + ), # 3 varying forecasts
108-111: Consider addingstrict=Truetozip()calls.Adding
strict=Trueensures both iterables have equal length, catching shape mismatches early during test execution rather than silently truncating.✏️ 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)
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (16)
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.tsxnotifications/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/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 (5)
- front_end/src/app/(main)/questions/components/question_form.tsx
- notifications/templates/emails/multiple_choice_option_addition.mjml
- front_end/src/app/(main)/questions/[id]/components/multiple_choices_chart_view/choices_legend/index.tsx
- notifications/templates/emails/multiple_choice_option_deletion.html
- front_end/messages/zh-TW.json
🧰 Additional context used
🧠 Learnings (4)
📓 Common learnings
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.
📚 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/tasks.pytests/unit/test_questions/test_services/test_multiple_choice_handlers.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:
tests/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:
questions/services/multiple_choice_handlers.py
🧬 Code graph analysis (2)
tests/unit/test_utils/test_the_math/test_aggregations.py (1)
utils/the_math/aggregations.py (2)
compute_weighted_semi_standard_deviations(115-141)ForecastSet(52-56)
questions/tasks.py (7)
comments/services/common.py (1)
create_comment(78-130)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/services/forecasts.py (2)
build_question_forecasts(420-461)get_forecasts_per_user(367-379)utils/email.py (1)
send_email_with_template(13-50)posts/models.py (1)
get_forecasters(914-921)notifications/services.py (2)
NotificationPostParams(46-68)from_post(63-68)
🪛 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/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: integration-tests
- GitHub Check: Frontend Checks
- GitHub Check: Backend Checks
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (17)
front_end/messages/zh.json (2)
1838-1845: LGTM!The new localization keys for the MC options change feature are correctly translated. The identical translations for
showNewOptionsandshowNewOptionare appropriate since Chinese does not grammatically distinguish singular and plural forms.
1887-1891: LGTM!The period and help text translations are correct. The ICU plural format is properly used even though Chinese typically doesn't require distinct plural forms.
front_end/messages/es.json (2)
1836-1837: LGTM!The translations for
dismiss,gracePeriodTooltip,showNewOptions,showNewOption,timeRemaining, andothersCountare correct and natural in Spanish.Also applies to: 1840-1843
1885-1890: LGTM!The period translations and
choicesLockedHelpcorrectly use ICU plural format with proper Spanish singular (día,hora,minuto,segundo) and plural (días,horas,minutos,segundos) forms.front_end/messages/pt.json (2)
1834-1841: LGTM — PT additions read well.
The grace‑period and new‑option strings are clear and consistent.
1883-1887: LGTM — pluralization + lock help text look solid.
Plural forms and the lock guidance read naturally.front_end/messages/en.json (1)
955-955: LGTM — choicesLockedHelp copy is clear.tests/unit/test_utils/test_the_math/test_aggregations.py (4)
26-32: LGTM!New imports for the
compute_weighted_semi_standard_deviationsfunction and associated type hints are appropriate for the added test coverage.
308-389: LGTM!The multiple choice placeholder test cases are well-structured with correct alignment between
forecasts_valuescount andtimestepslength. The expected outputs withNoneplaceholders appropriately test the NaN-aware aggregation logic.
402-422: LGTM!The dynamic question type selection and NaN-aware comparison logic correctly handle both binary and multiple choice scenarios with placeholders.
610-637: Test case is valid, but assertions may fail with None values.This test case includes
Noneplaceholders, but the assertion logic intest_RecencyWeightedAggregation(lines 660-683) usesnp.allclosedirectly without NaN-aware handling. See next comment for details.questions/services/multiple_choice_handlers.py (4)
17-105: Validation and history helper look solid.
Clean branching for add/delete/rename and a straightforward history merge.
138-185: Reorder flow and forecast remap look consistent.
The remap logic matches the example and keeps aggregates in sync.
192-299: Delete-options path and forecast slicing look correct.
The PMF migration + history update matches the intended semantics.
302-375: Add-options + grace-period updates are coherent.
PMF expansion and forecast termination align with the grace-period model.tests/unit/test_questions/test_services/test_multiple_choice_handlers.py (1)
230-278: Solid coverage for delete-option scenarios.
The parametrized cases exercise edge conditions and forecast slicing well.questions/tasks.py (1)
323-325: The code is already safe. Django'ssend_mail()function withrecipient_listparameter sends individual emails to each recipient, with only their own address in the To field. Each forecaster receives their own email, not a bulk send exposing all recipients. The current implementation follows Django best practices and poses no privacy/compliance risk. The suggested per-recipient loop is unnecessary and would be redundant with what Django already does internally.Likely an incorrect or invalid review comment.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
lsabor
force-pushed
the
feat/mc-options-change
branch
from
3ccc18f to
3132d90
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
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/utils/questions/choices.ts (1)
74-125: Upcoming options are mislabeled as deleted (and options may be undefined).
Line 75-124 marksisDeletedbeforeisUpcoming, so future options (not yet inquestion.options) get the “(deleted)” label. Also,question.optionsis optional and can throw onincludes.🐛 Suggested fix
- const choiceItems: ChoiceItem[] = labels.map((choice, index) => { - const isDeleted = !question.options.includes(choice); - const isUpcoming = upcomingOptions.includes(choice); + const currentOptions = question.options ?? []; + const choiceItems: ChoiceItem[] = labels.map((choice, index) => { + const isUpcoming = upcomingOptions.includes(choice); + const isDeleted = !currentOptions.includes(choice) && !isUpcoming; const userValues: (number | null)[] = []; const aggregationValues: (number | null)[] = []; @@ return { choice: choice, - label: isDeleted - ? choice + " (deleted)" - : isUpcoming - ? choice + " (upcoming)" - : choice, + label: isUpcoming + ? choice + " (upcoming)" + : isDeleted + ? choice + " (deleted)" + : choice, color: MULTIPLE_CHOICE_COLOR_SCALE[index] ?? METAC_COLORS.gray["400"],
🤖 Fix all issues with AI agents
In `@front_end/src/utils/questions/helpers.ts`:
- Around line 203-216: getAllOptionsHistory currently only builds allOptions
from question.options_history and then appends the last element of
question.options, which loses current options if options_history is empty;
update getAllOptionsHistory to fall back to iterating question.options when
question.options_history is null/empty, dedupe entries (use allOptions.includes
check already present) and ensure the catch-all "other" from question.options
(the last element) is only added if not already present; reference function
getAllOptionsHistory, properties question.options_history and question.options,
and variable allOptions to locate and modify the logic.
In `@notifications/templates/emails/multiple_choice_option_deletion.html`:
- Around line 211-213: The CTA anchor contains a hardcoded English label "Review
the question"; replace it with a translatable string using Django i18n (e.g.,
wrap the text in a {% trans %} or use a translation template variable) so the
link text is localizable; update the anchor element that currently contains the
literal "Review the question" (the CTA inside the <a ...> tag) to use the
translation tag or translated variable and ensure any required i18n tags/loaders
are present in the template.
In `@questions/services/multiple_choice_handlers.py`:
- Around line 290-297: The code currently calls the Dramatiq actors
multiple_choice_delete_option_notifications and
multiple_choice_add_option_notifications directly (e.g.,
multiple_choice_delete_option_notifications(...)), which runs them
synchronously; change those calls to use the actor .send() API (e.g.,
multiple_choice_delete_option_notifications.send(...)) and pass the same keyword
arguments (question_id, timestep, comment_author_id, comment_text) so the tasks
are queued asynchronously; locate the usage of
multiple_choice_delete_option_notifications and
multiple_choice_add_option_notifications in multiple_choice_handlers.py and
replace the direct invocations with .send() calls.
♻️ Duplicate comments (14)
front_end/messages/es.json (1)
1838-1839: Fix singular/plural copy for new-options notices (ES).
Current wording is awkward and uses “(s)”; align singular vs plural.Proposed wording
- "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-1844: Fix Czech plural “days” other‑form.
Line 1844 should use “dní”, not “dni”.🛠️ Suggested fix
- "periodDays": "{count, plural, one {# den} few {# dny} other {# dni}}", + "periodDays": "{count, plural, one {# den} few {# dny} other {# dní}}",notifications/templates/emails/multiple_choice_option_deletion.html (2)
277-287: Prefer real Django conditionals over HTML-comment wrappers.
Lines 277-287 are wrapped in HTML comments; consider removing them to avoid confusing output and keep conditionals explicit.♻️ Cleanup suggestion
- <!-- {% 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 %}
1-2: Remove generator artifact from template header.
Line 1 looks like a build artifact and should be removed.🛠️ Proposed fix
-<!-- FILE: undefined --> {% load i18n %} {% load static %} {% load urls %} {% load utils %}notifications/templates/emails/multiple_choice_option_addition.html (3)
277-287: Prefer real Django conditionals over HTML-comment wrappers.
Lines 277-287 are wrapped in HTML comments; consider removing them to keep template logic explicit.♻️ Cleanup suggestion
- <!-- {% 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 %}
1-2: Remove generator artifact from template header.
Line 1 appears to be a build artifact.🛠️ Proposed fix
-<!-- FILE: undefined --> {% load i18n %} {% load static %} {% load urls %} {% load utils %}
213-214: Localize remaining English UI strings.
Lines 213-214 and 281-286 are hardcoded; wrap them in translation tags.🌐 Suggested localization
- <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>- {{ post.nr_forecasters }} predictor{{ post.nr_forecasters|pluralize }} + {% blocktrans count count=post.nr_forecasters %} + {{ count }} predictor + {% plural %} + {{ count }} predictors + {% endblocktrans %} ... - {{ post.probability }}% chance </span> + {% blocktrans with probability=post.probability %} + {{ probability }}% chance + {% endblocktrans %}</span>Also applies to: 277-286
notifications/templates/emails/multiple_choice_option_deletion.mjml (1)
49-53: Localize the CTA label.
Line 52 is hardcoded; wrap it in a translation tag for i18n parity.🔧 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>questions/tasks.py (4)
330-335: Pass formatted timestamp to email context.The
timesteppassed to the email context (line 333) is the rawdatetimeobject, butformatted_timestepwas created for the comment. The template likely expects the human-readable string.🐛 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.Similar to the deletion function, 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, },
437-451: Redundant delete before update_or_create and missingend_timefilter.Two issues here:
The
delete()on line 439 is unnecessary sinceupdate_or_createwill update existing records. This adds an extra database operation.The
forecastersqueryset filters users by forecasts withend_time=grace_period_end, but the forecast lookup indefaultsdoesn't include this filter. This retrieves the most recent forecast rather than the one that triggered the notification. Additionally,.first()returnsNoneif no forecast exists, which will cause anIntegrityErrorsinceUserForecastNotification.forecasthasnull=False.🐛 Proposed fix
if grace_period_end - timedelta(days=1) > timestep: 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.objects.filter( - question=question, author=forecaster - ) - .order_by("-start_time") - .first(), + "forecast": forecast, }, )
271-271: Guard against missing post.If
question.postisNone,NotificationPostParams.from_post(post)on line 331 will raise anAttributeError. Consider adding a guard or usingquestion.get_post()with a null check.🛡️ Suggested guard
question = Question.objects.get(id=question_id) post = question.post + if not post: + logging.warning( + "MC option-change notification skipped: question %s has no post", + question_id, + ) + return options_history = question.options_historyAlso applies to line 351 in
multiple_choice_add_option_notifications.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.🐛 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/serializers/common.py (1)
235-250: Validation incorrectly blocks unrelated updates when forecasts exist.The condition
data.get("options") != question.optionsis true whenoptionsis not provided indata(returnsNone) but the question has options. This incorrectly blocks updates to other fields when forecasts exist.🐛 Proposed fix
def validate(self, data: dict): data = super().validate(data) if qid := data.get("id"): question = Question.objects.get(id=qid) - if data.get("options") != question.options: + if "options" in data and data.get("options") != question.options: # if there are user forecasts, we can't update options this way if question.user_forecasts.exists(): raise ValidationError( "Cannot update options through this endpoint while there are " "user forecasts. " "Instead, use /api/questions/update-mc-options/ or the UI on " "the question detail page." ) return data
🧹 Nitpick comments (7)
tests/unit/test_utils/test_the_math/test_aggregations.py (1)
108-111: Addstrict=Truetozip()for safer iteration.Using
zip()withoutstrict=Truecan silently truncate if the expected and result lists have different lengths, potentially masking test failures.♻️ Proposed fix
for v, e in zip(rl, el): - np.testing.assert_approx_equal(v, e) + 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/tasks.py (1)
296-306: Catch specific exceptions instead of bareException.Catching bare
Exceptioncan mask unexpected errors. Consider catchingKeyErrororValueErrorwhich are the likely failures fromstr.format().♻️ Proposed fix
try: text = template.format( removed_options=removed_options_text, timestep=formatted_timestep, catch_all_option=catch_all_option, ) - except Exception: + except (KeyError, ValueError): text = ( f"{template} (removed options: {removed_options_text}, " f"at {formatted_timestep}, catch-all: {catch_all_option})" )Also applies to line 390.
front_end/src/components/forecast_maker/forecast_maker_question/forecast_maker_multiple_choice.tsx (2)
616-622: Simplify conditional callback invocation.The ternary returns
undefinedon the false branch, which is effectively a no-op. A simple conditional call is clearer.♻️ Proposed fix
onInteraction={() => { - isNewOption - ? setInteractedOptions((prev) => - new Set(prev).add(choice.name) - ) - : undefined; + if (isNewOption) { + setInteractedOptions((prev) => + new Set(prev).add(choice.name) + ); + } }}
763-765: Simplify redundant findIndex callback.The callback
(_, index) => allOptions[index] === question.resolutionis equivalent to just checking the value at each index. UsingindexOfis clearer.♻️ Proposed fix
- const resolutionIndex = allOptions.findIndex( - (_, index) => allOptions[index] === question.resolution - ); + const resolutionIndex = allOptions.indexOf(question.resolution);questions/services/multiple_choice_handlers.py (1)
248-258: Addstrict=Truetozip()for safety.If
previous_pmflength doesn't matchall_options, the validation on line 242-246 catches it. However, addingstrict=Trueprovides defense-in-depth.♻️ Proposed 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 (1)
273-277: Addstrict=Truetozip()and verify source defaults.The
zip()withoutstrict=Truecould mask length mismatches. Also, someexpected_forecastsentries don't explicitly setsource, which will compare against the model default. Ensure this is intentional.♻️ Proposed fix for zip
- for f, e in zip(forecasts, expected_forecasts): + for f, e in zip(forecasts, expected_forecasts, strict=True):Also applies to line 452.
questions/serializers/common.py (1)
684-699: Reduced mutation of cached model instance.The previous approach mutated
last_forecast.start_timedirectly. The new approach only modifiesend_timeon the list copy, which is safer. However, line 698 still mutateslast_forecast.start_timewhenuser_forecastsis empty.Consider documenting that this mutation is intentional and scoped to serialization, or creating a shallow copy for the empty case as well:
else: # Create copy to avoid mutating cached instance last_forecast = copy.copy(last_forecast) last_forecast.start_time = timezone.now() user_forecasts = [last_forecast]
📜 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 (7)
- front_end/src/app/(main)/questions/components/question_form.tsx
- notifications/templates/emails/multiple_choice_option_addition.mjml
- front_end/messages/zh.json
- front_end/src/app/(main)/questions/[id]/components/multiple_choices_chart_view/choices_legend/index.tsx
- front_end/messages/en.json
- front_end/messages/pt.json
- front_end/src/components/detailed_question_card/detailed_question_card/multiple_choice_chart_card.tsx
🧰 Additional context used
🧠 Learnings (4)
📓 Common learnings
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.
📚 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:
questions/tasks.pytests/unit/test_utils/test_the_math/test_aggregations.pyquestions/serializers/common.pyquestions/services/multiple_choice_handlers.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:
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.pyquestions/services/multiple_choice_handlers.pytests/unit/test_questions/test_services/test_multiple_choice_handlers.py
🧬 Code graph analysis (4)
front_end/src/utils/questions/helpers.ts (1)
front_end/src/types/question.ts (1)
Question(266-311)
front_end/src/components/forecast_maker/forecast_maker_question/forecast_maker_multiple_choice.tsx (3)
front_end/src/types/fetch.ts (1)
ErrorResponse(35-41)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/utils/questions/choices.ts (1)
front_end/src/utils/questions/helpers.ts (2)
getAllOptionsHistory(203-217)getUpcomingOptions(219-232)
questions/services/multiple_choice_handlers.py (2)
questions/models.py (5)
Question(60-448)Forecast(576-712)QuestionType(80-85)save(291-319)save(708-712)questions/tasks.py (2)
multiple_choice_delete_option_notifications(264-339)multiple_choice_add_option_notifications(343-452)
🪛 Ruff (0.14.13)
questions/tasks.py
302-302: Do not catch blind exception: Exception
(BLE001)
390-390: Do not catch blind exception: Exception
(BLE001)
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/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)
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). (4)
- GitHub Check: integration-tests
- GitHub Check: Backend Checks
- GitHub Check: Frontend Checks
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (22)
front_end/messages/es.json (1)
1885-1889: LGTM for period units + lock-help copy.
Strings read naturally and pluralization looks correct.front_end/messages/zh-TW.json (2)
1830-1836: LGTM for new MC option/grace-period strings (zh‑TW).
Copy is clear and consistent.
1882-1886: LGTM for period unit labels + lock-help text (zh‑TW).
Plural forms and wording look good.front_end/messages/cs.json (1)
942-942: LGTM — clear and context‑appropriate Czech copy.front_end/src/types/choices.ts (1)
8-16: Optional label field fits history-aware rendering.
Line 11 looks good and keeps existing callers compatible.front_end/src/utils/questions/helpers.ts (1)
219-231: Upcoming-options detection looks good.
Line 219-231 has sensible guards for missing history and future-effective entries.tests/unit/test_utils/test_the_math/test_aggregations.py (3)
55-111: Good test coverage for semi-standard deviations with placeholders.The parameterized tests cover trivial cases, uniform forecasts, varying forecasts, and placeholder handling. The test structure is clean.
308-389: Well-structured test cases for multiple choice with placeholders.The new test cases properly validate aggregation behavior when forecast values contain
Noneplaceholders, covering both 2-forecaster and 3-forecaster scenarios with expected bounds and means.
412-422: Good approach for NaN-safe assertions.Converting
Nonetonp.nanand usingequal_nan=Trueis the right way to handle placeholder comparisons in aggregation tests.front_end/src/components/forecast_maker/forecast_maker_question/forecast_maker_multiple_choice.tsx (4)
587-627: Consider using filter instead of conditional return in map.The current pattern returns
undefinedfor non-current options inside the map, which works but is less idiomatic. However, since you need access tochoicedata for rendering, the current approach is acceptable.
84-149: Well-implemented NewOptionCallout component.The component handles pluralization, grace period countdown display, and provides clear user actions. The tooltip for grace period explanation is a nice UX touch.
326-352: Good defensive coding for grace period calculation.The try/catch with detailed null checks and validation ensures graceful handling of malformed or missing options history data.
272-295: Clean implementation of option-to-forecast value mappings.Using
useMemowithgetAllOptionsHistoryto create stable Maps for forecast value lookups is a good approach for handling the history-aware option system.questions/services/multiple_choice_handlers.py (3)
261-264: Branch is reachable - the query filters byend_time, notstart_time.Looking at lines 235-238, the queryset filters forecasts by
end_timeonly (end_time__isnull=Trueorend_time__gt=timestep). Forecasts withstart_time >= timestepcan be included. This branch is valid and necessary.The past review comment claiming this was unreachable was incorrect - the query does not filter on
start_time__lt=timestep.
17-80: Comprehensive validation logic for MC options updates.The serializer properly handles all cases: renaming (duplicate check), deletion (minimum 2 options, catch-all preserved), and addition (requires future grace period, catch-all preserved). The grace period enforcement prevents changes during active grace periods.
83-105: Clean utility for extracting all options from history.The function correctly preserves insertion order and ensures the catch-all option remains last. The docstring with example is helpful.
tests/unit/test_questions/test_services/test_multiple_choice_handlers.py (3)
86-228: Comprehensive test coverage for delete operations.The parameterized tests cover key scenarios: simple deletion, multi-option deletion, forecast slicing, preserving old forecasts, handling forecasts at/after timestep, invalid PMF detection, and no-op cases. Well structured.
280-399: Comprehensive test coverage for add operations.Tests cover: simple addition, duplicate prevention, multi-option addition, grace period validation, forecast PMF extension with None placeholders, and preserving historical forecasts. The edge cases around grace period timing are well tested.
152-159: The test case expectation is correct. Whenforecast.start_time >= timestep, the handler only updates theprobability_yes_per_categoryfield and leavessourceuntouched, preserving the original forecast's source value. Since the input forecast doesn't specify a source (defaults to empty string ""), the expected forecast correctly also has no source specified, matching the handler's behavior of preserving the source field.Likely an incorrect or invalid review comment.
questions/serializers/common.py (3)
461-503: Well-structured multiple choice validation with history awareness.The updated validation correctly:
- Validates against current options as a subset requirement
- Validates submitted keys against all historical options
- Enforces value constraints per option type (current vs inactive)
- Provides clear error messages with context
127-130: Clean implementation of all_options_ever serializer method.Delegates to the shared utility function and handles the case where options_history doesn't exist by returning
Noneimplicitly.
872-874: Correct use of history-aware options for resolution validation.Using
get_all_options_from_historyensures that resolutions can be set to any option that ever existed, not just current options.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
lsabor
force-pushed
the
feat/mc-options-change
branch
from
3132d90 to
0a09872
Compare
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
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
| label: isDeleted | ||
| ? option + " (deleted)" | ||
| : isUpcoming | ||
| ? option + " (upcoming)" | ||
| : option, |
There was a problem hiding this comment.
Can you add translations here as well?
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.