Skip to content

Conversation

@stktyagi
Copy link
Member

@stktyagi stktyagi commented Jun 9, 2025

Checklist

  • I have read the OpenWISP Contributing Guidelines.
  • I have manually tested the changes proposed in this pull request.
  • I have written new test cases for new code and/or updated existing tests for changes to existing code.
  • I have updated the documentation.

Reference to Existing Issue

Closes #373.

Description of Changes

Integrated PrivateStorageDetailView within a DRF APIView and applied @extend_schema to enable proper Swagger documentation. This update ensures the CSV download endpoint is both functional and visible in the Swagger UI. Verified the endpoint works correctly and appears as expected in the API docs.

Screenshot

image image

Summary by CodeRabbit

  • Documentation

    • Updated REST API docs for batch CSV download: clarified path, added descriptive text and a concrete curl example for downloading import CSVs.
  • API Improvements

    • Added a documented GET API endpoint for batch CSV downloads with OpenAPI/Swagger metadata to improve discoverability and usage examples.

✏️ Tip: You can customize this high-level summary in your review settings.

stktyagi added 2 commits June 8, 2025 20:21
Wrapped PrivateStorageDetailView with DRF APIView and added @extend_schema
metadata for Swagger documentation generation. This ensures the CSV download endpoint is now visible and documented properly in the Swagger UI.

Fixes openwisp#373
Tested visibility of csv batch download endpoint on swagger and working of the endpoint via admin dashboad and curl.

Fixes openwisp#373
Added drf-spectacular in requirements-test.txt tp prevent build failure.

Fixes openwisp#373
@coveralls
Copy link

coveralls commented Jul 1, 2025

Coverage Status

coverage: 97.252% (-0.02%) from 97.269%
when pulling 156531d on stktyagi:master
into 4053a46 on openwisp:master.

Copy link
Member

@nemesifier nemesifier left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks good but I think we need to add the new API URL to the docs (in the REST API page). Could you share a screenshot of the API docs section for this URL in the PR description?

Documented the GET request endpoint for Batch CSV Download.

Fixes openwisp#373
@stktyagi
Copy link
Member Author

@nemesifier
image

image

Copy link
Member

@nemesifier nemesifier left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for this @stktyagi, it looks good but needs improvement, please see my comments below.

Removed filename from URL and view arguments and updated documentation descriptions.

Fixes openwisp#373
@coderabbitai
Copy link

coderabbitai bot commented Dec 31, 2025

📝 Walkthrough

Walkthrough

Added a documented API endpoint for downloading the batch CSV used to import users: docs updated to show the new GET curl example and route; URL routing now exposes a consistent path under the API; a DRF APIView wrapper with Swagger/OpenAPI annotations was added to surface the endpoint in API docs.

Changes

Cohort / File(s) Summary
Documentation
docs/user/rest-api.rst
Replaced parameter table with a curl shell example; changed endpoint path from /csv/<filename> to /csv/; clarified GET behavior and added example usage.
Routing
openwisp_radius/private_storage/urls.py
Inserted new URL pattern for /api/v1/radius/organization/<slug:slug>/batch/<uuid:pk>/csv/ (named radius_organization_batch_csv_read) before existing private-file serve route.
API View / Docs
openwisp_radius/private_storage/views.py
Added RadiusBatchCsvDownloadAPIView (APIView) with @swagger_auto_schema documentation that delegates GET to the existing batch CSV download view; exported rad_batch_csv_download_api_view; added drf_yasg and DRF imports and translation alias.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Poem

🐇 A tiny rabbit found a route so neat,
CSVs now hop from storage to your seat,
Docs sing curl commands, the API's in view,
Download and dance — a small change, bright and new! 📥✨

Pre-merge checks and finishing touches

