[fix] Prevent HTTP 500 on missing device credentials#404
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:
📝 WalkthroughWalkthroughValidation for upgrade options was made more robust: resolving the upgrader class now converts missing related connection/credentials into a ValidationError instead of raising ObjectDoesNotExist/AttributeError; the SCHEMA check uses Sequence Diagram(s)sequenceDiagram
participant AdminForm as Admin form
participant Model as DeviceFirmware / UpgradeOperation
participant UpgradeMixin as UpgradeOptionsMixin
participant DB as Database (DeviceConnection)
participant Upgrader as Resolved upgrader class
AdminForm->>Model: full_clean() / save()
alt Model is UpgradeOperation and not adding
Model-->>UpgradeMixin: skip validate_upgrade_options()
Model-->>AdminForm: proceed (no upgrade-options validation)
else
Model->>UpgradeMixin: validate_upgrade_options()
UpgradeMixin->>DB: lookup related connection/credentials
DB-->>UpgradeMixin: connection data or ObjectDoesNotExist
alt ObjectDoesNotExist
UpgradeMixin-->>Model: raise ValidationError("No related connection or credentials found for this device.")
Model-->>AdminForm: propagate form error
else Connection found
UpgradeMixin->>Upgrader: resolve upgrader_class
UpgradeMixin->>Upgrader: check getattr(upgrader_class, "SCHEMA", None)
alt SCHEMA absent/falsey
UpgradeMixin-->>Model: raise ValidationError("Using upgrade options is not allowed with this upgrader.")
Model-->>AdminForm: propagate form error
else SCHEMA present
UpgradeMixin-->>Model: validate upgrade options (continue)
Model-->>AdminForm: save succeeds
end
end
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 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 |
There was a problem hiding this comment.
Pull request overview
Fixes an admin-side crash (HTTP 500) when saving models that use upgrade_options after device connection/credentials have been removed, by converting the underlying DoesNotExist into a user-facing ValidationError.
Changes:
- Catch
ObjectDoesNotExistwhen resolvingself.upgrader_classduringvalidate_upgrade_options()and raise aValidationErrorinstead. - Use a safe
getattr(..., None)when checkingSCHEMAto avoidAttributeErrorwhen no upgrader class is available. - Add regression tests covering both
DeviceFirmwareandUpgradeOperationvalidation when credentials/connections are removed.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| openwisp_firmware_upgrader/base/models.py | Prevents unhandled exceptions in validate_upgrade_options() by converting missing-connection scenarios into a ValidationError. |
| openwisp_firmware_upgrader/tests/test_models.py | Adds regression coverage ensuring missing credentials/connections produce validation errors instead of server errors. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Eeshu-Yadav
changed the title
pandafy
left a comment
pandafy
left a comment
There was a problem hiding this comment.
@Eeshu-Yadav Thanks for the patch. I’ve tested the changes and can now see an error message on the existing DeviceFirmware object:
However, the issue still cannot be resolved unless the DeviceFirmwareImage object is deleted and recreated.
Maybe, we can execute the following check only when the image field of the DeviceFirmware object changes or when a new DeviceFirmware object is created.
openwisp-firmware-upgrader/openwisp_firmware_upgrader/base/models.py
Lines 412 to 419 in 10e5aa3
github-project-automation
Bot
moved this to To do (general)
in OpenWISP Contributor's Board
github-project-automation
Bot
moved this from To do (general)
to In progress
in OpenWISP Contributor's Board
Eeshu-Yadav
force-pushed
the
issues/250-fix-http500-missing-credentials
branch
3 times, most recently
from
4ad6f3b to
efccfe0
Compare
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)
openwisp_firmware_upgrader/base/models.py (1)
412-419:⚠️ Potential issue | 🟠 MajorDon't skip the connection prerequisite for pending firmware.
This now lets an existing
DeviceFirmwarewithinstalled=Falsepassfull_clean()as long as the image is unchanged, butsave()still creates a new upgrade operation whennot self.installed(Lines 427-433). With no connection left, that operation immediately hits the"No device connection available"branch inupgrade()and never transitions out ofin-progress(Lines 916-922). Please keep this guard aligned with the save trigger, not justimage_has_changed.Suggested fix
- if self.image_has_changed and self.device.deviceconnection_set.count() < 1: + if ( + (self.image_has_changed or not self.installed) + and self.device.deviceconnection_set.count() < 1 + ): raise ValidationError( _( "This device does not have a related connection object defined "🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@openwisp_firmware_upgrader/base/models.py` around lines 412 - 419, The current validation in DeviceFirmware.full_clean only blocks missing device connections when image_has_changed, but save() will still create an upgrade when installed is False and later upgrade() fails due to no connection; update the guard in the model (the block referencing image_has_changed and device.deviceconnection_set.count()) to also enforce the prerequisite when the firmware is pending install (i.e. when self.installed is False) so it reads logically like "if self.image_has_changed or not self.installed" then raise ValidationError; ensure this touches the same DeviceFirmware attributes/methods referenced (image_has_changed, installed, device.deviceconnection_set.count()) so full_clean() prevents save() from creating an upgrade with no connections.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@openwisp_firmware_upgrader/tests/test_models.py`:
- Around line 180-184: Update the test_device_fw_save_after_credentials_removed
to cover the unchanged-image save path: create a DeviceFirmware instance with
installed=False (pending), record the current count of UpgradeOperation for that
device/firmware, remove device connections via
device_fw.device.deviceconnection_set.all().delete(), call device_fw.save() (not
just full_clean()), and assert that no new UpgradeOperation was queued (i.e.,
the UpgradeOperation count for that device/firmware remains unchanged). Repeat
the same pattern for the related test block around lines 252-264 to ensure
save() behavior is covered in both cases and reference
AbstractDeviceFirmware.clean() and save() logic when writing the assertions.
---
Outside diff comments:
In `@openwisp_firmware_upgrader/base/models.py`:
- Around line 412-419: The current validation in DeviceFirmware.full_clean only
blocks missing device connections when image_has_changed, but save() will still
create an upgrade when installed is False and later upgrade() fails due to no
connection; update the guard in the model (the block referencing
image_has_changed and device.deviceconnection_set.count()) to also enforce the
prerequisite when the firmware is pending install (i.e. when self.installed is
False) so it reads logically like "if self.image_has_changed or not
self.installed" then raise ValidationError; ensure this touches the same
DeviceFirmware attributes/methods referenced (image_has_changed, installed,
device.deviceconnection_set.count()) so full_clean() prevents save() from
creating an upgrade with no connections.
🪄 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: 199cd677-d840-4cc2-b038-7ebb7a51204f
📒 Files selected for processing (2)
openwisp_firmware_upgrader/base/models.pyopenwisp_firmware_upgrader/tests/test_models.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). (12)
- GitHub Check: Python==3.10 | django~=5.2.0
- GitHub Check: Python==3.12 | django~=5.2.0
- GitHub Check: Python==3.13 | django~=5.1.0
- GitHub Check: Python==3.13 | django~=5.2.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.11 | django~=4.2.0
- GitHub Check: Python==3.13 | 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
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{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_firmware_upgrader/tests/test_models.pyopenwisp_firmware_upgrader/base/models.py
🧠 Learnings (9)
📚 Learning: 2026-03-26T20:36:45.314Z
Learnt from: CR
Repo: openwisp/openwisp-firmware-upgrader PR: 0
File: coderabbit-custom-pre-merge-checks-unique-id-file-non-traceable-F7F2B60C-1728-4C9A-8889-4F2235E186CA.txt:0-0
Timestamp: 2026-03-26T20:36:45.314Z
Learning: Update tests to cover non-trivial behavior changes
Applied to files:
openwisp_firmware_upgrader/tests/test_models.py
📚 Learning: 2026-02-25T11:20:13.903Z
Learnt from: pandafy
Repo: openwisp/openwisp-firmware-upgrader PR: 377
File: runtests.py:27-38
Timestamp: 2026-02-25T11:20:13.903Z
Learning: In openwisp-firmware-upgrader, runtests.py now uses `execute_from_command_line()` to run Django tests in-process rather than via subprocess, so the `patch = subprocess` coverage directive is not needed in pyproject.toml.
Applied to files:
openwisp_firmware_upgrader/tests/test_models.pyopenwisp_firmware_upgrader/base/models.py
📚 Learning: 2026-02-27T19:08:56.218Z
Learnt from: nemesifier
Repo: openwisp/openwisp-firmware-upgrader PR: 383
File: openwisp_firmware_upgrader/tests/test_admin.py:50-50
Timestamp: 2026-02-27T19:08:56.218Z
Learning: In tests for openwisp-firmware-upgrader, derive app_label values from model meta instead of hard-coding. Specifically, let BaseTestAdmin compute app_label from Build._meta.app_label and config_app_label from Device._meta.app_label to ensure tests remain correct if models are swapped. This pattern should apply to all test files under openwisp_firmware_upgrader/tests.
Applied to files:
openwisp_firmware_upgrader/tests/test_models.py
📚 Learning: 2026-02-27T19:09:01.705Z
Learnt from: nemesifier
Repo: openwisp/openwisp-firmware-upgrader PR: 383
File: openwisp_firmware_upgrader/tests/test_admin.py:50-50
Timestamp: 2026-02-27T19:09:01.705Z
Learning: In openwisp-firmware-upgrader tests, the BaseTestAdmin class should derive app_label from Build._meta.app_label and config_app_label from Device._meta.app_label instead of hard-coding them. This ensures tests work correctly when models are swapped.
Applied to files:
openwisp_firmware_upgrader/base/models.py
📚 Learning: 2026-02-23T21:36:30.581Z
Learnt from: nemesifier
Repo: openwisp/openwisp-firmware-upgrader PR: 377
File: tests/openwisp2/sample_firmware_upgrader/migrations/0001_initial.py:61-67
Timestamp: 2026-02-23T21:36:30.581Z
Learning: In the openwisp/openwisp-firmware-upgrader repository, the sample app (tests/openwisp2/sample_firmware_upgrader) can have a different migration path from the main app. New migration files are generally not created for small changes in the sample app since it's only used for demonstration and testing purposes. The sample app's initial migration may include fields or choices that are added incrementally in the main app's migration history.
Applied to files:
openwisp_firmware_upgrader/base/models.py
📚 Learning: 2026-03-07T01:16:24.447Z
Learnt from: atif09
Repo: openwisp/openwisp-firmware-upgrader PR: 387
File: openwisp_firmware_upgrader/admin.py:420-423
Timestamp: 2026-03-07T01:16:24.447Z
Learning: In openwisp-firmware-upgrader, UpgradeOperation and BatchUpgradeOperation always share the same organization (BatchUpgradeOperation.build.category.organization). A user who can view an UpgradeOperation can always view its related BatchUpgradeOperation. Model-level has_view_permission(request) is sufficient for batch breadcrumb visibility in UpgradeOperationAdmin.change_view; object-level permission checks are not needed.
Applied to files:
openwisp_firmware_upgrader/base/models.py
📚 Learning: 2026-02-21T20:21:04.228Z
Learnt from: nemesifier
Repo: openwisp/openwisp-firmware-upgrader PR: 377
File: openwisp_firmware_upgrader/migrations/0015_add_group_to_batchupgradeoperation.py:16-26
Timestamp: 2026-02-21T20:21:04.228Z
Learning: In openwisp-firmware-upgrader project, the team prefers not to add explicit `related_name` to ForeignKey fields in migrations when the default reverse accessor (`<model>_set`) is acceptable. They are comfortable with Django's default naming convention.
Applied to files:
openwisp_firmware_upgrader/base/models.py
📚 Learning: 2026-02-25T00:17:23.479Z
Learnt from: nemesifier
Repo: openwisp/openwisp-firmware-upgrader PR: 377
File: openwisp_firmware_upgrader/static/firmware-upgrader/js/upgrade-progress.js:458-463
Timestamp: 2026-02-25T00:17:23.479Z
Learning: In the openwisp/openwisp-firmware-upgrader repository, focus review comments on real, actionable issues rather than theoretical edge cases, trivialities, or potential future problems. Avoid flagging issues that are functionally correct but could theoretically become problems only if the code were significantly refactored or used in different contexts.
Applied to files:
openwisp_firmware_upgrader/base/models.py
📚 Learning: 2026-02-23T21:44:41.470Z
Learnt from: nemesifier
Repo: openwisp/openwisp-firmware-upgrader PR: 377
File: openwisp_firmware_upgrader/websockets.py:233-246
Timestamp: 2026-02-23T21:44:41.470Z
Learning: In openwisp-firmware-upgrader, the `upgrade_operations` attribute on `BatchUpgradeOperation` is a cached property that returns a queryset, so it should be accessed as `batch_operation.upgrade_operations.all()` rather than being called as a method.
Applied to files:
openwisp_firmware_upgrader/base/models.py
🔇 Additional comments (1)
openwisp_firmware_upgrader/base/models.py (1)
64-75: Nice hardening around missing credentials.Catching
ObjectDoesNotExisthere turns the reported crash into a normal validation error, and the new message is correctly wrapped for translation.
Eeshu-Yadav
force-pushed
the
issues/250-fix-http500-missing-credentials
branch
from
efccfe0 to
872d992
Compare
Test Failures in Firmware UpgraderHello @Eeshu-Yadav, There is one test failure:
Fix: Review the test case |
Eeshu-Yadav
force-pushed
the
issues/250-fix-http500-missing-credentials
branch
from
872d992 to
c21ce16
Compare
|
I see the validation for UpgradeOperation also triggers this error. Can you propose a solution to mitigate this? As long as the error is solvable, e.g. the user can add back the credentials object it should be fine. |
Yes, the error is recoverable. I've added a fix: existing UpgradeOperation objects skip upgrade_options validation, so the user can add credentials back and save without being blocked by the old UpgradeOperation inline. |
pandafy
left a comment
pandafy
left a comment
There was a problem hiding this comment.
Great work @Eeshu-Yadav — I verified the initial fix, and the validation error no longer appears when saving a Device without a DeviceConnection, which is a good improvement.
However, the issue still reoccurs in a specific workflow when updating the firmware image on a device that no longer has an associated DeviceConnection.
Steps to reproduce:
- Create a Device with both
DeviceConnectionandDeviceFirmwareImage. - Delete the associated
DeviceConnection. - Update/change the Firmware Image on the Device.
- Observe the validation error:
This device does not have a related connection object defined yet and therefore it would not be possible to upgrade it, please add one in the section named "Credentials"
- Navigate to the Credentials section to add a new
DeviceConnection.
Problem:
At this point, the user is effectively blocked. Because the firmware image change is already staged in the form, they cannot add credentials without first saving. This forces a non-intuitive, multi-step workaround:
- Save the device with new credentials first
- Then go back and update the firmware image
Why this matters:
This creates a UX inconsistency and introduces friction in a fairly common workflow. Users are required to understand an implicit ordering constraint that isn’t communicated by the UI.
Can you add a test in test_admin.py which simulates submitting the complete admin form? The current tests do not cover the original scenario in which the bug appeared. You can take inspiration from test_using_upgrade_options_with_unsupported_upgrader on how to craft the payload for the device change form.
Code Review SummaryStatus: No Issues Found | Recommendation: Merge pending maintainer approval The PR effectively fixes issue #250 by converting unhandled Changes Reviewed
Key Implementation Details
Files Reviewed (5 files)
Reviewed by kimi-k2.5-0127 · 245,516 tokens |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@openwisp_firmware_upgrader/admin.py`:
- Around line 671-681: The helper _has_credentials_in_form currently treats any
submitted "...-credentials" value as active even when that inline is being
deleted; update _has_credentials_in_form to ignore credentials entries whose
corresponding inline delete flag is set (i.e. for keys like
"deviceconnection_set-<index>-credentials" skip if
"deviceconnection_set-<index>-DELETE" is truthy). Keep the logic in
_has_credentials_in_form and leave full_clean setting
instance._skip_connection_check unchanged, but ensure you only return True for
credentials on inlines that are not marked for deletion.
🪄 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: 5b8700d4-3dc5-443d-8e05-f46432e353c4
📒 Files selected for processing (3)
openwisp_firmware_upgrader/admin.pyopenwisp_firmware_upgrader/base/models.pyopenwisp_firmware_upgrader/tests/test_admin.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). (12)
- GitHub Check: Python==3.11 | django~=4.2.0
- GitHub Check: Python==3.12 | django~=5.2.0
- GitHub Check: Python==3.13 | django~=5.2.0
- GitHub Check: Python==3.12 | django~=4.2.0
- GitHub Check: Python==3.10 | django~=5.2.0
- GitHub Check: Python==3.10 | django~=5.1.0
- GitHub Check: Python==3.13 | django~=4.2.0
- GitHub Check: Python==3.11 | 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.11 | django~=5.2.0
- GitHub Check: Python==3.10 | django~=4.2.0
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{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_firmware_upgrader/admin.pyopenwisp_firmware_upgrader/tests/test_admin.pyopenwisp_firmware_upgrader/base/models.py
🧠 Learnings (7)
📓 Common learnings
Learnt from: nemesifier
Repo: openwisp/openwisp-firmware-upgrader PR: 377
File: openwisp_firmware_upgrader/static/firmware-upgrader/js/upgrade-progress.js:458-463
Timestamp: 2026-02-25T00:17:23.479Z
Learning: In the openwisp/openwisp-firmware-upgrader repository, focus review comments on real, actionable issues rather than theoretical edge cases, trivialities, or potential future problems. Avoid flagging issues that are functionally correct but could theoretically become problems only if the code were significantly refactored or used in different contexts.
📚 Learning: 2026-03-07T01:16:15.471Z
Learnt from: atif09
Repo: openwisp/openwisp-firmware-upgrader PR: 387
File: openwisp_firmware_upgrader/admin.py:420-423
Timestamp: 2026-03-07T01:16:15.471Z
Learning: In openwisp-firmware-upgrader, UpgradeOperation and BatchUpgradeOperation always share the same organization (BatchUpgradeOperation.build.category.organization). A user who can view an UpgradeOperation can always view its related BatchUpgradeOperation. For the UpgradeOperationAdmin.change_view, model-level has_view_permission(request) suffices to show breadcrumbs for batch, and object-level permission checks are not required.
Applied to files:
openwisp_firmware_upgrader/admin.py
📚 Learning: 2026-02-27T19:08:56.218Z
Learnt from: nemesifier
Repo: openwisp/openwisp-firmware-upgrader PR: 383
File: openwisp_firmware_upgrader/tests/test_admin.py:50-50
Timestamp: 2026-02-27T19:08:56.218Z
Learning: In tests for openwisp-firmware-upgrader, derive app_label values from model meta instead of hard-coding. Specifically, let BaseTestAdmin compute app_label from Build._meta.app_label and config_app_label from Device._meta.app_label to ensure tests remain correct if models are swapped. This pattern should apply to all test files under openwisp_firmware_upgrader/tests.
Applied to files:
openwisp_firmware_upgrader/tests/test_admin.py
📚 Learning: 2026-03-26T20:36:45.314Z
Learnt from: CR
Repo: openwisp/openwisp-firmware-upgrader PR: 0
File: coderabbit-custom-pre-merge-checks-unique-id-file-non-traceable-F7F2B60C-1728-4C9A-8889-4F2235E186CA.txt:0-0
Timestamp: 2026-03-26T20:36:45.314Z
Learning: Update tests to cover non-trivial behavior changes
Applied to files:
openwisp_firmware_upgrader/tests/test_admin.py
📚 Learning: 2026-02-25T11:20:13.903Z
Learnt from: pandafy
Repo: openwisp/openwisp-firmware-upgrader PR: 377
File: runtests.py:27-38
Timestamp: 2026-02-25T11:20:13.903Z
Learning: In openwisp-firmware-upgrader, runtests.py now uses `execute_from_command_line()` to run Django tests in-process rather than via subprocess, so the `patch = subprocess` coverage directive is not needed in pyproject.toml.
Applied to files:
openwisp_firmware_upgrader/tests/test_admin.py
📚 Learning: 2026-02-23T21:44:41.470Z
Learnt from: nemesifier
Repo: openwisp/openwisp-firmware-upgrader PR: 377
File: openwisp_firmware_upgrader/websockets.py:233-246
Timestamp: 2026-02-23T21:44:41.470Z
Learning: In openwisp-firmware-upgrader, the `upgrade_operations` attribute on `BatchUpgradeOperation` is a cached property that returns a queryset, so it should be accessed as `batch_operation.upgrade_operations.all()` rather than being called as a method.
Applied to files:
openwisp_firmware_upgrader/base/models.py
📚 Learning: 2026-03-07T01:16:24.447Z
Learnt from: atif09
Repo: openwisp/openwisp-firmware-upgrader PR: 387
File: openwisp_firmware_upgrader/admin.py:420-423
Timestamp: 2026-03-07T01:16:24.447Z
Learning: In openwisp-firmware-upgrader, UpgradeOperation and BatchUpgradeOperation always share the same organization (BatchUpgradeOperation.build.category.organization). A user who can view an UpgradeOperation can always view its related BatchUpgradeOperation. Model-level has_view_permission(request) is sufficient for batch breadcrumb visibility in UpgradeOperationAdmin.change_view; object-level permission checks are not needed.
Applied to files:
openwisp_firmware_upgrader/base/models.py
Eeshu-Yadav
force-pushed
the
issues/250-fix-http500-missing-credentials
branch
from
c39eaf5 to
5b39b9f
Compare
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@openwisp_firmware_upgrader/base/models.py`:
- Around line 1019-1022: The current validate_upgrade_options method in
UpgradeOperation short-circuits all validation on updates by returning when not
self._state.adding; instead, call UpgradeOptionsMixin.validate_upgrade_options()
(via super().validate_upgrade_options()) and wrap it in a try/except that only
suppresses the specific "missing credentials" error when the instance is an
update, re-raising any other exceptions; reference the validate_upgrade_options
method on the UpgradeOperation class and UpgradeOptionsMixin so you locate the
call site, catch the precise exception/type or error condition produced by
UpgradeOptionsMixin for missing credentials (or inspect the exception message),
and add a brief inline comment explaining why that specific error is tolerated
on updates (or keep the broad skip if that is intentional and annotate it).
In `@openwisp_firmware_upgrader/tests/test_admin.py`:
- Around line 397-422: The test test_save_device_after_credentials_deleted never
exercises the code path in UpgradeOptionsMixin.validate_upgrade_options that
looks up upgrader_class.SCHEMA because device_params currently passes an empty
upgrade_options; update the test to set a non-empty JSON string in
device_params["upgrade_options"] (e.g. '{"test":true}') before posting so
validate_upgrade_options proceeds to call
UpgradeOperation.validate_upgrade_options and thus triggers the upgrader_class
lookup (which is where the ObjectDoesNotExist handling was added for
upgrader_class.SCHEMA); keep the credential deletion steps intact so the
original missing-credentials path is exercised.
🪄 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: d99b4bad-56c9-4f63-90f6-6bda324b0fdf
📒 Files selected for processing (3)
openwisp_firmware_upgrader/admin.pyopenwisp_firmware_upgrader/base/models.pyopenwisp_firmware_upgrader/tests/test_admin.py
📜 Review details
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{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_firmware_upgrader/admin.pyopenwisp_firmware_upgrader/tests/test_admin.pyopenwisp_firmware_upgrader/base/models.py
🧠 Learnings (9)
📓 Common learnings
Learnt from: nemesifier
Repo: openwisp/openwisp-firmware-upgrader PR: 377
File: openwisp_firmware_upgrader/static/firmware-upgrader/js/upgrade-progress.js:458-463
Timestamp: 2026-02-25T00:17:23.479Z
Learning: In the openwisp/openwisp-firmware-upgrader repository, focus review comments on real, actionable issues rather than theoretical edge cases, trivialities, or potential future problems. Avoid flagging issues that are functionally correct but could theoretically become problems only if the code were significantly refactored or used in different contexts.
📚 Learning: 2026-02-24T00:04:16.310Z
Learnt from: nemesifier
Repo: openwisp/openwisp-firmware-upgrader PR: 377
File: openwisp_firmware_upgrader/static/firmware-upgrader/css/upgrade-options.css:11-13
Timestamp: 2026-02-24T00:04:16.310Z
Learning: In the openwisp-firmware-upgrader project, hiding empty submit rows with CSS (e.g., `#upgradeoperation_form .submit-row { display: none; }`) is preferred over removing them in templates for simplicity and speed.
Applied to files:
openwisp_firmware_upgrader/admin.py
📚 Learning: 2026-02-25T00:17:23.479Z
Learnt from: nemesifier
Repo: openwisp/openwisp-firmware-upgrader PR: 377
File: openwisp_firmware_upgrader/static/firmware-upgrader/js/upgrade-progress.js:458-463
Timestamp: 2026-02-25T00:17:23.479Z
Learning: In the openwisp/openwisp-firmware-upgrader repository, focus review comments on real, actionable issues rather than theoretical edge cases, trivialities, or potential future problems. Avoid flagging issues that are functionally correct but could theoretically become problems only if the code were significantly refactored or used in different contexts.
Applied to files:
openwisp_firmware_upgrader/admin.py
📚 Learning: 2026-03-07T01:16:15.471Z
Learnt from: atif09
Repo: openwisp/openwisp-firmware-upgrader PR: 387
File: openwisp_firmware_upgrader/admin.py:420-423
Timestamp: 2026-03-07T01:16:15.471Z
Learning: In openwisp-firmware-upgrader, UpgradeOperation and BatchUpgradeOperation always share the same organization (BatchUpgradeOperation.build.category.organization). A user who can view an UpgradeOperation can always view its related BatchUpgradeOperation. For the UpgradeOperationAdmin.change_view, model-level has_view_permission(request) suffices to show breadcrumbs for batch, and object-level permission checks are not required.
Applied to files:
openwisp_firmware_upgrader/admin.py
📚 Learning: 2026-03-26T20:36:45.314Z
Learnt from: CR
Repo: openwisp/openwisp-firmware-upgrader PR: 0
File: coderabbit-custom-pre-merge-checks-unique-id-file-non-traceable-F7F2B60C-1728-4C9A-8889-4F2235E186CA.txt:0-0
Timestamp: 2026-03-26T20:36:45.314Z
Learning: Update tests to cover non-trivial behavior changes
Applied to files:
openwisp_firmware_upgrader/tests/test_admin.py
📚 Learning: 2026-02-27T19:08:56.218Z
Learnt from: nemesifier
Repo: openwisp/openwisp-firmware-upgrader PR: 383
File: openwisp_firmware_upgrader/tests/test_admin.py:50-50
Timestamp: 2026-02-27T19:08:56.218Z
Learning: In tests for openwisp-firmware-upgrader, derive app_label values from model meta instead of hard-coding. Specifically, let BaseTestAdmin compute app_label from Build._meta.app_label and config_app_label from Device._meta.app_label to ensure tests remain correct if models are swapped. This pattern should apply to all test files under openwisp_firmware_upgrader/tests.
Applied to files:
openwisp_firmware_upgrader/tests/test_admin.py
📚 Learning: 2026-02-25T11:20:13.903Z
Learnt from: pandafy
Repo: openwisp/openwisp-firmware-upgrader PR: 377
File: runtests.py:27-38
Timestamp: 2026-02-25T11:20:13.903Z
Learning: In openwisp-firmware-upgrader, runtests.py now uses `execute_from_command_line()` to run Django tests in-process rather than via subprocess, so the `patch = subprocess` coverage directive is not needed in pyproject.toml.
Applied to files:
openwisp_firmware_upgrader/tests/test_admin.py
📚 Learning: 2026-03-07T01:16:24.447Z
Learnt from: atif09
Repo: openwisp/openwisp-firmware-upgrader PR: 387
File: openwisp_firmware_upgrader/admin.py:420-423
Timestamp: 2026-03-07T01:16:24.447Z
Learning: In openwisp-firmware-upgrader, UpgradeOperation and BatchUpgradeOperation always share the same organization (BatchUpgradeOperation.build.category.organization). A user who can view an UpgradeOperation can always view its related BatchUpgradeOperation. Model-level has_view_permission(request) is sufficient for batch breadcrumb visibility in UpgradeOperationAdmin.change_view; object-level permission checks are not needed.
Applied to files:
openwisp_firmware_upgrader/base/models.py
📚 Learning: 2026-02-23T21:44:41.470Z
Learnt from: nemesifier
Repo: openwisp/openwisp-firmware-upgrader PR: 377
File: openwisp_firmware_upgrader/websockets.py:233-246
Timestamp: 2026-02-23T21:44:41.470Z
Learning: In openwisp-firmware-upgrader, the `upgrade_operations` attribute on `BatchUpgradeOperation` is a cached property that returns a queryset, so it should be accessed as `batch_operation.upgrade_operations.all()` rather than being called as a method.
Applied to files:
openwisp_firmware_upgrader/base/models.py
🔇 Additional comments (4)
openwisp_firmware_upgrader/admin.py (1)
671-686: LGTM — addresses the DELETE-flag edge case from prior review.
_has_credentials_in_form()now iterates over indexed forms usingTOTAL_FORMSand correctly ignores rows marked for deletion, andfull_clean()flips_skip_connection_checkon the instance only when a live credentials row is present. This aligns with the gating added inAbstractDeviceFirmware.clean()(models.py Line 412-416).openwisp_firmware_upgrader/tests/test_admin.py (1)
424-462: LGTM — good coverage of the combined image-change + re-add-credentials flow.Tests the
_skip_connection_checkinteraction: deletes the existingDeviceConnection, submits a new image plus a brand-new inline credentials row in the same POST, and verifies both the image change and the single connection persisted. Exercises the gating logic atAbstractDeviceFirmware.clean()(image_has_changed and not _skip_connection_check and count() < 1).openwisp_firmware_upgrader/base/models.py (2)
64-75: LGTM — defensive handling of missing connection.Catching
ObjectDoesNotExist(raised byget_upgrader_class_for_deviceperutils.py) and converting to a translatedValidationError, plusgetattr(upgrader_class, "SCHEMA", None), correctly addresses both theDoesNotExistand the "NoneType has no attribute SCHEMA" crashes. Using the localupgrader_classin the validate call (Line 75) also avoids a second property evaluation.
412-416: LGTM — gating is correct.The three-way gate (
image_has_changed∧ ¬_skip_connection_check∧ no existing connections) correctly preserves the original UX (blocking image change when no credentials exist) while permitting saves where credentials are being added in the same POST (the admin form sets_skip_connection_check=True) or where the image is unchanged.
| def test_save_device_after_credentials_deleted(self, *args): | ||
| """Regression test for #250.""" | ||
| self._login() | ||
| device_fw = self._create_device_firmware() | ||
| device = device_fw.device | ||
| device_conn = device.deviceconnection_set.first() | ||
| device_params = self._get_device_params( | ||
| device, device_conn, device_fw.image, device_fw | ||
| ) | ||
| # delete credentials | ||
| device.deviceconnection_set.all().delete() | ||
| # remove connection data from form (simulates deleted inline) | ||
| device_params["deviceconnection_set-TOTAL_FORMS"] = 0 | ||
| device_params["deviceconnection_set-INITIAL_FORMS"] = 0 | ||
| del device_params["deviceconnection_set-0-credentials"] | ||
| del device_params["deviceconnection_set-0-id"] | ||
| del device_params["deviceconnection_set-0-update_strategy"] | ||
| del device_params["deviceconnection_set-0-enabled"] | ||
| # save device without changing image — should succeed | ||
| response = self.client.post( | ||
| reverse(f"admin:{self.config_app_label}_device_change", args=[device.id]), | ||
| data=device_params, | ||
| follow=True, | ||
| ) | ||
| self.assertEqual(response.status_code, 200) | ||
| self.assertNotContains(response, "Please correct the error") |
There was a problem hiding this comment.
Consider strengthening this regression test to actually exercise the original HTTP 500 path.
Issue #250's crash came from UpgradeOperation.validate_upgrade_options() dereferencing upgrader_class.SCHEMA when credentials were missing. With the default upgrade_options="" passed by _get_device_params, UpgradeOptionsMixin.validate_upgrade_options() early-returns on not self.upgrade_options, so the code path that originally raised is never hit here. The test still validates the admin flow/no-500 behavior, but to lock in the fix to validate_upgrade_options (catching ObjectDoesNotExist and using getattr(upgrader_class, "SCHEMA", None)), pass a non-empty upgrade_options JSON so the method reaches the upgrader_class lookup.
Proposed refinement
- device_params = self._get_device_params(
- device, device_conn, device_fw.image, device_fw
- )
+ device_params = self._get_device_params(
+ device, device_conn, device_fw.image, device_fw,
+ upgrade_options=json.dumps({"c": True}),
+ )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@openwisp_firmware_upgrader/tests/test_admin.py` around lines 397 - 422, The
test test_save_device_after_credentials_deleted never exercises the code path in
UpgradeOptionsMixin.validate_upgrade_options that looks up upgrader_class.SCHEMA
because device_params currently passes an empty upgrade_options; update the test
to set a non-empty JSON string in device_params["upgrade_options"] (e.g.
'{"test":true}') before posting so validate_upgrade_options proceeds to call
UpgradeOperation.validate_upgrade_options and thus triggers the upgrader_class
lookup (which is where the ObjectDoesNotExist handling was added for
upgrader_class.SCHEMA); keep the credential deletion steps intact so the
original missing-credentials path is exercised.
Eeshu-Yadav
force-pushed
the
issues/250-fix-http500-missing-credentials
branch
from
3e6a233 to
631be00
Compare
github-project-automation
Bot
moved this from In progress
to Done
in OpenWISP Contributor's Board
github-project-automation
Bot
moved this from Done
to In progress
in OpenWISP Contributor's Board
Test Failure in
|
Checklist
Reference to Existing Issue
Closes #250
Description of Changes
When device credentials are deleted after a firmware image is assigned, saving the device form crashes with HTTP 500 because
validate_upgrade_options()callsself.upgrader_classwhich raisesDoesNotExist(unhandled).This PR catches
ObjectDoesNotExistand raises aValidationErrorinstead, so the admin shows a proper error message.Also addsgetattrdefault to prevent potentialAttributeErroronSCHEMA.