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

Exam report: Avoid loading on question change #12082

Conversation

AlexVelezLl
Copy link
Member

Summary

Avoid fetching exam resources again if they are already loaded.

References

Closes #12062

Screenshots

Compartir.pantalla.-.2024-04-12.11_29_17.mp4

Reviewer guidance

  1. Take an exam as a learner.
  2. Review different exam reports and check that everything is loading correctly.
  3. Switch between questions, attempts and question interactions and the loading state should not appear

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

@github-actions github-actions bot added APP: Learn Re: Learn App (content, quizzes, lessons, etc.) DEV: frontend labels Apr 12, 2024
@AlexVelezLl AlexVelezLl added the TODO: needs review Waiting for review label Apr 12, 2024
@marcellamaki marcellamaki self-assigned this Apr 22, 2024
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.

The code read through makes sense to me, @AlexVelezLl - thanks for working on this. I also did a quick manual QA which checks out but I'll defer to @radinamatic and @pcenov for the final QA team approval.

My main thing that I'm still wondering is where exactly this regression came from, as this file hasn't been changed in years. My guess is that it was related to some of the other pretty significant loading state refactor work that went in to 0.16, and it's definitely possible that we just didn't notice it in the 0.16.0 QA. I'm just a bit mystified. This isn't blocking (or a criticism!) but I am wondering - did you discover anything about where the regression came from that led you to this solution in particular, or was it just a matter of looking at some of the handler patterns and deciding this would be the simplest path forward? Something else entirely? I realize you have less Past Knowledge of this part of the code base and what it looked like a while ago, so this is to satisfy my curiosity, mainly.

@pcenov pcenov self-requested a review April 23, 2024 13:46
@pcenov
Copy link
Member

pcenov commented Apr 23, 2024

Thanks @AlexVelezLl I confirm that the loading state of the quiz report when viewed as a learner is fixed now. However, while regression testing I noticed that this is not fixed at Coach > Reports so it will be great if you could fix it there as well:

2024-04-23_16-27-50.mp4

@AlexVelezLl
Copy link
Member Author

Hi @marcellamaki. This regression comes from here. The reason why the scroll is reset is because we have this conditional rendering, this way the ExamReport is unmounted from the DOM and mounted again.

Previously, although the data was reloaded and the references were updated, the ExamReport was always visible. And since we loaded the same data it still looked the same.

We could keep the same behavior of reloading the data, and make the ExamReport always visible, but perhaps with some opacity while the circular loader is shown, and revert the change to look if it is loaded in vue, If that is safer.

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.

Thank you @AlexVelezLl! :)

@pcenov, I've opened a follow up issue for the coach reports loading state.

@marcellamaki marcellamaki merged commit 54dd2c7 into learningequality:release-v0.16.x May 2, 2024
34 checks passed
@AlexVelezLl AlexVelezLl deleted the avoid-loadnig-exam-report branch May 6, 2024 15:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
APP: Learn Re: Learn App (content, quizzes, lessons, etc.) DEV: frontend SIZE: small TODO: needs review Waiting for review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants