Skip to content
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

reload on connect in quizsummarypage; avoid possible error w/ missing… #12554

Open
wants to merge 3 commits into
base: develop
Choose a base branch
from

Conversation

nucleogenesis
Copy link
Member

@nucleogenesis nucleogenesis commented Aug 12, 2024

Summary

Closes #12388

I was only able to replicate the error described there by @AlexVelezLl when I forced specifically a 502 error within the size endpoint's handler. I don't know any way that a 502 would come through unless the server is disconnected somehow during the page load.

I could not replicate it naturally, however, so I cannot be 100% this addresses the issue at hand here.

But, ultimately, what it came down to was that if the page got a 502 error, it would show the proper "disconnected" snackbar & backdrop. In this case, the value used this.quizId was not yet initialized.

The change here ensures that when the user regains connection to the server, their page will reload, hopefully successfully.


One thing of concern is that if there is a way in which we could end up getting some kind of unjustified 502, then this could just result in an infinite loop of failing to connect and reloading.

The only way that I can think of that the size endpoint's handler would cause a 502 is if it's crashing the server. I've made some large quizzes during development on EQM and I've not had any issues with it and trying it didn't help replicate the issue.

Without some kind of stack trace for the error from a naturally occurring replication of the error this will be hard to debug, as there are several calls within that could cause the failure:

    @action(detail=False)
    def size(self, request, **kwargs):
        exams, draft_exams = self.filter_querysets(
            self.get_queryset(), self.get_draft_queryset()
        )
        exams_sizes_set = []
        for exam in list(exams) + list(draft_exams):
            quiz_size = {}

            quiz_nodes = ContentNode.objects.filter(
                id__in=[question["exercise_id"] for question in exam.get_questions()]
            )

            quiz_size[exam.id] = total_file_size(quiz_nodes)
            exams_sizes_set.append(quiz_size)

        return Response(exams_sizes_set)

Also worth noting that my first thought was to force an error within the function there, but that just resulted in the expected 400 Bad request rather than the 502 that is the direct cause of this issue.

Reviewer guidance

Code review

  • Is there any kind of additional handing I should add in the size handler?
  • Any thoughts on how to better replicate this particular issue -- are there any quirks to our disconnection handling that might come into play here?

QA

Path 1: Fake replication

  • With Network Throttling enabled, click to view a Quiz's details (click it's name link on the quiz list page). Throttling will help buy you time to kill your server in the next step :)
  • Then, quickly kill your server kolibri stop, causing the backend to no longer be available
  • Restart the Kolibri server, click to reconnect on the snackbar message in your browser, and the page should reload when you reconnect successfully
  • Now you should be able to delete the quiz without issue

Path 2: Possibly replicate it naturally

  • Maybe try once more with a very large quiz w/ lots of exercises in it that would have a large size
  • Don't do the "kill your server" thing manually and see if you can get it to naturally occur that you get a 502 somehow?

@github-actions github-actions bot added APP: Coach Re: Coach App (lessons, quizzes, groups, reports, etc.) DEV: frontend labels Aug 12, 2024
@pcenov
Copy link
Member

pcenov commented Aug 13, 2024

Hi @nucleogenesis, following the directions in 'Path 1: Fake replication' I managed to actually replicate the original issue that is - I can see the quiz details page with "Resource not found on device" shown and I can't delete the quiz after having successfully restarted the server. Only if I manually refresh the entire page, then I can delete it.
So if we consider that a valid testing scenario, then we've not fixed the original issue since from the perspective of a normal user of Kolibri we are still getting in the edge-case scenario where we can't delete the quiz with missing resources for no obvious reason. Here's a video of what I am observing:

2024-08-13_15-25-09.mp4

Logs: logs.zip

@nucleogenesis nucleogenesis force-pushed the fix--delete-quiz-missing-content branch from 9054f85 to 278cd17 Compare August 26, 2024 20:55
@nucleogenesis
Copy link
Member Author

@pcenov I've updated the PR so that the deletion modal uses the always present quizId value from the URL itself. This ensures that the modal being submitted will always work.

