-
Notifications
You must be signed in to change notification settings - Fork 481
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
Fall back when no recommended course version #52919
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is super important work- thank you for cleaning it up!! Only two (related) comments.
const stableVersions = Object.values(courseVersions).filter( | ||
version => version.is_stable | ||
); | ||
courseVersionId = stableVersions[stableVersions.length - 1]?.id; |
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.
Do we know for sure that these will always be in chronological order?
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.
ah, no, that was just an assumption I made 😅 I'll check the api, and see if I need to sort these by something instead.
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.
the API isn't sorting, and just returns things in whatever order they come from sql. which should typically be oldest things first, but there's no guarantee. it looks like the key
is consistently a "version year", so I'm sorting by that instead. unless you know of any exceptions to that rule?
courseOfferings: noRecommendedVersionsOfferings, | ||
}); | ||
|
||
const radio = wrapper.find('input'); |
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.
Thanks for adding this test! The only thing I can think of would be to add one more stable version of a different year to ensure this is grabbing the most recent one.
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.
Thanks for adding this! I *think your instincts were right that it's sorted somewhere else, but I appreciate this explicitly residing here in case we ever change the display sorting
When there are no recommended course versions for a given selection, the section setup flow was raising an exception and not allowing certain courses to be assigned. Instead, we should fall back to the most recent stable course version. (Thanks Bethany for knowing what that behavior should be.)
Links