[feature] Introduced standalone certificate templates and device bindings#1378
[feature] Introduced standalone certificate templates and device bindings#1378stktyagi wants to merge 22 commits into
Conversation
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
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:
📝 WalkthroughWalkthroughThis PR adds a "cert" template type with CA and blueprint_cert relations, introduces a DeviceCertificate through model with auto-provisioning and revocation, wires an AbstractConfig.device_certificates M2M and m2m_changed handler, updates serializers/admin/JS, includes production and sample migrations, and adds unit and Selenium tests covering validation and lifecycle. Sequence Diagram(s)sequenceDiagram
participant Client as Client/API
participant Config as ConfigModel
participant Template as Template
participant DeviceCert as DeviceCertificate
participant X509 as django-x509.Cert
Client->>Config: assign cert Template (m2m add)
Config->>DeviceCert: manage_device_certs (post_add) -> create/get DeviceCertificate
DeviceCert->>Template: read ca/blueprint_cert, auto_cert flag
DeviceCert->>X509: _auto_create_cert (clone blueprint or CA defaults)
X509-->>DeviceCert: saved Cert instance assigned
Client->>Config: unassign cert Template (m2m remove)
Config->>DeviceCert: delete DeviceCertificate -> post_delete revokes X509 cert
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
Caution Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional.
❌ Failed checks (1 error)
✅ Passed checks (4 passed)
✨ 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 |
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
Code Review SummaryStatus: 1 Issues Found | Recommendation: Address before merge Overview
Issue Details (click to expand)WARNING
Other Observations (not in diff)Previous findings on No additional issues in unchanged code. Files Reviewed (5 files)
Fix these issues in Kilo Cloud Reviewed by gpt-5.4-20260305 · 41,749 tokens |
|
Caution Failed to replace (edit) comment. This is likely due to insufficient permissions or the comment being deleted. Error details |
![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
🤖 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 `@openwisp_controller/config/base/template.py`:
- Around line 265-267: The help text for the auto_cert field is out of date (it
still says it's only valid for VPN templates) — update the auto_cert field's
help/verbose/help_text in the Template definition in
openwisp_controller/config/base/template.py so it matches the new behavior
(auto_cert is allowed when type == "cert" as well as when type == "vpn"); locate
the auto_cert attribute (and any admin/API serializer or form label/help_text
referencing it) and change the message to something like "Valid for 'vpn' and
'cert' template types" or equivalent clear wording that includes both types.
🪄 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: 33bb61f8-c083-446e-8e45-44d753e7ff7b
📒 Files selected for processing (3)
openwisp_controller/config/base/template.pyopenwisp_controller/config/migrations/0064_template_blueprint_cert_template_ca_and_more.pytests/openwisp2/sample_config/migrations/0010_template_blueprint_cert_template_ca_and_more.py
📜 Review details
🧰 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:
tests/openwisp2/sample_config/migrations/0010_template_blueprint_cert_template_ca_and_more.pyopenwisp_controller/config/base/template.pyopenwisp_controller/config/migrations/0064_template_blueprint_cert_template_ca_and_more.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:
tests/openwisp2/sample_config/migrations/0010_template_blueprint_cert_template_ca_and_more.pyopenwisp_controller/config/base/template.pyopenwisp_controller/config/migrations/0064_template_blueprint_cert_template_ca_and_more.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:
tests/openwisp2/sample_config/migrations/0010_template_blueprint_cert_template_ca_and_more.pyopenwisp_controller/config/base/template.pyopenwisp_controller/config/migrations/0064_template_blueprint_cert_template_ca_and_more.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:
tests/openwisp2/sample_config/migrations/0010_template_blueprint_cert_template_ca_and_more.pyopenwisp_controller/config/base/template.pyopenwisp_controller/config/migrations/0064_template_blueprint_cert_template_ca_and_more.py
🧠 Learnings (4)
📚 Learning: 2026-01-12T22:27:40.078Z
Learnt from: nemesifier
Repo: openwisp/openwisp-controller PR: 1175
File: tests/openwisp2/sample_config/migrations/0008_whoisinfo_organizationconfigsettings_whois_enabled.py:18-67
Timestamp: 2026-01-12T22:27:40.078Z
Learning: In test migrations under tests/openwisp2/sample_config/migrations, verify scenarios where a swappable model (CONFIG_WHOISINFO_MODEL) is extended with extra fields (e.g., an additional 'details' field) to ensure compatibility and no errors when swapping to a custom implementation. This pattern helps confirm that extending AbstractWHOISInfo via a custom model works as intended.
Applied to files:
tests/openwisp2/sample_config/migrations/0010_template_blueprint_cert_template_ca_and_more.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:
tests/openwisp2/sample_config/migrations/0010_template_blueprint_cert_template_ca_and_more.pyopenwisp_controller/config/base/template.pyopenwisp_controller/config/migrations/0064_template_blueprint_cert_template_ca_and_more.py
📚 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/config/base/template.pyopenwisp_controller/config/migrations/0064_template_blueprint_cert_template_ca_and_more.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/config/base/template.pyopenwisp_controller/config/migrations/0064_template_blueprint_cert_template_ca_and_more.py
🔇 Additional comments (3)
openwisp_controller/config/base/template.py (1)
25-29: LGTM!Also applies to: 62-83, 251-253, 271-312
openwisp_controller/config/migrations/0064_template_blueprint_cert_template_ca_and_more.py (1)
1-59: LGTM!tests/openwisp2/sample_config/migrations/0010_template_blueprint_cert_template_ca_and_more.py (1)
1-58: LGTM!
{
"failures": [
{
"type": "Build/Infrastructure/Other",
"message": "Migrations check failed! Models' changes not migrated, please run './manage.py makemigrations' to solve the issue!",
"details": "The CI environment detected that the database migrations are out of sync. This is indicated by the error message 'Migrations check failed! Models' changes not migrated, please run './manage.py makemigrations' to solve the issue!' and the mention of a specific migration file '0005_user_expiration_date_user_user_active_expiry_idx.py'."
}
]
}CI Failure AnalysisHello @stktyagi,
Fix: Run |
Updated previous tests and added new tests for implemetation. Fixes #1356
Fixed help text for auto cert and updated migration files
Migrations Check FailedHello @stktyagi, The CI failed because there are unapplied database migrations. This means that the database schema is out of sync with the current state of the code. Fix: ./manage.py makemigrations |
|
The CI is failing due to transient infrastructure issues (not related to your code). I have restarted the failed jobs automatically (1/3). |
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 `@openwisp_controller/config/base/template.py`:
- Around line 252-253: The validation of certificate relations (calls to
self._validate_org_relation("ca") and
self._validate_org_relation("blueprint_cert")) is running unconditionally and
can fail on stale relations when the object is being switched away from the
"cert" type; restrict these validations to the cert branch so they only run when
the template's type is "cert" (e.g., wrap or move the
self._validate_org_relation(...) calls inside the same conditional that handles
the "cert" branch or after the type check that preserves/clears ca and
blueprint_cert), ensuring they do not run when the code path clears those fields
(see the branch that clears ca and blueprint_cert).
- Around line 304-305: The current check "if not self.config" coerces any falsy
value ([], "", False) into {}, bypassing BaseConfig.clean() validation; change
the condition to only handle missing configs by checking "if self.config is
None" (or equivalent explicit None check) so only absent configs are replaced
with {} and invalid/falsy payloads are left intact for
full_clean()/BaseConfig.clean() to reject.
🪄 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: d3d93328-e58d-41dd-a374-dffebd6d1e38
📒 Files selected for processing (5)
openwisp_controller/config/base/template.pyopenwisp_controller/config/migrations/0064_template_blueprint_cert_template_ca_and_more.pyopenwisp_controller/config/tests/test_template.pyopenwisp_controller/pki/tests/test_api.pytests/openwisp2/sample_config/migrations/0010_template_blueprint_cert_template_ca_and_more.py
📜 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). (11)
- GitHub Check: Python==3.12 | django~=4.2.0
- GitHub Check: Python==3.12 | django~=5.1.0
- GitHub Check: Python==3.11 | django~=4.2.0
- GitHub Check: Python==3.10 | django~=5.1.0
- GitHub Check: Python==3.13 | django~=5.1.0
- GitHub Check: Python==3.10 | 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.13 | django~=5.2.0
- GitHub Check: Python==3.11 | django~=5.2.0
- GitHub Check: Python==3.11 | django~=5.1.0
🧰 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/config/migrations/0064_template_blueprint_cert_template_ca_and_more.pytests/openwisp2/sample_config/migrations/0010_template_blueprint_cert_template_ca_and_more.pyopenwisp_controller/pki/tests/test_api.pyopenwisp_controller/config/base/template.pyopenwisp_controller/config/tests/test_template.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/config/migrations/0064_template_blueprint_cert_template_ca_and_more.pytests/openwisp2/sample_config/migrations/0010_template_blueprint_cert_template_ca_and_more.pyopenwisp_controller/pki/tests/test_api.pyopenwisp_controller/config/base/template.pyopenwisp_controller/config/tests/test_template.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/config/migrations/0064_template_blueprint_cert_template_ca_and_more.pytests/openwisp2/sample_config/migrations/0010_template_blueprint_cert_template_ca_and_more.pyopenwisp_controller/pki/tests/test_api.pyopenwisp_controller/config/base/template.pyopenwisp_controller/config/tests/test_template.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/config/migrations/0064_template_blueprint_cert_template_ca_and_more.pytests/openwisp2/sample_config/migrations/0010_template_blueprint_cert_template_ca_and_more.pyopenwisp_controller/pki/tests/test_api.pyopenwisp_controller/config/base/template.pyopenwisp_controller/config/tests/test_template.py
🧠 Learnings (4)
📚 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/config/migrations/0064_template_blueprint_cert_template_ca_and_more.pyopenwisp_controller/pki/tests/test_api.pyopenwisp_controller/config/base/template.pyopenwisp_controller/config/tests/test_template.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/config/migrations/0064_template_blueprint_cert_template_ca_and_more.pyopenwisp_controller/pki/tests/test_api.pyopenwisp_controller/config/base/template.pyopenwisp_controller/config/tests/test_template.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/config/migrations/0064_template_blueprint_cert_template_ca_and_more.pytests/openwisp2/sample_config/migrations/0010_template_blueprint_cert_template_ca_and_more.pyopenwisp_controller/pki/tests/test_api.pyopenwisp_controller/config/base/template.pyopenwisp_controller/config/tests/test_template.py
📚 Learning: 2026-01-12T22:27:40.078Z
Learnt from: nemesifier
Repo: openwisp/openwisp-controller PR: 1175
File: tests/openwisp2/sample_config/migrations/0008_whoisinfo_organizationconfigsettings_whois_enabled.py:18-67
Timestamp: 2026-01-12T22:27:40.078Z
Learning: In test migrations under tests/openwisp2/sample_config/migrations, verify scenarios where a swappable model (CONFIG_WHOISINFO_MODEL) is extended with extra fields (e.g., an additional 'details' field) to ensure compatibility and no errors when swapping to a custom implementation. This pattern helps confirm that extending AbstractWHOISInfo via a custom model works as intended.
Applied to files:
tests/openwisp2/sample_config/migrations/0010_template_blueprint_cert_template_ca_and_more.py
🔇 Additional comments (4)
openwisp_controller/pki/tests/test_api.py (1)
155-155: LGTM!Also applies to: 275-275
openwisp_controller/config/migrations/0064_template_blueprint_cert_template_ca_and_more.py (1)
12-16: LGTM!Also applies to: 19-44, 45-58, 59-74
tests/openwisp2/sample_config/migrations/0010_template_blueprint_cert_template_ca_and_more.py (1)
11-14: LGTM!Also applies to: 17-42, 43-56, 57-72
openwisp_controller/config/base/template.py (1)
25-29: LGTM!Also applies to: 62-83, 119-120
Migrations Check FailedHello @stktyagi, The CI failed because there are unapplied database migrations. Failure: Migrations check failed! Models' changes not migrated, please run './manage.py makemigrations' to solve the issue! Fix: ./manage.py makemigrations |
…s/1356-extend-abstract-template
Validate cert relations only inside the cert branch and Only coerce missing cert configs, not every falsy value. Fixes #1356
![kilo-code-bot[bot]](https://avatars.githubusercontent.com/in/2193792?s=60&v=4){kind=small}
Added test for the validation branch that now skips ca / blueprint_cert checks for non-cert templates Fixes #1356
|
Coverage is 98.404% — issues/1356-extend-abstract-template into gsoc26-x509-certificate-generator-templates. No base build found for |
Fixed line too long flake error Fixes #1356
Updated test by joining the list of strings into one sentence. Fixes #1377
stktyagi
changed the title
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
Multiple Test Failures DetectedHello @stktyagi,
|
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
openwisp_controller/config/tests/test_api.py (1)
1476-1495: 🧹 Nitpick | 🔵 Trivial | ⚡ Quick winCover
blueprint_certorganization scoping in the API tests too.This only exercises a foreign-organization
ca. A regression that acceptsblueprint_certfrom another organization would still pass, even though both relations are now exposed on the serializer. Please add a sibling assertion forblueprint_certscoped outside the template organization.🤖 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 `@openwisp_controller/config/tests/test_api.py` around lines 1476 - 1495, In test_template_create_api_org_scoping, also assert that a blueprint_cert belonging to a different organization is rejected: create a blueprint/blueprint_cert tied to org2 (similar to ca_org2), set data["blueprint_cert"] = that blueprint's pk when posting to template_list in test_template_create_api_org_scoping, then assert r.status_code == 400 and that "blueprint_cert" appears in r.data and the error message text for r.data["blueprint_cert"] mentions the organization/related-object mismatch (similar wording to the existing "related CA match" assertion).
🤖 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 `@openwisp_controller/config/api/serializers.py`:
- Around line 293-296: The DeviceCertificate.objects.filter(...).delete() call
in the serializer duplicates the centralized Config.templates m2m cleanup and
causes an extra write; remove that deletion so the serializer only calls
config.templates.set(config_templates, clear=True) and let the m2m handler own
lifecycle changes for Config.templates (locate the code around DeviceCertificate
and the config.templates.set call in the serializer and delete the
DeviceCertificate.objects.filter(...).exclude(...).delete() line).
- Around line 116-141: The validate method currently always performs
Config.objects.filter(templates=self.instance).exists() for existing instances;
change it so the expensive assignment lookup only runs when the certificate
fields are being modified by first checking if "ca" in data or "blueprint_cert"
in data before executing the block that references
Config.objects.filter(...).exists() and raises ValidationError (i.e., guard the
current if self.instance and self.instance.pk: ... block with an outer condition
like if ("ca" in data or "blueprint_cert" in data) so updates to other fields
like name/default_values/config avoid the extra query).
In
`@tests/openwisp2/sample_config/migrations/0010_template_blueprint_cert_template_ca_and_more.py`:
- Around line 132-137: The DeviceCertificate model options in the sample
migration (migrations/0010_template_blueprint_cert_template_ca_and_more.py) are
missing the swappable option; update the model's options dict for the
DeviceCertificate migration to include "swappable":
"CONFIG_DEVICECERTIFICATE_MODEL" (so the options contain verbose_name,
verbose_name_plural, abstract, unique_together and the swappable key) to match
production migration 0064_template_blueprint_cert_template_ca_and_more.py and
ensure Django's swapper mechanism works in tests with SAMPLE_APP=True.
---
Duplicate comments:
In `@openwisp_controller/config/tests/test_api.py`:
- Around line 1476-1495: In test_template_create_api_org_scoping, also assert
that a blueprint_cert belonging to a different organization is rejected: create
a blueprint/blueprint_cert tied to org2 (similar to ca_org2), set
data["blueprint_cert"] = that blueprint's pk when posting to template_list in
test_template_create_api_org_scoping, then assert r.status_code == 400 and that
"blueprint_cert" appears in r.data and the error message text for
r.data["blueprint_cert"] mentions the organization/related-object mismatch
(similar wording to the existing "related CA match" 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: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 3d2308a2-f0c4-47fa-a62b-d31870807d39
📒 Files selected for processing (16)
openwisp_controller/config/admin.pyopenwisp_controller/config/api/serializers.pyopenwisp_controller/config/apps.pyopenwisp_controller/config/base/config.pyopenwisp_controller/config/base/device_certificate.pyopenwisp_controller/config/base/template.pyopenwisp_controller/config/migrations/0064_template_blueprint_cert_template_ca_and_more.pyopenwisp_controller/config/models.pyopenwisp_controller/config/static/config/js/template_ui.jsopenwisp_controller/config/tests/test_api.pyopenwisp_controller/config/tests/test_selenium.pyopenwisp_controller/config/tests/test_template.pyopenwisp_controller/pki/tests/test_api.pytests/openwisp2/sample_config/migrations/0010_template_blueprint_cert_template_ca_and_more.pytests/openwisp2/sample_config/models.pytests/openwisp2/settings.py
📜 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). (2)
- GitHub Check: Python==3.12 | django~=5.1.0
- GitHub Check: Python==3.10 | django~=5.1.0
🧰 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:
tests/openwisp2/settings.pyopenwisp_controller/config/static/config/js/template_ui.jsopenwisp_controller/config/tests/test_selenium.pyopenwisp_controller/config/models.pyopenwisp_controller/pki/tests/test_api.pyopenwisp_controller/config/migrations/0064_template_blueprint_cert_template_ca_and_more.pyopenwisp_controller/config/admin.pytests/openwisp2/sample_config/models.pyopenwisp_controller/config/apps.pyopenwisp_controller/config/base/config.pyopenwisp_controller/config/api/serializers.pyopenwisp_controller/config/tests/test_api.pyopenwisp_controller/config/base/template.pytests/openwisp2/sample_config/migrations/0010_template_blueprint_cert_template_ca_and_more.pyopenwisp_controller/config/base/device_certificate.pyopenwisp_controller/config/tests/test_template.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:
tests/openwisp2/settings.pyopenwisp_controller/config/static/config/js/template_ui.jsopenwisp_controller/config/tests/test_selenium.pyopenwisp_controller/config/models.pyopenwisp_controller/pki/tests/test_api.pyopenwisp_controller/config/migrations/0064_template_blueprint_cert_template_ca_and_more.pyopenwisp_controller/config/admin.pytests/openwisp2/sample_config/models.pyopenwisp_controller/config/apps.pyopenwisp_controller/config/base/config.pyopenwisp_controller/config/api/serializers.pyopenwisp_controller/config/tests/test_api.pyopenwisp_controller/config/base/template.pytests/openwisp2/sample_config/migrations/0010_template_blueprint_cert_template_ca_and_more.pyopenwisp_controller/config/base/device_certificate.pyopenwisp_controller/config/tests/test_template.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:
tests/openwisp2/settings.pyopenwisp_controller/config/static/config/js/template_ui.jsopenwisp_controller/config/tests/test_selenium.pyopenwisp_controller/config/models.pyopenwisp_controller/pki/tests/test_api.pyopenwisp_controller/config/migrations/0064_template_blueprint_cert_template_ca_and_more.pyopenwisp_controller/config/admin.pytests/openwisp2/sample_config/models.pyopenwisp_controller/config/apps.pyopenwisp_controller/config/base/config.pyopenwisp_controller/config/api/serializers.pyopenwisp_controller/config/tests/test_api.pyopenwisp_controller/config/base/template.pytests/openwisp2/sample_config/migrations/0010_template_blueprint_cert_template_ca_and_more.pyopenwisp_controller/config/base/device_certificate.pyopenwisp_controller/config/tests/test_template.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:
tests/openwisp2/settings.pyopenwisp_controller/config/tests/test_selenium.pyopenwisp_controller/config/models.pyopenwisp_controller/pki/tests/test_api.pyopenwisp_controller/config/migrations/0064_template_blueprint_cert_template_ca_and_more.pyopenwisp_controller/config/admin.pytests/openwisp2/sample_config/models.pyopenwisp_controller/config/apps.pyopenwisp_controller/config/base/config.pyopenwisp_controller/config/api/serializers.pyopenwisp_controller/config/tests/test_api.pyopenwisp_controller/config/base/template.pytests/openwisp2/sample_config/migrations/0010_template_blueprint_cert_template_ca_and_more.pyopenwisp_controller/config/base/device_certificate.pyopenwisp_controller/config/tests/test_template.py
🧠 Learnings (4)
📚 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:
tests/openwisp2/settings.pyopenwisp_controller/config/tests/test_selenium.pyopenwisp_controller/config/models.pyopenwisp_controller/pki/tests/test_api.pyopenwisp_controller/config/migrations/0064_template_blueprint_cert_template_ca_and_more.pyopenwisp_controller/config/admin.pytests/openwisp2/sample_config/models.pyopenwisp_controller/config/apps.pyopenwisp_controller/config/base/config.pyopenwisp_controller/config/api/serializers.pyopenwisp_controller/config/tests/test_api.pyopenwisp_controller/config/base/template.pytests/openwisp2/sample_config/migrations/0010_template_blueprint_cert_template_ca_and_more.pyopenwisp_controller/config/base/device_certificate.pyopenwisp_controller/config/tests/test_template.py
📚 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/config/tests/test_selenium.pyopenwisp_controller/config/models.pyopenwisp_controller/pki/tests/test_api.pyopenwisp_controller/config/migrations/0064_template_blueprint_cert_template_ca_and_more.pyopenwisp_controller/config/admin.pyopenwisp_controller/config/apps.pyopenwisp_controller/config/base/config.pyopenwisp_controller/config/api/serializers.pyopenwisp_controller/config/tests/test_api.pyopenwisp_controller/config/base/template.pyopenwisp_controller/config/base/device_certificate.pyopenwisp_controller/config/tests/test_template.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/config/tests/test_selenium.pyopenwisp_controller/config/models.pyopenwisp_controller/pki/tests/test_api.pyopenwisp_controller/config/migrations/0064_template_blueprint_cert_template_ca_and_more.pyopenwisp_controller/config/admin.pyopenwisp_controller/config/apps.pyopenwisp_controller/config/base/config.pyopenwisp_controller/config/api/serializers.pyopenwisp_controller/config/tests/test_api.pyopenwisp_controller/config/base/template.pyopenwisp_controller/config/base/device_certificate.pyopenwisp_controller/config/tests/test_template.py
📚 Learning: 2026-01-12T22:27:40.078Z
Learnt from: nemesifier
Repo: openwisp/openwisp-controller PR: 1175
File: tests/openwisp2/sample_config/migrations/0008_whoisinfo_organizationconfigsettings_whois_enabled.py:18-67
Timestamp: 2026-01-12T22:27:40.078Z
Learning: In test migrations under tests/openwisp2/sample_config/migrations, verify scenarios where a swappable model (CONFIG_WHOISINFO_MODEL) is extended with extra fields (e.g., an additional 'details' field) to ensure compatibility and no errors when swapping to a custom implementation. This pattern helps confirm that extending AbstractWHOISInfo via a custom model works as intended.
Applied to files:
tests/openwisp2/sample_config/migrations/0010_template_blueprint_cert_template_ca_and_more.py
🔇 Additional comments (32)
openwisp_controller/config/admin.py (1)
120-125: LGTM!Also applies to: 1067-1068, 1083-1083, 1090-1091, 1103-1103
openwisp_controller/config/static/config/js/template_ui.js (1)
1-24: LGTM!openwisp_controller/config/tests/test_selenium.py (1)
772-804: LGTM!openwisp_controller/config/base/template.py (7)
25-29: LGTM!
40-45: LGTM!
69-92: LGTM!
128-129: LGTM!
251-278: LGTM!
280-324: LGTM!
326-362: LGTM!openwisp_controller/config/tests/test_template.py (9)
29-29: LGTM!
973-1012: LGTM!
1014-1046: LGTM!
1048-1065: LGTM!
1067-1084: LGTM!
1086-1114: LGTM!
1116-1168: LGTM!
1170-1201: LGTM!
1203-1283: LGTM!openwisp_controller/config/migrations/0064_template_blueprint_cert_template_ca_and_more.py (1)
1-152: LGTM!tests/openwisp2/sample_config/migrations/0010_template_blueprint_cert_template_ca_and_more.py (1)
106-107: LGTM!tests/openwisp2/settings.py (1)
275-275: LGTM!openwisp_controller/config/base/device_certificate.py (3)
1-56: LGTM!
67-143: LGTM!
145-162: LGTM!openwisp_controller/config/base/config.py (2)
66-72: LGTM!
505-520: LGTM!openwisp_controller/config/models.py (1)
5-5: LGTM!Also applies to: 97-105
tests/openwisp2/sample_config/models.py (1)
5-5: LGTM!Also applies to: 99-106
openwisp_controller/config/apps.py (3)
72-72: LGTM!
98-102: LGTM!
124-128: LGTM!
| Automatically creates and assigns a client x509 certificate | ||
| using Blueprint cloning and custom hardware OID injection. | ||
| """ | ||
| import copy |
nemesifier
left a comment
nemesifier
left a comment
There was a problem hiding this comment.
Good progress here @stktyagi. I focused only on what is already implemented and found one concrete lifecycle issue to fix.
| instance.devicecertificate_set.get_or_create( | ||
| template=tpl, defaults={"auto_cert": tpl.auto_cert} | ||
| ) | ||
| elif action in ["post_remove", "pre_clear"]: |
There was a problem hiding this comment.
This deletes all DeviceCertificate rows on pre_clear. The templates m2m is also cleared and re-added when templates are reordered or replaced with set(..., clear=True), so a certificate template that is still selected gets deleted, post_delete revokes its generated cert, and post_add creates a new one. Reordering templates, or an API update that keeps the same cert template, should not revoke the existing device certificate.
Please mirror the VPN client handling and delete only after the final template set is known, or only delete certificates for templates that were actually removed.
Refactors to remove the action from the m2m_changed signal, preventing accidental deletion and revocation of valid device certificates when templates are reordered or updated. Fixes #1358
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 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 `@openwisp_controller/config/base/config.py`:
- Around line 516-530: The post_clear branch currently returns early and skips
reconciliation, leaving DeviceCertificate rows for optional templates that were
removed; change the post_clear handling (in the block guarded by action ==
"post_clear" inside the SortedManyToManyField reconcile code) so that after
handling instance.is_deactivating_or_deactivated() you also run the same cleanup
used elsewhere (delete
instance.devicecertificate_set.exclude(template_id__in=instance.templates.values_list("id",
flat=True))). In practice remove or avoid the early return in post_clear and
ensure the deletion of stale DeviceCertificate rows (reference
cls.get_template_model(), templates, pk_set, instance.devicecertificate_set, and
instance.templates) so optional templates removed during the clear→re-add cycle
are cleaned up.
- Around line 531-535: The current get_or_create path creates per-config
certificates via instance.devicecertificate_set but
AbstractConfig.certificate_updated() only resolves instance.vpnclient.config and
skips DeviceCertificate-managed certs, so renewals don't mark configs dirty;
update certificate_updated (and/or add a DeviceCertificate.save handler) to
detect when the changed object is a DeviceCertificate and then locate and notify
the owning AbstractConfig(s) (e.g., the config(s) related to instance.device or
the vpnclient.config(s) that reference that DeviceCertificate) — call the same
logic used for other certificate types (mark the config modified/invalidate
checksum or call the config.certificate_updated hook) so configs referencing
DeviceCertificate certs get their checksum invalidated and changes propagated.
In `@openwisp_controller/config/base/device_certificate.py`:
- Around line 32-34: The __str__ method in device_certificate.py returns a
user-facing string "Pending Generation" that must be marked for translation;
update the method (DeviceCertificate.__str__) to wrap that literal with Django's
i18n helper (import gettext or gettext_lazy from django.utils.translation and
use _(...)) and add the corresponding import at the top of the module so the
admin list view displays the translated text.
In `@openwisp_controller/config/base/template.py`:
- Around line 251-278: The check in _validate_cert_template_changes currently
uses Config.objects.filter(templates=self).exists() which blocks edits even when
related Configs are deactivated; change the query to only consider active
assignments (e.g. filter(templates=self, status__in=[...]) or
filter(templates=self, is_active=True) depending on the Config model) so that
only configs in active/activating states prevent changing ca_id or
blueprint_cert_id; update the Config variable usage inside
_validate_cert_template_changes accordingly.
In `@openwisp_controller/config/tests/test_template.py`:
- Around line 1248-1274: The large commented subtest should be restored as an
actual test but marked skipped so it stays version-controlled; create a new test
method (e.g., test_injects_custom_mac_and_uuid_oids) that contains the existing
logic using generated_cert, mac_oid ("1.3.6.1.4.1.65901.1"), uuid_oid
("1.3.6.1.4.1.65901.2"), the mac_ext/uuid_ext lookups, and assertions against
device.mac_address and str(device.id), and decorate that method with
`@unittest.skip`("Pending django-x509#228") (or call self.skipTest with the same
message) so the subtest/self.subTest structure and assertion logic remain intact
but are explicitly skipped until the upstream issue is resolved.
🪄 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: cd0f0953-0381-4844-a2a0-10a63b5d6ba9
📒 Files selected for processing (16)
openwisp_controller/config/admin.pyopenwisp_controller/config/api/serializers.pyopenwisp_controller/config/apps.pyopenwisp_controller/config/base/config.pyopenwisp_controller/config/base/device_certificate.pyopenwisp_controller/config/base/template.pyopenwisp_controller/config/migrations/0064_template_blueprint_cert_template_ca_and_more.pyopenwisp_controller/config/models.pyopenwisp_controller/config/static/config/js/template_ui.jsopenwisp_controller/config/tests/test_api.pyopenwisp_controller/config/tests/test_selenium.pyopenwisp_controller/config/tests/test_template.pyopenwisp_controller/pki/tests/test_api.pytests/openwisp2/sample_config/migrations/0010_template_blueprint_cert_template_ca_and_more.pytests/openwisp2/sample_config/models.pytests/openwisp2/settings.py
📜 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). (10)
- GitHub Check: Python==3.11 | django~=5.2.0
- GitHub Check: Python==3.12 | django~=5.2.0
- GitHub Check: Python==3.12 | django~=4.2.0
- GitHub Check: Python==3.12 | django~=5.1.0
- GitHub Check: Python==3.13 | django~=5.2.0
- GitHub Check: Python==3.13 | django~=5.1.0
- GitHub Check: Python==3.11 | django~=5.1.0
- GitHub Check: Python==3.10 | django~=4.2.0
- GitHub Check: Python==3.10 | django~=5.2.0
- GitHub Check: Python==3.11 | django~=4.2.0
🧰 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/config/static/config/js/template_ui.jsopenwisp_controller/pki/tests/test_api.pytests/openwisp2/sample_config/models.pyopenwisp_controller/config/models.pytests/openwisp2/settings.pyopenwisp_controller/config/tests/test_selenium.pyopenwisp_controller/config/apps.pyopenwisp_controller/config/admin.pytests/openwisp2/sample_config/migrations/0010_template_blueprint_cert_template_ca_and_more.pyopenwisp_controller/config/tests/test_api.pyopenwisp_controller/config/api/serializers.pyopenwisp_controller/config/base/config.pyopenwisp_controller/config/base/template.pyopenwisp_controller/config/tests/test_template.pyopenwisp_controller/config/base/device_certificate.pyopenwisp_controller/config/migrations/0064_template_blueprint_cert_template_ca_and_more.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/config/static/config/js/template_ui.jsopenwisp_controller/pki/tests/test_api.pytests/openwisp2/sample_config/models.pyopenwisp_controller/config/models.pytests/openwisp2/settings.pyopenwisp_controller/config/tests/test_selenium.pyopenwisp_controller/config/apps.pyopenwisp_controller/config/admin.pytests/openwisp2/sample_config/migrations/0010_template_blueprint_cert_template_ca_and_more.pyopenwisp_controller/config/tests/test_api.pyopenwisp_controller/config/api/serializers.pyopenwisp_controller/config/base/config.pyopenwisp_controller/config/base/template.pyopenwisp_controller/config/tests/test_template.pyopenwisp_controller/config/base/device_certificate.pyopenwisp_controller/config/migrations/0064_template_blueprint_cert_template_ca_and_more.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/config/static/config/js/template_ui.jsopenwisp_controller/pki/tests/test_api.pytests/openwisp2/sample_config/models.pyopenwisp_controller/config/models.pytests/openwisp2/settings.pyopenwisp_controller/config/tests/test_selenium.pyopenwisp_controller/config/apps.pyopenwisp_controller/config/admin.pytests/openwisp2/sample_config/migrations/0010_template_blueprint_cert_template_ca_and_more.pyopenwisp_controller/config/tests/test_api.pyopenwisp_controller/config/api/serializers.pyopenwisp_controller/config/base/config.pyopenwisp_controller/config/base/template.pyopenwisp_controller/config/tests/test_template.pyopenwisp_controller/config/base/device_certificate.pyopenwisp_controller/config/migrations/0064_template_blueprint_cert_template_ca_and_more.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/pki/tests/test_api.pytests/openwisp2/sample_config/models.pyopenwisp_controller/config/models.pytests/openwisp2/settings.pyopenwisp_controller/config/tests/test_selenium.pyopenwisp_controller/config/apps.pyopenwisp_controller/config/admin.pytests/openwisp2/sample_config/migrations/0010_template_blueprint_cert_template_ca_and_more.pyopenwisp_controller/config/tests/test_api.pyopenwisp_controller/config/api/serializers.pyopenwisp_controller/config/base/config.pyopenwisp_controller/config/base/template.pyopenwisp_controller/config/tests/test_template.pyopenwisp_controller/config/base/device_certificate.pyopenwisp_controller/config/migrations/0064_template_blueprint_cert_template_ca_and_more.py
🧠 Learnings (4)
📚 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/pki/tests/test_api.pyopenwisp_controller/config/models.pyopenwisp_controller/config/tests/test_selenium.pyopenwisp_controller/config/apps.pyopenwisp_controller/config/admin.pyopenwisp_controller/config/tests/test_api.pyopenwisp_controller/config/api/serializers.pyopenwisp_controller/config/base/config.pyopenwisp_controller/config/base/template.pyopenwisp_controller/config/tests/test_template.pyopenwisp_controller/config/base/device_certificate.pyopenwisp_controller/config/migrations/0064_template_blueprint_cert_template_ca_and_more.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/pki/tests/test_api.pyopenwisp_controller/config/models.pyopenwisp_controller/config/tests/test_selenium.pyopenwisp_controller/config/apps.pyopenwisp_controller/config/admin.pyopenwisp_controller/config/tests/test_api.pyopenwisp_controller/config/api/serializers.pyopenwisp_controller/config/base/config.pyopenwisp_controller/config/base/template.pyopenwisp_controller/config/tests/test_template.pyopenwisp_controller/config/base/device_certificate.pyopenwisp_controller/config/migrations/0064_template_blueprint_cert_template_ca_and_more.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/pki/tests/test_api.pytests/openwisp2/sample_config/models.pyopenwisp_controller/config/models.pytests/openwisp2/settings.pyopenwisp_controller/config/tests/test_selenium.pyopenwisp_controller/config/apps.pyopenwisp_controller/config/admin.pytests/openwisp2/sample_config/migrations/0010_template_blueprint_cert_template_ca_and_more.pyopenwisp_controller/config/tests/test_api.pyopenwisp_controller/config/api/serializers.pyopenwisp_controller/config/base/config.pyopenwisp_controller/config/base/template.pyopenwisp_controller/config/tests/test_template.pyopenwisp_controller/config/base/device_certificate.pyopenwisp_controller/config/migrations/0064_template_blueprint_cert_template_ca_and_more.py
📚 Learning: 2026-01-12T22:27:40.078Z
Learnt from: nemesifier
Repo: openwisp/openwisp-controller PR: 1175
File: tests/openwisp2/sample_config/migrations/0008_whoisinfo_organizationconfigsettings_whois_enabled.py:18-67
Timestamp: 2026-01-12T22:27:40.078Z
Learning: In test migrations under tests/openwisp2/sample_config/migrations, verify scenarios where a swappable model (CONFIG_WHOISINFO_MODEL) is extended with extra fields (e.g., an additional 'details' field) to ensure compatibility and no errors when swapping to a custom implementation. This pattern helps confirm that extending AbstractWHOISInfo via a custom model works as intended.
Applied to files:
tests/openwisp2/sample_config/migrations/0010_template_blueprint_cert_template_ca_and_more.py
🔇 Additional comments (29)
openwisp_controller/config/base/device_certificate.py (3)
1-9: LGTM!Also applies to: 11-31, 36-56
58-81: LGTM!
83-109: LGTM!Also applies to: 126-163
openwisp_controller/config/models.py (1)
5-5: LGTM!Also applies to: 97-105
tests/openwisp2/sample_config/models.py (1)
5-5: LGTM!Also applies to: 99-106
tests/openwisp2/settings.py (1)
275-275: LGTM!openwisp_controller/config/migrations/0064_template_blueprint_cert_template_ca_and_more.py (1)
1-152: LGTM!tests/openwisp2/sample_config/migrations/0010_template_blueprint_cert_template_ca_and_more.py (1)
1-150: LGTM!openwisp_controller/config/tests/test_template.py (3)
29-29: LGTM!
973-1114: LGTM!
1285-1324: LGTM!openwisp_controller/config/api/serializers.py (6)
20-20: LGTM!
42-43: LGTM!
52-61: LGTM!
78-83: LGTM!
85-145: LGTM!
297-299: LGTM!openwisp_controller/config/tests/test_api.py (10)
33-33: LGTM!
742-742: LGTM!
1333-1350: LGTM!
1352-1367: LGTM!
1369-1388: LGTM!
1390-1413: LGTM!
1415-1445: LGTM!
1447-1474: LGTM!
1476-1495: LGTM!
1497-1532: LGTM!openwisp_controller/pki/tests/test_api.py (2)
275-275: 💤 Low valueVerify the 6→8 query increase comes from expected DeviceCertificate (
cert) checks.
DeviceCertificate.certis aOneToOneField(..., on_delete=models.CASCADE)and there’s apost_deletereceiver, so extra ORM work is plausible—but the evidence provided doesn’t tie the additional 2 queries insideopenwisp_controller/pki/tests/test_api.py(aroundself.assertNumQueries(8)) to thoseDeviceCertificate/certlookups. Capture/inspect the exact SQL executed in that block and map it to the expectedDeviceCertificateoperations.
155-155: 💤 Low valueDeviceCertificate delete cascade explains the extra queries in PKI API deletions
config.DeviceCertificateis now linked todjango_x509.Certviacert = models.OneToOneField(..., on_delete=models.CASCADE)and itspost_deletehook (instance.cert.revoke()whenauto_cert=True) is wired viapost_delete.connect(..., dispatch_uid="devicecert.post_delete"). When deleting a CA/Cert, Django must traverse these new CASCADE relationships during delete, which accounts for the increased query counts (+1 for CA deletion, +2 for Cert deletion) without implying an N+1 regression.
|
The CI is failing due to transient infrastructure issues (not related to your code). I have restarted the failed jobs automatically (1/3). |
Improved mutation lock and string formatting
|
The CI is failing due to transient infrastructure issues (not related to your code). I have restarted the failed jobs automatically (2/3). |
Multiple Test Failures DetectedHello @stktyagi, The CI build failed due to the following issues:
Action: |
Aryamanz29
left a comment
Aryamanz29
left a comment
There was a problem hiding this comment.
Good progress! @stktyagi - requested changes below:
Tip: also, don't hesitate to add one-liner comments to complex if-else ladders, or abstract them out into small functions/methods for reusability. It helps us follow the code more easily and keeps things maintainable!
| Automatically creates and assigns a client x509 certificate | ||
| using Blueprint cloning and custom hardware OID injection. | ||
| """ | ||
| import copy |
There was a problem hiding this comment.
could you please move inline imports to top of the file 🙏
| def save(self, *args, **kwargs): | ||
| """Performs automatic provisioning if ``auto_cert`` is True.""" | ||
| if self.auto_cert and not self.cert: | ||
| self._auto_x509() | ||
| super().save(*args, **kwargs) |
There was a problem hiding this comment.
_auto_create_cert is not transactional ^
cert.save() runs, then super().save() (the DeviceCertificate). If the second save raises (constraint, signal handler error), you've leaked a Cert row with no DeviceCertificate pointing at it. Should be wrapped in transaction.atomic() eg:
from django.db import transaction
def save(self, *args, **kwargs):
with transaction.atomic():
if self.auto_cert and not self.cert:
self._auto_x509()
super().save(*args, **kwargs)| # inject MAC and UUID as custom OIDs, prerequisite: #228 in django-x509 | ||
| # mac_oid = "1.3.6.1.4.1.65901.1" | ||
| # uuid_oid = "1.3.6.1.4.1.65901.2" | ||
| # extensions.extend([ | ||
| # { | ||
| # "oid": mac_oid, | ||
| # "value": f"ASN1:UTF8:string:{device.mac_address}", | ||
| # "critical": False | ||
| # }, | ||
| # { | ||
| # "oid": uuid_oid, | ||
| # "value": f"ASN1:UTF8:string:{device.id}", | ||
| # "critical": False | ||
| # } | ||
| # ]) |
There was a problem hiding this comment.
^ Dead commented-out code committed
There was a problem hiding this comment.
this is blocked by openwisp/django-x509#228 so i commented it out to make it easier to implement later and just uncomment. If that seems better i can remove it for now.
| # uncomment after #228 in django-x509 | ||
| # with self.subTest("Injects custom MAC and UUID OIDs"): | ||
| # extensions = generated_cert.extensions | ||
| # mac_oid = "1.3.6.1.4.1.65901.1" | ||
| # uuid_oid = "1.3.6.1.4.1.65901.2" | ||
| # mac_ext = next( | ||
| # ( | ||
| # ext | ||
| # for ext in extensions | ||
| # if ext.get("oid") == mac_oid | ||
| # or ext.get("name") == mac_oid | ||
| # ), | ||
| # None, | ||
| # ) | ||
| # uuid_ext = next( | ||
| # ( | ||
| # ext | ||
| # for ext in extensions | ||
| # if ext.get("oid") == uuid_oid | ||
| # or ext.get("name") == uuid_oid | ||
| # ), | ||
| # None, | ||
| # ) | ||
| # self.assertIsNotNone(mac_ext, "MAC OID extension missing") | ||
| # self.assertIn(device.mac_address, mac_ext["value"]) | ||
| # self.assertIsNotNone(uuid_ext, "UUID OID extension missing") | ||
| # self.assertIn(str(device.id), uuid_ext["value"]) |
There was a problem hiding this comment.
Dead commented-out code committed
There was a problem hiding this comment.
same here, this test is to verify custom oid implementation currently blocked by openwisp/django-x509#228
| def manage_device_certs(cls, sender, instance, action, pk_set, **kwargs): | ||
| """ | ||
| Syncs DeviceCertificate objects when templates are added/removed | ||
| """ | ||
| if instance._state.adding or action not in [ | ||
| "post_add", | ||
| "post_remove", | ||
| "post_clear", | ||
| ]: | ||
| return | ||
| if action == "post_clear": | ||
| if instance.is_deactivating_or_deactivated(): | ||
| instance.devicecertificate_set.all().delete() | ||
| return | ||
| if isinstance(pk_set, set): | ||
| template_model = cls.get_template_model() | ||
| templates = template_model.objects.filter(pk__in=list(pk_set)).order_by( | ||
| "created" | ||
| ) | ||
| else: | ||
| templates = pk_set | ||
| if len(pk_set) != templates.filter(required=True).count(): | ||
| instance.devicecertificate_set.exclude( | ||
| template_id__in=instance.templates.values_list("id", flat=True) | ||
| ).delete() | ||
| if action == "post_add": | ||
| for template in templates.filter(type="cert"): | ||
| instance.devicecertificate_set.get_or_create( | ||
| template=template, defaults={"auto_cert": template.auto_cert} | ||
| ) |
There was a problem hiding this comment.
can you please add a code comment to explain this if-else ladder in manage_device_certs? 🙏
| if ( | ||
| current.ca_id != self.ca_id | ||
| or current.blueprint_cert_id != self.blueprint_cert_id | ||
| ): |
There was a problem hiding this comment.
I think we only blocks ca_id / blueprint_cert_id mutation here. If a user keeps ca the same but changes type from cert -> generic, _clean_cert_template then sets ca = None and blueprint_cert = None on the active template - bypassing the protection and leaving DeviceCertificate rows that won't get cleaned up by the template save (only m2m signals clean them). Either block type-change-off-cert on active templates or trigger the cert cleanup explicitly.
| extra_kwargs = { | ||
| "blueprint_cert": { | ||
| "error_messages": { | ||
| "does_not_exist": _( | ||
| "This certificate does not exist or is already " | ||
| "assigned to an active device." | ||
| ) | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Misleading error message^
does_not_exist message says "already assigned to an active device" but the queryset filter (get_unassigned_certs()) excludes any cert with a DeviceCertificate, regardless of whether the device is active/deactivated. Either tighten the filter or relax the message.
There was a problem hiding this comment.
Thank you for this one, let me share a bit of context on why I did that in the first place:
If a user tries to assign a certificate that is already in use, DRF falls back to its native, generic relational error:
{'blueprint_cert': [ErrorDetail(string='Invalid pk "37" - object does not exist.', code='does_not_exist')]} so to avoid unhelpful error message I added that but you are right and we can improve the wording.
I will update the string to:
"This certificate does not exist or is already assigned to a device configuration profile."
This keeps our helpful error override intact while making the text technically accurate. If you have any other suggestion to do this better I'll appreciate that 🙏
| def validate(self, data): | ||
| """ | ||
| Explicitly validate certificate template fields and locks for the API. | ||
| """ |
There was a problem hiding this comment.
can you please add a code comment below for each if-else ladder in validate()? 🙏
|
The CI is failing due to transient infrastructure issues (not related to your code). I have restarted the failed jobs automatically (1/3). |
- Wrap DeviceCertificate.save() in transaction.atomic() to ensure automatic X.509 provisioning does not leak orphaned rows on failure. - Add validation to block changing the template type away from 'cert' on templates currently assigned to active devices.
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
openwisp_controller/config/base/config.py (1)
518-522:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy lift
post_clearstill leaks staleDeviceCertificaterows on real clears.For active configs this branch returns without any reconciliation, so any authoritative
templates.clear()path leaves olddevicecertificate_setrows behind.DeviceDetailSerializer.update()still does exactly that during organization changes, and directconfig.templates.clear()has the same effect, so certificates can remain linked and unreconciled even though their templates are no longer assigned. Please add an explicit cleanup path for non-reorder clears, or pass enough context to distinguish sortedm2m reordering from a real clear.🤖 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 `@openwisp_controller/config/base/config.py` around lines 518 - 522, The post_clear branch currently only deletes DeviceCertificate rows when instance.is_deactivating_or_deactivated() is true, which allows real clears (e.g., Config.templates.clear() or DeviceDetailSerializer.update() during org changes) to leak stale devicecertificate_set entries; modify the handler for action == "post_clear" in the config cleanup logic so that it also performs an explicit reconciliation/cleanup of instance.devicecertificate_set for non-deactivate clears (or use passed context to detect a sortedm2m reorder vs a true clear) — specifically, update the post_clear logic that checks instance.is_deactivating_or_deactivated() to also remove or reconcile certificates when templates are cleared (and/or accept a flag from DeviceDetailSerializer.update()/templates.clear() to indicate a real clear) so stale DeviceCertificate rows are deleted.
🤖 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 `@openwisp_controller/config/base/template.py`:
- Around line 40-44: get_unassigned_certs() currently builds assigned_cert_ids
from DeviceCertificate.objects.values_list("cert_id", flat=True) which can
include NULL and break the exclude(id__in=...) query; update the query to omit
NULL cert IDs (e.g., use
DeviceCertificate.objects.exclude(cert_id__isnull=True).values_list("cert_id",
flat=True) or .filter(cert_id__isnull=False).values_list(...)) so
assigned_cert_ids contains only real IDs before using it in
Cert.objects.exclude(id__in=assigned_cert_ids) in get_unassigned_certs().
---
Duplicate comments:
In `@openwisp_controller/config/base/config.py`:
- Around line 518-522: The post_clear branch currently only deletes
DeviceCertificate rows when instance.is_deactivating_or_deactivated() is true,
which allows real clears (e.g., Config.templates.clear() or
DeviceDetailSerializer.update() during org changes) to leak stale
devicecertificate_set entries; modify the handler for action == "post_clear" in
the config cleanup logic so that it also performs an explicit
reconciliation/cleanup of instance.devicecertificate_set for non-deactivate
clears (or use passed context to detect a sortedm2m reorder vs a true clear) —
specifically, update the post_clear logic that checks
instance.is_deactivating_or_deactivated() to also remove or reconcile
certificates when templates are cleared (and/or accept a flag from
DeviceDetailSerializer.update()/templates.clear() to indicate a real clear) so
stale DeviceCertificate rows are deleted.
🪄 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: 4b38b811-4f6b-4a60-91c5-a30808887443
📒 Files selected for processing (16)
openwisp_controller/config/admin.pyopenwisp_controller/config/api/serializers.pyopenwisp_controller/config/apps.pyopenwisp_controller/config/base/config.pyopenwisp_controller/config/base/device_certificate.pyopenwisp_controller/config/base/template.pyopenwisp_controller/config/migrations/0064_template_blueprint_cert_template_ca_and_more.pyopenwisp_controller/config/models.pyopenwisp_controller/config/static/config/js/template_ui.jsopenwisp_controller/config/tests/test_api.pyopenwisp_controller/config/tests/test_selenium.pyopenwisp_controller/config/tests/test_template.pyopenwisp_controller/pki/tests/test_api.pytests/openwisp2/sample_config/migrations/0010_template_blueprint_cert_template_ca_and_more.pytests/openwisp2/sample_config/models.pytests/openwisp2/settings.py
📜 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). (11)
- GitHub Check: Python==3.12 | django~=5.2.0
- GitHub Check: Python==3.11 | django~=4.2.0
- GitHub Check: Python==3.10 | django~=5.2.0
- GitHub Check: Python==3.11 | django~=5.2.0
- GitHub Check: Python==3.13 | django~=5.2.0
- GitHub Check: Python==3.10 | django~=5.1.0
- GitHub Check: Python==3.10 | django~=4.2.0
- GitHub Check: Python==3.12 | 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.11 | django~=5.1.0
🧰 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:
tests/openwisp2/settings.pytests/openwisp2/sample_config/models.pyopenwisp_controller/config/models.pyopenwisp_controller/config/static/config/js/template_ui.jsopenwisp_controller/config/admin.pyopenwisp_controller/pki/tests/test_api.pyopenwisp_controller/config/api/serializers.pyopenwisp_controller/config/apps.pyopenwisp_controller/config/tests/test_selenium.pyopenwisp_controller/config/base/config.pyopenwisp_controller/config/tests/test_template.pyopenwisp_controller/config/base/template.pyopenwisp_controller/config/tests/test_api.pyopenwisp_controller/config/base/device_certificate.pyopenwisp_controller/config/migrations/0064_template_blueprint_cert_template_ca_and_more.pytests/openwisp2/sample_config/migrations/0010_template_blueprint_cert_template_ca_and_more.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:
tests/openwisp2/settings.pytests/openwisp2/sample_config/models.pyopenwisp_controller/config/models.pyopenwisp_controller/config/static/config/js/template_ui.jsopenwisp_controller/config/admin.pyopenwisp_controller/pki/tests/test_api.pyopenwisp_controller/config/api/serializers.pyopenwisp_controller/config/apps.pyopenwisp_controller/config/tests/test_selenium.pyopenwisp_controller/config/base/config.pyopenwisp_controller/config/tests/test_template.pyopenwisp_controller/config/base/template.pyopenwisp_controller/config/tests/test_api.pyopenwisp_controller/config/base/device_certificate.pyopenwisp_controller/config/migrations/0064_template_blueprint_cert_template_ca_and_more.pytests/openwisp2/sample_config/migrations/0010_template_blueprint_cert_template_ca_and_more.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:
tests/openwisp2/settings.pytests/openwisp2/sample_config/models.pyopenwisp_controller/config/models.pyopenwisp_controller/config/static/config/js/template_ui.jsopenwisp_controller/config/admin.pyopenwisp_controller/pki/tests/test_api.pyopenwisp_controller/config/api/serializers.pyopenwisp_controller/config/apps.pyopenwisp_controller/config/tests/test_selenium.pyopenwisp_controller/config/base/config.pyopenwisp_controller/config/tests/test_template.pyopenwisp_controller/config/base/template.pyopenwisp_controller/config/tests/test_api.pyopenwisp_controller/config/base/device_certificate.pyopenwisp_controller/config/migrations/0064_template_blueprint_cert_template_ca_and_more.pytests/openwisp2/sample_config/migrations/0010_template_blueprint_cert_template_ca_and_more.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:
tests/openwisp2/settings.pytests/openwisp2/sample_config/models.pyopenwisp_controller/config/models.pyopenwisp_controller/config/admin.pyopenwisp_controller/pki/tests/test_api.pyopenwisp_controller/config/api/serializers.pyopenwisp_controller/config/apps.pyopenwisp_controller/config/tests/test_selenium.pyopenwisp_controller/config/base/config.pyopenwisp_controller/config/tests/test_template.pyopenwisp_controller/config/base/template.pyopenwisp_controller/config/tests/test_api.pyopenwisp_controller/config/base/device_certificate.pyopenwisp_controller/config/migrations/0064_template_blueprint_cert_template_ca_and_more.pytests/openwisp2/sample_config/migrations/0010_template_blueprint_cert_template_ca_and_more.py
🧠 Learnings (4)
📚 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:
tests/openwisp2/settings.pytests/openwisp2/sample_config/models.pyopenwisp_controller/config/models.pyopenwisp_controller/config/admin.pyopenwisp_controller/pki/tests/test_api.pyopenwisp_controller/config/api/serializers.pyopenwisp_controller/config/apps.pyopenwisp_controller/config/tests/test_selenium.pyopenwisp_controller/config/base/config.pyopenwisp_controller/config/tests/test_template.pyopenwisp_controller/config/base/template.pyopenwisp_controller/config/tests/test_api.pyopenwisp_controller/config/base/device_certificate.pyopenwisp_controller/config/migrations/0064_template_blueprint_cert_template_ca_and_more.pytests/openwisp2/sample_config/migrations/0010_template_blueprint_cert_template_ca_and_more.py
📚 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/config/models.pyopenwisp_controller/config/admin.pyopenwisp_controller/pki/tests/test_api.pyopenwisp_controller/config/api/serializers.pyopenwisp_controller/config/apps.pyopenwisp_controller/config/tests/test_selenium.pyopenwisp_controller/config/base/config.pyopenwisp_controller/config/tests/test_template.pyopenwisp_controller/config/base/template.pyopenwisp_controller/config/tests/test_api.pyopenwisp_controller/config/base/device_certificate.pyopenwisp_controller/config/migrations/0064_template_blueprint_cert_template_ca_and_more.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/config/models.pyopenwisp_controller/config/admin.pyopenwisp_controller/pki/tests/test_api.pyopenwisp_controller/config/api/serializers.pyopenwisp_controller/config/apps.pyopenwisp_controller/config/tests/test_selenium.pyopenwisp_controller/config/base/config.pyopenwisp_controller/config/tests/test_template.pyopenwisp_controller/config/base/template.pyopenwisp_controller/config/tests/test_api.pyopenwisp_controller/config/base/device_certificate.pyopenwisp_controller/config/migrations/0064_template_blueprint_cert_template_ca_and_more.py
📚 Learning: 2026-01-12T22:27:40.078Z
Learnt from: nemesifier
Repo: openwisp/openwisp-controller PR: 1175
File: tests/openwisp2/sample_config/migrations/0008_whoisinfo_organizationconfigsettings_whois_enabled.py:18-67
Timestamp: 2026-01-12T22:27:40.078Z
Learning: In test migrations under tests/openwisp2/sample_config/migrations, verify scenarios where a swappable model (CONFIG_WHOISINFO_MODEL) is extended with extra fields (e.g., an additional 'details' field) to ensure compatibility and no errors when swapping to a custom implementation. This pattern helps confirm that extending AbstractWHOISInfo via a custom model works as intended.
Applied to files:
tests/openwisp2/sample_config/migrations/0010_template_blueprint_cert_template_ca_and_more.py
🔇 Additional comments (16)
tests/openwisp2/settings.py (1)
275-275: LGTM!openwisp_controller/config/static/config/js/template_ui.js (1)
1-25: LGTM!openwisp_controller/config/admin.py (2)
118-126: LGTM!
1067-1068: LGTM!Also applies to: 1083-1083, 1090-1091, 1103-1103
openwisp_controller/config/apps.py (2)
72-72: LGTM!
98-102: LGTM!Also applies to: 124-128
openwisp_controller/config/tests/test_selenium.py (1)
772-805: LGTM!openwisp_controller/config/base/device_certificate.py (4)
13-52: LGTM!
54-84: LGTM!
86-151: LGTM!
153-163: LGTM!tests/openwisp2/sample_config/migrations/0010_template_blueprint_cert_template_ca_and_more.py (1)
13-150: LGTM!openwisp_controller/pki/tests/test_api.py (1)
155-155: ⚡ Quick winConfirm the delete query bump matches Django’s related-object handling
openwisp_controller/pki/tests/test_api.pynow expects more queries for deletes (test_ca_delete_api: 6,test_cert_delete_api: 8). With the new relations:
DeviceCertificate.cert→django_x509.Certuseson_delete=CASCADETemplate.blueprint_cert→django_x509.Certuseson_delete=SET_NULLDeviceCertificatehas apost_deletehook that can callinstance.cert.revoke()whenauto_cert=TrueThat explains why cert deletion can add more work than CA deletion (CASCADE on
DeviceCertificate.certplus SET_NULL updates forTemplate.blueprint_cert). Sincepki/tests/test_api.pydoesn’t explicitly createTemplate/DeviceCertificateobjects, the increase should be coming from Django’s delete collector existence checks/SET_NULL handling—not per-row churn.Check the actual SQL for these two DELETE endpoints and confirm the extra queries are constant-time (no N+1) even when multiple related
DeviceCertificate/Templaterows exist.openwisp_controller/config/base/template.py (1)
25-29: LGTM!Also applies to: 69-92, 128-129, 251-341, 351-355, 369-370, 378-378
openwisp_controller/config/tests/test_api.py (1)
33-33: LGTM!Also applies to: 742-742, 1333-1540
openwisp_controller/config/migrations/0064_template_blueprint_cert_template_ca_and_more.py (1)
1-152: LGTM!
Fixed NULL handling in get_unassigned_certs() to avoid empty blueprint_cert choices
|
The CI is failing due to transient infrastructure issues (not related to your code). I have restarted the failed jobs automatically (1/3). |
|
The CI is failing due to transient infrastructure issues (not related to your code). I have restarted the failed jobs automatically (2/3). |
Updated get_unassigned_certs to include revoked certificates
Dependency Conflict in
|
…s/1356-extend-abstract-template
4 participants
Checklist
Reference to Existing Issue
Closes #1356
Description of Changes
This PR establishes the database architecture, UI, API and lifecycle for standalone X.509 certificate templates.
Closes #1356
Closes #1377
Closes #1357
Closes #1361
Closes #1358
Manual test plan
Setup
PKI->Certification Authoritiesand create two CAs:CA-1andCA-2.PKI->Certificatesand create two certificates to act as blueprints:Blueprint-1(Must useCA-1)Blueprint-2(Must useCA-2)Devicesand create a device (test-device).Template Creation and Validation
Configuration->Templatesand clickADD TEMPLATE.Certificate.CA-1.Blueprint-2(which belongs toCA-2). Try to save.Blueprint-1. Name the templateActive-Cert-Template. Save it.Device Provisioning
test-device.Active-Cert-Template. Save.PKI->Certificates.test-device. Its status should be valid (not revoked).Active Mutation Locks
Configuration->Templatesand editActive-Cert-Template(which is now assigned to an active device).Generic. Try to save.CA-2. Try to save.Blueprint-2(ensure you also change the CA so they match, triggering the active lock). Try to save.Revocation on Removal
test-device.Active-Cert-Templateentirely from the templates list. Save.PKI->Certificatesand locate the device's certificate.