[Fix] Fixed triggering of uncaught exception on submitting empty imag…

[stktyagi]

…e #338

Ensures that DeviceFirmwareForm.full_clean does not crash when 'device' or 'image' keys are missing from cleaned_data. Fixes #338

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 #338.

Description of Changes

Explicitly added a ValidationError if the image is missing to prevent silent save failures with a test to verify the implementation.

Screenshot

[coderabbitai]

Walkthrough

Rewrites DeviceFirmwareForm.full_clean() to avoid accessing missing keys in self.cleaned_data when the form is unbound or has field errors. The new implementation checks binding and errors, ensures the image field is present (adding a field error if missing), short-circuits when device is absent, constructs an UpgradeOperation and calls its full_clean(), and reports any ValidationError via add_error(None, error).

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~12 minutes

🚥 Pre-merge checks | ✅ 3 | ❌ 1

❌ Failed checks (1 inconclusive)

Check name	Status	Explanation	Resolution
Title check	❓ Inconclusive	The title is truncated and unclear. It mentions 'Fixed triggering of uncaught exception on submitting empty imag…' but cuts off mid-word, making it incomplete and difficult to understand the full scope of the fix.	Provide a complete, clear title that fully describes the fix, e.g., '[Fix] Handle missing image field in DeviceFirmwareForm to prevent KeyError' or '[Fix] Fixed uncaught exception when submitting empty firmware image'.

✅ Passed checks (3 passed)

Check name	Status	Explanation
Description check	✅ Passed	The description covers all required sections: checklist items completed, reference to issue #338, description of changes, and screenshot included. The PR description is complete and well-structured.
Linked Issues check	✅ Passed	The PR successfully addresses issue #338 by implementing validation in DeviceFirmwareForm.full_clean to handle missing 'device' and 'image' keys, raising ValidationError instead of KeyError, with corresponding test coverage added.
Out of Scope Changes check	✅ Passed	All changes are directly related to fixing issue #338: modifications to DeviceFirmwareForm.full_clean and addition of test_device_firmware_validation_error_in_admin_ui test. No out-of-scope changes detected.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing touches

• 📝 Generate docstrings

---

📜 Recent review details

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between dfa48f5 and cb1227d.

📒 Files selected for processing (2)

• openwisp_firmware_upgrader/admin.py
• openwisp_firmware_upgrader/tests/test_admin.py

🚧 Files skipped from review as they are similar to previous changes (2)

• openwisp_firmware_upgrader/admin.py
• openwisp_firmware_upgrader/tests/test_admin.py

⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (12)

• GitHub Check: Python==3.13 | django~=5.2.0
• GitHub Check: Python==3.13 | django~=5.1.0
• GitHub Check: Python==3.12 | django~=4.2.0
• GitHub Check: Python==3.12 | django~=5.1.0
• GitHub Check: Python==3.12 | django~=5.2.0
• GitHub Check: Python==3.11 | django~=5.2.0
• GitHub Check: Python==3.13 | django~=4.2.0
• GitHub Check: Python==3.10 | django~=4.2.0
• GitHub Check: Python==3.11 | django~=5.1.0
• GitHub Check: Python==3.10 | django~=5.2.0
• GitHub Check: Python==3.10 | django~=5.1.0
• GitHub Check: Python==3.11 | django~=4.2.0

---

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

[coderabbitai]

Actionable comments posted: 0

🧹 Nitpick comments (2)

openwisp_firmware_upgrader/tests/test_admin.py (1)

512-531: Consider using a more explicit assertion for checking DeviceFirmware creation.

The test logic is sound and correctly validates the error handling behavior. However, line 530 uses hasattr(device, "devicefirmware") which may not reliably detect whether a DeviceFirmware instance was created, especially if the relationship is a OneToOneField (where hasattr returns True even when the related object doesn't exist, but accessing it raises an exception).

💡 More explicit assertion

-        self.assertFalse(hasattr(device, "devicefirmware"))
+        self.assertFalse(DeviceFirmware.objects.filter(device=device).exists())

This approach directly queries the database and is unambiguous regardless of the relationship type.

openwisp_firmware_upgrader/admin.py (1)

356-379: Fix correctly prevents KeyError, but exception handling can be simplified.

The rewritten validation logic properly addresses the issue described in #338 by:

• Checking if the form is bound and has no errors before proceeding
• Explicitly validating that image exists in cleaned_data before use
• Gracefully handling missing device to prevent KeyError
• Properly propagating UpgradeOperation validation errors to the form

This ensures validation errors are displayed in the admin UI instead of causing an uncaught exception.

🔧 Minor simplification: remove redundant exception type

Line 378 catches both ValidationError and forms.ValidationError, but these are the same exception class. django.forms.ValidationError is just a re-export of django.core.exceptions.ValidationError, so catching both is redundant:

         try:
             upgrade_op.full_clean()
-        except (ValidationError, forms.ValidationError) as error:
+        except ValidationError as error:
             self.add_error(None, error)

This simplification improves code clarity without changing behavior.

📜 Review details

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 3eaebe7 and dfa48f5.

📒 Files selected for processing (2)

• openwisp_firmware_upgrader/admin.py
• openwisp_firmware_upgrader/tests/test_admin.py

🧰 Additional context used

🧬 Code graph analysis (2)

openwisp_firmware_upgrader/tests/test_admin.py (2)

openwisp_firmware_upgrader/tests/test_api.py (1)

• _login (56-58)

openwisp_firmware_upgrader/tests/base.py (2)

• _create_device_with_connection (199-203)
• _create_firmware_image (76-91)

openwisp_firmware_upgrader/admin.py (3)

openwisp_firmware_upgrader/api/views.py (1)

• get (94-105)

openwisp_firmware_upgrader/models.py (1)

• UpgradeOperation (43-46)

tests/openwisp2/sample_firmware_upgrader/models.py (1)

• UpgradeOperation (45-47)

⏰ 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.13 | django~=5.2.0
• GitHub Check: Python==3.11 | django~=5.1.0
• GitHub Check: Python==3.12 | django~=5.2.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
• GitHub Check: Python==3.11 | django~=4.2.0
• GitHub Check: Python==3.12 | django~=4.2.0
• GitHub Check: Python==3.13 | django~=4.2.0
• GitHub Check: Python==3.12 | django~=5.1.0
• GitHub Check: Python==3.10 | django~=5.1.0

🔇 Additional comments (1)

openwisp_firmware_upgrader/admin.py (1)

11-11: LGTM: ValidationError import added for explicit error handling.

The import supports the improved validation logic in DeviceFirmwareForm.full_clean().

[nemesifier]

Please follow up with @coderabbitai review