-
Notifications
You must be signed in to change notification settings - Fork 22
Auto-close conditionals where child resolved before parent #1543
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
- Fixed unit tests
…to fix/1008-conditional-closure
| {t("predictionConditionalPartiallyClosedMessage", { | ||
| closedQuestion: closedQuestion.title, | ||
| activeQuestion: activeQuestion.title, | ||
| })} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think there should ever be a state where there is one close and one open branch?
What situation are you imagining?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we're already in the PostStatus.CLOSED situation, then how would there be an active branch question?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This just seems like unreachable code, so not blocking, just unnecessary
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@lsabor
It's not about Yes/No branches, but condition parent and condition child.
We have an example of this -- https://www.metaculus.com/questions/22051/starship-booster-tower-catch-attempt-in-2024/
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good tests
This might be asking you to do my work for me, but it would be great to have tests for "unresolve" parent / child actions as well.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure. I added a test case base, so you could expand it in the future. Though, I probably found a bug:
https://github.com/Metaculus/metaculus/pull/1543/files#diff-8f63f33d62c6c58b08f8ab06d0171c596d83d352ab6619e3b140831b779f9e65R132-R133
lsabor
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me.
I left a comment on prediction_status_message that should be looked at, but not blocking.
The other thing is optional addition of tests for conditional question unresolve. (If you don't want to do it, I can add those tests in a later PR).
Revisited auto-close logic for conditional questions:
PredictionStatusMessage: it refactors Forecast MakerpredictionMessagecomponent and generates status message JSX instead of string lang.Post.set_actual_close_timelogic for conditional questions: not it auto-closes parent if Condition OR Child was closedPostReadSerialize.get_statusgeneration. Now it respectsactual_close_timedatesTODO:
closes #1008