diff --git a/.gitignore b/.gitignore index 5c4d170..a6d2d3d 100644 --- a/.gitignore +++ b/.gitignore @@ -20,3 +20,4 @@ dump.rdb # Logs logs/ +.playwright-mcp/ diff --git a/backend/app/api/attachments/router.py b/backend/app/api/attachments/router.py index d57ced6..c67623b 100644 --- a/backend/app/api/attachments/router.py +++ b/backend/app/api/attachments/router.py @@ -8,6 +8,8 @@ from sqlalchemy.orm import Session from typing import Optional from app.core.database import get_db +from app.core.rate_limiter import limiter +from app.core.config import settings from app.middleware.auth import get_current_user, check_task_access, check_task_edit_access from app.models import User, Task, Project, Attachment, AttachmentVersion, EncryptionKey, AuditAction from app.schemas.attachment import ( @@ -156,9 +158,10 @@ def should_encrypt_file(project: Project, db: Session) -> tuple[bool, Optional[E @router.post("/tasks/{task_id}/attachments", response_model=AttachmentResponse) +@limiter.limit(settings.RATE_LIMIT_SENSITIVE) async def upload_attachment( - task_id: str, request: Request, + task_id: str, file: UploadFile = File(...), db: Session = Depends(get_db), current_user: User = Depends(get_current_user) @@ -167,6 +170,8 @@ async def upload_attachment( Upload a file attachment to a task. For confidential projects, files are automatically encrypted using AES-256-GCM. + + Rate limited: 20 requests per minute (sensitive tier). """ task = get_task_with_access_check(db, task_id, current_user, require_edit=True) diff --git a/backend/app/api/comments/router.py b/backend/app/api/comments/router.py index 8ae41a5..83b79fb 100644 --- a/backend/app/api/comments/router.py +++ b/backend/app/api/comments/router.py @@ -1,9 +1,11 @@ import uuid from typing import List -from fastapi import APIRouter, Depends, HTTPException, status +from fastapi import APIRouter, Depends, HTTPException, status, Request from sqlalchemy.orm import Session from app.core.database import get_db +from app.core.rate_limiter import limiter +from app.core.config import settings from app.models import User, Task, Comment from app.schemas.comment import ( CommentCreate, CommentUpdate, CommentResponse, CommentListResponse, @@ -49,13 +51,19 @@ def comment_to_response(comment: Comment) -> CommentResponse: @router.post("/api/tasks/{task_id}/comments", response_model=CommentResponse, status_code=status.HTTP_201_CREATED) +@limiter.limit(settings.RATE_LIMIT_STANDARD) async def create_comment( + request: Request, task_id: str, comment_data: CommentCreate, db: Session = Depends(get_db), current_user: User = Depends(get_current_user), ): - """Create a new comment on a task.""" + """ + Create a new comment on a task. + + Rate limited: 60 requests per minute (standard tier). + """ task = db.query(Task).filter(Task.id == task_id).first() if not task: diff --git a/backend/app/api/custom_fields/router.py b/backend/app/api/custom_fields/router.py index 7f27ab8..1227692 100644 --- a/backend/app/api/custom_fields/router.py +++ b/backend/app/api/custom_fields/router.py @@ -91,13 +91,17 @@ async def create_custom_field( detail="Formula is required for formula fields", ) - is_valid, error_msg = FormulaService.validate_formula( + is_valid, error_msg, cycle_path = FormulaService.validate_formula_with_details( field_data.formula, project_id, db ) if not is_valid: + detail = {"message": error_msg} + if cycle_path: + detail["cycle_path"] = cycle_path + detail["cycle_description"] = " -> ".join(cycle_path) raise HTTPException( status_code=status.HTTP_400_BAD_REQUEST, - detail=error_msg, + detail=detail, ) # Get next position @@ -229,13 +233,17 @@ async def update_custom_field( # Validate formula if updating formula field if field.field_type == "formula" and field_data.formula is not None: - is_valid, error_msg = FormulaService.validate_formula( + is_valid, error_msg, cycle_path = FormulaService.validate_formula_with_details( field_data.formula, field.project_id, db, field_id ) if not is_valid: + detail = {"message": error_msg} + if cycle_path: + detail["cycle_path"] = cycle_path + detail["cycle_description"] = " -> ".join(cycle_path) raise HTTPException( status_code=status.HTTP_400_BAD_REQUEST, - detail=error_msg, + detail=detail, ) # Validate options if updating dropdown field diff --git a/backend/app/api/projects/router.py b/backend/app/api/projects/router.py index f21a69a..6d4bc74 100644 --- a/backend/app/api/projects/router.py +++ b/backend/app/api/projects/router.py @@ -4,10 +4,17 @@ from fastapi import APIRouter, Depends, HTTPException, status, Request from sqlalchemy.orm import Session from app.core.database import get_db -from app.models import User, Space, Project, TaskStatus, AuditAction +from app.models import User, Space, Project, TaskStatus, AuditAction, ProjectMember from app.models.task_status import DEFAULT_STATUSES from app.schemas.project import ProjectCreate, ProjectUpdate, ProjectResponse, ProjectWithDetails from app.schemas.task_status import TaskStatusResponse +from app.schemas.project_member import ( + ProjectMemberCreate, + ProjectMemberUpdate, + ProjectMemberResponse, + ProjectMemberWithDetails, + ProjectMemberListResponse, +) from app.middleware.auth import ( get_current_user, check_space_access, check_space_edit_access, check_project_access, check_project_edit_access @@ -336,3 +343,271 @@ async def list_project_statuses( ).order_by(TaskStatus.position).all() return statuses + + +# ============================================================================ +# Project Members API - Cross-Department Collaboration +# ============================================================================ + + +@router.get("/api/projects/{project_id}/members", response_model=ProjectMemberListResponse) +async def list_project_members( + project_id: str, + db: Session = Depends(get_db), + current_user: User = Depends(get_current_user), +): + """ + List all members of a project. + + Only users with project access can view the member list. + """ + project = db.query(Project).filter(Project.id == project_id, Project.is_active == True).first() + + if not project: + raise HTTPException( + status_code=status.HTTP_404_NOT_FOUND, + detail="Project not found", + ) + + if not check_project_access(current_user, project): + raise HTTPException( + status_code=status.HTTP_403_FORBIDDEN, + detail="Access denied", + ) + + members = db.query(ProjectMember).filter( + ProjectMember.project_id == project_id + ).all() + + member_list = [] + for member in members: + user = db.query(User).filter(User.id == member.user_id).first() + added_by_user = db.query(User).filter(User.id == member.added_by).first() + + member_list.append(ProjectMemberWithDetails( + id=member.id, + project_id=member.project_id, + user_id=member.user_id, + role=member.role, + added_by=member.added_by, + created_at=member.created_at, + user_name=user.name if user else None, + user_email=user.email if user else None, + user_department_id=user.department_id if user else None, + user_department_name=user.department.name if user and user.department else None, + added_by_name=added_by_user.name if added_by_user else None, + )) + + return ProjectMemberListResponse( + members=member_list, + total=len(member_list), + ) + + +@router.post("/api/projects/{project_id}/members", response_model=ProjectMemberResponse, status_code=status.HTTP_201_CREATED) +async def add_project_member( + project_id: str, + member_data: ProjectMemberCreate, + request: Request, + db: Session = Depends(get_db), + current_user: User = Depends(get_current_user), +): + """ + Add a user as a project member for cross-department collaboration. + + Only project owners and members with 'admin' role can add new members. + """ + project = db.query(Project).filter(Project.id == project_id, Project.is_active == True).first() + + if not project: + raise HTTPException( + status_code=status.HTTP_404_NOT_FOUND, + detail="Project not found", + ) + + # Check if user has permission to add members (owner or admin member) + if not check_project_edit_access(current_user, project): + raise HTTPException( + status_code=status.HTTP_403_FORBIDDEN, + detail="Only project owner or admin members can add new members", + ) + + # Check if user exists + user_to_add = db.query(User).filter(User.id == member_data.user_id, User.is_active == True).first() + if not user_to_add: + raise HTTPException( + status_code=status.HTTP_404_NOT_FOUND, + detail="User not found", + ) + + # Check if user is already a member + existing_member = db.query(ProjectMember).filter( + ProjectMember.project_id == project_id, + ProjectMember.user_id == member_data.user_id, + ).first() + + if existing_member: + raise HTTPException( + status_code=status.HTTP_409_CONFLICT, + detail="User is already a member of this project", + ) + + # Don't add the owner as a member (they already have access) + if member_data.user_id == project.owner_id: + raise HTTPException( + status_code=status.HTTP_400_BAD_REQUEST, + detail="Project owner cannot be added as a member", + ) + + # Create the membership + member = ProjectMember( + id=str(uuid.uuid4()), + project_id=project_id, + user_id=member_data.user_id, + role=member_data.role.value, + added_by=current_user.id, + ) + + db.add(member) + + # Audit log + AuditService.log_event( + db=db, + event_type="project_member.add", + resource_type="project_member", + action=AuditAction.CREATE, + user_id=current_user.id, + resource_id=member.id, + changes=[ + {"field": "user_id", "old_value": None, "new_value": member_data.user_id}, + {"field": "role", "old_value": None, "new_value": member_data.role.value}, + ], + request_metadata=get_audit_metadata(request), + ) + + db.commit() + db.refresh(member) + + return member + + +@router.patch("/api/projects/{project_id}/members/{member_id}", response_model=ProjectMemberResponse) +async def update_project_member( + project_id: str, + member_id: str, + member_data: ProjectMemberUpdate, + request: Request, + db: Session = Depends(get_db), + current_user: User = Depends(get_current_user), +): + """ + Update a project member's role. + + Only project owners and members with 'admin' role can update member roles. + """ + project = db.query(Project).filter(Project.id == project_id, Project.is_active == True).first() + + if not project: + raise HTTPException( + status_code=status.HTTP_404_NOT_FOUND, + detail="Project not found", + ) + + if not check_project_edit_access(current_user, project): + raise HTTPException( + status_code=status.HTTP_403_FORBIDDEN, + detail="Only project owner or admin members can update member roles", + ) + + member = db.query(ProjectMember).filter( + ProjectMember.id == member_id, + ProjectMember.project_id == project_id, + ).first() + + if not member: + raise HTTPException( + status_code=status.HTTP_404_NOT_FOUND, + detail="Member not found", + ) + + old_role = member.role + member.role = member_data.role.value + + # Audit log + AuditService.log_event( + db=db, + event_type="project_member.update", + resource_type="project_member", + action=AuditAction.UPDATE, + user_id=current_user.id, + resource_id=member.id, + changes=[{"field": "role", "old_value": old_role, "new_value": member_data.role.value}], + request_metadata=get_audit_metadata(request), + ) + + db.commit() + db.refresh(member) + + return member + + +@router.delete("/api/projects/{project_id}/members/{member_id}", status_code=status.HTTP_204_NO_CONTENT) +async def remove_project_member( + project_id: str, + member_id: str, + request: Request, + db: Session = Depends(get_db), + current_user: User = Depends(get_current_user), +): + """ + Remove a member from a project. + + Only project owners and members with 'admin' role can remove members. + Members can also remove themselves from a project. + """ + project = db.query(Project).filter(Project.id == project_id, Project.is_active == True).first() + + if not project: + raise HTTPException( + status_code=status.HTTP_404_NOT_FOUND, + detail="Project not found", + ) + + member = db.query(ProjectMember).filter( + ProjectMember.id == member_id, + ProjectMember.project_id == project_id, + ).first() + + if not member: + raise HTTPException( + status_code=status.HTTP_404_NOT_FOUND, + detail="Member not found", + ) + + # Allow self-removal or admin access + is_self_removal = member.user_id == current_user.id + if not is_self_removal and not check_project_edit_access(current_user, project): + raise HTTPException( + status_code=status.HTTP_403_FORBIDDEN, + detail="Only project owner, admin members, or the member themselves can remove membership", + ) + + # Audit log + AuditService.log_event( + db=db, + event_type="project_member.remove", + resource_type="project_member", + action=AuditAction.DELETE, + user_id=current_user.id, + resource_id=member.id, + changes=[ + {"field": "user_id", "old_value": member.user_id, "new_value": None}, + {"field": "role", "old_value": member.role, "new_value": None}, + ], + request_metadata=get_audit_metadata(request), + ) + + db.delete(member) + db.commit() + + return None diff --git a/backend/app/api/reports/router.py b/backend/app/api/reports/router.py index 2e8c2cd..9b37153 100644 --- a/backend/app/api/reports/router.py +++ b/backend/app/api/reports/router.py @@ -1,8 +1,10 @@ -from fastapi import APIRouter, Depends, HTTPException, status, Query +from fastapi import APIRouter, Depends, HTTPException, status, Query, Request from sqlalchemy.orm import Session from typing import Optional from app.core.database import get_db +from app.core.rate_limiter import limiter +from app.core.config import settings from app.models import User, ReportHistory, ScheduledReport from app.schemas.report import ( WeeklyReportContent, ReportHistoryListResponse, ReportHistoryItem, @@ -35,12 +37,16 @@ async def preview_weekly_report( @router.post("/api/reports/weekly/generate", response_model=GenerateReportResponse) +@limiter.limit(settings.RATE_LIMIT_HEAVY) async def generate_weekly_report( + request: Request, db: Session = Depends(get_db), current_user: User = Depends(get_current_user), ): """ Manually trigger weekly report generation for the current user. + + Rate limited: 5 requests per minute (heavy tier). """ # Generate report report_history = ReportService.generate_weekly_report(db, current_user.id) @@ -112,13 +118,17 @@ async def list_report_history( @router.get("/api/reports/history/{report_id}") +@limiter.limit(settings.RATE_LIMIT_SENSITIVE) async def get_report_detail( + request: Request, report_id: str, db: Session = Depends(get_db), current_user: User = Depends(get_current_user), ): """ Get detailed content of a specific report. + + Rate limited: 20 requests per minute (sensitive tier). """ report = db.query(ReportHistory).filter(ReportHistory.id == report_id).first() diff --git a/backend/app/api/task_dependencies/router.py b/backend/app/api/task_dependencies/router.py index 911531d..8839114 100644 --- a/backend/app/api/task_dependencies/router.py +++ b/backend/app/api/task_dependencies/router.py @@ -10,13 +10,18 @@ from sqlalchemy.orm import Session from typing import Optional from app.core.database import get_db +from app.core.rate_limiter import limiter +from app.core.config import settings from app.models import User, Task, TaskDependency, AuditAction from app.schemas.task_dependency import ( TaskDependencyCreate, TaskDependencyUpdate, TaskDependencyResponse, TaskDependencyListResponse, - TaskInfo + TaskInfo, + BulkDependencyCreate, + BulkDependencyValidationResult, + BulkDependencyCreateResponse, ) from app.middleware.auth import get_current_user, check_task_access, check_task_edit_access from app.middleware.audit import get_audit_metadata @@ -429,3 +434,184 @@ async def list_project_dependencies( dependencies=[dependency_to_response(d) for d in dependencies], total=len(dependencies) ) + + +@router.post( + "/api/projects/{project_id}/dependencies/validate", + response_model=BulkDependencyValidationResult +) +async def validate_bulk_dependencies( + project_id: str, + bulk_data: BulkDependencyCreate, + db: Session = Depends(get_db), + current_user: User = Depends(get_current_user), +): + """ + Validate a batch of dependencies without creating them. + + This endpoint checks for: + - Self-references + - Cross-project dependencies + - Duplicate dependencies + - Circular dependencies (including cycles that would be created by the batch) + + Returns validation results without modifying the database. + """ + from app.models import Project + from app.middleware.auth import check_project_access + + project = db.query(Project).filter(Project.id == project_id).first() + if not project: + raise HTTPException( + status_code=status.HTTP_404_NOT_FOUND, + detail="Project not found" + ) + + if not check_project_access(current_user, project): + raise HTTPException( + status_code=status.HTTP_403_FORBIDDEN, + detail="Access denied" + ) + + # Convert to tuple format for validation + dependencies = [ + (dep.predecessor_id, dep.successor_id) + for dep in bulk_data.dependencies + ] + + errors = DependencyService.validate_bulk_dependencies(db, dependencies, project_id) + + return BulkDependencyValidationResult( + valid=len(errors) == 0, + errors=errors + ) + + +@router.post( + "/api/projects/{project_id}/dependencies/bulk", + response_model=BulkDependencyCreateResponse, + status_code=status.HTTP_201_CREATED +) +@limiter.limit(settings.RATE_LIMIT_HEAVY) +async def create_bulk_dependencies( + request: Request, + project_id: str, + bulk_data: BulkDependencyCreate, + db: Session = Depends(get_db), + current_user: User = Depends(get_current_user), +): + """ + Create multiple dependencies at once. + + This endpoint: + 1. Validates all dependencies together for cycle detection + 2. Creates valid dependencies + 3. Returns both created dependencies and any failures + + Cycle detection considers all dependencies in the batch together, + so cycles that would only appear when all dependencies are added + will be caught. + + Rate limited: 5 requests per minute (heavy tier). + """ + from app.models import Project + from app.middleware.auth import check_project_edit_access + + project = db.query(Project).filter(Project.id == project_id).first() + if not project: + raise HTTPException( + status_code=status.HTTP_404_NOT_FOUND, + detail="Project not found" + ) + + if not check_project_edit_access(current_user, project): + raise HTTPException( + status_code=status.HTTP_403_FORBIDDEN, + detail="Permission denied" + ) + + # Convert to tuple format for validation + dependencies_to_validate = [ + (dep.predecessor_id, dep.successor_id) + for dep in bulk_data.dependencies + ] + + # Validate all dependencies together + errors = DependencyService.validate_bulk_dependencies( + db, dependencies_to_validate, project_id + ) + + # Build a set of failed dependency pairs for quick lookup + failed_pairs = set() + for error in errors: + pair = (error.get("predecessor_id"), error.get("successor_id")) + failed_pairs.add(pair) + + created_dependencies = [] + failed_items = errors # Include validation errors + + # Create dependencies that passed validation + for dep_data in bulk_data.dependencies: + pair = (dep_data.predecessor_id, dep_data.successor_id) + if pair in failed_pairs: + continue + + # Additional check: verify dependency limit for successor + current_count = db.query(TaskDependency).filter( + TaskDependency.successor_id == dep_data.successor_id + ).count() + + if current_count >= DependencyService.MAX_DIRECT_DEPENDENCIES: + failed_items.append({ + "error_type": "limit_exceeded", + "predecessor_id": dep_data.predecessor_id, + "successor_id": dep_data.successor_id, + "message": f"Successor task already has {DependencyService.MAX_DIRECT_DEPENDENCIES} dependencies" + }) + continue + + # Create the dependency + dependency = TaskDependency( + id=str(uuid.uuid4()), + predecessor_id=dep_data.predecessor_id, + successor_id=dep_data.successor_id, + dependency_type=dep_data.dependency_type.value, + lag_days=dep_data.lag_days + ) + + db.add(dependency) + created_dependencies.append(dependency) + + # Audit log for each created dependency + AuditService.log_event( + db=db, + event_type="task.dependency.create", + resource_type="task_dependency", + action=AuditAction.CREATE, + user_id=current_user.id, + resource_id=dependency.id, + changes=[{ + "field": "dependency", + "old_value": None, + "new_value": { + "predecessor_id": dependency.predecessor_id, + "successor_id": dependency.successor_id, + "dependency_type": dependency.dependency_type, + "lag_days": dependency.lag_days + } + }], + request_metadata=get_audit_metadata(request) + ) + + db.commit() + + # Refresh created dependencies to get relationships + for dep in created_dependencies: + db.refresh(dep) + + return BulkDependencyCreateResponse( + created=[dependency_to_response(d) for d in created_dependencies], + failed=failed_items, + total_created=len(created_dependencies), + total_failed=len(failed_items) + ) diff --git a/backend/app/api/tasks/router.py b/backend/app/api/tasks/router.py index 714f62a..00c8044 100644 --- a/backend/app/api/tasks/router.py +++ b/backend/app/api/tasks/router.py @@ -1,16 +1,20 @@ import logging import uuid -from datetime import datetime, timezone +from datetime import datetime, timezone, timedelta from typing import List, Optional from fastapi import APIRouter, Depends, HTTPException, status, Query, Request from sqlalchemy.orm import Session from app.core.database import get_db from app.core.redis_pubsub import publish_task_event +from app.core.rate_limiter import limiter +from app.core.config import settings from app.models import User, Project, Task, TaskStatus, AuditAction, Blocker from app.schemas.task import ( TaskCreate, TaskUpdate, TaskResponse, TaskWithDetails, TaskListResponse, - TaskStatusUpdate, TaskAssignUpdate, CustomValueResponse + TaskStatusUpdate, TaskAssignUpdate, CustomValueResponse, + TaskRestoreRequest, TaskRestoreResponse, + TaskDeleteWarningResponse, TaskDeleteResponse ) from app.middleware.auth import ( get_current_user, check_project_access, check_task_access, check_task_edit_access @@ -72,6 +76,7 @@ def task_to_response(task: Task, db: Session = None, include_custom_values: bool created_by=task.created_by, created_at=task.created_at, updated_at=task.updated_at, + version=task.version, assignee_name=task.assignee.name if task.assignee else None, status_name=task.status.name if task.status else None, status_color=task.status.color if task.status else None, @@ -161,15 +166,18 @@ async def list_tasks( @router.post("/api/projects/{project_id}/tasks", response_model=TaskResponse, status_code=status.HTTP_201_CREATED) +@limiter.limit(settings.RATE_LIMIT_STANDARD) async def create_task( + request: Request, project_id: str, task_data: TaskCreate, - request: Request, db: Session = Depends(get_db), current_user: User = Depends(get_current_user), ): """ Create a new task in a project. + + Rate limited: 60 requests per minute (standard tier). """ project = db.query(Project).filter(Project.id == project_id).first() @@ -367,15 +375,18 @@ async def get_task( @router.patch("/api/tasks/{task_id}", response_model=TaskResponse) +@limiter.limit(settings.RATE_LIMIT_STANDARD) async def update_task( + request: Request, task_id: str, task_data: TaskUpdate, - request: Request, db: Session = Depends(get_db), current_user: User = Depends(get_current_user), ): """ Update a task. + + Rate limited: 60 requests per minute (standard tier). """ task = db.query(Task).filter(Task.id == task_id).first() @@ -391,6 +402,18 @@ async def update_task( detail="Permission denied", ) + # Optimistic locking: validate version if provided + if task_data.version is not None: + if task_data.version != task.version: + raise HTTPException( + status_code=status.HTTP_409_CONFLICT, + detail={ + "message": "Task has been modified by another user", + "current_version": task.version, + "provided_version": task_data.version, + }, + ) + # Capture old values for audit and triggers old_values = { "title": task.title, @@ -402,9 +425,10 @@ async def update_task( "time_spent": task.time_spent, } - # Update fields (exclude custom_values, handle separately) + # Update fields (exclude custom_values and version, handle separately) update_data = task_data.model_dump(exclude_unset=True) custom_values_data = update_data.pop("custom_values", None) + update_data.pop("version", None) # version is handled separately for optimistic locking # Track old assignee for workload cache invalidation old_assignee_id = task.assignee_id @@ -501,6 +525,9 @@ async def update_task( detail=str(e), ) + # Increment version for optimistic locking + task.version += 1 + db.commit() db.refresh(task) @@ -551,15 +578,20 @@ async def update_task( return task -@router.delete("/api/tasks/{task_id}", response_model=TaskResponse) +@router.delete("/api/tasks/{task_id}") async def delete_task( task_id: str, request: Request, + force_delete: bool = Query(False, description="Force delete even if task has unresolved blockers"), db: Session = Depends(get_db), current_user: User = Depends(get_current_user), ): """ Soft delete a task (cascades to subtasks). + + If the task has unresolved blockers and force_delete is False, + returns a warning response with status 200 and blocker count. + Use force_delete=true to delete anyway (auto-resolves blockers). """ task = db.query(Task).filter(Task.id == task_id).first() @@ -581,9 +613,35 @@ async def delete_task( detail="Permission denied", ) + # Check for unresolved blockers + unresolved_blockers = db.query(Blocker).filter( + Blocker.task_id == task.id, + Blocker.resolved_at == None, + ).all() + + blocker_count = len(unresolved_blockers) + + # If there are unresolved blockers and force_delete is False, return warning + if blocker_count > 0 and not force_delete: + return TaskDeleteWarningResponse( + warning="Task has unresolved blockers", + blocker_count=blocker_count, + message=f"Task has {blocker_count} unresolved blocker(s). Use force_delete=true to delete anyway.", + ) + # Use naive datetime for consistency with database storage now = datetime.now(timezone.utc).replace(tzinfo=None) + # Auto-resolve blockers if force deleting + blockers_resolved = 0 + if force_delete and blocker_count > 0: + for blocker in unresolved_blockers: + blocker.resolved_at = now + blocker.resolved_by = current_user.id + blocker.resolution_note = "Auto-resolved due to task deletion" + blockers_resolved += 1 + logger.info(f"Auto-resolved {blockers_resolved} blocker(s) for task {task_id} during force delete") + # Soft delete the task task.is_deleted = True task.deleted_at = now @@ -608,7 +666,11 @@ async def delete_task( action=AuditAction.DELETE, user_id=current_user.id, resource_id=task.id, - changes=[{"field": "is_deleted", "old_value": False, "new_value": True}], + changes=[ + {"field": "is_deleted", "old_value": False, "new_value": True}, + {"field": "force_delete", "old_value": None, "new_value": force_delete}, + {"field": "blockers_resolved", "old_value": None, "new_value": blockers_resolved}, + ], request_metadata=get_audit_metadata(request), ) @@ -635,18 +697,33 @@ async def delete_task( except Exception as e: logger.warning(f"Failed to publish task_deleted event: {e}") - return task + return TaskDeleteResponse( + task=task, + blockers_resolved=blockers_resolved, + force_deleted=force_delete and blocker_count > 0, + ) -@router.post("/api/tasks/{task_id}/restore", response_model=TaskResponse) +@router.post("/api/tasks/{task_id}/restore", response_model=TaskRestoreResponse) async def restore_task( task_id: str, request: Request, + restore_data: TaskRestoreRequest = None, db: Session = Depends(get_db), current_user: User = Depends(get_current_user), ): """ Restore a soft-deleted task (admin only). + + Supports cascade restore: when enabled (default), also restores child tasks + that were deleted at the same time as the parent task. + + Args: + task_id: ID of the task to restore + restore_data: Optional restore options (cascade=True by default) + + Returns: + TaskRestoreResponse with restored task and list of restored children """ if not current_user.is_system_admin: raise HTTPException( @@ -654,6 +731,10 @@ async def restore_task( detail="Only system administrators can restore deleted tasks", ) + # Handle default for optional body + if restore_data is None: + restore_data = TaskRestoreRequest() + task = db.query(Task).filter(Task.id == task_id).first() if not task: @@ -668,12 +749,16 @@ async def restore_task( detail="Task is not deleted", ) - # Restore the task + # Store the parent's deleted_at timestamp for cascade restore + parent_deleted_at = task.deleted_at + restored_children_ids = [] + + # Restore the parent task task.is_deleted = False task.deleted_at = None task.deleted_by = None - # Audit log + # Audit log for parent task AuditService.log_event( db=db, event_type="task.restore", @@ -681,18 +766,119 @@ async def restore_task( action=AuditAction.UPDATE, user_id=current_user.id, resource_id=task.id, - changes=[{"field": "is_deleted", "old_value": True, "new_value": False}], + changes=[ + {"field": "is_deleted", "old_value": True, "new_value": False}, + {"field": "cascade", "old_value": None, "new_value": restore_data.cascade}, + ], request_metadata=get_audit_metadata(request), ) + # Cascade restore child tasks if requested + if restore_data.cascade and parent_deleted_at: + restored_children_ids = _cascade_restore_children( + db=db, + parent_task=task, + parent_deleted_at=parent_deleted_at, + current_user=current_user, + request=request, + ) + db.commit() db.refresh(task) - # Invalidate workload cache for assignee + # Invalidate workload cache for parent task assignee if task.assignee_id: invalidate_user_workload_cache(task.assignee_id) - return task + # Invalidate workload cache for all restored children's assignees + for child_id in restored_children_ids: + child_task = db.query(Task).filter(Task.id == child_id).first() + if child_task and child_task.assignee_id: + invalidate_user_workload_cache(child_task.assignee_id) + + return TaskRestoreResponse( + restored_task=task, + restored_children_count=len(restored_children_ids), + restored_children_ids=restored_children_ids, + ) + + +def _cascade_restore_children( + db: Session, + parent_task: Task, + parent_deleted_at: datetime, + current_user: User, + request: Request, + tolerance_seconds: int = 5, +) -> List[str]: + """ + Recursively restore child tasks that were deleted at the same time as the parent. + + Args: + db: Database session + parent_task: The parent task being restored + parent_deleted_at: Timestamp when the parent was deleted + current_user: Current user performing the restore + request: HTTP request for audit metadata + tolerance_seconds: Time tolerance for matching deleted_at timestamps + + Returns: + List of restored child task IDs + """ + restored_ids = [] + + # Find all deleted child tasks with matching deleted_at timestamp + # Use a small tolerance window to account for slight timing differences + time_window_start = parent_deleted_at - timedelta(seconds=tolerance_seconds) + time_window_end = parent_deleted_at + timedelta(seconds=tolerance_seconds) + + deleted_children = db.query(Task).filter( + Task.parent_task_id == parent_task.id, + Task.is_deleted == True, + Task.deleted_at >= time_window_start, + Task.deleted_at <= time_window_end, + ).all() + + for child in deleted_children: + # Store child's deleted_at before restoring + child_deleted_at = child.deleted_at + + # Restore the child + child.is_deleted = False + child.deleted_at = None + child.deleted_by = None + restored_ids.append(child.id) + + # Audit log for child task + AuditService.log_event( + db=db, + event_type="task.restore", + resource_type="task", + action=AuditAction.UPDATE, + user_id=current_user.id, + resource_id=child.id, + changes=[ + {"field": "is_deleted", "old_value": True, "new_value": False}, + {"field": "restored_via_cascade", "old_value": None, "new_value": parent_task.id}, + ], + request_metadata=get_audit_metadata(request), + ) + + logger.info(f"Cascade restored child task {child.id} (parent: {parent_task.id})") + + # Recursively restore grandchildren + if child_deleted_at: + grandchildren_ids = _cascade_restore_children( + db=db, + parent_task=child, + parent_deleted_at=child_deleted_at, + current_user=current_user, + request=request, + tolerance_seconds=tolerance_seconds, + ) + restored_ids.extend(grandchildren_ids) + + return restored_ids @router.patch("/api/tasks/{task_id}/status", response_model=TaskResponse) diff --git a/backend/app/api/templates/__init__.py b/backend/app/api/templates/__init__.py new file mode 100644 index 0000000..4d9432d --- /dev/null +++ b/backend/app/api/templates/__init__.py @@ -0,0 +1,3 @@ +from app.api.templates.router import router + +__all__ = ["router"] diff --git a/backend/app/api/templates/router.py b/backend/app/api/templates/router.py new file mode 100644 index 0000000..c68b26e --- /dev/null +++ b/backend/app/api/templates/router.py @@ -0,0 +1,440 @@ +"""Project Templates API endpoints. + +Provides CRUD operations for project templates and +the ability to create projects from templates. +""" +import uuid +from typing import List, Optional +from fastapi import APIRouter, Depends, HTTPException, status, Request, Query +from sqlalchemy.orm import Session + +from app.core.database import get_db +from app.models import ( + User, Space, Project, TaskStatus, CustomField, ProjectTemplate, AuditAction +) +from app.schemas.project_template import ( + ProjectTemplateCreate, + ProjectTemplateUpdate, + ProjectTemplateResponse, + ProjectTemplateWithOwner, + ProjectTemplateListResponse, + CreateProjectFromTemplateRequest, + CreateProjectFromTemplateResponse, +) +from app.middleware.auth import get_current_user, check_space_access +from app.middleware.audit import get_audit_metadata +from app.services.audit_service import AuditService + +router = APIRouter(prefix="/api/templates", tags=["Project Templates"]) + + +def can_view_template(user: User, template: ProjectTemplate) -> bool: + """Check if a user can view a template.""" + if template.is_public: + return True + if template.owner_id == user.id: + return True + if user.is_system_admin: + return True + return False + + +def can_edit_template(user: User, template: ProjectTemplate) -> bool: + """Check if a user can edit a template.""" + if template.owner_id == user.id: + return True + if user.is_system_admin: + return True + return False + + +@router.get("", response_model=ProjectTemplateListResponse) +async def list_templates( + include_private: bool = Query(False, description="Include user's private templates"), + db: Session = Depends(get_db), + current_user: User = Depends(get_current_user), +): + """ + List available project templates. + + By default, only returns public templates. + Set include_private=true to also include the user's private templates. + """ + query = db.query(ProjectTemplate).filter(ProjectTemplate.is_active == True) + + if include_private: + # Public templates OR user's own templates + query = query.filter( + (ProjectTemplate.is_public == True) | + (ProjectTemplate.owner_id == current_user.id) + ) + else: + # Only public templates + query = query.filter(ProjectTemplate.is_public == True) + + templates = query.order_by(ProjectTemplate.name).all() + + result = [] + for template in templates: + result.append(ProjectTemplateWithOwner( + id=template.id, + name=template.name, + description=template.description, + is_public=template.is_public, + task_statuses=template.task_statuses, + custom_fields=template.custom_fields, + default_security_level=template.default_security_level, + owner_id=template.owner_id, + is_active=template.is_active, + created_at=template.created_at, + updated_at=template.updated_at, + owner_name=template.owner.name if template.owner else None, + )) + + return ProjectTemplateListResponse( + templates=result, + total=len(result), + ) + + +@router.post("", response_model=ProjectTemplateResponse, status_code=status.HTTP_201_CREATED) +async def create_template( + template_data: ProjectTemplateCreate, + request: Request, + db: Session = Depends(get_db), + current_user: User = Depends(get_current_user), +): + """ + Create a new project template. + + The template can include predefined task statuses and custom fields + that will be copied when creating a project from this template. + """ + # Convert Pydantic models to dict for JSON storage + task_statuses_json = None + if template_data.task_statuses: + task_statuses_json = [ts.model_dump() for ts in template_data.task_statuses] + + custom_fields_json = None + if template_data.custom_fields: + custom_fields_json = [cf.model_dump() for cf in template_data.custom_fields] + + template = ProjectTemplate( + id=str(uuid.uuid4()), + name=template_data.name, + description=template_data.description, + owner_id=current_user.id, + is_public=template_data.is_public, + task_statuses=task_statuses_json, + custom_fields=custom_fields_json, + default_security_level=template_data.default_security_level, + ) + + db.add(template) + + # Audit log + AuditService.log_event( + db=db, + event_type="template.create", + resource_type="project_template", + action=AuditAction.CREATE, + user_id=current_user.id, + resource_id=template.id, + changes=[{"field": "name", "old_value": None, "new_value": template.name}], + request_metadata=get_audit_metadata(request), + ) + + db.commit() + db.refresh(template) + + return template + + +@router.get("/{template_id}", response_model=ProjectTemplateWithOwner) +async def get_template( + template_id: str, + db: Session = Depends(get_db), + current_user: User = Depends(get_current_user), +): + """ + Get a project template by ID. + """ + template = db.query(ProjectTemplate).filter( + ProjectTemplate.id == template_id, + ProjectTemplate.is_active == True + ).first() + + if not template: + raise HTTPException( + status_code=status.HTTP_404_NOT_FOUND, + detail="Template not found", + ) + + if not can_view_template(current_user, template): + raise HTTPException( + status_code=status.HTTP_403_FORBIDDEN, + detail="Access denied", + ) + + return ProjectTemplateWithOwner( + id=template.id, + name=template.name, + description=template.description, + is_public=template.is_public, + task_statuses=template.task_statuses, + custom_fields=template.custom_fields, + default_security_level=template.default_security_level, + owner_id=template.owner_id, + is_active=template.is_active, + created_at=template.created_at, + updated_at=template.updated_at, + owner_name=template.owner.name if template.owner else None, + ) + + +@router.patch("/{template_id}", response_model=ProjectTemplateResponse) +async def update_template( + template_id: str, + template_data: ProjectTemplateUpdate, + request: Request, + db: Session = Depends(get_db), + current_user: User = Depends(get_current_user), +): + """ + Update a project template. + + Only the template owner or system admin can update a template. + """ + template = db.query(ProjectTemplate).filter( + ProjectTemplate.id == template_id, + ProjectTemplate.is_active == True + ).first() + + if not template: + raise HTTPException( + status_code=status.HTTP_404_NOT_FOUND, + detail="Template not found", + ) + + if not can_edit_template(current_user, template): + raise HTTPException( + status_code=status.HTTP_403_FORBIDDEN, + detail="Only template owner can update", + ) + + # Capture old values for audit + old_values = { + "name": template.name, + "description": template.description, + "is_public": template.is_public, + } + + # Update fields + update_data = template_data.model_dump(exclude_unset=True) + + # Convert Pydantic models to dict for JSON storage + if "task_statuses" in update_data and update_data["task_statuses"]: + update_data["task_statuses"] = [ts.model_dump() if hasattr(ts, 'model_dump') else ts for ts in update_data["task_statuses"]] + + if "custom_fields" in update_data and update_data["custom_fields"]: + update_data["custom_fields"] = [cf.model_dump() if hasattr(cf, 'model_dump') else cf for cf in update_data["custom_fields"]] + + for field, value in update_data.items(): + setattr(template, field, value) + + # Log changes + new_values = { + "name": template.name, + "description": template.description, + "is_public": template.is_public, + } + + changes = AuditService.detect_changes(old_values, new_values) + if changes: + AuditService.log_event( + db=db, + event_type="template.update", + resource_type="project_template", + action=AuditAction.UPDATE, + user_id=current_user.id, + resource_id=template.id, + changes=changes, + request_metadata=get_audit_metadata(request), + ) + + db.commit() + db.refresh(template) + + return template + + +@router.delete("/{template_id}", status_code=status.HTTP_204_NO_CONTENT) +async def delete_template( + template_id: str, + request: Request, + db: Session = Depends(get_db), + current_user: User = Depends(get_current_user), +): + """ + Delete a project template (soft delete). + + Only the template owner or system admin can delete a template. + """ + template = db.query(ProjectTemplate).filter( + ProjectTemplate.id == template_id, + ProjectTemplate.is_active == True + ).first() + + if not template: + raise HTTPException( + status_code=status.HTTP_404_NOT_FOUND, + detail="Template not found", + ) + + if not can_edit_template(current_user, template): + raise HTTPException( + status_code=status.HTTP_403_FORBIDDEN, + detail="Only template owner can delete", + ) + + # Audit log + AuditService.log_event( + db=db, + event_type="template.delete", + resource_type="project_template", + action=AuditAction.DELETE, + user_id=current_user.id, + resource_id=template.id, + changes=[{"field": "is_active", "old_value": True, "new_value": False}], + request_metadata=get_audit_metadata(request), + ) + + # Soft delete + template.is_active = False + db.commit() + + return None + + +@router.post("/create-project", response_model=CreateProjectFromTemplateResponse, status_code=status.HTTP_201_CREATED) +async def create_project_from_template( + data: CreateProjectFromTemplateRequest, + request: Request, + db: Session = Depends(get_db), + current_user: User = Depends(get_current_user), +): + """ + Create a new project from a template. + + This will: + 1. Create a new project with the specified title and description + 2. Copy all task statuses from the template + 3. Copy all custom field definitions from the template + """ + # Get the template + template = db.query(ProjectTemplate).filter( + ProjectTemplate.id == data.template_id, + ProjectTemplate.is_active == True + ).first() + + if not template: + raise HTTPException( + status_code=status.HTTP_404_NOT_FOUND, + detail="Template not found", + ) + + if not can_view_template(current_user, template): + raise HTTPException( + status_code=status.HTTP_403_FORBIDDEN, + detail="Access denied to template", + ) + + # Check space access + space = db.query(Space).filter( + Space.id == data.space_id, + Space.is_active == True + ).first() + + if not space: + raise HTTPException( + status_code=status.HTTP_404_NOT_FOUND, + detail="Space not found", + ) + + if not check_space_access(current_user, space): + raise HTTPException( + status_code=status.HTTP_403_FORBIDDEN, + detail="Access denied to space", + ) + + # Create the project + project = Project( + id=str(uuid.uuid4()), + space_id=data.space_id, + title=data.title, + description=data.description, + owner_id=current_user.id, + security_level=template.default_security_level or "department", + department_id=data.department_id or current_user.department_id, + ) + + db.add(project) + db.flush() # Get project ID + + # Copy task statuses from template + task_statuses_created = 0 + if template.task_statuses: + for status_data in template.task_statuses: + task_status = TaskStatus( + id=str(uuid.uuid4()), + project_id=project.id, + name=status_data.get("name", "Unnamed"), + color=status_data.get("color", "#808080"), + position=status_data.get("position", 0), + is_done=status_data.get("is_done", False), + ) + db.add(task_status) + task_statuses_created += 1 + + # Copy custom fields from template + custom_fields_created = 0 + if template.custom_fields: + for field_data in template.custom_fields: + custom_field = CustomField( + id=str(uuid.uuid4()), + project_id=project.id, + name=field_data.get("name", "Unnamed"), + field_type=field_data.get("field_type", "text"), + options=field_data.get("options"), + formula=field_data.get("formula"), + is_required=field_data.get("is_required", False), + position=field_data.get("position", 0), + ) + db.add(custom_field) + custom_fields_created += 1 + + # Audit log + AuditService.log_event( + db=db, + event_type="project.create_from_template", + resource_type="project", + action=AuditAction.CREATE, + user_id=current_user.id, + resource_id=project.id, + changes=[ + {"field": "title", "old_value": None, "new_value": project.title}, + {"field": "template_id", "old_value": None, "new_value": template.id}, + ], + request_metadata=get_audit_metadata(request), + ) + + db.commit() + + return CreateProjectFromTemplateResponse( + id=project.id, + title=project.title, + template_id=template.id, + template_name=template.name, + task_statuses_created=task_statuses_created, + custom_fields_created=custom_fields_created, + ) diff --git a/backend/app/api/websocket/router.py b/backend/app/api/websocket/router.py index b2b1377..0cd30b9 100644 --- a/backend/app/api/websocket/router.py +++ b/backend/app/api/websocket/router.py @@ -1,6 +1,7 @@ import asyncio import logging import time +from typing import Optional from fastapi import APIRouter, WebSocket, WebSocketDisconnect, Query from sqlalchemy.orm import Session @@ -19,6 +20,9 @@ router = APIRouter(tags=["websocket"]) PING_INTERVAL = 60.0 # Send ping after this many seconds of no messages PONG_TIMEOUT = 30.0 # Disconnect if no pong received within this time after ping +# Authentication timeout (10 seconds) +AUTH_TIMEOUT = 10.0 + async def get_user_from_token(token: str) -> tuple[str | None, User | None]: """Validate token and return user_id and user object.""" @@ -47,6 +51,56 @@ async def get_user_from_token(token: str) -> tuple[str | None, User | None]: db.close() +async def authenticate_websocket( + websocket: WebSocket, + query_token: Optional[str] = None +) -> tuple[str | None, User | None]: + """ + Authenticate WebSocket connection. + + Supports two authentication methods: + 1. First message authentication (preferred, more secure) + - Client sends: {"type": "auth", "token": ""} + 2. Query parameter authentication (deprecated, for backward compatibility) + - Client connects with: ?token= + + Returns (user_id, user) if authenticated, (None, None) otherwise. + """ + # If token provided via query parameter (backward compatibility) + if query_token: + logger.warning( + "WebSocket authentication via query parameter is deprecated. " + "Please use first-message authentication for better security." + ) + return await get_user_from_token(query_token) + + # Wait for authentication message with timeout + try: + data = await asyncio.wait_for( + websocket.receive_json(), + timeout=AUTH_TIMEOUT + ) + + msg_type = data.get("type") + if msg_type != "auth": + logger.warning("Expected 'auth' message type, got: %s", msg_type) + return None, None + + token = data.get("token") + if not token: + logger.warning("No token provided in auth message") + return None, None + + return await get_user_from_token(token) + + except asyncio.TimeoutError: + logger.warning("WebSocket authentication timeout after %.1f seconds", AUTH_TIMEOUT) + return None, None + except Exception as e: + logger.error("Error during WebSocket authentication: %s", e) + return None, None + + async def get_unread_notifications(user_id: str) -> list[dict]: """Query all unread notifications for a user.""" db = SessionLocal() @@ -90,14 +144,22 @@ async def get_unread_count(user_id: str) -> int: @router.websocket("/ws/notifications") async def websocket_notifications( websocket: WebSocket, - token: str = Query(..., description="JWT token for authentication"), + token: Optional[str] = Query(None, description="JWT token (deprecated, use first-message auth)"), ): """ WebSocket endpoint for real-time notifications. - Connect with: ws://host/ws/notifications?token= + Authentication methods (in order of preference): + 1. First message authentication (recommended): + - Connect without token: ws://host/ws/notifications + - Send: {"type": "auth", "token": ""} + - Must authenticate within 10 seconds or connection will be closed + + 2. Query parameter (deprecated, for backward compatibility): + - Connect with: ws://host/ws/notifications?token= Messages sent by server: + - {"type": "auth_required"} - Sent when waiting for auth message - {"type": "connected", "data": {"user_id": "...", "message": "..."}} - Connection success - {"type": "unread_sync", "data": {"notifications": [...], "unread_count": N}} - All unread on connect - {"type": "notification", "data": {...}} - New notification @@ -106,9 +168,18 @@ async def websocket_notifications( - {"type": "pong"} - Response to client ping Messages accepted from client: + - {"type": "auth", "token": "..."} - Authentication (must be first message if no query token) - {"type": "ping"} - Client keepalive ping """ - user_id, user = await get_user_from_token(token) + # Accept WebSocket connection first + await websocket.accept() + + # If no query token, notify client that auth is required + if not token: + await websocket.send_json({"type": "auth_required"}) + + # Authenticate + user_id, user = await authenticate_websocket(websocket, token) if user_id is None: await websocket.close(code=4001, reason="Invalid or expired token") @@ -263,14 +334,22 @@ async def verify_project_access(user_id: str, project_id: str) -> tuple[bool, Pr async def websocket_project_sync( websocket: WebSocket, project_id: str, - token: str = Query(..., description="JWT token for authentication"), + token: Optional[str] = Query(None, description="JWT token (deprecated, use first-message auth)"), ): """ WebSocket endpoint for project task real-time sync. - Connect with: ws://host/ws/projects/{project_id}?token= + Authentication methods (in order of preference): + 1. First message authentication (recommended): + - Connect without token: ws://host/ws/projects/{project_id} + - Send: {"type": "auth", "token": ""} + - Must authenticate within 10 seconds or connection will be closed + + 2. Query parameter (deprecated, for backward compatibility): + - Connect with: ws://host/ws/projects/{project_id}?token= Messages sent by server: + - {"type": "auth_required"} - Sent when waiting for auth message - {"type": "connected", "data": {"project_id": "...", "user_id": "..."}} - {"type": "task_created", "data": {...}, "triggered_by": "..."} - {"type": "task_updated", "data": {...}, "triggered_by": "..."} @@ -280,10 +359,18 @@ async def websocket_project_sync( - {"type": "ping"} / {"type": "pong"} Messages accepted from client: + - {"type": "auth", "token": "..."} - Authentication (must be first message if no query token) - {"type": "ping"} - Client keepalive ping """ + # Accept WebSocket connection first + await websocket.accept() + + # If no query token, notify client that auth is required + if not token: + await websocket.send_json({"type": "auth_required"}) + # Authenticate user - user_id, user = await get_user_from_token(token) + user_id, user = await authenticate_websocket(websocket, token) if user_id is None: await websocket.close(code=4001, reason="Invalid or expired token") @@ -300,8 +387,7 @@ async def websocket_project_sync( await websocket.close(code=4004, reason="Project not found") return - # Accept connection and join project room - await websocket.accept() + # Join project room await manager.join_project(websocket, user_id, project_id) # Create Redis subscriber for project task events diff --git a/backend/app/api/workload/router.py b/backend/app/api/workload/router.py index 66b03ab..0775858 100644 --- a/backend/app/api/workload/router.py +++ b/backend/app/api/workload/router.py @@ -41,22 +41,34 @@ def check_workload_access( """ Check if current user has access to view workload data. + Access rules: + - System admin: can access all workloads + - Department manager: can access workloads of users in their department + - Regular user: can only access their own workload + Raises HTTPException if access is denied. """ # System admin can access all if current_user.is_system_admin: return - # If querying specific user, must be self - # (Phase 1: only self access for non-admin users) + # If querying specific user if target_user_id and target_user_id != current_user.id: + # Department manager can view subordinates' workload + if current_user.is_department_manager: + # Manager can view users in their department + # target_user_department_id must be provided for this check + if target_user_department_id and target_user_department_id == current_user.department_id: + return + # Access denied for non-manager or user not in same department raise HTTPException( status_code=status.HTTP_403_FORBIDDEN, detail="Access denied: Cannot view other users' workload", ) - # If querying by department, must be same department + # If querying by department if department_id and department_id != current_user.department_id: + # Department manager can only query their own department raise HTTPException( status_code=status.HTTP_403_FORBIDDEN, detail="Access denied: Cannot view other departments' workload", @@ -66,15 +78,40 @@ def check_workload_access( def filter_accessible_users( current_user: User, user_ids: Optional[List[str]] = None, + db: Optional[Session] = None, ) -> Optional[List[str]]: """ Filter user IDs to only those accessible by current user. Returns None if user can access all (system admin). + + Access rules: + - System admin: can see all users + - Department manager: can see all users in their department + - Regular user: can only see themselves """ # System admin can access all if current_user.is_system_admin: return user_ids + # Department manager can see all users in their department + if current_user.is_department_manager and current_user.department_id and db: + # Get all users in the same department + department_users = db.query(User.id).filter( + User.department_id == current_user.department_id, + User.is_active == True + ).all() + department_user_ids = {u.id for u in department_users} + + if user_ids: + # Filter to only users in manager's department + accessible = [uid for uid in user_ids if uid in department_user_ids] + if not accessible: + return [current_user.id] + return accessible + else: + # No filter specified, return all department users + return list(department_user_ids) + # Regular user can only see themselves if user_ids: # Filter to only accessible users @@ -111,6 +148,11 @@ async def get_heatmap( """ Get workload heatmap for users. + Access Rules: + - System admin: Can view all users' workload + - Department manager: Can view workload of all users in their department + - Regular user: Can only view their own workload + Returns workload summaries for users showing: - allocated_hours: Total estimated hours from tasks due this week - capacity_hours: User's weekly capacity @@ -126,8 +168,8 @@ async def get_heatmap( if department_id: check_workload_access(current_user, department_id=department_id) - # Filter user_ids based on access - accessible_user_ids = filter_accessible_users(current_user, parsed_user_ids) + # Filter user_ids based on access (pass db for manager department lookup) + accessible_user_ids = filter_accessible_users(current_user, parsed_user_ids, db) # Normalize week_start if week_start is None: @@ -181,12 +223,25 @@ async def get_user_workload( """ Get detailed workload for a specific user. + Access rules: + - System admin: can view any user's workload + - Department manager: can view workload of users in their department + - Regular user: can only view their own workload + Returns: - Workload summary (same as heatmap) - List of tasks contributing to the workload """ - # Check access - check_workload_access(current_user, target_user_id=user_id) + # Get target user's department for manager access check + target_user = db.query(User).filter(User.id == user_id).first() + target_user_department_id = target_user.department_id if target_user else None + + # Check access (pass target user's department for manager check) + check_workload_access( + current_user, + target_user_id=user_id, + target_user_department_id=target_user_department_id + ) # Calculate workload detail detail = get_user_workload_detail(db, user_id, week_start) diff --git a/backend/app/core/config.py b/backend/app/core/config.py index 36f432e..7c51286 100644 --- a/backend/app/core/config.py +++ b/backend/app/core/config.py @@ -115,6 +115,13 @@ class Settings(BaseSettings): "exe", "bat", "cmd", "sh", "ps1", "dll", "msi", "com", "scr", "vbs", "js" ] + # Rate Limiting Configuration + # Tiers: standard, sensitive, heavy + # Format: "{requests}/{period}" (e.g., "60/minute", "20/minute", "5/minute") + RATE_LIMIT_STANDARD: str = "60/minute" # Task CRUD, comments + RATE_LIMIT_SENSITIVE: str = "20/minute" # Attachments, password change, report export + RATE_LIMIT_HEAVY: str = "5/minute" # Report generation, bulk operations + class Config: env_file = ".env" case_sensitive = True diff --git a/backend/app/core/database.py b/backend/app/core/database.py index f2c8b4c..392d072 100644 --- a/backend/app/core/database.py +++ b/backend/app/core/database.py @@ -1,19 +1,109 @@ -from sqlalchemy import create_engine +import logging +import threading +import os +from sqlalchemy import create_engine, event from sqlalchemy.ext.declarative import declarative_base from sqlalchemy.orm import sessionmaker from app.core.config import settings +logger = logging.getLogger(__name__) + +# Connection pool configuration with environment variable overrides +POOL_SIZE = int(os.getenv("DB_POOL_SIZE", "10")) +MAX_OVERFLOW = int(os.getenv("DB_MAX_OVERFLOW", "20")) +POOL_TIMEOUT = int(os.getenv("DB_POOL_TIMEOUT", "30")) +POOL_STATS_INTERVAL = int(os.getenv("DB_POOL_STATS_INTERVAL", "300")) # 5 minutes + engine = create_engine( settings.DATABASE_URL, pool_pre_ping=True, pool_recycle=3600, + pool_size=POOL_SIZE, + max_overflow=MAX_OVERFLOW, + pool_timeout=POOL_TIMEOUT, ) SessionLocal = sessionmaker(autocommit=False, autoflush=False, bind=engine) Base = declarative_base() +# Connection pool statistics tracking +_pool_stats_lock = threading.Lock() +_pool_stats = { + "checkouts": 0, + "checkins": 0, + "overflow_connections": 0, + "invalidated_connections": 0, +} + + +def _log_pool_statistics(): + """Log current connection pool statistics.""" + pool = engine.pool + with _pool_stats_lock: + logger.info( + "Database connection pool statistics: " + "size=%d, checked_in=%d, overflow=%d, " + "total_checkouts=%d, total_checkins=%d, invalidated=%d", + pool.size(), + pool.checkedin(), + pool.overflow(), + _pool_stats["checkouts"], + _pool_stats["checkins"], + _pool_stats["invalidated_connections"], + ) + + +def _start_pool_stats_logging(): + """Start periodic logging of connection pool statistics.""" + if POOL_STATS_INTERVAL <= 0: + return + + def log_stats(): + _log_pool_statistics() + # Schedule next log + timer = threading.Timer(POOL_STATS_INTERVAL, log_stats) + timer.daemon = True + timer.start() + + # Start the first timer + timer = threading.Timer(POOL_STATS_INTERVAL, log_stats) + timer.daemon = True + timer.start() + logger.info( + "Database connection pool initialized: pool_size=%d, max_overflow=%d, pool_timeout=%d, stats_interval=%ds", + POOL_SIZE, MAX_OVERFLOW, POOL_TIMEOUT, POOL_STATS_INTERVAL + ) + + +# Register pool event listeners for statistics +@event.listens_for(engine, "checkout") +def _on_checkout(dbapi_conn, connection_record, connection_proxy): + """Track connection checkout events.""" + with _pool_stats_lock: + _pool_stats["checkouts"] += 1 + + +@event.listens_for(engine, "checkin") +def _on_checkin(dbapi_conn, connection_record): + """Track connection checkin events.""" + with _pool_stats_lock: + _pool_stats["checkins"] += 1 + + +@event.listens_for(engine, "invalidate") +def _on_invalidate(dbapi_conn, connection_record, exception): + """Track connection invalidation events.""" + with _pool_stats_lock: + _pool_stats["invalidated_connections"] += 1 + if exception: + logger.warning("Database connection invalidated due to exception: %s", exception) + + +# Start pool statistics logging on module load +_start_pool_stats_logging() + def get_db(): """Dependency for getting database session.""" @@ -22,3 +112,18 @@ def get_db(): yield db finally: db.close() + + +def get_pool_status() -> dict: + """Get current connection pool status for health checks.""" + pool = engine.pool + with _pool_stats_lock: + return { + "pool_size": pool.size(), + "checked_in": pool.checkedin(), + "checked_out": pool.checkedout(), + "overflow": pool.overflow(), + "total_checkouts": _pool_stats["checkouts"], + "total_checkins": _pool_stats["checkins"], + "invalidated_connections": _pool_stats["invalidated_connections"], + } diff --git a/backend/app/core/deprecation.py b/backend/app/core/deprecation.py new file mode 100644 index 0000000..9e5c811 --- /dev/null +++ b/backend/app/core/deprecation.py @@ -0,0 +1,45 @@ +"""Deprecation middleware for legacy API routes. + +Provides middleware to add deprecation warning headers to legacy /api/ routes +during the transition to /api/v1/. +""" +import logging +from datetime import datetime +from starlette.middleware.base import BaseHTTPMiddleware +from starlette.requests import Request + +logger = logging.getLogger(__name__) + + +class DeprecationMiddleware(BaseHTTPMiddleware): + """Middleware to add deprecation headers to legacy API routes. + + This middleware checks if a request is using a legacy /api/ route + (instead of /api/v1/) and adds appropriate deprecation headers to + encourage migration to the new versioned API. + """ + + # Sunset date for legacy routes (6 months from now, adjust as needed) + SUNSET_DATE = "2026-07-01T00:00:00Z" + + async def dispatch(self, request: Request, call_next): + response = await call_next(request) + + # Check if this is a legacy /api/ route (not /api/v1/) + path = request.url.path + if path.startswith("/api/") and not path.startswith("/api/v1/"): + # Skip deprecation headers for health check endpoints + if path in ["/health", "/health/ready", "/health/live", "/health/detailed"]: + return response + + # Add deprecation headers (RFC 8594) + response.headers["Deprecation"] = "true" + response.headers["Sunset"] = self.SUNSET_DATE + response.headers["Link"] = f'; rel="successor-version"' + response.headers["X-Deprecation-Notice"] = ( + "This API endpoint is deprecated. " + "Please migrate to /api/v1/ prefix. " + f"This endpoint will be removed after {self.SUNSET_DATE}." + ) + + return response diff --git a/backend/app/core/rate_limiter.py b/backend/app/core/rate_limiter.py index 8e9a5cd..de48657 100644 --- a/backend/app/core/rate_limiter.py +++ b/backend/app/core/rate_limiter.py @@ -3,11 +3,19 @@ Rate limiting configuration using slowapi with Redis backend. This module provides rate limiting functionality to protect against brute force attacks and DoS attempts on sensitive endpoints. + +Rate Limit Tiers: +- standard: 60/minute - For normal CRUD operations (tasks, comments) +- sensitive: 20/minute - For sensitive operations (attachments, password change) +- heavy: 5/minute - For resource-intensive operations (reports, bulk operations) """ import logging import os +from functools import wraps +from typing import Callable, Optional +from fastapi import Request, Response from slowapi import Limiter from slowapi.util import get_remote_address @@ -60,8 +68,56 @@ _storage_uri = _get_storage_uri() # Create limiter instance with appropriate storage # Uses the client's remote address (IP) as the key for rate limiting +# Note: headers_enabled=False because slowapi's header injection requires Response objects, +# which conflicts with endpoints that return Pydantic models directly. +# Rate limit status can be checked via the 429 Too Many Requests response. limiter = Limiter( key_func=get_remote_address, storage_uri=_storage_uri, strategy="fixed-window", # Fixed window strategy for predictable rate limiting + headers_enabled=False, # Disabled due to compatibility issues with Pydantic model responses ) + + +# Convenience functions for rate limit tiers +def get_rate_limit_standard() -> str: + """Get the standard rate limit tier (60/minute by default).""" + return settings.RATE_LIMIT_STANDARD + + +def get_rate_limit_sensitive() -> str: + """Get the sensitive rate limit tier (20/minute by default).""" + return settings.RATE_LIMIT_SENSITIVE + + +def get_rate_limit_heavy() -> str: + """Get the heavy rate limit tier (5/minute by default).""" + return settings.RATE_LIMIT_HEAVY + + +# Pre-configured rate limit decorators for common use cases +def rate_limit_standard(func: Optional[Callable] = None): + """ + Apply standard rate limit (60/minute) for normal CRUD operations. + + Use for: Task creation/update, comment creation, etc. + """ + return limiter.limit(get_rate_limit_standard())(func) if func else limiter.limit(get_rate_limit_standard()) + + +def rate_limit_sensitive(func: Optional[Callable] = None): + """ + Apply sensitive rate limit (20/minute) for sensitive operations. + + Use for: File uploads, password changes, report exports, etc. + """ + return limiter.limit(get_rate_limit_sensitive())(func) if func else limiter.limit(get_rate_limit_sensitive()) + + +def rate_limit_heavy(func: Optional[Callable] = None): + """ + Apply heavy rate limit (5/minute) for resource-intensive operations. + + Use for: Report generation, bulk operations, data exports, etc. + """ + return limiter.limit(get_rate_limit_heavy())(func) if func else limiter.limit(get_rate_limit_heavy()) diff --git a/backend/app/core/response.py b/backend/app/core/response.py new file mode 100644 index 0000000..db6577a --- /dev/null +++ b/backend/app/core/response.py @@ -0,0 +1,178 @@ +"""Standardized API response wrapper. + +Provides utility classes and functions for consistent API response formatting +across all endpoints. +""" +from datetime import datetime +from typing import Any, Generic, Optional, TypeVar +from pydantic import BaseModel, Field + +T = TypeVar("T") + + +class ErrorDetail(BaseModel): + """Detailed error information.""" + error_code: str = Field(..., description="Machine-readable error code") + message: str = Field(..., description="Human-readable error message") + field: Optional[str] = Field(None, description="Field that caused the error, if applicable") + details: Optional[dict] = Field(None, description="Additional error details") + + +class ApiResponse(BaseModel, Generic[T]): + """Standard API response wrapper. + + All API endpoints should return responses in this format for consistency. + + Attributes: + success: Whether the request was successful + data: The actual response data (null for errors) + message: Human-readable message about the result + timestamp: ISO 8601 timestamp of the response + error: Error details if success is False + """ + success: bool = Field(..., description="Whether the request was successful") + data: Optional[T] = Field(None, description="Response data") + message: Optional[str] = Field(None, description="Human-readable message") + timestamp: str = Field( + default_factory=lambda: datetime.utcnow().isoformat() + "Z", + description="ISO 8601 timestamp" + ) + error: Optional[ErrorDetail] = Field(None, description="Error details if failed") + + class Config: + from_attributes = True + + +class PaginatedData(BaseModel, Generic[T]): + """Paginated data structure.""" + items: list[T] = Field(default_factory=list, description="List of items") + total: int = Field(..., description="Total number of items") + page: int = Field(..., description="Current page number (1-indexed)") + page_size: int = Field(..., description="Number of items per page") + total_pages: int = Field(..., description="Total number of pages") + + class Config: + from_attributes = True + + +# Error codes for common scenarios +class ErrorCode: + """Standard error codes for API responses.""" + # Authentication & Authorization + UNAUTHORIZED = "AUTH_001" + FORBIDDEN = "AUTH_002" + TOKEN_EXPIRED = "AUTH_003" + INVALID_TOKEN = "AUTH_004" + + # Validation + VALIDATION_ERROR = "VAL_001" + INVALID_INPUT = "VAL_002" + MISSING_FIELD = "VAL_003" + INVALID_FORMAT = "VAL_004" + + # Resource + NOT_FOUND = "RES_001" + ALREADY_EXISTS = "RES_002" + CONFLICT = "RES_003" + DELETED = "RES_004" + + # Business Logic + BUSINESS_ERROR = "BIZ_001" + INVALID_STATE = "BIZ_002" + LIMIT_EXCEEDED = "BIZ_003" + DEPENDENCY_ERROR = "BIZ_004" + + # Server + INTERNAL_ERROR = "SRV_001" + DATABASE_ERROR = "SRV_002" + EXTERNAL_SERVICE_ERROR = "SRV_003" + RATE_LIMITED = "SRV_004" + + +def success_response( + data: Any = None, + message: Optional[str] = None, +) -> dict: + """Create a successful API response. + + Args: + data: The response data + message: Optional human-readable message + + Returns: + Dictionary with standard response structure + """ + return { + "success": True, + "data": data, + "message": message, + "timestamp": datetime.utcnow().isoformat() + "Z", + "error": None, + } + + +def error_response( + error_code: str, + message: str, + field: Optional[str] = None, + details: Optional[dict] = None, +) -> dict: + """Create an error API response. + + Args: + error_code: Machine-readable error code (use ErrorCode constants) + message: Human-readable error message + field: Optional field name that caused the error + details: Optional additional error details + + Returns: + Dictionary with standard error response structure + """ + return { + "success": False, + "data": None, + "message": message, + "timestamp": datetime.utcnow().isoformat() + "Z", + "error": { + "error_code": error_code, + "message": message, + "field": field, + "details": details, + }, + } + + +def paginated_response( + items: list, + total: int, + page: int, + page_size: int, + message: Optional[str] = None, +) -> dict: + """Create a paginated API response. + + Args: + items: List of items for current page + total: Total number of items across all pages + page: Current page number (1-indexed) + page_size: Number of items per page + message: Optional human-readable message + + Returns: + Dictionary with standard paginated response structure + """ + total_pages = (total + page_size - 1) // page_size if page_size > 0 else 0 + + return { + "success": True, + "data": { + "items": items, + "total": total, + "page": page, + "page_size": page_size, + "total_pages": total_pages, + }, + "message": message, + "timestamp": datetime.utcnow().isoformat() + "Z", + "error": None, + } diff --git a/backend/app/main.py b/backend/app/main.py index 054e85d..69d92bc 100644 --- a/backend/app/main.py +++ b/backend/app/main.py @@ -1,13 +1,16 @@ from contextlib import asynccontextmanager -from fastapi import FastAPI, Request +from datetime import datetime +from fastapi import FastAPI, Request, APIRouter from fastapi.middleware.cors import CORSMiddleware from fastapi.responses import JSONResponse from slowapi import _rate_limit_exceeded_handler from slowapi.errors import RateLimitExceeded +from sqlalchemy import text from app.middleware.audit import AuditMiddleware -from app.core.scheduler import start_scheduler, shutdown_scheduler +from app.core.scheduler import start_scheduler, shutdown_scheduler, scheduler from app.core.rate_limiter import limiter +from app.core.deprecation import DeprecationMiddleware @asynccontextmanager @@ -18,6 +21,8 @@ async def lifespan(app: FastAPI): yield # Shutdown shutdown_scheduler() + + from app.api.auth import router as auth_router from app.api.users import router as users_router from app.api.departments import router as departments_router @@ -38,12 +43,17 @@ from app.api.custom_fields import router as custom_fields_router from app.api.task_dependencies import router as task_dependencies_router from app.api.admin import encryption_keys as admin_encryption_keys_router from app.api.dashboard import router as dashboard_router +from app.api.templates import router as templates_router from app.core.config import settings +from app.core.database import get_pool_status, engine +from app.core.redis import redis_client +from app.services.notification_service import get_redis_fallback_status +from app.services.file_storage_service import file_storage_service app = FastAPI( title="Project Control API", description="Cross-departmental project management system API", - version="0.1.0", + version="1.0.0", lifespan=lifespan, ) @@ -63,13 +73,37 @@ app.add_middleware( # Audit middleware - extracts request metadata for audit logging app.add_middleware(AuditMiddleware) -# Include routers +# Deprecation middleware - adds deprecation headers to legacy /api/ routes +app.add_middleware(DeprecationMiddleware) + +# ============================================================================= +# API Version 1 Router - Primary API namespace +# ============================================================================= +api_v1_router = APIRouter(prefix="/api/v1") + +# Include routers under /api/v1/ +api_v1_router.include_router(auth_router.router, prefix="/auth", tags=["Authentication"]) +api_v1_router.include_router(users_router.router, prefix="/users", tags=["Users"]) +api_v1_router.include_router(departments_router.router, prefix="/departments", tags=["Departments"]) +api_v1_router.include_router(workload_router, prefix="/workload", tags=["Workload"]) +api_v1_router.include_router(dashboard_router, prefix="/dashboard", tags=["Dashboard"]) +api_v1_router.include_router(templates_router, tags=["Project Templates"]) + +# Mount the v1 router +app.include_router(api_v1_router) + +# ============================================================================= +# Legacy /api/ Routes (Deprecated - for backwards compatibility) +# ============================================================================= +# These routes will be removed in a future version. +# All new integrations should use /api/v1/ prefix. + app.include_router(auth_router.router, prefix="/api/auth", tags=["Authentication"]) app.include_router(users_router.router, prefix="/api/users", tags=["Users"]) app.include_router(departments_router.router, prefix="/api/departments", tags=["Departments"]) -app.include_router(spaces_router) -app.include_router(projects_router) -app.include_router(tasks_router) +app.include_router(spaces_router) # Has /api/spaces prefix in router +app.include_router(projects_router) # Has routes with /api prefix +app.include_router(tasks_router) # Has routes with /api prefix app.include_router(workload_router, prefix="/api/workload", tags=["Workload"]) app.include_router(comments_router) app.include_router(notifications_router) @@ -79,13 +113,176 @@ app.include_router(audit_router) app.include_router(attachments_router) app.include_router(triggers_router) app.include_router(reports_router) -app.include_router(health_router) +app.include_router(health_router) # Has /api/projects/health prefix app.include_router(custom_fields_router) app.include_router(task_dependencies_router) app.include_router(admin_encryption_keys_router.router) app.include_router(dashboard_router, prefix="/api/dashboard", tags=["Dashboard"]) +app.include_router(templates_router) # Has /api/templates prefix in router + + +# ============================================================================= +# Health Check Endpoints +# ============================================================================= + +def check_database_health() -> dict: + """Check database connectivity and return status.""" + try: + # Execute a simple query to verify connection + with engine.connect() as conn: + conn.execute(text("SELECT 1")) + pool_status = get_pool_status() + return { + "status": "healthy", + "connected": True, + **pool_status, + } + except Exception as e: + return { + "status": "unhealthy", + "connected": False, + "error": str(e), + } + + +def check_redis_health() -> dict: + """Check Redis connectivity and return status.""" + try: + # Ping Redis to verify connection + redis_client.ping() + redis_fallback = get_redis_fallback_status() + return { + "status": "healthy", + "connected": True, + **redis_fallback, + } + except Exception as e: + redis_fallback = get_redis_fallback_status() + return { + "status": "unhealthy", + "connected": False, + "error": str(e), + **redis_fallback, + } + + +def check_scheduler_health() -> dict: + """Check scheduler status and return details.""" + try: + running = scheduler.running + jobs = [] + if running: + for job in scheduler.get_jobs(): + jobs.append({ + "id": job.id, + "name": job.name, + "next_run": job.next_run_time.isoformat() if job.next_run_time else None, + }) + return { + "status": "healthy" if running else "stopped", + "running": running, + "jobs": jobs, + "job_count": len(jobs), + } + except Exception as e: + return { + "status": "unhealthy", + "running": False, + "error": str(e), + } @app.get("/health") async def health_check(): + """Basic health check endpoint for load balancers. + + Returns a simple healthy status if the application is running. + For detailed status, use /health/detailed. + """ return {"status": "healthy"} + + +@app.get("/health/live") +async def liveness_check(): + """Kubernetes liveness probe endpoint. + + Returns healthy if the application process is running. + Does not check external dependencies. + """ + return { + "status": "healthy", + "timestamp": datetime.utcnow().isoformat() + "Z", + } + + +@app.get("/health/ready") +async def readiness_check(): + """Kubernetes readiness probe endpoint. + + Returns healthy only if all critical dependencies are available. + The application should not receive traffic until ready. + """ + db_health = check_database_health() + redis_health = check_redis_health() + + # Application is ready only if database is healthy + # Redis degradation is acceptable (we have fallback) + is_ready = db_health["status"] == "healthy" + + return { + "status": "ready" if is_ready else "not_ready", + "timestamp": datetime.utcnow().isoformat() + "Z", + "checks": { + "database": db_health["status"], + "redis": redis_health["status"], + }, + } + + +@app.get("/health/detailed") +async def detailed_health_check(): + """Detailed health check endpoint. + + Returns comprehensive status of all system components: + - database: Connection pool status and connectivity + - redis: Connection status and fallback queue status + - storage: File storage validation status + - scheduler: Background job scheduler status + """ + db_health = check_database_health() + redis_health = check_redis_health() + scheduler_health = check_scheduler_health() + storage_status = file_storage_service.get_storage_status() + + # Determine overall health + is_healthy = ( + db_health["status"] == "healthy" and + storage_status.get("validated", False) + ) + + # Degraded if Redis or scheduler has issues but DB is ok + is_degraded = ( + is_healthy and ( + redis_health["status"] != "healthy" or + scheduler_health["status"] != "healthy" + ) + ) + + overall_status = "unhealthy" + if is_healthy: + overall_status = "degraded" if is_degraded else "healthy" + + return { + "status": overall_status, + "timestamp": datetime.utcnow().isoformat() + "Z", + "version": app.version, + "components": { + "database": db_health, + "redis": redis_health, + "scheduler": scheduler_health, + "storage": { + "status": "healthy" if storage_status.get("validated", False) else "unhealthy", + **storage_status, + }, + }, + } diff --git a/backend/app/middleware/auth.py b/backend/app/middleware/auth.py index 6167e90..470ad0b 100644 --- a/backend/app/middleware/auth.py +++ b/backend/app/middleware/auth.py @@ -207,12 +207,16 @@ def check_space_edit_access(user: User, space) -> bool: def check_project_access(user: User, project) -> bool: """ - Check if user has access to a project based on security level. + Check if user has access to a project based on security level and membership. - Security Levels: - - public: All logged-in users - - department: Same department users + project owner - - confidential: Only project owner (+ system admin) + Access is granted if any of the following conditions are met: + 1. User is a system admin + 2. User is the project owner + 3. User is an explicit project member (cross-department collaboration) + 4. Security level allows access: + - public: All logged-in users + - department: Same department users + - confidential: Only owner/members/admin """ # System admin bypasses all restrictions if user.is_system_admin: @@ -222,6 +226,13 @@ def check_project_access(user: User, project) -> bool: if project.owner_id == user.id: return True + # Check if user is an explicit project member (for cross-department collaboration) + # This allows users from other departments to access the project + if hasattr(project, 'members') and project.members: + for member in project.members: + if member.user_id == user.id: + return True + # Check by security level security_level = project.security_level @@ -235,20 +246,34 @@ def check_project_access(user: User, project) -> bool: return False else: # confidential - # Only owner has access (already checked above) + # Only owner/members have access (already checked above) return False def check_project_edit_access(user: User, project) -> bool: """ Check if user can edit/delete a project. + + Edit access is granted if: + 1. User is a system admin + 2. User is the project owner + 3. User is a project member with 'admin' role """ # System admin has full access if user.is_system_admin: return True - # Only owner can edit - return project.owner_id == user.id + # Owner can edit + if project.owner_id == user.id: + return True + + # Project member with admin role can edit + if hasattr(project, 'members') and project.members: + for member in project.members: + if member.user_id == user.id and member.role == "admin": + return True + + return False def check_task_access(user: User, task, project) -> bool: diff --git a/backend/app/models/__init__.py b/backend/app/models/__init__.py index b66a76f..40895a4 100644 --- a/backend/app/models/__init__.py +++ b/backend/app/models/__init__.py @@ -23,6 +23,8 @@ from app.models.project_health import ProjectHealth, RiskLevel, ScheduleStatus, from app.models.custom_field import CustomField, FieldType from app.models.task_custom_value import TaskCustomValue from app.models.task_dependency import TaskDependency, DependencyType +from app.models.project_member import ProjectMember +from app.models.project_template import ProjectTemplate __all__ = [ "User", "Role", "Department", "Space", "Project", "TaskStatus", "Task", "WorkloadSnapshot", @@ -33,5 +35,7 @@ __all__ = [ "ScheduledReport", "ReportType", "ReportHistory", "ReportHistoryStatus", "ProjectHealth", "RiskLevel", "ScheduleStatus", "ResourceStatus", "CustomField", "FieldType", "TaskCustomValue", - "TaskDependency", "DependencyType" + "TaskDependency", "DependencyType", + "ProjectMember", + "ProjectTemplate" ] diff --git a/backend/app/models/project.py b/backend/app/models/project.py index b53a778..c27b7d7 100644 --- a/backend/app/models/project.py +++ b/backend/app/models/project.py @@ -42,3 +42,6 @@ class Project(Base): triggers = relationship("Trigger", back_populates="project", cascade="all, delete-orphan") health = relationship("ProjectHealth", back_populates="project", uselist=False, cascade="all, delete-orphan") custom_fields = relationship("CustomField", back_populates="project", cascade="all, delete-orphan") + + # Project membership for cross-department collaboration + members = relationship("ProjectMember", back_populates="project", cascade="all, delete-orphan") diff --git a/backend/app/models/project_member.py b/backend/app/models/project_member.py new file mode 100644 index 0000000..e0964d4 --- /dev/null +++ b/backend/app/models/project_member.py @@ -0,0 +1,56 @@ +"""ProjectMember model for cross-department project collaboration. + +This model tracks explicit project membership, allowing users from different +departments to be granted access to projects they wouldn't normally have +access to based on department isolation rules. +""" +import uuid +from sqlalchemy import Column, String, ForeignKey, DateTime, UniqueConstraint +from sqlalchemy.sql import func +from sqlalchemy.orm import relationship +from app.core.database import Base + + +class ProjectMember(Base): + """ + Represents a user's membership in a project. + + This enables cross-department collaboration by explicitly granting + project access to users regardless of their department. + + Roles: + - member: Can view and edit tasks + - admin: Can manage project settings and add other members + """ + __tablename__ = "pjctrl_project_members" + + id = Column(String(36), primary_key=True, default=lambda: str(uuid.uuid4())) + project_id = Column( + String(36), + ForeignKey("pjctrl_projects.id", ondelete="CASCADE"), + nullable=False, + index=True + ) + user_id = Column( + String(36), + ForeignKey("pjctrl_users.id", ondelete="CASCADE"), + nullable=False, + index=True + ) + role = Column(String(50), nullable=False, default="member") + added_by = Column( + String(36), + ForeignKey("pjctrl_users.id"), + nullable=False + ) + created_at = Column(DateTime, server_default=func.now(), nullable=False) + + # Unique constraint to prevent duplicate memberships + __table_args__ = ( + UniqueConstraint('project_id', 'user_id', name='uq_project_member'), + ) + + # Relationships + project = relationship("Project", back_populates="members") + user = relationship("User", foreign_keys=[user_id], back_populates="project_memberships") + added_by_user = relationship("User", foreign_keys=[added_by]) diff --git a/backend/app/models/project_template.py b/backend/app/models/project_template.py new file mode 100644 index 0000000..80b370a --- /dev/null +++ b/backend/app/models/project_template.py @@ -0,0 +1,125 @@ +"""Project Template model for reusable project configurations. + +Allows users to create templates with predefined task statuses and custom fields +that can be used to quickly set up new projects. +""" +import uuid +from sqlalchemy import Column, String, Text, Boolean, DateTime, ForeignKey, JSON +from sqlalchemy.orm import relationship +from sqlalchemy.sql import func +from app.core.database import Base + + +class ProjectTemplate(Base): + """Template for creating projects with predefined configurations. + + A template stores: + - Basic project metadata (name, description) + - Predefined task statuses (stored as JSON) + - Predefined custom field definitions (stored as JSON) + + When a project is created from a template, the TaskStatus and CustomField + records are copied to the new project. + """ + __tablename__ = "pjctrl_project_templates" + + id = Column(String(36), primary_key=True, default=lambda: str(uuid.uuid4())) + name = Column(String(200), nullable=False) + description = Column(Text, nullable=True) + + # Template owner + owner_id = Column(String(36), ForeignKey("pjctrl_users.id"), nullable=False) + + # Whether the template is available to all users or just the owner + is_public = Column(Boolean, default=False, nullable=False) + + # Soft delete flag + is_active = Column(Boolean, default=True, nullable=False) + + # Predefined task statuses as JSON array + # Format: [{"name": "To Do", "color": "#808080", "position": 0, "is_done": false}, ...] + task_statuses = Column(JSON, nullable=True) + + # Predefined custom field definitions as JSON array + # Format: [{"name": "Priority", "field_type": "dropdown", "options": [...], ...}, ...] + custom_fields = Column(JSON, nullable=True) + + # Optional default project settings + default_security_level = Column(String(20), default="department", nullable=True) + + created_at = Column(DateTime, server_default=func.now(), nullable=False) + updated_at = Column(DateTime, server_default=func.now(), onupdate=func.now(), nullable=False) + + # Relationships + owner = relationship("User", foreign_keys=[owner_id]) + + +# Default template data for system templates +SYSTEM_TEMPLATES = [ + { + "name": "Basic Project", + "description": "A simple project template with standard task statuses.", + "is_public": True, + "task_statuses": [ + {"name": "To Do", "color": "#808080", "position": 0, "is_done": False}, + {"name": "In Progress", "color": "#0066cc", "position": 1, "is_done": False}, + {"name": "Done", "color": "#00cc66", "position": 2, "is_done": True}, + ], + "custom_fields": [], + }, + { + "name": "Software Development", + "description": "Template for software development projects with extended workflow.", + "is_public": True, + "task_statuses": [ + {"name": "Backlog", "color": "#808080", "position": 0, "is_done": False}, + {"name": "To Do", "color": "#3366cc", "position": 1, "is_done": False}, + {"name": "In Progress", "color": "#0066cc", "position": 2, "is_done": False}, + {"name": "Code Review", "color": "#cc6600", "position": 3, "is_done": False}, + {"name": "Testing", "color": "#9933cc", "position": 4, "is_done": False}, + {"name": "Done", "color": "#00cc66", "position": 5, "is_done": True}, + ], + "custom_fields": [ + { + "name": "Story Points", + "field_type": "number", + "is_required": False, + "position": 0, + }, + { + "name": "Sprint", + "field_type": "dropdown", + "options": ["Sprint 1", "Sprint 2", "Sprint 3", "Backlog"], + "is_required": False, + "position": 1, + }, + ], + }, + { + "name": "Marketing Campaign", + "description": "Template for marketing campaign management.", + "is_public": True, + "task_statuses": [ + {"name": "Planning", "color": "#808080", "position": 0, "is_done": False}, + {"name": "Content Creation", "color": "#cc6600", "position": 1, "is_done": False}, + {"name": "Review", "color": "#9933cc", "position": 2, "is_done": False}, + {"name": "Scheduled", "color": "#0066cc", "position": 3, "is_done": False}, + {"name": "Published", "color": "#00cc66", "position": 4, "is_done": True}, + ], + "custom_fields": [ + { + "name": "Channel", + "field_type": "dropdown", + "options": ["Email", "Social Media", "Website", "Print", "Event"], + "is_required": False, + "position": 0, + }, + { + "name": "Target Audience", + "field_type": "text", + "is_required": False, + "position": 1, + }, + ], + }, +] diff --git a/backend/app/models/task.py b/backend/app/models/task.py index 05e5d71..34fdd92 100644 --- a/backend/app/models/task.py +++ b/backend/app/models/task.py @@ -37,6 +37,9 @@ class Task(Base): created_at = Column(DateTime, server_default=func.now(), nullable=False) updated_at = Column(DateTime, server_default=func.now(), onupdate=func.now(), nullable=False) + # Optimistic locking field + version = Column(Integer, default=1, nullable=False) + # Soft delete fields is_deleted = Column(Boolean, default=False, nullable=False, index=True) deleted_at = Column(DateTime, nullable=True) diff --git a/backend/app/models/user.py b/backend/app/models/user.py index b18e3d2..bebf621 100644 --- a/backend/app/models/user.py +++ b/backend/app/models/user.py @@ -18,6 +18,7 @@ class User(Base): capacity = Column(Numeric(5, 2), default=40.00) is_active = Column(Boolean, default=True) is_system_admin = Column(Boolean, default=False) + is_department_manager = Column(Boolean, default=False) created_at = Column(DateTime, server_default=func.now()) updated_at = Column(DateTime, server_default=func.now(), onupdate=func.now()) @@ -41,3 +42,11 @@ class User(Base): # Automation relationships created_triggers = relationship("Trigger", back_populates="creator") scheduled_reports = relationship("ScheduledReport", back_populates="recipient", cascade="all, delete-orphan") + + # Project membership relationships (for cross-department collaboration) + project_memberships = relationship( + "ProjectMember", + foreign_keys="ProjectMember.user_id", + back_populates="user", + cascade="all, delete-orphan" + ) diff --git a/backend/app/schemas/auth.py b/backend/app/schemas/auth.py index 68fc287..d0bb326 100644 --- a/backend/app/schemas/auth.py +++ b/backend/app/schemas/auth.py @@ -1,10 +1,10 @@ -from pydantic import BaseModel +from pydantic import BaseModel, Field from typing import Optional class LoginRequest(BaseModel): - email: str - password: str + email: str = Field(..., max_length=255) + password: str = Field(..., min_length=1, max_length=128) class LoginResponse(BaseModel): diff --git a/backend/app/schemas/department.py b/backend/app/schemas/department.py index 7ce4361..48b55d6 100644 --- a/backend/app/schemas/department.py +++ b/backend/app/schemas/department.py @@ -1,10 +1,10 @@ -from pydantic import BaseModel +from pydantic import BaseModel, Field from typing import Optional from datetime import datetime class DepartmentBase(BaseModel): - name: str + name: str = Field(..., min_length=1, max_length=200) parent_id: Optional[str] = None @@ -13,7 +13,7 @@ class DepartmentCreate(DepartmentBase): class DepartmentUpdate(BaseModel): - name: Optional[str] = None + name: Optional[str] = Field(None, min_length=1, max_length=200) parent_id: Optional[str] = None diff --git a/backend/app/schemas/project.py b/backend/app/schemas/project.py index 685f68a..6d46184 100644 --- a/backend/app/schemas/project.py +++ b/backend/app/schemas/project.py @@ -1,4 +1,4 @@ -from pydantic import BaseModel +from pydantic import BaseModel, Field from typing import Optional from datetime import datetime, date from decimal import Decimal @@ -12,9 +12,9 @@ class SecurityLevel(str, Enum): class ProjectBase(BaseModel): - title: str - description: Optional[str] = None - budget: Optional[Decimal] = None + title: str = Field(..., min_length=1, max_length=500) + description: Optional[str] = Field(None, max_length=10000) + budget: Optional[Decimal] = Field(None, ge=0, le=99999999999) start_date: Optional[date] = None end_date: Optional[date] = None security_level: SecurityLevel = SecurityLevel.DEPARTMENT @@ -25,13 +25,13 @@ class ProjectCreate(ProjectBase): class ProjectUpdate(BaseModel): - title: Optional[str] = None - description: Optional[str] = None - budget: Optional[Decimal] = None + title: Optional[str] = Field(None, min_length=1, max_length=500) + description: Optional[str] = Field(None, max_length=10000) + budget: Optional[Decimal] = Field(None, ge=0, le=99999999999) start_date: Optional[date] = None end_date: Optional[date] = None security_level: Optional[SecurityLevel] = None - status: Optional[str] = None + status: Optional[str] = Field(None, max_length=50) department_id: Optional[str] = None diff --git a/backend/app/schemas/project_member.py b/backend/app/schemas/project_member.py new file mode 100644 index 0000000..3a0b15f --- /dev/null +++ b/backend/app/schemas/project_member.py @@ -0,0 +1,56 @@ +"""Project member schemas for cross-department collaboration.""" +from pydantic import BaseModel, Field +from typing import Optional, List +from datetime import datetime +from enum import Enum + + +class ProjectMemberRole(str, Enum): + """Roles that can be assigned to project members.""" + MEMBER = "member" + ADMIN = "admin" + + +class ProjectMemberBase(BaseModel): + """Base schema for project member.""" + user_id: str = Field(..., description="ID of the user to add as project member") + role: ProjectMemberRole = Field( + default=ProjectMemberRole.MEMBER, + description="Role of the member: 'member' (view/edit tasks) or 'admin' (manage project)" + ) + + +class ProjectMemberCreate(ProjectMemberBase): + """Schema for creating a project member.""" + pass + + +class ProjectMemberUpdate(BaseModel): + """Schema for updating a project member.""" + role: ProjectMemberRole = Field(..., description="New role for the member") + + +class ProjectMemberResponse(ProjectMemberBase): + """Schema for project member response.""" + id: str + project_id: str + added_by: str + created_at: datetime + + class Config: + from_attributes = True + + +class ProjectMemberWithDetails(ProjectMemberResponse): + """Schema for project member with user details.""" + user_name: Optional[str] = None + user_email: Optional[str] = None + user_department_id: Optional[str] = None + user_department_name: Optional[str] = None + added_by_name: Optional[str] = None + + +class ProjectMemberListResponse(BaseModel): + """Schema for listing project members.""" + members: List[ProjectMemberWithDetails] + total: int diff --git a/backend/app/schemas/project_template.py b/backend/app/schemas/project_template.py new file mode 100644 index 0000000..d01a4c5 --- /dev/null +++ b/backend/app/schemas/project_template.py @@ -0,0 +1,95 @@ +"""Schemas for project template API endpoints.""" +from typing import Optional, List, Any +from datetime import datetime +from pydantic import BaseModel, Field + + +class TaskStatusDefinition(BaseModel): + """Task status definition for templates.""" + name: str = Field(..., min_length=1, max_length=50) + color: str = Field(default="#808080", pattern=r"^#[0-9A-Fa-f]{6}$") + position: int = Field(default=0, ge=0) + is_done: bool = Field(default=False) + + +class CustomFieldDefinition(BaseModel): + """Custom field definition for templates.""" + name: str = Field(..., min_length=1, max_length=100) + field_type: str = Field(..., pattern=r"^(text|number|dropdown|date|person|formula)$") + options: Optional[List[str]] = None + formula: Optional[str] = None + is_required: bool = Field(default=False) + position: int = Field(default=0, ge=0) + + +class ProjectTemplateBase(BaseModel): + """Base schema for project template.""" + name: str = Field(..., min_length=1, max_length=200) + description: Optional[str] = None + is_public: bool = Field(default=False) + task_statuses: Optional[List[TaskStatusDefinition]] = None + custom_fields: Optional[List[CustomFieldDefinition]] = None + default_security_level: Optional[str] = Field( + default="department", + pattern=r"^(public|department|confidential)$" + ) + + +class ProjectTemplateCreate(ProjectTemplateBase): + """Schema for creating a project template.""" + pass + + +class ProjectTemplateUpdate(BaseModel): + """Schema for updating a project template.""" + name: Optional[str] = Field(None, min_length=1, max_length=200) + description: Optional[str] = None + is_public: Optional[bool] = None + task_statuses: Optional[List[TaskStatusDefinition]] = None + custom_fields: Optional[List[CustomFieldDefinition]] = None + default_security_level: Optional[str] = Field( + None, + pattern=r"^(public|department|confidential)$" + ) + + +class ProjectTemplateResponse(ProjectTemplateBase): + """Schema for project template response.""" + id: str + owner_id: str + is_active: bool + created_at: datetime + updated_at: datetime + + class Config: + from_attributes = True + + +class ProjectTemplateWithOwner(ProjectTemplateResponse): + """Project template response with owner details.""" + owner_name: Optional[str] = None + + +class ProjectTemplateListResponse(BaseModel): + """Response schema for listing project templates.""" + templates: List[ProjectTemplateWithOwner] + total: int + + +class CreateProjectFromTemplateRequest(BaseModel): + """Request schema for creating a project from a template.""" + template_id: str + title: str = Field(..., min_length=1, max_length=500) + description: Optional[str] = Field(None, max_length=10000) + space_id: str + department_id: Optional[str] = None + + +class CreateProjectFromTemplateResponse(BaseModel): + """Response schema for project created from template.""" + id: str + title: str + template_id: str + template_name: str + task_statuses_created: int + custom_fields_created: int diff --git a/backend/app/schemas/space.py b/backend/app/schemas/space.py index a7033b1..7702e06 100644 --- a/backend/app/schemas/space.py +++ b/backend/app/schemas/space.py @@ -1,11 +1,11 @@ -from pydantic import BaseModel +from pydantic import BaseModel, Field from typing import Optional from datetime import datetime class SpaceBase(BaseModel): - name: str - description: Optional[str] = None + name: str = Field(..., min_length=1, max_length=200) + description: Optional[str] = Field(None, max_length=2000) class SpaceCreate(SpaceBase): @@ -13,8 +13,8 @@ class SpaceCreate(SpaceBase): class SpaceUpdate(BaseModel): - name: Optional[str] = None - description: Optional[str] = None + name: Optional[str] = Field(None, min_length=1, max_length=200) + description: Optional[str] = Field(None, max_length=2000) class SpaceResponse(SpaceBase): diff --git a/backend/app/schemas/task.py b/backend/app/schemas/task.py index c58cce4..3cb361e 100644 --- a/backend/app/schemas/task.py +++ b/backend/app/schemas/task.py @@ -1,4 +1,4 @@ -from pydantic import BaseModel, computed_field +from pydantic import BaseModel, computed_field, Field, field_validator from typing import Optional, List, Any, Dict from datetime import datetime from decimal import Decimal @@ -28,10 +28,10 @@ class CustomValueResponse(BaseModel): class TaskBase(BaseModel): - title: str - description: Optional[str] = None + title: str = Field(..., min_length=1, max_length=500) + description: Optional[str] = Field(None, max_length=10000) priority: Priority = Priority.MEDIUM - original_estimate: Optional[Decimal] = None + original_estimate: Optional[Decimal] = Field(None, ge=0, le=99999) start_date: Optional[datetime] = None due_date: Optional[datetime] = None @@ -44,17 +44,18 @@ class TaskCreate(TaskBase): class TaskUpdate(BaseModel): - title: Optional[str] = None - description: Optional[str] = None + title: Optional[str] = Field(None, min_length=1, max_length=500) + description: Optional[str] = Field(None, max_length=10000) priority: Optional[Priority] = None status_id: Optional[str] = None assignee_id: Optional[str] = None - original_estimate: Optional[Decimal] = None - time_spent: Optional[Decimal] = None + original_estimate: Optional[Decimal] = Field(None, ge=0, le=99999) + time_spent: Optional[Decimal] = Field(None, ge=0, le=99999) start_date: Optional[datetime] = None due_date: Optional[datetime] = None - position: Optional[int] = None + position: Optional[int] = Field(None, ge=0) custom_values: Optional[List[CustomValueInput]] = None + version: Optional[int] = Field(None, ge=1, description="Version for optimistic locking") class TaskStatusUpdate(BaseModel): @@ -77,6 +78,7 @@ class TaskResponse(TaskBase): created_by: str created_at: datetime updated_at: datetime + version: int = 1 # Optimistic locking version class Config: from_attributes = True @@ -100,3 +102,32 @@ class TaskWithDetails(TaskResponse): class TaskListResponse(BaseModel): tasks: List[TaskWithDetails] total: int + + +class TaskRestoreRequest(BaseModel): + """Request body for restoring a soft-deleted task.""" + cascade: bool = Field( + default=True, + description="If True, also restore child tasks deleted at the same time. If False, restore only the parent task." + ) + + +class TaskRestoreResponse(BaseModel): + """Response for task restore operation.""" + restored_task: TaskResponse + restored_children_count: int = 0 + restored_children_ids: List[str] = [] + + +class TaskDeleteWarningResponse(BaseModel): + """Response when task has unresolved blockers and force_delete is False.""" + warning: str + blocker_count: int + message: str = "Task has unresolved blockers. Use force_delete=true to delete anyway." + + +class TaskDeleteResponse(BaseModel): + """Response for task delete operation.""" + task: TaskResponse + blockers_resolved: int = 0 + force_deleted: bool = False diff --git a/backend/app/schemas/task_dependency.py b/backend/app/schemas/task_dependency.py index 1559fef..b9171ce 100644 --- a/backend/app/schemas/task_dependency.py +++ b/backend/app/schemas/task_dependency.py @@ -76,3 +76,46 @@ class DependencyValidationError(BaseModel): error_type: str # 'circular', 'self_reference', 'duplicate', 'cross_project' message: str details: Optional[dict] = None + + +class BulkDependencyItem(BaseModel): + """Single dependency item for bulk operations.""" + predecessor_id: str + successor_id: str + dependency_type: DependencyType = DependencyType.FS + lag_days: int = 0 + + @field_validator('lag_days') + @classmethod + def validate_lag_days(cls, v): + if v < -365 or v > 365: + raise ValueError('lag_days must be between -365 and 365') + return v + + +class BulkDependencyCreate(BaseModel): + """Schema for creating multiple dependencies at once.""" + dependencies: List[BulkDependencyItem] + + @field_validator('dependencies') + @classmethod + def validate_dependencies(cls, v): + if not v: + raise ValueError('At least one dependency is required') + if len(v) > 50: + raise ValueError('Cannot create more than 50 dependencies at once') + return v + + +class BulkDependencyValidationResult(BaseModel): + """Result of bulk dependency validation.""" + valid: bool + errors: List[dict] = [] + + +class BulkDependencyCreateResponse(BaseModel): + """Response for bulk dependency creation.""" + created: List[TaskDependencyResponse] + failed: List[dict] = [] + total_created: int + total_failed: int diff --git a/backend/app/schemas/task_status.py b/backend/app/schemas/task_status.py index e5a93e5..121edd2 100644 --- a/backend/app/schemas/task_status.py +++ b/backend/app/schemas/task_status.py @@ -1,12 +1,12 @@ -from pydantic import BaseModel +from pydantic import BaseModel, Field from typing import Optional from datetime import datetime class TaskStatusBase(BaseModel): - name: str - color: str = "#808080" - position: int = 0 + name: str = Field(..., min_length=1, max_length=100) + color: str = Field("#808080", max_length=20) + position: int = Field(0, ge=0) is_done: bool = False @@ -15,9 +15,9 @@ class TaskStatusCreate(TaskStatusBase): class TaskStatusUpdate(BaseModel): - name: Optional[str] = None - color: Optional[str] = None - position: Optional[int] = None + name: Optional[str] = Field(None, min_length=1, max_length=100) + color: Optional[str] = Field(None, max_length=20) + position: Optional[int] = Field(None, ge=0) is_done: Optional[bool] = None diff --git a/backend/app/schemas/user.py b/backend/app/schemas/user.py index 93b89e2..9a8a9b9 100644 --- a/backend/app/schemas/user.py +++ b/backend/app/schemas/user.py @@ -1,16 +1,16 @@ -from pydantic import BaseModel, field_validator +from pydantic import BaseModel, Field, field_validator from typing import Optional, List from datetime import datetime from decimal import Decimal class UserBase(BaseModel): - email: str - name: str + email: str = Field(..., max_length=255) + name: str = Field(..., min_length=1, max_length=200) department_id: Optional[str] = None role_id: Optional[str] = None skills: Optional[List[str]] = None - capacity: Optional[Decimal] = Decimal("40.00") + capacity: Optional[Decimal] = Field(Decimal("40.00"), ge=0, le=168) class UserCreate(UserBase): @@ -18,11 +18,11 @@ class UserCreate(UserBase): class UserUpdate(BaseModel): - name: Optional[str] = None + name: Optional[str] = Field(None, min_length=1, max_length=200) department_id: Optional[str] = None role_id: Optional[str] = None skills: Optional[List[str]] = None - capacity: Optional[Decimal] = None + capacity: Optional[Decimal] = Field(None, ge=0, le=168) is_active: Optional[bool] = None diff --git a/backend/app/services/dependency_service.py b/backend/app/services/dependency_service.py index c9a3627..cdd960d 100644 --- a/backend/app/services/dependency_service.py +++ b/backend/app/services/dependency_service.py @@ -6,6 +6,7 @@ Handles task dependency validation including: - Date constraint validation based on dependency types - Self-reference prevention - Cross-project dependency prevention +- Bulk dependency operations with cycle detection """ from typing import List, Optional, Set, Tuple, Dict, Any from collections import defaultdict @@ -25,6 +26,27 @@ class DependencyValidationError(Exception): super().__init__(message) +class CycleDetectionResult: + """Result of cycle detection with detailed path information.""" + + def __init__( + self, + has_cycle: bool, + cycle_path: Optional[List[str]] = None, + cycle_task_titles: Optional[List[str]] = None + ): + self.has_cycle = has_cycle + self.cycle_path = cycle_path or [] + self.cycle_task_titles = cycle_task_titles or [] + + def get_cycle_description(self) -> str: + """Get a human-readable description of the cycle.""" + if not self.has_cycle or not self.cycle_task_titles: + return "" + # Format: Task A -> Task B -> Task C -> Task A + return " -> ".join(self.cycle_task_titles) + + class DependencyService: """Service for managing task dependencies with validation.""" @@ -53,9 +75,36 @@ class DependencyService: Returns: List of task IDs forming the cycle if circular, None otherwise """ - # If adding predecessor -> successor, check if successor can reach predecessor - # This would mean predecessor depends (transitively) on successor, creating a cycle + result = DependencyService.detect_circular_dependency_detailed( + db, predecessor_id, successor_id, project_id + ) + return result.cycle_path if result.has_cycle else None + @staticmethod + def detect_circular_dependency_detailed( + db: Session, + predecessor_id: str, + successor_id: str, + project_id: str, + additional_edges: Optional[List[Tuple[str, str]]] = None + ) -> CycleDetectionResult: + """ + Detect if adding a dependency would create a circular reference. + + Uses DFS to traverse from the successor to check if we can reach + the predecessor through existing dependencies. + + Args: + db: Database session + predecessor_id: The task that must complete first + successor_id: The task that depends on the predecessor + project_id: Project ID to scope the query + additional_edges: Optional list of additional (predecessor_id, successor_id) + edges to consider (for bulk operations) + + Returns: + CycleDetectionResult with detailed cycle information + """ # Build adjacency list for the project's dependencies dependencies = db.query(TaskDependency).join( Task, TaskDependency.successor_id == Task.id @@ -71,6 +120,20 @@ class DependencyService: # Simulate adding the new edge graph[successor_id].append(predecessor_id) + # Add any additional edges for bulk operations + if additional_edges: + for pred_id, succ_id in additional_edges: + graph[succ_id].append(pred_id) + + # Build task title map for readable error messages + task_ids_in_graph = set() + for succ_id, pred_ids in graph.items(): + task_ids_in_graph.add(succ_id) + task_ids_in_graph.update(pred_ids) + + tasks = db.query(Task).filter(Task.id.in_(task_ids_in_graph)).all() + task_title_map: Dict[str, str] = {t.id: t.title for t in tasks} + # DFS to find if there's a path from predecessor back to successor # (which would complete a cycle) visited: Set[str] = set() @@ -101,7 +164,18 @@ class DependencyService: return None # Start DFS from the successor to check if we can reach back to it - return dfs(successor_id) + cycle_path = dfs(successor_id) + + if cycle_path: + # Build task titles for the cycle + cycle_titles = [task_title_map.get(task_id, task_id) for task_id in cycle_path] + return CycleDetectionResult( + has_cycle=True, + cycle_path=cycle_path, + cycle_task_titles=cycle_titles + ) + + return CycleDetectionResult(has_cycle=False) @staticmethod def validate_dependency( @@ -183,15 +257,19 @@ class DependencyService: ) # Check circular dependency - cycle = DependencyService.detect_circular_dependency( + cycle_result = DependencyService.detect_circular_dependency_detailed( db, predecessor_id, successor_id, predecessor.project_id ) - if cycle: + if cycle_result.has_cycle: raise DependencyValidationError( error_type="circular", - message="Adding this dependency would create a circular reference", - details={"cycle": cycle} + message=f"Adding this dependency would create a circular reference: {cycle_result.get_cycle_description()}", + details={ + "cycle": cycle_result.cycle_path, + "cycle_description": cycle_result.get_cycle_description(), + "cycle_task_titles": cycle_result.cycle_task_titles + } ) @staticmethod @@ -422,3 +500,202 @@ class DependencyService: queue.append(dep.successor_id) return successors + + @staticmethod + def validate_bulk_dependencies( + db: Session, + dependencies: List[Tuple[str, str]], + project_id: str + ) -> List[Dict[str, Any]]: + """ + Validate a batch of dependencies for cycle detection. + + This method validates multiple dependencies together to detect cycles + that would only appear when all dependencies are added together. + + Args: + db: Database session + dependencies: List of (predecessor_id, successor_id) tuples + project_id: Project ID to scope the query + + Returns: + List of validation errors (empty if all valid) + """ + errors: List[Dict[str, Any]] = [] + + if not dependencies: + return errors + + # First, validate each dependency individually for basic checks + for predecessor_id, successor_id in dependencies: + # Check self-reference + if predecessor_id == successor_id: + errors.append({ + "error_type": "self_reference", + "predecessor_id": predecessor_id, + "successor_id": successor_id, + "message": "A task cannot depend on itself" + }) + continue + + # Get tasks to validate project membership + predecessor = db.query(Task).filter(Task.id == predecessor_id).first() + successor = db.query(Task).filter(Task.id == successor_id).first() + + if not predecessor: + errors.append({ + "error_type": "not_found", + "predecessor_id": predecessor_id, + "successor_id": successor_id, + "message": f"Predecessor task not found: {predecessor_id}" + }) + continue + + if not successor: + errors.append({ + "error_type": "not_found", + "predecessor_id": predecessor_id, + "successor_id": successor_id, + "message": f"Successor task not found: {successor_id}" + }) + continue + + if predecessor.project_id != project_id or successor.project_id != project_id: + errors.append({ + "error_type": "cross_project", + "predecessor_id": predecessor_id, + "successor_id": successor_id, + "message": "All tasks must be in the same project" + }) + continue + + # Check for duplicates within the batch + existing = db.query(TaskDependency).filter( + TaskDependency.predecessor_id == predecessor_id, + TaskDependency.successor_id == successor_id + ).first() + + if existing: + errors.append({ + "error_type": "duplicate", + "predecessor_id": predecessor_id, + "successor_id": successor_id, + "message": "This dependency already exists" + }) + + # If there are basic validation errors, return them first + if errors: + return errors + + # Now check for cycles considering all dependencies together + # Build the graph incrementally and check for cycles + accumulated_edges: List[Tuple[str, str]] = [] + + for predecessor_id, successor_id in dependencies: + # Check if adding this edge (plus all previously accumulated edges) + # would create a cycle + cycle_result = DependencyService.detect_circular_dependency_detailed( + db, + predecessor_id, + successor_id, + project_id, + additional_edges=accumulated_edges + ) + + if cycle_result.has_cycle: + errors.append({ + "error_type": "circular", + "predecessor_id": predecessor_id, + "successor_id": successor_id, + "message": f"Adding this dependency would create a circular reference: {cycle_result.get_cycle_description()}", + "cycle": cycle_result.cycle_path, + "cycle_description": cycle_result.get_cycle_description(), + "cycle_task_titles": cycle_result.cycle_task_titles + }) + else: + # Add this edge to accumulated edges for subsequent checks + accumulated_edges.append((predecessor_id, successor_id)) + + return errors + + @staticmethod + def detect_cycles_in_graph( + db: Session, + project_id: str + ) -> List[CycleDetectionResult]: + """ + Detect all cycles in the existing dependency graph for a project. + + This is useful for auditing or cleanup operations. + + Args: + db: Database session + project_id: Project ID to check + + Returns: + List of CycleDetectionResult for each cycle found + """ + cycles: List[CycleDetectionResult] = [] + + # Get all dependencies for the project + dependencies = db.query(TaskDependency).join( + Task, TaskDependency.successor_id == Task.id + ).filter(Task.project_id == project_id).all() + + if not dependencies: + return cycles + + # Build the graph + graph: Dict[str, List[str]] = defaultdict(list) + for dep in dependencies: + graph[dep.successor_id].append(dep.predecessor_id) + + # Get task titles + task_ids = set() + for succ_id, pred_ids in graph.items(): + task_ids.add(succ_id) + task_ids.update(pred_ids) + + tasks = db.query(Task).filter(Task.id.in_(task_ids)).all() + task_title_map: Dict[str, str] = {t.id: t.title for t in tasks} + + # Find all cycles using DFS + visited: Set[str] = set() + found_cycles: Set[Tuple[str, ...]] = set() + + def find_cycles_dfs(node: str, path: List[str], in_path: Set[str]): + """DFS to find all cycles.""" + if node in in_path: + # Found a cycle + cycle_start = path.index(node) + cycle = tuple(sorted(path[cycle_start:])) # Normalize for dedup + if cycle not in found_cycles: + found_cycles.add(cycle) + actual_cycle = path[cycle_start:] + [node] + cycle_titles = [task_title_map.get(tid, tid) for tid in actual_cycle] + cycles.append(CycleDetectionResult( + has_cycle=True, + cycle_path=actual_cycle, + cycle_task_titles=cycle_titles + )) + return + + if node in visited: + return + + visited.add(node) + in_path.add(node) + path.append(node) + + for neighbor in graph.get(node, []): + find_cycles_dfs(neighbor, path.copy(), in_path.copy()) + + path.pop() + in_path.remove(node) + + # Start DFS from all nodes + for start_node in graph.keys(): + if start_node not in visited: + find_cycles_dfs(start_node, [], set()) + + return cycles diff --git a/backend/app/services/file_storage_service.py b/backend/app/services/file_storage_service.py index b666864..644c1be 100644 --- a/backend/app/services/file_storage_service.py +++ b/backend/app/services/file_storage_service.py @@ -1,26 +1,271 @@ import os import hashlib import shutil +import logging +import tempfile from pathlib import Path from typing import BinaryIO, Optional, Tuple from fastapi import UploadFile, HTTPException from app.core.config import settings +logger = logging.getLogger(__name__) + + +class PathTraversalError(Exception): + """Raised when a path traversal attempt is detected.""" + pass + + +class StorageValidationError(Exception): + """Raised when storage validation fails.""" + pass + + class FileStorageService: """Service for handling file storage operations.""" + # Common NAS mount points to detect + NAS_MOUNT_INDICATORS = [ + "/mnt/", "/mount/", "/nas/", "/nfs/", "/smb/", "/cifs/", + "/Volumes/", "/media/", "/srv/", "/storage/" + ] + def __init__(self): - self.base_dir = Path(settings.UPLOAD_DIR) - self._ensure_base_dir() + self.base_dir = Path(settings.UPLOAD_DIR).resolve() + self._storage_status = { + "validated": False, + "path_exists": False, + "writable": False, + "is_nas": False, + "error": None, + } + self._validate_storage_on_init() + + def _validate_storage_on_init(self): + """Validate storage configuration on service initialization.""" + try: + # Step 1: Ensure directory exists + self._ensure_base_dir() + self._storage_status["path_exists"] = True + + # Step 2: Check write permissions + self._check_write_permissions() + self._storage_status["writable"] = True + + # Step 3: Check if using NAS + is_nas = self._detect_nas_storage() + self._storage_status["is_nas"] = is_nas + + if not is_nas: + logger.warning( + "Storage directory '%s' appears to be local storage, not NAS. " + "Consider configuring UPLOAD_DIR to a NAS mount point for production use.", + self.base_dir + ) + + self._storage_status["validated"] = True + logger.info( + "Storage validated successfully: path=%s, is_nas=%s", + self.base_dir, is_nas + ) + + except StorageValidationError as e: + self._storage_status["error"] = str(e) + logger.error("Storage validation failed: %s", e) + raise + except Exception as e: + self._storage_status["error"] = str(e) + logger.error("Unexpected error during storage validation: %s", e) + raise StorageValidationError(f"Storage validation failed: {e}") + + def _check_write_permissions(self): + """Check if the storage directory has write permissions.""" + test_file = self.base_dir / f".write_test_{os.getpid()}" + try: + # Try to create and write to a test file + test_file.write_text("write_test") + # Verify we can read it back + content = test_file.read_text() + if content != "write_test": + raise StorageValidationError( + f"Write verification failed for directory: {self.base_dir}" + ) + # Clean up + test_file.unlink() + logger.debug("Write permission check passed for %s", self.base_dir) + except PermissionError as e: + raise StorageValidationError( + f"No write permission for storage directory '{self.base_dir}': {e}" + ) + except OSError as e: + raise StorageValidationError( + f"Failed to verify write permissions for '{self.base_dir}': {e}" + ) + finally: + # Ensure test file is removed even on partial failure + if test_file.exists(): + try: + test_file.unlink() + except Exception: + pass + + def _detect_nas_storage(self) -> bool: + """ + Detect if the storage directory appears to be on a NAS mount. + + This is a best-effort detection based on common mount point patterns. + """ + path_str = str(self.base_dir) + + # Check common NAS mount point patterns + for indicator in self.NAS_MOUNT_INDICATORS: + if indicator in path_str: + logger.debug("NAS storage detected: path contains '%s'", indicator) + return True + + # Check if it's a mount point (Unix-like systems) + try: + if self.base_dir.is_mount(): + logger.debug("NAS storage detected: path is a mount point") + return True + except Exception: + pass + + # Check mount info on Linux + try: + with open("/proc/mounts", "r") as f: + mounts = f.read() + if path_str in mounts: + # Check for network filesystem types + for line in mounts.splitlines(): + if path_str in line: + fs_type = line.split()[2] if len(line.split()) > 2 else "" + if fs_type in ["nfs", "nfs4", "cifs", "smb", "smbfs"]: + logger.debug("NAS storage detected: mounted as %s", fs_type) + return True + except FileNotFoundError: + pass # Not on Linux + except Exception as e: + logger.debug("Could not check /proc/mounts: %s", e) + + return False + + def get_storage_status(self) -> dict: + """Get current storage status for health checks.""" + return { + **self._storage_status, + "base_dir": str(self.base_dir), + "exists": self.base_dir.exists(), + "is_directory": self.base_dir.is_dir() if self.base_dir.exists() else False, + } def _ensure_base_dir(self): """Ensure the base upload directory exists.""" self.base_dir.mkdir(parents=True, exist_ok=True) + def _validate_path_component(self, component: str, component_name: str) -> None: + """ + Validate a path component to prevent path traversal attacks. + + Args: + component: The path component to validate (e.g., project_id, task_id) + component_name: Name of the component for error messages + + Raises: + PathTraversalError: If the component contains path traversal patterns + """ + if not component: + raise PathTraversalError(f"Empty {component_name} is not allowed") + + # Check for path traversal patterns + dangerous_patterns = ['..', '/', '\\', '\x00'] + for pattern in dangerous_patterns: + if pattern in component: + logger.warning( + "Path traversal attempt detected in %s: %r", + component_name, + component + ) + raise PathTraversalError( + f"Invalid characters in {component_name}: path traversal not allowed" + ) + + # Additional check: component should not start with special characters + if component.startswith('.') or component.startswith('-'): + logger.warning( + "Suspicious path component in %s: %r", + component_name, + component + ) + raise PathTraversalError( + f"Invalid {component_name}: cannot start with '.' or '-'" + ) + + def _validate_path_in_base_dir(self, path: Path, context: str = "") -> Path: + """ + Validate that a resolved path is within the base directory. + + Args: + path: The path to validate + context: Additional context for logging + + Returns: + The resolved path if valid + + Raises: + PathTraversalError: If the path is outside the base directory + """ + resolved_path = path.resolve() + + # Check if the resolved path is within the base directory + try: + resolved_path.relative_to(self.base_dir) + except ValueError: + logger.warning( + "Path traversal attempt detected: path %s is outside base directory %s. Context: %s", + resolved_path, + self.base_dir, + context + ) + raise PathTraversalError( + "Access denied: path is outside the allowed directory" + ) + + return resolved_path + def _get_file_path(self, project_id: str, task_id: str, attachment_id: str, version: int) -> Path: - """Generate the file path for an attachment version.""" - return self.base_dir / project_id / task_id / attachment_id / str(version) + """ + Generate the file path for an attachment version. + + Args: + project_id: The project ID + task_id: The task ID + attachment_id: The attachment ID + version: The version number + + Returns: + Safe path within the base directory + + Raises: + PathTraversalError: If any component contains path traversal patterns + """ + # Validate all path components + self._validate_path_component(project_id, "project_id") + self._validate_path_component(task_id, "task_id") + self._validate_path_component(attachment_id, "attachment_id") + + if version < 0: + raise PathTraversalError("Version must be non-negative") + + # Build the path + path = self.base_dir / project_id / task_id / attachment_id / str(version) + + # Validate the final path is within base directory + return self._validate_path_in_base_dir( + path, + f"project={project_id}, task={task_id}, attachment={attachment_id}, version={version}" + ) @staticmethod def calculate_checksum(file: BinaryIO) -> str: @@ -89,6 +334,10 @@ class FileStorageService: """ Save uploaded file to storage. Returns (file_path, file_size, checksum). + + Raises: + PathTraversalError: If path traversal is detected + HTTPException: If file validation fails """ # Validate file extension, _ = self.validate_file(file) @@ -96,14 +345,22 @@ class FileStorageService: # Calculate checksum first checksum = self.calculate_checksum(file.file) - # Create directory structure - dir_path = self._get_file_path(project_id, task_id, attachment_id, version) + # Create directory structure (path validation is done in _get_file_path) + try: + dir_path = self._get_file_path(project_id, task_id, attachment_id, version) + except PathTraversalError as e: + logger.error("Path traversal attempt during file save: %s", e) + raise HTTPException(status_code=400, detail=str(e)) + dir_path.mkdir(parents=True, exist_ok=True) # Save file with original extension filename = f"file.{extension}" if extension else "file" file_path = dir_path / filename + # Final validation of the file path + self._validate_path_in_base_dir(file_path, f"saving file {filename}") + # Get file size file.file.seek(0, 2) file_size = file.file.tell() @@ -125,8 +382,15 @@ class FileStorageService: """ Get the file path for an attachment version. Returns None if file doesn't exist. + + Raises: + PathTraversalError: If path traversal is detected """ - dir_path = self._get_file_path(project_id, task_id, attachment_id, version) + try: + dir_path = self._get_file_path(project_id, task_id, attachment_id, version) + except PathTraversalError as e: + logger.error("Path traversal attempt during file retrieval: %s", e) + return None if not dir_path.exists(): return None @@ -139,21 +403,48 @@ class FileStorageService: return files[0] def get_file_by_path(self, file_path: str) -> Optional[Path]: - """Get file by stored path. Handles both absolute and relative paths.""" + """ + Get file by stored path. Handles both absolute and relative paths. + + This method validates that the requested path is within the base directory + to prevent path traversal attacks. + + Args: + file_path: The stored file path + + Returns: + Path object if file exists and is within base directory, None otherwise + """ + if not file_path: + return None + path = Path(file_path) - # If path is absolute and exists, return it directly - if path.is_absolute() and path.exists(): - return path + # For absolute paths, validate they are within base_dir + if path.is_absolute(): + try: + validated_path = self._validate_path_in_base_dir( + path, + f"get_file_by_path absolute: {file_path}" + ) + if validated_path.exists(): + return validated_path + except PathTraversalError: + return None + return None - # If path is relative, try prepending base_dir + # For relative paths, resolve from base_dir full_path = self.base_dir / path - if full_path.exists(): - return full_path - # Fallback: check if original path exists (e.g., relative from current dir) - if path.exists(): - return path + try: + validated_path = self._validate_path_in_base_dir( + full_path, + f"get_file_by_path relative: {file_path}" + ) + if validated_path.exists(): + return validated_path + except PathTraversalError: + return None return None @@ -168,13 +459,29 @@ class FileStorageService: Delete file(s) from storage. If version is None, deletes all versions. Returns True if successful. + + Raises: + PathTraversalError: If path traversal is detected """ - if version is not None: - # Delete specific version - dir_path = self._get_file_path(project_id, task_id, attachment_id, version) - else: - # Delete all versions (attachment directory) - dir_path = self.base_dir / project_id / task_id / attachment_id + try: + if version is not None: + # Delete specific version + dir_path = self._get_file_path(project_id, task_id, attachment_id, version) + else: + # Delete all versions (attachment directory) + # Validate components first + self._validate_path_component(project_id, "project_id") + self._validate_path_component(task_id, "task_id") + self._validate_path_component(attachment_id, "attachment_id") + + dir_path = self.base_dir / project_id / task_id / attachment_id + dir_path = self._validate_path_in_base_dir( + dir_path, + f"delete attachment: project={project_id}, task={task_id}, attachment={attachment_id}" + ) + except PathTraversalError as e: + logger.error("Path traversal attempt during file deletion: %s", e) + return False if dir_path.exists(): shutil.rmtree(dir_path) @@ -182,8 +489,26 @@ class FileStorageService: return False def delete_task_files(self, project_id: str, task_id: str) -> bool: - """Delete all files for a task.""" - dir_path = self.base_dir / project_id / task_id + """ + Delete all files for a task. + + Raises: + PathTraversalError: If path traversal is detected + """ + try: + # Validate components + self._validate_path_component(project_id, "project_id") + self._validate_path_component(task_id, "task_id") + + dir_path = self.base_dir / project_id / task_id + dir_path = self._validate_path_in_base_dir( + dir_path, + f"delete task files: project={project_id}, task={task_id}" + ) + except PathTraversalError as e: + logger.error("Path traversal attempt during task file deletion: %s", e) + return False + if dir_path.exists(): shutil.rmtree(dir_path) return True diff --git a/backend/app/services/formula_service.py b/backend/app/services/formula_service.py index e1069f6..525e969 100644 --- a/backend/app/services/formula_service.py +++ b/backend/app/services/formula_service.py @@ -29,7 +29,17 @@ class FormulaError(Exception): class CircularReferenceError(FormulaError): """Exception raised when circular references are detected in formulas.""" - pass + + def __init__(self, message: str, cycle_path: Optional[List[str]] = None): + super().__init__(message) + self.message = message + self.cycle_path = cycle_path or [] + + def get_cycle_description(self) -> str: + """Get a human-readable description of the cycle.""" + if not self.cycle_path: + return "" + return " -> ".join(self.cycle_path) class FormulaService: @@ -140,24 +150,43 @@ class FormulaService: field_id: str, references: Set[str], visited: Optional[Set[str]] = None, + path: Optional[List[str]] = None, ) -> None: """ Check for circular references in formula fields. Raises CircularReferenceError if a cycle is detected. + + Args: + db: Database session + project_id: Project ID to scope the query + field_id: The current field being validated + references: Set of field names referenced in the formula + visited: Set of visited field IDs (for cycle detection) + path: Current path of field names (for error reporting) """ if visited is None: visited = set() + if path is None: + path = [] # Get the current field's name current_field = db.query(CustomField).filter( CustomField.id == field_id ).first() + current_field_name = current_field.name if current_field else "unknown" + + # Add current field to path if not already there + if current_field_name not in path: + path = path + [current_field_name] + if current_field: if current_field.name in references: + cycle_path = path + [current_field.name] raise CircularReferenceError( - f"Circular reference detected: field cannot reference itself" + f"Circular reference detected: field '{current_field.name}' cannot reference itself", + cycle_path=cycle_path ) # Get all referenced formula fields @@ -173,22 +202,199 @@ class FormulaService: for field in formula_fields: if field.id in visited: + # Found a cycle + cycle_path = path + [field.name] raise CircularReferenceError( - f"Circular reference detected involving field '{field.name}'" + f"Circular reference detected: {' -> '.join(cycle_path)}", + cycle_path=cycle_path ) visited.add(field.id) + new_path = path + [field.name] if field.formula: nested_refs = FormulaService.extract_field_references(field.formula) if current_field and current_field.name in nested_refs: + cycle_path = new_path + [current_field.name] raise CircularReferenceError( - f"Circular reference detected: '{field.name}' references the current field" + f"Circular reference detected: {' -> '.join(cycle_path)}", + cycle_path=cycle_path ) FormulaService._check_circular_references( - db, project_id, field_id, nested_refs, visited + db, project_id, field_id, nested_refs, visited, new_path ) + @staticmethod + def build_formula_dependency_graph( + db: Session, + project_id: str + ) -> Dict[str, Set[str]]: + """ + Build a dependency graph for all formula fields in a project. + + Returns a dict where keys are field names and values are sets of + field names that the key field depends on. + + Args: + db: Database session + project_id: Project ID to scope the query + + Returns: + Dict mapping field names to their dependencies + """ + graph: Dict[str, Set[str]] = {} + + formula_fields = db.query(CustomField).filter( + CustomField.project_id == project_id, + CustomField.field_type == "formula", + ).all() + + for field in formula_fields: + if field.formula: + refs = FormulaService.extract_field_references(field.formula) + # Only include custom field references (not builtin fields) + custom_refs = refs - FormulaService.BUILTIN_FIELDS + graph[field.name] = custom_refs + else: + graph[field.name] = set() + + return graph + + @staticmethod + def detect_formula_cycles( + db: Session, + project_id: str + ) -> List[List[str]]: + """ + Detect all cycles in the formula dependency graph for a project. + + This is useful for auditing or cleanup operations. + + Args: + db: Database session + project_id: Project ID to check + + Returns: + List of cycles, where each cycle is a list of field names + """ + graph = FormulaService.build_formula_dependency_graph(db, project_id) + + if not graph: + return [] + + cycles: List[List[str]] = [] + visited: Set[str] = set() + found_cycles: Set[Tuple[str, ...]] = set() + + def dfs(node: str, path: List[str], in_path: Set[str]): + """DFS to find cycles.""" + if node in in_path: + # Found a cycle + cycle_start = path.index(node) + cycle = path[cycle_start:] + [node] + # Normalize for deduplication + normalized = tuple(sorted(cycle[:-1])) + if normalized not in found_cycles: + found_cycles.add(normalized) + cycles.append(cycle) + return + + if node in visited: + return + + if node not in graph: + return + + visited.add(node) + in_path.add(node) + path.append(node) + + for neighbor in graph.get(node, set()): + dfs(neighbor, path.copy(), in_path.copy()) + + path.pop() + in_path.discard(node) + + for start_node in graph.keys(): + if start_node not in visited: + dfs(start_node, [], set()) + + return cycles + + @staticmethod + def validate_formula_with_details( + formula: str, + project_id: str, + db: Session, + current_field_id: Optional[str] = None, + ) -> Tuple[bool, Optional[str], Optional[List[str]]]: + """ + Validate a formula expression with detailed error information. + + Similar to validate_formula but returns cycle path on circular reference errors. + + Args: + formula: The formula expression to validate + project_id: Project ID to scope field lookups + db: Database session + current_field_id: Optional ID of the field being edited (for self-reference check) + + Returns: + Tuple of (is_valid, error_message, cycle_path) + """ + if not formula or not formula.strip(): + return False, "Formula cannot be empty", None + + # Extract field references + references = FormulaService.extract_field_references(formula) + + if not references: + return False, "Formula must reference at least one field", None + + # Validate syntax by trying to parse + try: + # Replace field references with dummy numbers for syntax check + test_formula = formula + for ref in references: + test_formula = test_formula.replace(f"{{{ref}}}", "1") + + # Try to parse and evaluate with safe operations + FormulaService._safe_eval(test_formula) + except Exception as e: + return False, f"Invalid formula syntax: {str(e)}", None + + # Separate builtin and custom field references + custom_references = references - FormulaService.BUILTIN_FIELDS + + # Validate custom field references exist and are numeric types + if custom_references: + fields = db.query(CustomField).filter( + CustomField.project_id == project_id, + CustomField.name.in_(custom_references), + ).all() + + found_names = {f.name for f in fields} + missing = custom_references - found_names + + if missing: + return False, f"Unknown field references: {', '.join(missing)}", None + + # Check field types (must be number or formula) + for field in fields: + if field.field_type not in ("number", "formula"): + return False, f"Field '{field.name}' is not a numeric type", None + + # Check for circular references + if current_field_id: + try: + FormulaService._check_circular_references( + db, project_id, current_field_id, references + ) + except CircularReferenceError as e: + return False, str(e), e.cycle_path + + return True, None, None + @staticmethod def _safe_eval(expression: str) -> Decimal: """ diff --git a/backend/app/services/notification_service.py b/backend/app/services/notification_service.py index 6467054..d4e2691 100644 --- a/backend/app/services/notification_service.py +++ b/backend/app/services/notification_service.py @@ -4,8 +4,10 @@ import re import asyncio import logging import threading +import os from datetime import datetime, timezone from typing import List, Optional, Dict, Set +from collections import deque from sqlalchemy.orm import Session from sqlalchemy import event @@ -22,9 +24,152 @@ _pending_publish: Dict[int, List[dict]] = {} # Track which sessions have handlers registered _registered_sessions: Set[int] = set() +# Redis fallback queue configuration +REDIS_FALLBACK_MAX_QUEUE_SIZE = int(os.getenv("REDIS_FALLBACK_MAX_QUEUE_SIZE", "1000")) +REDIS_FALLBACK_RETRY_INTERVAL = int(os.getenv("REDIS_FALLBACK_RETRY_INTERVAL", "5")) # seconds +REDIS_FALLBACK_MAX_RETRIES = int(os.getenv("REDIS_FALLBACK_MAX_RETRIES", "10")) + +# Redis fallback queue for failed publishes +_redis_fallback_lock = threading.Lock() +_redis_fallback_queue: deque = deque(maxlen=REDIS_FALLBACK_MAX_QUEUE_SIZE) +_redis_retry_timer: Optional[threading.Timer] = None +_redis_available = True +_redis_consecutive_failures = 0 + + +def _add_to_fallback_queue(user_id: str, data: dict, retry_count: int = 0) -> bool: + """ + Add a failed notification to the fallback queue. + + Returns True if added successfully, False if queue is full. + """ + global _redis_consecutive_failures + + with _redis_fallback_lock: + if len(_redis_fallback_queue) >= REDIS_FALLBACK_MAX_QUEUE_SIZE: + logger.warning( + "Redis fallback queue is full (%d items), dropping notification for user %s", + REDIS_FALLBACK_MAX_QUEUE_SIZE, user_id + ) + return False + + _redis_fallback_queue.append({ + "user_id": user_id, + "data": data, + "retry_count": retry_count, + "queued_at": datetime.now(timezone.utc).isoformat(), + }) + _redis_consecutive_failures += 1 + + queue_size = len(_redis_fallback_queue) + logger.debug("Added notification to fallback queue (size: %d)", queue_size) + + # Start retry mechanism if not already running + _ensure_retry_timer_running() + + return True + + +def _ensure_retry_timer_running(): + """Ensure the retry timer is running if there are items in the queue.""" + global _redis_retry_timer + + if _redis_retry_timer is None or not _redis_retry_timer.is_alive(): + _redis_retry_timer = threading.Timer(REDIS_FALLBACK_RETRY_INTERVAL, _process_fallback_queue) + _redis_retry_timer.daemon = True + _redis_retry_timer.start() + + +def _process_fallback_queue(): + """Process the fallback queue and retry sending notifications to Redis.""" + global _redis_available, _redis_consecutive_failures, _redis_retry_timer + + items_to_retry = [] + + with _redis_fallback_lock: + # Get all items from queue + while _redis_fallback_queue: + items_to_retry.append(_redis_fallback_queue.popleft()) + + if not items_to_retry: + _redis_retry_timer = None + return + + logger.info("Processing %d items from Redis fallback queue", len(items_to_retry)) + + failed_items = [] + success_count = 0 + + for item in items_to_retry: + user_id = item["user_id"] + data = item["data"] + retry_count = item["retry_count"] + + if retry_count >= REDIS_FALLBACK_MAX_RETRIES: + logger.warning( + "Notification for user %s exceeded max retries (%d), dropping", + user_id, REDIS_FALLBACK_MAX_RETRIES + ) + continue + + try: + redis_client = get_redis_sync() + channel = get_channel_name(user_id) + message = json.dumps(data, default=str) + redis_client.publish(channel, message) + success_count += 1 + except Exception as e: + logger.debug("Retry failed for user %s: %s", user_id, e) + failed_items.append({ + **item, + "retry_count": retry_count + 1, + }) + + # Re-queue failed items + if failed_items: + with _redis_fallback_lock: + for item in failed_items: + if len(_redis_fallback_queue) < REDIS_FALLBACK_MAX_QUEUE_SIZE: + _redis_fallback_queue.append(item) + + # Log recovery if we had successes + if success_count > 0: + with _redis_fallback_lock: + _redis_consecutive_failures = 0 + if not _redis_fallback_queue: + _redis_available = True + logger.info( + "Redis connection recovered. Successfully processed %d notifications from fallback queue", + success_count + ) + + # Schedule next retry if queue is not empty + with _redis_fallback_lock: + if _redis_fallback_queue: + _redis_retry_timer = threading.Timer(REDIS_FALLBACK_RETRY_INTERVAL, _process_fallback_queue) + _redis_retry_timer.daemon = True + _redis_retry_timer.start() + else: + _redis_retry_timer = None + + +def get_redis_fallback_status() -> dict: + """Get current Redis fallback queue status for health checks.""" + with _redis_fallback_lock: + return { + "queue_size": len(_redis_fallback_queue), + "max_queue_size": REDIS_FALLBACK_MAX_QUEUE_SIZE, + "redis_available": _redis_available, + "consecutive_failures": _redis_consecutive_failures, + "retry_interval_seconds": REDIS_FALLBACK_RETRY_INTERVAL, + "max_retries": REDIS_FALLBACK_MAX_RETRIES, + } + def _sync_publish(user_id: str, data: dict): """Sync fallback to publish notification via Redis when no event loop available.""" + global _redis_available + try: redis_client = get_redis_sync() channel = get_channel_name(user_id) @@ -33,6 +178,10 @@ def _sync_publish(user_id: str, data: dict): logger.debug(f"Sync published notification to channel {channel}") except Exception as e: logger.error(f"Failed to sync publish notification to Redis: {e}") + # Add to fallback queue for retry + with _redis_fallback_lock: + _redis_available = False + _add_to_fallback_queue(user_id, data) def _cleanup_session(session_id: int, remove_registration: bool = True): @@ -86,10 +235,16 @@ def _register_session_handlers(db: Session, session_id: int): async def _async_publish(user_id: str, data: dict): """Async helper to publish notification to Redis.""" + global _redis_available + try: await redis_publish(user_id, data) except Exception as e: logger.error(f"Failed to publish notification to Redis: {e}") + # Add to fallback queue for retry + with _redis_fallback_lock: + _redis_available = False + _add_to_fallback_queue(user_id, data) class NotificationService: diff --git a/backend/app/services/trigger_scheduler.py b/backend/app/services/trigger_scheduler.py index 0fc7d58..38865e8 100644 --- a/backend/app/services/trigger_scheduler.py +++ b/backend/app/services/trigger_scheduler.py @@ -7,6 +7,7 @@ scheduled triggers based on their cron schedule, including deadline reminders. import uuid import logging +import time from datetime import datetime, timezone, timedelta from typing import Optional, List, Dict, Any, Tuple, Set @@ -22,6 +23,10 @@ logger = logging.getLogger(__name__) # Key prefix for tracking deadline reminders already sent DEADLINE_REMINDER_LOG_TYPE = "deadline_reminder" +# Retry configuration +MAX_RETRIES = 3 +BASE_DELAY_SECONDS = 1 # 1s, 2s, 4s exponential backoff + class TriggerSchedulerService: """Service for scheduling and executing cron-based triggers.""" @@ -220,50 +225,170 @@ class TriggerSchedulerService: @staticmethod def _execute_trigger(db: Session, trigger: Trigger) -> TriggerLog: """ - Execute a scheduled trigger's actions. + Execute a scheduled trigger's actions with retry mechanism. + + Implements exponential backoff retry (1s, 2s, 4s) for transient failures. + After max retries are exhausted, marks as permanently failed and sends alert. Args: db: Database session trigger: The trigger to execute + Returns: + TriggerLog entry for this execution + """ + return TriggerSchedulerService._execute_trigger_with_retry( + db=db, + trigger=trigger, + task_id=None, + log_type="schedule", + ) + + @staticmethod + def _execute_trigger_with_retry( + db: Session, + trigger: Trigger, + task_id: Optional[str] = None, + log_type: str = "schedule", + ) -> TriggerLog: + """ + Execute trigger actions with exponential backoff retry. + + Args: + db: Database session + trigger: The trigger to execute + task_id: Optional task ID for context (deadline reminders) + log_type: Type of trigger execution for logging + Returns: TriggerLog entry for this execution """ actions = trigger.actions if isinstance(trigger.actions, list) else [trigger.actions] executed_actions = [] - error_message = None + last_error = None + attempt = 0 - try: - for action in actions: - action_type = action.get("type") + while attempt < MAX_RETRIES: + attempt += 1 + executed_actions = [] + last_error = None - if action_type == "notify": - TriggerSchedulerService._execute_notify_action(db, action, trigger) - executed_actions.append({"type": action_type, "status": "success"}) + try: + logger.info( + f"Executing trigger {trigger.id} (attempt {attempt}/{MAX_RETRIES})" + ) - # Add more action types here as needed + for action in actions: + action_type = action.get("type") - status = "success" + if action_type == "notify": + TriggerSchedulerService._execute_notify_action(db, action, trigger) + executed_actions.append({"type": action_type, "status": "success"}) - except Exception as e: - status = "failed" - error_message = str(e) - executed_actions.append({"type": "error", "message": str(e)}) - logger.error(f"Error executing trigger {trigger.id} actions: {e}") + # Add more action types here as needed + + # Success - return log + logger.info(f"Trigger {trigger.id} executed successfully on attempt {attempt}") + return TriggerSchedulerService._log_execution( + db=db, + trigger=trigger, + status="success", + details={ + "trigger_name": trigger.name, + "trigger_type": log_type, + "cron_expression": trigger.conditions.get("cron_expression") if trigger.conditions else None, + "actions_executed": executed_actions, + "attempts": attempt, + }, + error_message=None, + task_id=task_id, + ) + + except Exception as e: + last_error = e + executed_actions.append({"type": "error", "message": str(e)}) + logger.warning( + f"Trigger {trigger.id} failed on attempt {attempt}/{MAX_RETRIES}: {e}" + ) + + # Calculate exponential backoff delay + if attempt < MAX_RETRIES: + delay = BASE_DELAY_SECONDS * (2 ** (attempt - 1)) + logger.info(f"Retrying trigger {trigger.id} in {delay}s...") + time.sleep(delay) + + # All retries exhausted - permanent failure + logger.error( + f"Trigger {trigger.id} permanently failed after {MAX_RETRIES} attempts: {last_error}" + ) + + # Send alert notification for permanent failure + TriggerSchedulerService._send_failure_alert(db, trigger, str(last_error), MAX_RETRIES) return TriggerSchedulerService._log_execution( db=db, trigger=trigger, - status=status, + status="permanently_failed", details={ "trigger_name": trigger.name, - "trigger_type": "schedule", - "cron_expression": trigger.conditions.get("cron_expression"), + "trigger_type": log_type, + "cron_expression": trigger.conditions.get("cron_expression") if trigger.conditions else None, "actions_executed": executed_actions, + "attempts": MAX_RETRIES, + "permanent_failure": True, }, - error_message=error_message, + error_message=f"Failed after {MAX_RETRIES} retries: {last_error}", + task_id=task_id, ) + @staticmethod + def _send_failure_alert( + db: Session, + trigger: Trigger, + error_message: str, + attempts: int, + ) -> None: + """ + Send alert notification when trigger exhausts all retries. + + Args: + db: Database session + trigger: The failed trigger + error_message: The last error message + attempts: Number of attempts made + """ + try: + # Notify the project owner about the failure + project = trigger.project + if not project: + logger.warning(f"Cannot send failure alert: trigger {trigger.id} has no project") + return + + target_user_id = project.owner_id + if not target_user_id: + logger.warning(f"Cannot send failure alert: project {project.id} has no owner") + return + + message = ( + f"Trigger '{trigger.name}' has permanently failed after {attempts} attempts. " + f"Last error: {error_message}" + ) + + NotificationService.create_notification( + db=db, + user_id=target_user_id, + notification_type="trigger_failure", + reference_type="trigger", + reference_id=trigger.id, + title=f"Trigger Failed: {trigger.name}", + message=message, + ) + + logger.info(f"Sent failure alert for trigger {trigger.id} to user {target_user_id}") + + except Exception as e: + logger.error(f"Failed to send failure alert for trigger {trigger.id}: {e}") + @staticmethod def _execute_notify_action(db: Session, action: Dict[str, Any], trigger: Trigger) -> None: """ diff --git a/backend/migrations/versions/014_permission_enhancements.py b/backend/migrations/versions/014_permission_enhancements.py new file mode 100644 index 0000000..0d99157 --- /dev/null +++ b/backend/migrations/versions/014_permission_enhancements.py @@ -0,0 +1,49 @@ +"""Add permission enhancements - manager flag and project members table + +Revision ID: 014 +Revises: a0a0f2710e01 +Create Date: 2026-01-10 + +Add is_department_manager flag to users and create project_members table +for cross-department collaboration support. +""" +from typing import Sequence, Union +from alembic import op +import sqlalchemy as sa + +revision: str = '014' +down_revision: Union[str, None] = 'a0a0f2710e01' +branch_labels: Union[str, Sequence[str], None] = None +depends_on: Union[str, Sequence[str], None] = None + + +def upgrade() -> None: + # Add is_department_manager column to pjctrl_users table + op.add_column( + 'pjctrl_users', + sa.Column('is_department_manager', sa.Boolean(), nullable=False, server_default='0') + ) + + # Create project_members table for cross-department collaboration + op.create_table( + 'pjctrl_project_members', + sa.Column('id', sa.String(36), primary_key=True), + sa.Column('project_id', sa.String(36), sa.ForeignKey('pjctrl_projects.id', ondelete='CASCADE'), nullable=False), + sa.Column('user_id', sa.String(36), sa.ForeignKey('pjctrl_users.id', ondelete='CASCADE'), nullable=False), + sa.Column('role', sa.String(50), nullable=False, server_default='member'), + sa.Column('added_by', sa.String(36), sa.ForeignKey('pjctrl_users.id'), nullable=False), + sa.Column('created_at', sa.DateTime(), server_default=sa.func.now(), nullable=False), + # Ensure a user can only be added once per project + sa.UniqueConstraint('project_id', 'user_id', name='uq_project_member'), + ) + + # Create indexes for efficient lookups + op.create_index('ix_pjctrl_project_members_project_id', 'pjctrl_project_members', ['project_id']) + op.create_index('ix_pjctrl_project_members_user_id', 'pjctrl_project_members', ['user_id']) + + +def downgrade() -> None: + op.drop_index('ix_pjctrl_project_members_user_id', table_name='pjctrl_project_members') + op.drop_index('ix_pjctrl_project_members_project_id', table_name='pjctrl_project_members') + op.drop_table('pjctrl_project_members') + op.drop_column('pjctrl_users', 'is_department_manager') diff --git a/backend/migrations/versions/015_add_task_version_field.py b/backend/migrations/versions/015_add_task_version_field.py new file mode 100644 index 0000000..a2845ae --- /dev/null +++ b/backend/migrations/versions/015_add_task_version_field.py @@ -0,0 +1,29 @@ +"""Add version field to tasks for optimistic locking + +Revision ID: 015 +Revises: 014 +Create Date: 2026-01-10 + +Add version integer field to tasks table for optimistic locking. +This prevents concurrent update conflicts by tracking version numbers. +""" +from typing import Sequence, Union +from alembic import op +import sqlalchemy as sa + +revision: str = '015' +down_revision: Union[str, None] = '014' +branch_labels: Union[str, Sequence[str], None] = None +depends_on: Union[str, Sequence[str], None] = None + + +def upgrade() -> None: + # Add version column to pjctrl_tasks table for optimistic locking + op.add_column( + 'pjctrl_tasks', + sa.Column('version', sa.Integer(), nullable=False, server_default='1') + ) + + +def downgrade() -> None: + op.drop_column('pjctrl_tasks', 'version') diff --git a/backend/migrations/versions/016_project_templates_table.py b/backend/migrations/versions/016_project_templates_table.py new file mode 100644 index 0000000..43bc894 --- /dev/null +++ b/backend/migrations/versions/016_project_templates_table.py @@ -0,0 +1,47 @@ +"""Add project templates table + +Revision ID: 016 +Revises: 015 +Create Date: 2026-01-10 + +Adds project_templates table for storing reusable project configurations +with predefined task statuses and custom fields. +""" +from typing import Sequence, Union +from alembic import op +import sqlalchemy as sa + +revision: str = '016' +down_revision: Union[str, None] = '015' +branch_labels: Union[str, Sequence[str], None] = None +depends_on: Union[str, Sequence[str], None] = None + + +def upgrade() -> None: + # Create pjctrl_project_templates table + op.create_table( + 'pjctrl_project_templates', + sa.Column('id', sa.String(36), primary_key=True), + sa.Column('name', sa.String(200), nullable=False), + sa.Column('description', sa.Text, nullable=True), + sa.Column('owner_id', sa.String(36), sa.ForeignKey('pjctrl_users.id'), nullable=False), + sa.Column('is_public', sa.Boolean, default=False, nullable=False), + sa.Column('is_active', sa.Boolean, default=True, nullable=False), + sa.Column('task_statuses', sa.JSON, nullable=True), + sa.Column('custom_fields', sa.JSON, nullable=True), + sa.Column('default_security_level', sa.String(20), default='department', nullable=True), + sa.Column('created_at', sa.DateTime, server_default=sa.func.now(), nullable=False), + sa.Column('updated_at', sa.DateTime, server_default=sa.func.now(), onupdate=sa.func.now(), nullable=False), + ) + + # Create indexes + op.create_index('ix_pjctrl_project_templates_owner_id', 'pjctrl_project_templates', ['owner_id']) + op.create_index('ix_pjctrl_project_templates_is_public', 'pjctrl_project_templates', ['is_public']) + op.create_index('ix_pjctrl_project_templates_is_active', 'pjctrl_project_templates', ['is_active']) + + +def downgrade() -> None: + op.drop_index('ix_pjctrl_project_templates_is_active', table_name='pjctrl_project_templates') + op.drop_index('ix_pjctrl_project_templates_is_public', table_name='pjctrl_project_templates') + op.drop_index('ix_pjctrl_project_templates_owner_id', table_name='pjctrl_project_templates') + op.drop_table('pjctrl_project_templates') diff --git a/backend/tests/test_api_enhancements.py b/backend/tests/test_api_enhancements.py new file mode 100644 index 0000000..b88b858 --- /dev/null +++ b/backend/tests/test_api_enhancements.py @@ -0,0 +1,257 @@ +""" +Tests for API enhancements. + +Tests cover: +- Standardized response format +- API versioning +- Enhanced health check endpoints +- Project templates +""" + +import os +os.environ["TESTING"] = "true" + +import pytest + + +class TestStandardizedResponse: + """Test standardized API response format.""" + + def test_success_response_structure(self, client, admin_token, db): + """Test that success responses have standard structure.""" + from app.models import Space + + space = Space(id="resp-space", name="Response Test", owner_id="00000000-0000-0000-0000-000000000001") + db.add(space) + db.commit() + + response = client.get( + "/api/spaces", + headers={"Authorization": f"Bearer {admin_token}"} + ) + + assert response.status_code == 200 + data = response.json() + + # Response should be either wrapped or direct data + # Depending on implementation, check for standard fields + assert data is not None + # If wrapped: assert "success" in data and "data" in data + # If direct: assert isinstance(data, (list, dict)) + + def test_error_response_structure(self, client, admin_token): + """Test that error responses have standard structure.""" + # Request non-existent resource + response = client.get( + "/api/spaces/non-existent-id", + headers={"Authorization": f"Bearer {admin_token}"} + ) + + assert response.status_code == 404 + data = response.json() + + # Error response should have detail field + assert "detail" in data or "message" in data or "error" in data + + +class TestAPIVersioning: + """Test API versioning with /api/v1 prefix.""" + + def test_v1_routes_accessible(self, client, admin_token, db): + """Test that /api/v1 routes are accessible.""" + from app.models import Space + + space = Space(id="v1-space", name="V1 Test", owner_id="00000000-0000-0000-0000-000000000001") + db.add(space) + db.commit() + + # Try v1 endpoint + response = client.get( + "/api/v1/spaces", + headers={"Authorization": f"Bearer {admin_token}"} + ) + + # Should be 200 if v1 routes exist, or 404 if not yet migrated + assert response.status_code in [200, 404] + + def test_legacy_routes_still_work(self, client, admin_token, db): + """Test that legacy /api routes still work during transition.""" + from app.models import Space + + space = Space(id="legacy-space", name="Legacy Test", owner_id="00000000-0000-0000-0000-000000000001") + db.add(space) + db.commit() + + response = client.get( + "/api/spaces", + headers={"Authorization": f"Bearer {admin_token}"} + ) + + assert response.status_code == 200 + + def test_deprecation_headers(self, client, admin_token, db): + """Test that deprecated routes include deprecation headers.""" + from app.models import Space + + space = Space(id="deprecation-space", name="Deprecation Test", owner_id="00000000-0000-0000-0000-000000000001") + db.add(space) + db.commit() + + response = client.get( + "/api/spaces", + headers={"Authorization": f"Bearer {admin_token}"} + ) + + # Check for deprecation header (if implemented) + # This is optional depending on implementation + # assert "Deprecation" in response.headers or "Sunset" in response.headers + + +class TestEnhancedHealthCheck: + """Test enhanced health check endpoints.""" + + def test_health_endpoint_returns_status(self, client): + """Test basic health endpoint.""" + response = client.get("/health") + + assert response.status_code == 200 + data = response.json() + assert "status" in data or data == {"status": "healthy"} + + def test_health_live_endpoint(self, client): + """Test /health/live endpoint for liveness probe.""" + response = client.get("/health/live") + + assert response.status_code == 200 + data = response.json() + assert data.get("status") == "alive" or "live" in str(data).lower() or "healthy" in str(data).lower() + + def test_health_ready_endpoint(self, client, db): + """Test /health/ready endpoint for readiness probe.""" + response = client.get("/health/ready") + + assert response.status_code == 200 + data = response.json() + + # Should include component checks + assert "status" in data or "ready" in str(data).lower() + + def test_health_includes_database_check(self, client, db): + """Test that health check includes database connectivity.""" + response = client.get("/health/ready") + + if response.status_code == 200: + data = response.json() + # Check if database status is included + if "checks" in data or "components" in data or "database" in data: + checks = data.get("checks", data.get("components", data)) + # Database should be checked + assert "database" in str(checks).lower() or "db" in str(checks).lower() or data.get("status") == "ready" + + def test_health_includes_redis_check(self, client, mock_redis): + """Test that health check includes Redis connectivity.""" + response = client.get("/health/ready") + + if response.status_code == 200: + data = response.json() + # Redis check may or may not be included based on implementation + + +class TestProjectTemplates: + """Test project template functionality.""" + + def test_list_templates(self, client, admin_token, db): + """Test listing available project templates.""" + response = client.get( + "/api/templates", + headers={"Authorization": f"Bearer {admin_token}"} + ) + + assert response.status_code == 200 + data = response.json() + + # Should return list of templates + assert "templates" in data or isinstance(data, list) + + def test_create_template(self, client, admin_token, db): + """Test creating a new project template.""" + from app.models import Space + + space = Space(id="template-space", name="Template Space", owner_id="00000000-0000-0000-0000-000000000001") + db.add(space) + db.commit() + + response = client.post( + "/api/templates", + json={ + "name": "Test Template", + "description": "A test template", + "default_statuses": [ + {"name": "To Do", "color": "#808080"}, + {"name": "In Progress", "color": "#0000FF"}, + {"name": "Done", "color": "#00FF00"} + ] + }, + headers={"Authorization": f"Bearer {admin_token}"} + ) + + assert response.status_code in [200, 201] + data = response.json() + assert data.get("name") == "Test Template" + + def test_create_project_from_template(self, client, admin_token, db): + """Test creating a project from a template.""" + from app.models import Space, ProjectTemplate + + space = Space(id="from-template-space", name="From Template Space", owner_id="00000000-0000-0000-0000-000000000001") + db.add(space) + + template = ProjectTemplate( + id="test-template-id", + name="Test Template", + description="Test", + default_statuses=[ + {"name": "Backlog", "color": "#808080"}, + {"name": "Active", "color": "#0000FF"}, + {"name": "Complete", "color": "#00FF00"} + ], + created_by="00000000-0000-0000-0000-000000000001" + ) + db.add(template) + db.commit() + + # Create project from template + response = client.post( + "/api/spaces/from-template-space/projects", + json={ + "name": "Project from Template", + "description": "Created from template", + "template_id": "test-template-id" + }, + headers={"Authorization": f"Bearer {admin_token}"} + ) + + assert response.status_code in [200, 201] + data = response.json() + assert data.get("name") == "Project from Template" + + def test_delete_template(self, client, admin_token, db): + """Test deleting a project template.""" + from app.models import ProjectTemplate + + template = ProjectTemplate( + id="delete-template-id", + name="Template to Delete", + description="Will be deleted", + default_statuses=[], + created_by="00000000-0000-0000-0000-000000000001" + ) + db.add(template) + db.commit() + + response = client.delete( + "/api/templates/delete-template-id", + headers={"Authorization": f"Bearer {admin_token}"} + ) + + assert response.status_code in [200, 204] diff --git a/backend/tests/test_backend_reliability.py b/backend/tests/test_backend_reliability.py new file mode 100644 index 0000000..a1cb7f5 --- /dev/null +++ b/backend/tests/test_backend_reliability.py @@ -0,0 +1,301 @@ +""" +Tests for backend reliability improvements. + +Tests cover: +- Database connection pool behavior +- Redis disconnect and recovery +- Blocker deletion scenarios +""" + +import os +os.environ["TESTING"] = "true" + +import pytest +from unittest.mock import patch, MagicMock +from datetime import datetime + + +class TestDatabaseConnectionPool: + """Test database connection pool behavior.""" + + def test_pool_handles_multiple_connections(self, client, admin_token, db): + """Test that connection pool handles multiple concurrent requests.""" + from app.models import Space + + # Create test space + space = Space(id="pool-test-space", name="Pool Test", owner_id="00000000-0000-0000-0000-000000000001") + db.add(space) + db.commit() + + # Make multiple concurrent requests + responses = [] + for i in range(10): + response = client.get( + "/api/spaces", + headers={"Authorization": f"Bearer {admin_token}"} + ) + responses.append(response) + + # All should succeed + assert all(r.status_code == 200 for r in responses) + + def test_pool_recovers_from_connection_error(self, client, admin_token, db): + """Test that pool recovers after connection errors.""" + from app.models import Space + + space = Space(id="recovery-space", name="Recovery Test", owner_id="00000000-0000-0000-0000-000000000001") + db.add(space) + db.commit() + + # First request should work + response1 = client.get( + "/api/spaces", + headers={"Authorization": f"Bearer {admin_token}"} + ) + assert response1.status_code == 200 + + # Simulate and recover from error - subsequent request should still work + response2 = client.get( + "/api/spaces", + headers={"Authorization": f"Bearer {admin_token}"} + ) + assert response2.status_code == 200 + + +class TestRedisFailover: + """Test Redis disconnect and recovery.""" + + def test_redis_publish_fallback_on_failure(self): + """Test that Redis publish failures are handled gracefully.""" + from app.core.redis import RedisManager + + manager = RedisManager() + + # Mock Redis failure + mock_redis = MagicMock() + mock_redis.publish.side_effect = Exception("Redis connection lost") + + with patch.object(manager, 'get_client', return_value=mock_redis): + # Should not raise, should queue message + try: + manager.publish_with_fallback("test_channel", {"test": "message"}) + except Exception: + pass # Some implementations may raise, that's ok for this test + + def test_message_queue_on_redis_failure(self): + """Test that messages are queued when Redis is unavailable.""" + from app.core.redis import RedisManager + + manager = RedisManager() + + # If manager has queue functionality + if hasattr(manager, '_message_queue') or hasattr(manager, 'queue_message'): + initial_queue_size = len(getattr(manager, '_message_queue', [])) + + # Force failure and queue + with patch.object(manager, '_publish_direct', side_effect=Exception("Redis down")): + try: + manager.publish_with_fallback("channel", {"data": "test"}) + except Exception: + pass + + # Check if message was queued (implementation dependent) + # This is a best-effort test + + def test_redis_reconnection(self, mock_redis): + """Test that Redis reconnects after failure.""" + # Simulate initial failure then success + call_count = [0] + original_get = mock_redis.get + + def intermittent_failure(key): + call_count[0] += 1 + if call_count[0] == 1: + raise Exception("Connection lost") + return original_get(key) + + mock_redis.get = intermittent_failure + + # First call fails + with pytest.raises(Exception): + mock_redis.get("test_key") + + # Second call succeeds (reconnected) + result = mock_redis.get("test_key") + assert call_count[0] == 2 + + +class TestBlockerDeletionCheck: + """Test blocker check before task deletion.""" + + def test_delete_task_with_blockers_warning(self, client, admin_token, db): + """Test that deleting task with blockers shows warning.""" + from app.models import Space, Project, Task, TaskStatus, TaskDependency + + # Create test data + space = Space(id="blocker-space", name="Blocker Test", owner_id="00000000-0000-0000-0000-000000000001") + db.add(space) + + project = Project(id="blocker-project", name="Blocker Project", space_id="blocker-space", owner_id="00000000-0000-0000-0000-000000000001") + db.add(project) + + status = TaskStatus(id="blocker-status", name="To Do", project_id="blocker-project", position=0) + db.add(status) + + # Task to delete + blocker_task = Task( + id="blocker-task", + title="Blocker Task", + project_id="blocker-project", + status_id="blocker-status", + created_by="00000000-0000-0000-0000-000000000001" + ) + db.add(blocker_task) + + # Dependent task + dependent_task = Task( + id="dependent-task", + title="Dependent Task", + project_id="blocker-project", + status_id="blocker-status", + created_by="00000000-0000-0000-0000-000000000001" + ) + db.add(dependent_task) + + # Create dependency + dependency = TaskDependency( + task_id="dependent-task", + depends_on_task_id="blocker-task", + dependency_type="FS" + ) + db.add(dependency) + db.commit() + + # Try to delete without force + response = client.delete( + "/api/tasks/blocker-task", + headers={"Authorization": f"Bearer {admin_token}"} + ) + + # Should return warning or require confirmation + # Response could be 200 with warning, or 409/400 requiring force_delete + if response.status_code == 200: + data = response.json() + # Check if it's a warning response + if "warning" in data or "blocker_count" in data: + assert data.get("blocker_count", 0) >= 1 or "blocker" in str(data).lower() + + def test_force_delete_resolves_blockers(self, client, admin_token, db): + """Test that force delete resolves blockers.""" + from app.models import Space, Project, Task, TaskStatus, TaskDependency + + # Create test data + space = Space(id="force-del-space", name="Force Del Test", owner_id="00000000-0000-0000-0000-000000000001") + db.add(space) + + project = Project(id="force-del-project", name="Force Del Project", space_id="force-del-space", owner_id="00000000-0000-0000-0000-000000000001") + db.add(project) + + status = TaskStatus(id="force-del-status", name="To Do", project_id="force-del-project", position=0) + db.add(status) + + # Task to delete + task_to_delete = Task( + id="force-del-task", + title="Task to Delete", + project_id="force-del-project", + status_id="force-del-status", + created_by="00000000-0000-0000-0000-000000000001" + ) + db.add(task_to_delete) + + # Dependent task + dependent = Task( + id="force-dependent", + title="Dependent", + project_id="force-del-project", + status_id="force-del-status", + created_by="00000000-0000-0000-0000-000000000001" + ) + db.add(dependent) + + # Create dependency + dep = TaskDependency( + task_id="force-dependent", + depends_on_task_id="force-del-task", + dependency_type="FS" + ) + db.add(dep) + db.commit() + + # Force delete + response = client.delete( + "/api/tasks/force-del-task?force_delete=true", + headers={"Authorization": f"Bearer {admin_token}"} + ) + + assert response.status_code == 200 + + # Verify task is deleted + db.refresh(task_to_delete) + assert task_to_delete.is_deleted is True + + def test_delete_task_without_blockers(self, client, admin_token, db): + """Test deleting task without blockers succeeds normally.""" + from app.models import Space, Project, Task, TaskStatus + + # Create test data + space = Space(id="no-blocker-space", name="No Blocker", owner_id="00000000-0000-0000-0000-000000000001") + db.add(space) + + project = Project(id="no-blocker-project", name="No Blocker Project", space_id="no-blocker-space", owner_id="00000000-0000-0000-0000-000000000001") + db.add(project) + + status = TaskStatus(id="no-blocker-status", name="To Do", project_id="no-blocker-project", position=0) + db.add(status) + + task = Task( + id="no-blocker-task", + title="Task without blockers", + project_id="no-blocker-project", + status_id="no-blocker-status", + created_by="00000000-0000-0000-0000-000000000001" + ) + db.add(task) + db.commit() + + # Delete should succeed without warning + response = client.delete( + "/api/tasks/no-blocker-task", + headers={"Authorization": f"Bearer {admin_token}"} + ) + + assert response.status_code == 200 + + # Verify task is deleted + db.refresh(task) + assert task.is_deleted is True + + +class TestStorageValidation: + """Test NAS/storage validation.""" + + def test_storage_path_validation_on_startup(self): + """Test that storage path is validated on startup.""" + from app.services.file_storage_service import FileStorageService + + service = FileStorageService() + + # Service should have validated upload directory + assert hasattr(service, 'upload_dir') or hasattr(service, '_upload_dir') + + def test_storage_write_permission_check(self): + """Test that storage write permissions are checked.""" + from app.services.file_storage_service import FileStorageService + + service = FileStorageService() + + # Check if service has permission validation + if hasattr(service, 'check_permissions'): + result = service.check_permissions() + assert result is True or result is None # Should not raise diff --git a/backend/tests/test_concurrency_reliability.py b/backend/tests/test_concurrency_reliability.py new file mode 100644 index 0000000..de23fe2 --- /dev/null +++ b/backend/tests/test_concurrency_reliability.py @@ -0,0 +1,310 @@ +""" +Tests for concurrency handling and reliability improvements. + +Tests cover: +- Optimistic locking with version conflicts +- Trigger retry mechanism +- Cascade restore for soft-deleted tasks +""" + +import os +os.environ["TESTING"] = "true" + +import pytest +from unittest.mock import patch, MagicMock +from datetime import datetime, timedelta + + +class TestOptimisticLocking: + """Test optimistic locking for concurrent updates.""" + + def test_version_increments_on_update(self, client, admin_token, db): + """Test that task version increments on successful update.""" + from app.models import Space, Project, Task, TaskStatus + + # Create test data + space = Space(id="space-1", name="Test Space", owner_id="00000000-0000-0000-0000-000000000001") + db.add(space) + + project = Project(id="project-1", name="Test Project", space_id="space-1", owner_id="00000000-0000-0000-0000-000000000001") + db.add(project) + + status = TaskStatus(id="status-1", name="To Do", project_id="project-1", position=0) + db.add(status) + + task = Task( + id="task-1", + title="Test Task", + project_id="project-1", + status_id="status-1", + created_by="00000000-0000-0000-0000-000000000001", + version=1 + ) + db.add(task) + db.commit() + + # Update task with correct version + response = client.patch( + "/api/tasks/task-1", + json={"title": "Updated Task", "version": 1}, + headers={"Authorization": f"Bearer {admin_token}"} + ) + + assert response.status_code == 200 + data = response.json() + assert data["title"] == "Updated Task" + assert data["version"] == 2 # Version should increment + + def test_version_conflict_returns_409(self, client, admin_token, db): + """Test that stale version returns 409 Conflict.""" + from app.models import Space, Project, Task, TaskStatus + + # Create test data + space = Space(id="space-2", name="Test Space 2", owner_id="00000000-0000-0000-0000-000000000001") + db.add(space) + + project = Project(id="project-2", name="Test Project 2", space_id="space-2", owner_id="00000000-0000-0000-0000-000000000001") + db.add(project) + + status = TaskStatus(id="status-2", name="To Do", project_id="project-2", position=0) + db.add(status) + + task = Task( + id="task-2", + title="Test Task", + project_id="project-2", + status_id="status-2", + created_by="00000000-0000-0000-0000-000000000001", + version=5 # Task is at version 5 + ) + db.add(task) + db.commit() + + # Try to update with stale version (1) + response = client.patch( + "/api/tasks/task-2", + json={"title": "Stale Update", "version": 1}, + headers={"Authorization": f"Bearer {admin_token}"} + ) + + assert response.status_code == 409 + assert "conflict" in response.json().get("detail", "").lower() or "version" in response.json().get("detail", "").lower() + + def test_update_without_version_succeeds(self, client, admin_token, db): + """Test that update without version (for backward compatibility) still works.""" + from app.models import Space, Project, Task, TaskStatus + + # Create test data + space = Space(id="space-3", name="Test Space 3", owner_id="00000000-0000-0000-0000-000000000001") + db.add(space) + + project = Project(id="project-3", name="Test Project 3", space_id="space-3", owner_id="00000000-0000-0000-0000-000000000001") + db.add(project) + + status = TaskStatus(id="status-3", name="To Do", project_id="project-3", position=0) + db.add(status) + + task = Task( + id="task-3", + title="Test Task", + project_id="project-3", + status_id="status-3", + created_by="00000000-0000-0000-0000-000000000001", + version=1 + ) + db.add(task) + db.commit() + + # Update without version field + response = client.patch( + "/api/tasks/task-3", + json={"title": "No Version Update"}, + headers={"Authorization": f"Bearer {admin_token}"} + ) + + # Should succeed (backward compatibility) + assert response.status_code == 200 + + +class TestTriggerRetryMechanism: + """Test trigger retry with exponential backoff.""" + + def test_trigger_scheduler_has_retry_config(self): + """Test that trigger scheduler has retry configuration.""" + from app.services.trigger_scheduler import MAX_RETRIES, BASE_DELAY_SECONDS + + # Verify configuration exists + assert MAX_RETRIES == 3 + assert BASE_DELAY_SECONDS == 1 + + def test_retry_mechanism_structure(self): + """Test that retry mechanism follows exponential backoff pattern.""" + from app.services.trigger_scheduler import TriggerSchedulerService + + # The service should have the retry method + assert hasattr(TriggerSchedulerService, '_execute_trigger_with_retry') + + def test_exponential_backoff_calculation(self): + """Test exponential backoff delay calculation.""" + from app.services.trigger_scheduler import BASE_DELAY_SECONDS + + # Verify backoff pattern (1s, 2s, 4s) + delays = [BASE_DELAY_SECONDS * (2 ** i) for i in range(3)] + assert delays == [1, 2, 4] + + def test_retry_on_failure_mock(self, db): + """Test retry behavior using mock.""" + from app.services.trigger_scheduler import TriggerSchedulerService + from app.models import ScheduleTrigger + + service = TriggerSchedulerService() + + call_count = [0] + + def mock_execute(*args, **kwargs): + call_count[0] += 1 + if call_count[0] < 3: + raise Exception("Transient failure") + return {"success": True} + + # Test the retry logic conceptually + # The actual retry happens internally, we verify the config exists + assert hasattr(service, 'execute_trigger') or hasattr(TriggerSchedulerService, '_execute_trigger_with_retry') + + +class TestCascadeRestore: + """Test cascade restore for soft-deleted tasks.""" + + def test_restore_parent_with_children(self, client, admin_token, db): + """Test restoring parent task also restores children deleted at same time.""" + from app.models import Space, Project, Task, TaskStatus + from datetime import datetime + + # Create test data + space = Space(id="space-4", name="Test Space 4", owner_id="00000000-0000-0000-0000-000000000001") + db.add(space) + + project = Project(id="project-4", name="Test Project 4", space_id="space-4", owner_id="00000000-0000-0000-0000-000000000001") + db.add(project) + + status = TaskStatus(id="status-4", name="To Do", project_id="project-4", position=0) + db.add(status) + + deleted_time = datetime.utcnow() + + parent_task = Task( + id="parent-task", + title="Parent Task", + project_id="project-4", + status_id="status-4", + created_by="00000000-0000-0000-0000-000000000001", + is_deleted=True, + deleted_at=deleted_time + ) + db.add(parent_task) + + child_task1 = Task( + id="child-task-1", + title="Child Task 1", + project_id="project-4", + status_id="status-4", + parent_task_id="parent-task", + created_by="00000000-0000-0000-0000-000000000001", + is_deleted=True, + deleted_at=deleted_time + ) + db.add(child_task1) + + child_task2 = Task( + id="child-task-2", + title="Child Task 2", + project_id="project-4", + status_id="status-4", + parent_task_id="parent-task", + created_by="00000000-0000-0000-0000-000000000001", + is_deleted=True, + deleted_at=deleted_time + ) + db.add(child_task2) + db.commit() + + # Restore parent with cascade=True + response = client.post( + "/api/tasks/parent-task/restore", + json={"cascade": True}, + headers={"Authorization": f"Bearer {admin_token}"} + ) + + assert response.status_code == 200 + data = response.json() + assert data["restored_children_count"] == 2 + assert "child-task-1" in data["restored_children_ids"] + assert "child-task-2" in data["restored_children_ids"] + + # Verify tasks are restored + db.refresh(parent_task) + db.refresh(child_task1) + db.refresh(child_task2) + + assert parent_task.is_deleted is False + assert child_task1.is_deleted is False + assert child_task2.is_deleted is False + + def test_restore_parent_only(self, client, admin_token, db): + """Test restoring parent task without cascade leaves children deleted.""" + from app.models import Space, Project, Task, TaskStatus + from datetime import datetime + + # Create test data + space = Space(id="space-5", name="Test Space 5", owner_id="00000000-0000-0000-0000-000000000001") + db.add(space) + + project = Project(id="project-5", name="Test Project 5", space_id="space-5", owner_id="00000000-0000-0000-0000-000000000001") + db.add(project) + + status = TaskStatus(id="status-5", name="To Do", project_id="project-5", position=0) + db.add(status) + + deleted_time = datetime.utcnow() + + parent_task = Task( + id="parent-task-2", + title="Parent Task 2", + project_id="project-5", + status_id="status-5", + created_by="00000000-0000-0000-0000-000000000001", + is_deleted=True, + deleted_at=deleted_time + ) + db.add(parent_task) + + child_task = Task( + id="child-task-3", + title="Child Task 3", + project_id="project-5", + status_id="status-5", + parent_task_id="parent-task-2", + created_by="00000000-0000-0000-0000-000000000001", + is_deleted=True, + deleted_at=deleted_time + ) + db.add(child_task) + db.commit() + + # Restore parent with cascade=False + response = client.post( + "/api/tasks/parent-task-2/restore", + json={"cascade": False}, + headers={"Authorization": f"Bearer {admin_token}"} + ) + + assert response.status_code == 200 + data = response.json() + assert data["restored_children_count"] == 0 + + # Verify parent restored but child still deleted + db.refresh(parent_task) + db.refresh(child_task) + + assert parent_task.is_deleted is False + assert child_task.is_deleted is True diff --git a/backend/tests/test_cycle_detection.py b/backend/tests/test_cycle_detection.py new file mode 100644 index 0000000..2c36d1f --- /dev/null +++ b/backend/tests/test_cycle_detection.py @@ -0,0 +1,732 @@ +""" +Tests for Cycle Detection in Task Dependencies and Formula Fields + +Tests cover: +- Task dependency cycle detection (direct and indirect) +- Bulk dependency validation with cycle detection +- Formula field circular reference detection +- Detailed cycle path reporting +""" +import pytest +from unittest.mock import MagicMock + +from app.models import Task, TaskDependency, Space, Project, TaskStatus, CustomField +from app.services.dependency_service import ( + DependencyService, + DependencyValidationError, + CycleDetectionResult +) +from app.services.formula_service import ( + FormulaService, + CircularReferenceError +) + + +class TestTaskDependencyCycleDetection: + """Test task dependency cycle detection.""" + + def setup_project(self, db, project_id: str, space_id: str): + """Create a space and project for testing.""" + space = Space( + id=space_id, + name="Test Space", + owner_id="00000000-0000-0000-0000-000000000001", + ) + db.add(space) + + project = Project( + id=project_id, + space_id=space_id, + title="Test Project", + owner_id="00000000-0000-0000-0000-000000000001", + security_level="public", + ) + db.add(project) + + status = TaskStatus( + id=f"status-{project_id}", + project_id=project_id, + name="To Do", + color="#808080", + position=0, + ) + db.add(status) + db.commit() + return project, status + + def create_task(self, db, task_id: str, project_id: str, status_id: str, title: str): + """Create a task for testing.""" + task = Task( + id=task_id, + project_id=project_id, + title=title, + priority="medium", + created_by="00000000-0000-0000-0000-000000000001", + status_id=status_id, + ) + db.add(task) + return task + + def test_direct_circular_dependency_A_B_A(self, db): + """Test detection of direct cycle: A -> B -> A.""" + project, status = self.setup_project(db, "proj-cycle-1", "space-cycle-1") + + task_a = self.create_task(db, "task-a-1", project.id, status.id, "Task A") + task_b = self.create_task(db, "task-b-1", project.id, status.id, "Task B") + db.commit() + + # Create A -> B dependency + dep = TaskDependency( + id="dep-ab-1", + predecessor_id="task-a-1", + successor_id="task-b-1", + dependency_type="FS", + lag_days=0, + ) + db.add(dep) + db.commit() + + # Try to create B -> A (would create cycle) + result = DependencyService.detect_circular_dependency_detailed( + db, "task-b-1", "task-a-1", project.id + ) + + assert result.has_cycle is True + assert len(result.cycle_path) > 0 + assert "task-a-1" in result.cycle_path + assert "task-b-1" in result.cycle_path + assert "Task A" in result.cycle_task_titles + assert "Task B" in result.cycle_task_titles + + def test_indirect_circular_dependency_A_B_C_A(self, db): + """Test detection of indirect cycle: A -> B -> C -> A.""" + project, status = self.setup_project(db, "proj-cycle-2", "space-cycle-2") + + task_a = self.create_task(db, "task-a-2", project.id, status.id, "Task A") + task_b = self.create_task(db, "task-b-2", project.id, status.id, "Task B") + task_c = self.create_task(db, "task-c-2", project.id, status.id, "Task C") + db.commit() + + # Create A -> B and B -> C dependencies + dep_ab = TaskDependency( + id="dep-ab-2", + predecessor_id="task-a-2", + successor_id="task-b-2", + dependency_type="FS", + lag_days=0, + ) + dep_bc = TaskDependency( + id="dep-bc-2", + predecessor_id="task-b-2", + successor_id="task-c-2", + dependency_type="FS", + lag_days=0, + ) + db.add_all([dep_ab, dep_bc]) + db.commit() + + # Try to create C -> A (would create cycle A -> B -> C -> A) + result = DependencyService.detect_circular_dependency_detailed( + db, "task-c-2", "task-a-2", project.id + ) + + assert result.has_cycle is True + cycle_desc = result.get_cycle_description() + assert "Task A" in cycle_desc + assert "Task B" in cycle_desc + assert "Task C" in cycle_desc + + def test_longer_cycle_path(self, db): + """Test detection of longer cycle: A -> B -> C -> D -> E -> A.""" + project, status = self.setup_project(db, "proj-cycle-3", "space-cycle-3") + + tasks = [] + for letter in ["A", "B", "C", "D", "E"]: + task = self.create_task( + db, f"task-{letter.lower()}-3", project.id, status.id, f"Task {letter}" + ) + tasks.append(task) + db.commit() + + # Create chain: A -> B -> C -> D -> E + deps = [] + task_ids = [f"task-{l.lower()}-3" for l in ["A", "B", "C", "D", "E"]] + for i in range(len(task_ids) - 1): + dep = TaskDependency( + id=f"dep-{i}-3", + predecessor_id=task_ids[i], + successor_id=task_ids[i + 1], + dependency_type="FS", + lag_days=0, + ) + deps.append(dep) + db.add_all(deps) + db.commit() + + # Try to create E -> A (would create cycle) + result = DependencyService.detect_circular_dependency_detailed( + db, "task-e-3", "task-a-3", project.id + ) + + assert result.has_cycle is True + assert len(result.cycle_path) >= 5 # Should contain all 5 tasks + repeat + + def test_no_cycle_valid_dependency(self, db): + """Test that valid dependency chains are accepted.""" + project, status = self.setup_project(db, "proj-valid-1", "space-valid-1") + + task_a = self.create_task(db, "task-a-v1", project.id, status.id, "Task A") + task_b = self.create_task(db, "task-b-v1", project.id, status.id, "Task B") + task_c = self.create_task(db, "task-c-v1", project.id, status.id, "Task C") + db.commit() + + # Create A -> B + dep = TaskDependency( + id="dep-ab-v1", + predecessor_id="task-a-v1", + successor_id="task-b-v1", + dependency_type="FS", + lag_days=0, + ) + db.add(dep) + db.commit() + + # B -> C should be valid (no cycle) + result = DependencyService.detect_circular_dependency_detailed( + db, "task-b-v1", "task-c-v1", project.id + ) + + assert result.has_cycle is False + assert len(result.cycle_path) == 0 + + def test_cycle_description_format(self, db): + """Test that cycle description is formatted correctly.""" + project, status = self.setup_project(db, "proj-desc-1", "space-desc-1") + + task_a = self.create_task(db, "task-a-d1", project.id, status.id, "Alpha Task") + task_b = self.create_task(db, "task-b-d1", project.id, status.id, "Beta Task") + db.commit() + + # Create A -> B + dep = TaskDependency( + id="dep-ab-d1", + predecessor_id="task-a-d1", + successor_id="task-b-d1", + dependency_type="FS", + lag_days=0, + ) + db.add(dep) + db.commit() + + # Try B -> A + result = DependencyService.detect_circular_dependency_detailed( + db, "task-b-d1", "task-a-d1", project.id + ) + + description = result.get_cycle_description() + assert " -> " in description # Should use arrow format + + +class TestBulkDependencyValidation: + """Test bulk dependency validation with cycle detection.""" + + def setup_project_with_tasks(self, db, project_id: str, space_id: str, task_count: int): + """Create a project with multiple tasks.""" + space = Space( + id=space_id, + name="Test Space", + owner_id="00000000-0000-0000-0000-000000000001", + ) + db.add(space) + + project = Project( + id=project_id, + space_id=space_id, + title="Test Project", + owner_id="00000000-0000-0000-0000-000000000001", + security_level="public", + ) + db.add(project) + + status = TaskStatus( + id=f"status-{project_id}", + project_id=project_id, + name="To Do", + color="#808080", + position=0, + ) + db.add(status) + + tasks = [] + for i in range(task_count): + task = Task( + id=f"task-{project_id}-{i}", + project_id=project_id, + title=f"Task {i}", + priority="medium", + created_by="00000000-0000-0000-0000-000000000001", + status_id=f"status-{project_id}", + ) + db.add(task) + tasks.append(task) + + db.commit() + return project, tasks + + def test_bulk_validation_detects_cycle_in_batch(self, db): + """Test that bulk validation detects cycles created by the batch itself.""" + project, tasks = self.setup_project_with_tasks(db, "proj-bulk-1", "space-bulk-1", 3) + + # Create A -> B -> C -> A in a single batch + dependencies = [ + (tasks[0].id, tasks[1].id), # A -> B + (tasks[1].id, tasks[2].id), # B -> C + (tasks[2].id, tasks[0].id), # C -> A (creates cycle) + ] + + errors = DependencyService.validate_bulk_dependencies(db, dependencies, project.id) + + # Should detect the cycle + assert len(errors) > 0 + cycle_errors = [e for e in errors if e.get("error_type") == "circular"] + assert len(cycle_errors) > 0 + + def test_bulk_validation_accepts_valid_chain(self, db): + """Test that bulk validation accepts valid dependency chains.""" + project, tasks = self.setup_project_with_tasks(db, "proj-bulk-2", "space-bulk-2", 4) + + # Create A -> B -> C -> D (valid chain) + dependencies = [ + (tasks[0].id, tasks[1].id), # A -> B + (tasks[1].id, tasks[2].id), # B -> C + (tasks[2].id, tasks[3].id), # C -> D + ] + + errors = DependencyService.validate_bulk_dependencies(db, dependencies, project.id) + + assert len(errors) == 0 + + def test_bulk_validation_detects_self_reference(self, db): + """Test that bulk validation detects self-references.""" + project, tasks = self.setup_project_with_tasks(db, "proj-bulk-3", "space-bulk-3", 2) + + dependencies = [ + (tasks[0].id, tasks[0].id), # Self-reference + ] + + errors = DependencyService.validate_bulk_dependencies(db, dependencies, project.id) + + assert len(errors) > 0 + assert errors[0]["error_type"] == "self_reference" + + def test_bulk_validation_detects_duplicate_in_existing(self, db): + """Test that bulk validation detects duplicates with existing dependencies.""" + project, tasks = self.setup_project_with_tasks(db, "proj-bulk-4", "space-bulk-4", 2) + + # Create existing dependency + dep = TaskDependency( + id="dep-existing-bulk-4", + predecessor_id=tasks[0].id, + successor_id=tasks[1].id, + dependency_type="FS", + lag_days=0, + ) + db.add(dep) + db.commit() + + # Try to add same dependency in bulk + dependencies = [ + (tasks[0].id, tasks[1].id), # Duplicate + ] + + errors = DependencyService.validate_bulk_dependencies(db, dependencies, project.id) + + assert len(errors) > 0 + assert errors[0]["error_type"] == "duplicate" + + +class TestFormulaFieldCycleDetection: + """Test formula field circular reference detection.""" + + def setup_project_with_fields(self, db, project_id: str, space_id: str): + """Create a project with custom fields.""" + space = Space( + id=space_id, + name="Test Space", + owner_id="00000000-0000-0000-0000-000000000001", + ) + db.add(space) + + project = Project( + id=project_id, + space_id=space_id, + title="Test Project", + owner_id="00000000-0000-0000-0000-000000000001", + security_level="public", + ) + db.add(project) + + status = TaskStatus( + id=f"status-{project_id}", + project_id=project_id, + name="To Do", + color="#808080", + position=0, + ) + db.add(status) + db.commit() + return project + + def test_formula_self_reference_detected(self, db): + """Test that a formula referencing itself is detected.""" + project = self.setup_project_with_fields(db, "proj-formula-1", "space-formula-1") + + # Create a formula field + field = CustomField( + id="field-self-ref", + project_id=project.id, + name="self_ref_field", + field_type="formula", + formula="{self_ref_field} + 1", # References itself + position=0, + ) + db.add(field) + db.commit() + + # Validate the formula + is_valid, error_msg, cycle_path = FormulaService.validate_formula_with_details( + "{self_ref_field} + 1", project.id, db, field.id + ) + + assert is_valid is False + assert "self_ref_field" in error_msg or (cycle_path and "self_ref_field" in cycle_path) + + def test_formula_indirect_cycle_detected(self, db): + """Test detection of indirect cycle: A -> B -> A.""" + project = self.setup_project_with_fields(db, "proj-formula-2", "space-formula-2") + + # Create field B that references field A + field_a = CustomField( + id="field-a-f2", + project_id=project.id, + name="field_a", + field_type="number", + position=0, + ) + db.add(field_a) + + field_b = CustomField( + id="field-b-f2", + project_id=project.id, + name="field_b", + field_type="formula", + formula="{field_a} * 2", + position=1, + ) + db.add(field_b) + db.commit() + + # Now try to update field_a to reference field_b (would create cycle) + field_a.field_type = "formula" + field_a.formula = "{field_b} + 1" + db.commit() + + is_valid, error_msg, cycle_path = FormulaService.validate_formula_with_details( + "{field_b} + 1", project.id, db, field_a.id + ) + + assert is_valid is False + assert "Circular" in error_msg or (cycle_path is not None and len(cycle_path) > 0) + + def test_formula_long_cycle_detected(self, db): + """Test detection of longer cycle: A -> B -> C -> A.""" + project = self.setup_project_with_fields(db, "proj-formula-3", "space-formula-3") + + # Create a chain: field_a (number), field_b = {field_a}, field_c = {field_b} + field_a = CustomField( + id="field-a-f3", + project_id=project.id, + name="field_a", + field_type="number", + position=0, + ) + field_b = CustomField( + id="field-b-f3", + project_id=project.id, + name="field_b", + field_type="formula", + formula="{field_a} * 2", + position=1, + ) + field_c = CustomField( + id="field-c-f3", + project_id=project.id, + name="field_c", + field_type="formula", + formula="{field_b} + 10", + position=2, + ) + db.add_all([field_a, field_b, field_c]) + db.commit() + + # Now try to make field_a reference field_c (would create cycle) + field_a.field_type = "formula" + field_a.formula = "{field_c} / 2" + db.commit() + + is_valid, error_msg, cycle_path = FormulaService.validate_formula_with_details( + "{field_c} / 2", project.id, db, field_a.id + ) + + assert is_valid is False + # Should have a cycle path + if cycle_path: + assert len(cycle_path) >= 3 + + def test_valid_formula_chain_accepted(self, db): + """Test that valid formula chains are accepted.""" + project = self.setup_project_with_fields(db, "proj-formula-4", "space-formula-4") + + # Create valid chain: field_a (number), field_b = {field_a} + field_a = CustomField( + id="field-a-f4", + project_id=project.id, + name="field_a", + field_type="number", + position=0, + ) + db.add(field_a) + db.commit() + + # Validate formula for field_b referencing field_a + is_valid, error_msg, cycle_path = FormulaService.validate_formula_with_details( + "{field_a} * 2", project.id, db + ) + + assert is_valid is True + assert error_msg is None + assert cycle_path is None + + def test_builtin_fields_not_cause_cycle(self, db): + """Test that builtin fields don't cause false cycle detection.""" + project = self.setup_project_with_fields(db, "proj-formula-5", "space-formula-5") + + # Create formula using builtin fields + field = CustomField( + id="field-builtin-f5", + project_id=project.id, + name="progress", + field_type="formula", + formula="{time_spent} / {original_estimate} * 100", + position=0, + ) + db.add(field) + db.commit() + + is_valid, error_msg, cycle_path = FormulaService.validate_formula_with_details( + "{time_spent} / {original_estimate} * 100", project.id, db, field.id + ) + + assert is_valid is True + + +class TestCycleDetectionInGraph: + """Test cycle detection in existing graphs.""" + + def test_detect_cycles_in_graph_finds_existing_cycle(self, db): + """Test that detect_cycles_in_graph finds existing cycles.""" + # Create project + space = Space( + id="space-graph-1", + name="Test Space", + owner_id="00000000-0000-0000-0000-000000000001", + ) + db.add(space) + + project = Project( + id="proj-graph-1", + space_id="space-graph-1", + title="Test Project", + owner_id="00000000-0000-0000-0000-000000000001", + security_level="public", + ) + db.add(project) + + status = TaskStatus( + id="status-graph-1", + project_id="proj-graph-1", + name="To Do", + color="#808080", + position=0, + ) + db.add(status) + + # Create tasks + task_a = Task( + id="task-a-graph", + project_id="proj-graph-1", + title="Task A", + priority="medium", + created_by="00000000-0000-0000-0000-000000000001", + status_id="status-graph-1", + ) + task_b = Task( + id="task-b-graph", + project_id="proj-graph-1", + title="Task B", + priority="medium", + created_by="00000000-0000-0000-0000-000000000001", + status_id="status-graph-1", + ) + db.add_all([task_a, task_b]) + + # Manually create a cycle (bypassing validation for testing) + dep_ab = TaskDependency( + id="dep-ab-graph", + predecessor_id="task-a-graph", + successor_id="task-b-graph", + dependency_type="FS", + lag_days=0, + ) + dep_ba = TaskDependency( + id="dep-ba-graph", + predecessor_id="task-b-graph", + successor_id="task-a-graph", + dependency_type="FS", + lag_days=0, + ) + db.add_all([dep_ab, dep_ba]) + db.commit() + + # Detect cycles + cycles = DependencyService.detect_cycles_in_graph(db, "proj-graph-1") + + assert len(cycles) > 0 + assert cycles[0].has_cycle is True + + def test_detect_cycles_in_graph_empty_when_no_cycles(self, db): + """Test that detect_cycles_in_graph returns empty when no cycles.""" + # Create project + space = Space( + id="space-graph-2", + name="Test Space", + owner_id="00000000-0000-0000-0000-000000000001", + ) + db.add(space) + + project = Project( + id="proj-graph-2", + space_id="space-graph-2", + title="Test Project", + owner_id="00000000-0000-0000-0000-000000000001", + security_level="public", + ) + db.add(project) + + status = TaskStatus( + id="status-graph-2", + project_id="proj-graph-2", + name="To Do", + color="#808080", + position=0, + ) + db.add(status) + + # Create tasks with valid chain + task_a = Task( + id="task-a-graph-2", + project_id="proj-graph-2", + title="Task A", + priority="medium", + created_by="00000000-0000-0000-0000-000000000001", + status_id="status-graph-2", + ) + task_b = Task( + id="task-b-graph-2", + project_id="proj-graph-2", + title="Task B", + priority="medium", + created_by="00000000-0000-0000-0000-000000000001", + status_id="status-graph-2", + ) + task_c = Task( + id="task-c-graph-2", + project_id="proj-graph-2", + title="Task C", + priority="medium", + created_by="00000000-0000-0000-0000-000000000001", + status_id="status-graph-2", + ) + db.add_all([task_a, task_b, task_c]) + + # Create valid chain A -> B -> C + dep_ab = TaskDependency( + id="dep-ab-graph-2", + predecessor_id="task-a-graph-2", + successor_id="task-b-graph-2", + dependency_type="FS", + lag_days=0, + ) + dep_bc = TaskDependency( + id="dep-bc-graph-2", + predecessor_id="task-b-graph-2", + successor_id="task-c-graph-2", + dependency_type="FS", + lag_days=0, + ) + db.add_all([dep_ab, dep_bc]) + db.commit() + + # Detect cycles + cycles = DependencyService.detect_cycles_in_graph(db, "proj-graph-2") + + assert len(cycles) == 0 + + +class TestCycleDetectionResultClass: + """Test CycleDetectionResult class methods.""" + + def test_cycle_detection_result_no_cycle(self): + """Test CycleDetectionResult when no cycle.""" + result = CycleDetectionResult(has_cycle=False) + assert result.has_cycle is False + assert result.cycle_path == [] + assert result.get_cycle_description() == "" + + def test_cycle_detection_result_with_cycle(self): + """Test CycleDetectionResult when cycle exists.""" + result = CycleDetectionResult( + has_cycle=True, + cycle_path=["task-a", "task-b", "task-a"], + cycle_task_titles=["Task A", "Task B", "Task A"] + ) + assert result.has_cycle is True + assert result.cycle_path == ["task-a", "task-b", "task-a"] + description = result.get_cycle_description() + assert "Task A" in description + assert "Task B" in description + assert " -> " in description + + +class TestCircularReferenceErrorClass: + """Test CircularReferenceError class methods.""" + + def test_circular_reference_error_with_path(self): + """Test CircularReferenceError with cycle path.""" + error = CircularReferenceError( + "Test error", + cycle_path=["field_a", "field_b", "field_a"] + ) + assert error.message == "Test error" + assert error.cycle_path == ["field_a", "field_b", "field_a"] + description = error.get_cycle_description() + assert "field_a" in description + assert "field_b" in description + assert " -> " in description + + def test_circular_reference_error_without_path(self): + """Test CircularReferenceError without cycle path.""" + error = CircularReferenceError("Test error") + assert error.message == "Test error" + assert error.cycle_path == [] + assert error.get_cycle_description() == "" diff --git a/backend/tests/test_input_validation.py b/backend/tests/test_input_validation.py new file mode 100644 index 0000000..5e042c2 --- /dev/null +++ b/backend/tests/test_input_validation.py @@ -0,0 +1,291 @@ +""" +Tests for input validation and security enhancements. + +Tests cover: +- Schema input validation (max_length, numeric ranges) +- Path traversal prevention +- WebSocket authentication flow +""" + +import os +os.environ["TESTING"] = "true" + +import pytest +from pydantic import ValidationError +from app.schemas.task import TaskCreate, TaskUpdate, TaskBase +from app.schemas.project import ProjectCreate +from app.schemas.space import SpaceCreate +from app.schemas.comment import CommentCreate + + +class TestSchemaInputValidation: + """Test input validation for schemas.""" + + def test_task_title_max_length(self): + """Test task title max length validation (500 chars).""" + # Valid title + valid_task = TaskCreate(title="A" * 500) + assert len(valid_task.title) == 500 + + # Invalid - too long + with pytest.raises(ValidationError) as exc_info: + TaskCreate(title="A" * 501) + assert "String should have at most 500 characters" in str(exc_info.value) + + def test_task_title_min_length(self): + """Test task title min length validation (1 char).""" + # Valid - single char + valid_task = TaskCreate(title="A") + assert valid_task.title == "A" + + # Invalid - empty string + with pytest.raises(ValidationError) as exc_info: + TaskCreate(title="") + assert "String should have at least 1 character" in str(exc_info.value) + + def test_task_description_max_length(self): + """Test task description max length validation (10000 chars).""" + # Valid description + valid_task = TaskCreate(title="Test", description="A" * 10000) + assert len(valid_task.description) == 10000 + + # Invalid - too long + with pytest.raises(ValidationError) as exc_info: + TaskCreate(title="Test", description="A" * 10001) + assert "String should have at most 10000 characters" in str(exc_info.value) + + def test_task_original_estimate_range(self): + """Test original_estimate numeric range validation.""" + from decimal import Decimal + + # Valid values + task_zero = TaskCreate(title="Test", original_estimate=Decimal("0")) + assert task_zero.original_estimate == Decimal("0") + + task_max = TaskCreate(title="Test", original_estimate=Decimal("99999")) + assert task_max.original_estimate == Decimal("99999") + + # Invalid - negative + with pytest.raises(ValidationError) as exc_info: + TaskCreate(title="Test", original_estimate=Decimal("-1")) + assert "greater than or equal to 0" in str(exc_info.value) + + # Invalid - too large + with pytest.raises(ValidationError) as exc_info: + TaskCreate(title="Test", original_estimate=Decimal("100000")) + assert "less than or equal to 99999" in str(exc_info.value) + + def test_task_update_version_validation(self): + """Test version field validation for optimistic locking.""" + # Valid version + update = TaskUpdate(version=1) + assert update.version == 1 + + # Invalid - version 0 + with pytest.raises(ValidationError) as exc_info: + TaskUpdate(version=0) + assert "greater than or equal to 1" in str(exc_info.value) + + def test_task_position_validation(self): + """Test position field validation.""" + # Valid position + update = TaskUpdate(position=0) + assert update.position == 0 + + # Invalid - negative position + with pytest.raises(ValidationError) as exc_info: + TaskUpdate(position=-1) + assert "greater than or equal to 0" in str(exc_info.value) + + +class TestPathTraversalSecurity: + """Test path traversal prevention in file storage.""" + + def test_path_traversal_detection_in_component(self): + """Test that path traversal attempts in components are detected.""" + from app.services.file_storage_service import FileStorageService, PathTraversalError + + service = FileStorageService() + + # These should raise security exceptions + malicious_components = [ + "../../../etc/passwd", + "..\\..\\windows", + "foo/../bar", + "test/../../secret", + ] + + for component in malicious_components: + with pytest.raises(PathTraversalError) as exc_info: + service._validate_path_component(component, "test_component") + assert "path traversal" in str(exc_info.value).lower() or "invalid" in str(exc_info.value).lower() + + def test_path_component_starting_with_dot(self): + """Test that components starting with '.' are rejected.""" + from app.services.file_storage_service import FileStorageService, PathTraversalError + + service = FileStorageService() + + with pytest.raises(PathTraversalError): + service._validate_path_component(".hidden", "test") + + with pytest.raises(PathTraversalError): + service._validate_path_component("..parent", "test") + + def test_valid_path_components_allowed(self): + """Test that valid path components are allowed.""" + from app.services.file_storage_service import FileStorageService + + service = FileStorageService() + + # These should be valid + valid_components = [ + "project-123", + "task_456", + "attachment789", + "uuid-like-string", + ] + + for component in valid_components: + # Should not raise + service._validate_path_component(component, "test") + + def test_path_in_base_dir_validation(self): + """Test that paths outside base dir are rejected.""" + from app.services.file_storage_service import FileStorageService, PathTraversalError + from pathlib import Path + + service = FileStorageService() + + # Try to access path outside base directory + outside_path = Path("/etc/passwd") + + with pytest.raises(PathTraversalError): + service._validate_path_in_base_dir(outside_path, "test") + + +class TestWebSocketAuthentication: + """Test WebSocket authentication flow.""" + + def test_websocket_requires_auth(self, client): + """Test that WebSocket connection requires authentication.""" + # Try to connect without sending auth message + with pytest.raises(Exception): + with client.websocket_connect("/ws/projects/test-project") as websocket: + # Should receive error or disconnect without auth + data = websocket.receive_json() + assert data.get("type") == "error" or "auth" in str(data).lower() + + def test_websocket_auth_with_valid_token(self, client, admin_token, db): + """Test WebSocket connection with valid token in first message.""" + from app.models import Space, Project + + # Create test project + space = Space( + id="test-space-id", + name="Test Space", + owner_id="00000000-0000-0000-0000-000000000001" + ) + db.add(space) + + project = Project( + id="test-project-id", + name="Test Project", + space_id="test-space-id", + owner_id="00000000-0000-0000-0000-000000000001" + ) + db.add(project) + db.commit() + + # Connect and authenticate + with client.websocket_connect("/ws/projects/test-project-id") as websocket: + # Send auth message first + websocket.send_json({ + "type": "auth", + "token": admin_token + }) + + # Should receive acknowledgment + response = websocket.receive_json() + assert response.get("type") in ["authenticated", "sync", "error"] or "connected" in str(response).lower() + + def test_websocket_auth_with_invalid_token(self, client, db): + """Test WebSocket connection with invalid token is rejected.""" + from app.models import Space, Project + + # Create test project + space = Space( + id="test-space-id-2", + name="Test Space 2", + owner_id="00000000-0000-0000-0000-000000000001" + ) + db.add(space) + + project = Project( + id="test-project-id-2", + name="Test Project 2", + space_id="test-space-id-2", + owner_id="00000000-0000-0000-0000-000000000001" + ) + db.add(project) + db.commit() + + with client.websocket_connect("/ws/projects/test-project-id-2") as websocket: + # Send auth message with invalid token + websocket.send_json({ + "type": "auth", + "token": "invalid-token-12345" + }) + + # Should receive error + response = websocket.receive_json() + assert response.get("type") == "error" or "invalid" in str(response).lower() or "unauthorized" in str(response).lower() + + +class TestInputValidationEdgeCases: + """Test edge cases for input validation.""" + + def test_unicode_in_title(self): + """Test that unicode characters are handled correctly.""" + # Chinese characters + task = TaskCreate(title="測試任務 🎉") + assert task.title == "測試任務 🎉" + + # Japanese + task = TaskCreate(title="テストタスク") + assert task.title == "テストタスク" + + # Emojis + task = TaskCreate(title="Task with emojis 👍🏻✅🚀") + assert "👍" in task.title + + def test_whitespace_handling(self): + """Test whitespace handling in title.""" + # Title with only whitespace should fail min_length + with pytest.raises(ValidationError): + TaskCreate(title=" ") # Spaces only, but length > 0 + + def test_special_characters_in_description(self): + """Test special characters in description.""" + special_desc = "\n\t\"quotes\" 'apostrophe'" + task = TaskCreate(title="Test", description=special_desc) + assert task.description == special_desc # Should store as-is, sanitize on output + + def test_decimal_precision(self): + """Test decimal precision for estimates.""" + from decimal import Decimal + + task = TaskCreate(title="Test", original_estimate=Decimal("123.456789")) + assert task.original_estimate == Decimal("123.456789") + + def test_none_optional_fields(self): + """Test that optional fields accept None.""" + task = TaskCreate( + title="Test", + description=None, + original_estimate=None, + start_date=None, + due_date=None + ) + assert task.description is None + assert task.original_estimate is None diff --git a/backend/tests/test_permission_enhancements.py b/backend/tests/test_permission_enhancements.py new file mode 100644 index 0000000..1aa8144 --- /dev/null +++ b/backend/tests/test_permission_enhancements.py @@ -0,0 +1,286 @@ +"""Tests for permission enhancements. + +Tests for: +1. Manager workload access - department managers can view subordinate workloads +2. Cross-department project access via project membership +""" +import pytest +from unittest.mock import MagicMock + +from app.middleware.auth import check_project_access, check_project_edit_access + + +# ============================================================================ +# Test Helpers +# ============================================================================ + +def get_mock_user( + user_id="test-user-id", + is_admin=False, + is_department_manager=False, + department_id="dept-1", +): + """Create a mock user for testing.""" + user = MagicMock() + user.id = user_id + user.is_system_admin = is_admin + user.is_department_manager = is_department_manager + user.department_id = department_id + return user + + +def get_mock_project_member(user_id, role="member"): + """Create a mock project member.""" + member = MagicMock() + member.user_id = user_id + member.role = role + return member + + +def get_mock_project( + owner_id="owner-id", + security_level="department", + department_id="dept-1", + members=None, +): + """Create a mock project for testing.""" + project = MagicMock() + project.id = "project-id" + project.owner_id = owner_id + project.security_level = security_level + project.department_id = department_id + project.members = members or [] + return project + + +# ============================================================================ +# Test Manager Workload Access +# ============================================================================ + +class TestManagerWorkloadAccess: + """Test that department managers can view subordinate workloads.""" + + def test_manager_flag_exists_on_user(self): + """Test that is_department_manager flag exists on mock user.""" + manager = get_mock_user(is_department_manager=True) + assert manager.is_department_manager == True + + regular_user = get_mock_user(is_department_manager=False) + assert regular_user.is_department_manager == False + + def test_system_admin_can_view_all_workloads(self): + """Test that system admin can view any user's workload.""" + from app.api.workload.router import check_workload_access + + admin = get_mock_user(is_admin=True) + + # Should not raise for any target user + check_workload_access(admin, target_user_id="any-user-id") + check_workload_access(admin, department_id="any-dept") + + def test_manager_can_view_same_department_workload(self): + """Test that manager can view workload of users in their department.""" + from app.api.workload.router import check_workload_access + + manager = get_mock_user( + is_department_manager=True, + department_id="dept-1" + ) + + # Manager can view workload of user in same department + check_workload_access( + manager, + target_user_id="subordinate-user-id", + target_user_department_id="dept-1" + ) + + def test_manager_cannot_view_other_department_workload(self): + """Test that manager cannot view workload of users in other departments.""" + from app.api.workload.router import check_workload_access + from fastapi import HTTPException + + manager = get_mock_user( + is_department_manager=True, + department_id="dept-1" + ) + + # Manager cannot view workload of user in different department + with pytest.raises(HTTPException) as exc_info: + check_workload_access( + manager, + target_user_id="other-dept-user-id", + target_user_department_id="dept-2" + ) + + assert exc_info.value.status_code == 403 + + def test_regular_user_can_only_view_own_workload(self): + """Test that regular users can only view their own workload.""" + from app.api.workload.router import check_workload_access + from fastapi import HTTPException + + user = get_mock_user( + user_id="user-123", + is_department_manager=False + ) + + # User can view their own workload + check_workload_access(user, target_user_id="user-123") + + # User cannot view others' workload + with pytest.raises(HTTPException) as exc_info: + check_workload_access(user, target_user_id="other-user") + + assert exc_info.value.status_code == 403 + + +# ============================================================================ +# Test Cross-Department Project Access via Membership +# ============================================================================ + +class TestProjectMemberAccess: + """Test that project members have access regardless of department.""" + + def test_project_member_has_access(self): + """Test that project member can access project from different department.""" + user = get_mock_user(user_id="member-user", department_id="dept-2") + + # Project is in dept-1 but user from dept-2 is a member + member = get_mock_project_member(user_id="member-user", role="member") + project = get_mock_project( + security_level="department", + department_id="dept-1", + members=[member], + ) + + assert check_project_access(user, project) == True + + def test_non_member_from_different_dept_denied(self): + """Test that non-member from different department is denied access.""" + user = get_mock_user(user_id="outsider", department_id="dept-2") + + project = get_mock_project( + security_level="department", + department_id="dept-1", + members=[], # No members + ) + + assert check_project_access(user, project) == False + + def test_member_access_confidential_project(self): + """Test that members can access confidential projects.""" + user = get_mock_user(user_id="member-user", department_id="dept-2") + + member = get_mock_project_member(user_id="member-user", role="member") + project = get_mock_project( + owner_id="owner-id", # User is not owner + security_level="confidential", + department_id="dept-1", + members=[member], + ) + + # Member should have access even to confidential project + assert check_project_access(user, project) == True + + def test_member_with_admin_role_can_edit(self): + """Test that project member with admin role can edit project.""" + user = get_mock_user(user_id="admin-member", department_id="dept-2") + + member = get_mock_project_member(user_id="admin-member", role="admin") + project = get_mock_project( + owner_id="owner-id", # User is not owner + security_level="department", + members=[member], + ) + + assert check_project_edit_access(user, project) == True + + def test_member_with_member_role_cannot_edit(self): + """Test that project member with member role cannot edit project.""" + user = get_mock_user(user_id="regular-member", department_id="dept-2") + + member = get_mock_project_member(user_id="regular-member", role="member") + project = get_mock_project( + owner_id="owner-id", # User is not owner + security_level="department", + members=[member], + ) + + assert check_project_edit_access(user, project) == False + + def test_owner_can_still_edit(self): + """Test that project owner can edit regardless of members.""" + user = get_mock_user(user_id="owner-id") + + project = get_mock_project( + owner_id="owner-id", + security_level="confidential", + members=[], + ) + + assert check_project_access(user, project) == True + assert check_project_edit_access(user, project) == True + + +# ============================================================================ +# Test Filter Accessible Users for Manager +# ============================================================================ + +class TestFilterAccessibleUsersForManager: + """Test the filter_accessible_users function for managers.""" + + def test_admin_can_see_all_users(self): + """Test that admin can see all users.""" + from app.api.workload.router import filter_accessible_users + + admin = get_mock_user(is_admin=True) + + # Admin with no filter gets None (means all users) + result = filter_accessible_users(admin, None, None) + assert result is None + + # Admin with specific users gets those users + result = filter_accessible_users(admin, ["user1", "user2"], None) + assert result == ["user1", "user2"] + + def test_regular_user_sees_only_self(self): + """Test that regular user can only see themselves.""" + from app.api.workload.router import filter_accessible_users + + user = get_mock_user(user_id="user-123", is_department_manager=False) + + # Regular user with no filter gets only self + result = filter_accessible_users(user, None, None) + assert result == ["user-123"] + + # Regular user with other users gets only self + result = filter_accessible_users(user, ["user1", "user2", "user-123"], None) + assert result == ["user-123"] + + +class TestAccessDeniedForNonManagersAndNonMembers: + """Test that access is properly denied for unauthorized users.""" + + def test_non_manager_cannot_view_subordinate_workload(self): + """Test that non-manager cannot view other users' workload.""" + from app.api.workload.router import check_workload_access + from fastapi import HTTPException + + user = get_mock_user(is_department_manager=False) + + with pytest.raises(HTTPException) as exc_info: + check_workload_access(user, target_user_id="other-user") + + assert exc_info.value.status_code == 403 + + def test_non_member_cannot_access_department_project(self): + """Test that non-member from different department cannot access.""" + user = get_mock_user(department_id="dept-2") + + project = get_mock_project( + security_level="department", + department_id="dept-1", + members=[], + ) + + assert check_project_access(user, project) == False diff --git a/backend/tests/test_rate_limit.py b/backend/tests/test_rate_limit.py index 36bbe08..1425d55 100644 --- a/backend/tests/test_rate_limit.py +++ b/backend/tests/test_rate_limit.py @@ -1,8 +1,14 @@ """ Test suite for rate limiting functionality. -Tests the rate limiting feature on the login endpoint to ensure -protection against brute force attacks. +Tests the rate limiting feature on various endpoints to ensure +protection against brute force attacks and DoS attempts. + +Rate Limit Tiers: +- Standard (60/minute): Task CRUD, comments +- Sensitive (20/minute): Attachments, report exports +- Heavy (5/minute): Report generation, bulk operations +- Login (5/minute): Authentication """ import pytest @@ -11,7 +17,7 @@ from unittest.mock import patch, MagicMock, AsyncMock from app.services.auth_client import AuthAPIError -class TestRateLimiting: +class TestLoginRateLimiting: """Test rate limiting on the login endpoint.""" def test_login_rate_limit_exceeded(self, client): @@ -122,3 +128,120 @@ class TestRateLimiterConfiguration: # The key function should be get_remote_address assert limiter._key_func == get_remote_address + + def test_rate_limit_tiers_configured(self): + """ + Test that rate limit tiers are properly configured. + + GIVEN the settings configuration + WHEN we check the rate limit tier values + THEN they should match the expected defaults + """ + from app.core.config import settings + + # Standard tier: 60/minute + assert settings.RATE_LIMIT_STANDARD == "60/minute" + + # Sensitive tier: 20/minute + assert settings.RATE_LIMIT_SENSITIVE == "20/minute" + + # Heavy tier: 5/minute + assert settings.RATE_LIMIT_HEAVY == "5/minute" + + def test_rate_limit_helper_functions(self): + """ + Test that rate limit helper functions return correct values. + + GIVEN the rate limiter module + WHEN we call the helper functions + THEN they should return the configured rate limit strings + """ + from app.core.rate_limiter import ( + get_rate_limit_standard, + get_rate_limit_sensitive, + get_rate_limit_heavy + ) + + assert get_rate_limit_standard() == "60/minute" + assert get_rate_limit_sensitive() == "20/minute" + assert get_rate_limit_heavy() == "5/minute" + + +class TestRateLimitHeaders: + """Test rate limit headers in responses.""" + + def test_rate_limit_headers_present(self, client): + """ + Test that rate limit headers are included in responses. + + GIVEN a rate-limited endpoint + WHEN a request is made + THEN the response includes X-RateLimit-* headers + """ + with patch("app.api.auth.router.verify_credentials", new_callable=AsyncMock) as mock_verify: + mock_verify.side_effect = AuthAPIError("Invalid credentials") + + login_data = {"email": "test@example.com", "password": "wrongpassword"} + response = client.post("/api/auth/login", json=login_data) + + # Check that rate limit headers are present + # Note: slowapi uses these header names when headers_enabled=True + headers = response.headers + + # The exact header names depend on slowapi version + # Common patterns: X-RateLimit-Limit, X-RateLimit-Remaining, X-RateLimit-Reset + # or: RateLimit-Limit, RateLimit-Remaining, RateLimit-Reset + rate_limit_headers = [ + key for key in headers.keys() + if "ratelimit" in key.lower() or "rate-limit" in key.lower() + ] + + # At minimum, we should have rate limit information in headers + # when the limiter has headers_enabled=True + assert len(rate_limit_headers) > 0 or response.status_code == 401, \ + "Rate limit headers should be present in response" + + +class TestEndpointRateLimits: + """Test rate limits on specific endpoint categories.""" + + def test_rate_limit_tier_values_are_valid(self): + """ + Test that rate limit tier values are in valid format. + + GIVEN the rate limit configuration + WHEN we validate the format + THEN all values should be in "{number}/{period}" format + """ + from app.core.config import settings + import re + + pattern = r"^\d+/(second|minute|hour|day)$" + + assert re.match(pattern, settings.RATE_LIMIT_STANDARD), \ + f"Invalid format: {settings.RATE_LIMIT_STANDARD}" + assert re.match(pattern, settings.RATE_LIMIT_SENSITIVE), \ + f"Invalid format: {settings.RATE_LIMIT_SENSITIVE}" + assert re.match(pattern, settings.RATE_LIMIT_HEAVY), \ + f"Invalid format: {settings.RATE_LIMIT_HEAVY}" + + def test_rate_limit_ordering(self): + """ + Test that rate limit tiers are ordered correctly. + + GIVEN the rate limit configuration + WHEN we compare the limits + THEN heavy < sensitive < standard + """ + from app.core.config import settings + + def extract_limit(rate_str): + """Extract numeric limit from rate string like '60/minute'.""" + return int(rate_str.split("/")[0]) + + standard_limit = extract_limit(settings.RATE_LIMIT_STANDARD) + sensitive_limit = extract_limit(settings.RATE_LIMIT_SENSITIVE) + heavy_limit = extract_limit(settings.RATE_LIMIT_HEAVY) + + assert heavy_limit < sensitive_limit < standard_limit, \ + f"Rate limits should be ordered: heavy({heavy_limit}) < sensitive({sensitive_limit}) < standard({standard_limit})" diff --git a/frontend/public/locales/en/common.json b/frontend/public/locales/en/common.json index f9f0402..d17a59d 100644 --- a/frontend/public/locales/en/common.json +++ b/frontend/public/locales/en/common.json @@ -53,7 +53,10 @@ "searchUsers": "Search users...", "noUsersFound": "No users found", "typeToSearch": "Type to search users", - "task": "Task" + "task": "Task", + "admin": "Admin", + "live": "Live", + "offline": "Offline" }, "messages": { "success": "Operation successful", @@ -81,7 +84,9 @@ "health": "Project Health", "audit": "Audit Log", "settings": "Settings", - "logout": "Logout" + "logout": "Logout", + "toggleMenu": "Toggle Menu", + "menu": "Menu" }, "language": { "switch": "Switch language", diff --git a/frontend/public/locales/en/projects.json b/frontend/public/locales/en/projects.json index 1e2d85b..9e69fb6 100644 --- a/frontend/public/locales/en/projects.json +++ b/frontend/public/locales/en/projects.json @@ -64,5 +64,16 @@ "empty": { "title": "No Projects", "description": "Create your first project to start managing tasks" + }, + "template": { + "label": "Template", + "selectTemplate": "Select a template", + "blankProject": "Blank Project", + "blankProjectDescription": "Start with a clean slate", + "loadingTemplates": "Loading templates...", + "loadFailed": "Failed to load templates", + "statusCount": "{{count}} statuses", + "fieldCount": "{{count}} custom fields", + "publicTemplate": "Public" } } diff --git a/frontend/public/locales/en/settings.json b/frontend/public/locales/en/settings.json index 1ade65d..f225797 100644 --- a/frontend/public/locales/en/settings.json +++ b/frontend/public/locales/en/settings.json @@ -39,13 +39,39 @@ }, "members": { "title": "Member Management", + "description": "Manage users who can access this project. Project members can view and edit project content.", + "addMember": "Add Member", "invite": "Invite Member", "inviteByEmail": "Invite by email", "emailPlaceholder": "Enter email address", + "selectUser": "Select User", + "searchUserPlaceholder": "Search users...", + "user": "User", "role": "Role", + "joinedAt": "Joined", "changeRole": "Change Role", - "remove": "Remove Member", - "confirmRemove": "Are you sure you want to remove this member?" + "remove": "Remove", + "confirmRemove": "Are you sure you want to remove this member?", + "removeConfirmTitle": "Remove Member", + "removeConfirmMessage": "Are you sure you want to remove {{name}} from this project? They will no longer have access.", + "empty": "No members in this project yet.", + "emptyHint": "Click \"Add Member\" to add project members.", + "loadError": "Failed to load member list", + "addError": "Failed to add member", + "removeError": "Failed to remove member", + "roleChangeError": "Failed to change role", + "memberAdded": "Member added successfully", + "adding": "Adding...", + "selectUserRequired": "Please select a user to add", + "alreadyMember": "This user is already a project member", + "roles": { + "member": "Member", + "admin": "Admin" + }, + "roleHelp": { + "member": "Members can view and edit tasks in this project.", + "admin": "Admins can manage project settings and members." + } }, "customFields": { "title": "Custom Fields", @@ -104,6 +130,12 @@ }, "validation": { "nameRequired": "Field name is required" + }, + "circularError": { + "title": "Circular Reference Detected", + "description": "This formula creates a circular reference, which is not allowed.", + "cyclePath": "Reference Cycle", + "helpText": "To resolve this issue, modify the formula to avoid referencing fields that directly or indirectly reference this field." } }, "notifications": { diff --git a/frontend/public/locales/en/tasks.json b/frontend/public/locales/en/tasks.json index b09f9e6..92d9a96 100644 --- a/frontend/public/locales/en/tasks.json +++ b/frontend/public/locales/en/tasks.json @@ -181,5 +181,33 @@ "title": "No Tasks", "description": "There are no tasks yet. Create your first task to get started!", "filtered": "No tasks match your filters" + }, + "dependencies": { + "title": "Task Dependencies", + "add": "Add Dependency", + "remove": "Remove Dependency", + "predecessor": "Predecessor", + "successor": "Successor", + "type": "Dependency Type", + "types": { + "FS": "Finish-to-Start", + "SS": "Start-to-Start", + "FF": "Finish-to-Finish", + "SF": "Start-to-Finish" + }, + "circularError": { + "title": "Circular Dependency Detected", + "description": "Adding this dependency would create a circular reference, which is not allowed.", + "cyclePath": "Dependency Cycle", + "helpText": "To resolve this issue, choose a different task as a dependency, or remove one of the existing dependencies in the cycle." + }, + "error": { + "addFailed": "Failed to add dependency", + "removeFailed": "Failed to remove dependency" + } + }, + "conflict": { + "title": "Update Conflict", + "message": "This task has been modified by another user. Please refresh to see the latest version and try again." } } diff --git a/frontend/public/locales/zh-TW/common.json b/frontend/public/locales/zh-TW/common.json index 71f80d5..86583e6 100644 --- a/frontend/public/locales/zh-TW/common.json +++ b/frontend/public/locales/zh-TW/common.json @@ -53,7 +53,10 @@ "searchUsers": "搜尋使用者...", "noUsersFound": "找不到使用者", "typeToSearch": "輸入以搜尋使用者", - "task": "任務" + "task": "任務", + "admin": "管理員", + "live": "即時", + "offline": "離線" }, "messages": { "success": "操作成功", @@ -81,7 +84,9 @@ "health": "專案健康度", "audit": "稽核日誌", "settings": "設定", - "logout": "登出" + "logout": "登出", + "toggleMenu": "切換選單", + "menu": "選單" }, "language": { "switch": "切換語言", diff --git a/frontend/public/locales/zh-TW/projects.json b/frontend/public/locales/zh-TW/projects.json index b340450..1f9c46d 100644 --- a/frontend/public/locales/zh-TW/projects.json +++ b/frontend/public/locales/zh-TW/projects.json @@ -64,5 +64,16 @@ "empty": { "title": "沒有專案", "description": "建立您的第一個專案來開始管理任務" + }, + "template": { + "label": "模板", + "selectTemplate": "選擇模板", + "blankProject": "空白專案", + "blankProjectDescription": "從頭開始建立", + "loadingTemplates": "載入模板中...", + "loadFailed": "載入模板失敗", + "statusCount": "{{count}} 個狀態", + "fieldCount": "{{count}} 個自訂欄位", + "publicTemplate": "公開" } } diff --git a/frontend/public/locales/zh-TW/settings.json b/frontend/public/locales/zh-TW/settings.json index c72b25d..4a7a905 100644 --- a/frontend/public/locales/zh-TW/settings.json +++ b/frontend/public/locales/zh-TW/settings.json @@ -39,13 +39,39 @@ }, "members": { "title": "成員管理", + "description": "管理可以存取此專案的使用者。專案成員可以檢視和編輯專案內容。", + "addMember": "新增成員", "invite": "邀請成員", "inviteByEmail": "透過電子郵件邀請", "emailPlaceholder": "輸入電子郵件地址", + "selectUser": "選擇使用者", + "searchUserPlaceholder": "搜尋使用者...", + "user": "使用者", "role": "角色", + "joinedAt": "加入時間", "changeRole": "變更角色", - "remove": "移除成員", - "confirmRemove": "確定要移除此成員嗎？" + "remove": "移除", + "confirmRemove": "確定要移除此成員嗎？", + "removeConfirmTitle": "移除成員", + "removeConfirmMessage": "確定要將 {{name}} 從此專案移除嗎？移除後該成員將無法存取此專案。", + "empty": "此專案尚無成員。", + "emptyHint": "點擊「新增成員」來添加專案成員。", + "loadError": "載入成員列表失敗", + "addError": "新增成員失敗", + "removeError": "移除成員失敗", + "roleChangeError": "變更角色失敗", + "memberAdded": "成員已新增", + "adding": "新增中...", + "selectUserRequired": "請選擇要新增的使用者", + "alreadyMember": "此使用者已經是專案成員", + "roles": { + "member": "成員", + "admin": "管理員" + }, + "roleHelp": { + "member": "成員可以檢視和編輯專案中的任務。", + "admin": "管理員可以管理專案設定和成員。" + } }, "customFields": { "title": "自訂欄位", @@ -104,6 +130,12 @@ }, "validation": { "nameRequired": "欄位名稱為必填" + }, + "circularError": { + "title": "偵測到循環參照", + "description": "此公式會產生循環參照，這是不被允許的。", + "cyclePath": "參照循環路徑", + "helpText": "要解決此問題，請修改公式以避免參照直接或間接參照此欄位的其他欄位。" } }, "notifications": { diff --git a/frontend/public/locales/zh-TW/tasks.json b/frontend/public/locales/zh-TW/tasks.json index a74beed..4380b26 100644 --- a/frontend/public/locales/zh-TW/tasks.json +++ b/frontend/public/locales/zh-TW/tasks.json @@ -181,5 +181,33 @@ "title": "沒有任務", "description": "目前沒有任務。建立您的第一個任務開始吧！", "filtered": "沒有符合篩選條件的任務" + }, + "dependencies": { + "title": "任務相依性", + "add": "新增相依性", + "remove": "移除相依性", + "predecessor": "前置任務", + "successor": "後續任務", + "type": "相依性類型", + "types": { + "FS": "完成後開始", + "SS": "同時開始", + "FF": "同時完成", + "SF": "開始後完成" + }, + "circularError": { + "title": "偵測到循環相依", + "description": "新增此相依性會產生循環參照，這是不被允許的。", + "cyclePath": "相依性循環路徑", + "helpText": "要解決此問題，請選擇不同的任務作為相依項，或移除循環中現有的某個相依關係。" + }, + "error": { + "addFailed": "新增相依性失敗", + "removeFailed": "移除相依性失敗" + } + }, + "conflict": { + "title": "更新衝突", + "message": "此任務已被其他使用者修改。請重新整理頁面以取得最新版本，然後再試一次。" } } diff --git a/frontend/src/components/AddMemberModal.tsx b/frontend/src/components/AddMemberModal.tsx new file mode 100644 index 0000000..62bfa1e --- /dev/null +++ b/frontend/src/components/AddMemberModal.tsx @@ -0,0 +1,240 @@ +import { useState, useEffect, useRef } from 'react' +import { useTranslation } from 'react-i18next' +import { UserSelect } from './UserSelect' +import { UserSearchResult } from '../services/collaboration' + +interface AddMemberModalProps { + isOpen: boolean + onClose: () => void + onAdd: (userId: string, role: 'member' | 'admin') => void + existingMemberIds: string[] + loading?: boolean +} + +export function AddMemberModal({ + isOpen, + onClose, + onAdd, + existingMemberIds, + loading = false, +}: AddMemberModalProps) { + const { t } = useTranslation('settings') + const [selectedUserId, setSelectedUserId] = useState(null) + const [selectedRole, setSelectedRole] = useState<'member' | 'admin'>('member') + const [error, setError] = useState(null) + const modalOverlayRef = useRef(null) + + // Reset state when modal opens/closes + useEffect(() => { + if (isOpen) { + setSelectedUserId(null) + setSelectedRole('member') + setError(null) + } + }, [isOpen]) + + // Handle Escape key to close modal + useEffect(() => { + const handleKeyDown = (e: KeyboardEvent) => { + if (e.key === 'Escape' && isOpen && !loading) { + onClose() + } + } + + if (isOpen) { + document.addEventListener('keydown', handleKeyDown) + } + + return () => { + document.removeEventListener('keydown', handleKeyDown) + } + }, [isOpen, loading, onClose]) + + if (!isOpen) return null + + const handleOverlayClick = (e: React.MouseEvent) => { + if (e.target === e.currentTarget && !loading) { + onClose() + } + } + + const handleUserChange = (userId: string | null, _user: UserSearchResult | null) => { + setSelectedUserId(userId) + setError(null) + + // Check if user is already a member + if (userId && existingMemberIds.includes(userId)) { + setError(t('members.alreadyMember')) + } + } + + const handleSubmit = (e: React.FormEvent) => { + e.preventDefault() + + if (!selectedUserId) { + setError(t('members.selectUserRequired')) + return + } + + if (existingMemberIds.includes(selectedUserId)) { + setError(t('members.alreadyMember')) + return + } + + onAdd(selectedUserId, selectedRole) + } + + return ( +
+
+

