Skip to content

Range surveys: require integers and improve validation #1102

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

Open
mkbehr wants to merge 5 commits into
base: main
Choose a base branch
from
Open

Range surveys: require integers and improve validation #1102

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
50fb7fa
feat: survey validation enhancements, UI error surfacing, and 422 HTT…
mkbehr May 1, 2026
8a94f91
feat: survey validation enhancements, UI error surfacing, and 422 HTT…
mkbehr May 1, 2026
81d9f5a
feat: survey validation enhancements, UI error surfacing, and 422 HTT…
mkbehr May 1, 2026
5b68480
feat: survey validation enhancements, UI error surfacing, and 422 HTT…
mkbehr May 1, 2026
cf7e55a
feat: survey validation enhancements, UI error surfacing, and 422 HTT…
mkbehr May 1, 2026
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
6 changes: 3 additions & 3 deletions docs/assets/api/schemas.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2293,13 +2293,13 @@
"type": "string"
},
"upperValue": {
"type": "number"
"type": "integer"
},
"upperText": {
"type": "string"
},
"lowerValue": {
"type": "number"
"type": "integer"
},
"lowerText": {
"type": "string"
Expand All @@ -2312,7 +2312,7 @@
},
"stepSize": {
"minimum": 1,
"type": "number"
"type": "integer"
},
"condition": {
"anyOf": [
Expand Down
9 changes: 8 additions & 1 deletion frontend/src/components/header/header.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -280,7 +280,13 @@ export class Header extends MobxLitElement {
if (this.experimentManager.isEditingFull) {
if (!this.experimentManager.isCreator) return nothing;

const editorErrors =
this.experimentEditor.getExperimentConfigErrors();

return html`
${editorErrors.length === 0
? nothing
: html` <div class="error">⚠️ ${editorErrors.join(', ')}</div> `}
<pr-button
color="tertiary"
variant="default"
Expand All @@ -291,7 +297,8 @@ export class Header extends MobxLitElement {
<pr-button
color="tertiary"
variant="tonal"
?disabled=${!this.experimentManager.isCreator}
?disabled=${!this.experimentManager.isCreator ||
editorErrors.length > 0}
@click=${() => {
this.analyticsService.trackButtonClick(
ButtonClick.EXPERIMENT_SAVE_EXISTING,
Expand Down
9 changes: 4 additions & 5 deletions frontend/src/components/stages/survey_editor.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -463,19 +463,18 @@ export class SurveyEditor extends MobxLitElement {
};

const updateLowerValue = (e: InputEvent) => {
const lowerValue =
parseInt((e.target as HTMLInputElement).value, 10) || 0;
const lowerValue = parseFloat((e.target as HTMLInputElement).value) || 0;
this.updateQuestion({...question, lowerValue}, index);
};

const updateUpperValue = (e: InputEvent) => {
const upperValue =
parseInt((e.target as HTMLInputElement).value, 10) || 10;
const upperValue = parseFloat((e.target as HTMLInputElement).value) || 10;
this.updateQuestion({...question, upperValue}, index);
};

const updateStepSize = (e: InputEvent) => {
const stepSize = parseInt((e.target as HTMLInputElement).value, 10) || 1;
const val = parseFloat((e.target as HTMLInputElement).value);
const stepSize = isNaN(val) ? 1 : val;
this.updateQuestion({...question, stepSize}, index);
};

Expand Down
71 changes: 63 additions & 8 deletions frontend/src/services/experiment.editor.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,8 @@ import {
requiresValues,
STAGE_MANAGER,
checkApiKeyExists,
SurveyQuestion,
MultipleChoiceItem,
createAgentMediatorPersonaConfig,
createAgentParticipantPersonaConfig,
createExperimentConfig,
Expand Down Expand Up @@ -118,17 +120,70 @@ export class ExperimentEditor extends Service {
renderApiErrorMessage(ApiKeyType.OLLAMA_CUSTOM_URL);

for (const stage of this.stages) {
switch (stage.kind) {
case StageKind.SURVEY:
// Ensure all questions have a non-empty title
stage.questions.forEach((question, index) => {
if (question.questionTitle === '') {
if (
stage.kind === StageKind.SURVEY ||
stage.kind === StageKind.SURVEY_PER_PARTICIPANT
) {
if (!stage.questions || stage.questions.length === 0) {
errors.push(`${stage.name} must contain at least one question`);
}
stage.questions.forEach((question: SurveyQuestion, index: number) => {
if (question.questionTitle === '') {
errors.push(
`${stage.name} question ${index + 1} is missing a title`,
);
}
if (question.kind === 'scale') {
if (
!Number.isInteger(question.lowerValue) ||
!Number.isInteger(question.upperValue) ||
(question.stepSize !== undefined &&
!Number.isInteger(question.stepSize))
) {
errors.push(
`${stage.name} question ${index + 1} ("${question.questionTitle}"): values must be integers`,
);
}
if (question.lowerValue >= question.upperValue) {
Copy link
Copy Markdown
Collaborator

@jimbojw jimbojw May 11, 2026

Choose a reason for hiding this comment

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

My main nit is that here and elsewhere, you assert a condition that would make the comparison potentially unspecified.

For example, earlier, you check !Number.isInteger(question.lowerValue) and if so, you append to the errors array. This is good. But in that case, when we get to this line, there are several possibilities, only some of which make sense. E.g., it could be that lowerValue is, say a float like 3.5, or even a string like "456". Those are legal values for this JavaScript >= comparison against upperValue, but they would have been flagged already as being illegal (non-integer).

Anyway, if the code works and the tests pass, it's good enough for me. Just flagging that it's strange to me to see later comparisons using potentially bad values that were checked earlier.

errors.push(
`${stage.name} question ${index + 1} is missing a title`,
`${stage.name} question ${index + 1} ("${question.questionTitle}"): lower value must be less than upper value`,
);
}
const range = question.upperValue - question.lowerValue;
const step = question.stepSize ?? 1;
if (step <= 0) {
errors.push(
`${stage.name} question ${index + 1} ("${question.questionTitle}"): step size must be greater than 0`,
);
}
if (range % step !== 0) {
Copy link
Copy Markdown
Collaborator

@jimbojw jimbojw May 11, 2026

Choose a reason for hiding this comment

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

Same here. Above we check that step is positive. But here, we're outside that check. So we could be comparing a negative step number's modulus against the range. It's strange to me to allow these potentially weird comparisons, given that earlier validation checks aim to ensure correct types and values.

errors.push(
`${stage.name} question ${index + 1} ("${question.questionTitle}"): step size must divide max-min (${range}) exactly`,
);
}
}
if (question.kind === 'mc') {
Copy link
Copy Markdown
Collaborator

@jimbojw jimbojw May 11, 2026

Choose a reason for hiding this comment

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

Nit: Is there a static variable for the string 'mc' we could use instead of the literal, such as an Enum value?

if (!question.options || question.options.length === 0) {
errors.push(
`${stage.name} question ${index + 1} ("${question.questionTitle}"): must have at least one option`,
);
}
if (
question.correctAnswerId != null &&
question.correctAnswerId !== ''
) {
const hasOption = question.options.some(
(opt: MultipleChoiceItem) =>
opt.id === question.correctAnswerId,
);
if (!hasOption) {
errors.push(
`${stage.name} question ${index + 1} ("${question.questionTitle}"): correct answer ID doesn't match any option ID`,
);
}
}
});
break;
}
});
}
}

