Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
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
2 changes: 2 additions & 0 deletions contentcuration/contentcuration/frontend/shared/app.js
Original file line number Diff line number Diff line change
Expand Up @@ -109,6 +109,7 @@ import { Workbox, messageSW } from 'workbox-window';
import KThemePlugin from 'kolibri-design-system/lib/KThemePlugin';
import trackInputModality from 'kolibri-design-system/lib/styles/trackInputModality';

import Teleport from 'vue2-teleport';
import AnalyticsPlugin from './analytics/plugin';
import { theme, icons } from 'shared/vuetify';

Expand Down Expand Up @@ -259,6 +260,7 @@ Vue.component('ActionLink', ActionLink);
Vue.component('BaseMenu', BaseMenu);
Vue.component('Divider', Divider);
Vue.component('Icon', Icon);
Vue.component('Teleport', Teleport);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

suggestion: this global registration is the only one — jest_config/setup.js registers only ActionLink, so in tests <Teleport> renders as an unknown element with its children in place, and Vue.config.silent = true suppresses the warning. That means the whole describe('Answer settings') block in ChoiceInteractionEditor.spec.js passes because the label rendered inline, not at the teleport target; target resolution, the settingsTargetId string surgery, and the ordering of the two teleported blocks are all untested.

It also makes these components silently depend on shared/app.js having run, which is a bad fit for code living in shared/views/. Importing Teleport from 'vue2-teleport' locally in InteractionSection and ChoiceInteractionEditor (and dropping the global registration) fixes both, and avoids squatting on a Vue 3 built-in name. Worth one integration assertion in QTIItemEditor.spec.js that the settings land inside #qti-question-settings-0.


