[WEB-2598] fix: show toast when existing issue is added to cycles#5771
[WEB-2598] fix: show toast when existing issue is added to cycles#5771sharma01ketan wants to merge 9 commits intopreviewfrom
Conversation
WalkthroughThe changes involve updates to the Changes
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
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 (
|
sriramveeraghanta
force-pushed
the
preview
branch
from
45880b3 to
d97ca68
Compare
| if (!workspaceSlug || !projectId) return; | ||
|
|
||
| const issueIds = data.map((i) => i.id); | ||
| const updatePromise = Promise.all( |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (9)
web/helpers/string.helper.ts (1)
273-273: Improved URL validation regexThe updated regex pattern for URL validation is more comprehensive and aligns well with the PR objectives. It now correctly handles optional protocols, various domain structures, and optional paths and query parameters.
Consider the following minor improvements:
- Pre-compile the regex pattern for better performance, especially if this function is called frequently:
const URL_PATTERN = /^(https?:\/\/)?([\da-z.-]+)\.([a-z]{2,6})(\/[\w.-]*)*\/?(\?[=&\w.-]*)?$/i; export const checkURLValidity = (url: string): boolean => { if (!url) return false; return URL_PATTERN.test(url); };
- Add examples to the JSDoc comment to illustrate valid and invalid URLs:
/** * @description * This function tests whether a URL is valid or not. * It accepts URLs with or without the protocol. * @param {string} url * @returns {boolean} * @example * checkURLValidity("https://example.com") => true * checkURLValidity("example.com") => true * checkURLValidity("example") => false * checkURLValidity("https://example.com/path?query=param") => true */These changes will improve performance and provide better documentation for future developers.
apiserver/plane/app/views/project/base.py (8)
416-416: Consider expanding permission checksThe
@allow_permission([ROLE.ADMIN])decorator restricts this method to admin users only. Consider if project owners or other roles should also have partial update capabilities.You might want to implement a more granular permission system that allows certain updates based on user roles and the specific fields being updated.
Line range hint
417-428: Improve error handling for non-existent projectsThe error handling for non-existent projects is combined with workspace not found errors. Consider separating these for more precise error reporting.
Consider refactoring the error handling as follows:
try: workspace = Workspace.objects.get(slug=slug) except Workspace.DoesNotExist: return Response( {"error": "Workspace does not exist"}, status=status.HTTP_404_NOT_FOUND, ) try: project = Project.objects.get(pk=pk) except Project.DoesNotExist: return Response( {"error": "Project does not exist"}, status=status.HTTP_404_NOT_FOUND, )
Line range hint
429-443: Consider moving serializer logic to a separate methodThe serializer validation and saving logic could be moved to a separate method for better readability and reusability.
Consider creating a helper method like
_update_projectto handle serializer logic:def _update_project(self, project, data, workspace_id): serializer = ProjectSerializer( project, data=data, context={"workspace_id": workspace_id}, partial=True, ) if serializer.is_valid(): serializer.save() return serializer return None
Line range hint
444-461: Ensure atomicity for inbox and triage state creationThe creation of the inbox and triage state is not atomic. If one operation fails, it could leave the system in an inconsistent state.
Consider wrapping these operations in a transaction to ensure atomicity:
from django.db import transaction # ... with transaction.atomic(): if serializer.data["inbox_view"]: inbox = Inbox.objects.filter( project=project, is_default=True, ).first() if not inbox: Inbox.objects.create( name=f"{project.name} Inbox", project=project, is_default=True, ) State.objects.get_or_create( name="Triage", group="triage", description="Default state for managing all Inbox Issues", project_id=pk, color="#ff7700", is_triage=True, )
Line range hint
462-475: Consider optimizing database queriesThe method performs multiple database queries that could potentially be optimized.
Consider using
select_relatedorprefetch_relatedto reduce the number of database queries:project = ( self.get_queryset() .select_related('workspace') .filter(pk=serializer.data["id"]) .first() )
Line range hint
476-486: Ensure proper error handling for model_activity taskThe
model_activity.delay()call is not wrapped in a try-except block. If this task fails, it could potentially cause the entire update operation to fail silently.Consider adding error handling for the
model_activitytask:try: model_activity.delay( model_name="project", model_id=str(project.id), requested_data=request.data, current_instance=current_instance, actor_id=request.user.id, slug=slug, origin=request.META.get("HTTP_ORIGIN"), ) except Exception as e: # Log the error, but don't fail the update operation logger.error(f"Failed to log model activity: {str(e)}")
Line range hint
487-506: Improve error handling and response messagesThe error handling for IntegrityError and ValidationError could be improved to provide more specific error messages.
Consider refactoring the error handling as follows:
except IntegrityError as e: if "already exists" in str(e): return Response( {"error": "The project name is already taken"}, status=status.HTTP_409_CONFLICT, ) return Response( {"error": "An integrity error occurred"}, status=status.HTTP_400_BAD_REQUEST, ) except serializers.ValidationError as e: return Response( {"error": "Validation error", "details": e.detail}, status=status.HTTP_400_BAD_REQUEST, )
Line range hint
416-506: Overall assessment of the partial_update methodThe
partial_updatemethod in theProjectViewSetclass is well-structured and covers important aspects of project updates. However, there are several areas for improvement:
- Permission checks could be more granular.
- Error handling can be more specific and informative.
- Database queries could be optimized to reduce the number of calls.
- The creation of inbox and triage states should be atomic.
- Error handling for the
model_activitytask should be added.Addressing these points will improve the robustness and efficiency of the method.
Consider refactoring this method into smaller, more focused methods to improve readability and maintainability. Also, consider adding unit tests to cover various scenarios, especially around error handling and permission checks.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
- apiserver/plane/app/views/project/base.py (1 hunks)
- web/helpers/string.helper.ts (1 hunks)
🧰 Additional context used
…into fix/add-existing-issue-to-cycle-toast" This reverts commit 1aed308.
…/add-existing-issue-to-cycle-toast
3 participants
[WEB-2598]
This PR aims to display a toast message on the loading, success and failure state, when an existing issue is added to a cycle in the calendar view at the project level.
Previous State:
Screen.Recording.2024-10-08.at.3.34.30.PM.mov
New State:
Screen.Recording.2024-10-08.at.3.34.51.PM.mov
Summary by CodeRabbit
Summary by CodeRabbit
New Features
Bug Fixes