Add localizable metadata to ban notices by ocepkejepintu-droid · Pull Request #1083 · FAForever/server

ocepkejepintu-droid · 2026-05-10T21:48:21Z

Addresses #640

This keeps the existing English ban notice text for current clients, but adds structured fields clients can use for localization:

i18n_key: login.error.banned
i18n_args: [expires_at, ban_reason]
expires_at: the ban expiry as an ISO timestamp

That gives the Downlord client enough data to translate/render the message locally without breaking older clients that still read text.

Companion PRs:

Verification:

GitHub Actions unit-test — passed
GitHub Actions docker-build — passed
GitHub Actions lint jobs isort, flake8, mypy, pipenv-verify — passed
Codecov patch and project — passed
Codacy Static Code Analysis — passed
CodeRabbit — passed
Local: python3 -m py_compile server/exceptions.py server/lobbyconnection.py tests/unit_tests/test_lobbyconnection.py tests/integration_tests/test_login.py
Local: git diff --check

Not run locally: pytest, because this machine does not have the repo pipenv/pytest environment installed.

Keep the existing English notice text for old clients while adding machine-readable ban fields that newer clients can translate.\n\nConstraint: FAF/server#640 asks the server to stop relying only on rendered English ban text.\nRejected: Replace the notice text outright | would break existing clients before client-side localization lands.\nConfidence: medium\nScope-risk: narrow\nDirective: Preserve the legacy text field until all supported clients consume i18n_key.\nTested: python3 -m py_compile server/exceptions.py server/lobbyconnection.py tests/unit_tests/test_lobbyconnection.py tests/integration_tests/test_login.py; git diff --check\nNot-tested: pytest suite, because local Python environment lacks pytest/pipenv and project services.

coderabbitai · 2026-05-10T21:48:35Z

📝 Walkthrough

Walkthrough

The PR refactors ban error messaging by extracting notice payload construction into a BanError.notice() method. This centralizes the structured notice generation—including i18n metadata, expiry, and reason—then updates LobbyConnection to use this method and adjusts all tests to validate the new payload structure.

Changes

Ban Notice Refactoring

Layer / File(s)	Summary
Notice Contract `server/exceptions.py`	`BanError.notice()` method constructs a structured error-style notice payload containing command, style, i18n fields (`i18n_key`, `i18n_args`), rendered text, and `expires_at` ISO timestamp.
Connection Wiring `server/lobbyconnection.py`	`LobbyConnection.on_message_received` exception handler now uses `e.notice()` to send the notice payload instead of manually constructing the message.
Test Verification `tests/integration_tests/test_login.py`, `tests/unit_tests/test_lobbyconnection.py`	Ban-related tests updated to assert structured i18n fields, expiry metadata, and formatted text instead of full dict equality.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~12 minutes

Poem

🐰 A notice method hops into the fray,
Structuring ban messages in a cleaner way,
With i18n fields and expiry dates too,
The tests now verify what once we knew.
One change ripples through—so neat, so true! ✨

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 0.00% 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
Title check	✅ Passed	The title directly and clearly describes the main change: adding localizable metadata fields (i18n_key, i18n_args, expires_at) to ban notice messages across the codebase.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.

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

✨ Finishing Touches

🧪 Generate unit tests (beta)

Create PR with unit tests

Tip

💬 Introducing Slack Agent: The best way for teams to turn conversations into code.

Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.

Generate code and open pull requests
Plan features and break down work
Investigate incidents and troubleshoot customer tickets together
Automate recurring tasks and respond to alerts with triggers
Summarize progress and report instantly

Built for teams:

Shared memory across your entire org—no repeating context
Per-thread sandboxes to safely plan and execute work
Governance built-in—scoped access, auditability, and budget controls

One agent for your entire SDLC. Right inside Slack.

👉 Get started

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

Refresh the lock hash without dependency churn and make the existing QDataStream byte packing type explicit so the PR can be reviewed against its ban-notice change instead of unrelated CI drift. Constraint: upstream CI runs pipenv verify and mypy on the PR merge ref. Rejected: full pipenv lock regeneration | it changed many package pins unrelated to this issue. Confidence: high Scope-risk: narrow Directive: keep future bounty follow-ups focused on merge blockers only; avoid dependency churn unless a maintainer requests it. Tested: /tmp/faf-pipenv-venv/bin/pipenv verify; /opt/homebrew/bin/python3.13 -m py_compile server/exceptions.py server/lobbyconnection.py server/protocol/qdatastream.py tests/integration_tests/test_login.py tests/unit_tests/test_lobbyconnection.py; git diff --check Not-tested: full pytest suite unavailable locally without project service dependencies

