[fix] Close telnet connection properly in convert_called_station_id

[omlahore]

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.

Explanation

Exscript.protocols.telnetlib.Telnet is a re-implementation of the standard library's telnetlib and does not implement the context manager protocol (no __enter__/__exit__). As a result, the with telnetlib.Telnet(...) as tn in _get_raw_management_info raised a TypeError, which was silently swallowed by the broad except Exception in _get_openvpn_routing_info. The convert_called_station_id command therefore exited without converting any called_station_id, and no error was surfaced.

This opens the connection explicitly and closes it in a finally block instead of using a context manager.

A regression test drives the real telnet path (patching telnetlib.Telnet with a fake that lacks the context manager protocol, mirroring Exscript) and asserts that the called_station_id is converted and the connection is closed. Without the fix, the conversion silently does not happen.

Fixes #727

[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: 058c67b6-92ac-4eb9-ac3e-d4b9a974a62d

📥 Commits

Reviewing files that changed from the base of the PR and between 512c659 and b8d0169.

📒 Files selected for processing (2)

• openwisp_radius/management/commands/base/convert_called_station_id.py
• openwisp_radius/tests/test_commands.py

📜 Recent review details

⏰ Context from checks skipped due to timeout. (13)

• GitHub Check: CodeRabbit / Review
• GitHub Check: Python==3.10 | django~=5.2.0
• GitHub Check: Python==3.12 | django~=4.2.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.10 | 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.12 | django~=5.1.0
• GitHub Check: Python==3.13 | django~=5.2.0
• GitHub Check: Python==3.11 | django~=5.2.0
• GitHub Check: Kilo Code Review

🧰 Additional context used

📓 Path-based instructions (1)

**/*.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_radius/tests/test_commands.py
• openwisp_radius/management/commands/base/convert_called_station_id.py

🔇 Additional comments (2)

openwisp_radius/management/commands/base/convert_called_station_id.py (1)

26-46: LGTM!

openwisp_radius/tests/test_commands.py (1)

480-538: LGTM!

---

📝 Walkthrough

Walkthrough

The convert_called_station_id command now creates the Exscript Telnet connection explicitly and closes it in a finally block, avoiding unsupported context-manager behavior. A regression test uses a fake Telnet implementation without context-manager methods and verifies both explicit closure and successful called_station_id conversion.

Estimated code review effort: 2 (Simple) | ~10 minutes

🚥 Pre-merge checks | ✅ 8

✅ Passed checks (8 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title is descriptive, matches the fix, and references the convert_called_station_id telnet issue.
Description check	✅ Passed	The description covers the checklist, issue reference, and change summary, with only non-critical sections left incomplete.
Linked Issues check	✅ Passed	The PR fixes the silent failure by replacing the unsupported context manager and adds regression coverage as requested in #727.
Out of Scope Changes check	✅ Passed	The changes stay focused on the telnet handling fix and regression test, with no clear unrelated additions.
Bug Fixes	✅ Passed	Fix uses explicit Telnet try/finally; regression test fakes no-enter/exit and asserts conversion plus close.
Features	✅ Passed	PASS: This is a bug fix for an existing management command, not a feature request; UI/screenshot criteria don’t apply, and a regression test was added.
Changes	✅ Passed	PASS: user-facing docs/UI unchanged; new regression test covers Telnet close and conversion; no public API break.
General Rules	✅ Passed	Explicit Telnet open/close fixes the swallowed TypeError, and the regression test verifies both conversion and close behavior.

✨ 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.}

[kilo-code-bot]

Code Review Summary

Status: No Issues Found | Recommendation: Merge

Nice, well-scoped fix. Replacing the with telnetlib.Telnet(...) context manager (unsupported by Exscript's re-implementation) with an explicit open + try/finally: tn.close() correctly resolves the silently-swallowed TypeError. The finally block is safe because tn is assigned before the try. The regression test drives the real telnet path with a FakeTelnet that intentionally lacks __enter__/__exit__, so it fails on the unpatched code and passes with the fix, and it also asserts the connection is closed. Good coverage of the reported bug (#727).

Files Reviewed (2 files)

• openwisp_radius/management/commands/base/convert_called_station_id.py
• openwisp_radius/tests/test_commands.py

---

_{Reviewed by claude-opus-4.8 · Input: 26 · Output: 3.3K · Cached: 583.5K}

[omlahore]

The CI failures here look unrelated to this change. Every job fails at the QA checkmigrations step, not in the tests:

ERROR: Migrations check failed! Models' changes not migrated
Migrations for 'sample_users':
  tests/openwisp2/sample_users/migrations/0005_alter_organizationuser_is_admin.py
  - Alter field is_admin on organizationuser

This PR only touches convert_called_station_id.py and its test — no models. The pending migration comes from an upstream openwisp-users change to OrganizationUser.is_admin, so the sample_users test app needs a regenerated migration on master. The same 11 failures appear on other current PRs (e.g. #743, #739), which confirms it is not specific to this branch.

Would you prefer the sample_users migration be regenerated on master, or should I include 0005_alter_organizationuser_is_admin in this PR to get it green? Happy to add it if that is the preferred route.