function initiateServiceWorker() {
// Second conditional must be removed if you are doing dev work on the service
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@ import { render, screen } from '@testing-library/vue';
import { nextTick } from 'vue';
import VueRouter from 'vue-router';
import InteractionSection from '../index.vue';
import { qtiEditorStrings as tr } from '../../../qtiEditorStrings';

import {
CHOICE_SINGLE_SELECT_XML,
Expand All @@ -10,6 +11,13 @@ import {
} from '../../../utils/testingFixtures';

jest.mock('shared/views/TipTapEditor/TipTapEditor/TipTapEditor');
jest.mock('kolibri-design-system/lib/composables/useKResponsiveWindow', () => {
const { ref } = require('vue');
return {
__esModule: true,
default: () => ({ windowIsSmall: ref(false) }),
};
});

const renderSection = (props = {}) =>
render(InteractionSection, {
Expand Down Expand Up @@ -41,7 +49,7 @@ describe('InteractionSection', () => {
describe('parse error handling', () => {
it('shows a parse error when XML is malformed', () => {
renderSection({ interaction: interactionBlock('not-xml<{{') });
expect(screen.getByText('This question could not be loaded')).toBeInTheDocument();
expect(screen.getByText(tr.$tr('errorParsingQuestion'))).toBeInTheDocument();
});
});

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -7,16 +7,36 @@
>
{{ parseError }}
</p>
<component
:is="descriptor.editorComponent"
v-else
:key="descriptor.type"
:questionType="questionType"
:interaction="interaction"
:mode="mode"
:showAnswers="showAnswers"
@update:interaction="interaction => $emit('update:interaction', interaction)"
/>
<div v-else>
<Teleport
v-if="teleportTarget"
:to="teleportTarget"
>
<QuestionTypeSelector
v-if="mode === 'edit'"
:questionType="questionType"
:questionTypeOptions="typeOptions"
:settingsTargetId="settingsTargetId"
@update:questionType="

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

nitpick: a two-statement handler that writes to a setup-owned ref from a template expression. It works in Vue 2.7 (proxyWithRefUnwrap forwards the set), but it hides the mutation from the script block — and the watch(questionType, ..., { immediate: true }) at line ~55 already emits update:questionType, so each selection emits twice. A named onQuestionTypeChange(newType) in setup that only assigns the ref, letting the watcher emit, is clearer and emits once.

Related: currentQuestionType is stored in QTIItemEditor but never fed back as a prop, so this ref is the real source of truth. Fine functionally — worth a comment so the duplicated state isn't read as a wiring bug later.

newType => {
questionType = newType;
$emit('update:questionType', newType);
}
"
/>
</Teleport>

<component
:is="descriptor.editorComponent"
:key="descriptor.type"
:questionType="questionType"
:interaction="interaction"
:mode="mode"
:showAnswers="showAnswers"
:teleportTarget="`#${settingsTargetId}`"
@update:interaction="interaction => $emit('update:interaction', interaction)"
/>
</div>
</div>

</template>
Expand All @@ -26,13 +46,19 @@

import { computed, watch } from 'vue';
import useInteractionDescriptor from '../../composables/useInteractionDescriptor';
import QuestionTypeSelector from '../QuestionTypeSelector/index.vue';

export default {
name: 'InteractionSection',

components: {
QuestionTypeSelector,
},

setup(props, { emit }) {
const interactionRef = computed(() => props.interaction);
const { descriptor, questionType, parseError } = useInteractionDescriptor(interactionRef);
const { descriptor, questionType, typeOptions, parseError } =
useInteractionDescriptor(interactionRef);

watch(
questionType,
Expand All @@ -42,7 +68,14 @@
{ immediate: true },
);

return { descriptor, questionType, parseError };
const settingsTargetId = computed(() => {
if (props.teleportTarget && props.teleportTarget.startsWith('#')) {
return `${props.teleportTarget.substring(1)}-answer-settings`;
}
return 'qti-interaction-settings';
});

return { descriptor, questionType, typeOptions, parseError, settingsTargetId };
},

props: {
Expand All @@ -66,6 +99,10 @@
type: Boolean,
default: false,
},
teleportTarget: {
type: String,
default: '',
},
},

emits: ['update:questionType', 'update:interaction'],
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,13 @@ import { qtiEditorStrings } from '../../../qtiEditorStrings';
import { AssessmentItemTypes } from '../../../constants';

jest.mock('shared/views/TipTapEditor/TipTapEditor/TipTapEditor');
jest.mock('kolibri-design-system/lib/composables/useKResponsiveWindow', () => {
const { ref } = require('vue');
return {
__esModule: true,
default: () => ({ windowIsSmall: ref(false) }),
};
});

const { closeBtnLabel$, questionContentPlaceholder$ } = qtiEditorStrings;

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -26,12 +26,15 @@
</div>
</div>

<div :id="`qti-question-settings-${index}`"></div>

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

nitpick: unique across questions in one QTIEditor, but vue2-teleport resolves targets with document.querySelector, which takes the first document-wide match. Two QTIEditor instances on a page would teleport both selectors into the first card. Suffixing with _uid makes it collision-proof for one interpolation.


<div class="question-card-body">
<InteractionSection
v-if="interactions.length > 0"
:interaction="interactions[0]"
:mode="mode"
:showAnswers="showAnswers"
:teleportTarget="`#qti-question-settings-${index}`"
@update:questionType="type => (currentQuestionType = type)"
@update:interaction="onUpdateInteraction"
/>
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,86 @@
import { render, screen, fireEvent } from '@testing-library/vue';
import VueRouter from 'vue-router';
import QuestionTypeSelector from '../index.vue';
import { QuestionType } from '../../../constants';
import { qtiEditorStrings as tr } from '../../../qtiEditorStrings';

jest.mock('kolibri-design-system/lib/composables/useKResponsiveWindow', () => {
const { ref } = require('vue');
return {
__esModule: true,
default: () => ({ windowIsSmall: ref(false) }),
};
});

const defaultProps = {
questionType: QuestionType.SINGLE_SELECT,
questionTypeOptions: [
{
value: QuestionType.SINGLE_SELECT,
label: tr.$tr('singleSelectLabel'),
description: tr.$tr('singleChoiceDescription'),
},
{
value: QuestionType.MULTI_SELECT,
label: tr.$tr('multiSelectLabel'),
description: tr.$tr('multipleSelectionDescription'),
},
],
mode: 'edit',
};

const renderHeader = (props = {}) =>
render(QuestionTypeSelector, {
props: { ...defaultProps, ...props },
routes: new VueRouter(),
});

describe('QuestionTypeSelector', () => {
it('renders the type meta-label in edit mode', () => {
renderHeader();
expect(screen.getByText(tr.$tr('typeLabel'))).toBeInTheDocument();
});

it('renders a KSelect with the selected option label (not raw enum)', () => {
renderHeader();
expect(screen.getByText(tr.$tr('singleSelectLabel'))).toBeInTheDocument();
expect(screen.queryByText(QuestionType.SINGLE_SELECT)).not.toBeInTheDocument();
});

it('renders the globe icon inside the KSelect via #display slot', () => {
renderHeader();
expect(document.querySelector('.select-globe-icon')).not.toBeNull();
expect(document.querySelector('.select-display-row')).not.toBeNull();
});

it('opens type info modal when info button clicked', async () => {
renderHeader();

const helpButton = screen.getByRole('button', { name: tr.$tr('responseTypeInfoTitle') });
await fireEvent.click(helpButton);

expect(screen.getByRole('dialog')).toBeInTheDocument();
expect(screen.getByText(tr.$tr('singleChoiceDescription'))).toBeInTheDocument();
expect(screen.getByText(tr.$tr('multipleSelectionDescription'))).toBeInTheDocument();
});

it('closes type info modal when Close button clicked', async () => {
renderHeader();

await fireEvent.click(screen.getByRole('button', { name: tr.$tr('responseTypeInfoTitle') }));
expect(screen.getByRole('dialog')).toBeInTheDocument();

await fireEvent.click(screen.getByRole('button', { name: tr.$tr('closeBtnLabel') }));
expect(screen.queryByRole('dialog')).not.toBeInTheDocument();
});

it('disables selector when only one option available', () => {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

suggestion: neither assertion touches :disabled — the hidden input exists with two options too, and the "Type" label is static. This test passes with :disabled deleted entirely. Assert on the control: expect(screen.getByRole('combobox')).toBeDisabled().

More importantly, the component's one behaviour — @update:questionType (index.vue:23) — has no test, and neither do the two acceptance criteria with real logic behind them: "changing the type updates questionType and re-renders ChoiceInteractionEditor", and "cardinality in the response declaration XML updates to match the new type". The second is the one that has to survive any refactor of the buildXML plumbing; a useChoiceInteraction spec that flips questionTypeRef and parses responseDeclarations.value[0] covers it cheaply.

renderHeader({
questionTypeOptions: [defaultProps.questionTypeOptions[0]],
});

const hiddenInput = document.querySelector('input[type="hidden"]');
expect(hiddenInput).not.toBeNull();
expect(screen.getByText(tr.$tr('typeLabel'))).toBeInTheDocument();
});
});
Loading
Loading