Add question status data to user progress endpoint#760
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #760 +/- ##
==========================================
- Coverage 39.95% 39.94% -0.02%
==========================================
Files 546 546
Lines 23901 23907 +6
Branches 2873 2873
==========================================
Hits 9550 9550
- Misses 13444 13450 +6
Partials 907 907 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
| } | ||
|
|
||
| // Convert QuestionPageDTO to ContentSummaryDTO and calculate completion state | ||
| ContentSummaryDTO contentSummaryDTO = contentSummarizerService.extractContentSummary(questionPageDTO); |
There was a problem hiding this comment.
Do we have a good understanding of how expensive this method is? I think this will add a major overhead on this endpoint, since mapping (which extractContentSummary does under the hood) is an involved process requiring making a new object (which then needs garbage collecting in all but 5 cases here).
But MapStruct is less bad than Orika, so I would also not be surprised to find my memory of mapping being horrifyingly costly and slow is no longer true.
There was a problem hiding this comment.
I imagine you've got a better understanding of such things than I do, even now with MapStruct 🤷♀️
We could certainly avoid this either way by creating a parallel set of queues for each of question pages and question statuses, and in fact I nearly did in the first place except that this then didn't work with the current implementation of incompleteQuestionPages. It would either need another list of question statuses, or to limit its size within this loop rather than only doing it afterwards. The latter seems like a good idea in general - I'll see how that comes out.
There was a problem hiding this comment.
I've just tried moving the mapping out of the loop (now on this branch) and done some experimenting with my local data.
I see no major difference (no more than within regular variance) in the time taken to load the progress data a) without these changes, b) with these changes and the mapping inside the loop, and c) with these changes and the mapping outside the loop on my main test account with 1000s of question attempts. This isn't particularly thorough testing, but it suggests that MapStruct is efficient enough that it no longer makes much difference.
I think my original implementation is cleaner code, but if we still want to be careful about the mapping then the changes now present should be safer in that regard.
Converts
QuestionPageDTOtoContentSummaryDTOearlier in the method, and set question state according to the already calculated question part correctness loop.This is done so that the "My progress" page can display the status of "Most recently answered questions"/"Oldest unsolved questions" on their ALVIs.
Also requires isaacphysics/isaac-react-app#2006, as status can otherwise take up too much space on small ALVIs such as these.