[fix] Guard admin reverse and template url behind FIRMWARE_UPGRADER_API #444#447
[fix] Guard admin reverse and template url behind FIRMWARE_UPGRADER_API #444#447BHARATH0153 wants to merge 2 commits into
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (3)
📜 Recent review details🧰 Additional context used📓 Path-based instructions (2)**/*.{py,html}📄 CodeRabbit inference engine (Custom checks)
Files:
**/*.py📄 CodeRabbit inference engine (AGENTS.md)
Files:
🧠 Learnings (4)📚 Learning: 2026-03-07T01:16:15.471ZApplied to files:
📚 Learning: 2026-06-02T08:16:00.439ZApplied to files:
📚 Learning: 2026-02-27T19:08:56.218ZApplied to files:
📚 Learning: 2026-06-15T20:19:28.326ZApplied to files:
🔇 Additional comments (6)
📝 WalkthroughWalkthroughThis PR updates admin URL handling to respect Estimated code review effort: 2 (Simple) | ~10 minutes Suggested reviewers: Caution Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional.
❌ Failed checks (2 errors, 1 warning)
✅ Passed checks (5 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
Bot
added
bug
|
@coderabbitai review |
✅ Action performedReview finished.
|
|
@coderabbitai any outstanding issues? |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
# Get PR details, review comments, and checks
gh pr view 447 --repo openwisp/openwisp-firmware-upgrader --json title,state,reviews,reviewDecision,statusCheckRollup
echo "=== PR REVIEW COMMENTS ==="
gh api repos/openwisp/openwisp-firmware-upgrader/pulls/447/comments --jq '.[] | {user: .user.login, body: .body, path: .path, line: .line, resolved: .pull_request_review_id}'
echo "=== PR ISSUE COMMENTS ==="
gh api repos/openwisp/openwisp-firmware-upgrader/issues/447/comments --jq '.[] | {user: .user.login, body: .body[0:300]}'
echo "=== PR FILES ==="
gh api repos/openwisp/openwisp-firmware-upgrader/pulls/447/files --jq '.[] | {filename: .filename, additions: .additions, deletions: .deletions}'Length of output: 7009 🏁 Script executed: #!/bin/bash
# Look at the actual diff to check for issues
gh api repos/openwisp/openwisp-firmware-upgrader/pulls/447 --jq '.body'Length of output: 892 🏁 Script executed: #!/bin/bash
# Get the changed files content
echo "=== admin.py patch ==="
gh api repos/openwisp/openwisp-firmware-upgrader/pulls/447/files --jq '.[] | select(.filename | contains("admin")) | .patch'
echo "=== template patch ==="
gh api repos/openwisp/openwisp-firmware-upgrader/pulls/447/files --jq '.[] | select(.filename | contains("template") or contains("html")) | .patch'Length of output: 2364
✅ Good newsAll CI checks pass across all Python/Django matrix combinations, and there are no unresolved review comments.
|
nemesifier
left a comment
nemesifier
left a comment
There was a problem hiding this comment.
A failing test would have been nice, it should be possible to achieve this by mocking app_settings.
Please clarify why the entire checklist is empty in the PR description, you should flag at least the first two checkboxes. Have you manually tested this?
|
Hi @BHARATH0153 👋, This is a friendly reminder that this pull request has had no activity for 7 days since changes were requested. We'd love to see this contribution merged! Please take a moment to:
If you're busy or need more time, no worries! Just leave a comment to let us know you're still working on it. Note: within 7 more days, the linked issue will be unassigned to allow other contributors to work on it. Thank you for your contribution! 🙏 |
2bc735b to
c51dd3a
Compare
![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_firmware_upgrader/templates/admin/firmware_upgrader/device_firmware_inline.html`:
- Line 8: The inline template’s cancel URL handling is relying on Django’s
implicit empty-string fallback and still appends a redundant default filter.
Remove the unnecessary fallback filter in device_firmware_inline.html, and align
this with the admin.py approach by resolving/passing the API availability or
cancel URL from Python context (using the relevant firmware upgrader
admin/template symbols). Also ensure the JavaScript that consumes
owUpgradeOperationCancelUrl checks for an empty string and disables/hides the
cancel action when the URL is unavailable.
🪄 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: 08f53779-238e-444f-b7de-da0fbcb08409
📒 Files selected for processing (3)
openwisp_firmware_upgrader/admin.pyopenwisp_firmware_upgrader/templates/admin/firmware_upgrader/device_firmware_inline.htmlopenwisp_firmware_upgrader/tests/test_admin.py
📜 Review details
⏰ Context from checks skipped due to timeout. (12)
- GitHub Check: Python==3.13 | django~=5.2.0
- GitHub Check: Python==3.12 | django~=5.1.0
- GitHub Check: Python==3.10 | django~=4.2.0
- GitHub Check: Python==3.13 | django~=4.2.0
- GitHub Check: Python==3.12 | django~=4.2.0
- GitHub Check: Python==3.11 | django~=5.1.0
- GitHub Check: Python==3.13 | django~=5.1.0
- GitHub Check: Python==3.10 | django~=5.1.0
- GitHub Check: Python==3.11 | django~=4.2.0
- GitHub Check: Python==3.12 | django~=5.2.0
- GitHub Check: Python==3.11 | django~=5.2.0
- GitHub Check: Python==3.10 | django~=5.2.0
🧰 Additional context used
📓 Path-based instructions (2)
**/*.{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/templates/admin/firmware_upgrader/device_firmware_inline.htmlopenwisp_firmware_upgrader/tests/test_admin.pyopenwisp_firmware_upgrader/admin.py
**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
**/*.py: Place imports at the top of the file. Only defer imports when necessary (e.g., Django model imports inside functions or methods where the app registry is not yet ready)
Avoid unnecessary blank lines inside function and method bodies
Mark user-facing strings for translation with Django i18n helpers in Django code
Write comments and docstrings only when they explain why code is shaped a certain way. Put comments before the relevant code block instead of scattering them inside it
Files:
openwisp_firmware_upgrader/tests/test_admin.pyopenwisp_firmware_upgrader/admin.py
🧠 Learnings (4)
📚 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-06-02T08:16:00.439Z
Learnt from: asmodehn
Repo: openwisp/openwisp-firmware-upgrader PR: 362
File: tests/openwisp2/urls.py:10-38
Timestamp: 2026-06-02T08:16:00.439Z
Learning: In this repository, treat `SAMPLE_APP` as a conventionally truthy environment variable: code should use the established pattern `os.environ.get("SAMPLE_APP", False)` and rely on the resulting value’s truthiness (truthy string → enabled). During code reviews, avoid recommending changes to explicit boolean parsing (e.g., checking for `
Applied to files:
openwisp_firmware_upgrader/tests/test_admin.pyopenwisp_firmware_upgrader/admin.py
📚 Learning: 2026-06-15T20:19:28.326Z
Learnt from: atif09
Repo: openwisp/openwisp-firmware-upgrader PR: 437
File: openwisp_firmware_upgrader/tests/test_admin.py:1072-1147
Timestamp: 2026-06-15T20:19:28.326Z
Learning: In this codebase (OpenWISP projects, including openwisp-firmware-upgrader), prefer test methods that are fully self-contained: each test should do its own setup and make its own explicit assertions within the test body. Avoid introducing shared assertion helpers or parameterized helper methods that abstract away assertions across tests, even if it leads to some duplication.
Applied to files:
openwisp_firmware_upgrader/tests/test_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
🔇 Additional comments (3)
openwisp_firmware_upgrader/admin.py (1)
31-31: LGTM!Also applies to: 438-444
openwisp_firmware_upgrader/tests/test_admin.py (2)
30-30: LGTM!Also applies to: 1070-1098
1070-1098: 🩺 Stability & AvailabilityNo follow-up needed on this URL lookup. The remaining template reversal already falls back to an empty string when the URL name is unavailable, and the API routes are only included when
FIRMWARE_UPGRADER_APIis enabled.> Likely an incorrect or invalid review comment.
c51dd3a to
f9438f2
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
openwisp_firmware_upgrader/templates/admin/firmware_upgrader/device_firmware_inline.html (1)
8-8: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winPast review concerns still apply.
The previous review flagged three issues on this line that remain unresolved:
|default:''is redundant —{% url ... as cancel_url %}already stores""when reversal fails, so the default filter never changes the output.- Inconsistent strategy —
admin.pyexplicitly checksapp_settings.FIRMWARE_UPGRADER_API, while this template relies on Django's implicitNoReverseMatchsuppression. Consider passing the setting or a pre-resolved URL from Python context for consistency.- JS empty-URL risk — when the API is disabled,
owUpgradeOperationCancelUrlbecomes"". The downstreamcancelUpgradeOperationinupgrade-progress.jscallsowUpgradeOperationCancelUrl.replace(...)and POSTs the result, which would send the request to the current page URL. The cancel UI should be hidden or disabled when the URL is empty.♻️ Remove redundant default filter
-var owUpgradeOperationCancelUrl = "{% url 'upgrader:api_upgradeoperation_cancel' '00000000-0000-0000-0000-000000000000' as cancel_url %}{{ cancel_url|default:'' }}"; +var owUpgradeOperationCancelUrl = "{% url 'upgrader:api_upgradeoperation_cancel' '00000000-0000-0000-0000-000000000000' as cancel_url %}{{ cancel_url }}";🤖 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_firmware_upgrader/templates/admin/firmware_upgrader/device_firmware_inline.html` at line 8, The cancel URL setup in device_firmware_inline.html still has three issues: the {% url ... as cancel_url %} tag already yields an empty string so the default filter should be removed, the template should be made consistent with admin.py by passing or computing the API-enabled state and resolved cancel URL from Python context rather than relying on implicit NoReverseMatch handling, and the cancel action in upgrade-progress.js must not run when owUpgradeOperationCancelUrl is empty. Update the template and the related cancelUpgradeOperation flow so the cancel UI is hidden or disabled when the URL is unavailable, and reference owUpgradeOperationCancelUrl, cancelUpgradeOperation, and app_settings.FIRMWARE_UPGRADER_API while making the change.
🤖 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_firmware_upgrader/tests/test_admin.py`:
- Around line 1082-1092: The `test_device_change_page_api_disabled` case is only
a smoke test because toggling `app_settings.FIRMWARE_UPGRADER_API` does not
prevent `upgrader:api_upgradeoperation_cancel` from resolving, so the template
can still render `cancel_url` and a 200 response won’t catch regressions. Update
the test around `self.client.get(change_url)` to mock the URL resolver for that
named route to raise `NoReverseMatch`, or assert the rendered `cancel_url` value
directly from the `device_change` response, so the behavior of the `{% url ...
as cancel_url %}` path is actually verified.
---
Duplicate comments:
In
`@openwisp_firmware_upgrader/templates/admin/firmware_upgrader/device_firmware_inline.html`:
- Line 8: The cancel URL setup in device_firmware_inline.html still has three
issues: the {% url ... as cancel_url %} tag already yields an empty string so
the default filter should be removed, the template should be made consistent
with admin.py by passing or computing the API-enabled state and resolved cancel
URL from Python context rather than relying on implicit NoReverseMatch handling,
and the cancel action in upgrade-progress.js must not run when
owUpgradeOperationCancelUrl is empty. Update the template and the related
cancelUpgradeOperation flow so the cancel UI is hidden or disabled when the URL
is unavailable, and reference owUpgradeOperationCancelUrl,
cancelUpgradeOperation, and app_settings.FIRMWARE_UPGRADER_API while making the
change.
🪄 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: 6bb1eef2-6b23-4308-83ab-9d112c97875f
📒 Files selected for processing (3)
openwisp_firmware_upgrader/admin.pyopenwisp_firmware_upgrader/templates/admin/firmware_upgrader/device_firmware_inline.htmlopenwisp_firmware_upgrader/tests/test_admin.py
📜 Review details
⏰ Context from checks skipped due to timeout. (12)
- GitHub Check: Python==3.12 | django~=5.1.0
- GitHub Check: Python==3.11 | 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.13 | django~=5.1.0
- GitHub Check: Python==3.12 | django~=5.2.0
- GitHub Check: Python==3.10 | django~=4.2.0
- GitHub Check: Python==3.11 | django~=5.1.0
- GitHub Check: Python==3.13 | 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.12 | django~=4.2.0
🧰 Additional context used
📓 Path-based instructions (2)
**/*.{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/templates/admin/firmware_upgrader/device_firmware_inline.htmlopenwisp_firmware_upgrader/admin.pyopenwisp_firmware_upgrader/tests/test_admin.py
**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
**/*.py: Place imports at the top of the file. Only defer imports when necessary (e.g., Django model imports inside functions or methods where the app registry is not yet ready)
Avoid unnecessary blank lines inside function and method bodies
Mark user-facing strings for translation with Django i18n helpers in Django code
Write comments and docstrings only when they explain why code is shaped a certain way. Put comments before the relevant code block instead of scattering them inside it
Files:
openwisp_firmware_upgrader/admin.pyopenwisp_firmware_upgrader/tests/test_admin.py
🧠 Learnings (4)
📚 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-06-02T08:16:00.439Z
Learnt from: asmodehn
Repo: openwisp/openwisp-firmware-upgrader PR: 362
File: tests/openwisp2/urls.py:10-38
Timestamp: 2026-06-02T08:16:00.439Z
Learning: In this repository, treat `SAMPLE_APP` as a conventionally truthy environment variable: code should use the established pattern `os.environ.get("SAMPLE_APP", False)` and rely on the resulting value’s truthiness (truthy string → enabled). During code reviews, avoid recommending changes to explicit boolean parsing (e.g., checking for `
Applied to files:
openwisp_firmware_upgrader/admin.pyopenwisp_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-06-15T20:19:28.326Z
Learnt from: atif09
Repo: openwisp/openwisp-firmware-upgrader PR: 437
File: openwisp_firmware_upgrader/tests/test_admin.py:1072-1147
Timestamp: 2026-06-15T20:19:28.326Z
Learning: In this codebase (OpenWISP projects, including openwisp-firmware-upgrader), prefer test methods that are fully self-contained: each test should do its own setup and make its own explicit assertions within the test body. Avoid introducing shared assertion helpers or parameterized helper methods that abstract away assertions across tests, even if it leads to some duplication.
Applied to files:
openwisp_firmware_upgrader/tests/test_admin.py
🔇 Additional comments (4)
openwisp_firmware_upgrader/admin.py (2)
31-31: LGTM!
438-444: LGTM!openwisp_firmware_upgrader/tests/test_admin.py (2)
30-30: LGTM!
1070-1081: LGTM!
0acd394 to
c930598
Compare
23d8753 to
99499be
Compare
openwisp#444 Guard the reverse("upgrader:api_upgradeoperation_cancel") call in the admin and the {% url %} tag in the template behind FIRMWARE_UPGRADER_API. When the API is disabled, these gracefully return empty string instead of raising NoReverseMatch. test_upgrade_operation_change_view_api_disabled verifies the cancel URL context variable is an empty string. test_device_change_page_api_disabled verifies the admin page renders without crashing when the API is disabled. Closes openwisp#444
99499be to
5efe2d4
Compare
openwisp#444 Guard the reverse("upgrader:api_upgradeoperation_cancel") call in the admin and the {% url %} tag in the template behind FIRMWARE_UPGRADER_API. When the API is disabled, these gracefully return empty string instead of raising NoReverseMatch. test_upgrade_operation_change_view_api_disabled verifies the cancel URL context variable is an empty string. test_device_change_page_api_disabled verifies the admin page renders without crashing when the API is disabled and the cancel URL is empty. Closes openwisp#444
github-project-automation
Bot
moved this from To do (general)
to In progress
in OpenWISP Contributor's Board
github-project-automation
Bot
moved this to To do (general)
in OpenWISP Contributor's Board
|
@nemesifier please review when you are free thanks! |
|
@coderabbitai review |
✅ Action performedReview finished.
|
3 participants
Checklist
Reference to Existing Issue
Closes #444
Changes
Guard the
reverse("upgrader:api_upgradeoperation_cancel")call in the admin and the{% url %}tag in the template behindFIRMWARE_UPGRADER_API. When the API is disabled, the URL doesn't exist and these causeNoReverseMatch— now they gracefully return empty string instead of crashing.