fix(api): pass project_lead_id (not User instance) when creating ProjectMember#8966
Conversation
Cover three scenarios: - project_lead set to the creator's own user_id - project_lead set to a different workspace member - project_lead omitted (baseline) The first two currently fail on preview because of a UUID coercion bug in ProjectMember.objects.create — see follow-up commit.
…ectMember The create-project endpoint built a ProjectMember row with member_id=serializer.instance.project_lead, which resolves to a User instance via Django's related descriptor instead of a UUID. Django's UUIDField coercion then fails with AttributeError: 'User' object has no attribute 'replace', which the generic exception handler converts to a 400 "Please provide valid detail" — but only after the Project row was already persisted, leaving an orphaned project without default states. Fix: - Use project_lead_id (FK ID, no descriptor lookup) on both the guard comparison and the ProjectMember creation. - Wrap the post-save flow in transaction.atomic() so any future exception triggers a clean rollback. - Defer model_activity.delay() with transaction.on_commit() so the activity log only fires after a successful commit. - Capture the exception with log_exception() in the generic catch so future regressions surface in api logs. Note: a related data integrity issue exists where ProjectCreateSerializer doesn't create a ProjectIdentifier row (unlike its frontend counterpart). Out of scope here, will follow up in a separate PR.
jamartineztelecoengineer84-dotcom
requested review from
dheeru0198 and
pablohashescobar
as code owners
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughProject creation now runs inside Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant API as "API (ProjectListCreateAPIEndpoint)"
participant DB as "Database\n(transaction)"
participant OnCommit as "transaction.on_commit"
participant Worker as "Task Broker / Worker"
Client->>API: POST /workspaces/{slug}/projects/ (payload)
API->>DB: begin atomic transaction
API->>DB: serializer.save() -> create Project
API->>DB: create ProjectMember (creator admin)
alt project_lead_id present and != creator
API->>DB: create ProjectMember (lead admin)
end
API->>DB: State.objects.bulk_create(...) (default States)
DB-->>API: commit transaction
API->>OnCommit: register model_activity.delay(...), robust=True
OnCommit->>Worker: schedule task (after commit)
Worker-->>API: async activity processing (later)
alt error occurs before commit
DB-->>API: rollback
OnCommit--x Worker: no task dispatched
API-->>Client: 500 Internal Server Error
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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. Review rate limit: 7/8 reviews remaining, refill in 7 minutes and 30 seconds.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: 1
🧹 Nitpick comments (3)
apps/api/plane/tests/contract/api/test_projects.py (2)
11-26: Nit: simplify the fixture withcreate_userand a top-level import.
User.objects.create(...)followed byset_password+savedoes two writes and leaves the password unhashed momentarily.User.objects.create_user(...)handles hashing in one step and is the idiomatic Django pattern. Also, thefrom uuid import uuid4is more naturally a module-level import.♻️ Optional cleanup
import pytest +from uuid import uuid4 from rest_framework import status from plane.db.models import Project, ProjectMember, State, User, WorkspaceMember @@ `@pytest.fixture` def other_workspace_member(db, workspace): """Create another user that is a member of the workspace, distinct from the creator.""" - from uuid import uuid4 - unique_id = uuid4().hex[:8] - other = User.objects.create( + other = User.objects.create_user( email=f"other-{unique_id}@plane.so", username=f"other_user_{unique_id}", first_name="Other", last_name="User", + password="test-password", ) - other.set_password("test-password") - other.save() WorkspaceMember.objects.create(workspace=workspace, member=other, role=20) return other🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/api/plane/tests/contract/api/test_projects.py` around lines 11 - 26, The fixture other_workspace_member should use Django's User.objects.create_user to set the password in one step (avoiding create + set_password + save) and move the "from uuid import uuid4" to a top-level import; update the fixture to call create_user(...) with the same email/username/first_name/last_name values and use uuid4().hex[:8] for uniqueness, and keep WorkspaceMember.objects.create(...) unchanged.
67-88: Optional: also assert the total admin count in the "other lead" case.
exists()on each user verifies both rows are present, but won't catch a regression where an extra/duplicate admin row gets inserted (e.g., a future change accidentally re-introduces the descriptor coercion path or adds a second admin grant). Pinningcount() == 2makes the invariant explicit.🧪 Optional tightening
# Both creator and other_workspace_member are admins. assert ProjectMember.objects.filter(project=project, member=create_user, role=20).exists() assert ProjectMember.objects.filter(project=project, member=other_workspace_member, role=20).exists() + assert ProjectMember.objects.filter(project=project, role=20).count() == 2 assert State.objects.filter(project=project).count() == 5🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/api/plane/tests/contract/api/test_projects.py` around lines 67 - 88, The test test_create_project_with_lead_as_other_user currently asserts each admin row exists but doesn't ensure there are exactly two admins; add an assertion that checks ProjectMember entries for the created project with role=20 have count() == 2 (e.g., using ProjectMember.objects.filter(project=project, role=20).count() == 2) to guard against duplicates or extra admin grants while keeping the existing per-user exists() checks.apps/api/plane/api/views/project.py (1)
264-282: Minor: defensive check that requeriedprojectis not None.
self.get_queryset()filters byis_member=request.user (active)ORnetwork=2. The creator membership added on line 231 will satisfy this in practice, so.first()is expected to return the row. If anything ever changes that invariant (e.g., a future filter onarchived_at, network defaults, or the queryset gaining a permission scope),projectcould beNone— thetransaction.on_commitlambda would thenAttributeErrorat commit time (after the atomic block exits, so the project would already be persisted), andProjectSerializer(project).dataon line 281 would also misbehave.A cheap guard via
serializer.instance.idfor the activity task removes the subtle dependency:🪶 Optional tightening
- project = self.get_queryset().filter(pk=serializer.instance.id).first() + project_id = serializer.instance.id + project = self.get_queryset().filter(pk=project_id).first() # Defer the activity-log task until the surrounding # transaction commits, so it never fires on a rolled-back # creation. transaction.on_commit( lambda: model_activity.delay( model_name="project", - model_id=str(project.id), + model_id=str(project_id), requested_data=request.data, current_instance=None, actor_id=request.user.id, slug=slug, origin=base_host(request=request, is_app=True), ) )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/api/plane/api/views/project.py` around lines 264 - 282, The code assumes project = self.get_queryset().filter(pk=serializer.instance.id).first() is non-null, but .first() can return None and later transaction.on_commit lambda (and ProjectSerializer(project)) would raise AttributeError; fix by using serializer.instance.id directly inside the on_commit call and avoid dereferencing project there, and add a defensive fallback before serializing (e.g., if project is None, re-fetch with get(pk=serializer.instance.id) or use serializer.instance to build response) so model_activity() is scheduled with model_id=str(serializer.instance.id) and ProjectSerializer is only called with a non-None object (reference symbols: self.get_queryset(), project, serializer.instance.id, transaction.on_commit, model_activity, ProjectSerializer).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/api/plane/api/views/project.py`:
- Around line 297-304: The generic exception handler in the view currently
catches all exceptions, logs them via log_exception(e), and returns a 400 with
"Please provide valid detail" which misclassifies server errors as client
errors; update the fallback to return a 500 (use
status.HTTP_500_INTERNAL_SERVER_ERROR) with a similarly generic public message
while keeping log_exception(e) so unexpected AttributeError/KeyError/DB/Celery
errors surface as server errors; keep explicit validation paths returning 400
unchanged and ensure you modify the Response construction in the same except
block that calls log_exception(e).
---
Nitpick comments:
In `@apps/api/plane/api/views/project.py`:
- Around line 264-282: The code assumes project =
self.get_queryset().filter(pk=serializer.instance.id).first() is non-null, but
.first() can return None and later transaction.on_commit lambda (and
ProjectSerializer(project)) would raise AttributeError; fix by using
serializer.instance.id directly inside the on_commit call and avoid
dereferencing project there, and add a defensive fallback before serializing
(e.g., if project is None, re-fetch with get(pk=serializer.instance.id) or use
serializer.instance to build response) so model_activity() is scheduled with
model_id=str(serializer.instance.id) and ProjectSerializer is only called with a
non-None object (reference symbols: self.get_queryset(), project,
serializer.instance.id, transaction.on_commit, model_activity,
ProjectSerializer).
In `@apps/api/plane/tests/contract/api/test_projects.py`:
- Around line 11-26: The fixture other_workspace_member should use Django's
User.objects.create_user to set the password in one step (avoiding create +
set_password + save) and move the "from uuid import uuid4" to a top-level
import; update the fixture to call create_user(...) with the same
email/username/first_name/last_name values and use uuid4().hex[:8] for
uniqueness, and keep WorkspaceMember.objects.create(...) unchanged.
- Around line 67-88: The test test_create_project_with_lead_as_other_user
currently asserts each admin row exists but doesn't ensure there are exactly two
admins; add an assertion that checks ProjectMember entries for the created
project with role=20 have count() == 2 (e.g., using
ProjectMember.objects.filter(project=project, role=20).count() == 2) to guard
against duplicates or extra admin grants while keeping the existing per-user
exists() checks.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: bb7337ef-d7bb-49e8-a014-4ecc520ead31
📒 Files selected for processing (2)
apps/api/plane/api/views/project.pyapps/api/plane/tests/contract/api/test_projects.py
SuggestionsThanks for the thorough write-up and the reproduction — the root-cause analysis is exemplary. A few suggestions before merging: Should address in this PR
Test improvements
Style nit
Follow-up (already noted in your description)
Already great — don't change
|
Address review feedback from @sriramveeraghanta on PR makeplane#8966: - The catch-all `except Exception` now returns 500 instead of 400. Reusing the generic 400 response on a server-side crash was the anti-pattern that hid the original ghost-create bug for nine months; a 500 lets clients distinguish between "bad input" and "server fault". - The `IntegrityError` branch no longer falls through silently when the message is unrecognised. It re-raises so the catch-all `except` logs the exception and returns a 500. - `transaction.on_commit()` now schedules `model_activity.delay` via `functools.partial` instead of a lambda, avoiding late-binding closure semantics. - `ProjectCreateSerializer.validate()` now rejects `project_lead` values that are not active workspace members, surfacing the error under the `project_lead` field key (rather than as `non_field_errors`) so API clients can react programmatically.
Address review feedback from @sriramveeraghanta on PR makeplane#8966: - The three existing tests now look up the created project via `Project.objects.get(id=response.data["id"])` instead of `.first()`. The assertion now fails for the right reason if the wrong project is returned by the endpoint. - New `test_create_project_with_lead_not_in_workspace_returns_400` guards the workspace-membership validation added to `ProjectCreateSerializer.validate()`. Expects a 400 with a field-shaped error and zero rows persisted. - New `test_model_activity_not_called_on_rollback` locks in the `transaction.on_commit()` semantics: when an exception is raised inside the atomic block (forced via mocking `State.objects.bulk_create`), the response is 500, no Project / ProjectMember / State rows are persisted, and the deferred `model_activity.delay` task is never dispatched. This prevents a future refactor from silently regressing the rollback contract.
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/api/plane/api/views/project.py (1)
267-314:⚠️ Potential issue | 🟠 MajorGuard the post-commit task dispatch to prevent callback failures from masking successful creates.
The
transaction.on_commit()callback executes synchronously after the atomic block commits, and by default (robust=False), any exception raised bymodel_activity.delay()propagates up and is caught by the outerexcept Exceptionhandler, returning a 500 despite the project, members, and states already being persisted. Userobust=Truewhen callingtransaction.on_commit()so that broker errors or task dispatch failures are logged internally without surfacing as a server error to the client:Suggested fix
transaction.on_commit( partial( model_activity.delay, model_name="project", model_id=str(project.id), requested_data=request.data, current_instance=None, actor_id=request.user.id, slug=slug, origin=base_host(request=request, is_app=True), ), robust=True, )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/api/plane/api/views/project.py` around lines 267 - 314, The on_commit callback currently uses transaction.on_commit(partial(model_activity.delay, ...)) without robust=True so exceptions from model_activity.delay can propagate and trigger the outer Exception handler; update the call to pass robust=True to transaction.on_commit (keep the same partial with model_activity.delay, model_name="project", model_id=str(project.id), requested_data=request.data, current_instance=None, actor_id=request.user.id, slug=slug, origin=base_host(...)) so task dispatch failures are logged internally and do not return 500 after a successful project create.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@apps/api/plane/api/views/project.py`:
- Around line 267-314: The on_commit callback currently uses
transaction.on_commit(partial(model_activity.delay, ...)) without robust=True so
exceptions from model_activity.delay can propagate and trigger the outer
Exception handler; update the call to pass robust=True to transaction.on_commit
(keep the same partial with model_activity.delay, model_name="project",
model_id=str(project.id), requested_data=request.data, current_instance=None,
actor_id=request.user.id, slug=slug, origin=base_host(...)) so task dispatch
failures are logged internally and do not return 500 after a successful project
create.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: feb47152-577d-4f5d-ba6d-d6a4bb296cf5
📒 Files selected for processing (3)
apps/api/plane/api/serializers/project.pyapps/api/plane/api/views/project.pyapps/api/plane/tests/contract/api/test_projects.py
🚧 Files skipped from review as they are similar to previous changes (1)
- apps/api/plane/tests/contract/api/test_projects.py
|
Thanks @sriramveeraghanta — really appreciate the depth of the review. All seven points addressed in two follow-up commits: Server-side (
Tests (
5/5 tests pass on this branch (3 original + 2 new). Happy to address anything else. |
Address coderabbit re-review feedback on PR makeplane#8966. Without robust=True, an exception raised by model_activity.delay (e.g., a Celery broker outage) propagates out of the on_commit callback and is caught by the outer `except Exception` handler, which returns a 500 despite the project, ProjectMember rows and default States having already been committed. The client sees a 500 and assumes the create failed — the same class of mismatch between actual state and reported status that the original bug exhibited, just at the post-commit phase. Set robust=True so Django logs the dispatch failure internally via the standard transaction logger and the response stays 201, reflecting the persisted state. Switch from `functools.partial` to a nested function (`_dispatch_model_activity`) for the on_commit callable. Django's robust on_commit logging path reads `func.__qualname__` to format the error message; `partial` objects lack that dunder by default, and the `functools.update_wrapper` workaround turns out to be brittle when the wrapped callable is replaced by a Mock (which the new regression test relies on). A nested function exposes `__qualname__` natively, and the locals it closes over are bound at definition time and never rebound before the callback fires, so the late-binding-closure motivation for `partial` over `lambda` does not apply here. A new test, test_response_still_201_when_broker_dispatch_fails, mirrors test_model_activity_not_called_on_rollback to lock in the post-commit branch. It uses `@pytest.mark.django_db(transaction=True)` so the surrounding test transaction is actually committed and the `on_commit` callback fires (the default wrapper suppresses it via rollback).
There was a problem hiding this comment.
Actionable comments posted: 1
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: cb48d587-752e-4460-803e-122460f80483
📒 Files selected for processing (2)
apps/api/plane/api/views/project.pyapps/api/plane/tests/contract/api/test_projects.py
Address coderabbit re-review feedback on PR makeplane#8966. The previous fix used `raise` inside the IntegrityError handler with the intent of "letting the catch-all `except Exception` below log it and return 500". Coderabbit correctly flagged that `raise` exits the try/except entirely — sibling except clauses don't fire — so unrecognised integrity errors actually skipped `log_exception` and the consistent 500 JSON shape, contradicting the stated intent. Replicate the catch-all behaviour inline: log the exception via `log_exception(e)` and return the same generic 500 response with `{"error": "An unexpected error occurred"}`. The client now gets a uniform error shape regardless of which `except` branch handled it.
|
Re: the Added Implementation note: while wiring the new regression test ( The new test uses 6/6 tests pass on this branch. |
|
Friendly ping @sriramveeraghanta — happy to address any further feedback when you have a moment. The two CodeRabbit follow-ups ( |
2 participants
Description
The public REST API endpoint
POST /api/v1/workspaces/{slug}/projects/returned
400 "Please provide valid detail"whenever the payloadincluded a
project_lead, even when the request was otherwise valid.Worse, the
Projectrow had already been persisted before theexception bubbled up, leaving an unusable orphan: no workflow states,
no
ProjectMemberentries, no default cycles.The bug had three contributing parts in
apps/api/plane/api/views/project.py:214 (def post):UUID vs User instance. The endpoint passed
serializer.instance.project_lead(the related descriptor, whichDjango resolves to a
Userinstance) where a UUID was expected:Django's
UUIDField.to_python(value=<User>)then crashed withAttributeError: 'User' object has no attribute 'replace'. Thegeneric exception handler in
BaseAPIView.handle_exceptionmappedany uncaught exception to
400 "Please provide valid detail"— amisleading response that hid the real failure.
Buggy lead-vs-creator guard. The guard compared
str(<User>)against
str(<UUID>), which never matched (different__str__outputs), so the buggy branch above was always entered when
project_leadwas provided — including the very common case wherethe creator is also the lead.
No transaction wrapping. There was no
transaction.atomic()around the post-save side effects (member creation, default state
seeding, queryset re-fetch), so any partial failure left the
database in an inconsistent state.
Reproduction (before fix)
Stack trace (with debug logging temporarily enabled in
BaseAPIView.handle_exception):Changes
apps/api/plane/api/views/project.pyproject_lead_id(FK column directly) for both the guardcomparison and
ProjectMember.objects.create(member_id=...). No moredescriptor-resolved
Userinstance leaking into a UUID field.!= request.user.id) instead of viastr()of two heterogeneous objects.is_valid()block inwith transaction.atomic():sothe
Project+ProjectMember+State.bulk_createall roll backas a unit if any future failure is added in between.
model_activity.delay(...)withtransaction.on_commit(...),so the activity-log task never fires on a rolled-back creation.
except Exception as e:branch that callslog_exception(e)before returning the generic 400 — keeps thepublic response stable but ensures unexpected regressions are
visible in API logs going forward.
apps/api/plane/tests/contract/api/test_projects.py(new file)Three contract tests covering the create-project endpoint:
test_create_project_with_lead_as_creator— regression for the bug(project_lead == creator).
test_create_project_with_lead_as_other_user— verifies bothcreator and lead become Admins of the project.
test_create_project_without_lead— baseline regression.The first two FAIL on
previewHEAD before this fix and PASS after.Type of Change
Test Scenarios
pytest plane/tests/contract/api/test_projects.py— 3/3 PASS onthis branch. Same 3 tests on
previewHEAD: 2 FAIL (the bug) + 1PASS (baseline).
ruff check apps/api/plane/api/views/project.py apps/api/plane/tests/contract/api/test_projects.py— All checks passed.ruff format --check ...— both files already formatted.POST /projects/withproject_lead = creator_id→before fix: 400 + ghost project (0 states, no member);
after fix: 201 + complete project (5 default states + creator as
Admin).
Note on follow-up
A related data-integrity issue exists where
ProjectCreateSerializer.create()(
apps/api/plane/api/serializers/project.py:135) does not createa corresponding
ProjectIdentifierrow, while its in-file siblingProjectSerializer.create()(line 276) and the frontend serializerapps/api/plane/app/serializers/project.py:95both do. This meansprojects created via the public API don't register their identifier
in the
project_identifierstable that tracks workspace-wideidentifier uniqueness. Out of scope for this PR; will follow up
separately.
References
The bug was not previously reported upstream (verified via issue +
PR search for
"Please provide valid detail","orphaned project","states not created","project create 400","transaction.atomic","create project api").Summary by CodeRabbit
Bug Fixes
Validation
Tests