feat: add custom fields backend models, API, and tests #8467
Conversation
- Add IssueProperty and IssuePropertyValue models - Add migration 0113 with unique constraints - Add ViewSets for property CRUD and bulk operations - Add serializers with type-specific validation - Add 30 tests (8 model, 10 serializer, 12 API) - all passing - Support 6 field types: text, number, date, boolean, select, multi_select
|
|
|
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. 📝 WalkthroughWalkthroughAdds project-scoped custom fields for issues: new IssueProperty and IssuePropertyValue models, serializers and serializer hooks, viewsets/endpoints for CRUD and bulk operations, URL registrations, and comprehensive unit and contract tests covering validation and atomic behavior. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Client
participant API as "Django URL / View"
participant Serializer
participant DB as "Models / DB"
Note over API,Serializer: Create/Update Issue with custom_fields
Client->>API: POST /projects/{p}/issues/ (payload + custom_fields)
API->>Serializer: validate() -> _handle_custom_fields (transaction start)
Serializer->>DB: SELECT IssueProperty WHERE project=p (validate keys/options)
DB-->>Serializer: property defs
Serializer->>DB: INSERT/UPDATE IssuePropertyValue (upsert) note right of DB: transactional writes
DB-->>Serializer: success
Serializer-->>API: validated data
API-->>Client: 201 Created (issue + custom_fields)
alt Bulk update
Client->>API: POST /projects/{p}/issues/{id}/custom_fields/bulk
API->>Serializer: validate bulk items (transaction start)
Serializer->>DB: validate properties, upsert values in transaction
DB-->>Serializer: per-item result / rollback on error
Serializer-->>API: aggregated per-item results
API-->>Client: 200 with per-item statuses
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
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 |
|
@vihar I’m planning to add this feature through multiple PRs in a step-by-step manner to keep reviews simple and the codebase maintainable. Could you please review this and share any suggestions? I’ll apply them here and in the next PRs as well. |
![cursor[bot]](https://cdn.statically.io/img/avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
This PR is being reviewed by Cursor Bugbot
Details
Your team is on the Bugbot Free tier. On this plan, Bugbot will review limited PRs each billing cycle for each member of your team.
To receive Bugbot reviews on all of your PRs, visit the Cursor dashboard to activate Pro and start your 14-day free trial.
![coderabbitai[bot]](https://cdn.statically.io/img/avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (9)
apps/api/plane/tests/unit/models/test_issue_property.py (1)
64-73: Consider strengthening the assertion for key uniqueness.Line 72 uses
startswithwhich is less precise than asserting the exact expected value. Based on the model's key generation logic, the second property should get"cost_center_id_1".🔎 Proposed refinement
# Second one should have incremented key - assert prop2.key.startswith("cost_center_id") + assert prop2.key == "cost_center_id_1"apps/api/plane/tests/contract/app/test_issue_property_api.py (1)
19-33: Consider extracting shared fixtures to reduce duplication.The
project_with_memberandissue_in_projectfixtures are duplicated across all three test classes. While this works correctly, consider moving them to a module-level fixture or aconftest.pyfile to improve maintainability.Example module-level fixture approach
At the top of the file, before the test classes:
@pytest.fixture def project_with_member(db, workspace, create_user): """Create a project with the test user as admin member""" project = Project.objects.create( name="Test Project", identifier="TEST", workspace=workspace, ) ProjectMember.objects.create( project=project, member=create_user, role=20, # Admin is_active=True, ) return project @pytest.fixture def issue_in_project(db, workspace, project_with_member): """Create an issue in the test project""" state = State.objects.create( name="Open", project=project_with_member, workspace=workspace, ) return Issue.objects.create( name="Test Issue", project=project_with_member, workspace=workspace, state=state, )Then remove the duplicate fixture definitions from each class.
Also applies to: 195-225, 283-312
apps/api/plane/db/models/issue.py (2)
907-930: Minor inconsistency insave()logic forsort_ordervskeygeneration.The key uniqueness check filters by
deleted_at__isnull=True(line 916), but thesort_ordercalculation (line 924-926) includes all properties (even soft-deleted ones). This is likely fine since sort_order doesn't need strict uniqueness, but it's worth noting the inconsistency.Also, consider adding the
deleted_at__isnull=Truefilter to the sort_order query for consistency:🔎 Suggested change (optional)
# Auto-increment sort_order if self._state.adding: last_order = IssueProperty.objects.filter( - project=self.project + project=self.project, + deleted_at__isnull=True ).aggregate(largest=models.Max("sort_order"))["largest"] if last_order is not None: self.sort_order = last_order + 10000
973-976: Redundant index on(issue, property).The
UniqueConstrainton["issue", "property"](line 963-967) already creates an index for this field combination. The explicitIndex(fields=["issue", "property"])on line 974 is redundant and adds unnecessary storage overhead.🔎 Suggested fix
indexes = [ - models.Index(fields=["issue", "property"]), models.Index(fields=["property"]), ]apps/api/plane/app/serializers/issue.py (2)
369-375: Optimize query by addingselect_related("property")for better performance.The existing values query at line 370-374 filters by
property__key, which triggers a join, but doesn't useselect_related. When iterating overexisting_valuesto buildexisting_map, each access toev.property.keywould trigger an additional query.🔎 Suggested fix
# Get existing values for this issue existing_values = IssuePropertyValue.objects.filter( issue=issue, property__key__in=custom_fields.keys(), deleted_at__isnull=True, - ) + ).select_related("property") existing_map = {ev.property.key: ev for ev in existing_values}
1299-1304: Consider adding date format validation.The validation for date type only checks
isinstance(value, str)but doesn't validate the format. Invalid date strings like"not-a-date"would pass validation.🔎 Suggested improvement
if property_type == "date" and value: # Date should be stored as ISO string if not isinstance(value, str): raise serializers.ValidationError({ "value": "Value must be a date string (YYYY-MM-DD) for date type" }) + # Validate date format + import re + if not re.match(r'^\d{4}-\d{2}-\d{2}$', value): + raise serializers.ValidationError({ + "value": "Value must be in YYYY-MM-DD format for date type" + }) + # Optionally validate it's a real date + from datetime import datetime + try: + datetime.strptime(value, '%Y-%m-%d') + except ValueError: + raise serializers.ValidationError({ + "value": "Value must be a valid date in YYYY-MM-DD format" + })apps/api/plane/app/views/project/property.py (3)
294-323: Consider bulk database operations for better performance.The current implementation calls
serializer.save()in a loop for each custom field, resulting in N database operations. For large custom field sets, this could be slow.Consider collecting valid values and using
bulk_createwithupdate_conflicts(Django 4.1+) or separating creates/updates into two bulk operations, similar to how_handle_custom_fieldsin the serializer works.🔎 Suggested approach
# Collect values to create and update values_to_create = [] values_to_update = [] for key, value in custom_fields.items(): prop = property_map[key] # Validate using serializer data = {"value": value, "property": prop.id} if key in existing_map: existing = existing_map[key] serializer = IssuePropertyValueSerializer( instance=existing, data=data, context={"project_id": project_id}, partial=True ) if serializer.is_valid(): existing.value = value values_to_update.append(existing) else: errors.append({key: serializer.errors}) else: serializer = IssuePropertyValueSerializer( data=data, context={"project_id": project_id} ) if serializer.is_valid(): values_to_create.append(IssuePropertyValue( issue_id=issue_id, property_id=prop.id, value=value, project_id=project_id, # ... other fields )) else: errors.append({key: serializer.errors}) # Bulk operations if values_to_create: IssuePropertyValue.objects.bulk_create(values_to_create) if values_to_update: IssuePropertyValue.objects.bulk_update(values_to_update, ["value"])
3-3: Remove unused import.
Prefetchis imported but not used in this file.🔎 Suggested fix
# Django imports from django.db import IntegrityError -from django.db.models import Prefetch
21-21: Remove unused import.
ProjectMemberis imported but not used in this file.🔎 Suggested fix
from plane.db.models import ( IssueProperty, IssuePropertyValue, Issue, - ProjectMember, )
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (12)
apps/api/plane/api/urls/project.pyapps/api/plane/app/serializers/__init__.pyapps/api/plane/app/serializers/issue.pyapps/api/plane/app/urls/issue.pyapps/api/plane/app/views/__init__.pyapps/api/plane/app/views/project/property.pyapps/api/plane/db/migrations/0113_custom_fields_issue_property.pyapps/api/plane/db/models/__init__.pyapps/api/plane/db/models/issue.pyapps/api/plane/tests/contract/app/test_issue_property_api.pyapps/api/plane/tests/unit/models/test_issue_property.pyapps/api/plane/tests/unit/serializers/test_issue_property.py
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-12-23T14:18:32.899Z
Learnt from: dheeru0198
Repo: makeplane/plane PR: 8339
File: apps/api/plane/db/models/api.py:35-35
Timestamp: 2025-12-23T14:18:32.899Z
Learning: Django REST Framework rate limit strings are flexible: only the first character of the time unit matters. Acceptable formats include: "60/s", "60/sec", "60/second" (all equivalent), "60/m", "60/min", "60/minute" (all equivalent), "60/h", "60/hr", "60/hour" (all equivalent), and "60/d", "60/day" (all equivalent). Abbreviations like "min" are valid and do not need to be changed to "minute". Apply this guidance to any Python files in the project that configure DRF throttling rules.
Applied to files:
apps/api/plane/app/serializers/__init__.pyapps/api/plane/app/views/__init__.pyapps/api/plane/db/models/issue.pyapps/api/plane/db/migrations/0113_custom_fields_issue_property.pyapps/api/plane/db/models/__init__.pyapps/api/plane/tests/contract/app/test_issue_property_api.pyapps/api/plane/tests/unit/models/test_issue_property.pyapps/api/plane/app/views/project/property.pyapps/api/plane/app/serializers/issue.pyapps/api/plane/tests/unit/serializers/test_issue_property.pyapps/api/plane/api/urls/project.pyapps/api/plane/app/urls/issue.py
📚 Learning: 2025-07-23T18:18:06.875Z
Learnt from: NarayanBavisetti
Repo: makeplane/plane PR: 7460
File: apps/api/plane/app/serializers/draft.py:112-122
Timestamp: 2025-07-23T18:18:06.875Z
Learning: In the Plane codebase serializers, workspace_id is not consistently passed in serializer context, so parent issue validation in DraftIssueCreateSerializer only checks project_id rather than both workspace_id and project_id. The existing project member authentication system already validates that users can only access projects they belong to, providing sufficient security without risking breaking functionality by adding workspace_id validation where the context might not be available.
Applied to files:
apps/api/plane/app/serializers/issue.pyapps/api/plane/tests/unit/serializers/test_issue_property.py
🧬 Code graph analysis (10)
apps/api/plane/app/serializers/__init__.py (1)
apps/api/plane/app/serializers/issue.py (3)
IssuePropertySerializer(1134-1232)IssuePropertyLiteSerializer(1235-1248)IssuePropertyValueSerializer(1251-1329)
apps/api/plane/app/views/__init__.py (1)
apps/api/plane/app/views/project/property.py (3)
IssuePropertyViewSet(25-120)IssuePropertyValueViewSet(123-230)BulkIssuePropertyValueEndpoint(233-362)
apps/api/plane/db/models/issue.py (7)
apps/api/plane/db/models/project.py (1)
ProjectBaseModel(175-184)apps/api/plane/db/models/webhook.py (3)
Meta(45-57)Meta(79-83)Meta(92-104)apps/api/plane/db/models/page.py (7)
Meta(56-60)Meta(98-110)Meta(121-125)Meta(136-148)Meta(165-169)save(66-73)save(171-178)apps/api/plane/db/models/state.py (1)
save(113-122)apps/api/plane/db/models/cycle.py (1)
save(84-93)apps/api/plane/db/models/view.py (1)
save(74-90)apps/api/plane/db/models/module.py (1)
save(111-120)
apps/api/plane/db/migrations/0113_custom_fields_issue_property.py (2)
apps/api/plane/utils/exporters/schemas/base.py (3)
DateTimeField(70-81)JSONField(127-140)BooleanField(97-105)apps/api/plane/db/models/project.py (1)
choices(32-33)
apps/api/plane/tests/contract/app/test_issue_property_api.py (7)
apps/api/plane/db/models/issue.py (3)
IssueProperty(859-933)IssuePropertyValue(936-979)Issue(108-250)apps/api/plane/db/models/state.py (1)
State(75-122)apps/api/plane/tests/conftest.py (2)
create_user(33-42)session_client(64-67)apps/api/plane/app/serializers/issue.py (2)
create(196-283)create(667-670)apps/api/plane/app/views/project/property.py (4)
create(71-86)create(156-223)get(334-362)post(242-331)apps/api/plane/settings/storage.py (1)
url(16-17)apps/api/plane/tests/unit/serializers/test_issue_property.py (1)
test_create_text_property(22-43)
apps/api/plane/tests/unit/models/test_issue_property.py (1)
apps/api/plane/db/models/issue.py (4)
IssueProperty(859-933)IssuePropertyValue(936-979)IssuePropertyTypeChoices(849-856)Issue(108-250)
apps/api/plane/app/views/project/property.py (2)
apps/api/plane/app/serializers/issue.py (5)
IssuePropertySerializer(1134-1232)IssuePropertyValueSerializer(1251-1329)IssuePropertyLiteSerializer(1235-1248)create(196-283)create(667-670)apps/api/plane/db/models/issue.py (6)
IssueProperty(859-933)IssuePropertyValue(936-979)Issue(108-250)save(182-246)save(475-524)save(907-930)
apps/api/plane/tests/unit/serializers/test_issue_property.py (2)
apps/api/plane/app/serializers/issue.py (5)
IssuePropertySerializer(1134-1232)IssuePropertyValueSerializer(1251-1329)IssuePropertyLiteSerializer(1235-1248)create(196-283)create(667-670)apps/api/plane/db/models/issue.py (7)
IssueProperty(859-933)IssuePropertyValue(936-979)IssuePropertyTypeChoices(849-856)Issue(108-250)save(182-246)save(475-524)save(907-930)
apps/api/plane/api/urls/project.py (1)
apps/api/plane/app/views/project/property.py (3)
IssuePropertyViewSet(25-120)IssuePropertyValueViewSet(123-230)BulkIssuePropertyValueEndpoint(233-362)
apps/api/plane/app/urls/issue.py (1)
apps/api/plane/app/views/project/property.py (3)
IssuePropertyViewSet(25-120)IssuePropertyValueViewSet(123-230)BulkIssuePropertyValueEndpoint(233-362)
⏰ 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). (1)
- GitHub Check: Cursor Bugbot
🔇 Additional comments (19)
apps/api/plane/db/models/__init__.py (1)
45-47: LGTM! Clean public API extension.The new exports for
IssueProperty,IssuePropertyValue, andIssuePropertyTypeChoicescorrectly extend the public API surface for the custom fields feature.apps/api/plane/app/serializers/__init__.py (1)
79-81: LGTM! Serializer exports properly integrated.The new serializer exports follow the established pattern and correctly expose the custom fields serialization layer.
apps/api/plane/app/views/__init__.py (1)
23-27: LGTM! View components properly exposed.The new view imports correctly extend the public API for custom fields management.
apps/api/plane/app/urls/issue.py (2)
30-33: LGTM! Custom fields views properly imported.The imports are clean and align with the new custom fields feature.
286-322: LGTM! URL routing correctly structured.The custom fields endpoints are well-organized with:
- Project-level property definitions (list, create, retrieve, update, delete)
- Issue-level property values (list, create, delete)
- Bulk operations endpoint for efficient multi-field updates
The HTTP method mappings follow DRF conventions and align with the ViewSet implementations.
apps/api/plane/tests/unit/models/test_issue_property.py (1)
15-264: LGTM! Comprehensive model test coverage.The test suite covers key scenarios:
- Auto-generated keys from names (with special characters)
- Key uniqueness handling for duplicates
- SELECT type with options
- Auto-incremented sort order
- Property value creation with multiple JSON types (text, number, boolean, multi_select)
All tests follow proper test structure with clear setup and assertions.
apps/api/plane/tests/contract/app/test_issue_property_api.py (1)
15-381: LGTM! Excellent contract test coverage.The test suite comprehensively validates the custom fields API:
IssueProperty API:
- Empty state, creation (text/select), listing, retrieval, updates
- Key immutability enforcement (400 on key update attempts)
- Deletion
IssuePropertyValue API:
- Setting and retrieving property values for issues
Bulk Custom Fields API:
- Bulk setting and flat-format retrieval
All tests properly assert status codes and response payloads, ensuring the API contract is validated end-to-end.
apps/api/plane/tests/unit/serializers/test_issue_property.py (1)
17-328: LGTM! Thorough serializer validation tests.The test suite provides excellent coverage of serializer behavior:
IssuePropertySerializer:
- Valid property types (text, select, boolean)
- Options validation for select types
- Default value type enforcement
- Duplicate name prevention
IssuePropertyValueSerializer:
- Type-specific value validation (text, number, select)
- Option membership validation for select types
- Required field enforcement
All tests properly validate both success paths and error cases with appropriate assertions.
apps/api/plane/db/migrations/0113_custom_fields_issue_property.py (1)
1-281: LGTM! Well-structured migration with proper constraints.The migration correctly implements the custom fields schema:
- IssueProperty model: Defines custom field schemas with appropriate field types, choices, and defaults
- IssuePropertyValue model: Stores per-issue values with flexible JSONField storage
- Soft-delete support: Conditional unique constraints respect
deleted_at IS NULL- Data integrity: Proper foreign key relationships with CASCADE/SET_NULL behaviors
- Performance: Indexes on
(issue, property)and(property)optimize lookupsThe unique constraints ensure:
- No duplicate property keys or names per project
- No duplicate property values per issue-property pair
apps/api/plane/api/urls/project.py (1)
8-66: LGTM! URL configuration is well-structured.The new URL patterns follow RESTful conventions and are consistent with existing patterns in the codebase. The endpoint hierarchy (project → properties, issue → property-values) is logical and the HTTP method mappings are appropriate.
apps/api/plane/db/models/issue.py (2)
849-856: LGTM!The field type choices cover common custom field use cases and follow Django's TextChoices pattern correctly.
978-978: LGTM!The
__str__method provides useful debugging information. The ForeignKey relations are protected by CASCADE, so null access isn't a concern.apps/api/plane/app/serializers/issue.py (5)
41-42: LGTM!Imports correctly added for the new models.
271-282: LGTM!Reading
custom_fieldsfrominitial_datais appropriate since it's not a model field. The delegation to_handle_custom_fieldskeeps the create method clean.
1020-1037: LGTM!The implementation correctly fetches and serializes custom fields with proper filtering and
select_relatedoptimization. SinceIssueDetailSerializeris used for single issue retrieval, the per-instance query is acceptable.
1182-1232: Consider restrictingproperty_typechanges after creation.Changing
property_typeafter creation could leave existingIssuePropertyValuerecords with values that don't match the new type (e.g., changing from "select" to "number" would leave string values that are now invalid). This could cause validation failures or data inconsistencies.You may want to prevent
property_typemodification similar to howkeyis protected in the view layer, or add migration logic to handle existing values.
1235-1248: LGTM!Lightweight serializer appropriately includes only essential fields for nested responses.
apps/api/plane/app/views/project/property.py (2)
25-120: LGTM! Well-structured ViewSet with proper permission controls.The implementation correctly:
- Filters queryset by workspace, project, and membership
- Protects key immutability on updates
- Handles IntegrityError for duplicate names/keys
- Uses appropriate role-based permissions (ADMIN for mutations)
339-349: Same soft-delete concern for issue existence check.Apply the same fix as mentioned earlier to add
deleted_at__isnull=Trueto the issue existence check.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (6)
apps/api/plane/app/serializers/issue.py (2)
1316-1323: Boolean type validation may incorrectly reject valid booleans due to Python's type hierarchy.In Python,
isinstance(True, int)returnsTruebecauseboolis a subclass ofint. The current validation order checksnumberbeforeboolean, so if a boolean value is passed to a number field, it would pass validation. Consider reordering checks or using explicit type comparison.🔎 Proposed fix to check boolean before number
if property_type == "text" and not isinstance(value, str): raise serializers.ValidationError({ "value": "Value must be a string for text type" }) - if property_type == "number" and not isinstance(value, (int, float)): + if property_type == "boolean" and not isinstance(value, bool): raise serializers.ValidationError({ - "value": "Value must be a number for number type" + "value": "Value must be a boolean for boolean type" }) - if property_type == "boolean" and not isinstance(value, bool): + if property_type == "number" and (not isinstance(value, (int, float)) or isinstance(value, bool)): raise serializers.ValidationError({ - "value": "Value must be a boolean for boolean type" + "value": "Value must be a number for number type" })
1324-1329: Date format validation is weak - only checks for string type.The current validation only verifies that the date value is a string but doesn't validate the actual date format (YYYY-MM-DD). Invalid date strings like
"not-a-date"would pass validation and be stored.🔎 Proposed fix to validate date format
if property_type == "date" and value: # Date should be stored as ISO string if not isinstance(value, str): raise serializers.ValidationError({ "value": "Value must be a date string (YYYY-MM-DD) for date type" }) + # Validate date format + import re + if not re.match(r'^\d{4}-\d{2}-\d{2}$', value): + raise serializers.ValidationError({ + "value": "Value must be a valid date string in YYYY-MM-DD format" + }) + # Optionally validate it's a real date + from datetime import datetime + try: + datetime.strptime(value, '%Y-%m-%d') + except ValueError: + raise serializers.ValidationError({ + "value": "Value must be a valid date" + })apps/api/plane/app/views/project/property.py (2)
35-48: Missing soft-delete filter in get_queryset.The queryset doesn't filter out soft-deleted
IssuePropertyrecords. While individual methods may handle this, the base queryset should exclude deleted records to prevent accidental exposure.🔎 Proposed fix to filter soft-deleted records
def get_queryset(self): return ( self.filter_queryset( super() .get_queryset() .filter(workspace__slug=self.kwargs.get("slug")) .filter(project_id=self.kwargs.get("project_id")) + .filter(deleted_at__isnull=True) .filter(project__project_projectmember__member=self.request.user) .filter(project__project_projectmember__is_active=True) .select_related("project", "workspace", "created_by", "updated_by") .distinct() ) .order_by("sort_order", "-created_at") )
132-146: Missing soft-delete filter in IssuePropertyValueViewSet.get_queryset.The queryset doesn't filter out soft-deleted
IssuePropertyValuerecords or values whose associatedIssuePropertyhas been soft-deleted. This could expose deleted data in list responses.🔎 Proposed fix to filter soft-deleted records
def get_queryset(self): return ( self.filter_queryset( super() .get_queryset() .filter(workspace__slug=self.kwargs.get("slug")) .filter(project_id=self.kwargs.get("project_id")) .filter(issue_id=self.kwargs.get("issue_id")) + .filter(deleted_at__isnull=True) + .filter(property__deleted_at__isnull=True) .filter(project__project_projectmember__member=self.request.user) .filter(project__project_projectmember__is_active=True) .select_related("property", "issue", "project", "workspace") .distinct() ) .order_by("property__sort_order") )apps/api/plane/tests/contract/app/test_issue_custom_fields_validation.py (2)
111-139: Good positive test case verifying all custom field types are saved correctly.The test validates that all six custom field types can be saved successfully and verifies the count of saved values. Consider also verifying the actual stored values match the input.
🔎 Optional enhancement to verify stored values
# Verify custom fields were saved issue_id = response.data["id"] issue = Issue.objects.get(id=issue_id) values = IssuePropertyValue.objects.filter(issue=issue) assert values.count() == 6 + + # Verify actual values + values_dict = {v.property.key: v.value for v in values.select_related('property')} + assert values_dict["story_points"] == 5 + assert values_dict["is_urgent"] == True + assert values_dict["quarter"] == "Q1" + assert values_dict["tags"] == ["bug", "feature"]
596-606: Inline import of timezone module.The
from django.utils import timezoneimport is done inside test methods multiple times. While functional, consider moving this to the top-level imports for consistency and slight performance improvement.🔎 Move timezone import to top of file
import pytest from django.urls import reverse +from django.utils import timezone from rest_framework import status from plane.db.models import (Then remove the inline imports on lines 598, 656, 697, 737, 780.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
apps/api/plane/app/serializers/issue.pyapps/api/plane/app/views/project/property.pyapps/api/plane/tests/contract/app/test_issue_custom_fields_validation.py
🧰 Additional context used
🧠 Learnings (4)
📚 Learning: 2025-07-23T18:18:06.875Z
Learnt from: NarayanBavisetti
Repo: makeplane/plane PR: 7460
File: apps/api/plane/app/serializers/draft.py:112-122
Timestamp: 2025-07-23T18:18:06.875Z
Learning: In the Plane codebase serializers, workspace_id is not consistently passed in serializer context, so parent issue validation in DraftIssueCreateSerializer only checks project_id rather than both workspace_id and project_id. The existing project member authentication system already validates that users can only access projects they belong to, providing sufficient security without risking breaking functionality by adding workspace_id validation where the context might not be available.
Applied to files:
apps/api/plane/tests/contract/app/test_issue_custom_fields_validation.pyapps/api/plane/app/serializers/issue.pyapps/api/plane/app/views/project/property.py
📚 Learning: 2025-12-23T14:18:32.899Z
Learnt from: dheeru0198
Repo: makeplane/plane PR: 8339
File: apps/api/plane/db/models/api.py:35-35
Timestamp: 2025-12-23T14:18:32.899Z
Learning: Django REST Framework rate limit strings are flexible: only the first character of the time unit matters. Acceptable formats include: "60/s", "60/sec", "60/second" (all equivalent), "60/m", "60/min", "60/minute" (all equivalent), "60/h", "60/hr", "60/hour" (all equivalent), and "60/d", "60/day" (all equivalent). Abbreviations like "min" are valid and do not need to be changed to "minute". Apply this guidance to any Python files in the project that configure DRF throttling rules.
Applied to files:
apps/api/plane/tests/contract/app/test_issue_custom_fields_validation.pyapps/api/plane/app/serializers/issue.pyapps/api/plane/app/views/project/property.py
📚 Learning: 2025-10-29T09:17:54.815Z
Learnt from: NarayanBavisetti
Repo: makeplane/plane PR: 7905
File: apps/api/plane/app/views/search/base.py:241-276
Timestamp: 2025-10-29T09:17:54.815Z
Learning: In apps/api/plane/app/views/search/base.py, the `filter_intakes` method uses `Issue.objects` (base manager) instead of `Issue.issue_objects` (custom manager) because the custom manager filters out all intake statuses, which would prevent querying pending and snoozed intake issues.
Applied to files:
apps/api/plane/app/views/project/property.py
📚 Learning: 2025-06-16T07:23:39.497Z
Learnt from: vamsikrishnamathala
Repo: makeplane/plane PR: 7214
File: web/core/store/issue/helpers/base-issues.store.ts:117-117
Timestamp: 2025-06-16T07:23:39.497Z
Learning: In the updateIssueDates method of BaseIssuesStore (web/core/store/issue/helpers/base-issues.store.ts), the projectId parameter is intentionally made optional to support override implementations in subclasses. The base implementation requires projectId and includes an early return check, but making it optional allows derived classes to override the method with different parameter requirements.
Applied to files:
apps/api/plane/app/views/project/property.py
🧬 Code graph analysis (2)
apps/api/plane/app/serializers/issue.py (1)
apps/api/plane/db/models/issue.py (5)
IssueProperty(859-933)IssuePropertyValue(936-979)save(182-246)save(475-524)save(907-930)
apps/api/plane/app/views/project/property.py (2)
apps/api/plane/app/serializers/issue.py (5)
IssuePropertySerializer(1159-1257)IssuePropertyValueSerializer(1276-1354)IssuePropertyLiteSerializer(1260-1273)create(196-287)create(690-693)apps/api/plane/db/models/issue.py (6)
IssueProperty(859-933)IssuePropertyValue(936-979)Issue(108-250)save(182-246)save(475-524)save(907-930)
⏰ 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). (1)
- GitHub Check: Cursor Bugbot
🔇 Additional comments (14)
apps/api/plane/app/serializers/issue.py (4)
271-286: LGTM! Proper error handling with rollback on custom_fields validation failure.The implementation correctly deletes the issue if custom_fields validation fails during creation, ensuring atomic behavior. The use of
_handle_custom_fieldswith serializer-based validation addresses the previous concern about bypassing type validation.
1043-1062: Well-implemented custom_fields injection with proper soft-delete filtering.The implementation correctly filters out both soft-deleted property values (
deleted_at__isnull=True) and values for soft-deleted properties (property__deleted_at__isnull=True). The use ofselect_related("property")optimizes the query. This addresses the previous review concern about exposing data for deleted properties.
359-435: Solid implementation of validation-first approach with atomic saves.The two-pass approach (validate all → save all in transaction) ensures atomic behavior and prevents partial saves. The use of
IssuePropertyValueSerializerfor validation addresses the previous bypass concern. Good error aggregation per field.
1195-1205: Name uniqueness validation correctly handles case-insensitivity and excludes self on updates.The validation uses
name__iexactfor case-insensitive matching and properly excludes the current instance during updates. Thedeleted_at__isnull=Truefilter ensures only active properties are considered.apps/api/plane/app/views/project/property.py (5)
70-86: LGTM! Property creation with proper context and error handling.The create method correctly passes
project_idin context for name uniqueness validation and handlesIntegrityErrorfor race conditions. The serializer handles all validation.
88-110: Good protection against key modification after creation.Preventing
keychanges ensures API stability and data integrity. The early return with clear error message is appropriate.
155-224: Well-implemented upsert semantics with proper validation.The create method correctly implements upsert behavior: checking for existing values and using the appropriate serializer mode (update vs. create). The explicit existence checks for both property and issue with soft-delete filtering are correct.
293-341: Excellent atomic bulk operation implementation.The two-pass approach (validate all → save all in transaction) prevents partial saves and returns all validation errors together. This addresses the previous review concern about data inconsistency on partial failures.
343-375: Proper soft-delete filtering in bulk GET endpoint.The implementation correctly filters out both soft-deleted values (
deleted_at__isnull=True) and values for soft-deleted properties (property__deleted_at__isnull=True). This matches the behavior inIssueDetailSerializer.to_representation().apps/api/plane/tests/contract/app/test_issue_custom_fields_validation.py (5)
22-110: Well-structured test fixtures covering all property types.The fixtures comprehensively cover all six property types (number, boolean, select, multi_select, text/required, date) with realistic options for select types. Good separation of concerns between project setup, state, and properties fixtures.
421-444: Excellent atomicity test ensuring no partial saves on validation failure.This test correctly verifies that when one custom field fails validation, none of the custom fields are saved and no issue is created. This is critical for data consistency.
568-624: Comprehensive test for soft-deleted property filtering.Good test coverage verifying that values for soft-deleted properties are hidden from API responses while still existing in the database. The test correctly distinguishes between property soft-delete and value soft-delete.
757-793: Important edge case: verifying 404 for soft-deleted issues.This test ensures the API correctly returns 404 for operations on soft-deleted issues, preventing data leakage through custom field endpoints.
500-516: Verify the correct endpoint for bulk custom field updates in the POST request.The test POSTs to
/property-values/but other tests GET from/custom-fields/for the same operation. The routing shows/custom-fields/is the designated "Bulk custom fields endpoint - get/set all custom fields for an issue" (line 319-320 inapps/api/plane/app/urls/issue.py). Confirm whether the POST at line 502 should use/custom-fields/instead of/property-values/for consistency with bulk operations.
| ) | ||
| if errors: | ||
| raise serializers.ValidationError({"custom_fields": errors}) | ||
|
|
There was a problem hiding this comment.
Issue update lacks atomicity when custom_fields validation fails
The update method modifies assignees and labels before validating custom_fields. If custom_fields validation fails at line 352-353, the assignee and label deletions/creations from lines 299-339 have already been committed to the database. This causes partial updates where assignee/label changes persist despite the request returning a validation error, violating the expected atomic behavior.
| if property_type == "number" and not isinstance(value, (int, float)): | ||
| raise serializers.ValidationError({ | ||
| "value": "Value must be a number for number type" | ||
| }) |
There was a problem hiding this comment.
Boolean values incorrectly accepted for number fields
The number type validation uses isinstance(value, (int, float)) which incorrectly accepts boolean values. In Python, bool is a subclass of int, so isinstance(True, (int, float)) returns True. This allows True/False to pass validation for number-type custom fields when they should be rejected.
| property_obj = self.instance.property | ||
|
|
||
| if property_obj and value is not None: | ||
| property_type = property_obj.property_type |
There was a problem hiding this comment.
Type validation bypassed for new property values
The IssuePropertyValueSerializer.validate() method retrieves the property via attrs.get("property"), but the serializer's fields list only includes property_id, not property. When _handle_custom_fields and BulkIssuePropertyValueEndpoint pass data = {"value": value, "property": prop.id}, the property key is ignored by DRF because it's not a declared field. For new values (no instance), property_obj remains None and the entire type validation block is skipped, allowing invalid values like strings for number fields to be saved.
Additional Locations (2)
2 participants
Implement backend support for custom fields (work item properties) to allow projects to define and manage custom metadata for issues.
Description
Type of Change
Test Scenarios
test cases are already addded
References
Linked to issue: #8157
Note
Adds backend support for project-defined custom fields and per-issue values with strict validation and atomic writes.
IssuePropertyandIssuePropertyValuewith migration0113_custom_fields_issue_property.py(unique constraints, indexes)IssuePropertySerializer,IssuePropertyValueSerializer(type-validated: text/number/date/boolean/select/multi_select), plusIssuePropertyLiteSerializerIssueCreateSerializer/update()handlecustom_fieldsatomically;IssueDetailSerializer.to_representation()returnscustom_fieldsIssuePropertyViewSet(project-level definitions CRUD),IssuePropertyValueViewSet(issue values list/create/update/delete),BulkIssuePropertyValueEndpoint(bulk get/set with transaction)apps/api/plane/api/urls/project.pyandapps/api/plane/app/urls/issue.pyfor/properties/,/issues/<issue_id>/property-values/, and/issues/<issue_id>/custom-fields/Written by Cursor Bugbot for commit 0a47750. This will update automatically on new commits. Configure here.
Summary by CodeRabbit
New Features
Improvements
Tests
✏️ Tip: You can customize this high-level summary in your review settings.