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
Original file line number Diff line number Diff line change
Expand Up @@ -131,10 +131,7 @@

<!-- Location -->
<h2 class="section-header">{{ $tr('locationLabel') }}*</h2>
<CountryField
v-model="form.locations"
clearable
/>
<CountryField v-model="form.locations" />
<div
v-if="!valid && (!form.locations || !form.locations.length)"
:style="{ color: $themeTokens.error }"
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -31,8 +31,19 @@ const makeStore = ({ offline = false } = {}) =>
online: !offline,
},
},
actions: {
register: mockRegisterAction,
modules: {
account: {
namespaced: true,
actions: {
register: mockRegisterAction,
},
},
policies: {
namespaced: true,
getters: {
getPolicyAcceptedData: () => () => ({}),
},
},
},
});

Expand Down Expand Up @@ -102,20 +113,7 @@ describe('Create account page', () => {
});
});

// NOTE:
// Full form submission tests are intentionally skipped here.
//
// The "locations" field uses CountryField, which internally uses VAutocomplete
// (a Vuetify component). VAutocomplete does not reliably update its v-model
// state when interacted with via Vue Testing Library's userEvent APIs, which
// prevents a fully user-centric submission flow from being exercised.
//
// The "source" field has been migrated to KSelect (KDS) and no longer has
// this limitation, but "locations" remains the blocker.
//
// These tests will be re-enabled once CountryField is migrated away from
// VAutocomplete as part of the ongoing Vuetify removal effort.
it.skip('creates an account when the user submits valid information', async () => {
it('creates an account when the user submits valid information', async () => {
await renderComponent();

await userEvent.type(screen.getByLabelText(/first name/i), 'Test');
Expand All @@ -126,6 +124,12 @@ describe('Create account page', () => {

await userEvent.click(screen.getByLabelText(/tagging content sources/i));

await userEvent.type(screen.getByRole('combobox', { name: /select all that apply/i }), 'Kenya');
await userEvent.click(await screen.findByRole('option', { name: 'Kenya' }));
Comment thread
rtibblesbot marked this conversation as resolved.

await userEvent.click(screen.getByText(/select one/i));
await userEvent.click(await screen.findByText(/learning equality website/i));

await userEvent.click(screen.getByLabelText(/i have read and agree to terms of service/i));

const finishButton = screen.getByRole('button', { name: /finish/i });
Expand All @@ -137,12 +141,14 @@ describe('Create account page', () => {
await userEvent.click(finishButton);

await waitFor(() => {
expect(mockRegisterAction).toHaveBeenCalled();
expect(mockRegisterAction).toHaveBeenCalledWith(
expect.anything(),
expect.objectContaining({ locations: 'Kenya' }),
);
});
});

// Skipped for the same reason as above — CountryField (VAutocomplete) blocker
it.skip('shows an offline error when the user is offline', async () => {
it('shows an offline error when the user is offline', async () => {
await renderComponent({ offline: true });

await userEvent.type(screen.getByLabelText(/first name/i), 'Test');
Expand All @@ -153,11 +159,19 @@ describe('Create account page', () => {

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

nitpick: If the offline test keeps its form steps, lines 154–168 duplicate lines 119–133. A fillValidForm() helper would avoid the duplication. If those steps are being removed, as the comment on line 173 suggests, feel free to ignore this.

await userEvent.click(screen.getByLabelText(/tagging content sources/i));

await userEvent.type(screen.getByRole('combobox', { name: /select all that apply/i }), 'Kenya');
await userEvent.click(await screen.findByRole('option', { name: 'Kenya' }));

await userEvent.click(screen.getByText(/select one/i));
await userEvent.click(await screen.findByText(/learning equality website/i));

await userEvent.click(screen.getByLabelText(/i have read and agree to terms of service/i));

const finishButton = screen.getByRole('button', { name: /finish/i });
await userEvent.click(finishButton);

expect(await screen.findByText(/offline/i)).toBeInTheDocument();
expect(

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

suggestion: This test passes without triggering the submit flow. The offline banner renders on mount (Create.vue:35–41), and Finish is disabled while offline (Create.vue:226), so the form steps and click have no effect.

Assert the banner and disabled Finish button immediately after rendering, then remove the form steps. Also, scope the message assertion to the banner rather than matching by text alone.

await screen.findByText(/please connect to the internet to create an account/i),
).toBeInTheDocument();
});
});
Original file line number Diff line number Diff line change
Expand Up @@ -57,7 +57,6 @@
<CountryField
ref="locationDropdown"
v-model="locationFilter"
:outline="false"
:multiple="false"
label="Target location"
/>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -204,7 +204,6 @@
:disabled="!canBeEdited"
:label="countryLabel$()"
fullWidth
:hide-details="true"
/>
</KTransition>
</div>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -64,12 +64,12 @@
},
},
messages() {
const { languageItemsSelectedLabel$, languageSelectionsClearedLabel$ } = commonStrings;
const { languageItemsSelectedLabel$, selectionsClearedLabel$ } = commonStrings;
const { clearAllAction$ } = communityChannelsStrings;
return createMultiSelectMessages({
clearText: clearAllAction$,
itemsSelected: languageItemsSelectedLabel$,
cleared: languageSelectionsClearedLabel$,
cleared: selectionsClearedLabel$,
});
},
},
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -133,11 +133,7 @@
<div class="mb-1 mt-3">
<label>{{ $tr('targetRegionsLabel') }}</label>
</div>
<CountryField
v-model="location"
:box="false"
:menu-props="{ zIndex: 1, offsetY: true }"
/>
<CountryField v-model="location" />
<KTextbox
v-model="import_count"
:invalid="errors.import_count"
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -102,8 +102,12 @@ export const commonStrings = createTranslator('CommonStrings', {
message: '{count, plural, one {# language selected} other {# languages selected}}',
context: 'Announced with the number of languages currently selected in a language picker',
},
languageSelectionsClearedLabel: {
countryItemsSelectedLabel: {
message: '{count, plural, one {# country selected} other {# countries selected}}',
context: 'Announced with the number of countries currently selected in a country picker',
},
selectionsClearedLabel: {
message: '{count, plural, one {Cleared # selection} other {Cleared # selections}}',
context: 'Announced when all selected languages are cleared from a language picker',
context: 'Announced when all selected options are cleared from a multiple selection dropdown',
},
});
Original file line number Diff line number Diff line change
Expand Up @@ -103,15 +103,13 @@
clearAction$,
optionRemovedLabel$,
languageItemsSelectedLabel$,
languageSelectionsClearedLabel$,
selectionsClearedLabel$,
} = commonStrings;
return createMultiSelectMessages({
clearText: clearAction$,
itemsSelected: languageItemsSelectedLabel$,
cleared: ({ label, count }) =>
this.multiple
? languageSelectionsClearedLabel$({ count })
: optionRemovedLabel$({ label }),
this.multiple ? selectionsClearedLabel$({ count }) : optionRemovedLabel$({ label }),
});
},
},
Expand Down
Original file line number Diff line number Diff line change
@@ -1,57 +1,44 @@
<template>

<DropdownWrapper>
<template #default="{ attach, menuProps }">
<VAutocomplete
v-model="locations"
:items="options"
:label="label || $tr('locationLabel')"
:multiple="multiple"
:box="box"
item-value="id"
item-text="name"
:required="required"
:rules="rules"
:search-input.sync="searchInput"
:no-data-text="$tr('noCountriesFound')"
:chips="multiple"
:attach="attach"
:menuProps="menuProps"
clearable
v-bind="$attrs"
/>
</template>
</DropdownWrapper>
<KMultiSelect
v-model="locations"
:options="options"
:label="label || $tr('locationLabel')"
:multiple="multiple"
itemValue="id"
itemText="name"
:disabled="disabled"
:invalid="invalid"
:invalidText="$tr('locationRequiredMessage')"
:noResultsText="$tr('noCountriesFound')"
:appearanceOverrides="{ maxWidth: fullWidth ? '100%' : '500px' }"
:messages="messages"
clearable
@blur="touched = true"

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

suggestion: Is blur a supported KMultiSelect event? The KDS documentation lists no events. In the source, blur is emitted only by the text input, one frame after focus leaves it: KMultiSelect.vue#L433-L441 . No event is emitted when focus leaves via the Clear button, yet the required-field validation depends on blur.

/>

</template>


<script>

import KMultiSelect from 'kolibri-design-system/lib/candidate/multiselect/KMultiSelect';
import countries from '../../utils/countries';

import DropdownWrapper from 'shared/views/form/DropdownWrapper';
import { commonStrings } from 'shared/strings/commonStrings';
import { createMultiSelectMessages } from 'shared/utils/multiSelectMessages';

export default {
name: 'CountryField',
components: { DropdownWrapper },
// $attrs are rebound to a descendent component
inheritAttrs: false,
components: { KMultiSelect },
props: {
value: {
type: [String, Array],
default() {
return [];
},
default: null,
},
required: {
type: Boolean,
default: false,
},
box: {
type: Boolean,
default: true,
},
multiple: {
type: Boolean,
default: true,
Expand All @@ -65,26 +52,27 @@
type: Boolean,
default: false,
},
disabled: {
type: Boolean,
default: false,
},
},
data() {
return {
searchInput: '',
touched: false,
};
},
computed: {
locations: {
get() {
return this.value;
// KMultiSelect expects an array in multiple mode and a string or null in single mode
if (this.multiple) {
return this.value || [];
}
return this.value || null;
},
set(value) {
this.$emit('input', value);

// If selecting multiple countries, a chip is created for the selected item,
// so the input can be cleared. If selecting a single country, the search input
// becomes the selected country and should not be cleared to stay visible.
if (this.multiple) {
setTimeout(this.searchInputClear, 1);
}
},
},
options() {
Expand All @@ -97,16 +85,24 @@
};
});
},
rules() {
return [v => (!this.required || v.length ? true : this.$tr('locationRequiredMessage'))];
invalid() {
const hasSelection = this.multiple ? this.locations.length > 0 : Boolean(this.locations);
return this.required && this.touched && !hasSelection;
},
autocompleteMaxWidth() {
return this.fullWidth ? '100%' : '500px';
},
},
methods: {
searchInputClear() {
this.searchInput = '';
messages() {
const {
clearAction$,
optionRemovedLabel$,
countryItemsSelectedLabel$,
selectionsClearedLabel$,
} = commonStrings;
return createMultiSelectMessages({
clearText: clearAction$,
itemsSelected: countryItemsSelectedLabel$,
// KMultiSelect passes a label in single mode and a count in multiple mode
cleared: ({ label, count }) =>

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

suggestion: No test covers the single-mode branch (optionRemovedLabel$({ label })). If both modes use selectionsClearedLabel$, all 13 tests still pass. languageDropdown.spec.js tests the same branch by mocking sendPoliteMessage. That is safer than reading the shared live region, which keeps text between tests.

it('announces the removed option label, not a count, when clearing in single mode', async () => {
mockSendPoliteMessage.mockClear();
renderComponent({ value: 'en' });
await userEvent.click(screen.getByRole('button', { name: clearAction$() }));
expect(mockSendPoliteMessage).toHaveBeenCalledWith(optionRemovedLabel$({ label: ENGLISH }));
});

this.multiple ? selectionsClearedLabel$({ count }) : optionRemovedLabel$({ label }),
});
},
},
$trs: {
Expand All @@ -117,17 +113,3 @@
};

</script>


<style lang="scss" scoped>

::v-deep .v-select__selections {
width: calc(100% - 48px); // Account for clear icon
min-height: 0 !important;
}

.v-autocomplete {
max-width: v-bind('autocompleteMaxWidth');
}

</style>
Loading
Loading