[US-2.1 / OBT-200] Deactivate a language (soft-delete) - #97
Conversation
Turn DELETE /api/languages/{id} into a soft-delete so projects linked to a
language are never broken and the language can be restored. Deactivated
languages are hidden from the default list; include_inactive=true reveals
them. The language response now exposes is_active.
- Language model: add is_active column (default true)
- deactivate_language service sets is_active=False
- list_languages filters is_active unless include_inactive
- Alembic migration adds is_active NOT NULL default true
Deactivating a language is now blocked for everyone — including platform admins — when any project still references it (409, listing the project names), and a non-admin may deactivate a language only if they created it (403 otherwise). To support ownership the languages table gains a nullable created_by FK (set from the current user on create) exposed on the response. - languages.created_by column + Alembic migration - create_language stamps the creator; POST passes the current user - deactivate_language(db, id, actor): creator/admin check + in-use block - LanguageResponse exposes created_by - tests for ownership, in-use block, and created_by on create
Co-Authored-By: Claude Opus 4.8 (1M context) <[email protected]>
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: QUIET Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughLanguages now track active status and creator attribution. Services support inactive filtering, visibility checks, and authorized soft deactivation, project operations reject inactive languages, and the API exposes creation, listing, visibility, and deactivation behavior. ChangesLanguage lifecycle controls
Sequence Diagram(s)sequenceDiagram
participant AuthenticatedUser
participant languages_api
participant language_service
participant Database
AuthenticatedUser->>languages_api: DELETE language_id
languages_api->>language_service: deactivate_language(language_id, actor)
language_service->>Database: load language
Database-->>language_service: language record
language_service->>Database: set is_active false and commit
Database-->>language_service: refreshed language
language_service-->>languages_api: deactivated language
languages_api-->>AuthenticatedUser: HTTP 204
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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/create_language.py`:
- Around line 8-14: Add a concise docstring to the public create_language
function describing that it creates and returns a language, while preserving its
existing validation and creation behavior. Use deactivate_language as the style
reference for the docstring.
In `@app/services/language/deactivate_language.py`:
- Around line 13-29: Enforce the active-language invariant in the project write
paths, updating the functions in create_project.py and update_project.py to
reject inactive target languages before persisting changes. Ensure both creation
and update flows share the same validation, and do not rely solely on
deactivate_language.py’s reference query or locking.
🪄 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: 9184a351-a3f9-464d-9b39-1ac595ccbd0f
📒 Files selected for processing (11)
alembic/versions/20260704_0003_add_language_is_active.pyalembic/versions/20260704_0004_add_language_created_by.pyapp/api/languages.pyapp/db/models/language.pyapp/models/language.pyapp/services/language/__init__.pyapp/services/language/create_language.pyapp/services/language/deactivate_language.pyapp/services/language/list_languages.pytests/baker.pytests/test_language_service.py
| async def create_language( | ||
| db: AsyncSession, name: str, code: str, created_by: str | None = None | ||
| ) -> Language: | ||
| existing = await get_language_by_code(db, code) | ||
| if existing: | ||
| raise ConflictError("Language code already exists") | ||
| language = Language(name=name, code=code.lower()) | ||
| language = Language(name=name, code=code.lower(), created_by=created_by) |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Add a docstring to create_language.
This public service function has no docstring, unlike sibling deactivate_language in the same package.
📝 Proposed fix
async def create_language(
db: AsyncSession, name: str, code: str, created_by: str | None = None
) -> Language:
+ """Create a language, raising ConflictError if the code already exists."""
existing = await get_language_by_code(db, code)As per coding guidelines, "Keep service functions function-oriented and composable, with concise docstrings on public service functions."
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| async def create_language( | |
| db: AsyncSession, name: str, code: str, created_by: str | None = None | |
| ) -> Language: | |
| existing = await get_language_by_code(db, code) | |
| if existing: | |
| raise ConflictError("Language code already exists") | |
| language = Language(name=name, code=code.lower()) | |
| language = Language(name=name, code=code.lower(), created_by=created_by) | |
| async def create_language( | |
| db: AsyncSession, name: str, code: str, created_by: str | None = None | |
| ) -> Language: | |
| """Create a language, raising ConflictError if the code already exists.""" | |
| existing = await get_language_by_code(db, code) | |
| if existing: | |
| raise ConflictError("Language code already exists") | |
| language = Language(name=name, code=code.lower(), created_by=created_by) |
🤖 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/create_language.py` around lines 8 - 14, Add a concise
docstring to the public create_language function describing that it creates and
returns a language, while preserving its existing validation and creation
behavior. Use deactivate_language as the style reference for the docstring.
Source: Coding guidelines
| language = await get_language_or_404(db, language_id) | ||
|
|
||
| if not actor.is_platform_admin and language.created_by != actor.id: | ||
| raise AuthorizationError("You can only deactivate languages you created") | ||
|
|
||
| result = await db.execute(select(Project.name).where(Project.language_id == language_id)) | ||
| project_names = list(result.scalars().all()) | ||
| if project_names: | ||
| joined = ", ".join(project_names) | ||
| raise ConflictError( | ||
| f"Cannot deactivate '{language.name}': it is used by " | ||
| f"{len(project_names)} project(s): {joined}" | ||
| ) | ||
|
|
||
| language.is_active = False | ||
| await db.commit() | ||
| await db.refresh(language) |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
rg -n -C 4 'Project\(|language_id|create_project' app tests --glob '*.py'Repository: shemaobt/tripod-api
Length of output: 50376
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n## deactivate_language\n'
sed -n '1,220p' app/services/language/deactivate_language.py
printf '\n## create_project\n'
sed -n '1,220p' app/services/project/create_project.py
printf '\n## update_project\n'
sed -n '1,220p' app/services/project/update_project.py
printf '\n## project model\n'
sed -n '1,220p' app/db/models/project.py
printf '\n## language model\n'
sed -n '1,220p' app/db/models/language.pyRepository: shemaobt/tripod-api
Length of output: 7520
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n## inactive/deactivate references in services and tests\n'
rg -n -C 3 'is_active|deactivate_language|deactivated|inactive language|language_id' app tests \
--glob '*.py' | sed -n '1,260p'
printf '\n## language service files\n'
find app/services/language -maxdepth 1 -type f | sortRepository: shemaobt/tripod-api
Length of output: 19941
Make inactive-language enforcement shared with project writes. The reference check here is still advisory: app/services/project/create_project.py and app/services/project/update_project.py can target an inactive language, so a concurrent project insert can still slip in after this query. Enforce the rule in every project write path or move it into the database; locking only in this service won’t preserve the invariant.
🤖 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/deactivate_language.py` around lines 13 - 29, Enforce
the active-language invariant in the project write paths, updating the functions
in create_project.py and update_project.py to reject inactive target languages
before persisting changes. Ensure both creation and update flows share the same
validation, and do not rely solely on deactivate_language.py’s reference query
or locking.
…; reject inactive language on project create/update Co-Authored-By: Claude Opus 4.8 (1M context) <[email protected]>
…k, and inactive-language rejection Co-Authored-By: Claude Opus 4.8 (1M context) <[email protected]>
|
@coderabbitai — decisions on this review:
|
|
Tip For best results, initiate chat on the files or code changes.
This all sounds consistent and closes the gap I raised. I'll look out for the corresponding changes in ✏️ Learnings added
|
There was a problem hiding this comment.
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/deactivate_language.py`:
- Around line 14-24: Add concise docstrings describing the purpose and behavior
of the public service functions deactivate_language, create_project, and
update_project. Apply the docstring change in
app/services/language/deactivate_language.py at lines 14-24,
app/services/project/create_project.py at lines 19-23, and
app/services/project/update_project.py at lines 22-23; make no other changes.
🪄 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: 1371f1cd-560e-458e-8b39-caa96b50ee12
📒 Files selected for processing (5)
app/services/language/deactivate_language.pyapp/services/project/create_project.pyapp/services/project/update_project.pytests/test_language_service.pytests/test_project_service.py
| if not actor.is_platform_admin: | ||
| if language.created_by != actor.id: | ||
| raise AuthorizationError("You can only deactivate languages you created") | ||
| result = await db.execute(select(Project.name).where(Project.language_id == language_id)) | ||
| project_names = list(result.scalars().all()) | ||
| if project_names: | ||
| joined = ", ".join(project_names) | ||
| raise ConflictError( | ||
| f"Cannot deactivate '{language.name}': it is used by " | ||
| f"{len(project_names)} project(s): {joined}" | ||
| ) |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Add concise docstrings to the changed public service functions.
app/services/language/deactivate_language.py#L14-L24: add a concise docstring abovedeactivate_language.app/services/project/create_project.py#L19-L23: add a concise docstring abovecreate_project.app/services/project/update_project.py#L22-L23: add a concise docstring aboveupdate_project.
As per coding guidelines, “Keep service functions function-oriented and composable, with concise docstrings on public service functions.”
📍 Affects 3 files
app/services/language/deactivate_language.py#L14-L24(this comment)app/services/project/create_project.py#L19-L23app/services/project/update_project.py#L22-L23
🤖 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/deactivate_language.py` around lines 14 - 24, Add
concise docstrings describing the purpose and behavior of the public service
functions deactivate_language, create_project, and update_project. Apply the
docstring change in app/services/language/deactivate_language.py at lines 14-24,
app/services/project/create_project.py at lines 19-23, and
app/services/project/update_project.py at lines 22-23; make no other changes.
Source: Coding guidelines
Co-Authored-By: Claude Opus 4.8 (1M context) <[email protected]>
Deactivating a language is now a platform-admin-only action, and it is allowed even when projects still reference the language. Managers can no longer deactivate languages they created, and `include_inactive` is honoured only for platform admins so managers never receive inactive languages. Co-Authored-By: Claude Opus 4.8 (1M context) <[email protected]>
list_languages_by_projects is the manager's list path and returned languages regardless of is_active, so a manager kept seeing a language after a platform admin deactivated it. Filter on is_active, and honour include_inactive only on the admin branch of the list route. This is why the branch now sits on top of US-2.1 (#97): is_active is introduced there, so the filter cannot be expressed on main. Co-Authored-By: Claude Opus 4.8 (1M context) <[email protected]>
The list route filters inactive languages, but GET /languages/{id} and
GET /languages/code/{code} did not: both resolved a language by primary key or
code with no is_active predicate while guarded only by get_current_user, so a
manager still received a deactivated language. The by-id path is reachable in the
normal product flow — every project response carries language_id, and a project
keeps referencing its language after an admin deactivates it.
Resolve both reads through get_visible_language_or_404 /
get_visible_language_by_code_or_404, which raise NotFoundError for a non-admin
when the language is inactive. get_language_or_404 keeps its behaviour: it is
shared with the approval and project paths, which must still resolve inactive
languages.
Co-Authored-By: Claude Opus 4.8 (1M context) <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
app/api/languages.py (1)
57-63: 🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy liftAllow creator-specific deactivation authorization.
app/api/languages.py#L57-L63: use the current-user dependency and enforce creator ownership/reference rules in the service, retaining the admin bypass.tests/test_language_service.py#L87-L91: expect successful deactivation for an unreferenced creator-owned language.tests/test_language_service.py#L114-L119: expectConflictError/HTTP 409 when the creator-owned language is referenced.🤖 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/api/languages.py` around lines 57 - 63, Update the delete endpoint’s user dependency in deactivate_language to use the current authenticated user, then enforce creator ownership and reference checks in language_service.deactivate_language while preserving the platform-admin bypass. In tests/test_language_service.py lines 87-91, expect successful deactivation for an unreferenced creator-owned language; in lines 114-119, expect ConflictError or HTTP 409 for a referenced creator-owned language.
🤖 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/get_visible_language_by_code_or_404.py`:
- Around line 9-13: Add docstrings to both public helpers,
get_visible_language_by_code_or_404 in
app/services/language/get_visible_language_by_code_or_404.py lines 9-13 and
get_visible_language_or_404 in
app/services/language/get_visible_language_or_404.py lines 9-13, documenting
that inactive languages are visible only to platform administrators while active
languages remain accessible.
---
Outside diff comments:
In `@app/api/languages.py`:
- Around line 57-63: Update the delete endpoint’s user dependency in
deactivate_language to use the current authenticated user, then enforce creator
ownership and reference checks in language_service.deactivate_language while
preserving the platform-admin bypass. In tests/test_language_service.py lines
87-91, expect successful deactivation for an unreferenced creator-owned
language; in lines 114-119, expect ConflictError or HTTP 409 for a referenced
creator-owned language.
🪄 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: 382eb7f5-5ca8-4d43-ab21-936a2fec6252
📒 Files selected for processing (5)
app/api/languages.pyapp/services/language/__init__.pyapp/services/language/get_visible_language_by_code_or_404.pyapp/services/language/get_visible_language_or_404.pytests/test_language_service.py
| async def get_visible_language_by_code_or_404(db: AsyncSession, code: str, actor: User) -> Language: | ||
| language = await get_language_by_code(db, code) | ||
| if language is None or (not language.is_active and not actor.is_platform_admin): | ||
| raise NotFoundError("Language not found") | ||
| return language |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Add docstrings to both public visibility helpers.
app/services/language/get_visible_language_by_code_or_404.py#L9-L13: document the inactive-language visibility rule.app/services/language/get_visible_language_or_404.py#L9-L13: document the same rule for ID-based lookup.
📍 Affects 2 files
app/services/language/get_visible_language_by_code_or_404.py#L9-L13(this comment)app/services/language/get_visible_language_or_404.py#L9-L13
🤖 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/get_visible_language_by_code_or_404.py` around lines 9
- 13, Add docstrings to both public helpers, get_visible_language_by_code_or_404
in app/services/language/get_visible_language_by_code_or_404.py lines 9-13 and
get_visible_language_or_404 in
app/services/language/get_visible_language_or_404.py lines 9-13, documenting
that inactive languages are visible only to platform administrators while active
languages remain accessible.
Source: Coding guidelines
| from alembic import op | ||
|
|
||
| revision: str = "20260704_0003" | ||
| down_revision: str | None = "20260609_0001" |
There was a problem hiding this comment.
main moved since july/4 — 20260709_0001_add_oc_acousteme_artifacts also revises 20260609_0001, and the chain continues until 20260714_0002. So merging this gives two heads and alembic upgrade head breaks on the deploy. Could you re-point this one to 20260714_0002?
| ) -> list[LanguageResponse]: | ||
| languages = await language_service.list_languages(db) | ||
| languages = await language_service.list_languages( | ||
| db, include_inactive=include_inactive and user.is_platform_admin |
There was a problem hiding this comment.
The "only admin sees inactive" rule lives here on the router, but the two new get_visible_* take the actor and decide inside the service. So list_languages(db, include_inactive=True) still returns inactive rows for any caller that forgets the and — and it is the only visibility rule of this PR without a service-level test. Maybe pass the user and decide inside list_languages like the other two? I saw my_apps on apps.py passing is_admin= through, so please verify which one is majority and adapt.
|
|
||
| async def get_visible_language_or_404(db: AsyncSession, language_id: str, actor: User) -> Language: | ||
| language = await get_language_or_404(db, language_id) | ||
| if not language.is_active and not actor.is_platform_admin: |
There was a problem hiding this comment.
Just make sure here about the grandfathered projects: ProjectResponse carries only language_id, so the project detail page resolves the name through this path. After an admin deactivates, the manager keeps a working project but gets 404 on its own language. Is that the intended behavior? Not asking for a display field, that is out of scope — only to confirm the decision covers this case.
2 participants
Summary
Turns the language delete into a soft-delete and enforces the revised role hierarchy: deactivating a language is platform-admin only, and is allowed even when projects still reference it. The project-usage list becomes a warning (surfaced by US-2.2's stats endpoint and the console dialog), never a block. Managers cannot deactivate — not even languages they created — and can never see an inactive language through any language read path.
Changes
is_active+created_bycolumns on languages —app/db/models/language.py,alembic/versions/20260704_0003_add_language_is_active.py,alembic/versions/20260704_0004_add_language_created_by.py.created_byis a nullable FK tousers.id(ON DELETE SET NULL), kept for attribution/display only — it grants no rights.app/services/language/create_language.py,app/api/languages.py.app/services/language/deactivate_language.pyraises403("Only platform admins can deactivate languages") for any non-admin actor, regardless ofcreated_byand regardless of project usage.DELETE /api/languages/{id}is guarded byrequire_platform_admin. There is no in-use block any more.GET /api/languages—include_inactiveis honoured only for platform admins, so a manager cannot opt into inactive rows.GET /api/languages/{id}andGET /api/languages/code/{code}— resolved through the newget_visible_language_or_404/get_visible_language_by_code_or_404, which raiseNotFoundErrorfor a non-admin when the language is inactive. Previously both were bare primary-key/code lookups behindget_current_userwith nois_activepredicate, so they bypassed the list filtering. The by-id hole was reachable in the normal product flow: every project response carrieslanguage_id, and a project keeps referencing its language after deactivation, so a manager opening a project detail page fetched and rendered the deactivated language.get_language_or_404is deliberately left as-is: the approval and project paths still need to resolve inactive rows.created_by—app/models/language.py.app/services/project/create_project.py,update_project.py:400("Language is not active"), so the soft-delete cannot be bypassed by pointing a project at a deactivated language. Existing projects are grandfathered.Type of Change
DELETEnow returns403for every non-admin; by-id/by-code return404for non-admins on inactive rows)Testing
tests/test_language_service.py,tests/test_project_service.py,tests/baker.py:403; non-creator non-admin →403; creator with the language in use →403.404.created_bystamped on create.include_inactivedefault hides inactive; admin sees them.404for an inactive language by id and by code; admin gets the row in both cases; non-admin still gets active languages; unknown code →404.Each test run in its own process (the suite's sqlite fixture races when a whole file shares one).
ruff check+ruff format --checkclean.Summary by CodeRabbit