Expand Down
190 changes: 187 additions & 3 deletions functions/src/dl_api/experiments.dl_api.integration.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,8 @@ import {
Visibility,
createExperimentConfig,
createPrivateChatStage,
createSurveyStage,
SurveyQuestionKind,
} from '@deliberation-lab/utils';
import {
TestContext,
Expand Down Expand Up @@ -926,7 +928,7 @@ describe('API Experiment Creation Integration Tests', () => {
stages: JSON.parse(JSON.stringify([stage1, stage2])),
});

expect(response.status).toBe(400);
expect(response.status).toBe(422);
const data = await response.json();
expect(data.error).toContain('Duplicate stage ID');
});
Expand All @@ -942,7 +944,7 @@ describe('API Experiment Creation Integration Tests', () => {
template: JSON.parse(JSON.stringify(template)),
});

expect(response.status).toBe(400);
expect(response.status).toBe(422);
const data = await response.json();
expect(data.error).toContain('Duplicate cohort alias found');
});
Expand Down Expand Up @@ -972,13 +974,195 @@ describe('API Experiment Creation Integration Tests', () => {
template: JSON.parse(JSON.stringify(template)),
});

expect(response.status).toBe(400);
expect(response.status).toBe(422);
const data = await response.json();
expect(data.error).toContain(
'values array must contain at least one item',
);
});

