-
Notifications
You must be signed in to change notification settings - Fork 315
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
Handle request for new variant when assessment is not available #9803
Handle request for new variant when assessment is not available #9803
Conversation
If a non-broken variant exists, return the most recent one. Otherwise, tell the user that a new variant cannot be generated.
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.
Not a full review, only a first pass.
apps/prairielearn/src/pages/studentInstanceQuestion/studentInstanceQuestion.ejs
Outdated
Show resolved
Hide resolved
apps/prairielearn/src/pages/studentInstanceQuestion/studentInstanceQuestion.ejs
Outdated
Show resolved
Hide resolved
apps/prairielearn/src/pages/studentInstanceQuestion/studentInstanceQuestion.js
Outdated
Show resolved
Hide resolved
apps/prairielearn/src/pages/studentInstanceQuestion/studentInstanceQuestion.js
Outdated
Show resolved
Hide resolved
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'm happy with the way things are done here, but I'd prefer to defer to @mwest1066 or @nwalters512 for an additional look.
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.
A few stylistic nits, nothing blocking though. @echau01 let me know when you're ready for this to be merged.
apps/prairielearn/src/pages/studentInstanceQuestion/studentInstanceQuestion.js
Show resolved
Hide resolved
@echau01 there's a failing test that you'll need to fix before this can be merged. |
Exam questions are randomized, so we might visit the "broken generation" question and assume that a variant was created. Let's just remove the broken question from the test exam.
@nwalters512 I think everything's ready for merging now. |
Resolves #9030, resolves #1718.
If an assessment is not open or not active, and a student clicks a question in the assessment:
In both cases, we avoid generating a new variant.
Note: in my implementation, we do not show a broken variant. Not sure if this is right, but we can discuss it.