🔧(backend) settings CONVERSION_UPLOAD_ENABLED to control usage of docspec#2151
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughA new boolean feature flag CONVERSION_UPLOAD_ENABLED (default False, env-configurable) was added. Backend: the setting is exposed via the config endpoint and DocumentViewSet.perform_create now rejects requests that include an uploaded Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes 🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 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. Comment |
![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)
src/backend/core/tests/documents/test_api_documents_create_with_file.py (1)
43-53: Consider centralizing repeated flag setup in a fixture.
settings.CONVERSION_UPLOAD_ENABLED = Trueis repeated across many tests. A local fixture (or autouse fixture scoped to this module) would reduce duplication and keep intent clearer.♻️ Example refactor
+@pytest.fixture +def conversion_upload_enabled(settings): + settings.CONVERSION_UPLOAD_ENABLED = True + return settings + `@patch`("core.services.converter_services.Converter.convert") -def test_api_documents_create_with_docx_file_success(mock_convert, settings): +def test_api_documents_create_with_docx_file_success(mock_convert, conversion_upload_enabled): @@ - settings.CONVERSION_UPLOAD_ENABLED = TrueAlso applies to: 121-130, 163-172, 197-206, 225-234, 257-266, 314-323, 341-352, 384-393, 421-422, 448-449
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/backend/core/tests/documents/test_api_documents_create_with_file.py` around lines 43 - 53, Multiple tests (e.g., test_api_documents_create_with_docx_file_success and others listed) repeatedly set settings.CONVERSION_UPLOAD_ENABLED = True; add a single pytest fixture (e.g., enable_conversion_upload) in this test module (or as an autouse fixture scoped to the module) that sets settings.CONVERSION_UPLOAD_ENABLED = True before yielding and restores the original value afterward, then remove the repeated assignments from the individual tests so they rely on the fixture.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/backend/impress/settings.py`:
- Around line 869-871: The environment variable name passed to
values.BooleanValue for the CONVERSION_UPLOAD_ENABLED setting is incorrect;
update the environ_name argument in the CONVERSION_UPLOAD_ENABLED declaration
(where values.BooleanValue is called) from "CONVERSION_ENABLED" to
"CONVERSION_UPLOAD_ENABLED" so the environment variable matches the
public-facing setting name used across the codebase and CHANGELOG (leave
environ_prefix=None as-is).
---
Nitpick comments:
In `@src/backend/core/tests/documents/test_api_documents_create_with_file.py`:
- Around line 43-53: Multiple tests (e.g.,
test_api_documents_create_with_docx_file_success and others listed) repeatedly
set settings.CONVERSION_UPLOAD_ENABLED = True; add a single pytest fixture
(e.g., enable_conversion_upload) in this test module (or as an autouse fixture
scoped to the module) that sets settings.CONVERSION_UPLOAD_ENABLED = True before
yielding and restores the original value afterward, then remove the repeated
assignments from the individual tests so they rely on the fixture.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 908db155-636c-4ffc-ad04-3c30eef1cf13
📒 Files selected for processing (6)
CHANGELOG.mdsrc/backend/core/api/viewsets.pysrc/backend/core/tests/documents/test_api_documents_create_with_file.pysrc/backend/core/tests/external_api/test_external_api_documents.pysrc/backend/core/tests/test_api_config.pysrc/backend/impress/settings.py
|
Size Change: +1 B (0%) Total Size: 4.25 MB 📦 View Changed
|
|
@CodeRabbit review |
✅ Actions performedReview triggered.
|
lunika
force-pushed
the
docspec-feature-flag
branch
from
599e2cd to
788d594
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/backend/core/tests/documents/test_api_documents_create_with_file.py`:
- Around line 86-118: In test_api_documents_create_with_docx_file_disabled, add
an assertion that no Document row is persisted when uploads are disabled: record
the initial Document.objects.count() (or use Document.objects.filter(...) for a
unique attribute) before the POST and assert the count is unchanged after the
response (or that no Document matching the uploaded file name exists); place
this check after the response assertions and before
mock_convert.assert_not_called() to ensure no partial write occurred. Use the
Document model (Document) and the test function name
test_api_documents_create_with_docx_file_disabled to locate where to add the
assertion.
🪄 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: Repository UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: ef983b00-58d8-468f-bb6f-06eb3444c996
📒 Files selected for processing (12)
CHANGELOG.mdenv.d/development/commonsrc/backend/core/api/viewsets.pysrc/backend/core/tests/documents/test_api_documents_create_with_file.pysrc/backend/core/tests/external_api/test_external_api_documents.pysrc/backend/core/tests/test_api_config.pysrc/backend/impress/settings.pysrc/frontend/apps/e2e/__tests__/app-impress/doc-import.spec.tssrc/frontend/apps/e2e/__tests__/app-impress/utils-common.tssrc/frontend/apps/impress/src/core/config/api/useConfig.tsxsrc/frontend/apps/impress/src/features/docs/docs-grid/components/DocsGrid.tsxsrc/frontend/apps/impress/src/features/docs/docs-grid/hooks/useImport.tsx
jmaupetit
left a comment
jmaupetit
left a comment
There was a problem hiding this comment.
I only have minor suggestions. Take it or leave it.
We want to control the conversion of document at upload time. We want to disable this feature using a settings. The new settings CONVERSION_UPLOAD_ENABLED should be used to enable or not the conversion at upload feature. If disabled and a file is uploaded, the reponse will return a 400
The frontend application needs to know the value of the settings CONVERSION_UPLOAD_ENABLED to allow the file upload or not.
We want to be able to enable/disable the document import feature for testing and gradual rollout purposes. This commit adds a feature flag for document import and updates the relevant components and tests to respect this flag.
lunika
force-pushed
the
docspec-feature-flag
branch
from
788d594 to
f166e75
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
src/backend/core/tests/documents/test_api_documents_create_with_file.py (1)
86-114:⚠️ Potential issue | 🟡 MinorAdd assertion to verify no document was persisted.
The test should verify that no
Documentis created when upload is rejected. This guards against partial writes before validation and ensures data integrity.Suggested fix
assert response.status_code == 400 assert response.json() == {"file": ["file upload is not allowed"]} + assert not Document.objects.exists() # Verify the converter was not called mock_convert.assert_not_called()🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/backend/core/tests/documents/test_api_documents_create_with_file.py` around lines 86 - 114, The test test_api_documents_create_with_docx_file_disabled should also assert that no Document was persisted when upload is rejected; after the response and before mock_convert.assert_not_called(), query the Document model (e.g., Document.objects.count() == 0 or assert not Document.objects.exists()) to ensure no partial write occurred; update the test to import the Document model if necessary and add a single assertion verifying zero persisted Document instances.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/frontend/apps/impress/src/features/docs/docs-grid/hooks/useImport.tsx`:
- Around line 115-121: Replace the fallback operator used when deriving the
exported isEnabled flag to make the intent explicit: change the expression that
returns isEnabled (currently using config?.CONVERSION_UPLOAD_ENABLED || false)
to use nullish coalescing so it only falls back for null/undefined (i.e., use
config?.CONVERSION_UPLOAD_ENABLED ?? false); update the return object where
getRootProps, getInputProps, open, and isEnabled are returned.
---
Duplicate comments:
In `@src/backend/core/tests/documents/test_api_documents_create_with_file.py`:
- Around line 86-114: The test test_api_documents_create_with_docx_file_disabled
should also assert that no Document was persisted when upload is rejected; after
the response and before mock_convert.assert_not_called(), query the Document
model (e.g., Document.objects.count() == 0 or assert not
Document.objects.exists()) to ensure no partial write occurred; update the test
to import the Document model if necessary and add a single assertion verifying
zero persisted Document instances.
🪄 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: Repository UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 977738ff-8b0d-4465-bd2b-cf1a48b7bc26
📒 Files selected for processing (12)
CHANGELOG.mdenv.d/development/commonsrc/backend/core/api/viewsets.pysrc/backend/core/tests/documents/test_api_documents_create_with_file.pysrc/backend/core/tests/external_api/test_external_api_documents.pysrc/backend/core/tests/test_api_config.pysrc/backend/impress/settings.pysrc/frontend/apps/e2e/__tests__/app-impress/doc-import.spec.tssrc/frontend/apps/e2e/__tests__/app-impress/utils-common.tssrc/frontend/apps/impress/src/core/config/api/useConfig.tsxsrc/frontend/apps/impress/src/features/docs/docs-grid/components/DocsGrid.tsxsrc/frontend/apps/impress/src/features/docs/docs-grid/hooks/useImport.tsx
| return { | ||
| getRootProps, | ||
| getInputProps, | ||
| open, | ||
| isEnabled: config?.CONVERSION_UPLOAD_ENABLED || false, | ||
| isPending, | ||
| }; |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
LGTM! Feature flag properly exposed from hook.
The hook correctly derives isEnabled from the config and returns it for consumers to gate UI behavior.
💡 Optional: Consider nullish coalescing for clearer intent
- isEnabled: config?.CONVERSION_UPLOAD_ENABLED || false,
+ isEnabled: config?.CONVERSION_UPLOAD_ENABLED ?? false,Using ?? instead of || makes it explicit that you're only providing a fallback for null/undefined, not for other falsy values. Since this is a boolean, both operators behave identically, but ?? better communicates the intent.
📝 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.
| return { | |
| getRootProps, | |
| getInputProps, | |
| open, | |
| isEnabled: config?.CONVERSION_UPLOAD_ENABLED || false, | |
| isPending, | |
| }; | |
| return { | |
| getRootProps, | |
| getInputProps, | |
| open, | |
| isEnabled: config?.CONVERSION_UPLOAD_ENABLED ?? false, | |
| isPending, | |
| }; |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/frontend/apps/impress/src/features/docs/docs-grid/hooks/useImport.tsx`
around lines 115 - 121, Replace the fallback operator used when deriving the
exported isEnabled flag to make the intent explicit: change the expression that
returns isEnabled (currently using config?.CONVERSION_UPLOAD_ENABLED || false)
to use nullish coalescing so it only falls back for null/undefined (i.e., use
config?.CONVERSION_UPLOAD_ENABLED ?? false); update the return object where
getRootProps, getInputProps, open, and isEnabled are returned.
3 participants
Purpose
We want to control the conversion of document at upload time. We want to
disable this feature using a settings. The new settings
CONVERSION_UPLOAD_ENABLED should be used to enable or not the conversion
at upload feature. If disabled and a file is uploaded, the reponse will
return a 400
Proposal