[WEB-5038] fix: cycle creation in external api endpoint#7866
[WEB-5038] fix: cycle creation in external api endpoint#7866sriramveeraghanta merged 4 commits intopreviewfrom
Conversation
* Updated CycleListCreateAPIEndpoint to assign the current user as the owner when the 'owned_by' field is not included in the request data. * Enhanced the CycleCreateSerializer initialization to ensure proper ownership assignment during cycle creation.
* Introduced a new test suite for Cycle API endpoints, covering creation, retrieval, updating, and deletion of cycles. * Implemented tests for various scenarios including successful operations, invalid data handling, and conflict resolution with external IDs. * Enhanced test coverage for listing cycles with different view filters and verifying cycle metrics annotations.
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. WalkthroughMove ownership defaulting into the serializer: the view now instantiates serializers with an explicit request context and no longer injects Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor User
participant API as Cycle View
participant Serializer as CycleCreateSerializer
participant DB as Database
User->>API: POST /cycles { payload (may omit owned_by) }
API->>Serializer: init(data=request.data, context={"request": request})
alt serializer.validate sees no owned_by
Serializer->>Serializer: set owned_by = request.user
end
Serializer->>DB: save Cycle (with owned_by)
DB-->>Serializer: Cycle created
Serializer-->>API: return instance / validated data
API-->>User: 201 Created (Cycle response)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Suggested labels
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ 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. Comment |
|
Linked to Plane Work Item(s) This comment was auto-generated by Plane |
There was a problem hiding this comment.
Pull Request Overview
Fixes cycle creation in external API endpoint by automatically assigning the current user as owner when the 'owned_by' field is not provided in the request data.
- Sets current user as cycle owner when 'owned_by' is missing from request
- Adds comprehensive test coverage for cycle API endpoints
- Handles the case where external API calls don't specify ownership
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| apps/api/plane/api/views/cycle.py | Modified CycleListCreateAPIEndpoint to auto-assign current user as owner |
| apps/api/plane/tests/contract/api/test_cycles.py | Added comprehensive test suite for cycle API endpoints |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
![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
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/cycle.py (1)
349-349: Remove unconditionalowned_byoverride
Passingowned_by=request.usertoserializer.savealways overwrites any client-supplied value (the precedingdata["owned_by"] = request.user.iddefault already covers the “missing” case). Change to:- serializer.save(project_id=project_id, owned_by=request.user) + serializer.save(project_id=project_id)Add a test asserting that when
owned_byis provided in the request payload, it’s persisted unchanged.
🧹 Nitpick comments (5)
apps/api/plane/api/views/cycle.py (2)
318-324: Avoid mutating request.data and align defaulting with spec ("when not included").
- Mutating request.data directly can have side effects (QueryDict immutability, audit logging).
- Current check uses truthiness; it will override if owned_by="" or 0. The PR objective says “when the field is not included”.
Suggested change:
- # If owned_by is not provided, set it to the current user - data = request.data - if not data.get("owned_by"): - data["owned_by"] = request.user.id + # If 'owned_by' is not present in payload, default to current user + data = request.data.copy() + owned_by_provided = "owned_by" in data + if not owned_by_provided: + data["owned_by"] = request.user.idPlease confirm whether empty-string owned_by should be treated as “not provided.” If yes, we can adjust the presence check accordingly.
324-324: Optional: Pass project context for timezone-aware validation.CycleCreateSerializer uses context["project"] to set field timezones. Consider:
- serializer = CycleCreateSerializer(data=data) + # Optional: provide project for timezone-aware fields in the serializer + project = Project.objects.get(workspace__slug=slug, pk=project_id) + serializer = CycleCreateSerializer(data=data, context={"project": project})This keeps behavior consistent with GET where context is supplied.
apps/api/plane/tests/contract/api/test_cycles.py (3)
71-87: Strengthen ownership assertion to verify defaulting picks the requester.Also assert the owner equals the authenticated user, and inject the user fixture.
Apply:
- def test_create_cycle_success(self, api_key_client, workspace, project, cycle_data): + def test_create_cycle_success(self, api_key_client, workspace, project, cycle_data, create_user): @@ - created_cycle = Cycle.objects.first() + created_cycle = Cycle.objects.first() assert created_cycle.name == cycle_data["name"] assert created_cycle.description == cycle_data["description"] assert created_cycle.project == project - assert created_cycle.owned_by_id is not None + assert created_cycle.owned_by_id == create_user.idIf the API key authenticates a different user than create_user in your fixtures, bind the assertion to that user instead.
136-163: Add coverage: ensuring client-supplied owned_by is honored.To prevent regressions of the overwrite bug, add a test like:
+ @pytest.mark.django_db + def test_create_cycle_respects_owned_by_from_payload(self, api_key_client, workspace, project, create_user): + url = self.get_cycle_url(workspace.slug, project.id) + # Create another member to assign as owner + other_owner = ProjectMember.objects.create(project=project, member=create_user, role=5, is_active=True).member + payload = {"name": "Owned By Other", "owned_by": other_owner.id} + response = api_key_client.post(url, payload, format="json") + assert response.status_code == status.HTTP_201_CREATED + created = Cycle.objects.get(name="Owned By Other") + assert created.owned_by_id == other_owner.id
1-9: Nit: remove unused imports to keep tests tidy.IntegrityError and datetime are unused. Safe to drop.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
apps/api/plane/api/views/cycle.py(1 hunks)apps/api/plane/tests/contract/api/test_cycles.py(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
apps/api/plane/api/views/cycle.py (1)
apps/api/plane/api/serializers/cycle.py (1)
CycleCreateSerializer(11-80)
apps/api/plane/tests/contract/api/test_cycles.py (3)
apps/api/plane/db/models/cycle.py (1)
Cycle(56-97)apps/api/plane/db/models/project.py (2)
Project(65-175)ProjectMember(212-256)apps/api/plane/tests/conftest.py (3)
workspace(122-138)create_user(33-42)api_key_client(57-60)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: Lint API
- GitHub Check: Build and lint web apps
- GitHub Check: Cursor Bugbot
- GitHub Check: Analyze (javascript)
* Added 'owned_by' field to CycleCreateSerializer to specify the user who owns the cycle. * Updated CycleListCreateAPIEndpoint to remove redundant ownership assignment logic, relying on the serializer to handle default ownership. * Ensured that if 'owned_by' is not provided, it defaults to the current user during cycle creation.
* Updated the assertion in the test for successful cycle creation to use the correct syntax for checking the response status code. * Ensured that the test accurately verifies the expected behavior of the API endpoint.
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
apps/api/plane/api/serializers/cycle.py (2)
102-106: Remove duplicateowned_byfield declaration.Since
owned_byis now included inCycleCreateSerializer.Meta.fields(line 41), adding it again inCycleUpdateSerializer.Meta.fieldsis redundant. The child serializer automatically inherits the parent's fields.Apply this diff to remove the duplication:
class Meta(CycleCreateSerializer.Meta): model = Cycle - fields = CycleCreateSerializer.Meta.fields + [ - "owned_by", - ] + fields = CycleCreateSerializer.Meta.fieldsOr simply remove the entire
fieldsdeclaration if no additional fields are needed:class Meta(CycleCreateSerializer.Meta): model = Cycle - fields = CycleCreateSerializer.Meta.fields + [ - "owned_by", - ]
1-1: Split the long help_text literal on line 23 to satisfy E501
The string in apps/api/plane/api/serializers/cycle.py:23 exceeds 88 characters; wrap or split it across lines.
🧹 Nitpick comments (1)
apps/api/plane/api/serializers/cycle.py (1)
19-24: Removeallow_null=Trueto match the model constraint.The
owned_byfield on theCyclemodel is a non-nullableForeignKey(seeapps/api/plane/db/models/cycle.pylines 58-62). While the validation logic at lines 88-89 will default null values torequest.user, allowingnullin the serializer is misleading since null values are never persisted.Apply this diff:
owned_by = serializers.PrimaryKeyRelatedField( queryset=User.objects.all(), required=False, - allow_null=True, help_text="User who owns the cycle. If not provided, defaults to the current user.", )
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
apps/api/plane/api/serializers/cycle.py(3 hunks)apps/api/plane/api/views/cycle.py(3 hunks)apps/api/plane/tests/contract/api/test_cycles.py(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- apps/api/plane/api/views/cycle.py
- apps/api/plane/tests/contract/api/test_cycles.py
🧰 Additional context used
🧬 Code graph analysis (1)
apps/api/plane/api/serializers/cycle.py (2)
apps/api/plane/db/models/cycle.py (2)
Cycle(56-97)CycleIssue(100-127)apps/api/plane/db/models/user.py (1)
User(38-170)
🪛 GitHub Actions: Build and lint API
apps/api/plane/api/serializers/cycle.py
[error] 1-1: ruff check --fix apps/api failed: Found 3 errors (2 fixed, 1 remaining).
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Build and lint web apps
- GitHub Check: Analyze (javascript)
🔇 Additional comments (3)
apps/api/plane/api/serializers/cycle.py (3)
7-7: LGTM!The
Userimport is necessary for the newPrimaryKeyRelatedFieldand correctly placed with other model imports.
41-41: LGTM!Adding
owned_byto the serializer fields correctly exposes the field in the public API, allowing clients to explicitly specify the cycle owner when needed.
88-89: Confirmrequestcontext is always passed to CycleCreateSerializer. Views consistently instantiate the serializer withcontext={'request': request}, soself.context['request'].useris safe.
Description
Type of Change
Test Scenarios
owned_byin the payloadReferences
WEB-5038
Note
Defaults
owned_byto the authenticated user on cycle creation and adds comprehensive contract tests for cycles endpoints.CycleListCreateAPIEndpoint.postinapps/api/plane/api/views/cycle.py: defaultowned_byto the current user when not provided; pass updated data toCycleCreateSerializer.apps/api/plane/tests/contract/api/test_cycles.pycovering:current,upcoming,completed,draft)Written by Cursor Bugbot for commit bb9f4fa. This will update automatically on new commits. Configure here.
Summary by CodeRabbit
New Features
Bug Fixes
Tests