[fix] fix test environment pollution in TestAdminMedia #405

[shivsubh]

The test_object_notification_setting_configured test case was modifying UserAdmin.Media without proper restoration, causing pollution in the test environment. Added a try...finally block to ensure UserAdmin.Media is restored to its original state.

Closes #405

Checklist

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

[coderabbitai]

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: ASSERTIVE

Plan: Pro

Run ID: aff0b155-8521-45ba-971d-be71d5258089

📥 Commits

Reviewing files that changed from the base of the PR and between f644384 and 3687a8a.

📒 Files selected for processing (2)

• openwisp_notifications/tests/test_admin.py
• openwisp_notifications/tests/test_selenium.py

📜 Recent review details

🔇 Additional comments (5)

openwisp_notifications/tests/test_selenium.py (1)

174-178: Looks good — behavior preserved with clearer script injection.

The triple-quoted execute_script input keeps the same mocked fetch failure behavior and improves readability without changing test intent.

openwisp_notifications/tests/test_admin.py (4)

345-360: Consider adding assertions for the remaining test configurations.

The response variables on lines 348-352 and 356-360 are assigned but never asserted. While this implicitly verifies no exceptions are raised, adding explicit assertions would make the test's intent clearer and verify the widget JS is properly included in these configurations as well.

💡 Suggested assertions

             UserAdmin.Media.css = {"all": list()}
             UserAdmin.Media.js = list()
             _add_object_notification_widget()
             response = self.client.get(
                 reverse(
                     f"admin:{self.users_app_label}_user_change", args=(self.admin.pk,)
                 )
             )
+            self.assertContains(
+                response,
+                'src="/static/openwisp-notifications/js/object-notifications.js"',
+            )
             UserAdmin.Media.js = []
             UserAdmin.Media.css = {}
             _add_object_notification_widget()
             response = self.client.get(
                 reverse(
                     f"admin:{self.users_app_label}_user_change", args=(self.admin.pk,)
                 )
             )
+            self.assertContains(
+                response,
+                'src="/static/openwisp-notifications/js/object-notifications.js"',
+            )

---

1-8: LGTM!

The new imports are appropriate and properly used in the test method.

---

316-324: LGTM!

The state capture correctly handles all three attributes (js, css, extend) that _add_object_notification_widget() may modify. Using copy.deepcopy() for js and css is appropriate since they can be mutable collections, while extend (a boolean) doesn't need deep copying.

---

361-379: LGTM!

The finally block correctly restores UserAdmin.Media to its original state across all scenarios:

1. When original_media existed: restores the reference and replaces any mutated attributes from deep copies
2. When attributes weren't originally present: properly deletes them to avoid leaving new state
3. When Media didn't exist before: removes the attribute entirely

This properly addresses the test environment pollution issue described in #405.

---

📝 Walkthrough

Walkthrough

The test test_object_notification_setting_configured is refactored to avoid polluting the test environment by preserving and restoring UserAdmin.Media. The test captures any pre-existing UserAdmin.Media, deep-copies its js and css if present, runs assertions across multiple UserAdmin.Media configurations (including emitting MediaOrderConflictWarning when a prior media object exists), and restores the original state in a finally block—reinstating the object or its js/css, or deleting UserAdmin.Media if it was not originally defined. The direct import of Media and the assignment UserAdmin.Media = Media() were removed.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

🚥 Pre-merge checks | ✅ 5

