Selection output type #152
Conversation
Pull Request Review: Selection Output TypeI've completed a comprehensive review of PR #152. Overall, this is a well-implemented feature with good attention to validation and user experience. Here's my detailed feedback: ✅ Strengths
🐛 Issues FoundCritical: Data Loss Risk in JavaScriptLocation: if (!this.supportsSelection) {
this.options = [];
} else if (!wasSelection && this.options.length === 0) {
this.options = this.parseOptions(hiddenInput?.value);
}Problem: When a user switches from "Selection" to another type and back to "Selection", all their entered options are permanently lost because the array is cleared but not restored from the hidden input. Impact: Users would need to re-enter all selection options if they accidentally change the output type. Recommendation: Store the options in a temporary variable before clearing: if (!this.supportsSelection) {
this.lastSelectionOptions = [...this.options]; // Store before clearing
this.options = [];
} else if (!wasSelection && this.options.length === 0) {
this.options = this.lastSelectionOptions?.length
? this.lastSelectionOptions
: this.parseOptions(hiddenInput?.value);
}Medium: Edge Case in Model ValidationLocation: Issue: The model's
Recommendation: Consider warning the user before data loss or storing the options even when not active (just don't use them). This allows users to switch back without losing data. Low: Inconsistent Validation MessagesForm validation says: Recommendation: Standardize error messages across form and model validation for consistency. 🔒 Security Considerations
⚡ Performance Considerations
🧪 Test Coverage GapsCurrent tests are good, but consider adding:
📋 Code Quality Observations
Location: 🎯 Recommendations
📝 Documentation SuggestionsConsider adding inline comments explaining:
✨ SummaryThis is a well-structured feature addition with good validation and test coverage. The main concern is the potential data loss in the JavaScript component when users change output types. Once that's addressed, this should be ready to merge. Overall Assessment: ✅ Approve with minor changes recommended Great work on this implementation! The multi-layer validation approach and attention to UX details are particularly commendable. |
WalkthroughAdds a Selection output type and array support: new model fields and migrations, form fields and validation, Alpine/JS UI and CSS for editing selections, processor changes to emit Literal (and List[Literal]) types and prompts, template display updates, and tests covering form/model/processor behavior. Changes
Sequence DiagramsequenceDiagram
participant User as User (browser)
participant UI as SelectionOptions (client JS / Alpine)
participant Form as OutputFieldForm (server)
participant Model as OutputField (DB)
participant Processor as processor_utils
User->>UI: Add/edit option, toggle array
UI->>UI: enforce max, manage list, update UI states
UI->>UI: sync hidden `selection_options` and `is_array`
User->>Form: Submit form (includes hidden fields)
Form->>Form: parse JSON, validate options & is_array, require when type=SELECTION
Form->>Model: save -> Model.clean() normalizes & validates
Model->>Model: persist `selection_options` and `is_array`
Note over Processor,Model: Schema generation reads persisted fields
Processor->>Processor: If SELECTION -> build Literal[*options]
alt is_array == true
Processor->>Processor: Wrap in list[Literal[...]]
end
Processor->>User: return schema/prompt
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (14)
🚧 Files skipped from review as they are similar to previous changes (6)
🧰 Additional context used🧬 Code graph analysis (5)radis/extractions/tests/unit/test_processor_utils.py (3)
radis/extractions/utils/processor_utils.py (1)
radis/extractions/forms.py (1)
radis/extractions/migrations/0004_outputfield_selection_options.py (1)
radis/extractions/migrations/0005_outputfield_is_array.py (1)
🪛 Ruff (0.14.7)radis/extractions/utils/processor_utils.py27-27: Avoid specifying long messages outside the exception class (TRY003) 32-32: Avoid specifying long messages outside the exception class (TRY003) radis/extractions/forms.py205-211: Mutable class attributes should be annotated with (RUF012) 283-283: Avoid specifying long messages outside the exception class (TRY003) 286-286: Avoid specifying long messages outside the exception class (TRY003) 291-291: Avoid specifying long messages outside the exception class (TRY003) 294-294: Avoid specifying long messages outside the exception class (TRY003) 298-300: Avoid specifying long messages outside the exception class (TRY003) 302-302: Avoid specifying long messages outside the exception class (TRY003) radis/extractions/migrations/0004_outputfield_selection_options.py5-7: Mutable class attributes should be annotated with (RUF012) 9-33: Mutable class attributes should be annotated with (RUF012) radis/extractions/migrations/0005_outputfield_is_array.py5-7: Mutable class attributes should be annotated with (RUF012) 9-16: Mutable class attributes should be annotated with (RUF012) 🔇 Additional comments (9)
Comment |
Pull Request Review: Selection Output TypeSummaryThis PR adds a new "Selection" output type to the extraction system, allowing users to define enumerated values (up to 7 options) that the LLM can choose from. The implementation includes frontend (Alpine.js), backend (Django forms and models), validation, and tests. ✅ Strengths
🔍 Code Quality IssuesCritical1. Potential Type Safety Issue in processor_utils.py:22-25 options = tuple(field.selection_options)
if not options:
raise ValueError("Selection output requires at least one option.")
output_type = Literal.__getitem__(options)Issue: Literal types require literal values known at module load time. Using runtime values with Literal.getitem() may cause type checking issues or runtime errors with certain values (especially those containing special characters). Recommendation: Consider using Enum instead for runtime-generated types: from enum import Enum
SelectionEnum = Enum('SelectionEnum', {opt: opt for opt in field.selection_options})
output_type = SelectionEnumHigh Priority2. Inconsistent Whitespace Changes in formset.html:6-10 3. Missing Duplicate Option Validation Recommendation: Add validation in OutputFieldForm.clean_selection_options(): if len(cleaned) \!= len(set(cleaned)):
raise forms.ValidationError("Selection options must be unique.")4. No Maximum Length Validation for Individual Options Recommendation: Add length validation (e.g., max 100 characters per option). Medium Priority5. LocaleMiddleware Addition Appears Unrelated Recommendation: This change should be in a separate PR or needs explanation in the description of why it's required for this feature. 6. Console.log Statement in Production Code 7. JavaScript: No Validation for Empty String Options Low Priority8. Magic Number 7
Recommendation: Extract to a constant. 9. Type Annotation Inconsistency 10. Missing Accessibility Attributes 🔒 Security ConcernsLow Risk: No significant security vulnerabilities identified. The JSON parsing is properly error-handled, and all user inputs are validated and sanitized. ⚡ Performance ConsiderationsMinor: The implementation is efficient for the expected scale (max 7 options). The x-effect="syncOptions()" in Alpine.js will run on every reactive change, which is fine for this small dataset. 📝 Suggestions
🎯 Recommendations for MergeBefore merging, please address:
Nice to have:
Overall AssessmentThis is a solid feature implementation with good test coverage and user experience. The code follows Django and Google Python Style Guide conventions. With the critical issues addressed (particularly the Literal type issue), this PR will be ready to merge. Estimated effort to address critical issues: 1-2 hours Great work on the comprehensive implementation! 🎉 Review generated by Claude Code |
Pull Request Review: Selection Output TypeSummaryThis PR adds a new "Selection" output type to the extraction system, allowing users to define enumerated values (up to 7 options) that the LLM can choose from. It also introduces an "array" toggle feature for all output types. The implementation includes comprehensive form handling, validation, and testing. ✅ StrengthsCode Quality
Testing
UI/UX
|
Code Review: Selection Output Type FeatureThank you for this comprehensive PR! The addition of a Selection output type is a valuable feature. Here's my detailed review: ✅ StrengthsWell-Structured Implementation
Code Quality
🔍 Issues & RecommendationsHIGH PRIORITY1. Missing Model
|
PR Review: Selection Output TypeThank you for this comprehensive feature addition! This PR adds a valuable "Selection" output type that behaves like an enum, allowing users to define up to 7 predefined options for LLM-based extraction. Overall, the implementation is solid and well-tested. Below are my findings: ✅ Strengths
🐛 Potential Bugs & Issues
⚡ Performance Considerations
🔒 Security Concerns
No significant security vulnerabilities identified. 🧪 Test CoverageExcellent test coverage! The new tests include:
Suggestions for additional tests:
📝 Code Quality & Best Practices
🔧 Minor Issues & Suggestions
🎯 Recommendations SummaryMust fix:
Should fix:
Nice to have:
✨ ConclusionThis is a well-implemented feature with good test coverage and thoughtful UX considerations. The code follows Django best practices and the Google Python Style Guide. The main concerns are the unrelated middleware change and the missing duplicate validation. Once those are addressed, this will be ready to merge! Great work overall! 🚀 |
Pull Request Review: Selection Output Type FeatureSummaryThis PR adds a new "Selection" output type that allows users to define enumerated values (up to 7 options) for extraction output fields. The implementation includes database migrations, form validation, Alpine.js components, and comprehensive test coverage. Overall, this is a well-structured and thoughtfully implemented feature. ✅ StrengthsCode Quality
Test Coverage
User Experience
|
|
🔍 Edge Cases & Additional ConsiderationsEdge Cases to Consider:
📊 Performance AssessmentCurrent Performance: ✅ Good
Potential Improvements:
🛡️ Security AssessmentOverall: ✅ Good with minor concernsStrengths:
Concerns:
✅ Test Coverage AssessmentCoverage: Excellent 🌟 The PR includes 10 new tests that cover:
Suggested Additional Tests:
|
🎯 Overall Assessment
Overall: ⭐⭐⭐⭐⭐ (4.7/5) ✅ Approval RecommendationAPPROVE with minor changes This is a high-quality PR that demonstrates:
The issues identified are minor and can be addressed in follow-up PRs or quick fixes. None are blocking. Suggested Action Items (Priority Order):
Great work on this feature! The implementation is solid, the tests are comprehensive, and the code quality is excellent. 🎉 Reviewed by: Claude (AI Code Review Assistant) |
Pull Request Review: Selection Output TypeOverviewThis PR adds a "Selection" output type (enum-like) to the extraction system, along with an array toggle feature. The implementation includes frontend Alpine.js components, Django forms/models, migrations, and comprehensive test coverage. ✅ StrengthsCode Quality
Implementation Details
🔍 Issues & Concerns1. Security: XSS Vulnerability Risk
|
Pull Request Review: Selection Output TypeOverviewThis PR adds a "Selection" output type to the extraction system, allowing users to define enumerated values (up to 7 options) for LLM responses. The implementation includes frontend Alpine.js components, Django form/model updates, and comprehensive test coverage. ✅ StrengthsCode Quality
Best Practices
🔍 Issues & Suggestions1. Critical: Indentation-Only Change in formset.htmlLocation: Issue: This appears to be an unintentional formatting change. The indentation reduction doesn't improve the code and creates unnecessary diff noise. Recommendation: Revert these whitespace-only changes unless there's a specific reason for them. 2. Code Quality: Leftover Debug StatementLocation: console.log(inputs);Issue: Debug Recommendation: Remove this line. 3. JavaScript: Potential Memory LeakLocation: Issue: Event listeners are added in Recommendation: Consider using Alpine's 4. Performance: x-effect UsageLocation: x-effect="syncState()"Issue: The Recommendation: Consider using 5. Type Safety: Variable Name ReuseLocation: output_type = list[output_type]Issue: Type checkers may flag this as reassigning a type. While it works at runtime, it's technically reusing a variable name for a different type. Recommendation: Use a different variable name for clarity. 6. UX: Max Options Limit Not DocumentedLocation: Multiple files (hardcoded to 7) Issue: The 7-option maximum is hardcoded without explanation in comments. Is this a UX decision or an LLM limitation? Recommendation: Add a comment explaining the rationale. 7. Testing: Missing Integration TestsIssue: No tests verify the full form submission flow with JavaScript enabled. Recommendation: Add integration tests to ensure the Alpine.js components work correctly end-to-end. 🎯 Performance Considerations
🔒 Security Assessment
📋 Test CoverageExcellent test coverage! The PR includes:
🎨 Style & Conventions
📝 Recommendations SummaryMust Fix (Before Merge)
Should Fix
Nice to Have
✨ Overall AssessmentStrong PR with well-thought-out implementation! The code demonstrates good engineering practices with comprehensive validation, test coverage, and user experience considerations. The issues identified are mostly minor refinements rather than fundamental problems. With the debug statement removed and the indentation clarified, this is ready for merge. Recommendation: ✅ Approve with minor changes |
Code Review: Selection Output Type FeatureThank you for this comprehensive implementation! This is a well-structured PR that adds selection/enum type support to extraction output fields. Here's my detailed review: ✅ Strengths
🔍 Code Quality Issues1. Magic Number Extraction ✅ (Fixed in latest commit)The constant 2. JavaScript: Missing JSDoc for inner functionsIn // Add JSDoc comments like:
/**
* Parses a string value to boolean for array toggle state.
* @param {string} value - The value to parse
* @returns {boolean}
*/
const parseArrayValue = (value) => { ... }3. Potential XSS in Template (Low Risk)In 4. Inconsistent Indentation in formset.htmlLines 6-10 in 🐛 Potential Bugs1. Race Condition in Alpine ComponentIn 2. Form Submission on Enter KeyThe
3. Missing NULL Check in processor_utils.pyLine 25: ⚡ Performance Considerations1. Redundant TrimmingSelection options are trimmed in three places:
Recommendation: Trim only in the form's clean method and JavaScript. The model's clean should trust the form validation for performance. 2. JSON Serialization on Every ChangeThe syncState: Alpine.debounce(function() {
this.syncOptions();
this.syncArray();
this.updateArrayButton();
}, 300)🔒 Security Concerns1. JSON Injection ✅ (Properly Handled)The code correctly uses 2. SQL Injection ✅ (Not Applicable)Using Django ORM properly - no raw SQL. 3. Input Validation ✅ (Robust)Good validation at multiple layers (form, model, JavaScript). 📚 Best Practices1. Consider Adding Migration Data ValidationFor existing installations, consider adding a data migration to validate any manually created 2. Add Inline Comments for Complex LogicIn
3. Type Hinting in processor_utils.pyLine 8: 4. Consider Field Name ValidationThe model doesn't validate that def clean(self):
super().clean()
if not self.name.isidentifier():
raise ValidationError({'name': 'Must be a valid Python identifier'})
# ... rest of validation🧪 Test Coverage Suggestions
📊 SummaryOverall Assessment: ⭐⭐⭐⭐ (4/5) This is a solid, production-ready implementation with only minor issues. The code follows Django best practices and includes good test coverage. Required Changes
Recommended Changes (Before Merge)
Nice-to-Have (Future PRs)
Great work on this feature! 🚀 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (4)
radis/core/static/core/core.js (1)
110-247: Ensure array toggle click also syncs hidden field and button state
SelectionOptionslooks solid overall (parsing JSON, enforcingmaxOptions, handling SELECTION type), but thearrayToggleButtonclick handler currently only flipsisArray:arrayToggleButton.addEventListener("click", (event) => { event.preventDefault(); this.toggleArray(); });Unless the Alpine template separately calls
syncState()orsyncArray(), this risks the hiddendata-array-inputvalue and the button'sactive/aria-pressedstate getting out of sync withisArray.Consider wiring the click to sync everything so the component is self-contained, e.g.:
- if (arrayToggleButton) { - arrayToggleButton.addEventListener("click", (event) => { - event.preventDefault(); - this.toggleArray(); - }); - this.updateArrayButton(); - } + if (arrayToggleButton) { + arrayToggleButton.addEventListener("click", (event) => { + event.preventDefault(); + this.toggleArray(); + this.syncState(); + }); + this.updateArrayButton(); + }This guarantees the posted
is_arrayvalue and visual toggle always reflect the internal state, regardless of template wiring.radis/extractions/templates/extractions/extraction_job_detail.html (1)
117-132: Consider extracting shared array/selection display into a partialThe “Array Output” and “Selections” blocks here mirror the wizard summary template. To keep future copy/structure changes in sync, consider extracting this snippet into a small include/partial shared by both views.
radis/extractions/utils/processor_utils.py (1)
40-50: Minor simplification opportunity in prompt generationIn
generate_output_fields_prompt, you can avoid re-instantiatingOutputTypeon each iteration and mirror the model’s pattern by comparing the raw value directly:- if OutputType(field.output_type) == OutputType.SELECTION and field.selection_options: + if field.output_type == OutputType.SELECTION and field.selection_options:This is a small readability/robustness improvement and keeps the logic consistent with
OutputField.clean.radis/extractions/forms.py (1)
189-321: Form-side selection/array handling looks solid; consider centralizing validation logicThe new
OutputFieldFormfields, layout wiring, andclean_*methods correctly:
- Treat
selection_optionsas hidden JSON serialized data, normalizing to a list of trimmed, unique strings withinMAX_SELECTION_OPTIONS.- Parse
is_arrayfrom the hidden field into a boolean.- Enforce that selections are present only when
output_type == OutputType.SELECTION, mirroringOutputField.clean.One improvement to consider long term is centralizing the selection-options validation (type checks, trimming, length, uniqueness) in a shared helper used by both
OutputField.cleanandOutputFieldForm.clean_selection_options, to avoid the two copies drifting as the rules evolve.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (14)
radis/core/static/core/core.css(1 hunks)radis/core/static/core/core.js(2 hunks)radis/core/templates/cotton/formset.html(1 hunks)radis/extractions/constants.py(1 hunks)radis/extractions/forms.py(3 hunks)radis/extractions/migrations/0004_outputfield_selection_options.py(1 hunks)radis/extractions/migrations/0005_outputfield_is_array.py(1 hunks)radis/extractions/models.py(4 hunks)radis/extractions/templates/extractions/_selection_options_field.html(1 hunks)radis/extractions/templates/extractions/extraction_job_detail.html(1 hunks)radis/extractions/templates/extractions/extraction_job_wizard_summary.html(1 hunks)radis/extractions/tests/test_forms.py(1 hunks)radis/extractions/tests/unit/test_processor_utils.py(1 hunks)radis/extractions/utils/processor_utils.py(3 hunks)
🧰 Additional context used
🧬 Code graph analysis (3)
radis/extractions/utils/processor_utils.py (1)
radis/extractions/models.py (1)
OutputType(77-81)
radis/extractions/forms.py (1)
radis/extractions/models.py (2)
OutputType(77-81)clean(109-147)
radis/extractions/tests/unit/test_processor_utils.py (2)
radis/extractions/models.py (1)
OutputType(77-81)radis/extractions/utils/processor_utils.py (1)
generate_output_fields_schema(11-34)
🪛 GitHub Actions: Run Continuous Integration
radis/extractions/models.py
[error] 1-1: NotNullViolation: null value in column "owner_id" of relation "extractions_extractionjob" violates not-null constraint during INSERT. Ensure an owner is set when creating ExtractionJob.
radis/extractions/utils/processor_utils.py
[error] 24-24: ValueError: Selection output requires at least one option.
radis/extractions/tests/unit/test_processor_utils.py
[error] 1-1: NotNullViolation: null value in column "owner_id" of relation "extractions_extractionjob" during unit tests when creating ExtractionJob via factory.
radis/extractions/tests/test_forms.py
[error] 1-1: NotNullViolation: null value in column "owner_id" of relation "extractions_extractionjob" during test setup. ExtractionJobFactory.create() attempted to insert without an owner.
🪛 Ruff (0.14.6)
radis/extractions/utils/processor_utils.py
24-24: Avoid specifying long messages outside the exception class
(TRY003)
27-27: Avoid specifying long messages outside the exception class
(TRY003)
radis/extractions/migrations/0005_outputfield_is_array.py
5-7: Mutable class attributes should be annotated with typing.ClassVar
(RUF012)
9-15: Mutable class attributes should be annotated with typing.ClassVar
(RUF012)
radis/extractions/forms.py
201-207: Mutable class attributes should be annotated with typing.ClassVar
(RUF012)
270-270: Avoid specifying long messages outside the exception class
(TRY003)
273-273: Avoid specifying long messages outside the exception class
(TRY003)
278-278: Avoid specifying long messages outside the exception class
(TRY003)
281-281: Avoid specifying long messages outside the exception class
(TRY003)
285-287: Avoid specifying long messages outside the exception class
(TRY003)
289-289: Avoid specifying long messages outside the exception class
(TRY003)
radis/extractions/migrations/0004_outputfield_selection_options.py
5-7: Mutable class attributes should be annotated with typing.ClassVar
(RUF012)
9-29: Mutable class attributes should be annotated with typing.ClassVar
(RUF012)
🔇 Additional comments (9)
radis/core/templates/cotton/formset.html (1)
6-11: Crispy rendering and formset template structure look correctUsing
{% crispy ... %}inside<c-formset-form>keeps the formset template consistent with the JSFormSethelper and should not change behavior versus the previous|crispyusage.radis/core/static/core/core.js (1)
49-65: Enter-key suppression for marked forms is scoped and safeThe
data-prevent-enter-submithandler cleanly prevents accidental submits from text-like<input>elements while leaving other forms/pages unaffected. This is a good UX improvement with minimal risk.radis/extractions/migrations/0005_outputfield_is_array.py (1)
1-15: Migration foris_arrayfield matches the model changeAdding
is_array = models.BooleanField(default=False)onOutputFieldwith dependency on the previous selection-options migration is consistent and should apply cleanly.radis/extractions/constants.py (1)
1-1: Max-selection-options constant is clear and aligned with usageDefining
MAX_SELECTION_OPTIONS = 7centrally is appropriate and keeps model/form/UI validation in sync.radis/extractions/templates/extractions/extraction_job_wizard_summary.html (1)
94-109: Array/selections summary and Enter-prevention wiring look consistentThe new “Array Output” and “Selections” rows give a clear summary of
is_arrayandselection_options, and the wording aligns with the detail view. Addingdata-prevent-enter-submit="true"to this form correctly hooks into the new JS to avoid accidental submits while editing.Also applies to: 117-120
radis/extractions/models.py (1)
14-15: Selection output type and option validation are well-structuredThe
OutputType.SELECTIONaddition,selection_options/is_arrayfields, and theclean()implementation together provide a clear contract:
- SELECTION fields must have 1–
MAX_SELECTION_OPTIONSnon-empty, string, whitespace-trimmed, unique options.- Non-SELECTION fields cannot carry
selection_optionsand are normalized back to an empty list.This is a robust place to centralize invariants that other layers (forms, processor utils, UI) can safely rely on.
Also applies to: 81-82, 92-93, 109-148
radis/core/static/core/core.css (1)
42-72: Selection/array toggle styles look consistent and non-invasiveThe new flex gaps and array-toggle button styles integrate cleanly with the existing Bootstrap layout and the template markup; no issues spotted from a CSS/UX perspective.
radis/extractions/templates/extractions/_selection_options_field.html (1)
1-51: Selection options UI wiring looks coherentThe Alpine bindings, hidden form fields, and error display are all consistent with the form API and new CSS. The per-option controls and empty/supported states look correct.
radis/extractions/migrations/0004_outputfield_selection_options.py (1)
10-28: Migration matches model OutputType semanticsAdding
selection_optionsas a JSONField withblank=True, default=listand constrainingoutput_typechoices (including"S", "Selection") is consistent with theOutputTypeTextChoices and should migrate cleanly.
| @pytest.mark.django_db | ||
| def test_output_field_form_accepts_selection_options(): | ||
| job = ExtractionJobFactory.create() | ||
| form = OutputFieldForm( | ||
| data={ | ||
| "name": "tumor_grade", | ||
| "description": "Classified tumor grade.", | ||
| "output_type": OutputType.SELECTION, | ||
| "selection_options": json.dumps(["Grade 1", "Grade 2"]), | ||
| "is_array": "false", | ||
| }, | ||
| instance=OutputField(job=job), | ||
| ) | ||
|
|
||
| assert form.is_valid() | ||
| instance = form.save(commit=False) | ||
|
|
||
| assert instance.selection_options == ["Grade 1", "Grade 2"] | ||
|
|
||
|
|
||
| @pytest.mark.django_db | ||
| def test_output_field_form_requires_options_for_selection(): | ||
| job = ExtractionJobFactory.create() | ||
| form = OutputFieldForm( | ||
| data={ | ||
| "name": "tumor_grade", | ||
| "description": "Classified tumor grade.", | ||
| "output_type": OutputType.SELECTION, | ||
| "selection_options": json.dumps([]), | ||
| "is_array": "false", | ||
| }, | ||
| instance=OutputField(job=job), | ||
| ) | ||
|
|
||
| assert not form.is_valid() | ||
| assert "selection_options" in form.errors | ||
|
|
||
|
|
||
| @pytest.mark.django_db | ||
| def test_output_field_form_rejects_options_for_non_selection(): | ||
| job = ExtractionJobFactory.create() | ||
| form = OutputFieldForm( | ||
| data={ | ||
| "name": "tumor_grade", | ||
| "description": "Classified tumor grade.", | ||
| "output_type": OutputType.TEXT, | ||
| "selection_options": json.dumps(["Grade 1"]), | ||
| "is_array": "false", | ||
| }, | ||
| instance=OutputField(job=job), | ||
| ) | ||
|
|
||
| assert not form.is_valid() | ||
| assert "selection_options" in form.errors | ||
|
|
||
|
|
||
| @pytest.mark.django_db | ||
| def test_output_field_clean_trims_selection_options(): | ||
| job = ExtractionJobFactory.create() | ||
| field = OutputField( | ||
| job=job, | ||
| name="tumor_grade", | ||
| description="Classified tumor grade.", | ||
| output_type=OutputType.SELECTION, | ||
| selection_options=[" Grade 1 ", "Grade 2 "], | ||
| ) | ||
|
|
||
| field.full_clean() | ||
|
|
||
| assert field.selection_options == ["Grade 1", "Grade 2"] | ||
|
|
||
|
|
||
| @pytest.mark.django_db | ||
| def test_output_field_clean_rejects_selection_options_for_other_types(): | ||
| job = ExtractionJobFactory.create() | ||
| field = OutputField( | ||
| job=job, | ||
| name="tumor_grade", | ||
| description="Classified tumor grade.", | ||
| output_type=OutputType.TEXT, | ||
| selection_options=["Grade 1"], | ||
| ) | ||
|
|
||
| with pytest.raises(ValidationError): | ||
| field.full_clean() | ||
|
|
||
|
|
||
| @pytest.mark.django_db | ||
| def test_output_field_form_handles_array_toggle(): | ||
| job = ExtractionJobFactory.create() | ||
| form = OutputFieldForm( | ||
| data={ | ||
| "name": "measurements", | ||
| "description": "Multiple numeric values.", | ||
| "output_type": OutputType.NUMERIC, | ||
| "selection_options": json.dumps([]), | ||
| "is_array": "true", | ||
| }, | ||
| instance=OutputField(job=job), | ||
| ) | ||
|
|
||
| assert form.is_valid() | ||
| instance = form.save(commit=False) | ||
| assert instance.is_array is True | ||
|
|
||
|
|
||
| @pytest.mark.django_db | ||
| def test_output_field_form_rejects_duplicate_selection_options(): | ||
| job = ExtractionJobFactory.create() | ||
| form = OutputFieldForm( | ||
| data={ | ||
| "name": "tumor_grade", | ||
| "description": "Classified tumor grade.", | ||
| "output_type": OutputType.SELECTION, | ||
| "selection_options": json.dumps(["Grade 1", "Grade 1"]), | ||
| "is_array": "false", | ||
| }, | ||
| instance=OutputField(job=job), | ||
| ) | ||
|
|
||
| assert not form.is_valid() | ||
| assert "selection_options" in form.errors | ||
|
|
||
|
|
||
| @pytest.mark.django_db | ||
| def test_output_field_clean_rejects_duplicate_selection_options(): | ||
| job = ExtractionJobFactory.create() | ||
| field = OutputField( | ||
| job=job, | ||
| name="tumor_grade", | ||
| description="Classified tumor grade.", | ||
| output_type=OutputType.SELECTION, | ||
| selection_options=["Grade 1", "Grade 1"], | ||
| ) | ||
|
|
||
| with pytest.raises(ValidationError): | ||
| field.full_clean() | ||
|
|
||
|
|
||
| @pytest.mark.django_db | ||
| def test_output_field_form_rejects_whitespace_only_selection_option(): | ||
| job = ExtractionJobFactory.create() | ||
| form = OutputFieldForm( | ||
| data={ | ||
| "name": "tumor_grade", | ||
| "description": "Classified tumor grade.", | ||
| "output_type": OutputType.SELECTION, | ||
| "selection_options": json.dumps(["Grade 1", " "]), | ||
| "is_array": "false", | ||
| }, | ||
| instance=OutputField(job=job), | ||
| ) | ||
|
|
||
| assert not form.is_valid() | ||
| assert "selection_options" in form.errors | ||
|
|
||
|
|
||
| @pytest.mark.django_db | ||
| def test_output_field_form_accepts_unicode_and_long_selection_options(): | ||
| job = ExtractionJobFactory.create() | ||
| unicode_option = "Grädé αβγ測試" | ||
| long_option = "Grade " + ("X" * 150) | ||
| form = OutputFieldForm( | ||
| data={ | ||
| "name": "tumor_grade", | ||
| "description": "Classified tumor grade.", | ||
| "output_type": OutputType.SELECTION, | ||
| "selection_options": json.dumps([unicode_option, long_option]), | ||
| "is_array": "false", | ||
| }, | ||
| instance=OutputField(job=job), | ||
| ) | ||
|
|
||
| assert form.is_valid() | ||
| instance = form.save(commit=False) | ||
| assert instance.selection_options == [unicode_option.strip(), long_option.strip()] |
There was a problem hiding this comment.
ExtractionJobFactory usage in tests is causing NotNullViolation (missing owner)
CI is failing with a NotNullViolation on owner_id when ExtractionJobFactory.create() is called without an owner. All tests here construct jobs that way, so they’ll break as soon as owner becomes required.
Consider centralizing job creation in a pytest fixture that sets a valid owner via django_user_model, and then injecting that fixture into all tests instead of calling ExtractionJobFactory.create() directly:
@@
import json
import pytest
from django.core.exceptions import ValidationError
from radis.extractions.factories import ExtractionJobFactory
@@
-from radis.extractions.models import OutputField, OutputType
+from radis.extractions.models import OutputField, OutputType
+
+
+@pytest.fixture
+def job(django_user_model):
+ owner = django_user_model.objects.create_user(
+ username="test-owner",
+ password="test-pass",
+ )
+ return ExtractionJobFactory.create(owner=owner)
@@
-@pytest.mark.django_db
-def test_output_field_form_accepts_selection_options():
- job = ExtractionJobFactory.create()
+@pytest.mark.django_db
+def test_output_field_form_accepts_selection_options(job):
@@
- instance=OutputField(job=job),
+ instance=OutputField(job=job),Apply the same pattern (accepting the job fixture and dropping the local ExtractionJobFactory.create() call) to the remaining tests in this module.
Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
radis/extractions/tests/test_forms.py lines 11-186: tests call
ExtractionJobFactory.create() without an owner which fails when owner becomes
required; create a pytest fixture (e.g., job) that accepts django_user_model to
create a user and then create the ExtractionJob via
ExtractionJobFactory.create(owner=user) and update every test in this file to
accept that job fixture (remove local ExtractionJobFactory.create() calls) so
each test uses the provided job with a valid owner.
| @pytest.mark.django_db | ||
| def test_generate_output_fields_schema_uses_literal_for_selection_fields(): | ||
| job = ExtractionJobFactory.create() | ||
| field = OutputFieldFactory( | ||
| job=job, | ||
| name="grade", | ||
| output_type=OutputType.SELECTION, | ||
| ) | ||
| field.selection_options = ["Grade 1", "Grade 2"] | ||
| field.save() | ||
|
|
||
| schema = generate_output_fields_schema(job.output_fields) | ||
|
|
||
| grade_field = schema.model_fields["grade"] | ||
| annotation = grade_field.annotation | ||
| assert get_origin(annotation) is Literal | ||
| assert set(get_args(annotation)) == {"Grade 1", "Grade 2"} | ||
|
|
||
|
|
||
| @pytest.mark.django_db | ||
| def test_generate_output_fields_schema_wraps_literal_in_list_for_array_selections(): | ||
| job = ExtractionJobFactory.create() | ||
| field = OutputFieldFactory( | ||
| job=job, | ||
| name="grade_multi", | ||
| output_type=OutputType.SELECTION, | ||
| ) | ||
| field.selection_options = ["High", "Low"] | ||
| field.is_array = True | ||
| field.save() | ||
|
|
||
| schema = generate_output_fields_schema(job.output_fields) | ||
|
|
||
| grade_field = schema.model_fields["grade_multi"] | ||
| annotation = grade_field.annotation | ||
| assert get_origin(annotation) is list | ||
| (inner_annotation,) = get_args(annotation) | ||
| assert get_origin(inner_annotation) is Literal | ||
| assert set(get_args(inner_annotation)) == {"High", "Low"} |
There was a problem hiding this comment.
Schema assertions look good; fix factory usage to set ExtractionJob.owner
Both tests correctly check that SELECTION fields become Literal[...] and array selections become list[Literal[...]], matching generate_output_fields_schema.
However, CI is failing with NotNullViolation: null value in column "owner_id" of relation "extractions_extractionjob" when calling ExtractionJobFactory.create(). That indicates the factory (or this call site) is not providing a non-null owner, which is required via the base AnalysisJob model.
Update the tests (or the factory) so that ExtractionJob instances created here always have a valid owner, e.g. by:
- Passing an owner explicitly in these tests, or
- Adjusting
ExtractionJobFactoryso it constructs an owner by default.
After that change, re-run the test suite to confirm the migration/tests pass.
🤖 Prompt for AI Agents
In radis/extractions/tests/unit/test_processor_utils.py around lines 10 to 48,
the tests call ExtractionJobFactory.create() but fail with NotNullViolation due
to a missing required owner on ExtractionJob; fix by ensuring created
ExtractionJob instances have a valid owner: either pass an owner explicitly in
these tests (e.g., ExtractionJobFactory.create(owner=SomeUserFactory.create()))
or modify ExtractionJobFactory to always create and assign a default owner
(e.g., Add a related owner factory/factory_boy SubFactory on
ExtractionJobFactory) so the owner_id is never null; update both test cases
accordingly and re-run the test suite.
| MAX_SELECTION_OPTIONS = 7.0 # Maximum number of Enum values (selection output types) a | ||
| # user can add per Output Field the Extraction Output Fields Page. |
There was a problem hiding this comment.
Define as an integer, not a float.
The constant represents a maximum count and should be defined as an integer (7) rather than a float (7.0). Using a float for a count is semantically incorrect and may cause issues with type checkers.
Apply this diff:
-MAX_SELECTION_OPTIONS = 7.0 # Maximum number of Enum values (selection output types) a
+MAX_SELECTION_OPTIONS = 7 # Maximum number of Enum values (selection output types) a
# user can add per Output Field the Extraction Output Fields Page.📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| MAX_SELECTION_OPTIONS = 7.0 # Maximum number of Enum values (selection output types) a | |
| # user can add per Output Field the Extraction Output Fields Page. | |
| MAX_SELECTION_OPTIONS = 7 # Maximum number of Enum values (selection output types) a | |
| # user can add per Output Field the Extraction Output Fields Page. |
🤖 Prompt for AI Agents
In radis/extractions/constants.py around lines 1 to 2, the MAX_SELECTION_OPTIONS
constant is defined as a float (7.0) but represents a count; change its
definition to an integer (7) so the value is semantically correct and type
checkers treat it as an int.
NumericalAdvantage
force-pushed
the
selectionOutputType
branch
2 times, most recently
from
2569c89 to
2b33b22
Compare
NumericalAdvantage
force-pushed
the
selectionOutputType
branch
from
7043d4a to
9a9500b
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
radis/core/static/core/core.js (1)
159-159: Remove unused variable.The
maxSelectionOptionsvariable is declared but never used. The actual max options value is computed byparseMaxOptions()and stored ininitialMaxOptionsat line 172.Apply this diff to remove the unused variable:
- const maxSelectionOptions = outputTypeField?.dataset.maxSelectionOptions; - /*Reads the maximum number of selections allowed.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
radis/core/static/core/core.js(2 hunks)radis/extractions/models.py(4 hunks)radis/extractions/templates/extractions/_selection_options_field.html(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- radis/extractions/models.py
- radis/extractions/templates/extractions/_selection_options_field.html
🔇 Additional comments (3)
radis/core/static/core/core.js (3)
49-68: LGTM!The Enter key prevention logic is well-implemented with appropriate type checking and selective application only to text inputs. The opt-in approach using
data-prevent-enter-submitprovides good flexibility.
184-227: Well-implemented state restoration logic.The
init()method correctly preserves user input when toggling between output types. The restoration logic at lines 202-226 ensures that selection options are saved when switching away from Selection type and restored when switching back, preventing data loss.
231-331: Excellent implementation with good practices.The parsing, syncing, and manipulation methods demonstrate several strengths:
- Robust error handling in
parseOptionswith try-catch- Data sanitization in
syncOptionsremoves empty entries- Good UX with automatic focus management in
addOption- Proper accessibility via
aria-pressedattribute inupdateArrayButton
|
Closed in favor of #172 (the changes/commits are integrated there). |
Adding a "Selection Output Type" to the list of possible output types like Text, Boolean, Numeric. This essentially behaves like a enum type - It accepts (for now upto 7) possible options (or lets say "Selections"), which are then sent to the LLM and the LLM is asked to choose one of the "Selections" as answer.
The form is rendered by adding an Alpine component called SelectionOptions. A new template is created to serve the form called _selection_options_field. Other templates that small updates are extraction_job_detail adn extraction_job_wizard_summary, because out new selection type has to be shown on these pages.
Added tests to verify correctness of data coming out of the Selection form.
Summary by CodeRabbit
New Features
UX / Style
Templates
Forms / Validation
Tests
Chores
✏️ Tip: You can customize this high-level summary in your review settings.