Skip to content

[fix] Prevent HTTP 500 on missing device credentials#404

Open
Eeshu-Yadav wants to merge 4 commits intoopenwisp:masterfrom
Eeshu-Yadav:issues/250-fix-http500-missing-credentials
Open

[fix] Prevent HTTP 500 on missing device credentials#404
Eeshu-Yadav wants to merge 4 commits intoopenwisp:masterfrom
Eeshu-Yadav:issues/250-fix-http500-missing-credentials

Conversation

@Eeshu-Yadav
Copy link
Copy Markdown

@Eeshu-Yadav Eeshu-Yadav commented Mar 31, 2026

Checklist

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

Reference to Existing Issue

Closes #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() calls self.upgrader_class which raises DoesNotExist (unhandled).
This PR catches ObjectDoesNotExist and raises a ValidationError instead, so the admin shows a proper error message.Also adds getattr default to prevent potential AttributeError on SCHEMA.

Copilot AI review requested due to automatic review settings March 31, 2026 21:27
@coderabbitai
Copy link
Copy Markdown

coderabbitai bot commented Mar 31, 2026

Note

Reviews paused

It 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 reviews.auto_review.auto_pause_after_reviewed_commits setting.

Use the following commands to manage reviews:

  • @coderabbitai resume to resume automatic reviews.
  • @coderabbitai review to trigger a single review.

Use the checkboxes below for quick actions:

  • ▶️ Resume reviews
  • 🔍 Trigger review
📝 Walkthrough

Walkthrough

Validation for upgrade options was made more robust: resolving the upgrader class is wrapped to convert missing related connection/credentials into a ValidationError with a device-specific message; the SCHEMA check uses the resolved class via getattr(upgrader_class, "SCHEMA", None) to avoid AttributeError; call sites use the local upgrader_class variable. AbstractDeviceFirmware.clean() enforces related DeviceConnection existence only when image_has_changed is true and "_skip_connection_check" is not set. AbstractUpgradeOperation.validate_upgrade_options() runs superclass validation only when the operation is being created. Tests and some query-count assertions were added/adjusted.

Sequence Diagram(s)

sequenceDiagram
    participant AdminForm as Admin form
    participant Model as DeviceFirmware / UpgradeOperation
    participant UpgradeMixin as UpgradeOptionsMixin.validate_upgrade_options
    participant DB as Database (DeviceConnection)
    participant Upgrader as Resolved upgrader class

    AdminForm->>Model: full_clean() / save()
    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
Loading

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Suggested reviewers

  • pandafy
  • nemesifier

Caution

Pre-merge checks failed

Please resolve all errors before merging. Addressing warnings is optional.

  • Ignore

❌ Failed checks (1 error)

Check name Status Explanation Resolution
Bug Fixes ❌ Error PR fixes HTTP 500 error but leaves unresolved bug in _has_credentials_in_form() that fails to check DELETE flag for deleted inlines, bypassing validation when credentials are removed. Lacks Selenium tests for admin form submission scenario. Fix _has_credentials_in_form() to check -DELETE suffix for deleted inlines and add Selenium tests simulating admin credential deletion scenario to verify proper validation error instead of HTTP 500.
✅ Passed checks (4 passed)
Check name Status Explanation
Title check ✅ Passed The PR title clearly describes the main fix: preventing HTTP 500 errors when device credentials are missing, which directly addresses issue #250.
Description check ✅ Passed The PR description covers the problem statement, root cause, and implemented solution. All required sections from the template are completed with meaningful content.
Linked Issues check ✅ Passed The PR successfully addresses issue #250 by catching ObjectDoesNotExist exceptions, using getattr with defaults for SCHEMA access, and skipping validation for existing UpgradeOperation objects.
Out of Scope Changes check ✅ Passed All changes are scoped to fixing the HTTP 500 error on missing credentials and validating upgrade options appropriately. Changes to admin forms, models, and tests directly support the stated objectives.
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

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.

❤️ Share

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

Copy link
Copy Markdown

Copilot AI left a comment

Choose a reason for hiding this comment

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

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 ObjectDoesNotExist when resolving self.upgrader_class during validate_upgrade_options() and raise a ValidationError instead.
  • Use a safe getattr(..., None) when checking SCHEMA to avoid AttributeError when no upgrader class is available.
  • Add regression tests covering both DeviceFirmware and UpgradeOperation validation 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]
coderabbitai bot previously approved these changes Mar 31, 2026
@Eeshu-Yadav Eeshu-Yadav changed the title [fix] Prevent HTTP 500 on missing device credentials #250 [fix] Prevent HTTP 500 on missing device credentials Mar 31, 2026
Copy link
Copy Markdown
Member

@pandafy pandafy left a comment

Choose a reason for hiding this comment

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

@Eeshu-Yadav Thanks for the patch. I’ve tested the changes and can now see an error message on the existing DeviceFirmware object:

Image

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.

if self.device.deviceconnection_set.count() < 1:
raise ValidationError(
_(
"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"'
)
)

@Eeshu-Yadav Eeshu-Yadav force-pushed the issues/250-fix-http500-missing-credentials branch 3 times, most recently from 4ad6f3b to efccfe0 Compare April 2, 2026 22:12
@Eeshu-Yadav
Copy link
Copy Markdown
Author

@pandafy
image

Copy link
Copy Markdown

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 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 | 🟠 Major

Don't skip the connection prerequisite for pending firmware.

