Moving multiple instances of assessments into their own subpage #2185
Conversation
…into instancesCondensed
…arn into instancesCondensed
…arn into instancesCondensed
…arn into instancesCondensed
There was a problem hiding this comment.
Looks okay, I will defer review of the SQL stuff to @mwest1066 since I'm not sure what he's wanting in terms of code style here.
astrosticks
added
enhancement
|
This looks great and will be very helpful to students. |
astrosticks
added
under development
astrosticks
removed
the
under development
astrosticks
force-pushed
the
instancesCondensed
branch
from
b943b01
to
c8655aa
Compare
| JOIN assessment_sets AS aset ON (aset.id = a.assessment_set_id) | ||
| LEFT JOIN LATERAL authz_assessment(a.id, $authz_data, $req_date, ci.display_timezone) AS aa ON TRUE | ||
| WHERE | ||
| ci.id = $course_instance_id |
There was a problem hiding this comment.
This gets all assessments, but we know that we only want the one with a.id = $assessment_id
| AND a.multiple_instance | ||
| AND a.deleted_at IS NULL | ||
| ), | ||
|
|
There was a problem hiding this comment.
code style: no blank lines within queries
| ai.assessment_id = $assessment_id | ||
| AND ai.user_id = $user_id | ||
| ) | ||
|
|
| WHEN assessment_instance_id IS NULL THEN '/assessment/' || assessment_id || '/' | ||
| ELSE '/assessment_instance/' || assessment_instance_id || '/' | ||
| END AS link, | ||
| (lag(assessment_set_id) OVER (PARTITION BY assessment_set_id ORDER BY assessment_order_by, assessment_id, assessment_instance_number NULLS FIRST) IS NULL) AS start_new_set |
There was a problem hiding this comment.
This shouldn't be needed? It looks to me like you copied this code from somewhere else and didn't entirely clean it up? Perhaps it's worth going back over this query and thinking about whether any parts of it could be simpler.
| WHERE | ||
| authorized | ||
| ORDER BY | ||
| assessment_set_number, assessment_order_by, assessment_id, assessment_instance_number NULLS FIRST; |
There was a problem hiding this comment.
Some of these seem unnecessary (e.g., won't all the rows have the same assessment_set_number?
| if (res.locals.assessment.type !== 'Exam') return next(); | ||
|
|
||
| var params = { | ||
| course_instance_id: res.locals.course_instance.id, |
| SELECT | ||
| *, | ||
| CASE | ||
| WHEN assessment_instance_id IS NULL THEN '/assessment/' || assessment_id || '/' | ||
| WHEN (multiple_instance = true OR assessment_instance_count = 0 ) THEN '/assessment/' || assessment_id || '/' | ||
| ELSE '/assessment_instance/' || assessment_instance_id || '/' | ||
| END AS link, | ||
| (lag(assessment_set_id) OVER (PARTITION BY assessment_set_id ORDER BY assessment_order_by, assessment_id, assessment_instance_number NULLS FIRST) IS NULL) AS start_new_set |
| @@ -1,118 +1,56 @@ | |||
| -- BLOCK select_assessments | |||
| WITH | |||
| multiple_instance_assessments AS ( | |||
| assessments_ AS ( | |||
There was a problem hiding this comment.
Do you still need a WITH here? Can you collapse everything down into a single SELECT?
|
Can you add a skeleton news item to this PR? With some hires screenshots of before and after? I'll write the text of the news item, but having some images and an outline of the main points would be very helpful. |
mwest1066
removed
the
high impact
nwalters512
dismissed
nicknytko’s stale review
This PR is outdated; dismissing review to clean up the "approved PRs" view
|
Discussed with the dev team: while we still like the concept, this has drifted sufficiently far from |
|
CLA Assistant Lite bot: Thanks for your contribution! Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution. You can sign the CLA by posting a comment with the below format: I have reviewed and hereby sign the CLA 1 out of 2 committers have signed the CLA. |
Fixes #2086 by moving links to instances of assessments that allow multiple instances into a student view that was previously only used to generate a new instance. @nikitaa4 and I worked on this at HackIllinois 2020. Screenshots showing the changes are below:
pages/studentAssessments
*
Currently shows the maximum of all of the score percentages, but Dave's wording in #2068 seems to imply that this might not always be the final score that the student receives.pages/studentAssessmentExam
*
May want to switch order so that the most recent attempt is at the top of the list? This would keep the most relevant information near the top of the page.Still to-do (based on conversation from Jun 18):