-
Notifications
You must be signed in to change notification settings - Fork 638
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Quiz editing capability for not yet activated quizzes #12232
Conversation
Build Artifacts
|
kolibri/plugins/coach/assets/src/views/reports/ReportsLessonListPage.vue
Show resolved
Hide resolved
A few follow up items as discussed during the code review walk through (hopefully comprehensible reminders to Richard):
|
… CoachImmersivePage permissions handling.
Hopefully I've addressed all the issues we identified @nucleogenesis @marcellamaki - I did a rebase, so as to keep the diff clean and drop the unneeded commit. |
Ah yes, this is the result of my updating things based on yesterday's code review, and then only testing the editing flow, not the creation flow, afterwards! |
Remove cruft from serializers left over from when they weren't write only. Defer as many fields as possible to backend data handling.
…roperties to exams.
Remove unneeded queries.
…and quiz data. Remove use of 'this' and refer to 'getters' to avoid an implicit coupling with the commonCoach mixin.
… a form) To simplify API and allow usage as an input component.
Replace all quiz editing with the component.
Delete unused examsRoot vuex module.
Remove unused examReport vuex module.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
✅ Code review
I'll leave PR approval until QA has given it a pass. Once we're cleared of regressions we'll merge this.
Hi @rtibbles, here are the issues I able to identify:
Clicking.Save.closes.the.quiz.mp4
Cannot.replace.questions.mp4
started.quiz.mp4
select.a.quiz.mp4
Question.order.mp4 |
Hi @pcenov - thanks for this review, only 3 is directly related to the changes here, so I'll address that within the scope of this PR, but leave the others for follow up, as they are pre-existing to my work here. |
I did not do such an extensive testing as @pcenov , but I can replicate 2. (replacing a question does not work because clicking the checkbox of a question results in selecting all the questions), which got me into a 400 error: |
Limit patch updates to only things we need/are allowed to update.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Blocking changes have been addressed, good to merge once follow-ups are made! Thanks Richard
@rtibbles @radinamatic I've tested this locally and the code LGTM |
@radinamatic @nucleogenesis confirmed the fix of the final issue of activating a quiz from the summary page, we're going to merge here to keep development going, and we're confident of the fix. |
Summary
Backend tech debt/cleanup:
Frontend tech debt/cleanup:
update
event to allow it to be used more like a two way data binding componentBackend feature work:
Frontend feature work:
References
Fixes #12150
Handles some aspects of #11615 inasmuch as it deletes all the vuex state specific to this route, and switches to composables
Reviewer guidance
Create a quiz.
Edit the quiz before activating.
Activate the quiz.
Edit after activation.
Copy quiz.
Edit a copied quiz.
Testing checklist
PR process
Reviewer checklist
yarn
andpip
)