@rtibbles @AlexVelezLl -- I've updated the QuizSummaryPage a bit here. We were duplicating the fetch from the backend to get data we already had available by way of the classSummary store. So the component got the same data two different ways and used them differently.

My changes consolidate them into one exam property derived from classSummary/examMap, then uses the exam utils fetchExamWithContent function to be sure we're getting the content in the right shape. I think that we could probably just derive this from the classSummary/contentNodeMap but I figured I'd get feedback on this first as I think the changes here should sufficiently resolve the issue this PR targets.

@pcenov
Copy link
Member

pcenov commented Sep 4, 2024

Hi @nucleogenesis - I confirm that now it's possible to delete a quiz while the 'Resource not found on device' message is displayed:

delete.mp4

I can also see the missing resources loaded correctly when I manually refresh the page after the successful server restart:

reload.mp4

Copy link
Member

@rtibbles rtibbles left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't have any concrete reason to think this is a problem, but I suspect there could be a regression lurking in here somewhere.

Most likely if the quiz being loaded is an old quiz made on 0.16 or older.

@rtibbles
Copy link
Member

As a final manual test here before we merge, @pcenov could you:

  1. Install 0.16
  2. Create a quiz on 0.16.
  3. Upgrade to the asset in this PR.
  4. Navigate to the quiz summary page for a specific quiz under plan.
  5. Confirm that the questions all display correctly.

@pcenov
Copy link
Member

pcenov commented Sep 18, 2024

Hi @rtibbles - I confirm that there is a regression when opening a quiz created in 0.16:

errors.when.opening.a.quiz.created.in.0.16.mp4

I'm seeing the following error in the console:

TypeError: undefined is not iterable (cannot read property Symbol(Symbol.iterator))
    at ManageExamModals.vue:95:1
    at QuizSummaryPagevue_type_script_lang_js_toConsumableArray (ManageExamModals.vue:95:1)
    at index.vue:116:1
    at Array.reduce (<anonymous>)
    at Sub.selectedQuestions (index.vue:115:1)
    at Watcher.get (vue.runtime.esm.js:4495:25)
    at Watcher.evaluate (vue.runtime.esm.js:4597:21)
    at Sub.selectedQuestions (vue.runtime.esm.js:4851:17)
    at Sub.QuizSummaryPagevue_type_template_id_f77f4f98_scoped_true_render (index.vue:1:1036)
    at vue-composition-api.mjs:1940:35

Here's the entire home folder:
kolibri.zip

@rtibbles rtibbles added the DEV: Core JS API Changes related to, or to the Core JS API label Nov 5, 2024
@nucleogenesis nucleogenesis force-pushed the fix--delete-quiz-missing-content branch from 278cd17 to 5bebdd3 Compare December 5, 2024 23:28
@nucleogenesis nucleogenesis changed the base branch from release-v0.17.x to develop December 5, 2024 23:29
@github-actions github-actions bot added DEV: dev-ops Continuous integration & deployment DEV: renderers HTML5 apps, videos, exercises, etc. DEV: backend Python, databases, networking, filesystem... APP: Device Re: Device App (content import/export, facility-syncing, user permissions, etc.) APP: Facility Re: Facility App (user/class management, facility settings, csv import/export, etc.) APP: Learn Re: Learn App (content, quizzes, lessons, etc.) APP: User Re: User app (sign-in, sign-up, user profile, etc.) APP: Setup Wizard Re: Setup Wizard (facility import, superuser creation, settings, etc.) labels Dec 5, 2024
@github-actions github-actions bot added the DEV: tools Internal tooling for development label Dec 5, 2024
@nucleogenesis
Copy link
Member Author

@pcenov I've updated the PR and two things to note:

  • The most recently reported issue no longer exists in 0.18 and since this is no longer targeting 0.17 it's no longer fixable.
  • Because of some of those changes made to QuizSummaryPage, I've reapplied the fixes that I had for being able to delete a quiz when things don't load correctly on the QuizSummaryPage.

