Conversation
|
abu-siddique-shipsy seems not to be a GitHub user. You need a GitHub account to be able to sign the CLA. If you have already a GitHub account, please add the email address used for this commit to your account. You have signed the CLA already but the status is still pending? Let us recheck it. |
WalkthroughThis pull request introduces significant changes across multiple components of the Plane application, focusing on enhancing issue management, authentication, and system configuration. The modifications span several key areas including API serializers, models, middleware, authentication flows, and Docker configurations. The changes aim to improve custom property handling for issues, refine user authentication processes, and streamline deployment configurations. Changes
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 42
🔭 Outside diff range comments (2)
apiserver/plane/db/models/__init__.py (1)
Line range hint
73-115: Remove duplicate importsSeveral modules are imported multiple times. This creates unnecessary code duplication and potential confusion.
- from .deploy_board import DeployBoard - from .importer import Importer - from .page import Page, PageLog, PageLabel - from .estimate import Estimate, EstimatePoint - from .inbox import Inbox, InboxIssue - from .analytic import AnalyticView - from .notification import ( - Notification, - UserNotificationPreference, - EmailNotificationLog, - ) - from .exporter import ExporterHistory - from .webhook import Webhook, WebhookLog - from .dashboard import Dashboard, DashboardWidget, Widget🧰 Tools
🪛 Ruff (0.8.2)
113-113:
.favorite.UserFavoriteimported but unused; consider removing, adding to__all__, or using a redundant alias(F401)
115-115:
.issue_type.IssueTypeimported but unused; consider removing, adding to__all__, or using a redundant alias(F401)
115-115:
.issue_type.IssueTypeCustomPropertyimported but unused; consider removing, adding to__all__, or using a redundant alias(F401)
117-117:
.recent_visit.UserRecentVisitimported but unused; consider removing, adding to__all__, or using a redundant alias(F401)
docker-compose-local.yml (1)
RabbitMQ dependencies still present and need to be addressed
The verification reveals that RabbitMQ dependencies are still present in the codebase:
- RabbitMQ configuration exists in
.env.examplewith default settings- Celery is configured to use RabbitMQ as broker in
apiserver/plane/settings/common.py- Multiple Celery tasks are actively being used in the application
The removal of the
plane-mqservice from docker-compose without addressing these dependencies could cause issues. The following need attention:
- Update broker configuration to use Redis instead of RabbitMQ
- Remove RabbitMQ-related environment variables
- Ensure Celery tasks are configured to work with the new broker
🔗 Analysis chain
Line range hint
1-195: Verify removal of message queue dependenciesThe AI summary indicates that the plane-mq (RabbitMQ) service was removed. Ensure all related message queue dependencies and configurations have been properly addressed.
Let me gather more information about the Celery configuration and broker settings to verify if the message queue dependencies have been properly addressed.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check for remaining RabbitMQ references echo "Checking for RabbitMQ configurations..." rg -l "RABBITMQ|CELERY" .env* || true rg -l "rabbitmq" apiserver/plane/settings/* || true # Check for Celery tasks that might depend on RabbitMQ echo "Checking for Celery tasks..." rg -l "celery" apiserver/plane/ || trueLength of output: 1782
Script:
#!/bin/bash # Check Celery configuration and broker settings echo "Checking Celery configuration in settings..." rg -A 5 "CELERY" apiserver/plane/settings/common.py echo "\nChecking broker configuration in celery.py..." cat apiserver/plane/celery.py echo "\nChecking environment variables template..." grep -A 5 "CELERY\|RABBITMQ" .env.example || trueLength of output: 3070
🧹 Nitpick comments (27)
apiserver/plane/authentication/utils/redirection_path.py (1)
9-10: Remove commented code if change is intentionalIf the removal of onboarding redirection is intentional, the commented code should be removed rather than left as comments. This improves code maintainability and prevents confusion.
- # if not profile.is_onboarded: - # return "onboarding"apiserver/Dockerfile.api (3)
57-57: Security consideration for environment files
Including an environment file inside the image may inadvertently reveal secrets. Ensure sensitive data is not stored permanently in the Docker image layers.
60-60: Validate environment variable usage
Double-check that ENV_FILE_PATH is correctly mapped to downstream processes and not overwritten at runtime.
62-71: Consider adding a default fallback
If ENV_TYPE is omitted or misspelled, the container exits. A default fallback or a more descriptive usage message could help.apiserver/plane/api/views/attachment.py (1)
9-9: Remove unused import
Ruff flags “allow_permission” as unused. Please remove it to keep the codebase clean.- from plane.app.permissions import ProjectEntityPermission, allow_permission + from plane.app.permissions import ProjectEntityPermission🧰 Tools
🪛 Ruff (0.8.2)
9-9:
plane.app.permissions.allow_permissionimported but unusedRemove unused import:
plane.app.permissions.allow_permission(F401)
apiserver/plane/api/views/issue_type.py (3)
8-19: Remove unused imports
Many django.db.models imports are unused (Case, CharField, Exists, F, Func, Max, OuterRef, Q, Value, When, Subquery).
Likewise, MultiPartParser, FormParser, and issue_activity appear unused.- from django.db.models import ( - Case, - CharField, - Exists, - F, - Func, - Max, - OuterRef, - Q, - Value, - When, - Subquery, - ) - from rest_framework.parsers import MultiPartParser, FormParser - from plane.bgtasks.issue_activities_task import issue_activity + from django.db.models import Q # Keep only what's actually needed, or remove entirely if unused.🧰 Tools
🪛 Ruff (0.8.2)
8-8:
django.db.models.Caseimported but unusedRemove unused import
(F401)
9-9:
django.db.models.CharFieldimported but unusedRemove unused import
(F401)
10-10:
django.db.models.Existsimported but unusedRemove unused import
(F401)
11-11:
django.db.models.Fimported but unusedRemove unused import
(F401)
12-12:
django.db.models.Funcimported but unusedRemove unused import
(F401)
13-13:
django.db.models.Maximported but unusedRemove unused import
(F401)
14-14:
django.db.models.OuterRefimported but unusedRemove unused import
(F401)
15-15:
django.db.models.Qimported but unusedRemove unused import
(F401)
16-16:
django.db.models.Valueimported but unusedRemove unused import
(F401)
17-17:
django.db.models.Whenimported but unusedRemove unused import
(F401)
18-18:
django.db.models.Subqueryimported but unusedRemove unused import
(F401)
140-141: Remove unused variables
“requested_data” and “current_instance” are never used.- requested_data = json.dumps(self.request.data, cls=DjangoJSONEncoder) - current_instance = json.dumps( - IssueTypeSerializer(issue_comment).data, - cls=DjangoJSONEncoder, - )🧰 Tools
🪛 Ruff (0.8.2)
140-140: Local variable
requested_datais assigned to but never usedRemove assignment to unused variable
requested_data(F841)
141-141: Local variable
current_instanceis assigned to but never usedRemove assignment to unused variable
current_instance(F841)
200-200: Remove unused variable
“workspace” is assigned but never used.- workspace = Workspace.objects.get(slug=slug)🧰 Tools
🪛 Ruff (0.8.2)
200-200: Local variable
workspaceis assigned to but never usedRemove assignment to unused variable
workspace(F841)
apiserver/plane/api/views/search.py (1)
250-258: Limited search types
Only the "issue" filter is currently enabled in the MODELS_MAPPER. If you plan to enable the other entities, ensure they are thoroughly tested for correct authorization and performance.apiserver/plane/authentication/views/app/magic.py (1)
95-102: Recommend removing debug prints in production
Using print statements can clutter logs and potentially expose internal data. Consider replacing them with structured logging, at a debug level if necessary.apiserver/plane/api/serializers/issue.py (2)
243-259: Bulk insert for custom properties
The bulk insert approach is efficient, but watch out for potential partial failures. If a single record fails validation or insertion, the entire bulk create will fail. Ensure rollback and error management is in place.
307-324: Re-creating custom properties on update
Deleting and re-inserting custom properties ensures data consistency but might cause versioning or audit log gaps. If historical tracking is required, consider a soft-delete or versioning approach.apiserver/plane/api/views/issue.py (3)
36-36: Remove unused import to avoid lint issues.According to the static analysis hints (F401), this import is unused. Removing it will help keep the codebase clean.
- IssueTypeSerializer🧰 Tools
🪛 Ruff (0.8.2)
36-36:
plane.api.serializers.IssueTypeSerializerimported but unusedRemove unused import:
plane.api.serializers.IssueTypeSerializer(F401)
54-54: Remove unused import to avoid lint issues.Similar to the previous note, this import is also flagged as unused (F401). Consider removing it.
- IssueType🧰 Tools
🪛 Ruff (0.8.2)
54-54:
plane.db.models.IssueTypeimported but unusedRemove unused import:
plane.db.models.IssueType(F401)
1111-1111: Remove commented debug statement.Stray debugger calls can clutter production code. Consider removing this commented-out line before merging.
- # import pdb;pdb.set_trace()apiserver/plane/api/urls/issue_type.py (1)
23-25: Add missing trailing commaAdd a trailing comma after the last URL pattern to maintain consistency and make future additions cleaner.
path( "workspaces/<str:slug>/issue-type/<uuid:issue_type>/custom-properties/", IssueTypeCustomPropertyAPIEndpoint.as_view(), name="issue-type-custom-property", - ) + ),apiserver/plane/api/serializers/issue_type.py (2)
1-6: Remove unused importThe
serializersimport fromrest_frameworkis unused and can be removed.-from rest_framework import serializers from plane.db.models import IssueType, IssueTypeCustomProperty from .base import BaseSerializer🧰 Tools
🪛 Ruff (0.8.2)
1-1:
rest_framework.serializersimported but unusedRemove unused import:
rest_framework.serializers(F401)
12-24: Resolve redundant field specificationsThe
created_byfield is listed in bothread_only_fieldsandexclude, which is redundant.Choose one approach:
class Meta: model = IssueType read_only_fields = [ "id", "workspace", - "created_by", "updated_by", "created_at", "updated_at", ] exclude = [ "created_by", "updated_by", ]apiserver/plane/middleware/api_log_middleware.py (1)
22-32: Remove commented-out codeThe
process_viewmethod is entirely commented out. Either implement it properly or remove it completely to maintain clean code.apiserver/plane/api/serializers/project.py (1)
Line range hint
92-107: Document the purpose of ProjectLiteSerializerThe new ProjectLiteSerializer appears to be a lightweight version of ProjectSerializer, but its intended use case and the rationale for its introduction are not documented.
Consider adding a docstring explaining:
- The purpose of this serializer
- When to use it vs. the full ProjectSerializer
- Any performance benefits it provides
apiserver/plane/app/permissions/project.py (1)
56-56: Remove unnecessary empty lines.Multiple consecutive empty lines affect code readability.
- - +Also applies to: 73-73, 123-123
apiserver/plane/settings/storage.py (1)
22-26: Address TODO and improve method documentation.The TODO comment about max_length should be addressed, and the method could benefit from better documentation.
def get_available_name(self, name, max_length=None): - # TODO: something with max_length? + """ + Returns a filename that's free on the target storage system. + + Args: + name: Base filename + max_length: Maximum length of filename (currently not enforced) + + Returns: + Available filename that won't overwrite existing files if file_overwrite=False + """ if self.file_overwrite: return name return super().get_available_name(name, max_length=max_length)apiserver/plane/api/views/member.py (1)
133-133: Remove unnecessary empty line.Extra empty line affects code readability.
-apiserver/plane/api/views/base.py (1)
166-168: Remove commented debug codeDebug code should be removed rather than commented out. This includes the pdb trace statement.
- # if project_id == "DEFAULT": - # import pdb;pdb.set_trace - # return self.workspace.workspace_project.filter(name='default').first().idapiserver/plane/authentication/provider/credentials/magic_code.py (1)
119-124: Improve Redis data handlingConsider the following improvements:
- Move expiration time to settings
- Add data compression for large objects
- Use Redis hash instead of storing JSON string
+ REDIS_EXPIRY = settings.MAGIC_CODE_EXPIRY_SECONDS + def _store_redis_data(self, key, value): - expiry = 600 - ri.set(key, json.dumps(value), ex=expiry) + ri.hmset(key, value) + ri.expire(key, self.REDIS_EXPIRY)apiserver/plane/utils/issue_filters.py (1)
590-590: Consider adding documentation for custom properties filter.The custom properties filter is a complex feature that would benefit from documentation explaining its usage and expected format.
Add docstring to explain the feature:
+ # Add to ISSUE_FILTER docstring: + # custom_properties: Filter issues by custom properties using format "key:value,key2:value2" + # Example: "priority_id:high,status_id:active" "custom_properties": filter_custom_properties,apiserver/plane/api/views/__init__.py (1)
Line range hint
1-36: Consider adding__all__to make exports explicit.The module has several imports that appear unused. If these are meant to be re-exported, make it explicit by defining
__all__.Add this at the end of the file:
__all__ = [ 'ProjectAPIEndpoint', 'ProjectArchiveUnarchiveAPIEndpoint', 'StateAPIEndpoint', 'WorkspaceIssueAPIEndpoint', 'IssueAPIEndpoint', 'LabelAPIEndpoint', 'IssueLinkAPIEndpoint', 'IssueCommentAPIEndpoint', 'IssueActivityAPIEndpoint', 'IssueAttachmentEndpoint', 'IssueTypeAPIEndpoint', 'IssueTypeCustomPropertyAPIEndpoint', 'IssueAttachmentV2Endpoint', 'CycleAPIEndpoint', 'CycleIssueAPIEndpoint', 'TransferCycleIssueAPIEndpoint', 'CycleArchiveUnarchiveAPIEndpoint', 'ModuleAPIEndpoint', 'ModuleIssueAPIEndpoint', 'ModuleArchiveUnarchiveAPIEndpoint', 'ProjectMemberAPIEndpoint', 'InboxIssueAPIEndpoint', 'GlobalSearchEndpoint', ]🧰 Tools
🪛 Ruff (0.8.2)
12-12:
.issue.IssueLinkAPIEndpointimported but unused; consider removing, adding to__all__, or using a redundant alias(F401)
13-13:
.issue.IssueCommentAPIEndpointimported but unused; consider removing, adding to__all__, or using a redundant alias(F401)
14-14:
.issue.IssueActivityAPIEndpointimported but unused; consider removing, adding to__all__, or using a redundant alias(F401)
15-15:
.issue.IssueAttachmentEndpointimported but unused; consider removing, adding to__all__, or using a redundant alias(F401)
17-17:
.issue_type.IssueTypeAPIEndpointimported but unused; consider removing, adding to__all__, or using a redundant alias(F401)
17-17:
.issue_type.IssueTypeCustomPropertyAPIEndpointimported but unused; consider removing, adding to__all__, or using a redundant alias(F401)
18-18:
.attachment.IssueAttachmentV2Endpointimported but unused; consider removing, adding to__all__, or using a redundant alias(F401)
20-20:
.cycle.CycleAPIEndpointimported but unused; consider removing, adding to__all__, or using a redundant alias(F401)
21-21:
.cycle.CycleIssueAPIEndpointimported but unused; consider removing, adding to__all__, or using a redundant alias(F401)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (52)
admin/Dockerfile.admin(1 hunks)admin/ce/components/common/upgrade-button.tsx(1 hunks)admin/core/components/admin-sidebar/help-section.tsx(0 hunks)apiserver/Dockerfile.api(1 hunks)apiserver/plane/api/middleware/api_authentication.py(3 hunks)apiserver/plane/api/serializers/__init__.py(1 hunks)apiserver/plane/api/serializers/issue.py(9 hunks)apiserver/plane/api/serializers/issue_type.py(1 hunks)apiserver/plane/api/serializers/project.py(2 hunks)apiserver/plane/api/urls/__init__.py(2 hunks)apiserver/plane/api/urls/issue.py(2 hunks)apiserver/plane/api/urls/issue_type.py(1 hunks)apiserver/plane/api/urls/project.py(2 hunks)apiserver/plane/api/urls/search.py(1 hunks)apiserver/plane/api/views/__init__.py(3 hunks)apiserver/plane/api/views/attachment.py(1 hunks)apiserver/plane/api/views/base.py(6 hunks)apiserver/plane/api/views/issue.py(6 hunks)apiserver/plane/api/views/issue_type.py(1 hunks)apiserver/plane/api/views/member.py(1 hunks)apiserver/plane/api/views/project.py(3 hunks)apiserver/plane/api/views/search.py(1 hunks)apiserver/plane/app/permissions/project.py(5 hunks)apiserver/plane/app/views/issue/base.py(2 hunks)apiserver/plane/authentication/adapter/base.py(2 hunks)apiserver/plane/authentication/provider/credentials/magic_code.py(4 hunks)apiserver/plane/authentication/utils/redirection_path.py(1 hunks)apiserver/plane/authentication/views/app/magic.py(5 hunks)apiserver/plane/db/migrations/0084_projectcustomproperty_issuecustomproperty_and_more.py(1 hunks)apiserver/plane/db/migrations/0085_remove_issuecustomproperty_project_custom_property_and_more.py(1 hunks)apiserver/plane/db/migrations/0086_issuetypecustomproperty_delete_projectcustomproperty_and_more.py(1 hunks)apiserver/plane/db/migrations/0087_issuecustomproperty_issue_type_and_more.py(1 hunks)apiserver/plane/db/migrations/0088_alter_issuecustomproperty_issue_type.py(1 hunks)apiserver/plane/db/migrations/0089_rename_issue_type_issuecustomproperty_issue_type_custom_proerty.py(1 hunks)apiserver/plane/db/migrations/0090_rename_issue_type_custom_proerty_issuecustomproperty_issue_type_custom_property.py(1 hunks)apiserver/plane/db/models/__init__.py(3 hunks)apiserver/plane/db/models/base.py(1 hunks)apiserver/plane/db/models/issue.py(1 hunks)apiserver/plane/db/models/issue_type.py(1 hunks)apiserver/plane/middleware/api_log_middleware.py(2 hunks)apiserver/plane/settings/common.py(4 hunks)apiserver/plane/settings/redis.py(1 hunks)apiserver/plane/settings/storage.py(1 hunks)apiserver/plane/utils/issue_filters.py(3 hunks)apiserver/requirements/base.txt(3 hunks)demo.Jenkinsfile(1 hunks)docker-compose-local.yml(2 hunks)web/Dockerfile.web(2 hunks)web/ce/constants/project/settings/features.tsx(1 hunks)web/core/components/workspace/sidebar/dropdown.tsx(0 hunks)web/core/components/workspace/sidebar/help-section.tsx(1 hunks)web/core/constants/dashboard.ts(1 hunks)
💤 Files with no reviewable changes (2)
- admin/core/components/admin-sidebar/help-section.tsx
- web/core/components/workspace/sidebar/dropdown.tsx
✅ Files skipped from review due to trivial changes (4)
- apiserver/plane/settings/redis.py
- admin/ce/components/common/upgrade-button.tsx
- apiserver/plane/db/migrations/0089_rename_issue_type_issuecustomproperty_issue_type_custom_proerty.py
- apiserver/plane/db/migrations/0090_rename_issue_type_custom_proerty_issuecustomproperty_issue_type_custom_property.py
🧰 Additional context used
🪛 Ruff (0.8.2)
apiserver/plane/api/serializers/__init__.py
14-14: .issue_type.IssueTypeSerializer imported but unused; consider removing, adding to __all__, or using a redundant alias
(F401)
14-14: .issue_type.IssueTypeCustomPropertySerializer imported but unused; consider removing, adding to __all__, or using a redundant alias
(F401)
apiserver/plane/api/views/project.py
253-253: Undefined name user
(F821)
apiserver/plane/api/serializers/project.py
12-12: .issue_type.IssueTypeSerializer imported but unused
Remove unused import: .issue_type.IssueTypeSerializer
(F401)
apiserver/plane/db/models/__init__.py
44-44: .issue.IssueCustomProperty imported but unused; consider removing, adding to __all__, or using a redundant alias
(F401)
71-71: .project.ProjectPublicMember imported but unused; consider removing, adding to __all__, or using a redundant alias
(F401)
115-115: .issue_type.IssueType imported but unused; consider removing, adding to __all__, or using a redundant alias
(F401)
115-115: .issue_type.IssueTypeCustomProperty imported but unused; consider removing, adding to __all__, or using a redundant alias
(F401)
apiserver/plane/api/urls/issue.py
11-11: plane.api.views.IssueTypeAPIEndpoint imported but unused
Remove unused import: plane.api.views.IssueTypeAPIEndpoint
(F401)
apiserver/plane/api/serializers/issue_type.py
1-1: rest_framework.serializers imported but unused
Remove unused import: rest_framework.serializers
(F401)
apiserver/plane/api/views/issue_type.py
8-8: django.db.models.Case imported but unused
Remove unused import
(F401)
9-9: django.db.models.CharField imported but unused
Remove unused import
(F401)
10-10: django.db.models.Exists imported but unused
Remove unused import
(F401)
11-11: django.db.models.F imported but unused
Remove unused import
(F401)
12-12: django.db.models.Func imported but unused
Remove unused import
(F401)
13-13: django.db.models.Max imported but unused
Remove unused import
(F401)
14-14: django.db.models.OuterRef imported but unused
Remove unused import
(F401)
15-15: django.db.models.Q imported but unused
Remove unused import
(F401)
16-16: django.db.models.Value imported but unused
Remove unused import
(F401)
17-17: django.db.models.When imported but unused
Remove unused import
(F401)
18-18: django.db.models.Subquery imported but unused
Remove unused import
(F401)
25-25: rest_framework.parsers.MultiPartParser imported but unused
Remove unused import
(F401)
25-25: rest_framework.parsers.FormParser imported but unused
Remove unused import
(F401)
35-35: plane.bgtasks.issue_activities_task.issue_activity imported but unused
Remove unused import: plane.bgtasks.issue_activities_task.issue_activity
(F401)
140-140: Local variable requested_data is assigned to but never used
Remove assignment to unused variable requested_data
(F841)
141-141: Local variable current_instance is assigned to but never used
Remove assignment to unused variable current_instance
(F841)
142-142: Undefined name issue_comment
(F821)
162-162: Undefined name issue_comment
(F821)
200-200: Local variable workspace is assigned to but never used
Remove assignment to unused variable workspace
(F841)
236-236: Undefined name ValidationError
(F821)
apiserver/plane/api/serializers/issue.py
38-38: Local variable uuid_obj is assigned to but never used
Remove assignment to unused variable uuid_obj
(F841)
apiserver/plane/api/views/base.py
89-89: Multiple statements on one line (semicolon)
(E702)
apiserver/plane/api/views/attachment.py
9-9: plane.app.permissions.allow_permission imported but unused
Remove unused import: plane.app.permissions.allow_permission
(F401)
apiserver/plane/api/views/issue.py
36-36: plane.api.serializers.IssueTypeSerializer imported but unused
Remove unused import: plane.api.serializers.IssueTypeSerializer
(F401)
54-54: plane.db.models.IssueType imported but unused
Remove unused import: plane.db.models.IssueType
(F401)
apiserver/plane/authentication/views/app/magic.py
287-287: Local variable is_app is assigned to but never used
Remove assignment to unused variable is_app
(F841)
apiserver/plane/api/views/__init__.py
2-2: .project.ProjectAPIEndpoint imported but unused; consider removing, adding to __all__, or using a redundant alias
(F401)
3-3: .project.ProjectArchiveUnarchiveAPIEndpoint imported but unused; consider removing, adding to __all__, or using a redundant alias
(F401)
15-15: .issue.IssueAttachmentEndpoint imported but unused; consider removing, adding to __all__, or using a redundant alias
(F401)
17-17: .issue_type.IssueTypeAPIEndpoint imported but unused; consider removing, adding to __all__, or using a redundant alias
(F401)
17-17: .issue_type.IssueTypeCustomPropertyAPIEndpoint imported but unused; consider removing, adding to __all__, or using a redundant alias
(F401)
18-18: .attachment.IssueAttachmentV2Endpoint imported but unused; consider removing, adding to __all__, or using a redundant alias
(F401)
36-36: .search.GlobalSearchEndpoint imported but unused; consider removing, adding to __all__, or using a redundant alias
(F401)
🔇 Additional comments (45)
apiserver/plane/authentication/utils/redirection_path.py (1)
9-10: Verify: Is bypassing onboarding redirection intentional?
The removal of onboarding redirection could have significant implications:
- Users might access workspaces without completing necessary setup
- Other parts of the system might expect users to be onboarded
- Could affect user experience and data integrity
Let's verify the impact:
apiserver/Dockerfile.api (2)
53-55: Clarify handling when no ENV_FILE_PATH is given
If ENV_FILE_PATH is not provided at build time, COPY will fail. Consider a default or conditional copy logic to avoid unexpected build failures.
74-74: No changes detected
apiserver/plane/db/migrations/0084_projectcustomproperty_issuecustomproperty_and_more.py (1)
1-69: Migration logic appears correct
Both new models and constraints look good. Indexing “name” may improve query performance, but that can be addressed later.
demo.Jenkinsfile (3)
95-126: Parallel build stages are well-structured
The parallelized Docker image build logic is clear and straightforward, allowing multiple builds to run concurrently without blocking one another. This is functional and aligns with best practices for CI pipelines.
129-156: Effective parallel push to registry
Similarly, pushing images to the registry in parallel is a time-saving measure. The approach is valid and merges well with the rest of the pipeline.
158-228: Deployment strategy is aligned with ECS best practices
The parallel deployment steps for each ECS service improve speed and maintain clarity in the pipeline.
apiserver/plane/api/views/search.py (2)
68-78: Good approach for partial integer matching
The logic correctly isolates whole numbers in the search term for matching sequence IDs, ensuring fractional or partial matches won't break the query.
99-123: Consistent filtering usage
The repeated pattern (filter on slug, membership checks, is_active checks) is consistent across multiple filter methods. This improves maintainability and clarity.
apiserver/plane/api/serializers/issue.py (1)
78-79: New field 'custom_properties'
Introducing custom properties in issue serialization is a solid approach to handle dynamic fields. Validate that the ingestion of these properties meets performance requirements for large data sets.
apiserver/plane/api/views/issue.py (7)
56-56: LGTM on filter modularization.
The import for “issue_filters” keeps filtering logic modular. Looks good.
137-139: Good approach for applying base filters.
The logic to pop the "base" filter and apply it conditionally is clear and flexible. No issues found.
146-147: All good with project and workspace filters.
These filter clauses appropriately constrain the query. Looks fine.
148-149: Conditional base_filter usage looks correct.
No concerns; straightforward application of the filter.
151-152: Applying filters from query params is fine.
The usage of dictionary unpacking (filter(**filters)) is idiomatic and easy to maintain.
159-160: User-supplied ordering can pose a safety concern in some contexts.
Django generally sanitizes order_by fields, but consider validating user-supplied order_by strings to ensure no malicious injection or unknown fields are used.
352-355: Potential security check for user assignment.
Allowing request data to override the creator field could be abused if not properly validated. Confirm that only privileged contexts can assign “created_by” to avoid impersonation or data tampering.
apiserver/plane/api/urls/search.py (1)
1-13: New search route looks good overall.
The URL pattern and view import are straightforward, enabling a workspace-level search endpoint. No issues found.
apiserver/plane/api/urls/project.py (3)
5-5: No issues with the import.
Importing “ProjectArchiveUnarchiveAPIEndpoint” remains valid. No additional improvements required.
15-15: Switching from UUID to string key for project.
Changing uuid:pk to str:pk can increase flexibility. Ensure your business logic handles non-UUID strings properly (such as slug-based IDs).
[approve]
20-20: Consistency in using string for project_id.
Similarly, switching uuid:project_id to str:project_id can be beneficial if not all IDs are UUID. Confirm any existing references or DB relations referencing UUID remain consistent.
[approve]
apiserver/plane/db/migrations/0088_alter_issuecustomproperty_issue_type.py (1)
1-19: Migration logic appears correct.
Altering the foreign key to allow null and setting on_delete=SET_NULL is a common pattern. Ensure you handle any data consistency checks if needed.
apiserver/plane/db/migrations/0085_remove_issuecustomproperty_project_custom_property_and_more.py (1)
8-10: Verify migration dependencies
Please ensure that migration 0084 has been properly tested and deployed before this migration.
✅ Verification successful
Let me verify if there are any migrations between 0084 and 0085 to ensure the dependency chain is correct.
Migration dependency chain is correct
The migration files listing confirms that migration 0084_projectcustomproperty_issuecustomproperty_and_more.py is the direct predecessor of 0085_remove_issuecustomproperty_project_custom_property_and_more.py. There are no intermediate migrations between them, confirming that the dependency chain is properly configured.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check for any intermediate migrations between 0084 and this one
fd --type f "^0084.*\.py$" "apiserver/plane/db/migrations/"
Length of output: 148
Script:
#!/bin/bash
# List all migration files to check the sequence
fd --type f "^\d{4}.*\.py$" "apiserver/plane/db/migrations/" | sort
Length of output: 6310
apiserver/plane/api/urls/issue_type.py (1)
3-6: Verify view implementations
Ensure that the imported view classes exist and are properly implemented.
✅ Verification successful
Let me check the actual implementation of these classes to ensure they are properly implemented.
Let me verify if these classes inherit properly from BaseAPIView and have the necessary methods implemented.
Let me try a different approach using ripgrep to get the full class implementations.
View implementations are properly defined
Both view classes are correctly implemented:
IssueTypeAPIEndpointextendsBaseAPIViewwith proper serializer, model, and permission classes definedIssueTypeCustomPropertyAPIEndpointextendsBaseAPIViewwith GET and POST methods implemented correctly
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check if the view classes exist in the codebase
rg -l "class IssueTypeAPIEndpoint|class IssueTypeCustomPropertyAPIEndpoint" "apiserver/plane/api/views/"
Length of output: 144
Script:
#!/bin/bash
# Check the implementation of the view classes
rg -A 5 "class IssueTypeAPIEndpoint|class IssueTypeCustomPropertyAPIEndpoint" "apiserver/plane/api/views/issue_type.py"
Length of output: 601
Script:
#!/bin/bash
# Get the full implementation of both classes
ast-grep --pattern 'class IssueTypeAPIEndpoint {
$$$
}'
ast-grep --pattern 'class IssueTypeCustomPropertyAPIEndpoint {
$$$
}'
Length of output: 133
Script:
#!/bin/bash
# Get the complete class implementations with more context
rg -U "class IssueTypeAPIEndpoint\(.*?\)|class IssueTypeCustomPropertyAPIEndpoint\(.*?\)" -A 20 "apiserver/plane/api/views/issue_type.py"
Length of output: 1668
apiserver/plane/api/serializers/__init__.py (1)
14-14: Consider documenting or utilizing the imported serializers
While these serializers might be needed for URL routing, they appear unused in this file. Consider either:
- Adding them to
__all__if they're meant to be exposed at the module level - Adding a comment explaining their purpose if they're required for URL routing
- Moving them to where they're actually used if they're not needed here
🧰 Tools
🪛 Ruff (0.8.2)
14-14: .issue_type.IssueTypeSerializer imported but unused; consider removing, adding to __all__, or using a redundant alias
(F401)
14-14: .issue_type.IssueTypeCustomPropertySerializer imported but unused; consider removing, adding to __all__, or using a redundant alias
(F401)
apiserver/plane/db/migrations/0087_issuecustomproperty_issue_type_and_more.py (2)
19-23: LGTM: Field modification is well-defined
The modification to allow null/blank values in the value field is appropriate and follows Django's best practices.
14-18: Consider data migration needs
The addition of the issue_type field looks good, but consider if existing records need to be updated with appropriate issue types.
✅ Verification successful
Let me gather more information about the migration history and any existing data.
Let me check the specific migration files related to IssueCustomProperty to understand the evolution of the model.
Based on the migration history, I can now provide the final response:
Data migration is not required for this change
The migration chain shows this is part of a larger refactoring from project-based to issue-type-based custom properties. The issue_type field is being added as nullable (blank=True, null=True) with SET_NULL on delete, which means:
- Existing records can safely continue without an issue type
- The field is optional, so no data migration is needed
- The change is backward compatible with existing records
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check if there are existing records that might need migration
rg "class IssueCustomProperty|class IssueType" -A 10 -t py
Length of output: 6703
Script:
#!/bin/bash
# Check migration history and related migrations
rg "IssueCustomProperty" apiserver/plane/db/migrations/*.py
Length of output: 6438
Script:
#!/bin/bash
# Check the content of relevant migrations
cat apiserver/plane/db/migrations/0084_projectcustomproperty_issuecustomproperty_and_more.py apiserver/plane/db/migrations/0085_remove_issuecustomproperty_project_custom_property_and_more.py apiserver/plane/db/migrations/0086_issuetypecustomproperty_delete_projectcustomproperty_and_more.py apiserver/plane/db/migrations/0087_issuecustomproperty_issue_type_and_more.py
Length of output: 8684
apiserver/requirements/base.txt (1)
13-13: LGTM: Dependency updates look good
The addition of python-dotenv and upgrade of django-storages are appropriate improvements:
- python-dotenv (1.0.0) will enhance environment configuration management
- django-storages upgrade (1.14.4) includes latest improvements
Also applies to: 33-33
apiserver/plane/api/urls/issue.py (1)
71-79: Verify impact of switching to IssueAttachmentV2Endpoint
The change from IssueAttachmentEndpoint to IssueAttachmentV2Endpoint suggests a breaking change in the API. Please ensure:
- API consumers are notified of this change
- Proper API versioning strategy is in place
- Migration path is documented for existing clients
apiserver/plane/db/migrations/0086_issuetypecustomproperty_delete_projectcustomproperty_and_more.py (2)
40-47: LGTM: Robust constraint implementation
The implementation of both a conditional unique constraint and a unique_together constraint provides robust data integrity:
- Ensures unique names per issue type when not deleted
- Allows for soft deletion without constraint violations
37-39: Verify data migration plan for ProjectCustomProperty deletion
The deletion of ProjectCustomProperty model requires careful consideration:
- Is there existing data that needs to be migrated to the new
IssueTypeCustomPropertymodel? - Has a data migration strategy been documented?
- Are there any foreign key dependencies that need to be handled?
✅ Verification successful
Let me gather more information about the migration history and any potential data dependencies.
Migration path for ProjectCustomProperty deletion is properly handled
The migration sequence shows a clear and safe transition:
- Migration 0084 created both ProjectCustomProperty and IssueCustomProperty models
- Migration 0085 removed the foreign key from IssueCustomProperty to ProjectCustomProperty
- Migration 0086 safely deletes ProjectCustomProperty after dependencies were removed
The code shows a planned transition to IssueTypeCustomProperty, with proper handling of dependencies and data migration through intermediate steps. No remaining foreign key references exist to ProjectCustomProperty, and the model can be safely deleted.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check for any remaining references to ProjectCustomProperty
rg "ProjectCustomProperty" --type py
# Look for potential foreign key references
ast-grep --pattern 'models.ForeignKey($_, to="db.ProjectCustomProperty")'
Length of output: 385
Script:
#!/bin/bash
# Check the migration that created ProjectCustomProperty
cat apiserver/plane/db/migrations/0084_projectcustomproperty_issuecustomproperty_and_more.py
# Check for any custom property related models and their relationships
ast-grep --pattern 'class $_ (models.Model):
$$$
custom_property $$$
$$$'
# Look for any usage of custom properties in views or APIs
rg "custom_property" --type py -A 3
Length of output: 11979
admin/Dockerfile.admin (1)
86-88: LGTM! Docker configuration changes look good.
The addition of the EXPOSE directive and CMD instruction properly configures the container to:
- Expose port 3000 for the admin server
- Start the admin server using the correct entry point
apiserver/plane/db/models/issue_type.py (1)
62-83: Model structure looks good!
The IssueTypeCustomProperty model is well-designed with:
- Appropriate fields and relationships
- Proper unique constraints
- Good meta options for ordering and verbose names
apiserver/plane/middleware/api_log_middleware.py (1)
33-44: Verify path manipulation security
The path rewriting functionality could potentially be exploited if not properly secured.
✅ Verification successful
Path manipulation code is currently inactive and safe
The code in question is completely commented out and the function simply returns the original request path without any modifications. The commented-out code shows it was intended for a specific project ID replacement pattern (DEFAULT -> specific UUID), but since it's not active, there are no security concerns.
Additionally, the middleware is properly integrated with Django's URL resolution system:
- Path changes are properly synchronized with Django's path handling (
request.path,request.path_info) - URL resolution is handled through Django's
resolve()function - The middleware follows standard Django patterns for path handling
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for path manipulation validation in the codebase
echo "Searching for path validation patterns..."
rg -l 'path\.(join|split|normalize)'
# Search for similar middleware security patterns
echo "Searching for security patterns in middleware..."
rg -l 'sanitize|validate|clean.*path' --type py ./apiserver/plane/middleware/
Length of output: 622
Script:
#!/bin/bash
# Let's examine the middleware implementation more closely
echo "Checking the middleware implementation:"
rg -A 10 -B 10 'project_rewiter' ./apiserver/plane/middleware/api_log_middleware.py
# Check for URL/path processing in the middleware
echo "Checking URL/path processing:"
rg -A 5 'request\.path' ./apiserver/plane/middleware/api_log_middleware.py
# Look for any authentication or permission checks
echo "Checking for authentication/permission checks:"
rg -A 5 'authenticate|permission|auth' ./apiserver/plane/middleware/api_log_middleware.py
Length of output: 3827
apiserver/plane/api/serializers/project.py (1)
Line range hint 28-36: Security concern: Verify the removal of created_by from read_only_fields
The removal of "created_by" from read_only_fields means this field can now be modified through the API. This could potentially allow users to modify who created a project, which might have security implications.
🧰 Tools
🪛 Ruff (0.8.2)
12-12: .issue_type.IssueTypeSerializer imported but unused
Remove unused import: .issue_type.IssueTypeSerializer
(F401)
web/Dockerfile.web (2)
104-107: LGTM: Container configuration looks good
The addition of EXPOSE 3000 and the CMD to run the server are appropriate changes. The port exposure documents the service's network interface, and the command correctly specifies the server script location.
73-74: Verify Next.js export directory structure
The COPY commands have been modified to handle the export directory separately from the .next directory. Ensure this matches the Next.js build output structure.
✅ Verification successful
These scripts will help us understand:
- Next.js configuration for export/build
- Server file location and structure
- Build and export scripts configuration
Based on the analysis of the provided context:
- The Next.js configuration shows
output: "standalone"which is the correct configuration for Docker deployments - The package.json scripts show both
buildandexportcommands for the web package - The build process includes Next.js export functionality
The COPY commands in the Dockerfile are correct and match the Next.js build output structure
The .next directory contains the standalone server build, while the export directory contains the static files. This separation is intentional and follows Next.js best practices for containerized deployments.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check if web/server.js exists and contains the expected startup code
test -f web/server.js && head -n 10 web/server.js
Length of output: 23
Script:
#!/bin/bash
# Check Next.js build configuration and server setup
fd -e js -e ts 'next.config' --exec head -n 20 {}
Length of output: 1442
Script:
#!/bin/bash
# Look for server-related files in the web directory
fd -t f . web/ -x echo "File: {}"
Length of output: 66300
Script:
#!/bin/bash
# Check package.json scripts to understand build and export process
fd package.json --exec jq '.scripts' {}
Length of output: 2232
docker-compose-local.yml (1)
113-114: LGTM: Development convenience features added
The addition of stdin_open and tty for the API service is helpful for debugging and development purposes.
apiserver/plane/app/permissions/project.py (1)
18-19: LGTM: Consistent superuser permission checks added.
The addition of superuser permission checks across all permission classes improves consistency in access control.
Also applies to: 53-54, 88-89, 124-125
apiserver/plane/settings/storage.py (1)
16-17: LGTM: Safe defaults for file storage.
Setting file_overwrite=False by default is a good safety measure to prevent accidental file overwrites.
apiserver/plane/authentication/adapter/base.py (1)
152-154:
Add username validation and conflict handling
The current implementation lacks validation for usernames and doesn't handle potential conflicts. Consider:
- Validating username format (allowed characters, length)
- Handling username collisions
- Sanitizing username input
apiserver/plane/app/views/issue/base.py (1)
Line range hint 714-793: LGTM! Well-structured date validation.
The date validation implementation is clean and properly handles edge cases. Good job on:
- Clear validation method
- Proper error handling
- Efficient bulk update implementation
web/ce/constants/project/settings/features.tsx (1)
87-88: Verify the implications of making time tracking available to all users
The change makes time tracking available to all users by setting isPro: false. Please ensure this aligns with:
- Business requirements and pricing strategy
- Backend service capacity planning
- User documentation updates
✅ Verification successful
Let me generate additional scripts to better understand the time tracking feature configuration and its implications.
Time tracking feature change appears to be part of broader feature accessibility update
The change to make time tracking available to all users by setting isPro: false aligns with the codebase structure and existing patterns:
- Multiple other features (modules, views, pages, inbox) are also configured with
isPro: falseandisEnabled: true - The backend model has time tracking as an optional feature (
default=False) that can be enabled per project - This appears to be part of a consistent feature management approach where features are controlled at the project level rather than by pro/free tier
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for any time tracking related configurations or dependencies
# Check for any other time tracking related configurations
rg -l "time_tracking"
# Check for any premium/pro feature checks in the codebase
rg "isPro.*true"
Length of output: 65793
Script:
#!/bin/bash
# Check for time tracking feature configuration and related code
# Look for time tracking related code and configurations
rg -A 5 "time_tracking"
# Check for feature flag configurations
rg -A 10 "isPro.*false" web/ce/constants/project/settings/features.tsx
# Check for any backend time tracking related configurations
rg -A 5 "time_tracking" apiserver/plane/db/models/project.py
Length of output: 4460
web/core/components/workspace/sidebar/help-section.tsx (1)
Line range hint 55-74: Consider documenting the simplified help section design
The help section has been significantly simplified. While the code is clean, consider:
- Documenting the rationale for removing help menu items
- Ensuring users can still access help resources through alternative means
web/core/constants/dashboard.ts (1)
261-292:
Review the impact of removing key navigation items
The removal of Projects, Cycles, and Analytics from the sidebar menu is a significant change that could impact user workflow. Please verify:
- Alternative access paths to these features
- Impact on existing user workflows
- Navigation structure consistency
✅ Verification successful
The previous commands failed due to incorrect file type specifications. Let's try with simpler commands to gather information about the usage of these features in the codebase.
Let's try to find evidence of alternative navigation paths or if these features have been moved elsewhere in the application.
Let's try to find if these features have been moved to workspace views, which seems to be the new navigation structure based on the changes.
Based on the search results, I can now generate a final response. The evidence shows that these features have been consolidated under a new "Views" navigation structure, with workspace-level views being the primary access point.
Navigation changes are part of UI consolidation
The removal of separate menu items for Projects, Cycles, and Analytics is intentional as these features are now accessible through the new "Views" system. Evidence shows:
- A new centralized "Views" navigation item has replaced the individual menu items
- The features are accessible via
/workspace-views/routes - Both workspace-level and project-level views are supported through:
/workspace-views/for workspace-wide access/projects/{projectId}/views/for project-specific access
This appears to be a UI improvement rather than a removal of functionality, consolidating navigation under a more flexible views system.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for alternative navigation paths and usage of removed features
# Check for other references to these features
rg -A 5 "Projects|Cycles|Analytics" --type tsx --type ts
# Look for route definitions
fd "routes|navigation" --type ts --type tsx --exec rg -l "projects|cycles|analytics" {}
Length of output: 348
Script:
#!/bin/bash
# Let's try a different approach to search for these features
# Search for references to these features in the codebase
rg -i "projects|cycles|analytics"
# Search for route definitions using ast-grep
ast-grep --pattern 'const $_ = {
$$$
projects: $_,
$$$
}'
# Also search for any remaining UI components related to these features
rg -i "ProjectsPage|CyclesPage|AnalyticsPage"
Length of output: 71580
Script:
#!/bin/bash
# Let's search for routes and pages related to these features
# Search for route definitions and page components
rg -i "path.*projects|path.*cycles|path.*analytics"
# Search for any imports or references to these pages
rg -i "import.*Projects|import.*Cycles|import.*Analytics"
# Search for any remaining UI components or hooks related to these features
rg -i "use.*Projects|use.*Cycles|use.*Analytics"
Length of output: 65700
Script:
#!/bin/bash
# Let's search for workspace views which might be the new location for these features
# Search for workspace views related files and components
rg -i "workspace.*views|workspace-views"
# Search for any references to these features in the workspace views
rg -i "workspace.*views.*projects|workspace.*views.*cycles|workspace.*views.*analytics"
# Look for any configuration or routing related to workspace views
rg -i "workspace.*views.*config|workspace.*views.*route"
Length of output: 14422
| ) | ||
|
|
||
| # Get the workspace | ||
| workspace = Workspace.objects.get(slug=slug) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Handle potential workspace lookup failure
Workspace.objects.get(...) can raise DoesNotExist. Consider handling errors or returning a 404 if the workspace doesn’t exist.
- workspace = Workspace.objects.get(slug=slug)
+ try:
+ workspace = Workspace.objects.get(slug=slug)
+ except Workspace.DoesNotExist:
+ return Response({"error": "Workspace not found"}, status=status.HTTP_404_NOT_FOUND)📝 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.
| workspace = Workspace.objects.get(slug=slug) | |
| try: | |
| workspace = Workspace.objects.get(slug=slug) | |
| except Workspace.DoesNotExist: | |
| return Response({"error": "Workspace not found"}, status=status.HTTP_404_NOT_FOUND) |
| {"name": "The Property Name is already taken"}, | ||
| status=status.HTTP_410_GONE, | ||
| ) | ||
| except ValidationError: |
There was a problem hiding this comment.
Add import for ValidationError
ValidationError is not defined. Import it from django.core.exceptions or rest_framework.exceptions if needed.
from django.core.exceptions import ValidationError🧰 Tools
🪛 Ruff (0.8.2)
236-236: Undefined name ValidationError
(F821)
| IssueTypeSerializer(issue_comment).data, | ||
| cls=DjangoJSONEncoder, | ||
| ) | ||
|
|
||
| # Validation check if the issue already exists | ||
| if ( | ||
| IssueType.objects.filter( | ||
| workspace__slug=slug, | ||
| name=request.data.get('name') | ||
| ).exists() | ||
| ): | ||
| return Response( | ||
| { | ||
| "error": "Issue Comment with the same name already exists", | ||
| "id": str(issue_type.id), | ||
| }, | ||
| status=status.HTTP_409_CONFLICT, | ||
| ) | ||
|
|
||
| serializer = IssueTypeSerializer( | ||
| issue_comment, data=request.data, partial=True |
There was a problem hiding this comment.
Fix undefined name 'issue_comment'
Use “issue_type” instead of “issue_comment.”
- current_instance = json.dumps(
- IssueTypeSerializer(issue_comment).data,
- cls=DjangoJSONEncoder,
- )
...
- serializer = IssueTypeSerializer(issue_comment, data=request.data, partial=True)
+ current_instance = json.dumps(
+ IssueTypeSerializer(issue_type).data,
+ cls=DjangoJSONEncoder,
+ )
...
+ serializer = IssueTypeSerializer(issue_type, data=request.data, partial=True)📝 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.
| IssueTypeSerializer(issue_comment).data, | |
| cls=DjangoJSONEncoder, | |
| ) | |
| # Validation check if the issue already exists | |
| if ( | |
| IssueType.objects.filter( | |
| workspace__slug=slug, | |
| name=request.data.get('name') | |
| ).exists() | |
| ): | |
| return Response( | |
| { | |
| "error": "Issue Comment with the same name already exists", | |
| "id": str(issue_type.id), | |
| }, | |
| status=status.HTTP_409_CONFLICT, | |
| ) | |
| serializer = IssueTypeSerializer( | |
| issue_comment, data=request.data, partial=True | |
| IssueTypeSerializer(issue_type).data, | |
| cls=DjangoJSONEncoder, | |
| ) | |
| # Validation check if the issue already exists | |
| if ( | |
| IssueType.objects.filter( | |
| workspace__slug=slug, | |
| name=request.data.get('name') | |
| ).exists() | |
| ): | |
| return Response( | |
| { | |
| "error": "Issue Comment with the same name already exists", | |
| "id": str(issue_type.id), | |
| }, | |
| status=status.HTTP_409_CONFLICT, | |
| ) | |
| serializer = IssueTypeSerializer( | |
| issue_type, data=request.data, partial=True |
🧰 Tools
🪛 Ruff (0.8.2)
142-142: Undefined name issue_comment
(F821)
162-162: Undefined name issue_comment
(F821)
| echo "Vault CONFIG: \${CONFIG}" # Debugging, remove in production | ||
| pwd | ||
| mkdir -p apiserver/${configStoragePath} | ||
| echo "\${CONFIG}" > apiserver/${configStoragePath}/.env | ||
| """ | ||
|
|
||
| // Debugging: Show the contents of the .env file (optional for development only) | ||
| sh "cat apiserver/${configStoragePath}/.env" |
There was a problem hiding this comment.
Avoid printing secret values in logs
It appears that the value of CONFIG is being echoed for debugging. This can expose sensitive information in logs if left in production.
Below is a diff to comment out the line that prints the secret value:
- echo "Vault CONFIG: ${CONFIG}" # Debugging, remove in production
+ # echo "Vault CONFIG: ${CONFIG}" # Commented out to prevent logging secrets📝 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.
| echo "Vault CONFIG: \${CONFIG}" # Debugging, remove in production | |
| pwd | |
| mkdir -p apiserver/${configStoragePath} | |
| echo "\${CONFIG}" > apiserver/${configStoragePath}/.env | |
| """ | |
| // Debugging: Show the contents of the .env file (optional for development only) | |
| sh "cat apiserver/${configStoragePath}/.env" | |
| # echo "Vault CONFIG: ${CONFIG}" # Commented out to prevent logging secrets | |
| pwd | |
| mkdir -p apiserver/${configStoragePath} | |
| echo "\${CONFIG}" > apiserver/${configStoragePath}/.env | |
| """ | |
| // Debugging: Show the contents of the .env file (optional for development only) | |
| sh "cat apiserver/${configStoragePath}/.env" |
| q |= Q(**{"sequence_id": sequence_id}) | ||
| else: | ||
| q |= Q(**{f"{field}__icontains": query}) | ||
| q |= Q(**{'custom_properties__value__icontains': query}) |
There was a problem hiding this comment.
💡 Codebase verification
Based on the code analysis from both searches, I can now provide a final response:
Custom properties search could impact performance - indexes recommended
The custom_properties field is implemented as a related model (IssueCustomProperty) with a foreign key relationship to Issue, but there are no indexes defined on the value field that's being searched with icontains. Since this is used in a global search functionality, adding an index on the value field would improve query performance.
- Consider adding a database index on
IssueCustomProperty.valuefield inapiserver/plane/db/models/issue.py - The current implementation performs a case-insensitive contains search (
icontains) which might not efficiently use indexes. Consider if exact matches or starts-with searches would be sufficient for your use case.
🔗 Analysis chain
Potential performance considerations
Appending an additional Q(...) for custom_properties might impact performance if the search is done frequently on large datasets. Confirm if an index or other optimizations exist for the JSON fields.
Let me analyze the search results to check for any database indexes or optimizations. I'll search for database migrations or model configurations related to indexes.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Searching references to custom_properties fields to see if indexes or optimizations are implemented
rg -A 5 "custom_properties"
Length of output: 10566
Script:
#!/bin/bash
# Search for database indexes in migrations and models related to custom_properties
rg -A 5 "class Meta.*db_index|Index\(" apiserver/plane/db/models/
rg -A 5 "db\.Index|indexes" apiserver/plane/db/migrations/
Length of output: 128
| def create_project(slug, origin, user, serializer, request_data): | ||
|
|
||
| # Add the user as Administrator to the project | ||
| _ = ProjectMember.objects.create( | ||
| project_id=serializer.data["id"], | ||
| member=user, | ||
| role=20, | ||
| ) | ||
| # Also create the issue property for the user | ||
| _ = IssueUserProperty.objects.create( | ||
| project_id=serializer.data["id"], | ||
| user=user, | ||
| ) | ||
|
|
||
| if serializer.data["project_lead"] is not None and str( | ||
| serializer.data["project_lead"] | ||
| ) != str(user.id): | ||
| ProjectMember.objects.create( | ||
| project_id=serializer.data["id"], | ||
| member_id=serializer.data["project_lead"], | ||
| role=20, | ||
| ) | ||
| # Also create the issue property for the user | ||
| IssueUserProperty.objects.create( | ||
| project_id=serializer.data["id"], | ||
| user_id=serializer.data["project_lead"], | ||
| ) | ||
|
|
||
| # Default states | ||
| states = [ | ||
| { | ||
| "name": "Backlog", | ||
| "color": "#A3A3A3", | ||
| "sequence": 15000, | ||
| "group": "backlog", | ||
| "default": True, | ||
| }, | ||
| { | ||
| "name": "Todo", | ||
| "color": "#3A3A3A", | ||
| "sequence": 25000, | ||
| "group": "unstarted", | ||
| }, | ||
| { | ||
| "name": "In Progress", | ||
| "color": "#F59E0B", | ||
| "sequence": 35000, | ||
| "group": "started", | ||
| }, | ||
| { | ||
| "name": "Done", | ||
| "color": "#16A34A", | ||
| "sequence": 45000, | ||
| "group": "completed", | ||
| }, | ||
| { | ||
| "name": "Cancelled", | ||
| "color": "#EF4444", | ||
| "sequence": 55000, | ||
| "group": "cancelled", | ||
| }, | ||
| ] | ||
|
|
||
| State.objects.bulk_create( | ||
| [ | ||
| State( | ||
| name=state["name"], | ||
| color=state["color"], | ||
| project=serializer.instance, | ||
| sequence=state["sequence"], | ||
| workspace=serializer.instance.workspace, | ||
| group=state["group"], | ||
| default=state.get("default", False), | ||
| created_by=user, | ||
| ) | ||
| for state in states | ||
| ] | ||
| ) | ||
| # Model activity | ||
| model_activity.delay( | ||
| model_name="project", | ||
| model_id=str(serializer.data["id"]), | ||
| requested_data=request_data, | ||
| current_instance=None, | ||
| actor_id=user.id, | ||
| slug=slug, | ||
| origin=origin, | ||
| ) | ||
| return True |
There was a problem hiding this comment.
Add transaction management and error handling.
The create_project function performs multiple database operations without transaction management. If any operation fails, it could leave the database in an inconsistent state.
Wrap the operations in a transaction and add error handling:
+from django.db import transaction
-def create_project(slug, origin, user, serializer, request_data):
+def create_project(slug, origin, user, serializer, request_data):
+ try:
+ with transaction.atomic():
# Add the user as Administrator to the project
_ = ProjectMember.objects.create(
project_id=serializer.data["id"],
member=user,
role=20,
)
# Rest of the function...
+ except Exception as e:
+ # Log the error
+ raise ValidationError(f"Failed to create project: {str(e)}")Committable suggestion skipped: line range outside the PR's diff.
| def filter_custom_properties(params, issue_filter, method, prefix=""): | ||
| if method == "GET": | ||
| custom_properties = [ | ||
| item | ||
| for item in params.get("custom_properties").split(",") | ||
| if item != "null" | ||
| ] | ||
| query = Q() | ||
| for row in custom_properties: | ||
| key, value = row.split(":") | ||
| query &= Q( | ||
| Q(custom_properties__project_custom_property_id=key) & | ||
| Q(custom_properties__value=value) | ||
| ) | ||
|
|
||
| issue_filter['base'] = query | ||
|
|
||
| print(issue_filter) | ||
| return issue_filter |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve custom properties filter implementation.
Several issues in the filter_custom_properties function:
- Debug print statement should be removed
- No validation of the key:value format
- Query could be inefficient for large datasets
Consider this improved implementation:
def filter_custom_properties(params, issue_filter, method, prefix=""):
if method == "GET":
custom_properties = [
item
for item in params.get("custom_properties").split(",")
if item != "null"
]
query = Q()
for row in custom_properties:
+ try:
+ key, value = row.split(":", 1) # Use maxsplit=1 to handle values containing ':'
+ if not key or not value:
+ continue
query &= Q(
Q(custom_properties__project_custom_property_id=key) &
Q(custom_properties__value=value)
)
+ except ValueError:
+ continue # Skip invalid format
-
issue_filter['base'] = query
-
- print(issue_filter)
return issue_filter📝 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.
| def filter_custom_properties(params, issue_filter, method, prefix=""): | |
| if method == "GET": | |
| custom_properties = [ | |
| item | |
| for item in params.get("custom_properties").split(",") | |
| if item != "null" | |
| ] | |
| query = Q() | |
| for row in custom_properties: | |
| key, value = row.split(":") | |
| query &= Q( | |
| Q(custom_properties__project_custom_property_id=key) & | |
| Q(custom_properties__value=value) | |
| ) | |
| issue_filter['base'] = query | |
| print(issue_filter) | |
| return issue_filter | |
| def filter_custom_properties(params, issue_filter, method, prefix=""): | |
| if method == "GET": | |
| custom_properties = [ | |
| item | |
| for item in params.get("custom_properties").split(",") | |
| if item != "null" | |
| ] | |
| query = Q() | |
| for row in custom_properties: | |
| try: | |
| key, value = row.split(":", 1) # Use maxsplit=1 to handle values containing ':' | |
| if not key or not value: | |
| continue | |
| query &= Q( | |
| Q(custom_properties__project_custom_property_id=key) & | |
| Q(custom_properties__value=value) | |
| ) | |
| except ValueError: | |
| continue # Skip invalid format | |
| issue_filter['base'] = query | |
| return issue_filter |
| class IssueCustomProperty(ProjectBaseModel): | ||
| issue = models.ForeignKey( | ||
| Issue, on_delete=models.CASCADE, related_name="custom_properties" | ||
| ) | ||
| key = models.CharField(max_length=255) | ||
| value = models.CharField(max_length=255, null=True, blank=True) | ||
| issue_type_custom_property = models.ForeignKey( | ||
| "db.IssueTypeCustomProperty", | ||
| on_delete=models.SET_NULL, | ||
| related_name="issue_type_custom_property", | ||
| null=True, | ||
| blank=True, | ||
| ) | ||
|
|
||
| class Meta: | ||
| verbose_name = "Issue Custom Property" | ||
| verbose_name_plural = "Issue Custom Properties" | ||
| db_table = "issue_custom_properties" | ||
| ordering = ("-created_at",) | ||
|
|
||
| def __str__(self): | ||
| return f"{self.issue.name} {self.key}" No newline at end of file |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider adding constraints and indexes for better data integrity and performance.
- Add a unique constraint to prevent duplicate keys for the same issue
- Add an index on the foreign key fields for better query performance
- Consider adding validation for the key field format
Apply this diff to enhance the model:
class IssueCustomProperty(ProjectBaseModel):
issue = models.ForeignKey(
Issue, on_delete=models.CASCADE, related_name="custom_properties"
)
- key = models.CharField(max_length=255)
+ key = models.CharField(max_length=255, validators=[
+ RegexValidator(
+ regex=r'^[a-zA-Z][a-zA-Z0-9_]*$',
+ message='Key must start with a letter and contain only letters, numbers, and underscores'
+ )
+ ])
value = models.CharField(max_length=255, null=True, blank=True)
issue_type_custom_property = models.ForeignKey(
"db.IssueTypeCustomProperty",
on_delete=models.SET_NULL,
related_name="issue_type_custom_property",
null=True,
blank=True,
+ db_index=True
)
class Meta:
verbose_name = "Issue Custom Property"
verbose_name_plural = "Issue Custom Properties"
db_table = "issue_custom_properties"
ordering = ("-created_at",)
+ unique_together = ["issue", "key", "deleted_at"]
+ indexes = [
+ models.Index(fields=["issue", "key"]),
+ ]📝 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.
| class IssueCustomProperty(ProjectBaseModel): | |
| issue = models.ForeignKey( | |
| Issue, on_delete=models.CASCADE, related_name="custom_properties" | |
| ) | |
| key = models.CharField(max_length=255) | |
| value = models.CharField(max_length=255, null=True, blank=True) | |
| issue_type_custom_property = models.ForeignKey( | |
| "db.IssueTypeCustomProperty", | |
| on_delete=models.SET_NULL, | |
| related_name="issue_type_custom_property", | |
| null=True, | |
| blank=True, | |
| ) | |
| class Meta: | |
| verbose_name = "Issue Custom Property" | |
| verbose_name_plural = "Issue Custom Properties" | |
| db_table = "issue_custom_properties" | |
| ordering = ("-created_at",) | |
| def __str__(self): | |
| return f"{self.issue.name} {self.key}" | |
| class IssueCustomProperty(ProjectBaseModel): | |
| issue = models.ForeignKey( | |
| Issue, on_delete=models.CASCADE, related_name="custom_properties" | |
| ) | |
| key = models.CharField(max_length=255, validators=[ | |
| RegexValidator( | |
| regex=r'^[a-zA-Z][a-zA-Z0-9_]*$', | |
| message='Key must start with a letter and contain only letters, numbers, and underscores' | |
| ) | |
| ]) | |
| value = models.CharField(max_length=255, null=True, blank=True) | |
| issue_type_custom_property = models.ForeignKey( | |
| "db.IssueTypeCustomProperty", | |
| on_delete=models.SET_NULL, | |
| related_name="issue_type_custom_property", | |
| null=True, | |
| blank=True, | |
| db_index=True | |
| ) | |
| class Meta: | |
| verbose_name = "Issue Custom Property" | |
| verbose_name_plural = "Issue Custom Properties" | |
| db_table = "issue_custom_properties" | |
| ordering = ("-created_at",) | |
| unique_together = ["issue", "key", "deleted_at"] | |
| indexes = [ | |
| models.Index(fields=["issue", "key"]), | |
| ] | |
| def __str__(self): | |
| return f"{self.issue.name} {self.key}" |
|
|
||
| if serializer.is_valid(): | ||
| serializer.save() | ||
| print(serializer.data) |
There was a problem hiding this comment.
Remove debug print statements.
Debug print statements should not be in production code. Consider using proper logging if debugging information is needed.
Apply this diff to remove the print statements:
- print(serializer.data)
# Track the issue
issue_activity.delay(
type="issue.activity.created",
- print(issue)
issue = user_timezone_converter(
issue, datetime_fields, request.user.user_timezone
)Also applies to: 455-455
| highlight: (pathname: string, baseUrl: string) => pathname.includes(`${baseUrl}/analytics/`), | ||
| Icon: BarChart2, | ||
| }, | ||
| } |
There was a problem hiding this comment.
Fix missing comma in array
There's a potential syntax error with the missing comma after the Views menu item, which could cause issues if items are uncommented.
- }
+ },Committable suggestion skipped: line range outside the PR's diff.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
web/Dockerfile.web(2 hunks)
🔇 Additional comments (2)
web/Dockerfile.web (2)
105-105: LGTM: Port exposure is correctly configured
The EXPOSE instruction correctly documents that the application runs on port 3000, which is the default port for Next.js applications.
107-107: Verify the server implementation and dependencies
The CMD instruction runs a custom server implementation. Ensure that:
- The server.js file is properly copied to the final image
- Required dependencies for running the server are included in package.json
web/Dockerfile.web
Outdated
| # COPY --from=installer /app/web/.next/export ./ | ||
| # COPY --from=installer /app/web/.next ./.next |
There was a problem hiding this comment.
💡 Codebase verification
The commented COPY commands should remain uncommented for standalone output
The Next.js configuration explicitly sets output: "standalone" in web/next.config.js, which requires the .next directory to be present in the final image. The commented COPY commands are actually necessary for the application to work properly.
- The
.nextdirectory must be copied when using standalone output mode - The current Dockerfile only copies next.config.js, package.json, and public directory
- Without the
.nextdirectory, the application will fail to run
- # COPY --from=installer /app/web/.next/export ./
+ COPY --from=installer /app/web/.next ./.nextNote: Only the second COPY command is needed since we're using standalone output mode, not export mode.
🔗 Analysis chain
Critical: Build artifacts are not being copied to the final image
The COPY commands for the Next.js build output are commented out. This will prevent the application from running as the built files are missing in the final image.
Uncomment and update the COPY commands based on your Next.js output configuration:
- # COPY --from=installer /app/web/.next/export ./
- # COPY --from=installer /app/web/.next ./.next
+ COPY --from=installer /app/web/.next ./.next🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Verify if server.js exists and check the build output structure
fd -t f "server.js" web/
ast-grep --pattern 'export const config = { output: $_ }'
Length of output: 84
Script:
#!/bin/bash
# Let's check the Next.js configuration and build setup
fd -t f "next.config" .
rg -l "output.*:" --type js
rg -l "standalone|output.*standalone" --type js
Length of output: 323
Script:
#!/bin/bash
# Let's examine the server setup
fd -t f "server" web/
cat web/server.js 2>/dev/null || true
Length of output: 152
Script:
#!/bin/bash
# Check the build script in package.json
rg '"build":.+' package.json
Length of output: 62
Script:
#!/bin/bash
# Check if there are any other Dockerfile stages copying .next
rg "COPY.+\.next" web/Dockerfile.web
Length of output: 134
Script:
#!/bin/bash
# Let's check the content of next.config.js to understand the build output configuration
cat web/next.config.js
Length of output: 4098
Script:
#!/bin/bash
# Let's check if there are any other COPY commands in the Dockerfile that might be handling the build output
rg "COPY.+installer" web/Dockerfile.web
Length of output: 280
Script:
#!/bin/bash
# Let's check for any custom server configuration in the package.json scripts
rg '"(start|dev)":.+' package.json
Length of output: 113
3 participants
Description
Type of Change
Screenshots and Media (if applicable)
Test Scenarios
References
Summary by CodeRabbit
New Features
GlobalSearchEndpointfor comprehensive searches across multiple fields within a workspace.IssueTypeCustomPropertyandIssueCustomPropertymodels to enhance issue management capabilities.filter_custom_propertiesfunction for advanced filtering of issues based on custom properties.S3Storageclass with a newfile_overwriteattribute to control file handling during uploads.PROJECT_FEATURES_LISTto enable time tracking for all users.Bug Fixes
Documentation
Chores
plane-mqservice from the Docker configuration.