[fix] Fix duplicate template entries in Device admin#1303
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:
📝 WalkthroughWalkthroughshowRelevantTemplates() now uses the specific container selector "#config-empty ul.sortedm2m-items" for inserting per-template prefix //ul[contains(@Class,"sortedm2m-items")]//input[@value="..."]). A new UI test TestDeviceAdmin.test_relevant_templates_duplicates creates two shared templates and verifies they render as two li.sortedm2m-item entries with expected checkbox ids, values, and data-required="false", then saves. A geo test builds the admin change URL dynamically from the model's app_label and model_name.
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Important Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional. ❌ Failed checks (1 inconclusive)
✅ Passed checks (2 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
|
Possible changelog entry Fixed an issue where relevant configuration templates could appear |
![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 the current code and only fix it if needed.
Inline comments:
In `@openwisp_controller/config/tests/test_selenium.py`:
- Around line 470-483: The test uses brittle string matching on
self.web_driver.page_source to assert checkbox HTML; replace this with DOM-based
assertions using Selenium APIs: for each template_id/idx in the loop, locate the
checkbox element (e.g., via find_element using a CSS selector combining
input[type="checkbox"] and value="{template_id}" or by id
"id_config-templates_{idx}") and then assert the relevant attributes (type,
value, id, class and data-required) via element.get_attribute(...) or
element.get_property(...); update imports to include
selenium.webdriver.common.by.By or WebDriverWait as needed and change the
assertions in the test method that currently references page_source to use these
DOM attribute checks on the found element.
🪄 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: e69f4e33-19ed-44f9-94d0-99b9f27c73f8
📒 Files selected for processing (2)
openwisp_controller/config/static/config/js/relevant_templates.jsopenwisp_controller/config/tests/test_selenium.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). (14)
- GitHub Check: Analyze (actions)
- GitHub Check: Analyze (javascript-typescript)
- GitHub Check: Analyze (python)
- GitHub Check: Python==3.13 | django~=5.1.0
- 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.2.0
- GitHub Check: Python==3.11 | django~=5.2.0
- GitHub Check: Python==3.12 | django~=5.1.0
- GitHub Check: Python==3.11 | 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~=4.2.0
- GitHub Check: Python==3.10 | django~=4.2.0
🧰 Additional context used
📓 Path-based instructions (4)
**/*.{js,ts,tsx,jsx,py,java,go,cs,rb,php,c,cpp,h,hpp}
📄 CodeRabbit inference engine (Custom checks)
**/*.{js,ts,tsx,jsx,py,java,go,cs,rb,php,c,cpp,h,hpp}: Flag potential security vulnerabilities in code
Avoid unnecessary comments or docstrings for code that is already clear
Code formatting is compact and readable. Do not add excessive blank lines, especially inside function or method bodies
Flag unused or redundant code
Ensure variables, functions, classes, and files have descriptive and consistent names
New code must handle errors properly: log errors that cannot be resolved by the user with error level, log unusual conditions with warning level, log important background actions with info level, and provide user-facing messages for errors that the user can solve autonomously
Files:
openwisp_controller/config/static/config/js/relevant_templates.jsopenwisp_controller/config/tests/test_selenium.py
**/*.{js,ts,tsx,jsx,py,java,go,cs,rb,php,c,cpp,h,hpp,sql}
📄 CodeRabbit inference engine (Custom checks)
Flag obvious performance regressions, such as heavy loops, repeated I/O, or unoptimized queries
Files:
openwisp_controller/config/static/config/js/relevant_templates.jsopenwisp_controller/config/tests/test_selenium.py
**/*.{js,ts,tsx,jsx,py,java,go,cs,rb,php,c,cpp,h,hpp,sh,bash,sql}
📄 CodeRabbit inference engine (Custom checks)
Cryptic or non-obvious code (regex, complex bash commands, or hard-to-read code) must include a concise comment explaining why it is needed and why the complexity is acceptable
Files:
openwisp_controller/config/static/config/js/relevant_templates.jsopenwisp_controller/config/tests/test_selenium.py
**/*.{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_controller/config/tests/test_selenium.py
🧠 Learnings (4)
📓 Common learnings
Learnt from: CR
Repo: openwisp/openwisp-controller PR: 0
File: coderabbit-custom-pre-merge-checks-unique-id-file-non-traceable-F7F2B60C-1728-4C9A-8889-4F2235E186CA.txt:0-0
Timestamp: 2026-03-14T19:34:33.773Z
Learning: Applies to **/*{.test,.spec,_test,test_}.{js,ts,tsx,jsx,py,java,go,cs,rb} : Tests must be updated to cover non-trivial changes and ensure proper validation of modified behavior
Learnt from: CR
Repo: openwisp/openwisp-controller PR: 0
File: coderabbit-custom-pre-merge-checks-unique-id-file-non-traceable-F7F2B60C-1728-4C9A-8889-4F2235E186CA.txt:0-0
Timestamp: 2026-03-14T19:34:33.773Z
Learning: Applies to **/*{.test,.spec,_test,test_}.{js,ts,tsx,jsx,py,java,go,cs,rb} **/*.{e2e,browser,selenium}.{js,ts,tsx,jsx} : For feature requests, tests are added and coverage does not decrease significantly. If the feature impacts the UI, Selenium browser tests are preferred
📚 Learning: 2026-01-15T15:05:49.557Z
Learnt from: DragnEmperor
Repo: openwisp/openwisp-controller PR: 1175
File: openwisp_controller/config/management/commands/clear_last_ip.py:38-42
Timestamp: 2026-01-15T15:05:49.557Z
Learning: In Django projects, when using select_related() to traverse relations (for example, select_related("organization__config_settings")), the traversed relation must not be deferred. If you also use .only() in the same query, include the relation name or FK field (e.g., "organization" or "organization_id") in the .only() list to avoid the error "Field X cannot be both deferred and traversed using select_related at the same time." Apply this guideline to Django code in openwisp_controller/config/management/commands/clear_last_ip.py and similar modules by ensuring any select_related with an accompanying only() includes the related field names to prevent deferred/traversed conflicts.
Applied to files:
openwisp_controller/config/tests/test_selenium.py
📚 Learning: 2026-02-17T19:13:10.088Z
Learnt from: nemesifier
Repo: openwisp/openwisp-controller PR: 1175
File: openwisp_controller/config/whois/commands.py:0-0
Timestamp: 2026-02-17T19:13:10.088Z
Learning: In reviews for the openwisp/openwisp-controller repository, do not propose changes based on Ruff warnings. The project does not use Ruff as its linter; ignore Ruff-related suggestions and follow the repository’s established linting and configuration rules. This guidance applies to all Python files under the openwisp_controller directory.
Applied to files:
openwisp_controller/config/tests/test_selenium.py
📚 Learning: 2026-01-15T15:07:17.354Z
Learnt from: DragnEmperor
Repo: openwisp/openwisp-controller PR: 1175
File: openwisp_controller/geo/estimated_location/tests/tests.py:172-175
Timestamp: 2026-01-15T15:07:17.354Z
Learning: In this repository, flake8 enforces E501 (line too long) via setup.cfg (max-line-length = 88) while ruff ignores E501 via ruff.toml. Therefore, use '# noqa: E501' on lines that intentionally exceed 88 characters to satisfy flake8 without affecting ruff checks. This applies to Python files across the project (any .py) and is relevant for tests as well. Use sparingly and only where breaking lines is not feasible without hurting readability or functionality.
Applied to files:
openwisp_controller/config/tests/test_selenium.py
🧬 Code graph analysis (1)
openwisp_controller/config/tests/test_selenium.py (2)
openwisp_controller/config/tests/utils.py (3)
_create_template(77-87)_create_config(60-73)_create_config(396-401)openwisp_controller/geo/tests/test_selenium.py (1)
wait_for_presence(43-44)
🔇 Additional comments (2)
openwisp_controller/config/static/config/js/relevant_templates.js (1)
141-141: Selector fix correctly removes duplicate append risk.Targeting
#config-empty ul.sortedm2m-itemsmakes the insertion point deterministic and prevents the double-append behavior when the empty inline is not rendered.openwisp_controller/config/tests/test_selenium.py (1)
49-59: Scoped template visibility locators are a solid improvement.Using the
ul.sortedm2m-items-scoped XPath makes these checks less error-prone than broad page-level value matching.
github-project-automation
bot
moved this from Needs review
to In progress
in OpenWISP Contributor's Board
pandafy
force-pushed
the
fix-relevant-templates
branch
from
89bdba1 to
7ea2118
Compare
Flake8 E501 Line Too Long FailuresHello @pandafy, The CI failed because several lines in the Fix:
|
There was a problem hiding this comment.
♻️ Duplicate comments (1)
openwisp_controller/config/tests/test_selenium.py (1)
468-487:⚠️ Potential issue | 🟠 MajorReplace brittle
page_sourceHTML matching with DOM-level assertions.The current check depends on serialized HTML text shape (including attribute order), which is flaky across browsers/drivers and can fail even when behavior is correct.
Proposed fix
- with self.subTest( - "Verify checkbox HTML elements are present in page source" - " with correct attributes" - ): - page_source = self.web_driver.page_source - # Verify HTML element strings for each template - for idx, template_id in enumerate([template1.id, template2.id], start=0): - html_element = ( - f'<input type="checkbox" value="{template_id}" ' - f'id="id_config-templates_{idx}" class="sortedm2m"' - ' data-required="false"' - ) - self.assertIn( - html_element, - page_source, - ( - f"Expected checkbox HTML for template {template_id} not found" - " in page source" - ), - ) + with self.subTest("Verify checkbox inputs have expected attributes"): + for idx, template in enumerate([template1, template2]): + checkbox = self.find_element( + by=By.ID, value=f"id_config-templates_{idx}" + ) + self.assertEqual(checkbox.get_attribute("value"), str(template.id)) + self.assertEqual(checkbox.get_attribute("data-required"), "false")#!/bin/bash # Verify whether page_source-based HTML assertions still exist in selenium tests. rg -n -C3 'page_source|html_element|assertIn\(' --type=pyBased on learnings: Tests must be updated to cover non-trivial changes and ensure proper validation of modified behavior.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@openwisp_controller/config/tests/test_selenium.py` around lines 468 - 487, Replace the brittle page_source string match in the subTest block: instead of building html_element and using page_source / self.assertIn, query the DOM via the Selenium WebDriver (self.web_driver) for checkbox inputs corresponding to each template (use value==template_id and id==f"id_config-templates_{idx}" or class/specific attributes) and assert presence/count using find_element(s) or appropriate By selectors; remove reliance on the page_source variable and the html_element string and update the assertions that currently call self.assertIn to check the found element(s) exist and have the expected attributes (e.g., type="checkbox", class contains "sortedm2m", data-required="false") for template1.id and template2.id.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@openwisp_controller/config/tests/test_selenium.py`:
- Around line 468-487: Replace the brittle page_source string match in the
subTest block: instead of building html_element and using page_source /
self.assertIn, query the DOM via the Selenium WebDriver (self.web_driver) for
checkbox inputs corresponding to each template (use value==template_id and
id==f"id_config-templates_{idx}" or class/specific attributes) and assert
presence/count using find_element(s) or appropriate By selectors; remove
reliance on the page_source variable and the html_element string and update the
assertions that currently call self.assertIn to check the found element(s) exist
and have the expected attributes (e.g., type="checkbox", class contains
"sortedm2m", data-required="false") for template1.id and template2.id.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 9ae9485e-3d78-4084-9dae-d09dc6c14147
📒 Files selected for processing (2)
openwisp_controller/config/static/config/js/relevant_templates.jsopenwisp_controller/config/tests/test_selenium.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). (11)
- GitHub Check: Python==3.10 | django~=5.2.0
- 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.11 | django~=4.2.0
- GitHub Check: Python==3.12 | django~=5.2.0
- GitHub Check: Python==3.10 | django~=5.1.0
- GitHub Check: Python==3.11 | django~=5.1.0
- GitHub Check: Python==3.13 | django~=5.1.0
- GitHub Check: Python==3.12 | django~=4.2.0
- GitHub Check: Python==3.11 | django~=5.2.0
🧰 Additional context used
📓 Path-based instructions (4)
**/*.{js,ts,tsx,jsx,py,java,go,cs,rb,php,c,cpp,h,hpp}
📄 CodeRabbit inference engine (Custom checks)
**/*.{js,ts,tsx,jsx,py,java,go,cs,rb,php,c,cpp,h,hpp}: Flag potential security vulnerabilities in code
Avoid unnecessary comments or docstrings for code that is already clear
Code formatting is compact and readable. Do not add excessive blank lines, especially inside function or method bodies
Flag unused or redundant code
Ensure variables, functions, classes, and files have descriptive and consistent names
New code must handle errors properly: log errors that cannot be resolved by the user with error level, log unusual conditions with warning level, log important background actions with info level, and provide user-facing messages for errors that the user can solve autonomously
Files:
openwisp_controller/config/static/config/js/relevant_templates.jsopenwisp_controller/config/tests/test_selenium.py
**/*.{js,ts,tsx,jsx,py,java,go,cs,rb,php,c,cpp,h,hpp,sql}
📄 CodeRabbit inference engine (Custom checks)
Flag obvious performance regressions, such as heavy loops, repeated I/O, or unoptimized queries
Files:
openwisp_controller/config/static/config/js/relevant_templates.jsopenwisp_controller/config/tests/test_selenium.py
**/*.{js,ts,tsx,jsx,py,java,go,cs,rb,php,c,cpp,h,hpp,sh,bash,sql}
📄 CodeRabbit inference engine (Custom checks)
Cryptic or non-obvious code (regex, complex bash commands, or hard-to-read code) must include a concise comment explaining why it is needed and why the complexity is acceptable
Files:
openwisp_controller/config/static/config/js/relevant_templates.jsopenwisp_controller/config/tests/test_selenium.py
**/*.{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_controller/config/tests/test_selenium.py
🧠 Learnings (5)
📚 Learning: 2026-03-14T19:34:33.773Z
Learnt from: CR
Repo: openwisp/openwisp-controller PR: 0
File: coderabbit-custom-pre-merge-checks-unique-id-file-non-traceable-F7F2B60C-1728-4C9A-8889-4F2235E186CA.txt:0-0
Timestamp: 2026-03-14T19:34:33.773Z
Learning: Applies to **/*.{e2e,browser,selenium}.{js,ts,tsx,jsx} : If a bug affects the user interface, include a Selenium browser test; if missing, raise a warning
Applied to files:
openwisp_controller/config/tests/test_selenium.py
📚 Learning: 2026-03-14T19:34:33.773Z
Learnt from: CR
Repo: openwisp/openwisp-controller PR: 0
File: coderabbit-custom-pre-merge-checks-unique-id-file-non-traceable-F7F2B60C-1728-4C9A-8889-4F2235E186CA.txt:0-0
Timestamp: 2026-03-14T19:34:33.773Z
Learning: Applies to **/*{.test,.spec,_test,test_}.{js,ts,tsx,jsx,py,java,go,cs,rb} : Tests must be updated to cover non-trivial changes and ensure proper validation of modified behavior
Applied to files:
openwisp_controller/config/tests/test_selenium.py
📚 Learning: 2026-01-15T15:05:49.557Z
Learnt from: DragnEmperor
Repo: openwisp/openwisp-controller PR: 1175
File: openwisp_controller/config/management/commands/clear_last_ip.py:38-42
Timestamp: 2026-01-15T15:05:49.557Z
Learning: In Django projects, when using select_related() to traverse relations (for example, select_related("organization__config_settings")), the traversed relation must not be deferred. If you also use .only() in the same query, include the relation name or FK field (e.g., "organization" or "organization_id") in the .only() list to avoid the error "Field X cannot be both deferred and traversed using select_related at the same time." Apply this guideline to Django code in openwisp_controller/config/management/commands/clear_last_ip.py and similar modules by ensuring any select_related with an accompanying only() includes the related field names to prevent deferred/traversed conflicts.
Applied to files:
openwisp_controller/config/tests/test_selenium.py
📚 Learning: 2026-02-17T19:13:10.088Z
Learnt from: nemesifier
Repo: openwisp/openwisp-controller PR: 1175
File: openwisp_controller/config/whois/commands.py:0-0
Timestamp: 2026-02-17T19:13:10.088Z
Learning: In reviews for the openwisp/openwisp-controller repository, do not propose changes based on Ruff warnings. The project does not use Ruff as its linter; ignore Ruff-related suggestions and follow the repository’s established linting and configuration rules. This guidance applies to all Python files under the openwisp_controller directory.
Applied to files:
openwisp_controller/config/tests/test_selenium.py
📚 Learning: 2026-01-15T15:07:17.354Z
Learnt from: DragnEmperor
Repo: openwisp/openwisp-controller PR: 1175
File: openwisp_controller/geo/estimated_location/tests/tests.py:172-175
Timestamp: 2026-01-15T15:07:17.354Z
Learning: In this repository, flake8 enforces E501 (line too long) via setup.cfg (max-line-length = 88) while ruff ignores E501 via ruff.toml. Therefore, use '# noqa: E501' on lines that intentionally exceed 88 characters to satisfy flake8 without affecting ruff checks. This applies to Python files across the project (any .py) and is relevant for tests as well. Use sparingly and only where breaking lines is not feasible without hurting readability or functionality.
Applied to files:
openwisp_controller/config/tests/test_selenium.py
🧬 Code graph analysis (1)
openwisp_controller/config/tests/test_selenium.py (1)
openwisp_controller/config/tests/utils.py (3)
_create_template(77-87)_create_config(60-73)_create_config(396-401)
🔇 Additional comments (1)
openwisp_controller/config/static/config/js/relevant_templates.js (1)
140-142: Selector fix correctly removes the duplicate-template root cause.Switching to
#config-empty ul.sortedm2m-itemsremoves DOM-order dependency and safely avoids duplicate append behavior when the empty inline is not rendered.
|
Kilo Code Review could not run — your account is out of credits. Add credits at app.kilo.ai to enable reviews on this change. |
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_controller/config/tests/test_selenium.py`:
- Around line 444-446: The test opens the admin device change URL with a
hardcoded "admin:config_device_change" but should use the class attribute for
app label consistency; update the call in the test method (the one that invokes
self.open(...)) to build the URL with
f"admin:{self.config_app_label}_device_change" (using reverse with
args=[device.id] and the same "#config-group" fragment) so it matches other
tests that reference self.config_app_label.
🪄 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: 3d82e577-a83c-478d-b910-232843546d87
📒 Files selected for processing (1)
openwisp_controller/config/tests/test_selenium.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). (11)
- GitHub Check: Python==3.13 | django~=5.2.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~=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.12 | django~=5.2.0
- GitHub Check: Python==3.10 | django~=4.2.0
- GitHub Check: Python==3.12 | django~=4.2.0
- GitHub Check: Python==3.11 | django~=5.2.0
- GitHub Check: Python==3.10 | django~=5.1.0
🧰 Additional context used
📓 Path-based instructions (4)
**/*.{js,ts,tsx,jsx,py,java,go,cs,rb,php,c,cpp,h,hpp}
📄 CodeRabbit inference engine (Custom checks)
**/*.{js,ts,tsx,jsx,py,java,go,cs,rb,php,c,cpp,h,hpp}: Flag potential security vulnerabilities in code
Avoid unnecessary comments or docstrings for code that is already clear
Code formatting is compact and readable. Do not add excessive blank lines, especially inside function or method bodies
Flag unused or redundant code
Ensure variables, functions, classes, and files have descriptive and consistent names
New code must handle errors properly: log errors that cannot be resolved by the user with error level, log unusual conditions with warning level, log important background actions with info level, and provide user-facing messages for errors that the user can solve autonomously
Files:
openwisp_controller/config/tests/test_selenium.py
**/*.{js,ts,tsx,jsx,py,java,go,cs,rb,php,c,cpp,h,hpp,sql}
📄 CodeRabbit inference engine (Custom checks)
Flag obvious performance regressions, such as heavy loops, repeated I/O, or unoptimized queries
Files:
openwisp_controller/config/tests/test_selenium.py
**/*.{js,ts,tsx,jsx,py,java,go,cs,rb,php,c,cpp,h,hpp,sh,bash,sql}
📄 CodeRabbit inference engine (Custom checks)
Cryptic or non-obvious code (regex, complex bash commands, or hard-to-read code) must include a concise comment explaining why it is needed and why the complexity is acceptable
Files:
openwisp_controller/config/tests/test_selenium.py
**/*.{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_controller/config/tests/test_selenium.py
🧠 Learnings (5)
📚 Learning: 2026-03-14T19:34:33.773Z
Learnt from: CR
Repo: openwisp/openwisp-controller PR: 0
File: coderabbit-custom-pre-merge-checks-unique-id-file-non-traceable-F7F2B60C-1728-4C9A-8889-4F2235E186CA.txt:0-0
Timestamp: 2026-03-14T19:34:33.773Z
Learning: Applies to **/*.{e2e,browser,selenium}.{js,ts,tsx,jsx} : If a bug affects the user interface, include a Selenium browser test; if missing, raise a warning
Applied to files:
openwisp_controller/config/tests/test_selenium.py
📚 Learning: 2026-03-14T19:34:33.773Z
Learnt from: CR
Repo: openwisp/openwisp-controller PR: 0
File: coderabbit-custom-pre-merge-checks-unique-id-file-non-traceable-F7F2B60C-1728-4C9A-8889-4F2235E186CA.txt:0-0
Timestamp: 2026-03-14T19:34:33.773Z
Learning: Applies to **/*{.test,.spec,_test,test_}.{js,ts,tsx,jsx,py,java,go,cs,rb} : Tests must be updated to cover non-trivial changes and ensure proper validation of modified behavior
Applied to files:
openwisp_controller/config/tests/test_selenium.py
📚 Learning: 2026-01-15T15:05:49.557Z
Learnt from: DragnEmperor
Repo: openwisp/openwisp-controller PR: 1175
File: openwisp_controller/config/management/commands/clear_last_ip.py:38-42
Timestamp: 2026-01-15T15:05:49.557Z
Learning: In Django projects, when using select_related() to traverse relations (for example, select_related("organization__config_settings")), the traversed relation must not be deferred. If you also use .only() in the same query, include the relation name or FK field (e.g., "organization" or "organization_id") in the .only() list to avoid the error "Field X cannot be both deferred and traversed using select_related at the same time." Apply this guideline to Django code in openwisp_controller/config/management/commands/clear_last_ip.py and similar modules by ensuring any select_related with an accompanying only() includes the related field names to prevent deferred/traversed conflicts.
Applied to files:
openwisp_controller/config/tests/test_selenium.py
📚 Learning: 2026-02-17T19:13:10.088Z
Learnt from: nemesifier
Repo: openwisp/openwisp-controller PR: 1175
File: openwisp_controller/config/whois/commands.py:0-0
Timestamp: 2026-02-17T19:13:10.088Z
Learning: In reviews for the openwisp/openwisp-controller repository, do not propose changes based on Ruff warnings. The project does not use Ruff as its linter; ignore Ruff-related suggestions and follow the repository’s established linting and configuration rules. This guidance applies to all Python files under the openwisp_controller directory.
Applied to files:
openwisp_controller/config/tests/test_selenium.py
📚 Learning: 2026-01-15T15:07:17.354Z
Learnt from: DragnEmperor
Repo: openwisp/openwisp-controller PR: 1175
File: openwisp_controller/geo/estimated_location/tests/tests.py:172-175
Timestamp: 2026-01-15T15:07:17.354Z
Learning: In this repository, flake8 enforces E501 (line too long) via setup.cfg (max-line-length = 88) while ruff ignores E501 via ruff.toml. Therefore, use '# noqa: E501' on lines that intentionally exceed 88 characters to satisfy flake8 without affecting ruff checks. This applies to Python files across the project (any .py) and is relevant for tests as well. Use sparingly and only where breaking lines is not feasible without hurting readability or functionality.
Applied to files:
openwisp_controller/config/tests/test_selenium.py
🧬 Code graph analysis (1)
openwisp_controller/config/tests/test_selenium.py (2)
openwisp_controller/config/tests/utils.py (3)
_create_template(77-87)_create_config(60-73)_create_config(396-401)openwisp_controller/geo/tests/test_selenium.py (1)
wait_for_presence(43-44)
🔇 Additional comments (4)
openwisp_controller/config/tests/test_selenium.py (4)
4-4: LGTM!The import addition is appropriate for the new test that creates a custom permission group.
45-59: LGTM - scoped selectors improve test reliability.The updated XPath selectors correctly scope template checkbox lookups to the
ul.sortedm2m-itemscontainer, preventing false matches when multiple elements share the same value attribute. This aligns well with the JS fix targeting the specific sortedm2m container.
467-475: DOM-based attribute verification is well-implemented.The test correctly uses Selenium's
find_elementandget_attributemethods to verify checkbox properties, addressing the prior review feedback about avoiding brittlepage_sourcestring matching.Note: The loop assumes template ordering by creation order. If the sortedm2m widget reorders templates (e.g., default templates first), the
idxmapping may not align. Consider verifying by value lookup if ordering becomes inconsistent.
401-481: Good test coverage for the duplicate templates bug.This test effectively validates the fix by:
- Setting up a limited-permission user (the bug's trigger condition)
- Creating shared templates that would previously appear duplicated
- Asserting exactly 2 items exist in the sortedm2m list (line 458-465)
- Verifying DOM attributes match expected values
The test aligns with the retrieved learning that UI bugs should include Selenium browser tests.
Test Failure in
|
Bug: The JS logic in relevant_templates.js assumed that the last `ul.sortedm2m-items` element belonged to the empty inline form used by Django admin formsets. This assumption breaks when the user does not have permission to add Config objects: in that case the ConfigInlineAdmin does not render the empty form. As a result, both selectors ended up referencing the same list and the script appended template elements twice to the same `sortedm2m` list, causing duplicate entries and issues when saving the form. Fix: Select the empty form container explicitly using `#config-empty ul.sortedm2m-items` instead of relying on the last occurrence of `ul.sortedm2m-items`. This ensures the logic works correctly regardless of whether the empty inline form is rendered. [backport 1.2]
pandafy
force-pushed
the
fix-relevant-templates
branch
from
755cf1e to
ed364bc
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
openwisp_controller/config/tests/test_selenium.py (1)
444-446:⚠️ Potential issue | 🟡 MinorUse
self.config_app_labelfor URL pattern consistency.Other tests in this class use
f"admin:{self.config_app_label}_device_change"(e.g., lines 72, 213, 278), but this test hardcodes"admin:config_device_change". For consistency and to support downstream projects that extend and reuse this test suite with custom app labels, use the class attribute.🔧 Proposed fix
self.open( - reverse("admin:config_device_change", args=[device.id]) + "#config-group" + reverse(f"admin:{self.config_app_label}_device_change", args=[device.id]) + "#config-group" )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@openwisp_controller/config/tests/test_selenium.py` around lines 444 - 446, Replace the hardcoded admin URL pattern "admin:config_device_change" with the class attribute-based pattern that uses self.config_app_label so the test uses the same dynamic app label as other tests; update the call to reverse (the reverse(...) invocation in the test) to construct the pattern using f"admin:{self.config_app_label}_device_change" and keep the existing args and fragment ("#config-group") intact.
🤖 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_controller/geo/tests/test_selenium.py`:
- Around line 94-97: The admin URL is reversed with the wrong app label (using
self.app_label which is "geo"), causing a NoReverseMatch; change the reverse
call to use the Device's actual admin app label ("config") or derive it from the
model (e.g., self.object_model._meta.app_label or the device instance's
_meta.app_label) so the pattern becomes "admin:config_<modelname>_change" when
calling reverse with args=[ol.device.id]; update the call that constructs path
(currently using
reverse(f"admin:{self.app_label}_{self.object_model._meta.model_name}_change",
args=[ol.device.id])) to use the correct app_label.
---
Duplicate comments:
In `@openwisp_controller/config/tests/test_selenium.py`:
- Around line 444-446: Replace the hardcoded admin URL pattern
"admin:config_device_change" with the class attribute-based pattern that uses
self.config_app_label so the test uses the same dynamic app label as other
tests; update the call to reverse (the reverse(...) invocation in the test) to
construct the pattern using f"admin:{self.config_app_label}_device_change" and
keep the existing args and fragment ("#config-group") intact.
🪄 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: bc1c75dc-a2ca-4694-ae30-867b759002d0
📒 Files selected for processing (3)
openwisp_controller/config/static/config/js/relevant_templates.jsopenwisp_controller/config/tests/test_selenium.pyopenwisp_controller/geo/tests/test_selenium.py
📜 Review details
🧰 Additional context used
📓 Path-based instructions (4)
**/*.{js,ts,tsx,jsx,py,java,go,cs,rb,php,c,cpp,h,hpp}
📄 CodeRabbit inference engine (Custom checks)
**/*.{js,ts,tsx,jsx,py,java,go,cs,rb,php,c,cpp,h,hpp}: Flag potential security vulnerabilities in code
Avoid unnecessary comments or docstrings for code that is already clear
Code formatting is compact and readable. Do not add excessive blank lines, especially inside function or method bodies
Flag unused or redundant code
Ensure variables, functions, classes, and files have descriptive and consistent names
New code must handle errors properly: log errors that cannot be resolved by the user with error level, log unusual conditions with warning level, log important background actions with info level, and provide user-facing messages for errors that the user can solve autonomously
Files:
openwisp_controller/geo/tests/test_selenium.pyopenwisp_controller/config/static/config/js/relevant_templates.jsopenwisp_controller/config/tests/test_selenium.py
**/*.{js,ts,tsx,jsx,py,java,go,cs,rb,php,c,cpp,h,hpp,sql}
📄 CodeRabbit inference engine (Custom checks)
Flag obvious performance regressions, such as heavy loops, repeated I/O, or unoptimized queries
Files:
openwisp_controller/geo/tests/test_selenium.pyopenwisp_controller/config/static/config/js/relevant_templates.jsopenwisp_controller/config/tests/test_selenium.py
**/*.{js,ts,tsx,jsx,py,java,go,cs,rb,php,c,cpp,h,hpp,sh,bash,sql}
📄 CodeRabbit inference engine (Custom checks)
Cryptic or non-obvious code (regex, complex bash commands, or hard-to-read code) must include a concise comment explaining why it is needed and why the complexity is acceptable
Files:
openwisp_controller/geo/tests/test_selenium.pyopenwisp_controller/config/static/config/js/relevant_templates.jsopenwisp_controller/config/tests/test_selenium.py
**/*.{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_controller/geo/tests/test_selenium.pyopenwisp_controller/config/tests/test_selenium.py
🧠 Learnings (6)
📚 Learning: 2026-01-12T22:27:48.342Z
Learnt from: nemesifier
Repo: openwisp/openwisp-controller PR: 1175
File: tests/openwisp2/sample_config/migrations/0008_whoisinfo_organizationconfigsettings_whois_enabled.py:18-67
Timestamp: 2026-01-12T22:27:48.342Z
Learning: In tests/openwisp2/sample_config/models.py and corresponding test migrations, the WHOISInfo model intentionally includes an additional "details" field not present in the base AbstractWHOISInfo model. This is a testing pattern to verify that swappable models (CONFIG_WHOISINFO_MODEL) can be extended with custom fields without errors.
Applied to files:
openwisp_controller/geo/tests/test_selenium.py
📚 Learning: 2026-01-15T15:05:49.557Z
Learnt from: DragnEmperor
Repo: openwisp/openwisp-controller PR: 1175
File: openwisp_controller/config/management/commands/clear_last_ip.py:38-42
Timestamp: 2026-01-15T15:05:49.557Z
Learning: In Django projects, when using select_related() to traverse relations (for example, select_related("organization__config_settings")), the traversed relation must not be deferred. If you also use .only() in the same query, include the relation name or FK field (e.g., "organization" or "organization_id") in the .only() list to avoid the error "Field X cannot be both deferred and traversed using select_related at the same time." Apply this guideline to Django code in openwisp_controller/config/management/commands/clear_last_ip.py and similar modules by ensuring any select_related with an accompanying only() includes the related field names to prevent deferred/traversed conflicts.
Applied to files:
openwisp_controller/geo/tests/test_selenium.pyopenwisp_controller/config/tests/test_selenium.py
📚 Learning: 2026-02-17T19:13:10.088Z
Learnt from: nemesifier
Repo: openwisp/openwisp-controller PR: 1175
File: openwisp_controller/config/whois/commands.py:0-0
Timestamp: 2026-02-17T19:13:10.088Z
Learning: In reviews for the openwisp/openwisp-controller repository, do not propose changes based on Ruff warnings. The project does not use Ruff as its linter; ignore Ruff-related suggestions and follow the repository’s established linting and configuration rules. This guidance applies to all Python files under the openwisp_controller directory.
Applied to files:
openwisp_controller/geo/tests/test_selenium.pyopenwisp_controller/config/tests/test_selenium.py
📚 Learning: 2026-01-15T15:07:17.354Z
Learnt from: DragnEmperor
Repo: openwisp/openwisp-controller PR: 1175
File: openwisp_controller/geo/estimated_location/tests/tests.py:172-175
Timestamp: 2026-01-15T15:07:17.354Z
Learning: In this repository, flake8 enforces E501 (line too long) via setup.cfg (max-line-length = 88) while ruff ignores E501 via ruff.toml. Therefore, use '# noqa: E501' on lines that intentionally exceed 88 characters to satisfy flake8 without affecting ruff checks. This applies to Python files across the project (any .py) and is relevant for tests as well. Use sparingly and only where breaking lines is not feasible without hurting readability or functionality.
Applied to files:
openwisp_controller/geo/tests/test_selenium.pyopenwisp_controller/config/tests/test_selenium.py
📚 Learning: 2026-03-14T19:34:33.773Z
Learnt from: CR
Repo: openwisp/openwisp-controller PR: 0
File: coderabbit-custom-pre-merge-checks-unique-id-file-non-traceable-F7F2B60C-1728-4C9A-8889-4F2235E186CA.txt:0-0
Timestamp: 2026-03-14T19:34:33.773Z
Learning: Applies to **/*.{e2e,browser,selenium}.{js,ts,tsx,jsx} : If a bug affects the user interface, include a Selenium browser test; if missing, raise a warning
Applied to files:
openwisp_controller/config/tests/test_selenium.py
📚 Learning: 2026-03-14T19:34:33.773Z
Learnt from: CR
Repo: openwisp/openwisp-controller PR: 0
File: coderabbit-custom-pre-merge-checks-unique-id-file-non-traceable-F7F2B60C-1728-4C9A-8889-4F2235E186CA.txt:0-0
Timestamp: 2026-03-14T19:34:33.773Z
Learning: Applies to **/*{.test,.spec,_test,test_}.{js,ts,tsx,jsx,py,java,go,cs,rb} : Tests must be updated to cover non-trivial changes and ensure proper validation of modified behavior
Applied to files:
openwisp_controller/config/tests/test_selenium.py
🔇 Additional comments (4)
openwisp_controller/config/static/config/js/relevant_templates.js (1)
140-141: LGTM! The selector fix correctly targets the empty form container.Changing from
:lastto#config-empty ul.sortedm2m-itemsensures prefix elements are only appended to the empty inline form when it exists. When users lack add permission and#config-emptyisn't rendered, the jQuery selector returns an empty set andappend()becomes a no-op, preventing duplicates.openwisp_controller/config/tests/test_selenium.py (3)
4-4: LGTM!Import addition is required for the new test that creates a custom permission group.
45-59: LGTM! Scoped XPath improves test reliability.The updated XPath
//ul[contains(@Class,"sortedm2m-items")]//input[@value="..."]correctly scopes checkbox lookups to the sortedm2m widget container, preventing false matches from other page elements and aligning with the JS fix.
401-481: Good test coverage for the duplicate templates bug.The test properly validates the fix by:
- Creating a user without
add_configpermission (the trigger condition)- Verifying exactly 2 template items in the sortedm2m list (no duplicates)
- Using DOM-based attribute assertions for checkbox verification
- Confirming the save operation completes successfully
Test Failures in OpenWISP Controller CIHello @pandafy,
|
pandafy
force-pushed
the
fix-relevant-templates
branch
from
ed364bc to
83f4005
Compare
Test Failures DetectedHello @pandafy,
|
|
@coderabbitai review! |
|
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
♻️ Duplicate comments (1)
openwisp_controller/config/tests/test_selenium.py (1)
444-446:⚠️ Potential issue | 🟠 MajorUse
self.config_app_labelhere instead of hardcodingconfig.This reintroduces app-label coupling in a test path that should stay swappable for downstream integrations.
🔧 Proposed fix
self.open( - reverse("admin:config_device_change", args=[device.id]) + "#config-group" + reverse( + f"admin:{self.config_app_label}_device_change", args=[device.id] + ) + + "#config-group" )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@openwisp_controller/config/tests/test_selenium.py` around lines 444 - 446, The test currently hardcodes the app label in the admin URL reverse call (reverse("admin:config_device_change", ...)), which breaks downstream swappability; change the URL name to use the instance attribute self.config_app_label so the call becomes reverse(f"admin:{self.config_app_label}_device_change", args=[device.id]) (i.e., replace the literal "config" with self.config_app_label) in the test method that calls self.open().
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@openwisp_controller/config/tests/test_selenium.py`:
- Around line 444-446: The test currently hardcodes the app label in the admin
URL reverse call (reverse("admin:config_device_change", ...)), which breaks
downstream swappability; change the URL name to use the instance attribute
self.config_app_label so the call becomes
reverse(f"admin:{self.config_app_label}_device_change", args=[device.id]) (i.e.,
replace the literal "config" with self.config_app_label) in the test method that
calls self.open().
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 2d1884c6-cbc1-4351-8115-b497462f0e21
📒 Files selected for processing (3)
openwisp_controller/config/static/config/js/relevant_templates.jsopenwisp_controller/config/tests/test_selenium.pyopenwisp_controller/geo/tests/test_selenium.py
📜 Review details
🧰 Additional context used
📓 Path-based instructions (4)
**/*.{js,ts,tsx,jsx,py,java,go,cs,rb,php,c,cpp,h,hpp}
📄 CodeRabbit inference engine (Custom checks)
**/*.{js,ts,tsx,jsx,py,java,go,cs,rb,php,c,cpp,h,hpp}: Flag potential security vulnerabilities in code
Avoid unnecessary comments or docstrings for code that is already clear
Code formatting is compact and readable. Do not add excessive blank lines, especially inside function or method bodies
Flag unused or redundant code
Ensure variables, functions, classes, and files have descriptive and consistent names
New code must handle errors properly: log errors that cannot be resolved by the user with error level, log unusual conditions with warning level, log important background actions with info level, and provide user-facing messages for errors that the user can solve autonomously
Files:
openwisp_controller/config/static/config/js/relevant_templates.jsopenwisp_controller/geo/tests/test_selenium.pyopenwisp_controller/config/tests/test_selenium.py
**/*.{js,ts,tsx,jsx,py,java,go,cs,rb,php,c,cpp,h,hpp,sql}
📄 CodeRabbit inference engine (Custom checks)
Flag obvious performance regressions, such as heavy loops, repeated I/O, or unoptimized queries
Files:
openwisp_controller/config/static/config/js/relevant_templates.jsopenwisp_controller/geo/tests/test_selenium.pyopenwisp_controller/config/tests/test_selenium.py
**/*.{js,ts,tsx,jsx,py,java,go,cs,rb,php,c,cpp,h,hpp,sh,bash,sql}
📄 CodeRabbit inference engine (Custom checks)
Cryptic or non-obvious code (regex, complex bash commands, or hard-to-read code) must include a concise comment explaining why it is needed and why the complexity is acceptable
Files:
openwisp_controller/config/static/config/js/relevant_templates.jsopenwisp_controller/geo/tests/test_selenium.pyopenwisp_controller/config/tests/test_selenium.py
**/*.{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_controller/geo/tests/test_selenium.pyopenwisp_controller/config/tests/test_selenium.py
🧠 Learnings (5)
📚 Learning: 2026-01-15T15:05:49.557Z
Learnt from: DragnEmperor
Repo: openwisp/openwisp-controller PR: 1175
File: openwisp_controller/config/management/commands/clear_last_ip.py:38-42
Timestamp: 2026-01-15T15:05:49.557Z
Learning: In Django projects, when using select_related() to traverse relations (for example, select_related("organization__config_settings")), the traversed relation must not be deferred. If you also use .only() in the same query, include the relation name or FK field (e.g., "organization" or "organization_id") in the .only() list to avoid the error "Field X cannot be both deferred and traversed using select_related at the same time." Apply this guideline to Django code in openwisp_controller/config/management/commands/clear_last_ip.py and similar modules by ensuring any select_related with an accompanying only() includes the related field names to prevent deferred/traversed conflicts.
Applied to files:
openwisp_controller/geo/tests/test_selenium.pyopenwisp_controller/config/tests/test_selenium.py
📚 Learning: 2026-02-17T19:13:10.088Z
Learnt from: nemesifier
Repo: openwisp/openwisp-controller PR: 1175
File: openwisp_controller/config/whois/commands.py:0-0
Timestamp: 2026-02-17T19:13:10.088Z
Learning: In reviews for the openwisp/openwisp-controller repository, do not propose changes based on Ruff warnings. The project does not use Ruff as its linter; ignore Ruff-related suggestions and follow the repository’s established linting and configuration rules. This guidance applies to all Python files under the openwisp_controller directory.
Applied to files:
openwisp_controller/geo/tests/test_selenium.pyopenwisp_controller/config/tests/test_selenium.py
📚 Learning: 2026-01-15T15:07:17.354Z
Learnt from: DragnEmperor
Repo: openwisp/openwisp-controller PR: 1175
File: openwisp_controller/geo/estimated_location/tests/tests.py:172-175
Timestamp: 2026-01-15T15:07:17.354Z
Learning: In this repository, flake8 enforces E501 (line too long) via setup.cfg (max-line-length = 88) while ruff ignores E501 via ruff.toml. Therefore, use '# noqa: E501' on lines that intentionally exceed 88 characters to satisfy flake8 without affecting ruff checks. This applies to Python files across the project (any .py) and is relevant for tests as well. Use sparingly and only where breaking lines is not feasible without hurting readability or functionality.
Applied to files:
openwisp_controller/geo/tests/test_selenium.pyopenwisp_controller/config/tests/test_selenium.py
📚 Learning: 2026-03-14T19:34:33.773Z
Learnt from: CR
Repo: openwisp/openwisp-controller PR: 0
File: coderabbit-custom-pre-merge-checks-unique-id-file-non-traceable-F7F2B60C-1728-4C9A-8889-4F2235E186CA.txt:0-0
Timestamp: 2026-03-14T19:34:33.773Z
Learning: Applies to **/*.{e2e,browser,selenium}.{js,ts,tsx,jsx} : If a bug affects the user interface, include a Selenium browser test; if missing, raise a warning
Applied to files:
openwisp_controller/config/tests/test_selenium.py
📚 Learning: 2026-03-14T19:34:33.773Z
Learnt from: CR
Repo: openwisp/openwisp-controller PR: 0
File: coderabbit-custom-pre-merge-checks-unique-id-file-non-traceable-F7F2B60C-1728-4C9A-8889-4F2235E186CA.txt:0-0
Timestamp: 2026-03-14T19:34:33.773Z
Learning: Applies to **/*{.test,.spec,_test,test_}.{js,ts,tsx,jsx,py,java,go,cs,rb} : Tests must be updated to cover non-trivial changes and ensure proper validation of modified behavior
Applied to files:
openwisp_controller/config/tests/test_selenium.py
🔇 Additional comments (3)
openwisp_controller/geo/tests/test_selenium.py (1)
94-98: Good fix: admin URL now derives from model metadata.This avoids app-label mismatches and keeps the Selenium test resilient to model/admin label differences.
openwisp_controller/config/static/config/js/relevant_templates.js (1)
141-141: Selector change is correct and fixes the duplication root cause.Using
#config-empty ul.sortedm2m-itemsremoves the brittle “last list” assumption and prevents double-appending to the same widget.openwisp_controller/config/tests/test_selenium.py (1)
49-59: Nice improvement: template visibility lookup is now properly scoped.Constraining XPath under
ul.sortedm2m-itemsreduces false matches from unrelated inputs.
pandafy
force-pushed
the
fix-relevant-templates
branch
from
83f4005 to
b9f6f1b
Compare
|
@coderabbitai review! |
|
✅ Actions performedReview triggered.
|
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_controller/config/tests/test_selenium.py`:
- Around line 401-483: The test_relevant_templates_duplicates test must
explicitly assert the precondition that no empty inline add form exists (so the
bug branch is exercised); before calling _verify_templates_visibility (or any
checks of the rendered items) add an assertion that the element with id
"config-empty" is not present (or equivalently that the inline add form is
unavailable) using the test's Selenium helpers (e.g., find_element/find_elements
or wait_for_absence) to fail fast if the empty inline is present; this ensures
the test actually covers the case where the empty inline form is missing and
prevents the permission fixture from masking the regression.
🪄 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: 904eb428-2a48-482c-8414-3475d5d3f0c0
📒 Files selected for processing (2)
openwisp_controller/config/tests/test_selenium.pyopenwisp_controller/geo/tests/test_selenium.py
📜 Review details
🧰 Additional context used
📓 Path-based instructions (4)
**/*.{js,ts,tsx,jsx,py,java,go,cs,rb,php,c,cpp,h,hpp}
📄 CodeRabbit inference engine (Custom checks)
**/*.{js,ts,tsx,jsx,py,java,go,cs,rb,php,c,cpp,h,hpp}: Flag potential security vulnerabilities in code
Avoid unnecessary comments or docstrings for code that is already clear
Code formatting is compact and readable. Do not add excessive blank lines, especially inside function or method bodies
Flag unused or redundant code
Ensure variables, functions, classes, and files have descriptive and consistent names
New code must handle errors properly: log errors that cannot be resolved by the user with error level, log unusual conditions with warning level, log important background actions with info level, and provide user-facing messages for errors that the user can solve autonomously
Files:
openwisp_controller/geo/tests/test_selenium.pyopenwisp_controller/config/tests/test_selenium.py
**/*.{js,ts,tsx,jsx,py,java,go,cs,rb,php,c,cpp,h,hpp,sql}
📄 CodeRabbit inference engine (Custom checks)
Flag obvious performance regressions, such as heavy loops, repeated I/O, or unoptimized queries
Files:
openwisp_controller/geo/tests/test_selenium.pyopenwisp_controller/config/tests/test_selenium.py
**/*.{js,ts,tsx,jsx,py,java,go,cs,rb,php,c,cpp,h,hpp,sh,bash,sql}
📄 CodeRabbit inference engine (Custom checks)
Cryptic or non-obvious code (regex, complex bash commands, or hard-to-read code) must include a concise comment explaining why it is needed and why the complexity is acceptable
Files:
openwisp_controller/geo/tests/test_selenium.pyopenwisp_controller/config/tests/test_selenium.py
**/*.{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_controller/geo/tests/test_selenium.pyopenwisp_controller/config/tests/test_selenium.py
🧠 Learnings (6)
📚 Learning: 2026-01-12T22:27:48.342Z
Learnt from: nemesifier
Repo: openwisp/openwisp-controller PR: 1175
File: tests/openwisp2/sample_config/migrations/0008_whoisinfo_organizationconfigsettings_whois_enabled.py:18-67
Timestamp: 2026-01-12T22:27:48.342Z
Learning: In tests/openwisp2/sample_config/models.py and corresponding test migrations, the WHOISInfo model intentionally includes an additional "details" field not present in the base AbstractWHOISInfo model. This is a testing pattern to verify that swappable models (CONFIG_WHOISINFO_MODEL) can be extended with custom fields without errors.
Applied to files:
openwisp_controller/geo/tests/test_selenium.py
📚 Learning: 2026-01-15T15:05:49.557Z
Learnt from: DragnEmperor
Repo: openwisp/openwisp-controller PR: 1175
File: openwisp_controller/config/management/commands/clear_last_ip.py:38-42
Timestamp: 2026-01-15T15:05:49.557Z
Learning: In Django projects, when using select_related() to traverse relations (for example, select_related("organization__config_settings")), the traversed relation must not be deferred. If you also use .only() in the same query, include the relation name or FK field (e.g., "organization" or "organization_id") in the .only() list to avoid the error "Field X cannot be both deferred and traversed using select_related at the same time." Apply this guideline to Django code in openwisp_controller/config/management/commands/clear_last_ip.py and similar modules by ensuring any select_related with an accompanying only() includes the related field names to prevent deferred/traversed conflicts.
Applied to files:
openwisp_controller/geo/tests/test_selenium.pyopenwisp_controller/config/tests/test_selenium.py
📚 Learning: 2026-02-17T19:13:10.088Z
Learnt from: nemesifier
Repo: openwisp/openwisp-controller PR: 1175
File: openwisp_controller/config/whois/commands.py:0-0
Timestamp: 2026-02-17T19:13:10.088Z
Learning: In reviews for the openwisp/openwisp-controller repository, do not propose changes based on Ruff warnings. The project does not use Ruff as its linter; ignore Ruff-related suggestions and follow the repository’s established linting and configuration rules. This guidance applies to all Python files under the openwisp_controller directory.
Applied to files:
openwisp_controller/geo/tests/test_selenium.pyopenwisp_controller/config/tests/test_selenium.py
📚 Learning: 2026-01-15T15:07:17.354Z
Learnt from: DragnEmperor
Repo: openwisp/openwisp-controller PR: 1175
File: openwisp_controller/geo/estimated_location/tests/tests.py:172-175
Timestamp: 2026-01-15T15:07:17.354Z
Learning: In this repository, flake8 enforces E501 (line too long) via setup.cfg (max-line-length = 88) while ruff ignores E501 via ruff.toml. Therefore, use '# noqa: E501' on lines that intentionally exceed 88 characters to satisfy flake8 without affecting ruff checks. This applies to Python files across the project (any .py) and is relevant for tests as well. Use sparingly and only where breaking lines is not feasible without hurting readability or functionality.
Applied to files:
openwisp_controller/geo/tests/test_selenium.pyopenwisp_controller/config/tests/test_selenium.py
📚 Learning: 2026-03-14T19:34:33.773Z
Learnt from: CR
Repo: openwisp/openwisp-controller PR: 0
File: coderabbit-custom-pre-merge-checks-unique-id-file-non-traceable-F7F2B60C-1728-4C9A-8889-4F2235E186CA.txt:0-0
Timestamp: 2026-03-14T19:34:33.773Z
Learning: Applies to **/*.{e2e,browser,selenium}.{js,ts,tsx,jsx} : If a bug affects the user interface, include a Selenium browser test; if missing, raise a warning
Applied to files:
openwisp_controller/config/tests/test_selenium.py
📚 Learning: 2026-03-14T19:34:33.773Z
Learnt from: CR
Repo: openwisp/openwisp-controller PR: 0
File: coderabbit-custom-pre-merge-checks-unique-id-file-non-traceable-F7F2B60C-1728-4C9A-8889-4F2235E186CA.txt:0-0
Timestamp: 2026-03-14T19:34:33.773Z
Learning: Applies to **/*{.test,.spec,_test,test_}.{js,ts,tsx,jsx,py,java,go,cs,rb} : Tests must be updated to cover non-trivial changes and ensure proper validation of modified behavior
Applied to files:
openwisp_controller/config/tests/test_selenium.py
🔇 Additional comments (2)
openwisp_controller/geo/tests/test_selenium.py (1)
94-98: Good switch to model-derived admin routing.Using
self.object_model._meta.app_labelandmodel_namekeeps this test aligned with the actual admin registration and swapped-model setups.openwisp_controller/config/tests/test_selenium.py (1)
49-58: Good tightening of the template visibility locator.Scoping these checks to
sortedm2m-itemsmakes the assertions target the template widget instead of any matching input on the page.
Code Style and Test FailuresHello @pandafy, There are two main issues in your commit:
Fix: Please run
Fix: The change in the test log indicates that the - with self.subTest("Regression precondition: empty Config inline is not rendered"):
+ with self.subTest(
+ "Regression precondition: empty Config inline is not rendered"
+ ):Please update the |
pandafy
force-pushed
the
fix-relevant-templates
branch
from
592b3ed to
4c9e834
Compare
|
@coderabbitai review! |
|
✅ Actions performedReview triggered.
|
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_controller/config/tests/test_selenium.py`:
- Around line 402-485: Remove the verbose docstring and redundant inline
comments in the test that defines permission_codenames and creates the
user/group/templates (the test containing permission_codenames, custom_group,
user creation and calls to
_create_template/_create_config/_verify_templates_visibility); keep only
succinct comments for non-obvious logic (e.g., why organization=None is used)
and preserve the subTest labels and assertions (the subTest blocks and selectors
using find_elements/By.CSS_SELECTOR and the checkbox checks using
id_config-templates_{idx}) so behavior is unchanged but noisy narration is
eliminated.
🪄 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: b6270880-d80e-48a9-8793-d4ba3a38b867
📒 Files selected for processing (1)
openwisp_controller/config/tests/test_selenium.py
📜 Review details
🧰 Additional context used
📓 Path-based instructions (4)
**/*.{js,ts,tsx,jsx,py,java,go,cs,rb,php,c,cpp,h,hpp}
📄 CodeRabbit inference engine (Custom checks)
**/*.{js,ts,tsx,jsx,py,java,go,cs,rb,php,c,cpp,h,hpp}: Flag potential security vulnerabilities in code
Avoid unnecessary comments or docstrings for code that is already clear
Code formatting is compact and readable. Do not add excessive blank lines, especially inside function or method bodies
Flag unused or redundant code
Ensure variables, functions, classes, and files have descriptive and consistent names
New code must handle errors properly: log errors that cannot be resolved by the user with error level, log unusual conditions with warning level, log important background actions with info level, and provide user-facing messages for errors that the user can solve autonomously
Files:
openwisp_controller/config/tests/test_selenium.py
**/*.{js,ts,tsx,jsx,py,java,go,cs,rb,php,c,cpp,h,hpp,sql}
📄 CodeRabbit inference engine (Custom checks)
Flag obvious performance regressions, such as heavy loops, repeated I/O, or unoptimized queries
Files:
openwisp_controller/config/tests/test_selenium.py
**/*.{js,ts,tsx,jsx,py,java,go,cs,rb,php,c,cpp,h,hpp,sh,bash,sql}
📄 CodeRabbit inference engine (Custom checks)
Cryptic or non-obvious code (regex, complex bash commands, or hard-to-read code) must include a concise comment explaining why it is needed and why the complexity is acceptable
Files:
openwisp_controller/config/tests/test_selenium.py
**/*.{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_controller/config/tests/test_selenium.py
🧠 Learnings (7)
📚 Learning: 2026-03-14T19:34:33.773Z
Learnt from: CR
Repo: openwisp/openwisp-controller PR: 0
File: coderabbit-custom-pre-merge-checks-unique-id-file-non-traceable-F7F2B60C-1728-4C9A-8889-4F2235E186CA.txt:0-0
Timestamp: 2026-03-14T19:34:33.773Z
Learning: Applies to **/*.{e2e,browser,selenium}.{js,ts,tsx,jsx} : If a bug affects the user interface, include a Selenium browser test; if missing, raise a warning
Applied to files:
openwisp_controller/config/tests/test_selenium.py
📚 Learning: 2026-03-14T19:34:33.773Z
Learnt from: CR
Repo: openwisp/openwisp-controller PR: 0
File: coderabbit-custom-pre-merge-checks-unique-id-file-non-traceable-F7F2B60C-1728-4C9A-8889-4F2235E186CA.txt:0-0
Timestamp: 2026-03-14T19:34:33.773Z
Learning: Applies to **/*{.test,.spec,_test,test_}.{js,ts,tsx,jsx,py,java,go,cs,rb} : Tests must be updated to cover non-trivial changes and ensure proper validation of modified behavior
Applied to files:
openwisp_controller/config/tests/test_selenium.py
📚 Learning: 2026-03-14T19:34:33.773Z
Learnt from: CR
Repo: openwisp/openwisp-controller PR: 0
File: coderabbit-custom-pre-merge-checks-unique-id-file-non-traceable-F7F2B60C-1728-4C9A-8889-4F2235E186CA.txt:0-0
Timestamp: 2026-03-14T19:34:33.773Z
Learning: Applies to **/*{.test,.spec,_test,test_}.{js,ts,tsx,jsx,py,java,go,cs,rb} : Regression test must fail if the patch is removed or commented out
Applied to files:
openwisp_controller/config/tests/test_selenium.py
📚 Learning: 2026-03-14T19:34:33.773Z
Learnt from: CR
Repo: openwisp/openwisp-controller PR: 0
File: coderabbit-custom-pre-merge-checks-unique-id-file-non-traceable-F7F2B60C-1728-4C9A-8889-4F2235E186CA.txt:0-0
Timestamp: 2026-03-14T19:34:33.773Z
Learning: Applies to **/*{.test,.spec,_test,test_}.{js,ts,tsx,jsx,py,java,go,cs,rb} : Include a regression test for bug fixes that reproduces the bug scenario
Applied to files:
openwisp_controller/config/tests/test_selenium.py
📚 Learning: 2026-01-15T15:05:49.557Z
Learnt from: DragnEmperor
Repo: openwisp/openwisp-controller PR: 1175
File: openwisp_controller/config/management/commands/clear_last_ip.py:38-42
Timestamp: 2026-01-15T15:05:49.557Z
Learning: In Django projects, when using select_related() to traverse relations (for example, select_related("organization__config_settings")), the traversed relation must not be deferred. If you also use .only() in the same query, include the relation name or FK field (e.g., "organization" or "organization_id") in the .only() list to avoid the error "Field X cannot be both deferred and traversed using select_related at the same time." Apply this guideline to Django code in openwisp_controller/config/management/commands/clear_last_ip.py and similar modules by ensuring any select_related with an accompanying only() includes the related field names to prevent deferred/traversed conflicts.
Applied to files:
openwisp_controller/config/tests/test_selenium.py
📚 Learning: 2026-02-17T19:13:10.088Z
Learnt from: nemesifier
Repo: openwisp/openwisp-controller PR: 1175
File: openwisp_controller/config/whois/commands.py:0-0
Timestamp: 2026-02-17T19:13:10.088Z
Learning: In reviews for the openwisp/openwisp-controller repository, do not propose changes based on Ruff warnings. The project does not use Ruff as its linter; ignore Ruff-related suggestions and follow the repository’s established linting and configuration rules. This guidance applies to all Python files under the openwisp_controller directory.
Applied to files:
openwisp_controller/config/tests/test_selenium.py
📚 Learning: 2026-01-15T15:07:17.354Z
Learnt from: DragnEmperor
Repo: openwisp/openwisp-controller PR: 1175
File: openwisp_controller/geo/estimated_location/tests/tests.py:172-175
Timestamp: 2026-01-15T15:07:17.354Z
Learning: In this repository, flake8 enforces E501 (line too long) via setup.cfg (max-line-length = 88) while ruff ignores E501 via ruff.toml. Therefore, use '# noqa: E501' on lines that intentionally exceed 88 characters to satisfy flake8 without affecting ruff checks. This applies to Python files across the project (any .py) and is relevant for tests as well. Use sparingly and only where breaking lines is not feasible without hurting readability or functionality.
Applied to files:
openwisp_controller/config/tests/test_selenium.py
🔇 Additional comments (2)
openwisp_controller/config/tests/test_selenium.py (2)
49-58: Widget-scoped locators make this helper safer.Confining the visibility checks to inputs under
ul.sortedm2m-itemskeeps_verify_templates_visibility()aligned with the sortedm2m widget instead of any page element that happens to share the same value.
449-487: Strong regression coverage for the missing-empty-inline path.Asserting that
#config-emptyis absent, then checking the rendered item count and checkbox attributes, makes the failure mode very explicit if the selector logic regresses again.Based on learnings: If a bug affects the user interface, include a Selenium browser test; include a regression test for bug fixes that reproduces the bug scenario.
github-project-automation
bot
moved this from In progress
to Done
in OpenWISP Contributor's Board
3 participants
Checklist
Description of Changes
Bug:
The JS logic in relevant_templates.js assumed that the last
ul.sortedm2m-itemselement belonged to the empty inline form used by Django admin formsets. This assumption breaks when the user does not have permission to add Config objects: in that case the ConfigInlineAdmin does not render the empty form.As a result, both selectors ended up referencing the same list and the script appended template elements twice to the same
sortedm2mlist, causing duplicate entries and issues when saving the form.Fix:
Select the empty form container explicitly using
#config-empty ul.sortedm2m-itemsinstead of relying on the last occurrence oful.sortedm2m-items. This ensures the logic works correctly regardless of whether the empty inline form is rendered.