❌ Failed checks (1 warning)
Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. You can run @coderabbitai generate docstrings to improve docstring coverage.
✅ Passed checks (4 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly and specifically describes the main change: adding Swagger visibility for the CSV batch download endpoint.
Description check ✅ Passed The description covers the main approach, includes issue reference, manual testing confirmation, and includes supporting screenshots showing the endpoint in Swagger UI.
Linked Issues check ✅ Passed The PR addresses all objectives from issue #373: normalizes the CSV download URL path to /api/v1/radius/organization//batch//csv/ [#373], integrates it into a DRF APIView with @extend_schema decorator [#373], and ensures it appears in Swagger documentation [#373].
Out of Scope Changes check ✅ Passed All code changes are focused on implementing the linked issue objectives: URL normalization, APIView wrapper, and Swagger documentation integration without unrelated modifications.
✨ Finishing touches
  • 📝 Generate docstrings

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 2

🧹 Nitpick comments (1)
openwisp_radius/private_storage/views.py (1)

42-42: Consider prefixing unused parameters with underscore.

The slug and args parameters are captured from the URL pattern but not used in the delegation to the underlying view. While this is acceptable, consider prefixing them with underscores to explicitly indicate they're intentionally unused:

🔎 Suggested refactor
-    def get(self, request, slug, pk, *args, **kwargs):
+    def get(self, request, _slug, pk, *_args, **kwargs):
         return rad_batch_csv_download_view(request, pk=pk, **kwargs)

This follows the Python convention for intentionally unused parameters and will silence the static analysis warnings.

📜 Review details

Configuration used: defaults

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 4053a46 and 052896a.

📒 Files selected for processing (3)
  • docs/user/rest-api.rst
  • openwisp_radius/private_storage/urls.py
  • openwisp_radius/private_storage/views.py
🧰 Additional context used
🪛 Gitleaks (8.30.0)
docs/user/rest-api.rst

[high] 871-873: Discovered a potential authorization token provided in a curl command header, which could compromise the curl accessed resource.

(curl-auth-header)

🪛 Ruff (0.14.10)
openwisp_radius/private_storage/views.py

42-42: Unused method argument: slug

(ARG002)


42-42: Unused method argument: args

(ARG002)

⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (11)
  • GitHub Check: Python==3.13 | django~=5.2.0
  • GitHub Check: Python==3.12 | django~=5.2.0
  • GitHub Check: Python==3.11 | django~=5.2.0
  • GitHub Check: Python==3.12 | django~=5.1.0
  • GitHub Check: Python==3.10 | django~=5.1.0
  • GitHub Check: Python==3.12 | django~=4.2.0
  • GitHub Check: Python==3.13 | django~=5.1.0
  • GitHub Check: Python==3.10 | django~=5.2.0
  • GitHub Check: Python==3.11 | django~=5.1.0
  • GitHub Check: Python==3.10 | django~=4.2.0
  • GitHub Check: Python==3.11 | django~=4.2.0
🔇 Additional comments (4)
openwisp_radius/private_storage/urls.py (1)

11-18: LGTM! URL pattern aligns with API consistency goals.

The new URL pattern correctly structures the CSV download endpoint to match the rest of the API (/api/v1/radius/organization/<slug>/batch/<pk>/csv/) and routes to the documented API view. The naming is consistent between the URL pattern name and the Swagger operation_id.

openwisp_radius/private_storage/views.py (2)

1-5: LGTM! Imports are appropriate for Swagger documentation.

The added imports support the Swagger/OpenAPI documentation functionality using drf_yasg, which aligns with the project's existing documentation approach.


46-46: LGTM! Module export follows Django conventions.

The view export correctly creates a callable view function from the class-based view, which is properly referenced in the URL configuration.

docs/user/rest-api.rst (1)

864-873: LGTM! Documentation accurately reflects the new endpoint.

The documentation updates correctly describe the CSV download endpoint with:

  • Accurate endpoint path matching the new URL pattern
  • Clear GET-only specification
  • Helpful curl example showing proper authentication

Note: The static analysis warning about the auth token in the curl example is a false positive—this is intentional example documentation with a placeholder value.

Added minor tweaks in views.py and formatted docs.

Fixes openwisp#373
Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 0

🧹 Nitpick comments (1)
openwisp_radius/private_storage/views.py (1)

43-44: *Consider prefixing unused args with underscore.

The delegation logic is correct. The _slug parameter is properly prefixed since it's intentionally unused. Similarly, the *args parameter could follow Python conventions by using *_args to indicate it's intentionally unused, though this is a minor style consideration.

🔎 Optional style improvement
-    def get(self, request, _slug, pk, *args, **kwargs):
+    def get(self, request, _slug, pk, *_args, **kwargs):
         return rad_batch_csv_download_view(request, pk=pk, **kwargs)
📜 Review details

Configuration used: defaults

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 052896a and 156531d.

📒 Files selected for processing (2)
  • docs/user/rest-api.rst
  • openwisp_radius/private_storage/views.py
🚧 Files skipped from review as they are similar to previous changes (1)
  • docs/user/rest-api.rst
🧰 Additional context used
🪛 Ruff (0.14.10)
openwisp_radius/private_storage/views.py

43-43: Unused method argument: args

(ARG002)

⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (11)
  • GitHub Check: Python==3.10 | django~=4.2.0
  • GitHub Check: Python==3.12 | django~=5.1.0
  • GitHub Check: Python==3.12 | django~=5.2.0
  • GitHub Check: Python==3.11 | django~=5.1.0
  • GitHub Check: Python==3.10 | django~=5.1.0
  • GitHub Check: Python==3.11 | django~=4.2.0
  • GitHub Check: Python==3.13 | django~=5.1.0
  • GitHub Check: Python==3.11 | django~=5.2.0
  • GitHub Check: Python==3.13 | django~=5.2.0
  • GitHub Check: Python==3.12 | django~=4.2.0
  • GitHub Check: Python==3.10 | django~=5.2.0
🔇 Additional comments (4)
openwisp_radius/private_storage/views.py (4)

1-5: LGTM! Imports align with the implementation approach.

The imports are appropriate for adding Swagger documentation using drf-yasg, which aligns with the existing openwisp-controller implementation as discussed in previous reviews.


17-17: Good addition - makes pk-based lookup explicit.

Adding pk_url_kwarg = "pk" addresses the previous review feedback and clarifies that the view supports pk-based lookups when called from the API wrapper.


29-42: Excellent Swagger configuration.

The decorator is properly configured with appropriate operation metadata, file response schema, and the operation description spacing issue from the previous review has been fixed.


47-47: LGTM! View reference follows standard Django pattern.

The view reference is properly created and will be used for URL routing, following the same pattern as the existing rad_batch_csv_download_view.

Copy link
Member

@nemesifier nemesifier left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, I will do another round of testing once the CI build passes and if I don't see anything wrong I will merge, thanks 🙏

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Status: In progress

Development

Successfully merging this pull request may close these issues.

[change] Download batch CSV URL is inconsistent and not showing up in API docs

4 participants