it('should reject experiment creation with invalid survey stage (scale lower >= upper bounds)', async () => {
const stage = createSurveyStage({
id: 'survey-1',
questions: [
{
id: 'q1',
kind: SurveyQuestionKind.SCALE,
questionTitle: 'Bad Scale',
lowerValue: 10,
lowerText: '',
upperValue: 5,
upperText: '',
stepSize: 1,
useSlider: false,
},
],
});

const response = await apiRequest('POST', '/v1/experiments', {
name: 'Bad Survey bounds Experiment',
stages: JSON.parse(JSON.stringify([stage])),
});

expect(response.status).toBe(422);
const data = await response.json();
expect(data.error).toContain('greater than or equal to upper value');
});

it('should reject experiment creation with invalid multiple choice survey question (no options)', async () => {
const stage = createSurveyStage({
id: 'survey-2',
questions: [
{
id: 'q2',
kind: SurveyQuestionKind.MULTIPLE_CHOICE,
questionTitle: 'Empty Options',
options: [],
correctAnswerId: null,
},
],
});

const response = await apiRequest('POST', '/v1/experiments', {
name: 'Bad MC Experiment',
stages: JSON.parse(JSON.stringify([stage])),
});

expect(response.status).toBe(422);
const data = await response.json();
expect(data.error).toContain('must have at least one option');
});

it('should reject experiment creation with survey stage containing no questions', async () => {
const stage = createSurveyStage({
id: 'survey-3',
questions: [],
});

const response = await apiRequest('POST', '/v1/experiments', {
name: 'Empty Survey Questions Experiment',
stages: JSON.parse(JSON.stringify([stage])),
});

expect(response.status).toBe(422);
const data = await response.json();
expect(data.error).toContain('must contain at least one question');
});

it('should reject experiment creation with non-integer scale question values', async () => {
const stage = createSurveyStage({
id: 'survey-4',
questions: [
{
id: 'q4',
kind: SurveyQuestionKind.SCALE,
questionTitle: 'Non-Integer Scale',
lowerValue: 1.5,
lowerText: '',
upperValue: 5.5,
upperText: '',
middleText: '',
stepSize: 1,
useSlider: false,
},
],
});

const response = await apiRequest('POST', '/v1/experiments', {
name: 'Non-Integer Experiment',
stages: JSON.parse(JSON.stringify([stage])),
});

expect(response.status).toBe(400);
const data = await response.json();
expect(data.error).toContain('Expected union value');
});

it('should reject experiment creation with scale question step size equal to 0', async () => {
const stage = createSurveyStage({
id: 'survey-7',
questions: [
{
id: 'q7',
kind: SurveyQuestionKind.SCALE,
questionTitle: 'Step Size 0',
lowerValue: 1,
lowerText: '',
upperValue: 5,
upperText: '',
middleText: '',
stepSize: 0,
useSlider: false,
},
],
});

const response = await apiRequest('POST', '/v1/experiments', {
name: 'Step Size 0 Experiment',
stages: JSON.parse(JSON.stringify([stage])),
});

expect(response.status).toBe(400);
const data = await response.json();
expect(data.error).toContain('Expected union value');
});

it('should reject experiment creation with scale question step size out of bounds', async () => {
const stage = createSurveyStage({
id: 'survey-5',
questions: [
{
id: 'q5',
kind: SurveyQuestionKind.SCALE,
questionTitle: 'Bad Step Size',
lowerValue: 1,
lowerText: '',
upperValue: 5,
upperText: '',
stepSize: 6,
useSlider: false,
},
],
});

const response = await apiRequest('POST', '/v1/experiments', {
name: 'Bad Step Size Experiment',
stages: JSON.parse(JSON.stringify([stage])),
});

expect(response.status).toBe(422);
const data = await response.json();
expect(data.error).toContain('must divide max-min');
});

it('should reject experiment creation with scale question step size not dividing range exactly', async () => {
const stage = createSurveyStage({
id: 'survey-6',
questions: [
{
id: 'q6',
kind: SurveyQuestionKind.SCALE,
questionTitle: 'Undivisible Step Size',
lowerValue: 1,
lowerText: '',
upperValue: 5,
upperText: '',
stepSize: 3,
useSlider: false,
},
],
});

const response = await apiRequest('POST', '/v1/experiments', {
name: 'Undivisible Range Experiment',
stages: JSON.parse(JSON.stringify([stage])),
});

expect(response.status).toBe(422);
const data = await response.json();
expect(data.error).toContain('must divide max-min');
});

it('should accept experiment creation with valid variable configuration', async () => {
const template = getFlipCardExperimentTemplate();
template.experiment.variableConfigs = [
Expand Down
Loading
Loading