Update the exact-dict assertion so the test checks the legacy English text plus the structured i18n fields added by the bounty fix. Constraint: CI still exercises the old live-ban game command path. Rejected: removing legacy text assertion | old clients still depend on that fallback field. Confidence: high Scope-risk: narrow Directive: keep notice tests asserting both backward-compatible text and localization metadata. Tested: /opt/homebrew/bin/python3.13 -m py_compile tests/integration_tests/test_server.py; git diff --check Not-tested: local integration pytest requires MySQL service; GitHub Actions provides that service

Trigger a fresh CI run after the only remaining failure was an unrelated matchmaker timeout in tests/integration_tests/test_matchmaker_violations.py::test_violation_config_disabled[json]. Constraint: GitHub does not allow fork author to rerun failed upstream Actions jobs without repository admin rights. Rejected: changing unrelated matchmaker tests | failure is outside the ban-notice bounty scope. Confidence: medium Scope-risk: narrow Directive: do not patch unrelated flaky matchmaker behavior unless maintainers request it. Tested: previous run passed pipenv-verify, mypy, flake8, isort, docker-build, Codacy, and 1009/1010 unit tests. Not-tested: rerun could still hit same upstream flake Co-authored-by: OmX <omx@oh-my-codex.dev>

ocepkejepintu-droid · 2026-05-10T22:14:06Z

Follow-up: CI is green now. The branch keeps the localized ban notice payload small and backward-compatible (text remains unchanged for old clients), with the extra i18n_key, i18n_args, and expires_at fields covered in the login/live-ban tests.

ocepkejepintu-droid · 2026-05-10T22:25:58Z

Opened the companion lobby-model PR so clients can consume the new notice metadata without raw JSON handling: FAForever/faf-java-commons#277

ocepkejepintu-droid · 2026-05-10T22:49:04Z

Opened the Downlord client companion PR as well: FAForever/downlords-faf-client#3514

That completes the chain for the funded issue: server emits the metadata, faf-java-commons models it, and the client can render localized notice text while falling back to the legacy text field.

Constraint: FAForever/server#1083 adds optional notice i18n metadata before faf-java-commons is released to this client. Rejected: Bump commons immediately | no published artifact contains the new notice fields yet. Confidence: medium Scope-risk: narrow Directive: Replace reflection with direct NoticeInfo getters after the commons dependency includes i18nKey and i18nArgs. Tested: git diff --check Not-tested: Gradle tests locally; no Java runtime installed in this environment. Co-authored-by: OmX <omx@oh-my-codex.dev>

Sheikah45 · 2026-05-12T08:32:29Z

The mixing of i18n_args and explicit fields like expired_at is inconsistent.

Also none of the projects use issue hunt any longer.

ocepkejepintu-droid mentioned this pull request May 10, 2026

Replace ban text with localizable message #640

Open

Yoseph and others added 3 commits May 11, 2026 04:53

ocepkejepintu-droid mentioned this pull request May 10, 2026

Expose localizable notice metadata FAForever/faf-java-commons#277

Open

ocepkejepintu-droid mentioned this pull request May 10, 2026

Display localized lobby notice text FAForever/downlords-faf-client#3514

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Add localizable metadata to ban notices#1083

Add localizable metadata to ban notices#1083
ocepkejepintu-droid wants to merge 4 commits into
FAForever:developfrom
ocepkejepintu-droid:issue-640-localizable-ban-notice

ocepkejepintu-droid commented May 10, 2026 •

edited

Loading

Uh oh!

coderabbitai Bot commented May 10, 2026 •

edited

Loading

Walkthrough

Changes

Estimated code review effort

Poem

❌ Failed checks (1 warning)

Uh oh!

ocepkejepintu-droid commented May 10, 2026 •

edited

Loading

Uh oh!

ocepkejepintu-droid commented May 10, 2026

Uh oh!

ocepkejepintu-droid commented May 10, 2026 •

edited

Loading

Uh oh!

Sheikah45 commented May 12, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Uh oh!

Conversation

ocepkejepintu-droid commented May 10, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

coderabbitai Bot commented May 10, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Walkthrough

Changes

Estimated code review effort

Poem

❌ Failed checks (1 warning)

Uh oh!

ocepkejepintu-droid commented May 10, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ocepkejepintu-droid commented May 10, 2026

Uh oh!

ocepkejepintu-droid commented May 10, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Sheikah45 commented May 12, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

ocepkejepintu-droid commented May 10, 2026 •

edited

Loading

coderabbitai Bot commented May 10, 2026 •

edited

Loading

ocepkejepintu-droid commented May 10, 2026 •

edited

Loading

ocepkejepintu-droid commented May 10, 2026 •

edited

Loading