Skip to content

Send localizable ban response#1088

Open
Hector8aJ wants to merge 2 commits into
FAForever:developfrom
Hector8aJ:fix/issue-640-localizable-ban-message
Open

Send localizable ban response#1088
Hector8aJ wants to merge 2 commits into
FAForever:developfrom
Hector8aJ:fix/issue-640-localizable-ban-message

Conversation

@Hector8aJ
Copy link
Copy Markdown

@Hector8aJ Hector8aJ commented Jun 2, 2026

Fixes #640

What: Sends a banned protocol message with an ISO expires_at timestamp instead of an English HTML notice.
Why: The client can localize the ban message and format the timestamp itself.

Verification: python -m py_compile on modified files. pytest was not available locally.

Summary by CodeRabbit

  • Refactor
    • Ban notifications now return structured responses with precise expiry timestamps in ISO‑8601 format instead of duration-based or HTML-formatted messages.
  • Behavior
    • Authentication/ban errors are forwarded to clients using their structured responses.
  • Tests
    • Integration and unit tests updated to validate the new structured ban payload and ISO‑8601 expiry parsing.
  • Bug Fix
    • Message packing now finalizes message bytes before length-prefixing to ensure stable framing.

@coderabbitai
Copy link
Copy Markdown

coderabbitai Bot commented Jun 2, 2026

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: 8a8a8afa-e906-4098-902c-1dc90240cdf1

📥 Commits

Reviewing files that changed from the base of the PR and between ecb0cf5 and d2327e2.

⛔ Files ignored due to path filters (1)
  • Pipfile.lock is excluded by !**/*.lock
📒 Files selected for processing (3)
  • server/exceptions.py
  • server/protocol/qdatastream.py
  • tests/integration_tests/test_server.py
💤 Files with no reviewable changes (1)
  • tests/integration_tests/test_server.py
✅ Files skipped from review due to trivial changes (1)
  • server/protocol/qdatastream.py
🚧 Files skipped from review as they are similar to previous changes (1)
  • server/exceptions.py

📝 Walkthrough

Walkthrough

Ban notices now use a structured {"command": "banned", "expires_at": ""} payload. LobbyConnection forwards error.response() to clients. Tests updated to assert the new payload and parse ISO timestamps. Minor qdatastream change converts a bytearray to bytes before packing.

Changes

Ban response and related updates

Layer / File(s) Summary
Ban error response contract
server/exceptions.py
Removed humanize and datetime_now imports. BanError.message and BanError.response now use ban_expiry.isoformat() and response() emits {"command":"banned","expires_at":"<ISO-8601>"}.
Connection handler ban response
server/lobbyconnection.py
on_message_received sends AuthenticationError.response() to the client instead of constructing a hardcoded notice payload, then aborts with the error message.
Ban response validation tests
tests/unit_tests/test_lobbyconnection.py, tests/integration_tests/test_login.py, tests/integration_tests/test_server.py
Tests updated to assert command == "banned" and validate expires_at via datetime.fromisoformat. datetime imports added; re import removed where applicable.
Pack message bytes conversion
server/protocol/qdatastream.py
pack_message() wraps the accumulated bytearray with bytes(...) before calling pack_block.

🎯 3 (Moderate) | ⏱️ ~20 minutes

🐰 A ban now speaks in timestamp tongue,
No English prose shall cloud the young,
ISO format, crisp and clear,
Localized for all to hear!
Humanize? Nay, we've moved along.

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 44.44% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title 'Send localizable ban response' directly reflects the main objective of the PR: replacing the server-sent English ban message with a structured, localizable response as required by issue #640.
Linked Issues check ✅ Passed The PR implements all coding requirements from issue #640: sends a 'banned' command with ISO-formatted UTC expires_at timestamp, removes humanize dependency usage, and replaces English strings with structured payloads.
Out of Scope Changes check ✅ Passed The change to QDataStreamProtocol.pack_message() converting bytearray to bytes is a necessary supporting change for proper message serialization and remains within scope.

✏️ 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

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Replace ban text with localizable message

1 participant