Range surveys: require integers and improve validation#1102
Conversation
…P error statuses - Add Scale range divisibility and integer constraints validation for Survey questions - Restrict survey stages to contain at least one question - Surface active config errors Lit Header UI editor edit mode and create mode - Support entering floats in frontend UI scale number form fields to trigger float validations - Return HTTP 422 Unprocessable Entity for REST cloud function experiment rejections - Add Jest unit and Express integrations tests covering positive/negative validation scenarios
…P error statuses - Add Scale range divisibility and integer constraints validation for Survey questions - Restrict survey stages to contain at least one question - Surface active config errors Lit Header UI editor edit mode and create mode - Support entering floats in frontend UI scale number form fields to trigger float validations - Return HTTP 422 Unprocessable Entity for REST cloud function experiment rejections - Add Jest unit and Express integrations tests covering positive/negative validation scenarios
…P error statuses - Add Scale range divisibility and integer constraints validation for Survey questions - Restrict survey stages to contain at least one question - Surface active config errors Lit Header UI editor edit mode and create mode - Support entering floats in frontend UI scale number form fields to trigger float validations - Return HTTP 422 Unprocessable Entity for REST cloud function experiment rejections - Add Jest unit and Express integrations tests covering positive/negative validation scenarios
mkbehr
changed the title
…P error statuses - Add Scale range divisibility and integer constraints validation for Survey questions - Restrict survey stages to contain at least one question - Surface active config errors Lit Header UI editor edit mode and create mode - Support entering floats in frontend UI scale number form fields to trigger float validations - Return HTTP 422 Unprocessable Entity for REST cloud function experiment rejections - Add Jest unit and Express integrations tests covering positive/negative validation scenarios
…P error statuses - Add Scale range divisibility and integer constraints validation for Survey questions - Restrict survey stages to contain at least one question - Surface active config errors Lit Header UI editor edit mode and create mode - Support entering floats in frontend UI scale number form fields to trigger float validations - Return HTTP 422 Unprocessable Entity for REST cloud function experiment rejections - Add Jest unit and Express integrations tests covering positive/negative validation scenarios
| msg.includes('Invalid stage configuration') || | ||
| msg.includes('Invalid variable configuration') || | ||
| msg.includes('Duplicate cohort alias found'); | ||
| throw createHttpError(isValidationError ? 422 : 500, msg); |
There was a problem hiding this comment.
Thanks for upgrading from 400 to more specific HTTP error codes. 🙏
| `${stage.name} question ${index + 1} ("${question.questionTitle}"): values must be integers`, | ||
| ); | ||
| } | ||
| if (question.lowerValue >= question.upperValue) { |
There was a problem hiding this comment.
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.
| `${stage.name} question ${index + 1} ("${question.questionTitle}"): step size must be greater than 0`, | ||
| ); | ||
| } | ||
| if (range % step !== 0) { |
There was a problem hiding this comment.
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.
| ); | ||
| } | ||
| } | ||
| if (question.kind === 'mc') { |
There was a problem hiding this comment.
Nit: Is there a static variable for the string 'mc' we could use instead of the literal, such as an Enum value?
2 participants
Description
Potentially breaking change: require numbers in range-type surveys to be integers
Check range-type survey stages for various requirements:
Require survey stages to contain at least one question
Validate experiments in edit mode as well as creation mode
Change response code for bad experiment config from 400 to 422 (more specific)
Documentation updated as needed
Verification
Related issues
This PR fixes: #1098