Skip to content

[US-1.1 / OBT-198] Edit language name and code (PUT) - #96

Open
levigtri wants to merge 6 commits into
mainfrom
levigft/obt-198-us-11-edit-a-languages-name-and-code-after-it-has-been
Open

[US-1.1 / OBT-198] Edit language name and code (PUT)#96
levigtri wants to merge 6 commits into
mainfrom
levigft/obt-198-us-11-edit-a-languages-name-and-code-after-it-has-been

Conversation

@levigtri

@levigtri levigtri commented Jul 4, 2026

Copy link
Copy Markdown
Member

Summary

Adds PUT /api/languages/{id} to edit a language's name and/or code (with updated_at, 409 on code conflict, 404 when missing), and — per the 2026-07 rule change — restricts the direct edit to platform admins. A manager no longer edits a language directly; a manager's edit is submitted as an edit request for a platform admin to approve, delivered by the change-requests feature (separate US) and scoped to languages linked to the manager's own projects.

Changes

  1. Editable name/codeapp/db/models/language.py (updated_at + migration), app/models/language.py (LanguageUpdate, updated_at on the response), app/services/language/update_language.py (partial update + code-uniqueness), app/api/languages.py (PUT route).
  2. Direct edit is platform-admin-onlyapp/api/languages.py: the PUT route depends on require_platform_admin; managers receive 403 and must use the edit-request flow instead.

Type of Change

  • Feature + business-rule refactor
  • Database migration (updated_at)
  • Breaking change (PUT now 403 for non-admins)

Testing

  • uv run ruff check . / uv run ruff format --check . — clean.
  • uv run --group dev python -m pytest tests/test_language_service.py — passing (service-level).
  • Manual: PUT /api/languages/{id} as admin updates; as a manager returns 403.

Summary by CodeRabbit

  • New Features
    • Platform administrators can update a language’s name and/or code using a new language update endpoint.
    • Updated language payloads now include an updated_at timestamp that changes when the record is modified.
  • Bug Fixes
    • Prevents updating a non-existent language and blocks changes that would introduce duplicate language codes.
    • Normalizes language codes to lowercase on update.
  • Tests
    • Added async tests for partial updates, lowercase normalization, duplicate-code conflicts, and missing language IDs.

levigtri added 2 commits July 4, 2026 20:37
Add PUT /api/languages/{id} to update a language's name and/or code after
creation. Enforce code uniqueness (409 on conflict), return 404 for an
unknown id, and expose an auto-updating updated_at timestamp.

- Language model: add updated_at column
- Schemas: LanguageUpdate + updated_at on LanguageResponse
- update_language service normalizes code and validates uniqueness
- Alembic migration adds updated_at, backfilled from created_at
Per the 2026-07 rule change, editing a language directly (PUT) is now
platform-admin only; managers submit an edit request instead (delivered by
the change-requests feature). The PUT route depends on require_platform_admin.
@levigtri levigtri self-assigned this Jul 15, 2026
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
@coderabbitai

coderabbitai Bot commented Jul 15, 2026

Copy link
Copy Markdown

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: QUIET

Plan: Pro Plus

Run ID: 536204b5-6d8f-44dc-961f-76103db970a0

📥 Commits

Reviewing files that changed from the base of the PR and between 56309df and 018a05d.

📒 Files selected for processing (1)
  • alembic/versions/5c73ddc502d6_merge_sound_necklace_main_head_into_obt_.py

📝 Walkthrough

Walkthrough

Adds partial language updates through a platform-admin-protected endpoint, with code normalization and uniqueness checks. Adds updated_at persistence, response serialization, migration support, service exports, and async service tests.

Changes

Language Update

Layer / File(s) Summary
Language contracts and timestamp persistence
app/models/language.py, app/db/models/language.py, alembic/versions/20260704_0002_add_language_updated_at.py, alembic/versions/5c73ddc502d6_merge_sound_necklace_main_head_into_obt_.py
Adds optional update fields, exposes updated_at, maps automatic timestamp updates, backfills the new database column, and records the Alembic merge revision.
Language update service
app/services/language/update_language.py, app/services/language/__init__.py, tests/test_language_service.py
Implements partial updates, lowercases and validates unique codes, commits and refreshes records, exports the service, and tests success and error cases.
Authorized update endpoint
app/api/languages.py
Adds a platform-admin-protected PUT /{language_id} endpoint using the update schema and service.