This now lets an existing DeviceFirmware with installed=False pass full_clean() as long as the image is unchanged, but save() still creates a new upgrade operation when not self.installed (Lines 427-433). With no connection left, that operation immediately hits the "No device connection available" branch in upgrade() and never transitions out of in-progress (Lines 916-922). Please keep this guard aligned with the save trigger, not just image_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

📥 Commits

Reviewing files that changed from the base of the PR and between 10e5aa3 and dec635a.

📒 Files selected for processing (2)
  • openwisp_firmware_upgrader/base/models.py
  • openwisp_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.py
  • openwisp_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.py
  • openwisp_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 ObjectDoesNotExist here turns the reported crash into a normal validation error, and the new message is correctly wrapped for translation.

Comment thread openwisp_firmware_upgrader/tests/test_models.py
@Eeshu-Yadav Eeshu-Yadav force-pushed the issues/250-fix-http500-missing-credentials branch from efccfe0 to 872d992 Compare April 2, 2026 22:18
@openwisp-companion
Copy link
Copy Markdown

Test Failures in Firmware Upgrader

Hello @Eeshu-Yadav,
(Analysis for commit 872d992)

There is one test failure:

  1. Test Failure: The test test_device_firmware_detail_400 in openwisp_firmware_upgrader.tests.test_api failed with an AssertionError. The error message 17 != 18 : 17 queries executed, 18 expected indicates a discrepancy in the number of database queries performed during the test.

Fix: Review the test case test_device_firmware_detail_400 and adjust the expected number of queries or the test logic to match the actual database interactions. This might involve optimizing queries or updating the test's expectations.

@Eeshu-Yadav Eeshu-Yadav force-pushed the issues/250-fix-http500-missing-credentials branch from 872d992 to c21ce16 Compare April 3, 2026 15:07
coderabbitai[bot]
coderabbitai bot previously approved these changes Apr 4, 2026
@Eeshu-Yadav Eeshu-Yadav requested a review from pandafy April 4, 2026 12:24
@pandafy
Copy link
Copy Markdown
Member

pandafy commented Apr 6, 2026

@pandafy image

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.

@Eeshu-Yadav
Copy link
Copy Markdown
Author

@pandafy image

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.

@pandafy image

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.
The validation now only runs when creating a new UpgradeOperation, not on every form save of an existing one.

Copy link
Copy Markdown
Member

@pandafy pandafy left a comment

Choose a reason for hiding this comment

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

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:

  1. Create a Device with both DeviceConnection and DeviceFirmwareImage.
  2. Delete the associated DeviceConnection.
  3. Update/change the Firmware Image on the Device.
  4. 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"
  1. 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.

@kilo-code-bot
Copy link
Copy Markdown

kilo-code-bot bot commented Apr 9, 2026

Code Review Summary

Status: No Issues Found | Recommendation: Merge pending maintainer approval

The PR effectively fixes issue #250 by converting unhandled ObjectDoesNotExist exceptions into user-facing ValidationError messages. The implementation is sound and includes comprehensive regression tests.

Changes Reviewed

File Changes Assessment
base/models.py Added try/except in validate_upgrade_options() to catch ObjectDoesNotExist; Added _skip_connection_check conditional in clean(); Added validate_upgrade_options() override in AbstractUpgradeOperation Proper error handling that prevents HTTP 500 crashes while maintaining validation logic
admin.py Added _has_credentials_in_form() method and _skip_connection_check flag in full_clean() Allows users to add credentials and change firmware image in a single form submission
tests/test_admin.py Added test_save_device_after_credentials_deleted() and test_change_image_and_add_credentials_together() Well-structured regression tests covering both edge cases
tests/test_models.py Added test_device_fw_save_after_credentials_removed() and test_upgrade_operation_credentials_removed() Unit tests verifying validation behavior at the model level
tests/test_api.py Updated query count assertions (18→17, 27→26) Accurate reflection of optimized query behavior

Key Implementation Details

  1. Safe getattr: Uses getattr(upgrader_class, "SCHEMA", None) instead of direct attribute access to prevent AttributeError
  2. Conditional validation: AbstractUpgradeOperation.validate_upgrade_options() only runs on new objects (self._state.adding), allowing existing operations to be edited without blocking credential updates
  3. Form integration: The _skip_connection_check flag properly detects when credentials are being added in the same form submission

Files Reviewed (5 files)

  • openwisp_firmware_upgrader/base/models.py
  • openwisp_firmware_upgrader/admin.py
  • openwisp_firmware_upgrader/tests/test_admin.py
  • openwisp_firmware_upgrader/tests/test_models.py
  • openwisp_firmware_upgrader/tests/test_api.py

Reviewed by kimi-k2.5-0127 · 192,903 tokens

Copy link
Copy Markdown

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 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

📥 Commits

Reviewing files that changed from the base of the PR and between b2e4e18 and c39eaf5.

📒 Files selected for processing (3)
  • openwisp_firmware_upgrader/admin.py
  • openwisp_firmware_upgrader/base/models.py
  • openwisp_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.py
  • openwisp_firmware_upgrader/tests/test_admin.py
  • openwisp_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

Comment thread openwisp_firmware_upgrader/admin.py
@Eeshu-Yadav Eeshu-Yadav force-pushed the issues/250-fix-http500-missing-credentials branch from c39eaf5 to 5b39b9f Compare April 9, 2026 22:45
@Eeshu-Yadav Eeshu-Yadav requested a review from pandafy April 9, 2026 22:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Status: In progress

Development

Successfully merging this pull request may close these issues.

[bug] Deleting DeviceConnection object for a device with DeviceFirmareImage raises HTTP 500 error

3 participants