+ {t('members.addMember')} +

+ +
+
+ + + {error && {error}} +
+ +
+ + +

+ {selectedRole === 'admin' + ? t('members.roleHelp.admin') + : t('members.roleHelp.member')} +

+
+ +
+ + +
+
+
+
+ ) +} + +const styles: Record = { + overlay: { + position: 'fixed', + top: 0, + left: 0, + right: 0, + bottom: 0, + backgroundColor: 'rgba(0, 0, 0, 0.5)', + display: 'flex', + justifyContent: 'center', + alignItems: 'center', + zIndex: 1200, + }, + modal: { + backgroundColor: 'white', + borderRadius: '8px', + padding: '24px', + width: '450px', + maxWidth: '90%', + boxShadow: '0 8px 32px rgba(0, 0, 0, 0.2)', + }, + title: { + margin: '0 0 24px 0', + fontSize: '18px', + fontWeight: 600, + color: '#212529', + }, + formGroup: { + marginBottom: '20px', + }, + label: { + display: 'block', + marginBottom: '8px', + fontSize: '14px', + fontWeight: 500, + color: '#495057', + }, + select: { + width: '100%', + padding: '10px', + fontSize: '14px', + border: '1px solid #ddd', + borderRadius: '6px', + backgroundColor: 'white', + cursor: 'pointer', + boxSizing: 'border-box', + }, + helpText: { + margin: '8px 0 0 0', + fontSize: '13px', + color: '#6c757d', + lineHeight: 1.4, + }, + errorText: { + display: 'block', + marginTop: '8px', + fontSize: '13px', + color: '#c92a2a', + }, + actions: { + display: 'flex', + justifyContent: 'flex-end', + gap: '12px', + marginTop: '24px', + }, + cancelButton: { + padding: '10px 20px', + backgroundColor: '#f8f9fa', + color: '#495057', + border: '1px solid #dee2e6', + borderRadius: '6px', + fontSize: '14px', + cursor: 'pointer', + }, + submitButton: { + padding: '10px 20px', + backgroundColor: '#0066cc', + color: 'white', + border: 'none', + borderRadius: '6px', + fontSize: '14px', + fontWeight: 500, + cursor: 'pointer', + }, +} + +export default AddMemberModal diff --git a/frontend/src/components/CalendarView.tsx b/frontend/src/components/CalendarView.tsx index 2b3dbc7..306e5bb 100644 --- a/frontend/src/components/CalendarView.tsx +++ b/frontend/src/components/CalendarView.tsx @@ -24,6 +24,7 @@ interface Task { time_estimate: number | null subtask_count: number parent_task_id: string | null + version?: number } interface TaskStatus { @@ -257,12 +258,25 @@ export function CalendarView({ const newDueDate = `${year}-${month}-${day}` try { - await api.patch(`/tasks/${task.id}`, { + const payload: Record = { due_date: newDueDate, - }) + } + // Include version for optimistic locking + if (task.version) { + payload.version = task.version + } + await api.patch(`/tasks/${task.id}`, payload) // Refresh to get updated data onTaskUpdate() - } catch (err) { + } catch (err: unknown) { + // Handle 409 Conflict - version mismatch + if (err && typeof err === 'object' && 'response' in err) { + const axiosError = err as { response?: { status?: number } } + if (axiosError.response?.status === 409) { + // Refresh to get latest data on conflict + onTaskUpdate() + } + } console.error('Failed to update task date:', err) // Rollback on error dropInfo.revert() diff --git a/frontend/src/components/CircularDependencyError.tsx b/frontend/src/components/CircularDependencyError.tsx new file mode 100644 index 0000000..7073bb2 --- /dev/null +++ b/frontend/src/components/CircularDependencyError.tsx @@ -0,0 +1,268 @@ +import { useTranslation } from 'react-i18next' + +interface CycleDetails { + cycle: string[] + cycle_description: string + cycle_task_titles?: string[] + cycle_field_names?: string[] +} + +interface CircularDependencyErrorProps { + errorType: 'task' | 'formula' + cycleDetails: CycleDetails + onDismiss?: () => void +} + +/** + * Component to display circular dependency errors with a user-friendly + * visualization of the cycle path. + */ +export function CircularDependencyError({ + errorType, + cycleDetails, + onDismiss, +}: CircularDependencyErrorProps) { + const { t } = useTranslation(['tasks', 'settings', 'common']) + + // Get display names for the cycle - prefer titles/names over IDs + const cycleDisplayNames = + errorType === 'task' + ? cycleDetails.cycle_task_titles || cycleDetails.cycle + : cycleDetails.cycle_field_names || cycleDetails.cycle + + return ( +
+
+
+ + + + + +
+
+

+ {errorType === 'task' + ? t('tasks:dependencies.circularError.title') + : t('settings:customFields.circularError.title')} +

+

+ {errorType === 'task' + ? t('tasks:dependencies.circularError.description') + : t('settings:customFields.circularError.description')} +

+
+ {onDismiss && ( + + )} +
+ +
+
+ {errorType === 'task' + ? t('tasks:dependencies.circularError.cyclePath') + : t('settings:customFields.circularError.cyclePath')} +
+
+ {cycleDisplayNames.map((name, index) => ( +
+
+ {name} +
+ {index < cycleDisplayNames.length - 1 && ( +
+ + + + +
+ )} +
+ ))} +
+
+ +
+

+ {errorType === 'task' + ? t('tasks:dependencies.circularError.helpText') + : t('settings:customFields.circularError.helpText')} +

+
+
+ ) +} + +/** + * Parse circular dependency error from API response + */ +export function parseCircularError( + errorDetail: unknown +): { isCircular: boolean; cycleDetails?: CycleDetails } { + if (!errorDetail || typeof errorDetail !== 'object') { + return { isCircular: false } + } + + const detail = errorDetail as Record + + // Check if this is a circular dependency error + if (detail.error_type === 'circular') { + const details = detail.details as CycleDetails | undefined + if (details) { + return { + isCircular: true, + cycleDetails: { + cycle: details.cycle || [], + cycle_description: details.cycle_description || '', + cycle_task_titles: details.cycle_task_titles, + cycle_field_names: details.cycle_field_names, + }, + } + } + } + + return { isCircular: false } +} + +const styles: Record = { + container: { + backgroundColor: '#fff8e1', + border: '1px solid #ffc107', + borderRadius: '8px', + padding: '16px', + marginBottom: '16px', + }, + header: { + display: 'flex', + alignItems: 'flex-start', + gap: '12px', + marginBottom: '16px', + }, + iconContainer: { + flexShrink: 0, + }, + icon: { + width: '24px', + height: '24px', + color: '#f57c00', + }, + titleContainer: { + flex: 1, + }, + title: { + margin: 0, + fontSize: '16px', + fontWeight: 600, + color: '#e65100', + }, + description: { + margin: '4px 0 0 0', + fontSize: '14px', + color: '#5d4037', + }, + dismissButton: { + padding: '4px', + backgroundColor: 'transparent', + border: 'none', + borderRadius: '4px', + cursor: 'pointer', + color: '#795548', + display: 'flex', + alignItems: 'center', + justifyContent: 'center', + }, + cycleVisualization: { + backgroundColor: '#fff3e0', + borderRadius: '6px', + padding: '12px', + marginBottom: '12px', + }, + cycleLabel: { + fontSize: '12px', + fontWeight: 500, + color: '#bf360c', + textTransform: 'uppercase', + marginBottom: '8px', + }, + cyclePathContainer: { + display: 'flex', + flexWrap: 'wrap', + alignItems: 'center', + gap: '4px', + }, + cycleItem: { + display: 'flex', + alignItems: 'center', + gap: '4px', + }, + cycleNode: { + padding: '6px 12px', + backgroundColor: 'white', + border: '1px solid #ffcc80', + borderRadius: '4px', + fontSize: '13px', + fontWeight: 500, + color: '#5d4037', + whiteSpace: 'nowrap', + }, + cycleNodeHighlight: { + backgroundColor: '#ffecb3', + borderColor: '#ffa726', + color: '#e65100', + }, + cycleArrow: { + color: '#ff9800', + display: 'flex', + alignItems: 'center', + }, + helpSection: { + borderTop: '1px solid #ffe082', + paddingTop: '12px', + }, + helpText: { + margin: 0, + fontSize: '13px', + color: '#6d4c41', + lineHeight: 1.5, + }, +} + +export default CircularDependencyError diff --git a/frontend/src/components/CustomFieldEditor.tsx b/frontend/src/components/CustomFieldEditor.tsx index 82ce69d..62bfb42 100644 --- a/frontend/src/components/CustomFieldEditor.tsx +++ b/frontend/src/components/CustomFieldEditor.tsx @@ -7,6 +7,13 @@ import { CustomFieldUpdate, FieldType, } from '../services/customFields' +import { CircularDependencyError, parseCircularError } from './CircularDependencyError' + +interface CycleDetails { + cycle: string[] + cycle_description: string + cycle_field_names?: string[] +} interface CustomFieldEditorProps { projectId: string @@ -40,6 +47,7 @@ export function CustomFieldEditor({ const [formula, setFormula] = useState(field?.formula || '') const [saving, setSaving] = useState(false) const [error, setError] = useState(null) + const [circularError, setCircularError] = useState(null) const modalOverlayRef = useRef(null) // A11Y: Handle Escape key to close modal @@ -74,6 +82,7 @@ export function CustomFieldEditor({ setFormula('') } setError(null) + setCircularError(null) }, [field]) const handleOverlayClick = (e: React.MouseEvent) => { @@ -125,6 +134,7 @@ export function CustomFieldEditor({ setSaving(true) setError(null) + setCircularError(null) try { if (isEditing && field) { @@ -164,10 +174,19 @@ export function CustomFieldEditor({ onSave() } catch (err: unknown) { - const errorMessage = - (err as { response?: { data?: { detail?: string } } })?.response?.data?.detail || - t('customFields.saveError') - setError(errorMessage) + const error = err as { response?: { data?: { detail?: unknown } } } + const errorDetail = error?.response?.data?.detail + + // Check if this is a circular reference error + const { isCircular, cycleDetails } = parseCircularError(errorDetail) + if (isCircular && cycleDetails) { + setCircularError(cycleDetails) + setError(null) + } else { + const errorMessage = + typeof errorDetail === 'string' ? errorDetail : t('customFields.saveError') + setError(errorMessage) + } } finally { setSaving(false) } @@ -194,7 +213,15 @@ export function CustomFieldEditor({
- {error &&
{error}
} + {circularError && ( + setCircularError(null)} + /> + )} + + {error && !circularError &&
{error}
} {/* Field Name */}
diff --git a/frontend/src/components/GanttChart.tsx b/frontend/src/components/GanttChart.tsx index f756b3d..64cc239 100644 --- a/frontend/src/components/GanttChart.tsx +++ b/frontend/src/components/GanttChart.tsx @@ -1,7 +1,15 @@ import { useEffect, useRef, useState, useCallback } from 'react' +import { useTranslation } from 'react-i18next' import Gantt, { GanttTask, ViewMode } from 'frappe-gantt' import api from '../services/api' import { dependenciesApi, TaskDependency, DependencyType } from '../services/dependencies' +import { CircularDependencyError, parseCircularError } from './CircularDependencyError' + +interface CycleDetails { + cycle: string[] + cycle_description: string + cycle_task_titles?: string[] +} interface Task { id: string @@ -20,6 +28,7 @@ interface Task { subtask_count: number parent_task_id: string | null progress?: number + version?: number } interface TaskStatus { @@ -52,6 +61,7 @@ export function GanttChart({ onTaskClick, onTaskUpdate, }: GanttChartProps) { + const { t } = useTranslation(['tasks', 'common']) const ganttRef = useRef(null) const ganttInstance = useRef(null) const [viewMode, setViewMode] = useState('Week') @@ -65,6 +75,7 @@ export function GanttChart({ const [selectedPredecessor, setSelectedPredecessor] = useState('') const [selectedDependencyType, setSelectedDependencyType] = useState('FS') const [dependencyError, setDependencyError] = useState(null) + const [circularError, setCircularError] = useState(null) // Task data mapping for quick lookup const taskMap = useRef>(new Map()) @@ -257,16 +268,32 @@ export function GanttChart({ const dueDate = formatLocalDate(end) try { - await api.patch(`/tasks/${taskId}`, { + // Find the task to get its version + const task = tasks.find(t => t.id === taskId) + const payload: Record = { start_date: startDate, due_date: dueDate, - }) + } + // Include version for optimistic locking + if (task?.version) { + payload.version = task.version + } + await api.patch(`/tasks/${taskId}`, payload) onTaskUpdate() } catch (err: unknown) { console.error('Failed to update task dates:', err) - const error = err as { response?: { data?: { detail?: string } } } - const errorMessage = error.response?.data?.detail || 'Failed to update task dates' - setError(errorMessage) + const error = err as { response?: { status?: number; data?: { detail?: string | { message?: string } } } } + // Handle 409 Conflict - version mismatch + if (error.response?.status === 409) { + const detail = error.response?.data?.detail + const errorMessage = typeof detail === 'object' ? detail?.message : 'Task has been modified by another user' + setError(errorMessage || 'Task has been modified by another user') + } else { + const errorMessage = typeof error.response?.data?.detail === 'string' + ? error.response?.data?.detail + : 'Failed to update task dates' + setError(errorMessage) + } // Refresh to rollback visual changes onTaskUpdate() } finally { @@ -286,6 +313,7 @@ export function GanttChart({ if (!selectedTaskForDependency || !selectedPredecessor) return setDependencyError(null) + setCircularError(null) try { await dependenciesApi.addDependency(selectedTaskForDependency.id, { @@ -299,9 +327,21 @@ export function GanttChart({ setSelectedDependencyType('FS') } catch (err: unknown) { console.error('Failed to add dependency:', err) - const error = err as { response?: { data?: { detail?: string } } } - const errorMessage = error.response?.data?.detail || 'Failed to add dependency' - setDependencyError(errorMessage) + const error = err as { response?: { data?: { detail?: unknown } } } + const errorDetail = error.response?.data?.detail + + // Check if this is a circular dependency error + const { isCircular, cycleDetails } = parseCircularError(errorDetail) + if (isCircular && cycleDetails) { + setCircularError(cycleDetails) + setDependencyError(null) + } else { + const errorMessage = + typeof errorDetail === 'string' + ? errorDetail + : t('dependencies.error.addFailed') + setDependencyError(errorMessage) + } } } @@ -321,6 +361,7 @@ export function GanttChart({ setSelectedTaskForDependency(task) setSelectedPredecessor('') setDependencyError(null) + setCircularError(null) setShowDependencyModal(true) } @@ -465,7 +506,15 @@ export function GanttChart({ Manage Dependencies for "{selectedTaskForDependency.title}" - {dependencyError && ( + {circularError && ( + setCircularError(null)} + /> + )} + + {dependencyError && !circularError && (
{dependencyError}
)} @@ -546,11 +595,12 @@ export function GanttChart({ setShowDependencyModal(false) setSelectedTaskForDependency(null) setDependencyError(null) + setCircularError(null) setSelectedDependencyType('FS') }} style={styles.closeButton} > - Close + {t('common:buttons.close')}
diff --git a/frontend/src/components/KanbanBoard.tsx b/frontend/src/components/KanbanBoard.tsx index d7d6ef0..7d84b1f 100644 --- a/frontend/src/components/KanbanBoard.tsx +++ b/frontend/src/components/KanbanBoard.tsx @@ -1,4 +1,4 @@ -import { useState } from 'react' +import { useState, useRef, useCallback, useEffect } from 'react' import { useTranslation } from 'react-i18next' import { CustomValueResponse } from '../services/customFields' @@ -19,6 +19,7 @@ interface Task { subtask_count: number parent_task_id: string | null custom_values?: CustomValueResponse[] + version?: number } interface TaskStatus { @@ -35,16 +36,37 @@ interface KanbanBoardProps { onTaskClick: (task: Task) => void } +// Touch event detection +const isTouchDevice = () => { + return 'ontouchstart' in window || navigator.maxTouchPoints > 0 +} + export function KanbanBoard({ tasks, statuses, onStatusChange, onTaskClick, }: KanbanBoardProps) { - const { t } = useTranslation('tasks') + const { t, i18n } = useTranslation('tasks') const [draggedTaskId, setDraggedTaskId] = useState(null) const [dragOverColumnId, setDragOverColumnId] = useState(null) + // Touch drag state + const [touchDragTask, setTouchDragTask] = useState(null) + const [touchDragPosition, setTouchDragPosition] = useState<{ x: number; y: number } | null>(null) + const touchStartRef = useRef<{ x: number; y: number; time: number } | null>(null) + const longPressTimerRef = useRef(null) + const boardRef = useRef(null) + + // Clean up long press timer on unmount + useEffect(() => { + return () => { + if (longPressTimerRef.current) { + clearTimeout(longPressTimerRef.current) + } + } + }, []) + // Group tasks by status const tasksByStatus: Record = {} statuses.forEach((status) => { @@ -53,6 +75,7 @@ export function KanbanBoard({ // Tasks without status const unassignedTasks = tasks.filter((task) => !task.status_id) + // Desktop drag handlers const handleDragStart = (e: React.DragEvent, taskId: string) => { setDraggedTaskId(taskId) e.dataTransfer.effectAllowed = 'move' @@ -97,6 +120,120 @@ export function KanbanBoard({ setDragOverColumnId(null) } + // Touch drag handlers for mobile + const handleTouchStart = useCallback((e: React.TouchEvent, task: Task) => { + const touch = e.touches[0] + touchStartRef.current = { + x: touch.clientX, + y: touch.clientY, + time: Date.now(), + } + + // Start long press timer for drag initiation + longPressTimerRef.current = window.setTimeout(() => { + // Vibrate for haptic feedback if available + if (navigator.vibrate) { + navigator.vibrate(50) + } + setTouchDragTask(task) + setTouchDragPosition({ x: touch.clientX, y: touch.clientY }) + }, 300) // 300ms long press to start drag + }, []) + + const handleTouchMove = useCallback((e: React.TouchEvent) => { + const touch = e.touches[0] + + // Cancel long press if moved too far before timer fires + if (touchStartRef.current && !touchDragTask) { + const dx = Math.abs(touch.clientX - touchStartRef.current.x) + const dy = Math.abs(touch.clientY - touchStartRef.current.y) + if (dx > 10 || dy > 10) { + if (longPressTimerRef.current) { + clearTimeout(longPressTimerRef.current) + longPressTimerRef.current = null + } + } + } + + // Update drag position + if (touchDragTask) { + e.preventDefault() // Prevent scrolling while dragging + setTouchDragPosition({ x: touch.clientX, y: touch.clientY }) + + // Find column under touch point + const columnElements = boardRef.current?.querySelectorAll('[data-status-id]') + columnElements?.forEach((col) => { + const rect = col.getBoundingClientRect() + if ( + touch.clientX >= rect.left && + touch.clientX <= rect.right && + touch.clientY >= rect.top && + touch.clientY <= rect.bottom + ) { + const statusId = col.getAttribute('data-status-id') + if (statusId && dragOverColumnId !== statusId) { + setDragOverColumnId(statusId) + } + } + }) + } + }, [touchDragTask, dragOverColumnId]) + + const handleTouchEnd = useCallback(() => { + // Clear long press timer + if (longPressTimerRef.current) { + clearTimeout(longPressTimerRef.current) + longPressTimerRef.current = null + } + + // Handle drop on touch end + if (touchDragTask && dragOverColumnId) { + if (touchDragTask.status_id !== dragOverColumnId) { + onStatusChange(touchDragTask.id, dragOverColumnId) + } + } + + // Reset touch drag state + setTouchDragTask(null) + setTouchDragPosition(null) + setDragOverColumnId(null) + touchStartRef.current = null + }, [touchDragTask, dragOverColumnId, onStatusChange]) + + const handleTouchCancel = useCallback(() => { + if (longPressTimerRef.current) { + clearTimeout(longPressTimerRef.current) + longPressTimerRef.current = null + } + setTouchDragTask(null) + setTouchDragPosition(null) + setDragOverColumnId(null) + touchStartRef.current = null + }, []) + + // Handle click/tap - prevent click if dragging + const handleCardClick = useCallback((e: React.MouseEvent | React.TouchEvent, task: Task) => { + // If there was a touch drag, don't trigger click + if (touchDragTask) { + e.preventDefault() + return + } + + // For touch events, check if it was a quick tap + if (touchStartRef.current) { + const elapsed = Date.now() - touchStartRef.current.time + if (elapsed < 300) { + // Quick tap - trigger click + onTaskClick(task) + } + touchStartRef.current = null + return + } + + // Desktop click + onTaskClick(task) + }, [touchDragTask, onTaskClick]) + const getPriorityColor = (priority: string): string => { const colors: Record = { low: '#808080', @@ -107,54 +244,115 @@ export function KanbanBoard({ return colors[priority] || colors.medium } - const renderTaskCard = (task: Task) => ( -
handleDragStart(e, task.id)} - onDragEnd={handleDragEnd} - onClick={() => onTaskClick(task)} - > -
{task.title}
- {task.description && ( -
- {task.description.length > 80 - ? task.description.substring(0, 80) + '...' - : task.description} + // Format date according to locale + const formatDate = (dateString: string): string => { + const date = new Date(dateString) + return date.toLocaleDateString(i18n.language, { + month: 'short', + day: 'numeric', + }) + } + + const renderTaskCard = (task: Task) => { + const isDragging = draggedTaskId === task.id || touchDragTask?.id === task.id + + return ( +
handleDragStart(e, task.id)} + onDragEnd={handleDragEnd} + onClick={(e) => handleCardClick(e, task)} + onTouchStart={(e) => handleTouchStart(e, task)} + onTouchMove={handleTouchMove} + onTouchEnd={handleTouchEnd} + onTouchCancel={handleTouchCancel} + > +
{task.title}
+ {task.description && ( +
+ {task.description.length > 80 + ? task.description.substring(0, 80) + '...' + : task.description} +
+ )} +
+ {task.assignee_name && ( + {task.assignee_name} + )} + {task.due_date && ( + + {formatDate(task.due_date)} + + )} + {task.subtask_count > 0 && ( + {t('subtasks.count', { count: task.subtask_count })} + )} + {/* Display custom field values (limit to first 2 for compact display) */} + {task.custom_values?.slice(0, 2).map((cv) => ( + + {cv.field_name}: {cv.display_value || cv.value || '-'} + + ))}
- )} -
- {task.assignee_name && ( - {task.assignee_name} + + {/* Touch-friendly quick action button */} + {isTouchDevice() && ( +
+ +
)} - {task.due_date && ( - - {new Date(task.due_date).toLocaleDateString()} - - )} - {task.subtask_count > 0 && ( - {t('subtasks.count', { count: task.subtask_count })} - )} - {/* Display custom field values (limit to first 2 for compact display) */} - {task.custom_values?.slice(0, 2).map((cv) => ( - - {cv.field_name}: {cv.display_value || cv.value || '-'} - - ))}
-
- ) + ) + } return ( -
+
+ {/* Touch drag ghost element */} + {touchDragTask && touchDragPosition && ( +
+
+
+ {touchDragTask.title} +
+
+ )} + {/* Unassigned column (if there are tasks without status) */} {unassignedTasks.length > 0 && ( -
+
(
= { overflowX: 'auto', paddingBottom: '16px', minHeight: '500px', + WebkitOverflowScrolling: 'touch', + position: 'relative', }, column: { flex: '0 0 280px', @@ -222,7 +423,7 @@ const styles: Record = { display: 'flex', flexDirection: 'column', maxHeight: 'calc(100vh - 200px)', - transition: 'background-color 0.2s ease', + transition: 'background-color 0.2s ease, box-shadow 0.2s ease', }, columnDragOver: { backgroundColor: '#e3f2fd', @@ -236,6 +437,7 @@ const styles: Record = { borderRadius: '8px 8px 0 0', color: 'white', fontWeight: 600, + minHeight: '48px', }, columnTitle: { fontSize: '14px', @@ -243,8 +445,10 @@ const styles: Record = { taskCount: { fontSize: '12px', backgroundColor: 'rgba(255, 255, 255, 0.3)', - padding: '2px 8px', + padding: '4px 10px', borderRadius: '10px', + minWidth: '24px', + textAlign: 'center', }, taskList: { flex: 1, @@ -252,16 +456,19 @@ const styles: Record = { overflowY: 'auto', display: 'flex', flexDirection: 'column', - gap: '8px', + gap: '10px', + WebkitOverflowScrolling: 'touch', }, taskCard: { backgroundColor: 'white', - borderRadius: '6px', - padding: '12px', + borderRadius: '8px', + padding: '14px', boxShadow: '0 1px 3px rgba(0, 0, 0, 0.1)', - cursor: 'grab', + cursor: 'pointer', borderLeft: '4px solid', transition: 'box-shadow 0.2s ease, transform 0.2s ease', + touchAction: 'manipulation', + userSelect: 'none', }, taskTitle: { fontSize: '14px', @@ -284,19 +491,21 @@ const styles: Record = { assigneeBadge: { backgroundColor: '#e3f2fd', color: '#1565c0', - padding: '2px 6px', + padding: '4px 8px', borderRadius: '4px', }, dueDate: { color: '#666', + padding: '4px 0', }, subtaskBadge: { color: '#767676', // WCAG AA compliant + padding: '4px 0', }, customValueBadge: { backgroundColor: '#f3e5f5', color: '#7b1fa2', - padding: '2px 6px', + padding: '4px 8px', borderRadius: '4px', fontSize: '10px', maxWidth: '100px', @@ -312,6 +521,51 @@ const styles: Record = { border: '2px dashed #ddd', borderRadius: '6px', }, + // Touch-friendly action area + touchActions: { + marginTop: '10px', + paddingTop: '10px', + borderTop: '1px solid #eee', + }, + statusSelectMini: { + width: '100%', + padding: '10px 12px', + border: '1px solid #ddd', + borderRadius: '6px', + fontSize: '13px', + backgroundColor: 'white', + minHeight: '44px', + }, + // Drag ghost for touch + dragGhost: { + position: 'fixed', + zIndex: 1000, + pointerEvents: 'none', + width: '240px', + backgroundColor: 'white', + borderRadius: '8px', + boxShadow: '0 8px 24px rgba(0, 0, 0, 0.2)', + padding: '12px', + opacity: 0.9, + }, + dragGhostContent: { + display: 'flex', + alignItems: 'center', + gap: '10px', + }, + dragGhostPriority: { + width: '4px', + height: '24px', + borderRadius: '2px', + flexShrink: 0, + }, + dragGhostTitle: { + fontSize: '14px', + fontWeight: 500, + overflow: 'hidden', + textOverflow: 'ellipsis', + whiteSpace: 'nowrap', + }, } export default KanbanBoard diff --git a/frontend/src/components/Layout.tsx b/frontend/src/components/Layout.tsx index f935c73..0ca1e71 100644 --- a/frontend/src/components/Layout.tsx +++ b/frontend/src/components/Layout.tsx @@ -1,4 +1,4 @@ -import { ReactNode } from 'react' +import { ReactNode, useState, useEffect } from 'react' import { useNavigate, useLocation } from 'react-router-dom' import { useTranslation } from 'react-i18next' import { useAuth } from '../contexts/AuthContext' @@ -9,45 +9,121 @@ interface LayoutProps { children: ReactNode } +const MOBILE_BREAKPOINT = 768 + export default function Layout({ children }: LayoutProps) { const { t } = useTranslation('common') const { user, logout } = useAuth() const navigate = useNavigate() const location = useLocation() + // Sidebar state management + const [isMobile, setIsMobile] = useState(false) + const [sidebarOpen, setSidebarOpen] = useState(false) + + // Detect mobile viewport + useEffect(() => { + const checkMobile = () => { + setIsMobile(window.innerWidth < MOBILE_BREAKPOINT) + } + + checkMobile() + window.addEventListener('resize', checkMobile) + return () => window.removeEventListener('resize', checkMobile) + }, []) + + // Close sidebar on route change (mobile only) + useEffect(() => { + if (isMobile) { + setSidebarOpen(false) + } + }, [location.pathname, isMobile]) + + // Handle escape key to close sidebar on mobile + useEffect(() => { + const handleKeyDown = (e: KeyboardEvent) => { + if (e.key === 'Escape' && isMobile && sidebarOpen) { + setSidebarOpen(false) + } + } + + document.addEventListener('keydown', handleKeyDown) + return () => document.removeEventListener('keydown', handleKeyDown) + }, [isMobile, sidebarOpen]) + const handleLogout = async () => { await logout() } + const toggleMobileSidebar = () => { + setSidebarOpen(!sidebarOpen) + } + + const handleNavClick = (path: string) => { + navigate(path) + if (isMobile) { + setSidebarOpen(false) + } + } + const navItems = [ - { path: '/', labelKey: 'nav.dashboard' }, - { path: '/spaces', labelKey: 'nav.spaces' }, - { path: '/workload', labelKey: 'nav.workload' }, - { path: '/project-health', labelKey: 'nav.health' }, - ...(user?.is_system_admin ? [{ path: '/audit', labelKey: 'nav.audit' }] : []), + { path: '/', labelKey: 'nav.dashboard', icon: 'dashboard' }, + { path: '/spaces', labelKey: 'nav.spaces', icon: 'folder' }, + { path: '/workload', labelKey: 'nav.workload', icon: 'chart' }, + { path: '/project-health', labelKey: 'nav.health', icon: 'health' }, + ...(user?.is_system_admin ? [{ path: '/audit', labelKey: 'nav.audit', icon: 'audit' }] : []), ] + // Render navigation icon + const renderIcon = (icon: string) => { + const icons: Record = { + dashboard: '\u25A0', // Square + folder: '\u25A1', // Empty square + chart: '\u25B2', // Triangle + health: '\u25CF', // Circle + audit: '\u25C6', // Diamond + } + return icons[icon] || '\u25CF' + } + return (
+ {/* Header */}
+ {/* Hamburger menu for mobile */} + {isMobile && ( + + )}

navigate('/')}> Project Control

- + {/* Desktop navigation in header */} + {!isMobile && ( + + )}
@@ -60,13 +136,71 @@ export default function Layout({ children }: LayoutProps) { {user?.name} {user?.is_system_admin && ( - Admin + {t('labels.admin')} )}
+ + {/* Mobile sidebar overlay */} + {isMobile && sidebarOpen && ( +
setSidebarOpen(false)} + aria-hidden="true" + /> + )} + + {/* Mobile slide-out sidebar */} + {isMobile && ( + + )} +
{children}
) @@ -84,11 +218,35 @@ const styles: { [key: string]: React.CSSProperties } = { padding: '12px 24px', backgroundColor: 'white', boxShadow: '0 1px 3px rgba(0, 0, 0, 0.1)', + position: 'sticky', + top: 0, + zIndex: 100, }, headerLeft: { display: 'flex', alignItems: 'center', - gap: '24px', + gap: '16px', + }, + hamburgerButton: { + display: 'flex', + flexDirection: 'column', + justifyContent: 'center', + alignItems: 'center', + width: '44px', + height: '44px', + padding: '8px', + backgroundColor: 'transparent', + border: 'none', + borderRadius: '8px', + cursor: 'pointer', + gap: '5px', + }, + hamburgerLine: { + display: 'block', + width: '24px', + height: '3px', + backgroundColor: '#333', + borderRadius: '2px', }, logo: { fontSize: '18px', @@ -102,13 +260,16 @@ const styles: { [key: string]: React.CSSProperties } = { gap: '4px', }, navItem: { - padding: '8px 16px', + padding: '10px 16px', backgroundColor: 'transparent', border: 'none', - borderRadius: '4px', + borderRadius: '6px', cursor: 'pointer', fontSize: '14px', color: '#666', + minHeight: '44px', + minWidth: '44px', + transition: 'background-color 0.2s, color 0.2s', }, navItemActive: { backgroundColor: '#e3f2fd', @@ -126,27 +287,120 @@ const styles: { [key: string]: React.CSSProperties } = { color: '#0066cc', fontSize: '14px', cursor: 'pointer', - padding: '4px 8px', - borderRadius: '4px', + padding: '10px 12px', + borderRadius: '6px', textDecoration: 'underline', + minHeight: '44px', + minWidth: '44px', }, badge: { backgroundColor: '#0066cc', color: 'white', - padding: '2px 8px', + padding: '4px 10px', borderRadius: '4px', fontSize: '11px', fontWeight: 500, }, logoutButton: { - padding: '8px 16px', + padding: '10px 16px', backgroundColor: '#f5f5f5', border: '1px solid #ddd', - borderRadius: '4px', + borderRadius: '6px', cursor: 'pointer', fontSize: '14px', + minHeight: '44px', + minWidth: '44px', + transition: 'background-color 0.2s', }, main: { minHeight: 'calc(100vh - 60px)', }, + // Mobile sidebar styles + overlay: { + position: 'fixed', + top: 0, + left: 0, + right: 0, + bottom: 0, + backgroundColor: 'rgba(0, 0, 0, 0.5)', + zIndex: 200, + }, + mobileSidebar: { + position: 'fixed', + top: 0, + left: 0, + width: '280px', + maxWidth: '85vw', + height: '100vh', + backgroundColor: 'white', + boxShadow: '4px 0 16px rgba(0, 0, 0, 0.15)', + zIndex: 300, + display: 'flex', + flexDirection: 'column', + transition: 'transform 0.3s ease-in-out', + }, + sidebarHeader: { + display: 'flex', + justifyContent: 'space-between', + alignItems: 'center', + padding: '16px 20px', + borderBottom: '1px solid #eee', + }, + sidebarTitle: { + margin: 0, + fontSize: '18px', + fontWeight: 600, + color: '#333', + }, + closeSidebarButton: { + width: '44px', + height: '44px', + display: 'flex', + alignItems: 'center', + justifyContent: 'center', + backgroundColor: 'transparent', + border: 'none', + borderRadius: '8px', + cursor: 'pointer', + fontSize: '18px', + color: '#666', + }, + sidebarNav: { + flex: 1, + padding: '16px 12px', + display: 'flex', + flexDirection: 'column', + gap: '4px', + overflowY: 'auto', + }, + sidebarNavItem: { + display: 'flex', + alignItems: 'center', + gap: '12px', + padding: '14px 16px', + backgroundColor: 'transparent', + border: 'none', + borderRadius: '8px', + cursor: 'pointer', + fontSize: '15px', + color: '#333', + textAlign: 'left', + width: '100%', + minHeight: '48px', + transition: 'background-color 0.2s', + }, + sidebarNavItemActive: { + backgroundColor: '#e3f2fd', + color: '#0066cc', + fontWeight: 500, + }, + navIcon: { + fontSize: '18px', + width: '24px', + textAlign: 'center', + }, + sidebarFooter: { + padding: '12px', + borderTop: '1px solid #eee', + }, } diff --git a/frontend/src/components/ProjectMemberList.tsx b/frontend/src/components/ProjectMemberList.tsx new file mode 100644 index 0000000..d403a5c --- /dev/null +++ b/frontend/src/components/ProjectMemberList.tsx @@ -0,0 +1,469 @@ +import { useState, useEffect, useCallback } from 'react' +import { useTranslation } from 'react-i18next' +import { projectMembersApi, ProjectMember } from '../services/projectMembers' +import { useToast } from '../contexts/ToastContext' +import { Skeleton } from './Skeleton' +import { ConfirmModal } from './ConfirmModal' +import { AddMemberModal } from './AddMemberModal' + +interface ProjectMemberListProps { + projectId: string +} + +export function ProjectMemberList({ projectId }: ProjectMemberListProps) { + const { t } = useTranslation('settings') + const { showToast } = useToast() + const [members, setMembers] = useState([]) + const [loading, setLoading] = useState(true) + const [error, setError] = useState(null) + const [isAddModalOpen, setIsAddModalOpen] = useState(false) + const [memberToRemove, setMemberToRemove] = useState(null) + const [editingMemberId, setEditingMemberId] = useState(null) + const [actionLoading, setActionLoading] = useState(false) + + const loadMembers = useCallback(async () => { + try { + setLoading(true) + setError(null) + const response = await projectMembersApi.list(projectId) + setMembers(response.members) + } catch (err) { + console.error('Failed to load project members:', err) + setError(t('members.loadError')) + } finally { + setLoading(false) + } + }, [projectId, t]) + + useEffect(() => { + loadMembers() + }, [loadMembers]) + + const handleAddMember = async (userId: string, role: 'member' | 'admin') => { + try { + setActionLoading(true) + const newMember = await projectMembersApi.add(projectId, { user_id: userId, role }) + setMembers((prev) => [...prev, newMember]) + setIsAddModalOpen(false) + showToast(t('members.memberAdded'), 'success') + } catch (err: unknown) { + console.error('Failed to add member:', err) + const errorMessage = err instanceof Error ? err.message : t('members.addError') + const axiosError = err as { response?: { data?: { detail?: string } } } + if (axiosError.response?.data?.detail) { + showToast(axiosError.response.data.detail, 'error') + } else { + showToast(errorMessage, 'error') + } + } finally { + setActionLoading(false) + } + } + + const handleRemoveMember = async () => { + if (!memberToRemove) return + + try { + setActionLoading(true) + await projectMembersApi.remove(projectId, memberToRemove.id) + setMembers((prev) => prev.filter((m) => m.id !== memberToRemove.id)) + setMemberToRemove(null) + showToast(t('messages.memberRemoved'), 'success') + } catch (err) { + console.error('Failed to remove member:', err) + showToast(t('members.removeError'), 'error') + } finally { + setActionLoading(false) + } + } + + const handleRoleChange = async (member: ProjectMember, newRole: 'member' | 'admin') => { + if (member.role === newRole) { + setEditingMemberId(null) + return + } + + try { + setActionLoading(true) + const updatedMember = await projectMembersApi.updateRole(projectId, member.id, { + role: newRole, + }) + setMembers((prev) => + prev.map((m) => (m.id === member.id ? updatedMember : m)) + ) + setEditingMemberId(null) + showToast(t('messages.roleChanged'), 'success') + } catch (err) { + console.error('Failed to update member role:', err) + showToast(t('members.roleChangeError'), 'error') + } finally { + setActionLoading(false) + } + } + + const formatDate = (dateString: string) => { + const date = new Date(dateString) + return date.toLocaleDateString(undefined, { + year: 'numeric', + month: 'short', + day: 'numeric', + }) + } + + if (loading) { + return ( +
+
+ + +
+
+ {[1, 2, 3].map((i) => ( +
+ +
+ + +
+ +
+ ))} +
+
+ ) + } + + if (error) { + return ( +
+
+

{error}

+ +
+
+ ) + } + + return ( +
+
+

{t('members.title')}

+ +
+ +

{t('members.description')}

+ + {members.length === 0 ? ( +
+

{t('members.empty')}

+

{t('members.emptyHint')}

+
+ ) : ( +
+
+ {t('members.user')} + {t('members.role')} + {t('members.joinedAt')} + {t('common:labels.actions')} +
+ {members.map((member) => ( +
+
+
+ {member.user.name.charAt(0).toUpperCase()} +
+
+ {member.user.name} + {member.user.email} +
+
+ +
+ {editingMemberId === member.id ? ( + + ) : ( + + {t(`members.roles.${member.role}`)} + + )} +
+ +
+ {formatDate(member.joined_at)} +
+ +
+ + +
+
+ ))} +
+ )} + + setIsAddModalOpen(false)} + onAdd={handleAddMember} + existingMemberIds={members.map((m) => m.user_id)} + loading={actionLoading} + /> + + setMemberToRemove(null)} + /> +
+ ) +} + +const styles: Record = { + section: { + backgroundColor: 'white', + borderRadius: '8px', + padding: '24px', + boxShadow: '0 1px 3px rgba(0, 0, 0, 0.1)', + }, + headerRow: { + display: 'flex', + justifyContent: 'space-between', + alignItems: 'center', + marginBottom: '16px', + }, + sectionTitle: { + fontSize: '18px', + fontWeight: 600, + margin: 0, + }, + description: { + fontSize: '14px', + color: '#666', + marginBottom: '20px', + }, + addButton: { + padding: '8px 16px', + backgroundColor: '#0066cc', + color: 'white', + border: 'none', + borderRadius: '6px', + cursor: 'pointer', + fontSize: '14px', + fontWeight: 500, + }, + memberList: { + display: 'flex', + flexDirection: 'column', + gap: '0', + }, + tableHeader: { + display: 'grid', + gridTemplateColumns: '1fr 120px 120px 180px', + gap: '16px', + padding: '12px 16px', + backgroundColor: '#f8f9fa', + borderRadius: '6px 6px 0 0', + borderBottom: '1px solid #e9ecef', + }, + headerCell: { + fontSize: '13px', + fontWeight: 600, + color: '#495057', + textTransform: 'uppercase', + letterSpacing: '0.5px', + }, + headerCellSmall: { + fontSize: '13px', + fontWeight: 600, + color: '#495057', + textTransform: 'uppercase', + letterSpacing: '0.5px', + }, + headerCellActions: { + fontSize: '13px', + fontWeight: 600, + color: '#495057', + textTransform: 'uppercase', + letterSpacing: '0.5px', + textAlign: 'right', + }, + memberItem: { + display: 'grid', + gridTemplateColumns: '1fr 120px 120px 180px', + gap: '16px', + alignItems: 'center', + padding: '16px', + borderBottom: '1px solid #eee', + }, + userInfo: { + display: 'flex', + alignItems: 'center', + gap: '12px', + }, + avatar: { + width: '40px', + height: '40px', + borderRadius: '50%', + backgroundColor: '#0066cc', + color: 'white', + display: 'flex', + alignItems: 'center', + justifyContent: 'center', + fontSize: '16px', + fontWeight: 600, + }, + userDetails: { + display: 'flex', + flexDirection: 'column', + gap: '2px', + }, + userName: { + fontSize: '14px', + fontWeight: 500, + color: '#212529', + }, + userEmail: { + fontSize: '13px', + color: '#6c757d', + }, + roleCell: { + display: 'flex', + alignItems: 'center', + }, + roleBadge: { + padding: '4px 10px', + borderRadius: '12px', + fontSize: '12px', + fontWeight: 500, + }, + adminBadge: { + backgroundColor: '#e7f5ff', + color: '#1971c2', + }, + memberBadge: { + backgroundColor: '#f1f3f4', + color: '#495057', + }, + roleSelect: { + padding: '6px 10px', + fontSize: '13px', + borderRadius: '6px', + border: '1px solid #ddd', + backgroundColor: 'white', + cursor: 'pointer', + minWidth: '100px', + }, + dateCell: { + display: 'flex', + alignItems: 'center', + }, + dateText: { + fontSize: '13px', + color: '#6c757d', + }, + actionsCell: { + display: 'flex', + justifyContent: 'flex-end', + gap: '8px', + }, + actionButton: { + padding: '6px 12px', + backgroundColor: '#f8f9fa', + color: '#495057', + border: '1px solid #dee2e6', + borderRadius: '4px', + fontSize: '13px', + cursor: 'pointer', + }, + removeButton: { + padding: '6px 12px', + backgroundColor: '#fff5f5', + color: '#c92a2a', + border: '1px solid #ffc9c9', + borderRadius: '4px', + fontSize: '13px', + cursor: 'pointer', + }, + emptyState: { + textAlign: 'center', + padding: '40px 20px', + backgroundColor: '#f8f9fa', + borderRadius: '8px', + }, + emptyText: { + fontSize: '14px', + color: '#495057', + margin: '0 0 8px 0', + }, + emptyHint: { + fontSize: '13px', + color: '#868e96', + margin: 0, + }, + errorContainer: { + textAlign: 'center', + padding: '40px 20px', + }, + errorText: { + fontSize: '14px', + color: '#c92a2a', + marginBottom: '16px', + }, + retryButton: { + padding: '8px 16px', + backgroundColor: '#f8f9fa', + color: '#495057', + border: '1px solid #dee2e6', + borderRadius: '6px', + cursor: 'pointer', + fontSize: '14px', + }, +} + +export default ProjectMemberList diff --git a/frontend/src/components/TaskDetailModal.tsx b/frontend/src/components/TaskDetailModal.tsx index 8026403..65416c6 100644 --- a/frontend/src/components/TaskDetailModal.tsx +++ b/frontend/src/components/TaskDetailModal.tsx @@ -26,6 +26,7 @@ interface Task { subtask_count: number parent_task_id: string | null custom_values?: CustomValueResponse[] + version?: number } interface TaskStatus { @@ -52,9 +53,14 @@ export function TaskDetailModal({ onUpdate, onSubtaskClick, }: TaskDetailModalProps) { - const { t } = useTranslation('tasks') + const { t, i18n } = useTranslation('tasks') const [isEditing, setIsEditing] = useState(false) const [saving, setSaving] = useState(false) + const [conflictError, setConflictError] = useState<{ + message: string + currentVersion: number + yourVersion: number + } | null>(null) const [editForm, setEditForm] = useState({ title: task.title, description: task.description || '', @@ -153,6 +159,7 @@ export function TaskDetailModal({ const handleSave = async () => { setSaving(true) + setConflictError(null) try { const payload: Record = { title: editForm.title, @@ -160,6 +167,11 @@ export function TaskDetailModal({ priority: editForm.priority, } + // Include version for optimistic locking + if (task.version) { + payload.version = task.version + } + // Always send status_id (null to clear, or the value) payload.status_id = editForm.status_id || null // Always send assignee_id (null to clear, or the value) @@ -194,13 +206,34 @@ export function TaskDetailModal({ await api.patch(`/tasks/${task.id}`, payload) setIsEditing(false) onUpdate() - } catch (err) { + } catch (err: unknown) { + // Handle 409 Conflict - version mismatch + if (err && typeof err === 'object' && 'response' in err) { + const axiosError = err as { response?: { status?: number; data?: { detail?: { error?: string; message?: string; current_version?: number; your_version?: number } } } } + if (axiosError.response?.status === 409) { + const detail = axiosError.response?.data?.detail + if (detail?.error === 'conflict') { + setConflictError({ + message: detail.message || t('conflict.message'), + currentVersion: detail.current_version || 0, + yourVersion: detail.your_version || 0, + }) + return + } + } + } console.error('Failed to update task:', err) } finally { setSaving(false) } } + const handleRefreshTask = () => { + setConflictError(null) + setIsEditing(false) + onUpdate() + } + const handleCustomFieldChange = (fieldId: string, value: string | number | null) => { setEditCustomValues((prev) => ({ ...prev, @@ -289,6 +322,22 @@ export function TaskDetailModal({
+ {/* Conflict Error Banner */} + {conflictError && ( +
+
+
!
+
+
{t('conflict.title')}
+
{t('conflict.message')}
+
+
+ +
+ )} +
{/* Description */} @@ -424,7 +473,11 @@ export function TaskDetailModal({ ) : (
{task.due_date - ? new Date(task.due_date).toLocaleDateString() + ? new Date(task.due_date).toLocaleDateString(i18n.language, { + year: 'numeric', + month: 'short', + day: 'numeric', + }) : t('status.noDueDate')}
)} @@ -742,6 +795,54 @@ const styles: Record = { color: '#888', fontStyle: 'italic', }, + conflictBanner: { + display: 'flex', + alignItems: 'center', + justifyContent: 'space-between', + padding: '12px 24px', + backgroundColor: '#fff3cd', + borderBottom: '1px solid #ffc107', + }, + conflictContent: { + display: 'flex', + alignItems: 'center', + gap: '12px', + }, + conflictIcon: { + width: '24px', + height: '24px', + borderRadius: '50%', + backgroundColor: '#ff9800', + color: 'white', + display: 'flex', + alignItems: 'center', + justifyContent: 'center', + fontWeight: 'bold', + fontSize: '14px', + }, + conflictText: { + flex: 1, + }, + conflictTitle: { + fontWeight: 600, + fontSize: '14px', + color: '#856404', + }, + conflictMessage: { + fontSize: '13px', + color: '#856404', + marginTop: '2px', + }, + conflictRefreshButton: { + padding: '8px 16px', + backgroundColor: '#ff9800', + color: 'white', + border: 'none', + borderRadius: '4px', + cursor: 'pointer', + fontSize: '14px', + fontWeight: 500, + }, } export default TaskDetailModal diff --git a/frontend/src/contexts/NotificationContext.tsx b/frontend/src/contexts/NotificationContext.tsx index 2c067a9..5b53332 100644 --- a/frontend/src/contexts/NotificationContext.tsx +++ b/frontend/src/contexts/NotificationContext.tsx @@ -81,13 +81,14 @@ export function NotificationProvider({ children }: { children: ReactNode }) { if (!token) return // Use env var if available, otherwise derive from current location + // Note: Token is NOT included in URL for security (use first-message auth instead) let wsUrl: string const envWsUrl = import.meta.env.VITE_WS_URL if (envWsUrl) { - wsUrl = `${envWsUrl}/ws/notifications?token=${token}` + wsUrl = `${envWsUrl}/ws/notifications` } else { const wsProtocol = window.location.protocol === 'https:' ? 'wss:' : 'ws:' - wsUrl = `${wsProtocol}//${window.location.host}/ws/notifications?token=${token}` + wsUrl = `${wsProtocol}//${window.location.host}/ws/notifications` } try { @@ -95,13 +96,11 @@ export function NotificationProvider({ children }: { children: ReactNode }) { wsRef.current = ws ws.onopen = () => { - console.log('WebSocket connected') - // Start ping interval - pingIntervalRef.current = setInterval(() => { - if (ws.readyState === WebSocket.OPEN) { - ws.send(JSON.stringify({ type: 'ping' })) - } - }, WS_PING_INTERVAL) + console.log('WebSocket opened, sending authentication...') + // Send authentication message as first message (more secure than query parameter) + if (ws.readyState === WebSocket.OPEN) { + ws.send(JSON.stringify({ type: 'auth', token })) + } } ws.onmessage = (event) => { @@ -109,8 +108,21 @@ export function NotificationProvider({ children }: { children: ReactNode }) { const message = JSON.parse(event.data) switch (message.type) { + case 'auth_required': + // Server requests authentication, send token + if (ws.readyState === WebSocket.OPEN) { + ws.send(JSON.stringify({ type: 'auth', token })) + } + break + case 'connected': console.log('WebSocket authenticated:', message.data.message) + // Start ping interval after successful authentication + pingIntervalRef.current = setInterval(() => { + if (ws.readyState === WebSocket.OPEN) { + ws.send(JSON.stringify({ type: 'ping' })) + } + }, WS_PING_INTERVAL) break case 'unread_sync': diff --git a/frontend/src/contexts/ProjectSyncContext.tsx b/frontend/src/contexts/ProjectSyncContext.tsx index 7aaf71e..39cf2b7 100644 --- a/frontend/src/contexts/ProjectSyncContext.tsx +++ b/frontend/src/contexts/ProjectSyncContext.tsx @@ -96,37 +96,55 @@ export function ProjectSyncProvider({ children }: { children: React.ReactNode }) // Close existing connection cleanup() - // Build WebSocket URL + // Build WebSocket URL (without token in query parameter for security) let wsUrl: string const envWsUrl = import.meta.env.VITE_WS_URL if (envWsUrl) { - wsUrl = `${envWsUrl}/ws/projects/${projectId}?token=${token}` + wsUrl = `${envWsUrl}/ws/projects/${projectId}` } else { const wsProtocol = window.location.protocol === 'https:' ? 'wss:' : 'ws:' - wsUrl = `${wsProtocol}//${window.location.host}/ws/projects/${projectId}?token=${token}` + wsUrl = `${wsProtocol}//${window.location.host}/ws/projects/${projectId}` } try { const ws = new WebSocket(wsUrl) ws.onopen = () => { - reconnectAttemptsRef.current = 0 // Reset on successful connection - setIsConnected(true) - setCurrentProjectId(projectId) - devLog(`Connected to project ${projectId} sync`) - - // Start ping interval to keep connection alive - pingIntervalRef.current = setInterval(() => { - if (ws.readyState === WebSocket.OPEN) { - ws.send(JSON.stringify({ type: 'ping' })) - } - }, WS_PING_INTERVAL) + devLog(`WebSocket opened for project ${projectId}, sending auth...`) + // Send authentication message as first message (more secure than query parameter) + if (ws.readyState === WebSocket.OPEN) { + ws.send(JSON.stringify({ type: 'auth', token })) + } } ws.onmessage = (event) => { try { const message = JSON.parse(event.data) + // Handle auth_required - send auth token + if (message.type === 'auth_required') { + if (ws.readyState === WebSocket.OPEN) { + ws.send(JSON.stringify({ type: 'auth', token })) + } + return + } + + // Handle successful authentication + if (message.type === 'connected') { + reconnectAttemptsRef.current = 0 // Reset on successful connection + setIsConnected(true) + setCurrentProjectId(projectId) + devLog(`Authenticated and connected to project ${projectId} sync`) + + // Start ping interval to keep connection alive + pingIntervalRef.current = setInterval(() => { + if (ws.readyState === WebSocket.OPEN) { + ws.send(JSON.stringify({ type: 'ping' })) + } + }, WS_PING_INTERVAL) + return + } + // Handle ping/pong if (message.type === 'ping') { if (ws.readyState === WebSocket.OPEN) { @@ -134,7 +152,7 @@ export function ProjectSyncProvider({ children }: { children: React.ReactNode }) } return } - if (message.type === 'pong' || message.type === 'connected') { + if (message.type === 'pong') { return } diff --git a/frontend/src/pages/ProjectSettings.tsx b/frontend/src/pages/ProjectSettings.tsx index 45a0477..760ce72 100644 --- a/frontend/src/pages/ProjectSettings.tsx +++ b/frontend/src/pages/ProjectSettings.tsx @@ -3,6 +3,7 @@ import { useParams, useNavigate } from 'react-router-dom' import { useTranslation } from 'react-i18next' import api from '../services/api' import { CustomFieldList } from '../components/CustomFieldList' +import { ProjectMemberList } from '../components/ProjectMemberList' import { useToast } from '../contexts/ToastContext' import { Skeleton } from '../components/Skeleton' @@ -22,7 +23,7 @@ export default function ProjectSettings() { const { showToast } = useToast() const [project, setProject] = useState(null) const [loading, setLoading] = useState(true) - const [activeTab, setActiveTab] = useState<'general' | 'custom-fields'>('custom-fields') + const [activeTab, setActiveTab] = useState<'general' | 'members' | 'custom-fields'>('custom-fields') useEffect(() => { loadProject() @@ -111,6 +112,15 @@ export default function ProjectSettings() { > {t('tabs.general')} +
)} + {activeTab === 'members' && ( + + )} + {activeTab === 'custom-fields' && ( )} diff --git a/frontend/src/pages/Projects.tsx b/frontend/src/pages/Projects.tsx index 6628e6e..f2a81ae 100644 --- a/frontend/src/pages/Projects.tsx +++ b/frontend/src/pages/Projects.tsx @@ -24,6 +24,31 @@ interface Space { name: string } +interface TaskStatusDef { + name: string + color: string + is_done: boolean + position: number +} + +interface CustomFieldDef { + name: string + field_type: string + options?: string[] + formula?: string +} + +interface ProjectTemplate { + id: string + name: string + description?: string + is_public: boolean + task_statuses: TaskStatusDef[] + custom_fields: CustomFieldDef[] + created_by: string + created_at: string +} + export default function Projects() { const { t } = useTranslation('projects') const { spaceId } = useParams() @@ -40,6 +65,9 @@ export default function Projects() { security_level: 'department', }) const [creating, setCreating] = useState(false) + const [templates, setTemplates] = useState([]) + const [templatesLoading, setTemplatesLoading] = useState(false) + const [selectedTemplateId, setSelectedTemplateId] = useState(null) const [showDeleteModal, setShowDeleteModal] = useState(false) const [projectToDelete, setProjectToDelete] = useState(null) const [deleting, setDeleting] = useState(false) @@ -94,14 +122,46 @@ export default function Projects() { } } + const loadTemplates = async () => { + setTemplatesLoading(true) + try { + const response = await api.get('/templates') + // API returns {templates: [], total: number} + setTemplates(response.data.templates || []) + } catch (err) { + console.error('Failed to load templates:', err) + showToast(t('template.loadFailed'), 'error') + } finally { + setTemplatesLoading(false) + } + } + + const handleOpenCreateModal = () => { + setShowCreateModal(true) + setSelectedTemplateId(null) + loadTemplates() + } + const handleCreateProject = async () => { if (!newProject.title.trim()) return setCreating(true) try { - await api.post(`/spaces/${spaceId}/projects`, newProject) + if (selectedTemplateId) { + // Create project from template + await api.post('/templates/create-project', { + template_id: selectedTemplateId, + space_id: spaceId, + title: newProject.title, + description: newProject.description || undefined, + }) + } else { + // Create blank project + await api.post(`/spaces/${spaceId}/projects`, newProject) + } setShowCreateModal(false) setNewProject({ title: '', description: '', security_level: 'department' }) + setSelectedTemplateId(null) loadData() showToast(t('messages.created'), 'success') } catch (err) { @@ -178,7 +238,7 @@ export default function Projects() {

{t('title')}

-
@@ -245,6 +305,73 @@ export default function Projects() { >

{t('createProject')}

+ + {/* Template Selection */} + + {templatesLoading ? ( +
{t('template.loadingTemplates')}
+ ) : ( +
+ {/* Blank Project Option */} +
setSelectedTemplateId(null)} + onKeyDown={(e) => { + if (e.key === 'Enter' || e.key === ' ') { + e.preventDefault() + setSelectedTemplateId(null) + } + }} + role="radio" + aria-checked={selectedTemplateId === null} + tabIndex={0} + > +
{t('template.blankProject')}
+
{t('template.blankProjectDescription')}
+
+ + {/* Template Options */} + {templates.map((template) => ( +
setSelectedTemplateId(template.id)} + onKeyDown={(e) => { + if (e.key === 'Enter' || e.key === ' ') { + e.preventDefault() + setSelectedTemplateId(template.id) + } + }} + role="radio" + aria-checked={selectedTemplateId === template.id} + tabIndex={0} + > +
+
{template.name}
+ {template.is_public && ( + {t('template.publicTemplate')} + )} +
+ {template.description && ( +
{template.description}
+ )} +
+ {t('template.statusCount', { count: template.task_statuses.length })} + {template.custom_fields.length > 0 && ( + {t('template.fieldCount', { count: template.custom_fields.length })} + )} +
+
+ ))} +
+ )} + @@ -267,16 +394,21 @@ export default function Projects() { onChange={(e) => setNewProject({ ...newProject, description: e.target.value })} style={styles.textarea} /> - - + {/* Only show security level for blank projects */} + {!selectedTemplateId && ( + <> + + + + )}
- + {!isMobile && ( + + )}
{/* Column Visibility Toggle - only show when there are custom fields and in list view */} - {viewMode === 'list' && customFields.length > 0 && ( + {!isMobile && viewMode === 'list' && customFields.length > 0 && (
)} + {!isMobile && ( + + )} -
{/* Conditional rendering based on view mode */} {viewMode === 'list' && ( -
- {tasks.map((task) => ( -
handleTaskClick(task)} - > -
-
-
{task.title}
-
- {task.assignee_name && ( - {task.assignee_name} - )} - {task.due_date && ( - - Due: {new Date(task.due_date).toLocaleDateString()} - - )} - {task.time_estimate && ( - - Est: {task.time_estimate}h - - )} - {task.subtask_count > 0 && ( - - {task.subtask_count} subtasks - - )} - {/* Display visible custom field values */} - {task.custom_values && - task.custom_values - .filter((cv) => isColumnVisible(cv.field_id)) - .map((cv) => ( - - {cv.field_name}: {cv.display_value || cv.value || '-'} - - ))} -
+ isMobile ? ( + // Mobile card view +
+ {tasks.map(renderTaskCard)} + {tasks.length === 0 && ( +
+

{t('empty.description')}

- -
- ))} + )} +
+ ) : ( + // Desktop list view with horizontal scroll for wide tables +
+
+ {tasks.map((task) => ( +
handleTaskClick(task)} + > +
+
+
{task.title}
+
+ {task.assignee_name && ( + {task.assignee_name} + )} + {task.due_date && ( + + {t('fields.dueDate')}: {formatDate(task.due_date)} + + )} + {task.time_estimate && ( + + {t('fields.hours', { count: task.time_estimate })} + + )} + {task.subtask_count > 0 && ( + + {t('subtasks.count', { count: task.subtask_count })} + + )} + {/* Display visible custom field values */} + {task.custom_values && + task.custom_values + .filter((cv) => isColumnVisible(cv.field_id)) + .map((cv) => ( + + {cv.field_name}: {cv.display_value || cv.value || '-'} + + ))} +
+
+ +
+ ))} - {tasks.length === 0 && ( -
-

{t('empty.description')}

+ {tasks.length === 0 && ( +
+

{t('empty.description')}

+
+ )}
- )} -
+
+ ) )} {viewMode === 'kanban' && ( - +
+ +
)} {viewMode === 'calendar' && projectId && ( @@ -700,7 +833,7 @@ export default function Tasks() { /> )} - {viewMode === 'gantt' && projectId && ( + {viewMode === 'gantt' && projectId && !isMobile && ( -
+

{t('createTask')}