[change] Refactor ReceiveUrlAdmin to inherit CopyableFieldAdmin #344 #550
Conversation
Eeshu-Yadav
force-pushed
the
issues/344-ReceiveUrlAdmin-inherit-CopyableFieldAdmin
branch
from
e6c43e2 to
edd8283
Compare
Eeshu-Yadav
force-pushed
the
issues/344-ReceiveUrlAdmin-inherit-CopyableFieldAdmin
branch
from
edd8283 to
559f85e
Compare
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
📝 WalkthroughWalkthroughReceiveUrlAdmin now inherits from CopyableFieldsAdmin and exposes receive_url via copyable_fields = ("receive_url",). Tests update ProjectAdmin to inherit only from ReceiveUrlAdmin, add copyable_fields = ("uuid", "receive_url") and a uuid() method returning obj.pk. Test expectations are adjusted: receive_url is treated as a copyable field (absent from ma.get_fields()) and appears at the end of ma.fields; the add form no longer expects receive_url among standard fields. Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: Organization UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (3)
🧰 Additional context used🧬 Code graph analysis (2)tests/test_project/tests/test_admin.py (1)
tests/test_project/admin.py (1)
🪛 Ruff (0.14.14)tests/test_project/admin.py[warning] 83-83: Mutable class attributes should be annotated with (RUF012) ⏰ 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). (16)
🔇 Additional comments (4)
✏️ Tip: You can disable this entire section by setting 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[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
…isp#344 - Refactored ReceiveUrlAdmin to inherit from CopyableFieldsAdmin instead of ModelAdmin - Eliminated code duplication in add_view/change_view methods - Made receive_url work as a copyable field automatically - Updated ProjectAdmin to use single inheritance (no longer needs UUIDAdmin + ReceiveUrlAdmin) - Updated tests to reflect correct behavior (copyable fields excluded from add forms) - Reduced JavaScript and Python logic repetition This addresses the enhancement request to reduce repetition between ReceiveUrlAdmin and CopyableFieldAdmin by using proper inheritance. Fixes openwisp#344
Eeshu-Yadav
force-pushed
the
issues/344-ReceiveUrlAdmin-inherit-CopyableFieldAdmin
branch
from
559f85e to
4715ba2
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
openwisp_utils/admin.py (2)
178-202:⚠️ Potential issue | 🟠 MajorPotential
NameErrorwhenreceive_url_querystring_argis falsy.If
receive_url_querystring_argis set toNoneor an empty string, theifblock at line 195 is skipped, leavingurlundefined. Line 202 then attempts to returnurl, which would raise aNameError.🐛 Proposed fix
if self.receive_url_querystring_arg: url = "{0}{1}?{2}={3}".format( baseurl, receive_path, self.receive_url_querystring_arg, getattr(obj, self.receive_url_querystring_arg), ) - return url + return url + return "{0}{1}".format(baseurl, receive_path)
204-205:⚠️ Potential issue | 🟠 MajorRemove
receive_url.jsfromReceiveUrlAdmin.Mediato eliminate duplicate field enhancement logic.Since
ReceiveUrlAdminextendsCopyableFieldsAdminand includes"receive_url"incopyable_fields, the parent class'scopyable.jsalready handles making this field selectable. Includingreceive_url.jscauses the same field to be enhanced twice with identical functionality—both convert the readonly text to an input field and add click-to-select behavior. Django's Media merging ensures both scripts load, resulting in unnecessary duplication. Removereceive_url.jsfrom the Media class and rely on the genericcopyable.jshandler.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
openwisp_utils/admin.pytests/test_project/admin.pytests/test_project/tests/test_admin.py
🧰 Additional context used
🧬 Code graph analysis (2)
tests/test_project/tests/test_admin.py (1)
openwisp_utils/admin.py (1)
get_fields(92-102)
tests/test_project/admin.py (1)
openwisp_utils/admin.py (2)
ReceiveUrlAdmin(153-207)uuid(147-148)
🪛 Ruff (0.14.14)
tests/test_project/admin.py
[warning] 83-83: Mutable class attributes should be annotated with typing.ClassVar
(RUF012)
⏰ 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). (16)
- GitHub Check: CodeQL analysis (python)
- GitHub Check: Agent
- GitHub Check: Python==3.12 | django~=5.0.0
- GitHub Check: Python==3.10 | django~=5.0.0
- GitHub Check: Python==3.11 | django~=5.2.0
- GitHub Check: Python==3.12 | django~=4.2.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~=4.2.0
- GitHub Check: Python==3.11 | django~=5.1.0
- GitHub Check: Python==3.12 | 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.11 | django~=5.0.0
- GitHub Check: Python==3.12 | django~=5.1.0
- GitHub Check: Python==3.10 | django~=5.2.0
🔇 Additional comments (4)
openwisp_utils/admin.py (1)
153-168: LGTM! Clean refactoring to eliminate code duplication.The inheritance change from
ModelAdmintoCopyableFieldsAdminproperly centralizes the copyable-field behavior, and addingcopyable_fields = ("receive_url",)correctly integrates with the parent class machinery.tests/test_project/admin.py (1)
82-93: LGTM! Proper adaptation to the refactored inheritance.The changes correctly:
- Use single inheritance from
ReceiveUrlAdmin(which now providesCopyableFieldsAdminfunctionality)- Define
copyable_fieldsto include bothuuidandreceive_url- Replicate the
uuidmethod from the formerUUIDAdminbase classRegarding the static analysis warning (RUF012) on
inlines: This is a standard Django admin pattern where mutable class attributes are intentionally shared. The warning can be safely ignored for Django admin classes.tests/test_project/tests/test_admin.py (2)
218-218: LGTM! Test correctly reflects the new copyable field behavior.Since
receive_urlis now a copyable field viaCopyableFieldsAdmin, it is correctly excluded from the add form (consistent withCopyableFieldsAdmin.get_fieldsreturning fields without copyable_fields whenobj is None).
250-262: LGTM! Test assertions properly updated.The test correctly validates:
- Both
uuidandreceive_urlare excluded fromma.get_fields()(as they are copyable fields)- The field ordering in
ma.fieldsplacesuuidfirst andreceive_urllast, with other fields in betweenThe inline comments clearly document the test intent.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
There was a problem hiding this comment.
Pull request overview
This pull request refactors ReceiveUrlAdmin to inherit from CopyableFieldsAdmin instead of ModelAdmin, reducing code duplication and making the receive_url field work as a copyable field automatically.
Changes:
- Refactored
ReceiveUrlAdminto inherit fromCopyableFieldsAdminwith propercopyable_fieldsconfiguration - Updated
ProjectAdminto use single inheritance fromReceiveUrlAdminand manually implement the uuid method - Fixed a documentation typo in
ReceiveUrlAdmindocstring - Updated tests to verify that copyable fields (uuid and receive_url) are properly excluded from add forms
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| openwisp_utils/admin.py | Changed ReceiveUrlAdmin base class from ModelAdmin to CopyableFieldsAdmin, added copyable_fields attribute, and fixed docstring typo |
| tests/test_project/admin.py | Removed UUIDAdmin from ProjectAdmin inheritance, added manual uuid method implementation, and added copyable_fields tuple |
| tests/test_project/tests/test_admin.py | Updated tests to verify both uuid and receive_url fields are excluded from add forms |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
3 participants
Checklist
Refactored ReceiveUrlAdmin to inherit from CopyableFieldsAdmin instead of ModelAdmin
Eliminated code duplication in add_view/change_view methods
Made receive_url work as a copyable field automatically
Updated ProjectAdmin to use single inheritance (no longer needs UUIDAdmin + ReceiveUrlAdmin)
Updated tests to reflect correct behavior (copyable fields excluded from add forms
Fixes #344