✨ Add questionnaire semantics validation #931
Conversation
There were issue on misbehavior with custom questionnaires caused by invalid data (e.g. non-unique order for answers or sections). In order to avoid it, a new validation is added when custom questionnaire is created or updated, so questionnaire authos can quickly fix its content before it ges uploaded to Konveyor. Fixes: konveyor#927 Signed-off-by: Marek Aufart <[email protected]>
WalkthroughThis change introduces comprehensive validation for custom questionnaires uploaded to the system. A new Validate() method enforces uniqueness of section and question/answer orders, ensures non-empty required fields, validates risk levels and thresholds, and checks risk messages. The validation is applied in both Create and Update operations for non-builtin questionnaires. Changes
Sequence DiagramsequenceDiagram
participant Client
participant API
participant Handler
participant Validate
participant DB
alt Create/Update Questionnaire
Client->>API: POST/PUT questionnaire
API->>Handler: Route to Create/Update
Handler->>Handler: Bind request data
Handler->>Validate: r.Validate()
Validate->>Validate: Check unique orders
Validate->>Validate: Check non-empty fields
Validate->>Validate: Check risk levels
Validate->>Validate: Check thresholds
Validate->>Validate: Check risk messages
alt Validation Succeeds
Validate-->>Handler: nil
Handler->>DB: Save questionnaire
DB-->>Handler: Success
Handler-->>Client: 201/204 OK
else Validation Fails
Validate-->>Handler: error
Handler-->>Client: 400 Bad Request
end
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes The change introduces new validation logic with multiple distinct rule checks (order uniqueness, field emptiness, risk levels, thresholds), modifies existing Create/Update paths, and includes a comprehensive test suite with diverse test cases. While each validation check is straightforward, the breadth of validation rules and test coverage requires careful verification of completeness and correctness. Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
aufi
changed the title
aufi
changed the title
Signed-off-by: Marek Aufart <[email protected]>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
api/questionnaire.go (1)
242-314: Comprehensive validation logic with minor optimization opportunity.The validation thoroughly checks all required constraints including uniqueness of orders, non-empty fields, and proper nesting. The logic is sound and error messages are descriptive.
Consider this minor optimization for the risk validation map:
+// validRisks defines the allowed risk levels +var validRisks = map[string]bool{"red": true, "yellow": true, "green": true, "unknown": true} + // Validate performs additional validation on the questionnaire beyond binding tags. func (r *Questionnaire) Validate() error { // Validate sections have unique order values sectionOrders := make(map[uint]bool) for i, section := range r.Sections { ... // Validate risk level (already validated by binding tag, but double-check) - validRisks := map[string]bool{"red": true, "yellow": true, "green": true, "unknown": true} if !validRisks[answer.Risk] {This avoids recreating the map in every iteration of the innermost loop.
test/api/questionnaire/validation_test.go (1)
440-452: Use standard librarystrings.Containsinstead of custom helper.The custom
containshelper reimplements functionality already available in the standard library.Apply this refactor:
+import ( + "strings" "testing" "github.com/konveyor/tackle2-hub/api" "github.com/konveyor/tackle2-hub/model" +)Then replace the helper usage:
// Check if error contains expected string if tt.errorContains != "" { errStr := err.Error() - if !contains(errStr, tt.errorContains) { + if !strings.Contains(errStr, tt.errorContains) { t.Errorf("Expected error containing '%s', but got: %s", tt.errorContains, errStr) } }And remove the custom helpers:
-// Helper function to check if a string contains a substring -func contains(s, substr string) bool { - return len(s) >= len(substr) && (s == substr || len(substr) == 0 || - (len(s) > 0 && len(substr) > 0 && containsHelper(s, substr))) -} - -func containsHelper(s, substr string) bool { - for i := 0; i <= len(s)-len(substr); i++ { - if s[i:i+len(substr)] == substr { - return true - } - } - return false -}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
api/questionnaire.go(4 hunks)test/api/questionnaire/samples.go(1 hunks)test/api/questionnaire/validation_test.go(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
api/questionnaire.go (2)
migration/v14/model/assessment.go (3)
Questionnaire(3-13)Thresholds(98-102)RiskMessages(90-95)api/error.go (1)
BadRequestError(21-23)
test/api/questionnaire/samples.go (2)
api/assessment.go (2)
Thresholds(166-166)RiskMessages(167-167)migration/v14/model/assessment.go (2)
Thresholds(98-102)RiskMessages(90-95)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: test-api
🔇 Additional comments (6)
test/api/questionnaire/samples.go (1)
14-24: LGTM! Sample data properly updated for new validation requirements.The addition of Thresholds and RiskMessages to the sample questionnaire correctly aligns with the new validation logic. The values are reasonable and all required fields are populated.
api/questionnaire.go (3)
99-104: LGTM! Validation properly integrated into Create flow.The validation is correctly positioned after binding and before model creation, with proper error handling that follows existing patterns in the codebase.
179-184: LGTM! Validation correctly applied to non-builtin questionnaires.The conditional validation for non-builtin questionnaires is appropriate, since builtin questionnaires only allow changes to the
Requiredfield and don't need comprehensive validation.
316-329: LGTM! Threshold and risk message validations are well-designed.These validations ensure questionnaires have meaningful threshold configurations and complete risk messaging, preventing confusing or broken assessment results.
test/api/questionnaire/validation_test.go (2)
10-119: LGTM! Excellent test template and structure.The valid questionnaire template is comprehensive and realistic, providing a solid foundation for the mutation-based test cases. The test structure is clear and maintainable.
121-397: LGTM! Comprehensive test coverage for all validation rules.The test suite thoroughly covers all validation scenarios including:
- Uniqueness constraints (section/question/answer orders)
- Required fields (texts, messages)
- Business rules (thresholds, risk levels)
Each test case is clearly named and properly validates expected error conditions.
aufi
added
the
cherry-pick/release-0.8
There were issue on misbehavior with custom questionnaires caused by invalid/confusing data (e.g. non-unique order for answers or sections). In order to avoid it, a new additional validation is added when custom questionnaire is created or updated, so questionnaire authos can quickly fix its content before it ges uploaded to Konveyor. This is an additional validation for Questionnaire semantics as an addition to default `h.Bind()` validation. Fixes: #927 <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit ## Release Notes * **New Features** * Enhanced questionnaire validation that enforces comprehensive data integrity checks during creation and updates, including validation of section structure, question-answer relationships, risk configurations, and threshold values. * **Tests** * Added comprehensive unit test suite for questionnaire validation to ensure all validation rules function correctly and maintain data quality standards. <!-- end of auto-generated comment: release notes by coderabbit.ai --> --------- Signed-off-by: Marek Aufart <[email protected]> Signed-off-by: Cherry Picker <[email protected]>
2 participants
There were issue on misbehavior with custom questionnaires caused by invalid/confusing data (e.g. non-unique order for answers or sections). In order to avoid it, a new additional validation is added when custom questionnaire is created or updated, so questionnaire authos can quickly fix its content before it ges uploaded to Konveyor.
This is an additional validation for Questionnaire semantics as an addition to default
h.Bind()validation.Fixes: #927
Summary by CodeRabbit
Release Notes
New Features
Tests