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

Encapsulate progress tracking logic in a composable to prevent content pages crossing their streams #8898

Merged

Conversation

rtibbles
Copy link
Member

@rtibbles rtibbles commented Dec 11, 2021

Summary

  • Migrates core logging state and actions into a composable in learn
  • Migrates and adds to tests
  • Adds retry behaviour when encountering a 503 for initializing a content session, parallel to the 503 retry handling for updating a content session
  • Fixes previously undocumented issue unique to quizzes where multiple interactions with the same attempt could result in the attempt object not being properly updated
  • Gets rid of use of patching the ContentNodeProgressResource cache to maintain client side representation, as this is now always bypassed
  • Instead updates contentNodeProgressMap from the useContentNodeProgress composable
  • Uses this source of truth for displaying the current content progress in the top bar of the content page

References

Fixes #8776
Fixes #8848

Reviewer guidance

Take a coach assigned quiz
Engage with an exercise
Watch a video

Does progress all get saved and tracked properly?

This will necessitate small tweaks to the QuizRenderer in develop - but they will be quick to implement.


Testing checklist

  • Contributor has fully tested the PR manually
  • If there are any front-end changes, before/after screenshots are included
  • Critical user journeys are covered by Gherkin stories
  • Critical and brittle code paths are covered by unit tests

PR process

  • PR has the correct target branch and milestone
  • PR has 'needs review' or 'work-in-progress' label
  • If PR is ready for review, a reviewer has been added. (Don't use 'Assignees')
  • If this is an important user-facing change, PR or related issue has a 'changelog' label
  • If this includes an internal dependency change, a link to the diff is provided

Reviewer checklist

  • Automated test coverage is satisfactory
  • PR is fully functional
  • PR has been tested for accessibility regressions
  • External dependency files were updated if necessary (yarn and pip)
  • Documentation is updated
  • Contributor is in AUTHORS.md

@rtibbles rtibbles added the TODO: needs review Waiting for review label Dec 11, 2021
@rtibbles rtibbles added this to the 0.15.0 milestone Dec 11, 2021
@sairina
Copy link
Contributor

sairina commented Dec 13, 2021

When doing manual QA based on the Reviewer Guidance above, the tracking in the UI was fine, but in the console, there were some errors:

Take a coach assigned quiz

  1. Assigned a quiz to a class
  2. As learner, took quiz, but before finishing quiz, returned to ClassAssignmentsPage
  3. Returned to finish quiz, and saw the screenshot below. Icons had disappeared (not sure if known issue)

Screen Shot 2021-12-12 at 3 55 25 PM

  1. As learner, when returned back to ClassAssignmentsPage, received redundant navigation error and 403 errors (one in useProgressTracking):

Screen Shot 2021-12-12 at 3 55 44 PM

When testing by doing an exercise and then a video, also received 403 errors

Screen Shot 2021-12-12 at 3 56 12 PM

@pcenov
Copy link
Member

pcenov commented Dec 13, 2021

Tested with the latest Buildkite deb build:
Observations:

  1. Assigned lesson: when I reopen a resource which is already marked as 'in progress' I'm not starting from the point where I left off - start a video and go to 20s then go back and then click again the video resource - it starts from the beginning. No errors in the console in that case.
  2. Assigned Lesson: noticed 503 errors when navigating back and forth after completion. The error is displayed in the console, while what the user observes is some slowness while loading the completed resource. I compared with a previous build and I was getting an error screen with multiple errors in the console in that scenario.
  3. Assigned Quiz: Getting a 'TypeError: Cannot read properties of null (reading 'hintsRenderer')' after exiting and going back to the quiz but the progress is kept. Also getting File not found errors for some exercises as reported by Sairina.
    2021-12-13_15-03-41
    2021-12-13_15-42-36
    logs.zip

@rtibbles
Copy link
Member Author

Icons had disappeared (not sure if known issue)

I can't imagine how my progress tracking PR might have affected this, but I'll take a closer look as we want this to be fixed regardless.

redundant navigation error

This is pre-existing to this PR.

403 errors (one in useProgressTracking)

I think this may be new, because I think the quiz renderer was failing to call stop tracking after submission. I can fix this.

When testing by doing an exercise and then a video, also received 403 errors

Looking at the screenshot, this is also a consequence of the failure to call stop tracking - it is repeatedly trying to make updates to the progress of the closed quiz, even though it is closed! So, I will make sure we call stop tracking before tear down, and also add some guard logic to the composable to prevent us trying to continue tracking user progress when they are no longer on the page.

start a video and go to 20s then go back and then click again the video resource - it starts from the beginning.

Hrm, this sounds like a timing issue - where we're not ensuring that progress data is saved quickly enough. I think adding some guard logic to ensure that the progress tracking data is finalized would help here.

I compared with a previous build and I was getting an error screen with multiple errors in the console in that scenario.

Excellent - this is one of the things this PR should be addressing!

Getting a 'TypeError: Cannot read properties of null (reading 'hintsRenderer')' after exiting and going back to the quiz

I think this is a regression in the perseus viewer plugin after recent updates, can fix.

@jredrejo
Copy link
Member

@rtibbles is this PR fixing #8848 too?

@rtibbles
Copy link
Member Author

@jredrejo I think it may well do that, looking at the errors in there! Thanks, I knew there were some other issues out there like this.

Copy link
Member

@marcellamaki marcellamaki left a comment

Choose a reason for hiding this comment

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

It's a lot of changes, but the tests are very helpful both for catching things and for understanding what the intended behaviors are. I'm going to do some manual testing and another read through after the updates for the issues mentioned previously during QA.

@rtibbles
Copy link
Member Author

start a video and go to 20s then go back and then click again the video resource - it starts from the beginning.

Note: this was actually an issue with a typo - I was referring to extraFields in camelCase rather than extra_fields in snake_case. A similar issue was affecting time_spent.

@marcellamaki
Copy link
Member

marcellamaki commented Dec 13, 2021

Manual QA after the latest changes:

  • I don't see any 403 errors in the console when viewing PDFs ✔️
  • I don't see any 403 errors when watching video ✔️
  • When I exist and resume a video, it loads to the correct spot where I previous exited ✔️

One error that I still see that @pcenov maybe was referencing is after completing an exercise in a lesson, when clicking "move on" I get a 503 error. First, this seems to not do anything (i.e. the page does not "move on" but continues to show the completed resource after the modal has closed), then there is a long loading spinner, then the content loads, after ~10 seconds...? (trying to replicate for more details)

Error is POST http://127.0.0.1:8000/api/logger/trackprogress/ 503 (Service Unavailable)
The response is Database is not available for write operations

@rtibbles
Copy link
Member Author

I get a 503 error

This is one of those cases where a properly handled API error is part of expected behaviour. This happens when the request receives a database locked error - it then waits 10 seconds before retrying. The user experience of this could be better, but would probably require strings.

The alternative, as @pcenov noted is a big fat error page and not actually saving the data.

@marcellamaki
Copy link
Member

marcellamaki commented Dec 13, 2021

Got it - thanks for clarifying. Code read through makes sense with that, and I don't see any blockers with the new changes

@marcellamaki marcellamaki self-requested a review December 13, 2021 22:49
Copy link
Member

@marcellamaki marcellamaki left a comment

Choose a reason for hiding this comment

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

With updates, looks good to me

@marcellamaki marcellamaki merged commit f8b6c5c into learningequality:release-v0.15.x Dec 13, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
TODO: needs review Waiting for review
Projects
None yet
5 participants