✅ Passed checks (5 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title clearly identifies the fix for test environment pollution caused by UserAdmin.Media modifications, matching the issue #405 and using the required [fix] prefix.
Description check	✅ Passed	The description covers the main issue and solution, includes issue reference, and has a mostly complete checklist, though documentation update is marked as not completed.
Linked Issues check	✅ Passed	The changes directly address issue #405 by adding a try...finally block to restore UserAdmin.Media to its original state, preventing test environment pollution.
Out of Scope Changes check	✅ Passed	All changes are within scope: test_admin.py modifications focus on fixing the UserAdmin.Media pollution issue, and test_selenium.py changes are a minor formatting adjustment to existing test code.
Bug Fixes	✅ Passed	Pull request addresses test environment pollution, not a bug in core user-facing functionality; custom check explicitly applies only to user-facing bugs.

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

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

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

[coderabbitai]

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_notifications/tests/test_admin.py`:
- Around line 343-358: The test assigns responses from self.client.get when
calling reverse for f"admin:{self.users_app_label}_user_change" after toggling
UserAdmin.Media but never asserts them; either replace the unused local name
with _ to show intentional discard or add explicit assertions (e.g., check
response.status_code == 200 and presence/absence of the widget JS/CSS in
response.content) after the calls that invoke _add_object_notification_widget
and self.client.get, referencing the UserAdmin.Media mutation,
_add_object_notification_widget(), reverse(...), self.client.get(...) and
self.admin.pk to locate the code to 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: 35571361-ce5f-4601-82c3-00ac24765e05

📥 Commits

Reviewing files that changed from the base of the PR and between d5b5bb9 and 55d5946.

📒 Files selected for processing (1)

• openwisp_notifications/tests/test_admin.py

📜 Review details

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

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

🔇 Additional comments (3)

openwisp_notifications/tests/test_admin.py (3)

1-8: LGTM!

The new imports are necessary and correctly used: copy for deep copying mutable js/css attributes, and MediaOrderConflictWarning for asserting the expected warning when the widget is added multiple times.

---

316-322: LGTM - state preservation logic is correct.

Deep copying the mutable js and css attributes ensures they can be properly restored after in-place mutations by _add_object_notification_widget().

Minor: For consistency with line 360's if original_media is not None:, consider using the same pattern here on line 317.

---

359-372: LGTM - restoration logic correctly handles all cases.

The finally block properly restores the original state:

• When Media existed originally: restores the class reference and its js/css attributes from deep copies
• When Media didn't exist: removes the Media class that was added during the test
• Correctly handles cases where js/css attributes didn't exist originally by deleting them

This effectively prevents test environment pollution as intended by the PR.

[coderabbitai]

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_notifications/tests/test_admin.py`:
- Around line 316-322: The test fails to preserve the Media.extend attribute
when patching UserAdmin.Media; capture the original extend value before
modifying by doing original_extend = getattr(original_media, "extend", None)
(alongside original_js/original_css) and in the finally block restore it with
setattr(UserAdmin, "Media", original_media) or if restoring piecewise, set
getattr(UserAdmin, "Media", "extend") back to original_extend; reference the
UserAdmin.Media object and the IgnoreObjectNotificationWidgetMedia class to
ensure Media.extend is preserved and restored to avoid test pollution.

🪄 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: 577a9354-086d-4803-89e2-77e86209b465

📥 Commits

Reviewing files that changed from the base of the PR and between 55d5946 and f644384.

📒 Files selected for processing (1)

• openwisp_notifications/tests/test_admin.py

📜 Review details

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

• GitHub Check: Python==3.13 | django~=5.2.0
• GitHub Check: Python==3.10 | django~=5.1.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~=5.0.0
• GitHub Check: Python==3.11 | django~=4.2.0
• GitHub Check: Python==3.12 | django~=4.2.0
• GitHub Check: Python==3.13 | django~=5.1.0
• GitHub Check: Python==3.12 | django~=5.2.0
• GitHub Check: Python==3.11 | 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 (3)

openwisp_notifications/tests/test_admin.py (3)

337-358: Unused response variables should either be asserted or discarded.

The response assignments on lines 337-342, 346-350, and 354-358 are never asserted. If the intent is only to verify no exceptions are raised, consider using _ to indicate intentional discard, or add explicit assertions to validate the expected behavior.

---

1-1: LGTM!

The copy import is appropriate for deep copying the js and css attributes.

---

359-372: LGTM!

The try...finally pattern correctly ensures UserAdmin.Media is restored regardless of test outcome. The restoration logic properly handles all edge cases:

• Original Media with/without js/css attributes
• Original Media not existing at all
• Media being replaced entirely by _add_object_notification_widget()

This addresses the test pollution issue described in #405.