Subscription and Extraction Features #172
Conversation
…uage header) and use it downstream to deal with format parsing and translations
…close db connections
Code Review: PR #172 - Subscription and Extraction FeaturesThank you for this substantial contribution! I've conducted a comprehensive review of the 3,727 lines added across 63 files. Below is my feedback organized by severity. 🔴 CRITICAL ISSUES1. Missing Permission Check (Security)Location: class SubscriptionCreateView(LoginRequiredMixin, CreateView): # TODO: Add PermissionRequiredMixinIssue: The TODO comment indicates missing authorization. Any authenticated user can create subscriptions without proper permission checks. Recommendation: Add class SubscriptionCreateView(LoginRequiredMixin, PermissionRequiredMixin, CreateView):
permission_required = "subscriptions.add_subscription"2. Database Connection Leak in Error Handling (Performance/Reliability)Location: except APIError as e:
logger.error(f"LLM API error filtering report {report.pk}: {e}")
db.close_old_connections()
return
except ValidationError as e:
logger.error(f"Response validation failed filtering report {report.pk}: {e}")
db.close_old_connections()
returnIssue: Recommendation: Use a finally block: try:
# processing logic
finally:
db.close_old_connections()3. Code Duplication in Sorting Logic (Security/Maintenance)Location: The Recommendation: Extract to a shared method or utility function. 🟠 HIGH PRIORITY ISSUES4. N+1 Query Problem in Processor (Performance)Location: for report in task.reports.filter(groups=active_group):
future = executor.submit(self.process_report, report, task)Issue:
Recommendation: Prefetch related objects before the loop. 5. Race Condition in Subscription Inbox View (Bug)Location: subscription = cast(Subscription, self.object)
subscription.last_viewed_at = timezone.now()
subscription.save(update_fields=["last_viewed_at"])Issue:
Recommendation: Consider using 6. Inconsistent Validation Between Form and Model (Bug Risk)The validation for
Recommendation: Extract validation logic to a shared method on the model. 🟡 MEDIUM PRIORITY ISSUES7. Missing Input Sanitization in Query Generator (Security/Quality)Location: Field names and descriptions are user-provided and passed directly to LLM without sanitization or length limits. Malicious users could craft descriptions designed to manipulate the LLM (prompt injection). Recommendation: Add length limits and sanitize special characters. 8. Weak Error Handling in CSV Export (Reliability)Location: No exception handling if Recommendation: Add try/except to handle database errors gracefully. 9. Magic Numbers in Formset Configuration (Code Quality)# extractions/forms.py:338
max_num=5, # Magic number
# subscriptions/forms.py
max_num=3, # Magic number
max_num=10, # Magic numberRecommendation: Define these as named constants at module level. 10. Inadequate Error Messages (Code Quality)Location: Error messages don't include context about what operation was being performed or the input that caused the failure. Recommendation: Include more context in error messages. 🔵 LOW PRIORITY ISSUES11. Commented Out CodeLocation: # @app.periodic(cron=settings.SUBSCRIPTION_CRON)
@app.task()
def subscription_launcher(timestamp: int):Recommendation: Remove commented code or add a clear explanation comment. 12. Overly Complex Wizard Step Logic (Code Quality)Location: The Recommendation: Extract step-specific logic into separate methods like 13. Missing Database Indexes (Performance)Based on queries observed, consider adding:
✅ POSITIVE OBSERVATIONS
📊 TEST COVERAGE GAPSConsider adding tests for:
🎯 RECOMMENDATIONS SUMMARYImmediate (Before Merge):
Short-term:
Long-term:
Overall Assessment: This is a well-structured PR with good practices in many areas. The main concerns are around permission checks, database connection management, and some N+1 query patterns. With the recommended fixes for critical issues, this should be safe to merge. Great work on the comprehensive test coverage and proper transaction handling! 🎉 |
Pull Request Review: Subscription and Extraction FeaturesThis is a comprehensive PR adding significant functionality across subscriptions and extractions. Overall, the implementation is solid with good test coverage, but there are several areas that need attention. 🔴 Critical Issues1. Error Handling Removes Assertions Without Proper ValidationFile: The changes replace if not completion.choices:
logger.error("LLM returned empty choices list")
raise ValueError("LLM returned no response choices")
answer = completion.choices[0].message.content
if answer is None:
logger.error("LLM returned None for message content")
raise ValueError("LLM returned empty response content")Issue: The error handling catches Recommendation:
2. Database Constraint Vulnerability in OutputFieldFile: The model has a models.CheckConstraint(
condition=(
Q(job__isnull=False, subscription__isnull=True)
| Q(job__isnull=True, subscription__isnull=False)
),
name="output_field_exactly_one_parent",
)Issue: The Recommendation: Add this validation to the def clean(self) -> None:
from django.core.exceptions import ValidationError
super().clean()
# Validate exactly one parent
has_job = self.job_id is not None
has_subscription = self.subscription_id is not None
if has_job == has_subscription: # Both true or both false
raise ValidationError(
"OutputField must have exactly one parent (either job or subscription, not both or neither)"
)
# ... rest of validation3. SQL Injection Risk in CSV ExportFile: for field_pk in subscription.output_fields.order_by('pk').values_list('pk', flat=True):
value = extraction_results.get(str(field_pk))Issue: While not a direct SQL injection (field PKs are integers), there's a design issue: the code uses field PK as the dictionary key in JSON, but Recommendation: Standardize on one approach across both modules. Using PKs is safer (prevents name collisions), but ensure consistency.
|
Code Review: PR #172 - Subscription and Extraction FeaturesThank you for this comprehensive PR! I've reviewed the 3,726 additions across 62 files. Overall, the code is well-structured with excellent test coverage, but there are some important issues to address before merging. CRITICAL Issues (Must Fix)1. Missing Transaction Atomicity in Subscription CreationFiles: radis/subscriptions/views.py:94-119, 151-172 The form_valid method saves the subscription before saving related formsets without wrapping everything in a transaction. If a formset save fails, you'll have an orphaned subscription. Apply transaction.atomic() wrapper to both SubscriptionCreateView and SubscriptionUpdateView. 2. N+1 Query Issue in CSV ExportFile: radis/subscriptions/utils/csv_export.py:56-82 Using .iterator() with prefetch_related() can cause N+1 queries. The order_by() inside the loop re-evaluates the queryset. Use Prefetch object with explicit ordering and remove iterator() or apply row limits. IMPORTANT Issues (Should Fix)3. Missing Input Validation in HTMX Preview EndpointFile: radis/extractions/views.py:311-324 The preview endpoint accepts IDs from GET parameters without validating they're numeric first, which could cause ValueError exceptions. Add try/except blocks for int() conversions. 4. Inconsistent Error Handling in LLM ClientFile: radis/chats/utils/chat_client.py Great improvements to error handling! However, mixing RuntimeError and ValueError makes it harder for callers. Consider defining custom LLM exception classes for consistency. 5. Missing Database IndexesFiles: radis/subscriptions/models.py, radis/extractions/models.py Fields used in filtering/sorting lack indexes:
Add db_index=True and generate migrations. 6. Remove AssertionError CatchingFile: radis/subscriptions/processors.py:94-102 Catching AssertionError is a code smell. Use proper exception types from the LLM client instead. 7. Add Memory Limits to CSV ExportsFile: radis/subscriptions/views.py:344-365 Large exports could consume excessive memory. Add a MAX_CSV_EXPORT_ROWS limit (e.g., 50000). MEDIUM Priority8. Race Condition in last_viewed_at UpdateFile: radis/subscriptions/views.py:266-269 Use Subscription.objects.filter(pk=...).update() for atomic updates instead of save(). 9. Duplicated Sorting LogicFiles: radis/subscriptions/views.py:206-225, 302-321 Extract duplicated sorting allowlist validation to a shared method or constant. 10. Google Style Guide - DocstringsSome public methods lack comprehensive docstrings with Args and Returns sections. EXCELLENT WorkTest CoverageOutstanding test coverage with 800+ new test lines covering:
Security Practices
Django Best Practices
SummaryOverall Assessment: Strong PR with excellent test coverage and good architecture. Main concerns are transaction safety and query performance. Before Merging:
Follow-up Tasks:
Security: No critical vulnerabilities Great work on this feature! The subscription and extraction functionality is well-designed and thoroughly tested. Once the critical issues are addressed, this will be ready to merge. |
Code Review: PR #172 - Subscription and Extraction FeaturesExecutive SummaryThis PR introduces substantial functionality (3,728 additions across 64 files) for subscription processing with LLM-based filtering/extraction, query generation, and CSV export. While the implementation shows good engineering practices in many areas, there are 5 critical security vulnerabilities and 3 critical performance issues that must be addressed before merging. Critical Issues Requiring Immediate Attention🔴 Security Vulnerabilities1. XSS Vulnerability in Template RenderingFile: {{ subscribed_item.extraction_results|get_item:field_key|default_if_none:"—" }}Issue: User-controlled extraction results from LLM are rendered without HTML escaping. Attack Vector: An attacker could manipulate the LLM to return Fix: Apply Django's auto-escaping or use {{ subscribed_item.extraction_results|get_item:field_key|escapejs|default_if_none:"—" }}2. Missing Authorization CheckFile: Issue: Fix: Add proper permission checks: class SubscriptionCreateView(LoginRequiredMixin, PermissionRequiredMixin, ...):
permission_required = 'subscriptions.add_subscription'3. NoSQL Injection Risk in JSON FieldsFile: Issue: Fix: Add JSON schema validation before saving: def clean(self):
super().clean()
if self.extraction_results:
validate_extraction_results_schema(self.extraction_results)🔴 Race Condition and Data Integrity4. Concurrent Processing Race ConditionFile: Issue: Evidence: # Line 44: Multiple threads processing simultaneously
with ThreadPoolExecutor(max_workers=settings.EXTRACTION_LLM_CONCURRENCY_LIMIT) as executor:
executor.map(self.process_report, reports)
# Line 146-152: No uniqueness check before creation
SubscribedItem.objects.create(
subscription=self._subscription,
report=report,
# ...
)Fix: Add database constraint in migration: class Meta:
constraints = [
models.UniqueConstraint(
fields=['subscription', 'report'],
name='unique_subscription_report'
)
]And use ⚡ Performance Issues5. N+1 Query Problem in CSV ExportFile: Issue: Inside the iteration loop, this executes a separate query for EACH subscribed item: subscription.output_fields.order_by("pk").values_list("pk", flat=True)Fix: Pre-fetch field PKs outside the loop (note: lines 35-37 already do this correctly, so remove the redundant query at line 78). 6. ThreadPool Resource ExhaustionFile: Issue: No timeout on LLM API calls means threads can hang indefinitely, exhausting the thread pool. Fix: Add timeout to HTTP client in self._client = openai.OpenAI(
base_url=base_url,
api_key=api_key,
timeout=30.0 # Add timeout
)7. Missing Database IndexesFile: Issue: No composite index on Fix: Add migration with index: class Meta:
indexes = [
models.Index(fields=['subscription', '-created_at'], name='inbox_ordering_idx'),
]Additional Important IssuesDatabase Migration SafetyFile: Issue: Fix: Add data migration step before applying constraint to handle orphaned records. Code Quality IssuesMissing Error NotificationsFile: Issue: Filter validation and extraction failures silently drop reports without notifying users. Recommendation: Log failed reports and provide admin notification mechanism so users know when processing fails. Assertion in Production CodeFile: assert not instance.is_processedIssue: Assertions can be disabled with Python's Fix: Replace with explicit validation: if instance.is_processed:
raise ValueError(f"ExtractionInstance {instance.pk} already processed")Test Coverage GapsMissing Test Coverage:
Recommendation:Run coverage analysis and aim for 85%+ coverage on new code: pytest --cov=radis.subscriptions --cov=radis.extractions --cov-report=htmlPositive Aspects Worth Noting✅ Good error handling in LLM client with proper exception catching and logging Priority RecommendationsMust Fix Before Merge (P0):
Should Fix (P1):
Nice to Have (P2):
Final VerdictStatus: ❌ CHANGES REQUESTED This PR introduces valuable features with generally solid engineering, but the identified security vulnerabilities and performance issues pose significant production risk. Once the P0 issues are addressed, this will be a strong addition to the codebase. Estimated Remediation Time: 8-12 hours for P0 issues Next Steps:
Please let me know if you need clarification on any of these issues or would like help implementing the fixes! |
Pull Request Review: Subscription and Extraction FeaturesOverviewThis is a substantial PR with 3,731 additions and 294 deletions across 64 files. The code quality is generally strong with good test coverage, but there are several areas requiring attention. 🟢 Strengths
🟡 Issues Requiring Attention1. Security: Potential XSS in JavaScript
|
Pull Request Review: Subscription and Extraction FeaturesThank you for this substantial PR! This adds significant functionality to RADIS with subscriptions, extractions, and auto-query generation. I've conducted a comprehensive review covering code quality, bugs, security, and test coverage. 📊 Overview
|
Code Review: PR #172 - Subscription and Extraction FeaturesThank you for this comprehensive PR adding filter questions, extractions to subscriptions, inbox notifications, pagination/sorting/filtering, auto-generated queries, and CSV downloads. CRITICAL ISSUES - Must Fix Before Merge1. Production assert Statements (HIGH)Multiple files use assert for runtime validation. Assertions are disabled with -O flag causing security vulnerabilities. Files: radis/extractions/forms.py, radis/subscriptions/forms.py, radis/subscriptions/tasks.py 2. Race Condition in Subscription Handler (HIGH)File: radis/subscriptions/handlers.py 3. Missing Exception Handling for .get() (MEDIUM-HIGH)Files: radis/extractions/views.py, radis/subscriptions/tasks.py 4. Missing Permission Check (MEDIUM)File: radis/subscriptions/views.py:78 5. Debug Code in Production (LOW)File: radis/core/static/core/core.js:38 PERFORMANCE ISSUES1. Missing Database Indexes (MEDIUM)Need indexes for:
2. Inefficient ThreadPool UsageFile: radis/subscriptions/processors.py:44 SECURITY CONSIDERATIONS1. JSON Payload DoS RiskFile: radis/extractions/forms.py 2. XSS ProtectionFile: radis/extractions/templates/extractions/_search_preview.html POSITIVE ASPECTS
TEST COVERAGE GAPS
CODE QUALITY IMPROVEMENTS
PRIORITY SUMMARYPriority 1 - Block Merge:
Priority 2 - Follow-up PR:
Priority 3 - Tech Debt:
RECOMMENDATIONThis PR adds valuable functionality but requires critical fixes before merging. The code structure is solid. Suggested approach:
Great work! The features are well-designed and LLM integration is thoughtful. |
Pull Request Review: Subscription and Extraction FeaturesThis is a comprehensive PR introducing significant new functionality. Overall, the implementation follows good Django patterns. StrengthsCode Quality:
Architecture:
Critical Issues
Medium Priority Issues
Performance ConsiderationsPositive:
Concerns:
SecurityGood:
Concerns:
Test CoverageExcellent:
Missing:
RecommendationsMust Fix Before Merge:
Should Fix Soon: Overall AssessmentScore: 8/10 Well-implemented PR with high code quality and good test coverage. Main concerns are edge cases in concurrent processing, input validation, and minor code duplication. The error handling improvements in LLM client (replacing assert with proper exceptions) are excellent. Recommendation: Approve with requested changes - Address critical issues before merging. Great work overall! |
|
/gemini review |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This is a substantial pull request that introduces a wide range of significant features, primarily focused on enhancing subscriptions and data extractions. The key additions include the ability to add extractions to subscriptions, auto-generation of search queries from output fields, a new 'Selection' output type with array support, and a completely revamped subscription inbox with pagination, sorting, filtering, and CSV download capabilities. The change to trigger subscription jobs on report creation instead of a cron job is a great architectural improvement.
My review focuses on improving maintainability by addressing areas of code duplication and suggesting simplifications. I've identified several instances of duplicated logic, particularly in form validation and between synchronous and asynchronous client methods. Refactoring these areas will make the new functionality easier to maintain and extend in the future. I've also suggested a simplification in one of the new views to improve readability and robustness. Overall, these are excellent additions to the application.
Code Review - PR #172This is an extensive PR (3,652 additions) adding significant functionality to RADIS. Overall, the implementation is solid with good test coverage, but there are important issues to address before merging. Critical Issues1. Database Performance: N+1 Query Potential The processor fetches filter_questions and output_fields without prefetching. With many subscriptions processing simultaneously, this will cause performance issues. Add prefetch_related when fetching the task in tasks.py. 2. Data Consistency: Missing Transaction Wrapping SubscribedItem creation is not wrapped in transaction.atomic(). If the process crashes during processing, you could have inconsistent data. 3. Model Validation: Incomplete Constraint The output_field_exactly_one_parent constraint ensures exactly one parent, but does not prevent both job and subscription being None. Add a constraint to ensure at least one parent is not null. High Priority Issues4. Error Handling: Silent Failures When LLM calls fail, the function returns early without recording the failure. Users will not know why reports were not processed. Consider adding a processing_errors JSONField to SubscriptionTask and implementing retry logic for transient failures. 5. Resource Management: Thread Pool Efficiency Creating a new ThreadPoolExecutor for each task is inefficient. Use a module-level executor or leverage Django async support (you already have Daphne configured). 6. Code Duplication: Chat Client Methods The chat() method is duplicated between AsyncChatClient and ChatClient with identical error handling. Extract common validation to a shared function. Security Considerations7. LLM Query Generation LLM-generated queries are passed to the search system. While basic query parsing validation exists, add comprehensive sanitization and consider using a whitelist approach for allowed operators. 8. LLM Prompt Injection User-controlled filter questions are inserted into LLM prompts. Sanitize question text before prompt insertion and add maximum length limits. 9. Input Validation: Selection Options Good validation exists, but also validate maximum length per option to prevent DoS via large strings. What Was Done Well
Medium Priority Issues
RecommendationsI recommend addressing the critical and high-priority issues before merging. The medium-priority issues can be handled in follow-up PRs. Overall, this is excellent work! The feature set is impressive and will significantly enhance RADIS capabilities. |
Code Review - PR #172: Subscription & Extraction EnhancementsThank you for this substantial feature addition! This PR adds important functionality to the subscription and extraction systems. I've conducted a thorough review and found several areas that need attention before merging. 🚨 Critical Issues (Must Fix)1. N+1 Query Problem in Subscription ProcessorFile: Inside the report processing loop, the code repeatedly queries for report in task.reports.filter(groups=active_group):
filter_questions = list(subscription.filter_questions.order_by("pk")) # Query per report\!
# ... later ...
output_fields = list(subscription.output_fields.order_by("pk")) # Another query per report\!Impact: Processing 100 reports triggers 200+ queries instead of 3. Fix: Move these queries outside the loop: subscription: Subscription = task.job.subscription
filter_questions = list(subscription.filter_questions.order_by("pk"))
output_fields = list(subscription.output_fields.order_by("pk"))
for report in task.reports.filter(groups=active_group):
# Use pre-fetched data2. Inefficient Report Fetching in Task CreationFile: for document_id in document_ids:
task.reports.add(Report.objects.get(document_id=document_id)) # N queries\!Fix: Fetch all reports at once: reports = Report.objects.filter(document_id__in=document_ids)
task.reports.add(*reports)3. Missing Max Length Validation on Selection OptionsFiles: While there's a limit of 7 selection options, there's no validation on the total character length. An attacker could submit 7 options with 10,000 characters each, causing:
Recommendation: Add constants and validation: MAX_SELECTION_OPTION_LENGTH = 200
MAX_TOTAL_SELECTION_LENGTH = 1000
# In validation:
total_length = sum(len(opt) for opt in cleaned)
if total_length > MAX_TOTAL_SELECTION_LENGTH:
raise forms.ValidationError(f"Total length exceeds {MAX_TOTAL_SELECTION_LENGTH} characters")4. Thread Pool Exception Handling IssueFile: with ThreadPoolExecutor(max_workers=settings.EXTRACTION_LLM_CONCURRENCY_LIMIT) as executor:
try:
for report in task.reports.filter(groups=active_group):
future = executor.submit(self.process_report, report, task)
futures.append(future)
for future in futures:
future.result() # If one raises, others keep running\!
finally:
db.close_old_connections() # Closes connections while threads active\!Fix: Handle exceptions per-future: for future in futures:
try:
future.result()
except Exception as e:
logger.error(f"Task processing failed: {e}")
|
| Aspect | Rating | Notes |
|---|---|---|
| Code Quality | 7/10 | Well-structured but has performance issues |
| Security | 6.5/10 | Input validation gaps, needs audit logging |
| Test Coverage | 7/10 | Good view tests, missing processor integration tests |
| Performance | 5/10 | Critical N+1 queries need fixing |
🎯 Summary & Recommendation
This PR adds valuable features with a solid architectural foundation. However, the N+1 query problems and missing validation must be addressed before merge as they will cause significant performance degradation and potential security issues in production.
Action Items:
- ✅ Fix N+1 query in subscription processor (move queries outside loop)
- ✅ Fix inefficient Report.objects.get() in task creation
- ✅ Add max length validation for selection options
- ✅ Fix thread pool exception handling
⚠️ Add handler registration duplicate prevention⚠️ Add error handling in CSV export⚠️ Add integration tests for handler system
Once the critical issues (1-4) are resolved, this will be ready to merge. Happy to review again after updates!
Review based on RADIS coding standards from CLAUDE.md: Google Python Style Guide, 100-char line length, proper error handling, and performance optimization.
…tions for extractions
Pull Request ReviewThis is a substantial PR (+3922 lines, 70 files) that merges multiple feature branches. Overall, the code quality is good with solid test coverage and thoughtful refactoring. Here's my detailed feedback: ✅ Strengths1. Excellent Code Refactoring
2. Robust Validation
3. Good Test Coverage
4. Smart Architecture Patterns
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
radis/subscriptions/tasks.py (1)
64-70: Potential N+1 query issue when fetching reports.Each iteration calls
Report.objects.get(document_id=document_id)individually. For large batches, this creates many database queries.Proposed optimization using bulk fetch
for document_ids in batched(new_document_ids, settings.SUBSCRIPTION_REFRESH_TASK_BATCH_SIZE): logger.debug("Creating SubscriptionTask for document IDs: %s", document_ids) task = SubscriptionTask.objects.create(job=job, status=SubscriptionTask.Status.PENDING) - for document_id in document_ids: - task.reports.add(Report.objects.get(document_id=document_id)) + reports = Report.objects.filter(document_id__in=document_ids) + task.reports.add(*reports) task.delay()radis/extractions/urls.py (1)
90-94: Duplicate URL pattern detected.The URL pattern
jobs/<int:pk>/verify/withExtractionJobVerifyViewand nameextraction_job_verifyis already defined at lines 65-69. This duplicate registration is likely a merge artifact and should be removed.🔧 Proposed fix
path( "jobs/<int:pk>/restart/", ExtractionJobRestartView.as_view(), name="extraction_job_restart", ), - path( - "jobs/<int:pk>/verify/", - ExtractionJobVerifyView.as_view(), - name="extraction_job_verify", - ), path( "tasks/<int:pk>/",radis/search/forms.py (2)
101-108: PotentialKeyErrorif individual field validation fails.If
clean_age_fromorclean_age_tillraises aValidationError, the corresponding key won't exist incleaned_data, causing aKeyErroron lines 102-103.Proposed fix using `.get()` for safe access
def clean(self) -> dict[str, Any] | None: - age_from = self.cleaned_data["age_from"] - age_till = self.cleaned_data["age_till"] + age_from = self.cleaned_data.get("age_from") + age_till = self.cleaned_data.get("age_till") if age_from is not None and age_till is not None and age_from >= age_till: raise forms.ValidationError("Age from must be less than age till") return super().clean()
43-48: No-op statement:form_show_errorsis accessed but not used.Line 45 accesses
self.query_helper.form_show_errorswithout assigning or using the result. This appears to be dead code or an incomplete statement.Proposed fix: remove the no-op or assign as intended
self.query_helper = FormHelper() self.query_helper.template = "search/form_elements/form_part.html" # type: ignore - self.query_helper.form_show_errors self.query_helper.form_tag = False self.query_helper.disable_csrf = True self.query_helper.layout = self.create_query_layout()If the intent was to enable error display, use:
self.query_helper.form_show_errors = True
🤖 Fix all issues with AI agents
In @radis/core/form_fields.py:
- Around line 72-76: The code incorrectly assigns empty_label to a
forms.ChoiceField (variable field) which will raise AttributeError; instead,
when empty_label is not None prepend an empty choice tuple to the choices list:
build choices = [('', empty_label)] + [(lang.code, LANGUAGE_LABELS[lang.code])
for lang in languages] (or insert at index 0) and then assign field.choices =
choices; keep using forms.ChoiceField, languages, LANGUAGE_LABELS and
empty_label names as shown.
In @radis/search/site.py:
- Line 48: The _build_filter_query function currently ignores the
SearchFilters.group field, so add a conditional filter to apply group-based
access control: inside _build_filter_query (used by radis/pgsearch/providers.py)
check if filters.group is not None and, if so, AND a clause filtering reports by
group primary key (e.g., Q(report__groups__pk=filters.group)); ensure you
reference the SearchFilters.group property and combine with the existing fq
variable so queries respect group membership when group is provided.
In @radis/subscriptions/processors.py:
- Around line 81-84: The logger.debug call is passing two separate f-strings so
only the first is logged; update the logger.debug invocation (the call that
currently uses logger.debug(f"LLM returned None for question {question.pk} ",
f"on report {report.pk}",)) to pass a single formatted message — e.g., combine
the parts into one f-string or format string that includes both question.pk and
report.pk — so the full message is emitted.
In @radis/subscriptions/templates/subscriptions/subscription_detail.html:
- Around line 102-116: Wrap the Filter Questions block (the section that
iterates subscription.filter_questions.all and renders question.question and
question.get_expected_answer_display) with an explicit auto-escape directive:
enable autoescaping before the {% with
questions=subscription.filter_questions.all %} block and disable it after the
loop (use {% autoescape on %} ... {% endautoescape %>) so user-generated
question.question is explicitly escaped consistent with other templates.
In @radis/subscriptions/tests/test_views.py:
- Around line 578-581: The assertion "assert item1 and item3 not in items" is
logically incorrect and only checks item3; update the assertion to explicitly
assert both are absent, e.g. replace it with "assert item1 not in items and
item3 not in items" or two separate asserts "assert item1 not in items" and
"assert item3 not in items" referencing the existing variables items, item1, and
item3 in the test.
- Around line 545-548: The assertion uses `item2 and item3 not in items`, which
evaluates incorrectly and only guarantees `item3` is absent; change to assert
both separately by replacing that expression with explicit checks like `assert
item2 not in items` and `assert item3 not in items` so both `item2` and `item3`
are individually verified not to be in `items` (keep `items`, `item1`, `item2`,
`item3` references intact).
🧹 Nitpick comments (34)
radis/subscriptions/static/subscriptions/subscriptions.css (1)
2-3: Avoid!important— address specificity instead.The use of
!importantonfont-sizeandfont-weightis a CSS anti-pattern that makes future overrides difficult and signals an underlying specificity issue. Consider removing!importantand using a more specific selector or relying on cascade order.♻️ Proposed fix: Remove `!important` flags
fieldset > legend { - font-size: 1rem !important; - font-weight: 500 !important; + font-size: 1rem; + font-weight: 500; line-height: 1.5; margin-bottom: 0.5rem; }If this rule is being overridden by other styles, consider increasing specificity with a more targeted selector (e.g.,
form fieldset > legendor adding a class) rather than relying on!important.radis/chats/utils/chat_client.py (2)
69-81: Consider usingNoReturntype hint.Since
_handle_api_erroralways raises and never returns normally, usingNoReturnfromtypinginstead ofNonewould be more accurate and help static type checkers understand the control flow.♻️ Suggested improvement
+from typing import Iterable, NoReturn -def _handle_api_error(error: openai.APIError, operation: str) -> None: +def _handle_api_error(error: openai.APIError, operation: str) -> NoReturn:
128-135: Potential type checker warning:completionmay appear unbound.The control flow is correct since
_handle_api_erroralways raises, ensuringcompletionis bound when reaching the validation call. However, some type checkers may flag this as a potentially unbound variable. Adding an explicitraiseafter the helper call or restructuring withelsewould make the intent clearer.♻️ Alternative structure for clarity
try: completion = await self._client.chat.completions.create(**request) except openai.APIError as e: _handle_api_error(e, "chat") + raise # Unreachable, but clarifies control flow for type checkers answer = _validate_completion_response(completion)Or use an
elseclause:try: completion = await self._client.chat.completions.create(**request) except openai.APIError as e: _handle_api_error(e, "chat") else: answer = _validate_completion_response(completion) self._log_response(answer) return answerradis/extractions/constants.py (1)
1-1: Add a docstring explaining the rationale for MAX_SELECTION_OPTIONS = 7.The choice of 7 as the maximum isn't self-documenting. Consider adding a module-level docstring or inline comment explaining whether this limit is based on UX considerations (e.g., cognitive load, UI space constraints), technical constraints, or another rationale.
📝 Example documentation
+""" +Constants for extraction field configuration. + +MAX_SELECTION_OPTIONS limits the number of choices to prevent UI clutter +and maintain usability in selection fields. +""" + MAX_SELECTION_OPTIONS = 7radis/core/constants.py (1)
6-8: Consider adding documentation for the age-related constants.While the values are reasonable (MIN_AGE=0 for newborns, MAX_AGE=120 as a practical upper limit, AGE_STEP=10 for range grouping), a brief comment explaining their purpose would improve maintainability.
📝 Example documentation
+# Age range constants for form field validation and range building +# MIN_AGE: Includes newborns and infants +# MAX_AGE: Practical upper limit based on maximum human lifespan +# AGE_STEP: Default step size for age range grouping in UI MIN_AGE = 0 MAX_AGE = 120 AGE_STEP = 10radis/subscriptions/handlers.py (1)
20-20: Use lazy % formatting for logging.F-strings in logging statements are evaluated eagerly, even when the log level would suppress output. Use lazy % formatting for better performance.
♻️ Proposed fix
- logger.info(f"Triggering subscription processing for {len(reports)} new report(s)") + logger.info("Triggering subscription processing for %d new report(s)", len(reports))radis/extractions/processors.py (1)
47-59: Consider adding defensive error handling for invalid output field configurations.The
generate_output_fields_schemautility can raiseValueErrorif an output field has invalid configuration (e.g., a selection field with no options). While validation likely occurs during field creation, adding error handling here would prevent thread crashes and improve resilience.🛡️ Proposed error handling
def process_output_fields(self, instance: ExtractionInstance) -> None: job = instance.task.job output_fields = list(job.output_fields.order_by("pk")) + + if not output_fields: + logger.warning(f"No output fields configured for job {job.pk}") + return + + try: - Schema = generate_output_fields_schema(output_fields) + Schema = generate_output_fields_schema(output_fields) + except ValueError as e: + logger.error(f"Invalid output field configuration for job {job.pk}: {e}") + raise + prompt = Template(settings.OUTPUT_FIELDS_SYSTEM_PROMPT).substitute( { "report": instance.text, "fields": generate_output_fields_prompt(output_fields), } ) result = self.client.extract_data(prompt.strip(), Schema) instance.output = result.model_dump() instance.save()radis/urls.py (1)
17-17: LGTM! More robust URL inclusion using apps.is_installed().Switching from
settings.DEBUGchecks toapps.is_installed()preventsNoReverseMatcherrors in test environments whereDEBUGmay be forced toFalse. The explanatory comment clearly documents the rationale.♻️ Optional: Use iterable unpacking per Ruff suggestion
Lines 42-44 and 47-49 could use iterable unpacking instead of list concatenation for slightly more idiomatic Python:
if apps.is_installed("django_browser_reload"): - urlpatterns = [ - path("__reload__/", include("django_browser_reload.urls")), - ] + urlpatterns + urlpatterns = [ + path("__reload__/", include("django_browser_reload.urls")), + *urlpatterns, + ] if apps.is_installed("debug_toolbar"): - urlpatterns = [ - path("__debug__/", include("debug_toolbar.urls")), - ] + urlpatterns + urlpatterns = [ + path("__debug__/", include("debug_toolbar.urls")), + *urlpatterns, + ]Also applies to: 38-49
radis/extractions/tests/test_query_generator.py (2)
49-52: Reorder assertions for clearer failure diagnostics.Checking
query != ""beforequery is not Nonecan produce a confusing failure message ifqueryisNone. Check forNonefirst.Suggested reordering
- assert query != "" assert metadata["success"] is True assert query is not None + assert query != "" assert "nodule" in query.lower()
70-76: Unused variablefixesflagged by static analysis.The
fixesvariable is assigned but never used. If the value is intentionally discarded, prefix with underscore to signal intent.Proposed fix
- fixed_query, fixes = self.generator.validate_and_fix_query(query) + fixed_query, _ = self.generator.validate_and_fix_query(query) assert fixed_query != "" assert "lung nodule" in fixed_query or "lung" in fixed_queryradis/subscriptions/apps.py (1)
34-45: Consider guarding against duplicate handler registration.Django's
ready()can be called multiple times in certain scenarios (e.g., during testing with--keepdb). Appending toreports_created_handlerswithout a guard could result in duplicate handlers.Suggested guard
def register_reports_handler(): """Register handler to trigger subscriptions when reports are created.""" from radis.reports.site import ReportsCreatedHandler, reports_created_handlers from .handlers import handle_reports_created + # Avoid duplicate registration if ready() is called multiple times + if any(h.name == "subscription_launcher" for h in reports_created_handlers): + return + reports_created_handlers.append( ReportsCreatedHandler( name="subscription_launcher", handle=handle_reports_created, ) )radis/subscriptions/tasks.py (1)
94-101: Redundantjob.save()afterSubscriptionJob.objects.create().
create()already persists the object to the database. The subsequentsave()on line 101 is unnecessary.Proposed fix
job = SubscriptionJob.objects.create( subscription=subscription, status=SubscriptionJob.Status.PREPARING, owner=subscription.owner, owner_id=subscription.owner_id, send_finished_mail=subscription.send_finished_mail, ) - job.save() transaction.on_commit(job.delay)radis/extractions/templates/extractions/_selection_options_field.html (2)
10-16: Consider adding aria-label to the add button.The remove button has
aria-label="Remove selection option", but the add button lacks an explicit aria-label. While it has visible text, adding an aria-label would provide consistency and better screen reader support.Suggested addition
<button type="button" class="btn btn-outline-secondary btn-sm" @click.prevent="addOption()" - :disabled="!supportsSelection || options.length >= maxOptions"> + :disabled="!supportsSelection || options.length >= maxOptions" + aria-label="Add selection option"> {% bootstrap_icon "plus-lg" %} Enter a selection </button>
41-45: Error messages may run together without spacing.If multiple validation errors exist, they'll be concatenated without separators. Consider adding spacing between errors.
Suggested fix
{% if form.selection_options.errors %} <div class="invalid-feedback d-block"> - {% for error in form.selection_options.errors %}{{ error }}{% endfor %} + {% for error in form.selection_options.errors %}{{ error }}{% if not forloop.last %} {% endif %}{% endfor %} </div> {% endif %}radis/subscriptions/tests/unit/test_processors.py (1)
39-42: Simplify mock setup.The call to
create_openai_client_mock(extraction_output)is immediately overridden by reassigningopenai_mock.beta.chat.completions.parsewithside_effect. Consider constructing the mock directly or updating the helper to accept multiple responses.♻️ Proposed refactor
- openai_mock = create_openai_client_mock(extraction_output) - openai_mock.beta.chat.completions.parse = MagicMock( - side_effect=[filter_response, extraction_response] - ) + openai_mock = MagicMock() + openai_mock.beta.chat.completions.parse = MagicMock( + side_effect=[filter_response, extraction_response] + )radis/subscriptions/utils/testing_helpers.py (1)
31-36: Consider removing redundant owner_id assignment.Line 34 explicitly sets
owner_id=user.idafter settingowner=user. Django automatically sets the foreign key ID field when you assign the related object, making the explicitowner_idassignment redundant.♻️ Proposed refactor
job = SubscriptionJob.objects.create( subscription=subscription, owner=user, - owner_id=user.id, status=SubscriptionJob.Status.PENDING, )radis/subscriptions/templates/subscriptions/subscription_inbox.html (1)
14-40: Refactor query string construction to reduce duplication and improve maintainability.Lines 16-31 repeat the same query string construction pattern four times with only
sort_byandorderparameters changing. This is verbose, error-prone, and difficult to maintain. Consider extracting this logic into a template tag or using Django'sQueryDictmanipulation.♻️ Suggested approach
Create a custom template tag that preserves query parameters while updating specific ones:
# In radis/subscriptions/templatetags/subscription_extras.py from django import template from django.http import QueryDict register = template.Library() @register.simple_tag(takes_context=True) def url_with_params(context, **kwargs): """Preserve existing query parameters while updating specified ones.""" request = context['request'] query_dict = request.GET.copy() query_dict.pop('page', None) # Always reset page for key, value in kwargs.items(): if value: query_dict[key] = value else: query_dict.pop(key, None) return f"?{query_dict.urlencode()}"Then in the template:
{% load subscription_extras %} <a href="{% url_with_params sort_by='created_at' order='desc' %}" class="btn btn-outline-secondary {% if current_sort_by == 'created_at' and current_order == 'desc' %}active{% endif %}"> Newest First </a>radis/extractions/tests/test_views.py (1)
37-46: Consider adding a type guard for unexpected chunk types.The current implementation handles
bytesexplicitly but falls through to encoding any non-bytes value. While this works for the expected string chunks, it could mask unexpected types.♻️ Optional: Add explicit type handling
def _collect_csv(response) -> str: chunks: list[bytes] = [] for chunk in response.streaming_content: if isinstance(chunk, bytes): chunks.append(chunk) + elif isinstance(chunk, str): + chunks.append(chunk.encode("utf-8")) else: - chunks.append(chunk.encode("utf-8")) + raise TypeError(f"Unexpected chunk type: {type(chunk)}") csv_bytes = b"".join(chunks) return csv_bytes.decode("utf-8-sig")radis/extractions/utils/csv_export.py (1)
11-17: Consider special handling for array values.The current implementation converts all non-None, non-bool values using
str(). For OutputFields withis_array=True, this will produce Python list syntax (e.g.,"['Option 1', 'Option 2']") which may not be ideal for CSV consumption.♻️ Optional: Format arrays as comma-separated values
def _format_cell(value: Any) -> str: """Format a single output value for CSV export.""" if value is None: return "" if isinstance(value, bool): return "yes" if value else "no" + if isinstance(value, list): + return ", ".join(str(item) for item in value) return str(value)radis/extractions/utils/processor_utils.py (1)
10-12: Misplaced module docstring.The docstring at line 10 is orphaned—it appears after the imports but isn't attached to any function or class. Move it to the top of the file (before imports) to serve as the module docstring, or remove it if the intent was to document
generate_output_fields_schema.♻️ Proposed fix
+"""Build a Pydantic model that describes the structure the extractor must output.""" + from collections.abc import Iterable from typing import Any, Literal from pydantic import BaseModel, create_model from ..models import OutputField, OutputType type Numeric = float | int -"""Build a Pydantic model that describes the structure the extractor must output""" - def generate_output_fields_schema(fields: Iterable[OutputField]) -> type[BaseModel]:radis/subscriptions/utils/csv_export.py (2)
35-42: Combine into a single query.Two separate queries fetch
nameandpkfrom the same queryset. Consider combining them to reduce database round-trips.♻️ Proposed fix
- # Get output field names in PK order (to match dict keys) - field_names: list[str] = list( - subscription.output_fields.order_by("pk").values_list("name", flat=True) - ) - - # Pre-fetch field PKs to avoid N+1 query in the loop below - field_pks: list[int] = list( - subscription.output_fields.order_by("pk").values_list("pk", flat=True) - ) + # Get output field names and PKs in PK order (to match dict keys) + output_fields = list( + subscription.output_fields.order_by("pk").values_list("pk", "name") + ) + field_pks = [pk for pk, _ in output_fields] + field_names = [name for _, name in output_fields]
57-59: Remove unnecessary prefetch.The
subscription__output_fieldsprefetch is not used in the loop—field PKs are already pre-fetched above. This adds overhead without benefit.♻️ Proposed fix
# Data rows - prefetch related fields for efficiency - items = queryset.select_related("report").prefetch_related( - "report__modalities", "subscription__output_fields" - ) + items = queryset.select_related("report").prefetch_related("report__modalities")radis/subscriptions/utils/processor_utils.py (1)
16-17: Consider if the wrapper is needed.
get_output_field_nameis a trivial wrapper that returnsfield.name. While this provides API symmetry withget_filter_question_field_name, consider whether direct access tofield.namewould be clearer at call sites, or document why the indirection is intentional (e.g., for future extensibility).radis/extractions/models.py (1)
126-145: Validation logic correctly uses shared utility, addressing DRY concern.The
clean()method properly delegates tovalidate_selection_optionsfrom the validation module, avoiding code duplication with the form's validation logic.However, the exception re-raise on line 139 should use exception chaining for proper traceback.
Proposed fix for exception chaining
try: self.selection_options = validate_selection_options(self.selection_options) except ValidationError as e: - raise ValidationError({"selection_options": e.message}) + raise ValidationError({"selection_options": e.message}) from eradis/settings/base.py (1)
465-465: Clarify the disabled cron setting.The comment indicates the cron is disabled in favor of a handler-based approach, but the value
"* * * * *"is still set. Consider using an empty string or removing the setting to avoid confusion, or add a more explicit comment explaining where the disabling logic resides.Option: Use a more explicit disabled value
-SUBSCRIPTION_CRON = "* * * * *" # Disabled - using handler-based approach +# Subscription cron disabled - using ReportsCreatedHandler for event-driven processing +# See radis/subscriptions/handlers.py for the handler-based implementation +SUBSCRIPTION_CRON = "" # Empty = disabledAlternatively, if the cron value must remain for backward compatibility, expand the comment:
-SUBSCRIPTION_CRON = "* * * * *" # Disabled - using handler-based approach +# NOTE: Cron-based subscription refresh is disabled. The cron value is retained for +# reference but subscription jobs are triggered via ReportsCreatedHandler instead. +SUBSCRIPTION_CRON = "* * * * *"radis/extractions/utils/query_generator.py (2)
75-77: Broad exception catch could mask unexpected errors.Catching bare
Exceptionis too broad. TheAsyncChatClient.chatmethod raisesopenai.APIError(handled via_handle_api_error) and potentially other specific exceptions. Consider catching more specific exceptions or at minimum uselogging.exceptionto preserve the stack trace.♻️ Suggested improvement
- except Exception as e: - logger.error(f"Error during async LLM query generation: {e}", exc_info=True) + except Exception: + logger.exception("Error during async LLM query generation") metadata["error"] = str(e)
111-113: Uselogging.exceptionfor better error diagnostics.When catching exceptions in
_call_llm, uselogger.exceptionto automatically include the stack trace, which aids debugging LLM integration issues.♻️ Suggested fix
except Exception as e: - logger.error(f"Async LLM call failed: {e}") + logger.exception("Async LLM call failed") return Noneradis/subscriptions/forms.py (1)
138-148: Avoid usingassertfor runtime validation.The
assert cleaned_dataon line 140 could be bypassed if Python runs with optimization (-Oflag), leading to unexpectedNoneaccess. Use explicit validation instead.♻️ Suggested fix
def clean(self) -> dict[str, Any]: cleaned_data = super().clean() - assert cleaned_data + if cleaned_data is None: + return {} question = cleaned_data.get("question") expected_answer = cleaned_data.get("expected_answer")radis/subscriptions/processors.py (1)
93-101: Consider usinglogging.exceptionfor error cases.For
RuntimeErrorandValidationError, the full traceback could help diagnose LLM integration issues.logging.exceptionautomatically includes the stack trace.♻️ Suggested improvement
except RuntimeError as e: - logger.error(f"LLM API error filtering report {report.pk}: {e}") + logger.exception(f"LLM API error filtering report {report.pk}") return except ValidationError as e: - logger.error(f"Response validation failed filtering report {report.pk}: {e}") + logger.exception(f"Response validation failed filtering report {report.pk}") returnradis/subscriptions/views.py (2)
207-227: DRY violation:get_orderingduplicated across views.The
get_orderingmethod is nearly identical inSubscriptionInboxViewandSubscriptionInboxDownloadView. Consider extracting to a shared mixin or utility function.♻️ Suggested approach
Create a mixin or extract the logic:
class SubscribedItemOrderingMixin: ALLOWED_SORT_FIELDS = { "created_at": "created_at", "study_date": "report__study_datetime", } def get_ordering(self) -> str: sort_by = self.request.GET.get("sort_by", "created_at") order = self.request.GET.get("order", "desc") if sort_by not in self.ALLOWED_SORT_FIELDS: sort_by = "created_at" if order not in ["asc", "desc"]: order = "desc" field = self.ALLOWED_SORT_FIELDS[sort_by] return field if order == "asc" else f"-{field}"Then have both views inherit from this mixin.
Also applies to: 303-323
268-271: Side effect inget_context_datamay cause unexpected behavior.Updating
last_viewed_atinget_context_datacauses a database write during template context preparation. Ifget_context_datais called multiple times (e.g., during error handling or by middleware), this could lead to unexpected updates. Consider moving todispatch()or a dedicated method.♻️ Alternative approach
Override
get()instead:def get(self, request, *args, **kwargs): response = super().get(request, *args, **kwargs) # Update last_viewed_at after successful response subscription = self.object subscription.last_viewed_at = timezone.now() subscription.save(update_fields=["last_viewed_at"]) return responseradis/extractions/views.py (3)
471-472: Hardcoded wizard session key is fragile.The session key
"wizard_extraction_job_wizard_view"is derived from the view class name. If the class is renamed, this will break. Consider computing the key dynamically.♻️ Suggested improvement
- wizard_session_key = "wizard_extraction_job_wizard_view" + # Django formtools uses: "wizard_" + view class name (snake_case) + from django.utils.text import camel_case_to_spaces + view_name = camel_case_to_spaces(ExtractionJobWizardView.__name__).replace(" ", "_") + wizard_session_key = f"wizard_{view_name}"Or define it as a class constant on
ExtractionJobWizardViewthat both views can reference.
530-543: Error handling is comprehensive but could uselogging.exception.The exception handling saves error state to the wizard session, which is good for UX. Consider using
logger.exceptioninstead oflogger.error(..., exc_info=True)for consistency with Python logging idioms.
627-631:_Echoclass duplicated across modules.This
_Echohelper class is identical to the one inradis/subscriptions/views.py. Consider extracting to a shared utility module (e.g.,radis/core/utils/csv_export.py).
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (70)
radis/chats/utils/chat_client.pyradis/core/constants.pyradis/core/form_fields.pyradis/core/static/core/core.jsradis/core/templates/cotton/formset.htmlradis/extractions/constants.pyradis/extractions/factories.pyradis/extractions/forms.pyradis/extractions/migrations/0004_outputfield_selection_options.pyradis/extractions/migrations/0005_outputfield_is_array.pyradis/extractions/migrations/0006_remove_extractionjob_provider.pyradis/extractions/migrations/0007_remove_outputfield_unique.pyradis/extractions/migrations/0008_outputfield_subscription_and_more.pyradis/extractions/models.pyradis/extractions/processors.pyradis/extractions/static/extractions/extractions.cssradis/extractions/static/extractions/extractions.jsradis/extractions/templates/extractions/_query_generation_result.htmlradis/extractions/templates/extractions/_query_generation_section.htmlradis/extractions/templates/extractions/_search_preview.htmlradis/extractions/templates/extractions/_search_preview_form_section.htmlradis/extractions/templates/extractions/_selection_options_field.htmlradis/extractions/templates/extractions/extraction_job_detail.htmlradis/extractions/templates/extractions/extraction_job_output_fields_form.htmlradis/extractions/templates/extractions/extraction_job_search_form.htmlradis/extractions/templates/extractions/extraction_job_wizard_summary.htmlradis/extractions/templates/extractions/extraction_result_list.htmlradis/extractions/tests/test_forms.pyradis/extractions/tests/test_query_generator.pyradis/extractions/tests/test_views.pyradis/extractions/tests/unit/test_processor_utils.pyradis/extractions/urls.pyradis/extractions/utils/__init__.pyradis/extractions/utils/csv_export.pyradis/extractions/utils/processor_utils.pyradis/extractions/utils/query_generator.pyradis/extractions/utils/testing_helpers.pyradis/extractions/utils/validation.pyradis/extractions/views.pyradis/search/forms.pyradis/search/site.pyradis/settings/base.pyradis/subscriptions/apps.pyradis/subscriptions/factories.pyradis/subscriptions/filters.pyradis/subscriptions/forms.pyradis/subscriptions/handlers.pyradis/subscriptions/migrations/0011_rename_answers_subscribeditem_filter_results_and_more.pyradis/subscriptions/migrations/0012_procrastinate_on_delete.pyradis/subscriptions/migrations/0013_subscription_last_viewed_at.pyradis/subscriptions/migrations/0014_remove_subscription_query.pyradis/subscriptions/models.pyradis/subscriptions/processors.pyradis/subscriptions/static/subscriptions/subscriptions.cssradis/subscriptions/tables.pyradis/subscriptions/tasks.pyradis/subscriptions/templates/subscriptions/_subscribed_item_preview.htmlradis/subscriptions/templates/subscriptions/subscription_detail.htmlradis/subscriptions/templates/subscriptions/subscription_inbox.htmlradis/subscriptions/templates/subscriptions/subscription_layout.htmlradis/subscriptions/templatetags/__init__.pyradis/subscriptions/templatetags/subscriptions_extras.pyradis/subscriptions/tests/test_views.pyradis/subscriptions/tests/unit/test_processors.pyradis/subscriptions/urls.pyradis/subscriptions/utils/csv_export.pyradis/subscriptions/utils/processor_utils.pyradis/subscriptions/utils/testing_helpers.pyradis/subscriptions/views.pyradis/urls.py
🧰 Additional context used
🧬 Code graph analysis (28)
radis/subscriptions/tests/unit/test_processors.py (4)
radis/chats/utils/testing_helpers.py (1)
create_openai_client_mock(28-32)radis/subscriptions/models.py (1)
SubscribedItem(98-114)radis/subscriptions/utils/processor_utils.py (2)
get_filter_question_field_name(12-13)get_output_field_name(16-17)radis/subscriptions/utils/testing_helpers.py (1)
create_subscription_task(11-43)
radis/extractions/migrations/0005_outputfield_is_array.py (1)
radis/extractions/migrations/0004_outputfield_selection_options.py (1)
Migration(4-29)
radis/extractions/utils/processor_utils.py (1)
radis/extractions/models.py (2)
OutputField(82-145)OutputType(75-79)
radis/extractions/utils/__init__.py (1)
radis/extractions/utils/csv_export.py (1)
iter_extraction_result_rows(20-54)
radis/subscriptions/migrations/0013_subscription_last_viewed_at.py (2)
radis/subscriptions/migrations/0011_rename_answers_subscribeditem_filter_results_and_more.py (1)
Migration(7-56)radis/subscriptions/migrations/0012_procrastinate_on_delete.py (1)
Migration(7-23)
radis/extractions/tests/unit/test_processor_utils.py (3)
radis/extractions/factories.py (3)
ExtractionJobFactory(26-69)OutputFieldFactory(72-88)create(22-23)radis/extractions/models.py (1)
OutputType(75-79)radis/extractions/utils/processor_utils.py (1)
generate_output_fields_schema(13-36)
radis/extractions/migrations/0004_outputfield_selection_options.py (1)
radis/extractions/migrations/0005_outputfield_is_array.py (1)
Migration(4-15)
radis/extractions/processors.py (1)
radis/extractions/utils/processor_utils.py (2)
generate_output_fields_schema(13-36)generate_output_fields_prompt(39-55)
radis/extractions/tests/test_query_generator.py (3)
radis/extractions/models.py (2)
OutputField(82-145)OutputType(75-79)radis/extractions/utils/query_generator.py (5)
AsyncQueryGenerator(21-190)generate_from_fields(31-82)validate_and_fix_query(160-190)_format_fields_for_prompt(115-135)_extract_query_from_response(137-158)radis/chats/utils/chat_client.py (2)
chat(120-135)chat(143-168)
radis/subscriptions/handlers.py (2)
radis/reports/models.py (1)
Report(36-88)radis/subscriptions/tasks.py (1)
subscription_launcher(84-102)
radis/subscriptions/tasks.py (2)
radis/pgsearch/providers.py (2)
search(81-122)filter(158-163)radis/search/site.py (1)
SearchFilters(32-59)
radis/extractions/tests/test_forms.py (2)
radis/extractions/forms.py (1)
OutputFieldForm(172-300)radis/extractions/models.py (2)
OutputField(82-145)OutputType(75-79)
radis/subscriptions/factories.py (1)
radis/subscriptions/models.py (6)
FilterQuestion(72-95)SubscribedItem(98-114)Subscription(26-69)Meta(16-17)Meta(59-66)Meta(110-111)
radis/extractions/utils/csv_export.py (1)
radis/extractions/models.py (2)
ExtractionInstance(167-180)ExtractionJob(27-72)
radis/extractions/utils/query_generator.py (3)
radis/extractions/models.py (1)
OutputField(82-145)radis/search/utils/query_parser.py (3)
QueryParser(57-300)parse(220-277)unparse(280-300)radis/chats/utils/chat_client.py (3)
AsyncChatClient(115-135)chat(120-135)chat(143-168)
radis/subscriptions/apps.py (2)
radis/reports/site.py (1)
ReportsCreatedHandler(8-10)radis/subscriptions/handlers.py (1)
handle_reports_created(10-21)
radis/subscriptions/tests/test_views.py (4)
radis/extractions/factories.py (2)
OutputFieldFactory(72-88)create(22-23)radis/reports/factories.py (3)
LanguageFactory(21-26)ReportFactory(48-88)code(35-37)radis/subscriptions/factories.py (2)
FilterQuestionFactory(26-31)SubscribedItemFactory(49-57)radis/subscriptions/models.py (1)
Subscription(26-69)
radis/subscriptions/utils/csv_export.py (2)
radis/subscriptions/models.py (2)
SubscribedItem(98-114)Subscription(26-69)radis/reports/models.py (1)
modality_codes(87-88)
radis/subscriptions/filters.py (2)
radis/reports/models.py (1)
Modality(25-33)radis/subscriptions/models.py (2)
SubscribedItem(98-114)Subscription(26-69)
radis/subscriptions/utils/processor_utils.py (2)
radis/extractions/models.py (1)
OutputField(82-145)radis/subscriptions/models.py (1)
FilterQuestion(72-95)
radis/extractions/tests/test_views.py (1)
radis/extractions/factories.py (4)
ExtractionInstanceFactory(98-103)ExtractionJobFactory(26-69)ExtractionTaskFactory(91-95)OutputFieldFactory(72-88)
radis/search/forms.py (1)
radis/core/form_fields.py (7)
create_age_range_fields(139-177)create_language_field(17-21)create_language_field(25-29)create_language_field(32-76)create_modality_field(80-84)create_modality_field(88-92)create_modality_field(95-136)
radis/subscriptions/processors.py (5)
radis/chats/utils/chat_client.py (2)
ChatClient(138-186)extract_data(170-186)radis/core/processors.py (1)
AnalysisTaskProcessor(11-77)radis/extractions/utils/processor_utils.py (2)
generate_output_fields_prompt(39-55)generate_output_fields_schema(13-36)radis/subscriptions/utils/processor_utils.py (4)
generate_filter_questions_prompt(31-35)generate_filter_questions_schema(20-28)get_filter_question_field_name(12-13)get_output_field_name(16-17)radis/subscriptions/models.py (2)
Subscription(26-69)SubscribedItem(98-114)
radis/subscriptions/views.py (5)
radis/subscriptions/filters.py (1)
SubscribedItemFilter(28-80)radis/subscriptions/tables.py (1)
SubscriptionTable(9-44)radis/subscriptions/forms.py (1)
SubscriptionForm(20-99)radis/subscriptions/models.py (2)
SubscribedItem(98-114)Subscription(26-69)radis/subscriptions/utils/csv_export.py (1)
iter_subscribed_item_rows(22-87)
radis/subscriptions/urls.py (1)
radis/subscriptions/views.py (1)
SubscriptionInboxDownloadView(283-382)
radis/subscriptions/models.py (1)
radis/extractions/models.py (1)
OutputField(82-145)
radis/extractions/models.py (2)
radis/extractions/forms.py (2)
clean(115-169)clean(278-300)radis/extractions/utils/validation.py (1)
validate_selection_options(8-48)
radis/extractions/forms.py (5)
radis/core/form_fields.py (7)
create_age_range_fields(139-177)create_language_field(17-21)create_language_field(25-29)create_language_field(32-76)create_modality_field(80-84)create_modality_field(88-92)create_modality_field(95-136)radis/core/layouts.py (1)
RangeSlider(24-59)radis/reports/models.py (2)
Language(18-22)Modality(25-33)radis/extractions/models.py (7)
ExtractionJob(27-72)OutputField(82-145)OutputType(75-79)clean(126-145)Meta(17-18)Meta(56-57)Meta(102-121)radis/extractions/utils/validation.py (1)
validate_selection_options(8-48)
🪛 Ruff (0.14.10)
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/utils/processor_utils.py
25-25: Avoid specifying long messages outside the exception class
(TRY003)
28-28: Avoid specifying long messages outside the exception class
(TRY003)
radis/subscriptions/migrations/0013_subscription_last_viewed_at.py
8-10: Mutable class attributes should be annotated with typing.ClassVar
(RUF012)
12-18: Mutable class attributes should be annotated with typing.ClassVar
(RUF012)
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)
radis/urls.py
42-44: Consider iterable unpacking instead of concatenation
Replace with iterable unpacking
(RUF005)
radis/extractions/utils/validation.py
27-27: Avoid specifying long messages outside the exception class
(TRY003)
32-32: Avoid specifying long messages outside the exception class
(TRY003)
36-36: Avoid specifying long messages outside the exception class
(TRY003)
41-43: Avoid specifying long messages outside the exception class
(TRY003)
46-46: Avoid specifying long messages outside the exception class
(TRY003)
radis/subscriptions/forms.py
105-105: Mutable class attributes should be annotated with typing.ClassVar
(RUF012)
146-146: Avoid specifying long messages outside the exception class
(TRY003)
radis/extractions/tests/test_query_generator.py
73-73: Unpacked variable fixes is never used
Prefix it with an underscore or any other dummy variable pattern
(RUF059)
radis/subscriptions/tasks.py
60-60: Avoid specifying long messages outside the exception class
(TRY003)
radis/subscriptions/migrations/0014_remove_subscription_query.py
8-10: Mutable class attributes should be annotated with typing.ClassVar
(RUF012)
12-17: Mutable class attributes should be annotated with typing.ClassVar
(RUF012)
radis/extractions/migrations/0008_outputfield_subscription_and_more.py
9-12: Mutable class attributes should be annotated with typing.ClassVar
(RUF012)
14-53: Mutable class attributes should be annotated with typing.ClassVar
(RUF012)
radis/extractions/migrations/0007_remove_outputfield_unique.py
9-11: Mutable class attributes should be annotated with typing.ClassVar
(RUF012)
13-33: Mutable class attributes should be annotated with typing.ClassVar
(RUF012)
radis/extractions/utils/query_generator.py
109-109: Consider moving this statement to an else block
(TRY300)
111-111: Do not catch blind exception: Exception
(BLE001)
112-112: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
186-186: Consider moving this statement to an else block
(TRY300)
188-188: Do not catch blind exception: Exception
(BLE001)
189-189: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
radis/extractions/factories.py
62-62: Unused method argument: extracted
(ARG002)
62-62: Unused method argument: kwargs
(ARG002)
radis/subscriptions/migrations/0012_procrastinate_on_delete.py
9-12: Mutable class attributes should be annotated with typing.ClassVar
(RUF012)
14-23: Mutable class attributes should be annotated with typing.ClassVar
(RUF012)
radis/chats/utils/chat_client.py
34-34: Avoid specifying long messages outside the exception class
(TRY003)
39-39: Avoid specifying long messages outside the exception class
(TRY003)
59-59: Avoid specifying long messages outside the exception class
(TRY003)
64-64: Avoid specifying long messages outside the exception class
(TRY003)
81-81: Avoid specifying long messages outside the exception class
(TRY003)
radis/subscriptions/processors.py
94-94: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
97-97: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
100-100: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
135-135: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
138-140: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
143-143: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
radis/subscriptions/views.py
111-111: Use raise without specifying exception name
Remove exception name
(TRY201)
164-164: Use raise without specifying exception name
Remove exception name
(TRY201)
197-197: Mutable class attributes should be annotated with typing.ClassVar
(RUF012)
346-346: Unused method argument: request
(ARG002)
346-346: Unused method argument: args
(ARG002)
346-346: Unused method argument: kwargs
(ARG002)
radis/subscriptions/migrations/0011_rename_answers_subscribeditem_filter_results_and_more.py
9-11: Mutable class attributes should be annotated with typing.ClassVar
(RUF012)
13-56: Mutable class attributes should be annotated with typing.ClassVar
(RUF012)
radis/extractions/views.py
86-86: Mutable class attributes should be annotated with typing.ClassVar
(RUF012)
237-237: Avoid specifying long messages outside the exception class
(TRY003)
242-242: Unused method argument: kwargs
(ARG002)
533-533: Use explicit conversion flag
Replace with conversion flag
(RUF010)
648-648: Unused method argument: request
(ARG002)
648-648: Unused method argument: args
(ARG002)
648-648: Unused method argument: kwargs
(ARG002)
radis/extractions/models.py
103-121: Mutable class attributes should be annotated with typing.ClassVar
(RUF012)
139-139: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
radis/extractions/forms.py
50-52: Mutable class attributes should be annotated with typing.ClassVar
(RUF012)
102-105: Avoid specifying long messages outside the exception class
(TRY003)
108-108: Avoid specifying long messages outside the exception class
(TRY003)
188-194: Mutable class attributes should be annotated with typing.ClassVar
(RUF012)
268-268: Avoid specifying long messages outside the exception class
(TRY003)
| field = forms.ChoiceField(required=required) | ||
| field.choices = [(lang.code, LANGUAGE_LABELS[lang.code]) for lang in languages] | ||
| if empty_label is not None: | ||
| field.empty_label = empty_label # type: ignore | ||
| return field |
There was a problem hiding this comment.
Fix incorrect attribute assignment on ChoiceField.
Line 75 attempts to set empty_label on a ChoiceField, but ChoiceField does not have an empty_label attribute—that's specific to ModelChoiceField. This will raise an AttributeError at runtime when use_pk=False and empty_label is not None.
To provide an empty option for ChoiceField, prepend an empty choice tuple to the choices list.
🐛 Proposed fix
else:
# Return ChoiceField - cleaned_data will contain code strings
field = forms.ChoiceField(required=required)
field.choices = [(lang.code, LANGUAGE_LABELS[lang.code]) for lang in languages]
if empty_label is not None:
- field.empty_label = empty_label # type: ignore
+ field.choices = [("", empty_label)] + field.choices
return field📝 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.
| field = forms.ChoiceField(required=required) | |
| field.choices = [(lang.code, LANGUAGE_LABELS[lang.code]) for lang in languages] | |
| if empty_label is not None: | |
| field.empty_label = empty_label # type: ignore | |
| return field | |
| field = forms.ChoiceField(required=required) | |
| field.choices = [(lang.code, LANGUAGE_LABELS[lang.code]) for lang in languages] | |
| if empty_label is not None: | |
| field.choices = [("", empty_label)] + field.choices | |
| return field |
🤖 Prompt for AI Agents
In @radis/core/form_fields.py around lines 72 - 76, The code incorrectly assigns
empty_label to a forms.ChoiceField (variable field) which will raise
AttributeError; instead, when empty_label is not None prepend an empty choice
tuple to the choices list: build choices = [('', empty_label)] + [(lang.code,
LANGUAGE_LABELS[lang.code]) for lang in languages] (or insert at index 0) and
then assign field.choices = choices; keep using forms.ChoiceField, languages,
LANGUAGE_LABELS and empty_label names as shown.
| hx-include="[name='1-query'], [name='1-language'], [name='1-modalities'], [name='1-study_date_from'], [name='1-study_date_till'], [name='1-study_description'], [name='1-patient_sex'], [name='1-age_from'], [name='1-age_till']" | ||
| hx-target="this" | ||
| hx-swap="innerHTML" | ||
| x-on:htmx:after-request="isLoading = false"> |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Search for templates that include this fragment
echo "=== Templates including _search_preview_form_section.html ==="
rg -n "{% include.*_search_preview_form_section" --type=html
echo -e "\n=== Alpine.js x-data patterns with isLoading ==="
rg -nP "x-data.*isLoading" radis/extractions/templates/
echo -e "\n=== Checking Alpine.js in extractions_layout.html ==="
rg -n "alpine" radis/extractions/templates/extractions/extractions_layout.html
echo -e "\n=== Checking for Alpine.js script inclusion in extractions templates ==="
rg -n "alpine" radis/extractions/templates/ -i
echo -e "\n=== Viewing the actual file for context ==="
cat -n radis/extractions/templates/extractions/_search_preview_form_section.html | head -20Repository: openradx/radis
Length of output: 1409
🏁 Script executed:
#!/bin/bash
echo "=== Checking extraction_job_search_form.html for inclusion ==="
cat -n radis/extractions/templates/extractions/extraction_job_search_form.html | head -40
echo -e "\n=== Searching for _search_preview_form_section inclusion in all templates ==="
rg -n "_search_preview_form_section" radis/extractions/templates/
echo -e "\n=== Searching for Alpine.js script tag across all extractions templates ==="
rg -n "script.*src" radis/extractions/templates/
echo -e "\n=== Checking base template or layouts for Alpine.js ==="
find radis/extractions/templates -name "*base*" -o -name "*layout*" | head -10
echo -e "\n=== Checking for Alpine.js in main templates directory ==="
rg -n "alpine" radis/templates/ -i 2>/dev/null | head -20Repository: openradx/radis
Length of output: 2077
🏁 Script executed:
#!/bin/bash
echo "=== Full content of extractions_layout.html ==="
cat -n radis/extractions/templates/extractions/extractions_layout.html
echo -e "\n=== Searching for Alpine.js CDN or import across entire codebase ==="
rg -rn "alpine" radis/ -i --type=html --type=js | grep -i "import\|cdn\|script" | head -20
echo -e "\n=== Checking if _search_preview_form_section is returned as AJAX response ==="
rg -rn "search_preview_form_section" radis/ --type=py
echo -e "\n=== Searching for the extraction_search_preview view ==="
rg -n "extraction_search_preview" radis/extractions/ --type=pyRepository: openradx/radis
Length of output: 1127
🏁 Script executed:
#!/bin/bash
echo "=== Checking core_layout.html for Alpine.js ==="
cat -n radis/templates/core/core_layout.html | grep -i alpine
echo -e "\n=== Searching for Alpine.js in base template ==="
rg -n "alpine" radis/templates/ -i
echo -e "\n=== Finding extraction_search_preview view in urls.py ==="
cat -n radis/extractions/urls.py | grep -A 3 "extraction_search_preview"
echo -e "\n=== Finding the view implementation ==="
rg -n "def extraction_search_preview" radis/extractions/
echo -e "\n=== Checking the view file for template rendering ==="
rg -B5 -A10 "extraction_search_preview" radis/extractions/views.py | head -40
echo -e "\n=== Verifying if _search_preview_form_section is actually included ==="
rg -rn "_search_preview_form_section" radis/Repository: openradx/radis
Length of output: 740
🏁 Script executed:
#!/bin/bash
echo "=== Finding actual templates directory ==="
find radis -type d -name templates 2>/dev/null
echo -e "\n=== Checking if _search_preview_form_section.html file exists ==="
find radis -name "_search_preview_form_section.html" 2>/dev/null
echo -e "\n=== Listing extractions templates directory ==="
ls -la radis/extractions/templates/extractions/ 2>/dev/null | head -20
echo -e "\n=== Finding views.py in extractions ==="
find radis/extractions -name "views.py" -o -name "views" -type d
echo -e "\n=== Searching for extraction_search_preview in Python files ==="
rg -rn "extraction_search_preview" radis/ --type=py
echo -e "\n=== Checking urls.py around line 48 for view reference ==="
sed -n '45,55p' radis/extractions/urls.pyRepository: openradx/radis
Length of output: 2458
🏁 Script executed:
#!/bin/bash
echo "=== Finding ExtractionSearchPreviewView in views.py ==="
rg -n "class ExtractionSearchPreviewView" radis/extractions/views.py -A 15
echo -e "\n=== Checking _search_preview.html to see if it includes the fragment ==="
cat -n radis/extractions/templates/extractions/_search_preview.html
echo -e "\n=== Checking core templates for Alpine.js ==="
cat -n radis/core/templates/core/core_layout.html | head -30
echo -e "\n=== Checking if Alpine.js is included in base.html or similar ==="
find radis -name "base.html" -type f -exec cat -n {} \; | head -50Repository: openradx/radis
Length of output: 3133
🏁 Script executed:
#!/bin/bash
echo "=== Checking common_layout.html for Alpine.js ==="
cat -n radis/templates/common/common_layout.html | head -40
echo -e "\n=== Getting complete ExtractionSearchPreviewView implementation ==="
rg -n "class ExtractionSearchPreviewView" radis/extractions/views.py -A 50 | head -60
echo -e "\n=== Verifying where _search_preview_form_section.html is actually used ==="
rg -rn "_search_preview_form_section" radis/ --type=html --type=py
echo -e "\n=== Checking if Alpine.js is loaded from CDN anywhere ==="
rg -rn "cdn\|unpkg\|jsdelivr" radis/ -i --type=htmlRepository: openradx/radis
Length of output: 2724
🏁 Script executed:
#!/bin/bash
echo "=== Getting complete ExtractionSearchPreviewView - looking for template_name or render ==="
sed -n '284,350p' radis/extractions/views.py
echo -e "\n=== Searching for template_name in ExtractionSearchPreviewView ==="
rg -n "template_name" radis/extractions/views.py
echo -e "\n=== Finding root base template ==="
find radis -name "*.html" -path "*/templates/*" -exec grep -l "<!DOCTYPE\|<html" {} \; | head -5
echo -e "\n=== Checking if any HTML file loads Alpine.js ==="
rg -rn "alpine\.js\|@alpinejs\|x-data" radis/templates radis/extractions/templates --type=html | grep -v "x-data\|x-on" | head -20
echo -e "\n=== Viewing extraction_job_search_form.html to understand context ==="
cat -n radis/extractions/templates/extractions/extraction_job_search_form.htmlRepository: openradx/radis
Length of output: 5122
🏁 Script executed:
#!/bin/bash
echo "=== Checking git history of the fragment ==="
cd radis && git log --oneline radis/extractions/templates/extractions/_search_preview_form_section.html 2>/dev/null | head -5
echo -e "\n=== Verifying extraction_job_search_form.html is actually used ==="
rg -rn "extraction_job_search_form" radis/ --type=py
echo -e "\n=== Check if _search_preview_form_section should be included in _search_preview.html ==="
grep -n "search_preview_form_section\|search-preview" radis/extractions/templates/extractions/_search_preview.html
echo -e "\n=== Check the HTMX target in extraction_job_search_form.html ==="
grep -n "id.*search-preview\|hx-target.*search-preview" radis/extractions/templates/extractions/extraction_job_search_form.htmlRepository: openradx/radis
Length of output: 589
🏁 Script executed:
#!/bin/bash
echo "=== Current working directory ==="
pwd
echo -e "\n=== Verifying file exists ==="
ls -la radis/extractions/templates/extractions/_search_preview_form_section.html
echo -e "\n=== Checking extraction_job_search_form.html for search-preview references ==="
cat radis/extractions/templates/extractions/extraction_job_search_form.html | grep -n "search-preview"
echo -e "\n=== Checking if there's embedded search-preview HTML ==="
cat radis/extractions/templates/extractions/extraction_job_search_form.html
echo -e "\n=== Looking for where _search_preview_form_section might be included dynamically ==="
rg -n "search.preview\|search-preview\|_search_preview" radis/extractions/static/ --type=js 2>/dev/null | head -20Repository: openradx/radis
Length of output: 1704
Alpine.js library is missing from the application. The template uses Alpine.js directives (including x-on:htmx:after-request="isLoading = false" in this fragment), but the Alpine.js library is not loaded anywhere in the template hierarchy. This will cause runtime errors when these directives are evaluated.
Additionally, this fragment appears to be unused—it is not included in any template. The actual search preview response renders _search_preview.html instead. Either integrate this fragment into the search preview flow or remove it as dead code.
Also note that extraction_job_search_form.html uses Alpine.js directives (x-data and @input.debounce) and will have the same Alpine.js loading issue.
| """ | ||
|
|
||
| group: int # TODO: Rename to group_id | ||
| group: int | None = None # TODO: Rename to group_id |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's look at the SearchFilters class definition
cat -n radis/search/site.py | head -60Repository: openradx/radis
Length of output: 2560
🏁 Script executed:
# Search for how group field is used in filtering logic
rg -n "\.group" radis/search/ --type=py -B2 -A2Repository: openradx/radis
Length of output: 390
🏁 Script executed:
# Check for None handling of group in the codebase
rg -n "group.*None\|None.*group" radis/search/ --type=pyRepository: openradx/radis
Length of output: 40
🏁 Script executed:
# Look for SearchFilters instantiation and usage
rg -n "SearchFilters" radis/search/ --type=py | head -20Repository: openradx/radis
Length of output: 299
🏁 Script executed:
# Look at views.py where SearchFilters is instantiated
sed -n '60,80p' radis/search/views.pyRepository: openradx/radis
Length of output: 824
🏁 Script executed:
# Find the search_provider and how it uses group
rg -n "search_provider|def search" radis/search/site.py | head -20Repository: openradx/radis
Length of output: 239
🏁 Script executed:
# Check how filters with group are actually used
rg -n "filters\.group\|if.*group\|where.*group" radis/search/ --type=py -B2 -A2Repository: openradx/radis
Length of output: 40
🏁 Script executed:
# Look for search provider implementations
fd "search.*provider" radis/ --type fRepository: openradx/radis
Length of output: 40
🏁 Script executed:
# Look for SearchProvider class definition
rg -n "class SearchProvider" radis/ --type=py -A10Repository: openradx/radis
Length of output: 681
🏁 Script executed:
# Look for Search class implementation
rg -n "class Search" radis/search/site.py -A20Repository: openradx/radis
Length of output: 2586
🏁 Script executed:
# Check if there are other places where SearchFilters is instantiated
rg -n "SearchFilters(" radis/ --type=py -B2 -A5Repository: openradx/radis
Length of output: 136
🏁 Script executed:
# Look for where group filter might be used in queries
rg -n "group" radis/search/ --type=py | grep -v "test\|active_group\|GroupFactory\|\.add(group)"Repository: openradx/radis
Length of output: 567
🏁 Script executed:
# Look for search provider implementations (not just the abstract definition)
find radis -name "*.py" -type f | xargs grep -l "SearchProvider\|def search" | grep -v test | grep -v __pycache__Repository: openradx/radis
Length of output: 129
🏁 Script executed:
# Check reports app for search implementation
fd "search" radis/reports/ --type fRepository: openradx/radis
Length of output: 40
🏁 Script executed:
# Look for Elasticsearch or database query building with group filter
rg -n "group" radis/reports/ --type=py -A2 -B2 | head -40Repository: openradx/radis
Length of output: 2610
🏁 Script executed:
# Check if there's a search implementation that uses filters
rg -n "filters\." radis/ --type=py | grep -v "test\|SearchFilters" | head -20Repository: openradx/radis
Length of output: 1702
🏁 Script executed:
# Examine the pgsearch provider implementation
cat -n radis/pgsearch/providers.pyRepository: openradx/radis
Length of output: 7150
Add group filtering to _build_filter_query function before making group optional.
The group filter is not applied in radis/pgsearch/providers.py (see _build_filter_query, lines 51-78). While SearchFilters.group is passed from views, it's never used in the actual search query. Making this field optional with a default of None masks a critical permission control gap: users could access reports from groups they don't belong to.
Add a check like:
if filters.group is not None:
fq &= Q(report__groups__pk=filters.group)🤖 Prompt for AI Agents
In @radis/search/site.py at line 48, The _build_filter_query function currently
ignores the SearchFilters.group field, so add a conditional filter to apply
group-based access control: inside _build_filter_query (used by
radis/pgsearch/providers.py) check if filters.group is not None and, if so, AND
a clause filtering reports by group primary key (e.g.,
Q(report__groups__pk=filters.group)); ensure you reference the
SearchFilters.group property and combine with the existing fq variable so
queries respect group membership when group is provided.
| logger.debug( | ||
| f"LLM returned None for question {question.pk} ", | ||
| f"on report {report.pk}", | ||
| ) |
There was a problem hiding this comment.
Bug: Logger arguments not concatenated.
The logger.debug() call passes two separate f-strings as arguments. Only the first string will be logged; the second is silently ignored. This will produce incomplete debug messages.
🐛 Proposed fix
if answer is None:
logger.debug(
- f"LLM returned None for question {question.pk} ",
- f"on report {report.pk}",
+ f"LLM returned None for question {question.pk} on report {report.pk}",
)
is_accepted = False📝 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.
| logger.debug( | |
| f"LLM returned None for question {question.pk} ", | |
| f"on report {report.pk}", | |
| ) | |
| logger.debug( | |
| f"LLM returned None for question {question.pk} on report {report.pk}", | |
| ) |
🤖 Prompt for AI Agents
In @radis/subscriptions/processors.py around lines 81 - 84, The logger.debug
call is passing two separate f-strings so only the first is logged; update the
logger.debug invocation (the call that currently uses logger.debug(f"LLM
returned None for question {question.pk} ", f"on report {report.pk}",)) to pass
a single formatted message — e.g., combine the parts into one f-string or format
string that includes both question.pk and report.pk — so the full message is
emitted.
| <h4>Filter Questions</h4> | ||
| {% with questions=subscription.filter_questions.all %} | ||
| {% if questions %} | ||
| <ul> | ||
| {% for question in questions %} | ||
| <li> | ||
| {{ question.question }} — accepts reports when answer is | ||
| {{ question.get_expected_answer_display|lower }} | ||
| </li> | ||
| {% endfor %} | ||
| </ul> | ||
| {% else %} | ||
| <p class="text-muted mb-0">No filter questions defined.</p> | ||
| {% endif %} | ||
| {% endwith %} |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Search for explicit auto-escape directives or XSS-related changes in templates
rg -n -C3 'autoescape|escape|safe' --type=html radis/subscriptions/templates/Repository: openradx/radis
Length of output: 1968
🏁 Script executed:
#!/bin/bash
# Find the question model definition to understand the question field
find . -type f -name "*.py" -path "*/subscriptions/*" | xargs rg -l "class.*Question" | head -5Repository: openradx/radis
Length of output: 155
🏁 Script executed:
#!/bin/bash
# Check the subscription model to understand filter_questions relationship
find . -type f -name "models.py" -path "*/subscriptions/*" | xargs catRepository: openradx/radis
Length of output: 6028
🏁 Script executed:
#!/bin/bash
# Get the full subscription_detail.html file to see autoescape directives
cat radis/subscriptions/templates/subscriptions/subscription_detail.html | head -150Repository: openradx/radis
Length of output: 5691
🏁 Script executed:
#!/bin/bash
# Search specifically for autoescape directives in subscription_detail.html
rg -n 'autoescape|safe' radis/subscriptions/templates/subscriptions/subscription_detail.htmlRepository: openradx/radis
Length of output: 40
🏁 Script executed:
#!/bin/bash
# Check if there's a |safe filter on question field anywhere
rg -n 'question.*safe|safe.*question' radis/subscriptions/templates/subscriptions/Repository: openradx/radis
Length of output: 40
Add explicit auto-escape directive for user-generated content in Filter Questions section.
Lines 108-109 render question.question (user-generated text field) without explicit auto-escape protection. While Django auto-escapes by default, other templates in this app (e.g., _subscribed_item_preview.html) use explicit {% autoescape on %} directives for similar user-generated content. For consistency and defense-in-depth, wrap this section:
Suggested fix
<h4>Filter Questions</h4>
{% autoescape on %}
{% with questions=subscription.filter_questions.all %}
{% if questions %}
<ul>
{% for question in questions %}
<li>
{{ question.question }} — accepts reports when answer is
{{ question.get_expected_answer_display|lower }}
</li>
{% endfor %}
</ul>
{% else %}
<p class="text-muted mb-0">No filter questions defined.</p>
{% endif %}
{% endwith %}
{% endautoescape %}🤖 Prompt for AI Agents
In @radis/subscriptions/templates/subscriptions/subscription_detail.html around
lines 102 - 116, Wrap the Filter Questions block (the section that iterates
subscription.filter_questions.all and renders question.question and
question.get_expected_answer_display) with an explicit auto-escape directive:
enable autoescaping before the {% with
questions=subscription.filter_questions.all %} block and disable it after the
loop (use {% autoescape on %} ... {% endautoescape %>) so user-generated
question.question is explicitly escaped consistent with other templates.
| assert len(items) == 1 # Should match "12345" | ||
| assert item1 in items | ||
| assert item2 and item3 not in items | ||
|
|
There was a problem hiding this comment.
Incorrect assertion logic - only checks one item.
The expression item2 and item3 not in items evaluates as (item2) and (item3 not in items), which only verifies item3 is not in items (assuming item2 is truthy). Both items should be checked separately.
🐛 Proposed fix
assert len(items) == 1 # Should match "12345"
assert item1 in items
- assert item2 and item3 not in items
+ assert item2 not in items
+ assert item3 not in items🤖 Prompt for AI Agents
In @radis/subscriptions/tests/test_views.py around lines 545 - 548, The
assertion uses `item2 and item3 not in items`, which evaluates incorrectly and
only guarantees `item3` is absent; change to assert both separately by replacing
that expression with explicit checks like `assert item2 not in items` and
`assert item3 not in items` so both `item2` and `item3` are individually
verified not to be in `items` (keep `items`, `item1`, `item2`, `item3`
references intact).
| assert len(items) == 1 | ||
| assert item2 in items | ||
| assert item1 and item3 not in items | ||
|
|
There was a problem hiding this comment.
Same incorrect assertion pattern.
Same issue as above - this only checks item3 not in items.
🐛 Proposed fix
assert len(items) == 1
assert item2 in items
- assert item1 and item3 not in items
+ assert item1 not in items
+ assert item3 not in items📝 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.
| assert len(items) == 1 | |
| assert item2 in items | |
| assert item1 and item3 not in items | |
| assert len(items) == 1 | |
| assert item2 in items | |
| assert item1 not in items | |
| assert item3 not in items |
🤖 Prompt for AI Agents
In @radis/subscriptions/tests/test_views.py around lines 578 - 581, The
assertion "assert item1 and item3 not in items" is logically incorrect and only
checks item3; update the assertion to explicitly assert both are absent, e.g.
replace it with "assert item1 not in items and item3 not in items" or two
separate asserts "assert item1 not in items" and "assert item3 not in items"
referencing the existing variables items, item1, and item3 in the test.
Contains changes from the following branches:
subscription_extract:
subscription_handler:
query_generation:
old-historySelectionOutput:
extractions:
Summary by CodeRabbit
Release Notes
New Features
Improvements
✏️ Tip: You can customize this high-level summary in your review settings.