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.

Sign up for GitHub

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
merged 4 commits into from
Oct 24, 2024
Merged

Prevent access to undefined AttemptLogs while looking at reports #12723

Changes from 2 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
1df1975
Update how the attemptLog for the current section and mobile selectio…
LianaHarris360 Oct 18, 2024
110070a
Display section number instead of empty string if section title is un…
LianaHarris360 Oct 21, 2024
d6af7cb
Show all KSelect question number options in AttemptLoglist
LianaHarris360 Oct 22, 2024
6b19bea
Add section_title field to annotateSections() func
LianaHarris360 Oct 23, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
41 changes: 32 additions & 9 deletions kolibri/core/assets/src/views/AttemptLogList.vue
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,15 +38,16 @@
<AttemptLogItem
class="attempt-selected"
:isSurvey="isSurvey"
:attemptLog="attemptLogs[selectedQuestionNumber]"
:attemptLog="selectedAttemptLog"
displayTag="span"
/>
</template>
<template #option="{ index }">
<AttemptLogItem
v-if="attemptLogsForCurrentSection[index]"
class="attempt-option"
:isSurvey="isSurvey"
:attemptLog="attemptLogs[sections[currentSectionIndex].startQuestionNumber + index]"
:attemptLog="attemptLogsForCurrentSection[index]"
displayTag="span"
/>
</template>
Expand All @@ -62,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:

kolibri/packages/kolibri-common/strings/enhancedQuizManagementStrings.js

Line 224 in 3d12dae

return section.section_title === ''
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 •
edited
Loading

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 •
edited
Loading

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!

@focus="expand(index)"
>
<template
Expand Down Expand Up @@ -131,8 +132,9 @@
"
>
<AttemptLogItem
v-if="attemptLogsForCurrentSection[qIndex]"
:isSurvey="isSurvey"
:attemptLog="attemptLogs[section.startQuestionNumber + qIndex]"
:attemptLog="attemptLogsForCurrentSection[qIndex]"
displayTag="p"
/>
</a>
Expand Down Expand Up @@ -169,7 +171,7 @@
AccordionItem,
},
setup(props, { emit }) {
const { questionsLabel$, quizSectionsLabel$ } = enhancedQuizManagementStrings;
const { questionsLabel$, quizSectionsLabel$, sectionLabel$ } = enhancedQuizManagementStrings;
const { questionNumberLabel$ } = coreStrings;
const { currentSectionIndex, sections, selectedQuestionNumber } = toRefs(props);

Expand All @@ -193,11 +195,24 @@
return sections.value[currentSectionIndex.value];
});

// Computed property for attempt logs of the current section
const attemptLogsForCurrentSection = computed(() => {
const start = currentSection.value.startQuestionNumber;
return currentSection.value.questions.map((_, index) => {
rtibbles marked this conversation as resolved.
Show resolved Hide resolved
return props.attemptLogs[start + index];
});
});

const questionSelectOptions = computed(() => {
return currentSection.value.questions.map((question, index) => ({
value: index,
label: questionNumberLabel$({ questionNumber: index + 1 }),
}));
return currentSection.value.questions.reduce((options, question, index) => {
if (attemptLogsForCurrentSection.value[index]) {
rtibbles marked this conversation as resolved.
Show resolved Hide resolved
options.push({
value: index,
label: questionNumberLabel$({ questionNumber: index + 1 }),
});
}
return options;
}, []);
});

// The KSelect-shaped object for the current section
Expand All @@ -212,6 +227,11 @@
];
});

// Computed property for the selected attempt log
const selectedAttemptLog = computed(() => {
return props.attemptLogs[selectedQuestionNumber.value];
});

function handleQuestionChange(index) {
emit('select', index + currentSection.value.startQuestionNumber);
expandCurrentSectionIfNeeded();
Expand All @@ -232,13 +252,16 @@
displaySectionTitle,
quizSectionsLabel$,
questionsLabel$,
sectionLabel$,
expand,
isExpanded,
toggle,
selectedSection,
sectionSelectOptions,
selectedQuestion,
questionSelectOptions,
attemptLogsForCurrentSection,
selectedAttemptLog,
};
},
props: {
Expand Down