Test pages types overhaul#2209
Conversation
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
jsharkey13
force-pushed
the
feature/test-pages-overhaul
branch
2 times, most recently
from
b20310d to
c504e1d
Compare
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #2209 +/- ##
==========================================
- Coverage 43.54% 43.54% -0.01%
==========================================
Files 597 597
Lines 25217 25274 +57
Branches 8378 8424 +46
==========================================
+ Hits 10981 11005 +24
- Misses 14187 14212 +25
- Partials 49 57 +8 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
|
I agree the refactor described above is an improvement, thank you for doing it. Besides the refactor, I also see the following user-facing changes:
There are a lot of subtle changes and it's difficult to see all implications, so there might be a few changes I've missed. At any rate, the core functionality around quizzes appears to work well enough. I've verified manually that tests can be assigned and completed, that results are correctly recorded, that students can see feedback (when this is set), and that teachers can see test results on the markbook. I've left a few comments with suggestions for simplifications, and flagged one logic and one rendering issue. |
|
A bug that needs fixing is that sidebar navigation is broken on http://localhost:8004/test/attempt/biology_summer_challenge_1. On the sidebar, clicking |
|
With the changes, the 
|
|
I'm not sure whether this change is intentional, but when viewing feedback for a test, Ada used to always hide the " |
| const deviceSize = useDeviceSize(); | ||
| const sections = attempt.quiz?.children; | ||
| const section = sections && sections[page - 1]; | ||
| const section = !!(page && sections) && sections[page - 1]; |
There was a problem hiding this comment.
I think this can just be page && sections && sections[page - 1]. The inferred type for section becomes messier, but the check on line 206 only leaves ContenBaseDto
There was a problem hiding this comment.
It could even be page && sections?.[page - 1]; if we want extra shortening!
I most likely did this as you say to tidy up the inferred type, but idrm.
| export const teacherQuizzesCrumbs = [{title: siteSpecific("Set / manage tests", "Tests"), to: `/set_tests`}]; | ||
| export const rubricCrumbs = [{title: "Practice tests", to: "/practice_tests"}]; | ||
| const getCrumbs = (preview: boolean | undefined, view: boolean | undefined, user: RegisteredUserDTO) => { | ||
| export const viewQuizzesCrumbs = [{title: "View tests", to: "/view_tests"}]; |
There was a problem hiding this comment.
I'd comment out this line too, as this is just an unused variable until we implement the TODO. A pity that eslint doesn't detect unused exports!
There was a problem hiding this comment.
If we comment it out, we'll need to uncomment it when we do the merge which uses this (#2228). I'm not intending one to exist in main without the other, so yes while I shouldn't have added this on this branch, now it's here let's just leave it.
|
|
||
| const questions = attempt ? props.questions : []; | ||
| const sections = attempt ? props.sections : {}; | ||
| const questions = isFullQuizProps(props) ? (props as QuizProps & FullQuizInfo).quizContents.questions : []; |
There was a problem hiding this comment.
we don't need these casts, that's the whole point of isFullQuizProps!
There was a problem hiding this comment.
Good spot. When I first wrote isFullQuizProps it was a function over the quiz, so this looked like
isFullQuiz(quiz) ? (props as QuizProps & FullQuizInfo).quizContents.questions : [];which does need the cast.
| const navigate = useNavigate(); | ||
| const location = useLocation(); | ||
|
|
||
| const isFullQuiz = (quiz: QuizSidebarProps['quiz']): quiz is IsaacQuizDTO => isDefined((quiz as IsaacQuizDTO).canonicalSourceFile); |
There was a problem hiding this comment.
this could be defined outside of the component. the performance penalty of defining it within the component is minimal (I certainly wouldn't want to optimise this, eg. usign useMemo), but I wonder if you agree, in general, that defining this outside the component would improve readability? for me, instantly knowing the function doesn't have any component state in its closure would be help by itself.
There was a problem hiding this comment.
Since it's a function definition, I don't think redeclaring it every render is a bad thing; after all, everything gets redeclared, but hooks etc are made to be idempotent under multiple runs with the same params such that they point to the same object. rubricPath, hasSections, etc. are all also variables that would be recalculated each render.
It's more of a code style thing – but yes, I do agree that declaring helper functions outside is a useful thing so happy to make the change.
It was hidden on Ada if you had completed the test: Since Ada haven't been using tests until very recently, there is no way anyone on their side requested this behaviour specifically for Ada. I've made the decision to simplify things by aligning the two sites, preferring the Science defaults. Do you think this is a bad idea? |
No I think it's fine to show the button and I'm glad we're simplifying stuff, I just wasn't sure why it was hidden from Ada in the first place (and whether bringing it back was intentional). Happy with the change, as long as it was made knowingly! |
|
I've made a couple of changes. To address your first point about it being more difficult for teachers to attempt a test themselves – I agree, but I don't want it to be as visible (i.e. on the ALVIs) – so you can now try the quiz yourself from the /preview/ link. I've also fixed the bug you mentioned surrounding the sidebar links – thanks for spotting. And lastly, many of the styling issues I had already fixed in the "future" branch
I don't think this distinction matters to them; as long as it isn't filling up their My Tests list, which was the reason we wanted the Thanks for the detailed review! |
3 participants
Overhauls the test pages' types to drastically increase code readability and robustness.
Quizzes across these pages can either be simplified rubric summary objects (if you have not yet committed to starting the test), or full quiz objects (if viewing a past attempt, previewing, or coming from an assignment). Previously, we represented the difference with two entirely separate objects:
This is fairly nasty for multiple reasons:
QuizViewProps.viewcontains aDetailedQuizSummaryDTO(containing the rubric), whereasQuizAttemptProps.attemptcontains anIsaacQuizDTOwith this same information.DetailedQuizSummaryDTOis a strict subset ofIsaacQuizDTO, which implies a significantly neater strategy of sharing an object between the two.A majority of the changes in this PR migrate the above types into this new format:
We can use the type
QuizProps & FullQuizInfowhenever we require parameters to be those of a complete quiz, and similarly forQuizProps & QuizSummaryInfoif only the summary is required. We can also useQuizPropswithout a second modifier in the case we can use either; in particular, note thatQuizProps.quizis of typeIsaacQuizDTO | DetailedQuizSummaryDTO, for whichquiz.rubricis a valid property in both cases, and so never requires further type checking or complication.