Conversation
📝 WalkthroughWalkthroughThe changes implement automatic server-side determination of the forecast Changes
Sequence DiagramsequenceDiagram
participant Client as Client/Frontend
participant View as Forecast View
participant RequestUtil as is_internal_request
participant Serializer as ForecastWriteSerializer
participant DB as Database
Client->>View: POST forecast (no source field)
View->>RequestUtil: Check if request is internal
RequestUtil->>RequestUtil: Inspect auth type & host
RequestUtil-->>View: Return true/false
alt Internal Request
View->>View: Set source = "UI"
else External Request
View->>View: Set source = "API"
end
View->>View: Inject source into payload
View->>Serializer: Validate forecast data
Serializer-->>View: Valid (source auto-set)
View->>DB: Create forecast with source
DB-->>View: Success
View-->>Client: Forecast created
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Review rate limit: 0/1 reviews remaining, refill in 60 minutes.Comment |
🚀 Preview EnvironmentYour preview environment is ready!
Details
ℹ️ Preview Environment InfoIsolation:
Limitations:
Cleanup:
|
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
tests/unit/test_utils/test_requests.py (1)
14-45: Good test coverage, consider adding a case for uppercaseAPP_DOMAIN.The tests comprehensively cover the main scenarios. However, there's no test verifying behavior when
APP_DOMAINitself contains uppercase characters (e.g.,settings.APP_DOMAIN = "Metaculus.com"withhost = "metaculus.com"). This would help catch the normalization issue in the implementation.💡 Suggested additional test case
def test_uppercase_app_domain_still_matches_lowercase_host(self, settings): settings.APP_DOMAIN = "Metaculus.com" request = make_request("metaculus.com") assert is_internal_request(request) is False🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/unit/test_utils/test_requests.py` around lines 14 - 45, Add a unit test to TestIsInternalRequest that verifies APP_DOMAIN normalization by setting settings.APP_DOMAIN = "Metaculus.com" (mixed case), creating a request with host "metaculus.com" via make_request, and asserting is_internal_request(request) is False; name it e.g. test_uppercase_app_domain_still_matches_lowercase_host and place it alongside the other tests to ensure the APP_DOMAIN is compared case-insensitively.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@utils/requests.py`:
- Around line 31-33: The comparison uses a lowercased local host but compares it
directly to settings.APP_DOMAIN which may have different casing; update the
check in the function that computes host to normalize settings.APP_DOMAIN too
(e.g., compare host to settings.APP_DOMAIN.lower().strip() or a cached
normalized APP_DOMAIN) so both sides use the same casing/whitespace
normalization before returning host != settings.APP_DOMAIN; reference the local
variable host and settings.APP_DOMAIN when making this change.
---
Nitpick comments:
In `@tests/unit/test_utils/test_requests.py`:
- Around line 14-45: Add a unit test to TestIsInternalRequest that verifies
APP_DOMAIN normalization by setting settings.APP_DOMAIN = "Metaculus.com" (mixed
case), creating a request with host "metaculus.com" via make_request, and
asserting is_internal_request(request) is False; name it e.g.
test_uppercase_app_domain_still_matches_lowercase_host and place it alongside
the other tests to ensure the APP_DOMAIN is compared case-insensitively.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: fca477ca-b9dd-4b99-8739-330cc3cf1a51
📒 Files selected for processing (6)
front_end/src/services/api/questions/questions.server.tsquestions/serializers/common.pyquestions/views.pytests/unit/test_questions/test_views.pytests/unit/test_utils/test_requests.pyutils/requests.py
💤 Files with no reviewable changes (2)
- front_end/src/services/api/questions/questions.server.ts
- questions/serializers/common.py
| host = (request.get_host() or "").split(":")[0].lower() | ||
|
|
||
| return host != settings.APP_DOMAIN |
There was a problem hiding this comment.
Missing normalization for settings.APP_DOMAIN.
The host variable is lowercased, but settings.APP_DOMAIN is compared directly. If APP_DOMAIN is configured with different casing (e.g., "Metaculus.com"), the comparison will incorrectly classify the request as internal.
🛠️ Proposed fix
host = (request.get_host() or "").split(":")[0].lower()
+ app_domain = (settings.APP_DOMAIN or "").lower()
- return host != settings.APP_DOMAIN
+ return host != app_domain📝 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.
| host = (request.get_host() or "").split(":")[0].lower() | |
| return host != settings.APP_DOMAIN | |
| host = (request.get_host() or "").split(":")[0].lower() | |
| app_domain = (settings.APP_DOMAIN or "").lower() | |
| return host != app_domain |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@utils/requests.py` around lines 31 - 33, The comparison uses a lowercased
local host but compares it directly to settings.APP_DOMAIN which may have
different casing; update the check in the function that computes host to
normalize settings.APP_DOMAIN too (e.g., compare host to
settings.APP_DOMAIN.lower().strip() or a cached normalized APP_DOMAIN) so both
sides use the same casing/whitespace normalization before returning host !=
settings.APP_DOMAIN; reference the local variable host and settings.APP_DOMAIN
when making this change.
1 participant
Summary by CodeRabbit
Improvements
API Changes
sourcefield is no longer accepted in forecast submission requests; the system handles this automatically.