Could you test the scenarios again mentioned in the reviewer's guidance?

@pcenov
Copy link
Member

pcenov commented Dec 6, 2024

Hi @nucleogenesis - could you check why the build checks have not passed yet and there are no new build artefacts for this PR?

@nucleogenesis
Copy link
Member Author

@pcenov I tried re-running the jobs but they are just sitting there pending still. Will see if @rtibbles knows where things may have gone sideways next week.

@nucleogenesis nucleogenesis force-pushed the fix--delete-quiz-missing-content branch from 5bebdd3 to fce634d Compare December 11, 2024 00:55
@pcenov
Copy link
Member

pcenov commented Dec 13, 2024

Hi @nucleogenesis - I see that there are new build artifacts now - is it ready for testing?

@pcenov
Copy link
Member

pcenov commented Dec 16, 2024

Hi @nucleogenesis,

  1. It's still not possible to see the questions of a quiz created in 0.16 (when editing it), and not possible to copy that quiz, so the regression concern raised by @rtibbles is still valid for 0.18:
2024-12-16_18-31-57.mp4
  1. I confirm that the user is being able to delete a quiz when things don't load correctly on the QuizSummaryPage.

@nucleogenesis
Copy link
Member Author

@pcenov thank you!

I spoke w/ @rtibbles about the issue of not seeing the questions when editing a quiz from 0.16

In 0.17, Richard introduced two separate models - an Exam and a DraftExam.

Prior to this, we only had Exam.

Users are only able to edit a DraftExam - so when a new exam is created in >=0.17 the user is creating a DraftExam - they can edit this until they start the exam.

One reason for this is that the DraftExam cannot be synced so we can never run into an situation like:

  • Device A creates an Exam, does not start it
  • Device B syncs that Exam from Device A, then starts it
  • Device A now edits the Exam

So since the Exam was created as an Exam in 0.16, when the Kolibri is upgraded, it is still an Exam and not a DraftExam, thus it cannot be edited and the questions are not listed in the "Edit Quiz" page.

@@ -47,6 +47,9 @@ export default [
name: PageNames.EXAM_CREATION_ROOT,
path: CLASS + QUIZ + '/edit/:sectionIndex',
component: CreateExamPage,
meta: {
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This silences an error message

@@ -260,8 +251,7 @@
methods: {
// @public
setData(data) {
const { exam, difficultQuestions } = data;
this.quiz = exam;
const { difficultQuestions } = data;
Copy link
Member Author

@nucleogenesis nucleogenesis Dec 16, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

exam is removed here because we have a computed property exam which gets the data from examMap for us

@@ -351,7 +341,7 @@
});
},
detailLink(learnerId) {
return this.classRoute(PageNames.QUIZ_LEARNER_PAGE_ROOT, {
return this.classRoute(this.PageNames.QUIZ_LEARNER_PAGE_ROOT, {
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This was weird to find but it's because we don't import PageNames here, but rather, we get it from the commonCoach mixin

@nucleogenesis
Copy link
Member Author

nucleogenesis commented Dec 17, 2024

@pcenov @rtibbles I've fixed the bug w/ the copying of <0.17.x quizzes so should be ready for final batch of QA & code review - also confirmed that those copies can be edited.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
APP: Coach Re: Coach App (lessons, quizzes, groups, reports, etc.) APP: Device Re: Device App (content import/export, facility-syncing, user permissions, etc.) APP: Facility Re: Facility App (user/class management, facility settings, csv import/export, etc.) APP: Learn Re: Learn App (content, quizzes, lessons, etc.) APP: Setup Wizard Re: Setup Wizard (facility import, superuser creation, settings, etc.) APP: User Re: User app (sign-in, sign-up, user profile, etc.) DEV: backend Python, databases, networking, filesystem... DEV: Core JS API Changes related to, or to the Core JS API DEV: dev-ops Continuous integration & deployment DEV: frontend DEV: renderers HTML5 apps, videos, exercises, etc. DEV: tools Internal tooling for development
Projects
None yet
3 participants