Fix: Add DELETE RLS policies and UI for tasks and tickets #85
Conversation
📝 WalkthroughWalkthroughThis pull request adds complete delete functionality for tasks and tickets by implementing row-level security (RLS) DELETE policies in the database and corresponding UI delete flows in both task and ticket screens with confirmation dialogs and user feedback. Changes
Sequence Diagram(s)sequenceDiagram
participant User as User (Creator/Admin)
participant UI as Task/Ticket Screen
participant Supabase as Supabase Service
participant DB as PostgreSQL + RLS
User->>UI: Tap Delete Button
UI->>UI: Show Confirmation Dialog
User->>UI: Confirm Deletion
UI->>Supabase: deleteTask() / deleteTicket()
Supabase->>DB: DELETE with auth.uid()
DB->>DB: Check RLS Policy<br/>(creator OR admin check)
alt Policy Allows
DB-->>Supabase: Success
Supabase-->>UI: Success Response
UI->>UI: Show Success SnackBar
UI->>UI: Refresh List / Pop Screen
else Policy Denies
DB-->>Supabase: Permission Denied
Supabase-->>UI: Error Response
UI->>UI: Show Error SnackBar
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (3)
supabase/migrations/20251021040000_tickets_schema.sql (1)
90-99: Tickets DELETE policy is correct; considerEXISTSfor consistency/readability.
This is functionally fine, but matching the tasks pattern (EXISTS ... WHERE id = auth.uid() ...) is easier to reason about.sqls/04_tickets_schema.sql (1)
90-99: Tickets DELETE policy parity with migrations looks good.
Same minor style note:EXISTSwould align with tasks policy. Based on learnings, duplicating policy definitions acrosssqls/and migrations is acceptable for dev clarity.lib/screens/tasks/task_detail_screen.dart (1)
698-717: Harden comment timestamp parsing (DateTime.parse) to avoid runtime exceptions.
Ifcreated_atis ever null/invalid, Line 701-703 will throw and break the screen.- final createdAt = - DateTime.parse(comment['created_at']); + final createdAtRaw = comment['created_at']; + final createdAt = createdAtRaw is String + ? DateTime.tryParse(createdAtRaw) + : null; ... - final difference = now.difference(createdAt); + final difference = createdAt == null + ? Duration.zero + : now.difference(createdAt);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
lib/screens/tasks/task_detail_screen.dart(26 hunks)lib/screens/tickets/ticket_detail_screen.dart(32 hunks)lib/services/supabase_service.dart(70 hunks)sqls/03_task_schema.sql(1 hunks)sqls/04_tickets_schema.sql(2 hunks)supabase/migrations/20251021030000_task_schema.sql(1 hunks)supabase/migrations/20251021040000_tickets_schema.sql(2 hunks)
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: SharkyBytes
Repo: AOSSIE-Org/Ell-ena PR: 11
File: sqls/03_task_schema.sql:57-78
Timestamp: 2025-07-06T10:06:27.181Z
Learning: SharkyBytes prefers to keep separate database RLS policies during the development phase in the Ell-ena project for clarity and debugging purposes, even when they might be combined for better security. They handle complex authorization logic on the application side during development, with plans to optimize database-level security later when focusing on production readiness.
📚 Learning: 2025-07-06T10:17:03.359Z
Learnt from: SharkyBytes
Repo: AOSSIE-Org/Ell-ena PR: 11
File: lib/screens/tickets/ticket_screen.dart:398-407
Timestamp: 2025-07-06T10:17:03.359Z
Learning: SharkyBytes prefers visual-only reordering for tickets in the Ell-ena Flutter project without backend persistence, allowing users to organize their view temporarily while keeping the implementation simple during development.
Applied to files:
lib/screens/tickets/ticket_detail_screen.dart
🔇 Additional comments (7)
sqls/03_task_schema.sql (1)
67-79: DELETE RLS policy looks correct and aligned with objective.
Creator-or-team-admin constraint is clear and consistent with the rest of the schema. Based on learnings, keeping this as a separate policy is also fine for debugging clarity.supabase/migrations/20251021030000_task_schema.sql (1)
67-79: Migration DELETE policy matches intended authorization.
Good parity with thesqls/version.supabase/migrations/20251021040000_tickets_schema.sql (1)
125-130: Ticket comment DELETE policy matches “author can delete” requirement.sqls/04_tickets_schema.sql (1)
125-130: Ticket comments DELETE policy parity with migrations looks good.lib/screens/tasks/task_detail_screen.dart (1)
371-379: Delete button guard is good, but ensurecurrentUsercan’t crash before service init.
This call path depends onSupabaseService.currentUserbeing safe even ifinitialize()hasn’t run yet (see service review).lib/screens/tickets/ticket_detail_screen.dart (1)
544-623: Menu visibility guard is correct; ensurecurrentUseris safe pre-init.
This UI check depends onSupabaseService.currentUserbeing non-throwing even if the service wasn’t initialized yet.lib/services/supabase_service.dart (1)
300-305: No call sites found forcreateTeamin the codebase. The function definition exists at lines 300-305 with the updatedpasswordparameter, but no actual invocations of this method were detected. If this is a newly added function, verification is not needed. If call sites exist elsewhere, they should be added with thepasswordparameter.
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
lib/screens/tasks/task_detail_screen.dart (2)
73-112: Blocker:_updateTaskStatustreats a Map response asbool(status updates likely always show failure).
SupabaseService.updateTaskStatus(...)returnsMap<String, dynamic>, but the UI checkssuccess == true.- final success = await _supabaseService.updateTaskStatus( + final result = await _supabaseService.updateTaskStatus( taskId: widget.taskId, status: status, ); if (mounted) { - if (success == true) { + if (result['success'] == true) { // Reload task details await _loadTaskDetails(); ScaffoldMessenger.of(context).showSnackBar( SnackBar( content: Text( 'Task status updated to ${_getStatusLabel(status)}', ), backgroundColor: Colors.green, ), ); } else { ScaffoldMessenger.of(context).showSnackBar( - const SnackBar( - content: Text('Failed to update task status'), - backgroundColor: Colors.red, - ), + SnackBar( + content: Text('Failed to update task status: ${result['error'] ?? 'Unknown error'}'), + backgroundColor: Colors.red, + ), ); } }
114-147: Approval path always “succeeds” in UI even if backend fails; also_loadTaskDetails()isn’t awaited.
You currently ignore the service result and show a success SnackBar unconditionally.- await _supabaseService.updateTaskApproval( + final result = await _supabaseService.updateTaskApproval( taskId: widget.taskId, approvalStatus: approvalStatus, ); - // Reload task details - _loadTaskDetails(); + if (!mounted) return; + + if (result['success'] == true) { + // Reload task details + await _loadTaskDetails(); + + ScaffoldMessenger.of(context).showSnackBar( + SnackBar( + content: Text( + 'Task ${approvalStatus == 'approved' ? 'approved' : 'rejected'}', + ), + backgroundColor: + approvalStatus == 'approved' ? Colors.green : Colors.red, + ), + ); + } else { + ScaffoldMessenger.of(context).showSnackBar( + SnackBar( + content: Text('Failed to update approval: ${result['error'] ?? 'Unknown error'}'), + backgroundColor: Colors.red, + ), + ); + } - - // Show success message - if (mounted) { - ScaffoldMessenger.of(context).showSnackBar( - SnackBar( - content: Text( - 'Task ${approvalStatus == 'approved' ? 'approved' : 'rejected'}', - ), - backgroundColor: - approvalStatus == 'approved' ? Colors.green : Colors.red, - ), - ); - }
🧹 Nitpick comments (3)
lib/screens/tasks/task_detail_screen.dart (1)
149-213: Delete UX + permission gating look correct, and avoids “SnackBar after pop” context issues.
Optional: consider relying solely onpop(true)and letting the parent screen show the success SnackBar (so it’s guaranteed visible on the list screen).Also applies to: 361-369
lib/screens/tickets/ticket_detail_screen.dart (1)
90-156: Delete flow looks correct and fixes the prior “SnackBar after pop” hazard.
Same optional suggestion as tasks: you can drop the success SnackBar here and let the caller show it after receivingpop(true).lib/services/supabase_service.dart (1)
816-866: Use the documented pattern for conditionally applied filters to ensure consistency with Supabase best practices.PostgrestFilterBuilder methods mutate in place and return
this, but the official Supabase Dart documentation specifically recommends reassigning when conditionally applying filters:if (name != null) query = query.eq('name', name);Apply this pattern to
getTasksandgetTicketsto maintain clarity and consistency:- final query = _client.from('tasks').select('*').eq('team_id', teamId); + var query = _client.from('tasks').select('*').eq('team_id', teamId); if (filterByAssignment && !isAdmin) { - query.eq('assigned_to', userId); + query = query.eq('assigned_to', userId); } else if (filterByAssignment) { - query.eq('assigned_to', userId); + query = query.eq('assigned_to', userId); } if (filterByStatus != null) { - query.eq('status', filterByStatus); + query = query.eq('status', filterByStatus); } if (filterByDueDate != null) { ... - query.gte('due_date', startOfDay.toIso8601String()); - query.lte('due_date', endOfDay.toIso8601String()); + query = query.gte('due_date', startOfDay.toIso8601String()); + query = query.lte('due_date', endOfDay.toIso8601String()); } final response = await query.order('created_at', ascending: false);Also apply to
getTickets(lines 1275-1316).
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
lib/screens/tasks/task_detail_screen.dart(28 hunks)lib/screens/tickets/ticket_detail_screen.dart(32 hunks)lib/services/supabase_service.dart(70 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-07-06T10:17:03.359Z
Learnt from: SharkyBytes
Repo: AOSSIE-Org/Ell-ena PR: 11
File: lib/screens/tickets/ticket_screen.dart:398-407
Timestamp: 2025-07-06T10:17:03.359Z
Learning: SharkyBytes prefers visual-only reordering for tickets in the Ell-ena Flutter project without backend persistence, allowing users to organize their view temporarily while keeping the implementation simple during development.
Applied to files:
lib/screens/tickets/ticket_detail_screen.dart
🔇 Additional comments (6)
lib/services/supabase_service.dart (3)
13-44: Good:currentUseris now safe pre-initialize (fixes prior LateInitializationError risk).
This resolves the “access before initialize()” crash scenario cleanly.
45-74: Fail-fast on missing Supabase config looks good.
Throwing on emptySUPABASE_URL/SUPABASE_ANON_KEYis much better than limping along with a partially initialized client.
195-219: Mostly formatting / line-break refactors and minor wiring changes — LGTM.
No obvious functional regressions in these hunks by inspection.Also applies to: 486-487, 725-741, 770-786, 851-866, 951-952, 1040-1043, 1065-1067, 1167-1173, 1261-1271, 1438-1440, 1485-1489, 1526-1530, 1576-1580, 1605-1607, 1683-1689, 1795-1799, 1916-1918, 2073-2075
lib/screens/tasks/task_detail_screen.dart (1)
8-8: UI polish / defensive null-handling changes look fine.
No issues worth blocking on these styling/formatting hunks.Also applies to: 44-71, 84-91, 331-369, 399-450, 486-517, 540-643, 716-758, 832-835
lib/screens/tickets/ticket_detail_screen.dart (2)
544-622: Popup menu gating (creator/admin) + admin-only status/priority controls are well-scoped.
Creators get delete; only admins get mutation controls beyond delete.
35-88: Remaining changes are mostly presentation + safer fallbacks — LGTM.
No new behavioral concerns spotted in these hunks.Also applies to: 329-419, 421-432, 645-675, 724-733, 757-851, 904-969, 1001-1017, 1026-1035
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
lib/services/supabase_service.dart (1)
2054-2087: Inconsistent deletion verification compared todeleteTaskanddeleteTicket.Unlike
deleteTask(line 1226) anddeleteTicket(line 1736), this method doesn't verify whether the deletion succeeded or provide permission-denied feedback. If RLS blocks the delete, this method will silently returnsuccess: true, misleading the user.Apply this diff to align with the pattern used in
deleteTaskanddeleteTicket:- // Delete the meeting - await _client.from('meetings').delete().eq('id', meetingId); + // Delete the meeting (RLS will handle authorization) + final response = await _client + .from('meetings') + .delete() + .eq('id', meetingId) + .select('id'); + + // Verify the meeting was actually deleted + if (response.isEmpty) { + // Meeting might not have been deleted due to RLS policy + // Check if meeting still exists + final checkMeeting = await _client + .from('meetings') + .select('id') + .eq('id', meetingId) + .maybeSingle(); + + return { + 'success': false, + 'error': checkMeeting != null + ? 'Permission denied: You can only delete meetings you created or if you are an admin' + : 'Meeting not found or permission denied', + }; + }
🧹 Nitpick comments (2)
lib/services/supabase_service.dart (2)
1206-1256: Consider refreshing the tasks stream after successful deletion.The implementation correctly verifies deletion and provides clear error messages. However, unlike other task mutation methods (e.g.,
createTaskat line 951), this method doesn't refresh the tasks stream. This means the UI won't automatically reflect the deletion unless manually refreshed.Consider adding after line 1248:
return { 'success': true, }; + + // Refresh tasks stream + await getTasks();Note: You may want to add the refresh before the return, similar to how
createTicket(line 1448) and other methods handle it.
1717-1770: Consider refreshing the tickets stream after successful deletion.Similar to
deleteTask, this method doesn't refresh the tickets stream after successful deletion. For consistency with other ticket mutations (e.g.,updateTicketStatusat line 1490), consider adding a stream refresh.Add after line 1762:
return { 'success': true, }; + + // Refresh tickets stream + await getTickets();
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
lib/services/supabase_service.dart(70 hunks)
🔇 Additional comments (1)
lib/services/supabase_service.dart (1)
43-43: LGTM! Initialization guard prevents crashes.The
currentUsergetter now safely guards againstLateInitializationErrorby checking_isInitializedbefore accessing_client.auth.currentUser.
|
Ok @may-tas, LGTM! 👍 |
Hey @may-tas , any updates? |
|
@SharkyBytes Will push a commit by tomorrow. |
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
sqls/03_task_schema.sqlsqls/04_tickets_schema.sql
🚧 Files skipped from review as they are similar to previous changes (1)
- sqls/03_task_schema.sql
- added coderabbit ai suggestions
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
lib/screens/tasks/task_screen.dart (2)
677-762: Delete workflow implementation looks solid, but repeatedSupabaseService()instantiation is inefficient.The delete confirmation flow, error handling with
context.mountedchecks, and list refresh viaTaskScreen.refreshTasks()are well-implemented.However, the code creates new
SupabaseService()instances at lines 680, 717, and 764 within the build method. SinceSupabaseServiceis already instantiated at line 552, reuse that instance or pass the service reference to avoid repeated instantiation.♻️ Suggested refactor: Reuse existing SupabaseService instance
- if (task['created_by'] == - SupabaseService() - .client - .auth - .currentUser - ?.id || - isAdmin) + if (task['created_by'] == + supabaseService + .client + .auth + .currentUser + ?.id || + isAdmin)And similarly for line 717:
- final result = await SupabaseService() - .deleteTask(task['id']); + final result = await supabaseService + .deleteTask(task['id']);
763-770: Duplicate permission check for conditional spacing.Lines 763-770 repeat the same permission check just to add an 8px
SizedBox. This duplicates the logic from lines 679-685.Consider restructuring to avoid the duplication, perhaps by wrapping both the delete button and spacing in a single conditional block.
♻️ Suggested consolidation
- if (task['created_by'] == - SupabaseService() - .client - .auth - .currentUser - ?.id || - isAdmin) - const SizedBox(width: 8), + // Move the SizedBox inside the previous if block, after the IconButtonlib/screens/tickets/ticket_screen.dart (2)
692-777: Delete workflow implemented correctly, but same refactoring opportunity applies.The delete confirmation, error handling, and refresh logic are well-structured. The
context.mountedchecks prevent state updates on unmounted widgets.Same recommendation: reuse the
supabaseServiceinstance from line 550 instead of creating new instances at lines 695, 732, and 779.♻️ Suggested refactor: Reuse existing SupabaseService instance
- if (ticket['created_by'] == - SupabaseService() - .client - .auth - .currentUser - ?.id || - isAdmin) + if (ticket['created_by'] == + supabaseService + .client + .auth + .currentUser + ?.id || + isAdmin)And for line 732:
- final result = await SupabaseService() - .deleteTicket(ticket['id']); + final result = await supabaseService + .deleteTicket(ticket['id']);
778-785: Duplicate permission check for spacing (same pattern as task_screen.dart).Consider consolidating with the main permission check block to reduce duplication.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
lib/screens/tasks/task_screen.dartlib/screens/tickets/ticket_screen.dart
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-07-06T10:17:03.359Z
Learnt from: SharkyBytes
Repo: AOSSIE-Org/Ell-ena PR: 11
File: lib/screens/tickets/ticket_screen.dart:398-407
Timestamp: 2025-07-06T10:17:03.359Z
Learning: SharkyBytes prefers visual-only reordering for tickets in the Ell-ena Flutter project without backend persistence, allowing users to organize their view temporarily while keeping the implementation simple during development.
Applied to files:
lib/screens/tickets/ticket_screen.dart
🔇 Additional comments (5)
lib/screens/tasks/task_screen.dart (2)
146-151: Potential infinite refresh or no-op on globalKey mismatch.When
globalKey.currentState != this, the scheduled refresh callsTaskScreen.refreshTasks()which uses the sameglobalKey. If the key isn't pointing to this instance, the refresh either targets a different state or does nothing. This check may cause unintended behavior or redundant refresh cycles.Consider whether this defensive check is necessary, or if the globalKey should be reassigned/fixed at construction time rather than triggering a refresh.
550-573: Good use of "You" label for current user display.The logic to display "You" when the current user is the creator or assignee improves UX clarity. The null checks and fallback to 'Unknown'/'Unassigned' are appropriate.
lib/screens/tickets/ticket_screen.dart (3)
136-141: Same globalKey mismatch concern as task_screen.dart.This defensive check has the same potential issue: if the key doesn't point to this state, the refresh may not work as intended or could cause redundant refresh cycles.
415-423: Visual-only reordering is intentional per project preferences.The
onReordercallback only updates the local list without backend persistence. Based on learnings, this is the preferred approach for temporary visual organization during development.
946-964: Helper methods_formatDateand_limitWordsare clean and defensive.Both methods handle null/empty inputs gracefully and have appropriate try-catch for date parsing.
@SharkyBytes I have done the needed. Please review and merge. Screen.Recording.2026-01-09.at.2.42.10.AM.mp4 |
2 participants
Changes
Database (SQL)
taskstable (creator + admin access)ticketstable (creator + admin access)ticket_commentstable (comment author access)sqls/andsupabase/migrations/foldersBackend (
lib/services/supabase_service.dart)deleteTask(String taskId)method with error handlingdeleteTicket(String ticketId)method with error handlingcurrentUsergetter for permission checksUI
Security
Testing
📷 Screenshots or Visual Changes (if applicable)
proof-of-work-ell-ena.mp4
✅ Checklist
Summary by CodeRabbit
New Features
Improvements
✏️ Tip: You can customize this high-level summary in your review settings.