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

Prevent access to undefined AttemptLogs while looking at reports #12723

Merged

Conversation

LianaHarris360
Copy link
Member

@LianaHarris360 LianaHarris360 commented Oct 18, 2024

Summary

This pull request fixes the console errors that would appear when viewing learner exercise attempts. The errors occurred because AttemptLogItem was being rendered with an undefined attemptLog prop and caused runtime errors when it tried to access properties of attemptLog. This pull request also addresses an issue where, on mobile screens, the KSelect options included selections that did not have associated attemptLogs but could still be selected.

  • Conditionally render the AttemptLogItem component only when attemptLog is defined.
  • A computed property named attemptLogsForCurrentSection has been added for the attempt logs of the current section.
  • Added a computed property for the selected attempt log called selectedAttemptLog to be displayed as the current KSelect item on mobile displays.
  • Updated the questionSelectOptions computed property to filter out any questions that don't have an associated attemptLog.

Console Errors fixed:
attemptLogUndefined
correctUndefined
titleUndefined

Before:

Before.mov

After:

After.mov

attemptloglist

attemptloglistmobile

References

Fixes #12550

Reviewer guidance

  1. Create a lesson using the resource "Make 10 (grids and number bonds)" found within Kolibri QA Channel > Exercises > CK12 Exercises (token: nakav-mafak) and assign it to a learner.
  2. While signed into the learner account, attempt the lesson.
  3. Sign back into Kolibri with an admin/coach account and navigate to Coach > Reports > Lessons and review the learner lesson attempt.
  4. Confirm that there are no console errors.

Testing checklist

  • Contributor has fully tested the PR manually
  • If there are any front-end changes, before/after screenshots are included

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

  • 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

@LianaHarris360 LianaHarris360 changed the title Update how the attemptLog for the current section and mobile selectio… Prevent access to undefined AttemptLogs while looking at reports Oct 18, 2024
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.

A couple of questions - have not manually tested.

kolibri/core/assets/src/views/AttemptLogList.vue Outdated Show resolved Hide resolved
kolibri/core/assets/src/views/AttemptLogList.vue Outdated Show resolved Hide resolved
@LianaHarris360 LianaHarris360 linked an issue Oct 22, 2024 that may be closed by this pull request
@marcellamaki
Copy link
Member

@pcenov @radinamatic this is a PR for planned patch 2 - if we can get manual QA before I tag the release beta, that would be great.

@rtibbles rtibbles self-assigned this Oct 22, 2024
@@ -63,7 +63,7 @@
v-for="(section, index) in sections"
:id="`section-questions-${index}`"
:key="`section-questions-${index}`"
:title="displaySectionTitle(section, index) || ''"
:title="displaySectionTitle(section, index) || sectionLabel$({ sectionNumber: index + 1 })"
Copy link
Member

Choose a reason for hiding this comment

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

Hrm, is this happening when section.title is undefined rather than being an empty string? Perhaps we could just tweak the implementation here:

to do a looser check, that section is defined, section.title is defined, and section.title is not empty?

So could maybe update it to:

export function displaySectionTitle(section, index) {
  return section?.section_title
    ? sectionLabel$({ sectionNumber: index + 1 })
    : section.section_title;
}

Copy link
Member Author

@LianaHarris360 LianaHarris360 Oct 22, 2024

Choose a reason for hiding this comment

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

Yes, this was happening because displaySectionTitle() is checking for section.section_title, but it seems that Attemptlogs for practice exercises have section.title instead. I did try this way, but had some issues with it not working when checking the attempt logs for exams.

export function displaySectionTitle(section, index) {
  const title = section.section_title || section.title;
  return title === ' '
    ? sectionLabel$({ sectionNumber: index + 1 })
    : title;
}

I could instead try

const title = section.section_title ? section.section_title : section.title;

Edit: I get this console error with this way as well: Invalid prop: type check failed for prop "title". Expected String, got Undefined

Copy link
Member

Choose a reason for hiding this comment

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

Hrm... I suspect that the discrepancy between section_title and title is the root issue here. So, maybe we should trace back where that is coming from and just rectify that instead?

Copy link
Member

Choose a reason for hiding this comment

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

i.e. it should always be section_title for the title for a section, and not title.

Copy link
Member Author

@LianaHarris360 LianaHarris360 Oct 22, 2024

Choose a reason for hiding this comment

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

Hmm, I may have found where it's coming from. Possibly this annotateSections() util function.

Copy link
Member

Choose a reason for hiding this comment

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

That looks rather like a bug, and one that I caused, looking at the git blame, so I'd be most obliged if you could fix it!

Copy link
Member Author

Choose a reason for hiding this comment

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

It's fixed, now all sections annotated with annotateSections() have the field section_title
Before:
AnnotatedSectionBefore
After:
AnnotatedSectionAfter

Copy link
Member

Choose a reason for hiding this comment

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

Thank you!

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.

All code review based questions resolved - manual QA sign off should leave this good to merge!

@pcenov
Copy link
Member

pcenov commented Oct 24, 2024

Thanks @LianaHarris360 - I confirm that the console errors are no longer displayed and there are no regressions!

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.

QA approved - good to go!

@rtibbles
Copy link
Member

Merging with failing check that is fixed on release-v0.17.x

@rtibbles rtibbles merged commit 068f25b into learningequality:release-v0.17.x Oct 24, 2024
33 of 34 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Attempt to access undefined AttemptLog when looking at reports
4 participants