feat: Enable copying of publicly shared question code (if allowed by instructor) #9800
Conversation
All images
|
...learn/src/pages/instructorCopyTemplateCourseQuestion/instructorCopyTemplateCourseQuestion.ts
Outdated
Show resolved
Hide resolved
|
I am aware that this doesn't actually make a way to mark the Doing that is going to be a bunch of UI design/mess on the question settings page, I thought it would be nice to do that in a separate PR from this. |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #9800 +/- ##
==========================================
- Coverage 67.40% 67.40% -0.01%
==========================================
Files 458 458
Lines 71712 71715 +3
Branches 5786 5782 -4
==========================================
+ Hits 48336 48338 +2
- Misses 22939 22940 +1
Partials 437 437 ☔ View full report in Codecov by Sentry. |
apps/prairielearn/src/server.js
Outdated
| '/pl/course/:course_id(\\d+)/copy_template_course_question', | ||
| ( | ||
| await import( | ||
| './pages/instructorCopyTemplateCourseQuestion/instructorCopyTemplateCourseQuestion.js' | ||
| ) | ||
| ).default, |
There was a problem hiding this comment.
Removing a route like this is generally not safe; what happens when someone tries to copy a question during a deploy? I agree that we want to rename this route, but we should do it with roughly the same strategy we'd use for any database columns:
- Add the new route; deploy
- Update anything referencing the old route to instead point to the new route; deploy
- Remove the old route
There was a problem hiding this comment.
Sounds good. I'll put the new route (and leave the old route) in this PR, and make follow-up PRs to do the other things.
apps/prairielearn/src/migrations/20240501185744_questions__shared_publicly_source__add.sql
Outdated
Show resolved
Hide resolved
| @@ -19,7 +19,7 @@ const sql = sqldb.loadSqlEquiv(import.meta.url); | |||
| export async function setQuestionCopyTargets(res: Response) { | |||
| // Avoid querying for editable courses if we won't be able to copy this | |||
| // question anyways. | |||
| if (!res.locals.course.template_course) { | |||
| if (!(res.locals.course.template_course || res.locals.question.shared_publicly_with_source)) { | |||
There was a problem hiding this comment.
Total nit: IMO the version of this where ! is distributed is easier to parse, what do you think?
| if (!(res.locals.course.template_course || res.locals.question.shared_publicly_with_source)) { | |
| if (!res.locals.course.template_course && !res.locals.question.shared_publicly_with_source) { |
Ditto for the similar conditional on the instructorCopyPublicQuestion page. Feel free to ignore if you disagree.
There was a problem hiding this comment.
It doesn't really matter to me either way, I'm happy to change it though.
implements parts of #9397