[GSoC'24] M2.6: Generalize diagnostic test player #20664
Conversation
|
Hi @AFZL210 please assign the required reviewer(s) for this PR. Thanks! |
|
@nikitaevg PTAL, Thanks!. |
core/controllers/classroom_test.py
Outdated
| 'public_classrooms_count': 1, | ||
| 'topic_id_to_prerequisite_topic_ids': { | ||
| self.public_topic_id_1: [], | ||
| self.private_topic_id: [] |
There was a problem hiding this comment.
Let's add test that a prerequisite is displayed here if present. Can be done within this test
There was a problem hiding this comment.
We don't need this now as we are not adding topic_id_to_prerequisite_topic_ids in this endpoint.
| .catch(() => { | ||
| this.router.navigate([ | ||
| `${AppConstants.PAGES_REGISTERED_WITH_FRONTEND.ERROR.ROUTE}/404`, | ||
| ]); |
There was a problem hiding this comment.
This doesn't look right, what if an error is not "not found", but some internal server error? Is there a page for 5xx status codes?
There was a problem hiding this comment.
Yes we do have a page for that.
Let's do this instead:
if there is server error redirect user to /500 internal server error page.
and if the classroom url fragment is not correct show an alert saying "Invalid classroom url fragment" and disable the 'Start Test' button.
core/templates/pages/diagnostic-test-player-page/diagnostic-test-player.component.ts
Show resolved
Hide resolved
|
Unassigning @nikitaevg since the review is done. |
|
@nikitaevg PTAL. Thanks! |
| }) | ||
| .catch(error => { | ||
| this.isStartTestButtonDisabled = true; | ||
| if (error.status === 500) { |
There was a problem hiding this comment.
This inconsistency is strange. For 500 we route to an error page, for others we show a warning. IIRC warnings are the default approach in Oppia to tell about the problem, so let's stick to it
There was a problem hiding this comment.
Done. let's just show warning and disable start button.
|
Unassigning @nikitaevg since they have already approved the PR. |
|
Hi @AFZL210, this PR is ready to be merged. Please address any remaining comments prior to merging, and feel free to merge this PR once the CI checks pass and you're happy with it. Thanks! |
Overview
Essential Checklist
Please follow the instructions for making a code change.
Proof that changes are correct
diagnostic-test.mp4
Proof of changes on desktop with slow/throttled network
N/A
Proof of changes on mobile phone
N/A
Proof of changes in Arabic language
N/A
PR Pointers