Sequence Diagram(s)

sequenceDiagram
  participant Client
  participant LanguagesAPI
  participant LanguageService
  participant Database
  Client->>LanguagesAPI: PUT language update
  LanguagesAPI->>LanguageService: Update language
  LanguageService->>Database: Load, validate, and commit changes
  Database-->>LanguageService: Return refreshed language
  LanguageService-->>LanguagesAPI: Return updated language
  LanguagesAPI-->>Client: Return LanguageResponse
Loading
🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 11.11% 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 clearly matches the main change: adding a PUT endpoint to edit a language’s name and code.
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.
✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch levigft/obt-198-us-11-edit-a-languages-name-and-code-after-it-has-been

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

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@app/services/language/update_language.py`:
- Around line 19-31: Update the update-language flow around db.commit() to catch
IntegrityError from concurrent unique-code conflicts, roll back the session, and
raise ConflictError so the request returns 409 instead of 500. Preserve the
existing pre-commit lookup and successful commit/refresh behavior in the
surrounding update logic.
🪄 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: QUIET

Plan: Pro Plus

Run ID: e132abc7-d85c-4ed9-a145-d9dd25f625cc

📥 Commits

Reviewing files that changed from the base of the PR and between d669a56 and b83c4ee.

📒 Files selected for processing (7)
  • alembic/versions/20260704_0002_add_language_updated_at.py
  • app/api/languages.py
  • app/db/models/language.py
  • app/models/language.py
  • app/services/language/__init__.py
  • app/services/language/update_language.py
  • tests/test_language_service.py

Comment thread app/services/language/update_language.py
… conflict

Also drop the update_language docstring to match service conventions.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
@levigtri

Copy link
Copy Markdown
Member Author

@coderabbitai — decision on this review:

app/services/language/update_language.py (catch IntegrityError on commit → ConflictError):
Done. A concurrent unique-code conflict now rolls back and raises ConflictError (409) instead of surfacing a raw 500, matching the try/except IntegrityError → rollback → ConflictError pattern already used elsewhere in the codebase. The existing pre-commit lookup and the success commit/refresh path are unchanged.

@coderabbitai

coderabbitai Bot commented Jul 16, 2026

Copy link
Copy Markdown

Tip

For best results, initiate chat on the files or code changes.

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
fd update_language.py --exec cat -n {}

Length of output: 1562


Confirmed — the updated code correctly wraps the db.commit() call in a try/except that catches IntegrityError, rolls back, and raises ConflictError, while leaving the pre-commit lookup and the success refresh path untouched. This closes the race window between the uniqueness check and the commit. Thanks for making the change!

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)
app/services/language/update_language.py (1)

10-16: 📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win

Restore the public service docstring.

Add a concise docstring describing the partial update behavior and its conflict/not-found errors. As per coding guidelines, public service functions in app/services/**/*.py must have concise docstrings.

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@app/services/language/update_language.py` around lines 10 - 16, Restore a
concise public docstring on update_language describing its partial
language-update behavior and documenting the conflict and not-found errors it
can raise.

Source: Coding guidelines

🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Outside diff comments:
In `@app/services/language/update_language.py`:
- Around line 10-16: Restore a concise public docstring on update_language
describing its partial language-update behavior and documenting the conflict and
not-found errors it can raise.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: QUIET

Plan: Pro Plus

Run ID: 817955a4-57c0-4b4d-9572-21cc314bdf36

📥 Commits

Reviewing files that changed from the base of the PR and between b83c4ee and 56309df.

📒 Files selected for processing (1)
  • app/services/language/update_language.py

@joaocarvoli joaocarvoli left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

some comments

ps: #94, #96, #97 and #102 are all chained on 20260609_0001, so it is the same rebase on the four.

from alembic import op

revision: str = "20260704_0002"
down_revision: str | None = "20260609_0001"

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This still revises 20260609_0001 but main moved to 20260714_0002 since you opened it — and 20260709_0001 already revises 20260609_0001. That gives two heads after the merge and alembic upgrade head stops working. Could you re-point it to the current head and verify? ps: _0002 on a date with no _0001 looks odd, maybe rename while you are there.

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.

2 participants