[fix] remove ValidatedModelSerializer workaround from OrganizationGeo #1371
[fix] remove ValidatedModelSerializer workaround from OrganizationGeo #1371BHARATH0153 wants to merge 27 commits into
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:
📝 WalkthroughWalkthroughRemoves the OrganizationGeoSettingsSerializer.validate() override and the unused django.db.models import. Updates the GitHub Actions CI workflow to pip-install a specific openwisp-utils GitHub tarball (using --no-deps) during dependency installation; requirements-test.txt and requirements.txt are updated to reference the fork/branch tarball. Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 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 |
Code Review SummaryStatus: No New Issues Found | Recommendation: Address existing comments Overview
Incremental Changes Reviewed (1f42810 → c518578)Submodule Removal (
Explanation: The submodule was likely added accidentally or for testing purposes. Its removal cleans up the repository structure. Previous Issues Status
Files Reviewed (1 file - incremental)
Previous Review Comments (carry forward)
Reviewed by kimi-k2.5 · 26,720 tokens |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@requirements.txt`:
- Line 9: Replace the mutable fork URL for openwisp-utils in requirements.txt
(the line containing "openwisp-utils[celery,channels] @
https://github.com/BHARATH0153/openwisp-utils/...fix/validated-model-serializer-update-validation.tar.gz")
with a URL pinned to the upstream repository at the immutable commit SHA that
contains the merged validation fix (use the upstream openwisp-utils archive URL
with the specific commit hash); do the same change in requirements-test.txt
where the same package is referenced so both runtime and test dependencies point
to the upstream immutable commit.
🪄 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: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: f458f8d0-6cb4-4b35-9818-facbb7996a5e
📒 Files selected for processing (3)
openwisp_controller/geo/api/serializers.pyrequirements-test.txtrequirements.txt
📜 Review details
⏰ 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). (12)
- GitHub Check: Python==3.13 | django~=5.2.0
- GitHub Check: Python==3.11 | django~=4.2.0
- GitHub Check: Python==3.11 | django~=5.2.0
- GitHub Check: Python==3.10 | django~=5.1.0
- GitHub Check: Python==3.12 | django~=5.1.0
- GitHub Check: Python==3.13 | django~=5.1.0
- GitHub Check: Python==3.12 | django~=4.2.0
- GitHub Check: Python==3.12 | django~=5.2.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: Kilo Code Review
🧰 Additional context used
📓 Path-based instructions (4)
**/*.{js,ts,tsx,jsx,py,java,go,cs,rb,php,c,cpp,h,hpp}
📄 CodeRabbit inference engine (Custom checks)
**/*.{js,ts,tsx,jsx,py,java,go,cs,rb,php,c,cpp,h,hpp}: Flag potential security vulnerabilities in code
Avoid unnecessary comments or docstrings for code that is already clear
Code formatting is compact and readable. Do not add excessive blank lines, especially inside function or method bodies
Flag unused or redundant code
Ensure variables, functions, classes, and files have descriptive and consistent names
New code must handle errors properly: log errors that cannot be resolved by the user with error level, log unusual conditions with warning level, log important background actions with info level, and provide user-facing messages for errors that the user can solve autonomously
Files:
openwisp_controller/geo/api/serializers.py
**/*.{js,ts,tsx,jsx,py,java,go,cs,rb,php,c,cpp,h,hpp,sql}
📄 CodeRabbit inference engine (Custom checks)
Flag obvious performance regressions, such as heavy loops, repeated I/O, or unoptimized queries
Files:
openwisp_controller/geo/api/serializers.py
**/*.{js,ts,tsx,jsx,py,java,go,cs,rb,php,c,cpp,h,hpp,sh,bash,sql}
📄 CodeRabbit inference engine (Custom checks)
Cryptic or non-obvious code (regex, complex bash commands, or hard-to-read code) must include a concise comment explaining why it is needed and why the complexity is acceptable
Files:
openwisp_controller/geo/api/serializers.py
**/*.{py,html}
📄 CodeRabbit inference engine (Custom checks)
For Django pull requests, ensure all user-facing strings are marked as translatable using the Django i18n framework
Files:
openwisp_controller/geo/api/serializers.py
🧠 Learnings (4)
📚 Learning: 2026-02-21T20:25:01.242Z
Learnt from: nemesifier
Repo: openwisp/openwisp-controller PR: 1175
File: requirements.txt:20-20
Timestamp: 2026-02-21T20:25:01.242Z
Learning: In the openwisp/openwisp-controller repository, do not use the '~=' compatible release specifier in requirements.txt. Use a range like '>=X.Y.Z,<MAJOR+1.0.0' (e.g., 'geoip2>=5.1.0,<6.0.0'). This reduces Dependabot PRs and provides clearer upper-bound control. Apply this rule to all requirements.txt files in the repository.
Applied to files:
requirements.txt
📚 Learning: 2026-01-15T15:05:49.557Z
Learnt from: DragnEmperor
Repo: openwisp/openwisp-controller PR: 1175
File: openwisp_controller/config/management/commands/clear_last_ip.py:38-42
Timestamp: 2026-01-15T15:05:49.557Z
Learning: In Django projects, when using select_related() to traverse relations (for example, select_related("organization__config_settings")), the traversed relation must not be deferred. If you also use .only() in the same query, include the relation name or FK field (e.g., "organization" or "organization_id") in the .only() list to avoid the error "Field X cannot be both deferred and traversed using select_related at the same time." Apply this guideline to Django code in openwisp_controller/config/management/commands/clear_last_ip.py and similar modules by ensuring any select_related with an accompanying only() includes the related field names to prevent deferred/traversed conflicts.
Applied to files:
openwisp_controller/geo/api/serializers.py
📚 Learning: 2026-02-17T19:13:10.088Z
Learnt from: nemesifier
Repo: openwisp/openwisp-controller PR: 1175
File: openwisp_controller/config/whois/commands.py:0-0
Timestamp: 2026-02-17T19:13:10.088Z
Learning: In reviews for the openwisp/openwisp-controller repository, do not propose changes based on Ruff warnings. The project does not use Ruff as its linter; ignore Ruff-related suggestions and follow the repository’s established linting and configuration rules. This guidance applies to all Python files under the openwisp_controller directory.
Applied to files:
openwisp_controller/geo/api/serializers.py
📚 Learning: 2026-01-15T15:07:17.354Z
Learnt from: DragnEmperor
Repo: openwisp/openwisp-controller PR: 1175
File: openwisp_controller/geo/estimated_location/tests/tests.py:172-175
Timestamp: 2026-01-15T15:07:17.354Z
Learning: In this repository, flake8 enforces E501 (line too long) via setup.cfg (max-line-length = 88) while ruff ignores E501 via ruff.toml. Therefore, use '# noqa: E501' on lines that intentionally exceed 88 characters to satisfy flake8 without affecting ruff checks. This applies to Python files across the project (any .py) and is relevant for tests as well. Use sparingly and only where breaking lines is not feasible without hurting readability or functionality.
Applied to files:
openwisp_controller/geo/api/serializers.py
🔇 Additional comments (2)
requirements-test.txt (1)
3-3: No additional comment in this hunk.openwisp_controller/geo/api/serializers.py (1)
5-5: LGTM!
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
.github/workflows/ci.yml (1)
2-4:⚠️ Potential issue | 🟠 Major | ⚡ Quick winAdd explicit least-privilege
permissionsto the workflow.This workflow currently uses default token scopes, which are broader and repo-setting dependent. Define explicit permissions at workflow level (and elevate per job only if required).
Suggested change
name: OpenWISP Controller CI Build +permissions: + contents: readAlso applies to: 16-17, 107-109
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.github/workflows/ci.yml around lines 2 - 4, Add explicit least-privilege GitHub Actions permissions at the workflow level by adding a top-level permissions mapping (e.g., permissions: contents: read, actions: read, id-token: write, pull-requests: write only if needed) to the "OpenWISP Controller CI Build" workflow instead of relying on default token scopes; then review jobs that need elevated rights and override permissions per-job only where absolutely required (refer to the top-level permissions key and any jobs that currently assume broader scopes around lines 16-17 and 107-109).
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.github/workflows/ci.yml:
- Line 76: Replace the pip install line that currently references a forked
branch URL with a pinned immutable reference to the official
openwisp/openwisp-utils repository (use a commit SHA or an official release tag
instead of the personal fork and branch); update the pip invocation string used
to install openwisp-utils so it points to
https://github.com/openwisp/openwisp-utils/archive/refs/heads/<SHA-or-tag>.tar.gz
(or a release tag) to ensure CI reproducibility and remove the forked URL.
---
Outside diff comments:
In @.github/workflows/ci.yml:
- Around line 2-4: Add explicit least-privilege GitHub Actions permissions at
the workflow level by adding a top-level permissions mapping (e.g., permissions:
contents: read, actions: read, id-token: write, pull-requests: write only if
needed) to the "OpenWISP Controller CI Build" workflow instead of relying on
default token scopes; then review jobs that need elevated rights and override
permissions per-job only where absolutely required (refer to the top-level
permissions key and any jobs that currently assume broader scopes around lines
16-17 and 107-109).
🪄 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: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: ba565cc9-edc9-4271-81c1-b4a5202facc1
📒 Files selected for processing (1)
.github/workflows/ci.yml
📜 Review details
⏰ 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). (12)
- GitHub Check: Python==3.12 | django~=5.1.0
- GitHub Check: Python==3.13 | django~=5.1.0
- GitHub Check: Python==3.12 | django~=4.2.0
- GitHub Check: Python==3.10 | django~=5.2.0
- GitHub Check: Python==3.13 | django~=5.2.0
- GitHub Check: Python==3.11 | django~=4.2.0
- GitHub Check: Python==3.12 | django~=5.2.0
- GitHub Check: Python==3.10 | django~=4.2.0
- GitHub Check: Python==3.10 | django~=5.1.0
- GitHub Check: Python==3.11 | django~=5.1.0
- GitHub Check: Python==3.11 | django~=5.2.0
- GitHub Check: Kilo Code Review
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2026-02-24T16:24:55.443Z
Learnt from: nemesifier
Repo: openwisp/openwisp-controller PR: 1233
File: .github/workflows/backport.yml:22-22
Timestamp: 2026-02-24T16:24:55.443Z
Learning: In repositories within the OpenWISP organization, it is acceptable to reference reusable workflows from other OpenWISP-controlled repos using mutable refs (e.g., master) in .github/workflows. This is permissible due to the shared trust boundary within the organization. If applying this pattern, ensure the target repos are under the same organization and maintain awareness of potential breakages from upstream mutable refs; consider pinning to a tagged version for longer-term stability when appropriate.
Applied to files:
.github/workflows/ci.yml
📚 Learning: 2026-02-24T16:25:20.080Z
Learnt from: nemesifier
Repo: openwisp/openwisp-controller PR: 1233
File: .github/workflows/backport.yml:35-35
Timestamp: 2026-02-24T16:25:20.080Z
Learning: In .github/workflows/backport.yml, enforce that backport-on-comment triggers only for users with author_association MEMBE R or OWNER (COLLABORATOR excluded), reflecting maintainer feedback. Update the trigger condition to check author_association and restrict to MEMBERS/OWNERS; document rationale and PR `#1233` reference in code comments.
Applied to files:
.github/workflows/ci.yml
🪛 zizmor (1.25.2)
.github/workflows/ci.yml
[warning] 2-116: overly broad permissions (excessive-permissions): default permissions used due to no permissions: block
(excessive-permissions)
[warning] 107-116: overly broad permissions (excessive-permissions): default permissions used due to no permissions: block
(excessive-permissions)
🔇 Additional comments (1)
.github/workflows/ci.yml (1)
116-116: LGTM!
b79b71b to
5b6a079
Compare
5b6a079 to
e809208
Compare
…SettingsSerializer openwisp#633 Fixes openwisp#633
e809208 to
93016c5
Compare
Replace the mutable branch ref in ci.yml with a pinned commit SHA to ensure reproducible CI builds. Related to openwisp/openwisp-utils#679
64aaaf9 to
99ed828
Compare
Replace the mutable branch ref in ci.yml with a pinned commit SHA to ensure reproducible CI builds. Related to openwisp#1371 Related to openwisp/openwisp-utils#679
99ed828 to
ec9d460
Compare
|
The CI is failing due to transient infrastructure issues (not related to your code). I have restarted the failed jobs automatically (1/3). |
4473a92 to
f3a1a0d
Compare
Replace mutable branch ref in ci.yml with a pinned commit SHA and remove --no-deps flag to ensure all dependencies are properly installed including django-loci. Related to openwisp#1371 Related to openwisp/openwisp-utils#679
cb3f31d to
ee3aa4f
Compare
|
The CI is failing due to transient infrastructure issues (not related to your code). I have restarted the failed jobs automatically (1/3). |
Replace mutable branch ref in ci.yml with a pinned commit SHA and remove --no-deps flag to ensure all dependencies are properly installed including django-loci. Related to openwisp#1371 Related to openwisp/openwisp-utils#679
ee3aa4f to
a9e7516
Compare
Update requirements.txt, requirements-test.txt and ci.yml to use the fixed openwisp-utils fork branch instead of the upstream 1.3 branch, ensuring the ValidatedModelSerializer fix is used consistently across all install steps. Related to openwisp#1371 Related to openwisp/openwisp-utils#679
a9e7516 to
84aefd5
Compare
Use keyword arguments instead of positional arguments when calling Model.save() to ensure compatibility with Django 6.0+. [ci] Pin Django version after openwisp-utils install Use --no-deps when force-reinstalling openwisp-utils from fork to prevent pip from upgrading Django to 6.x on Python >=3.12 runners.
0f2d185 to
d870c65
Compare
…validate_constraints
…st; use serialized_rollback for database is locked fix
004a8c9 to
bf87846
Compare
…se is locked errors
|
@nemesifier thanks for the insights |
pandafy
left a comment
There was a problem hiding this comment.
@BHARATH0153 I did a quick review of the changes. Please see my comments. Don't change the test cases lightly.
Change in number of queries is fine as long as it is justified, e.g. extra query due to executing full_clean() in the update method. But, we don't change the nature of the test.
…nError - _validate_certs(): clear mismatched cert so save() auto-creates a new one - save(): recreate cert when CA changes, not just when cert is missing - test_put_floorplan_detail: restore accidentally deleted test - test_vpn_put_api: revert to original (valid host, 200 OK)
- clean(): clear cert when updating existing VPN with mismatched CA so save() auto-creates a cert for the new CA - save(): recreate cert when CA changes, not just when cert is missing - test_put_floorplan_detail: fix query count (9 -> 8) due to fork changes - test_vpn_put_api: revert to original (valid host, 200 OK)
- VpnSerializer.validate(): clear cert when CA changes on update so save() auto-creates a cert for the new CA - vpn.py save(): recreate cert when CA changes, not just when missing - test_put_floorplan_detail: add seek(0) so tempfile is readable by full_clean(); adjust query count 9->8 The save() change ensures the cert is correctly tied to the CA even when save() is called without prior full_clean() validation.
The fork now validates the updated state (not the original). The location must have type='indoor' for floorplan validation to pass. This was always a required constraint but was masked by the original ValidatedModelSerializer not applying data before validation.
41962ff to
1f42810
Compare
Checklist
Reference to Existing Issue
Related to openwisp/openwisp-utils#633.
Description of Changes
Now that the bug has
been fixed in openwisp/openwisp-utils#679,