[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: 155cbc19-30f0-48a3-8f12-b82e075eae48

📥 Commits

Reviewing files that changed from the base of the PR and between b8d0169 and 0d87fc1.

📒 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. (12)

• GitHub Check: Python==3.10 | django~=4.2.0
• GitHub Check: Python==3.10 | django~=5.1.0
• GitHub Check: Python==3.10 | django~=5.2.0
• GitHub Check: Python==3.11 | django~=4.2.0
• GitHub Check: Python==3.12 | django~=5.2.0
• GitHub Check: Python==3.12 | django~=4.2.0
• GitHub Check: Python==3.11 | django~=5.1.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: Python==3.13 | django~=5.1.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/management/commands/base/convert_called_station_id.py
• openwisp_radius/tests/test_commands.py

🔇 Additional comments (2)

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

26-31: LGTM!

Also applies to: 44-45

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

Suggested reviewers: pandafy

🚥 Pre-merge checks | ✅ 8

✅ Passed checks (8 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title is descriptive, uses the required [fix] prefix, and matches the telnet connection bug being addressed.
Description check	✅ Passed	It includes the checklist, issue reference, and a clear fix/test explanation, though the explicit Description and Screenshot sections are omitted.
Linked Issues check	✅ Passed	The code now uses explicit Telnet open/close handling and the new test verifies conversion and closure, matching #727.
Out of Scope Changes check	✅ Passed	The changes stay focused on the reported bug and regression coverage, with no unrelated edits visible in the summary.
Bug Fixes	✅ Passed	Root cause is fixed with explicit Telnet handling/cleanup, and the regression test reproduces the no-context-manager bug and verifies conversion plus close deterministically.
Features	✅ Passed	Not a feature request; it’s a bug fix for #727, so the feature-checklist requirements don’t apply.
Changes	✅ Passed	Regression test covers the telnet cleanup fix; docs already describe the intended command behavior, and there are no UI/API changes or migrations needed.
General Rules	✅ Passed	The fix uses explicit Telnet cleanup in finally, and the new regression test verifies conversion still occurs and close() is called.

✨ 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

Files Reviewed (X files)

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

Previous Review Summary (commit b8d0169)

Current summary above is authoritative. Previous snapshots are kept for context only.

Previous review (commit b8d0169)

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 step-3.7-flash · Input: 51.4K · Output: 2.5K · Cached: 171.9K}

[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.

[coveralls]

coverage: 98.12% (+0.2%) from 97.964% — omlahore:issues/727-convert-called-station-id-telnet into openwisp:master

[nemesifier]

Thanks @omlahore 👍

[openwisp-companion]

Proposed change log entry:

[fix] Close telnet connection properly in convert_called_station_id #727

The `convert_called_station_id` management command was failing silently
because the `Exscript.protocols.telnetlib.Telnet` object did not support
the context manager protocol. This prevented the `called_station_id`
from being converted.

This change explicitly opens and closes the telnet connection using a
`try...finally` block, ensuring the connection is always closed and the
command functions as expected. A regression test has been added to
verify this fix.

Closes #727