diff --git a/README.md b/README.md index a839e13..9124902 100644 --- a/README.md +++ b/README.md @@ -156,6 +156,7 @@ The insert policy on `group_members` only lets a group's creator add their own f * `add_guest_member(group_id, name)` verifies the caller's membership before inserting a guest. * `update_expense(...)` rewrites an expense and its splits in one transaction, re-checking membership and re-verifying that the splits sum to the amount **server-side**, so client validation is a convenience rather than the enforcement point. * `delete_expense(id)` performs the soft delete. A plain `update` that sets `deleted_at` is rejected, because the resulting row no longer satisfies the `deleted_at is null` select policy. +* `delete_category(id)` detaches every expense / recurring template / tag pointing at a category and then deletes it. A direct `delete from categories` silently removes zero rows when RLS hides the target (a global default, another group's row) and fails on a foreign-key violation wherever the `0012` `on delete set null` cascade isn't in place; the RPC raises a real error for the former and doesn't depend on the latter. * `update_group_photo(group_id, url)` writes the photo URL after the storage upload. * `import_expenses(group_id, items)` promotes reviewed statement rows into expenses and splits in one transaction, re-checks the splits-sum-to-amount invariant per row, and returns `{ inserted, skipped, skipped_fingerprints }` so a row someone else already imported is reported rather than raised as an error. diff --git a/docs/categories-and-tags.md b/docs/categories-and-tags.md index aa4ec5d..7bd24de 100644 --- a/docs/categories-and-tags.md +++ b/docs/categories-and-tags.md @@ -22,13 +22,22 @@ a category never blocks or cascades, the rows that pointed at it just become uncategorized. RLS: `"members manage group categories"` (`for all`) lets a member -insert/update/delete rows scoped to their own group; a null `group_id` makes +insert/update rows scoped to their own group; a null `group_id` makes `is_group_member(group_id)` evaluate to null (not true), so global defaults are correctly read-only from the client. `CategoriesRepository` (`lib/features/expenses/data/categories_repository.dart`) only ever writes with an explicit `group_id` for this reason — there's no path to insert a global row from the app. +**Deletion** goes through the `delete_category(id)` RPC +(`0013_delete_category_rpc.sql`), not a direct `delete`. A plain +`delete from categories` removes zero rows — silently, with no error — when +RLS hides the target (a global default, or another group's category), and +fails outright with a foreign-key violation anywhere the `0012` `on delete +set null` cascade isn't in place. The `SECURITY DEFINER` RPC authorizes the +caller, raises a real error for the default / wrong-group cases, and detaches +every reference before deleting — the same pattern as `delete_expense`. + ## Tags are `merchant_rules`, not a separate table A tag — *"COSTCO always means Groceries"* — is the exact shape the statement @@ -89,7 +98,8 @@ lib/features/labels/ui/ labels_screen.dart standalone route (old /import/rules redirects here) category_dialog.dart name + icon picker tag_dialog.dart keyword + category, match type behind "Advanced" -supabase/migrations/0012_categories.sql +supabase/migrations/0012_categories.sql FKs -> on delete set null, unique names +supabase/migrations/0013_delete_category_rpc.sql delete_category() RPC test/features/import/merchant_rules_test.dart matchMerchantRule, matchTagCategory test/features/labels/labels_tab_test.dart ``` diff --git a/lib/features/balances/ui/balances_tab.dart b/lib/features/balances/ui/balances_tab.dart index 6eb6745..7d9cca7 100644 --- a/lib/features/balances/ui/balances_tab.dart +++ b/lib/features/balances/ui/balances_tab.dart @@ -4,7 +4,6 @@ import 'package:tally/core/money.dart'; import 'package:tally/features/balances/data/balances_repository.dart'; import 'package:tally/features/balances/logic/settle.dart'; import 'package:tally/features/balances/providers/balances_provider.dart'; -import 'package:tally/features/expenses/providers/expenses_provider.dart'; import 'package:tally/features/groups/providers/groups_provider.dart'; import 'package:tally/models/group_member.dart'; import 'package:flutter/material.dart'; @@ -156,8 +155,7 @@ class _SettlementRowState extends ConsumerState<_SettlementRow> { toMemberId: t.toMemberId, amount: t.amount.toString(), ); - ref.invalidate(balancesProvider(widget.groupId)); - ref.invalidate(expensesProvider(widget.groupId)); + invalidateGroupMoney(ref, widget.groupId); } on Exception catch (e) { if (mounted) { ScaffoldMessenger.of(context) diff --git a/lib/features/expenses/data/categories_repository.dart b/lib/features/expenses/data/categories_repository.dart index 2e2d48b..ef393c9 100644 --- a/lib/features/expenses/data/categories_repository.dart +++ b/lib/features/expenses/data/categories_repository.dart @@ -39,10 +39,12 @@ class CategoriesRepository { .update({'name': name.trim(), 'icon': icon}).eq('id', id); } - // Any expense, recurring template or tag pointing at this category falls - // back to uncategorized (on delete set null, migration 0012) rather than - // blocking the delete. + // Routed through the delete_category RPC (migration 0013), not a direct + // delete. The RPC detaches any expense/recurring/tag pointing at the + // category before removing it, and — unlike a plain delete, which just + // removes zero rows and reports nothing — raises a real error when the + // target is a global default or lives in another group. Future deleteCategory({required String id}) async { - await supabase.from('categories').delete().eq('id', id); + await supabase.rpc('delete_category', params: {'p_category_id': id}); } } diff --git a/lib/features/groups/providers/groups_provider.dart b/lib/features/groups/providers/groups_provider.dart index a5d7056..40a3e96 100644 --- a/lib/features/groups/providers/groups_provider.dart +++ b/lib/features/groups/providers/groups_provider.dart @@ -1,3 +1,5 @@ +import 'package:tally/features/balances/providers/balances_provider.dart'; +import 'package:tally/features/expenses/providers/expenses_provider.dart'; import 'package:tally/features/groups/data/groups_repository.dart'; import 'package:tally/models/group.dart'; import 'package:tally/models/group_member.dart'; @@ -27,3 +29,15 @@ final groupProvider = FutureProvider.family((ref, groupId) { final membersProvider = FutureProvider.family, String>( (ref, groupId) => GroupsRepository().fetchMembers(groupId: groupId), ); + +// Call after anything changes a group's money — adding, editing, deleting or +// importing an expense, or recording a settlement. Refreshes the expense list +// and the in-group balances, and — easy to forget — groupSummariesProvider, +// the separate view behind the "you owe / you're owed" tally on the groups +// list. Nothing else invalidates it, so skipping it leaves that tally stale +// until a manual pull-to-refresh. +void invalidateGroupMoney(WidgetRef ref, String groupId) { + ref.invalidate(expensesProvider(groupId)); + ref.invalidate(balancesProvider(groupId)); + ref.invalidate(groupSummariesProvider); +} diff --git a/lib/features/groups/ui/group_detail_screen.dart b/lib/features/groups/ui/group_detail_screen.dart index 36877ad..b6158b4 100644 --- a/lib/features/groups/ui/group_detail_screen.dart +++ b/lib/features/groups/ui/group_detail_screen.dart @@ -2,7 +2,6 @@ import 'package:tally/core/icons.dart'; import 'package:tally/core/money.dart'; import 'package:tally/core/supabase_client.dart'; import 'package:tally/core/widgets/page_body.dart'; -import 'package:tally/features/balances/providers/balances_provider.dart'; import 'package:tally/features/balances/ui/balances_tab.dart'; import 'package:tally/features/expenses/data/expenses_repository.dart'; import 'package:tally/features/expenses/providers/categories_provider.dart'; @@ -42,14 +41,12 @@ class _GroupDetailScreenState extends ConsumerState Future _addExpense() async { await context.push('/groups/${widget.groupId}/expenses/new'); - ref.invalidate(expensesProvider(widget.groupId)); - ref.invalidate(balancesProvider(widget.groupId)); + invalidateGroupMoney(ref, widget.groupId); } Future _importStatement() async { await context.push('/groups/${widget.groupId}/import'); - ref.invalidate(expensesProvider(widget.groupId)); - ref.invalidate(balancesProvider(widget.groupId)); + invalidateGroupMoney(ref, widget.groupId); } Future _addGuest() async { @@ -182,10 +179,7 @@ class _ExpenseCard extends ConsumerWidget { final String groupId; final Category? category; - void _refresh(WidgetRef ref) { - ref.invalidate(expensesProvider(groupId)); - ref.invalidate(balancesProvider(groupId)); - } + void _refresh(WidgetRef ref) => invalidateGroupMoney(ref, groupId); Future _edit(BuildContext context, WidgetRef ref) async { await context.push('/groups/$groupId/expenses/${expense.id}/edit'); diff --git a/supabase/migrations/0013_delete_category_rpc.sql b/supabase/migrations/0013_delete_category_rpc.sql new file mode 100644 index 0000000..bb43cf2 --- /dev/null +++ b/supabase/migrations/0013_delete_category_rpc.sql @@ -0,0 +1,55 @@ +-- TALLY — deletable categories, take two +-- +-- 0012 switched the category foreign keys to ON DELETE SET NULL so a category +-- still referenced by an expense could be removed without orphaning rows. That +-- fixes the database, but the client still deletes categories with a plain +-- delete from categories where id = ? +-- and that has two failure modes the user sees as "can't delete categories": +-- +-- 1. It silently deletes zero rows whenever RLS hides the target — a global +-- default (group_id is null), or a category owned by another group. No +-- error comes back, the list refetches, the category is still there. +-- 2. Where the SET NULL cascade from 0012 isn't in place (an environment +-- still on <= 0011), the delete fails outright with a foreign-key +-- violation from expenses / recurring_expenses / merchant_rules. +-- +-- Route deletion through a SECURITY DEFINER RPC instead — the same pattern +-- delete_expense, update_expense and update_group_photo already use. It +-- authorizes the caller as a member of the category's group, raises a clear +-- error for the cases that should fail, detaches every reference explicitly +-- (so it works regardless of the FK action), then deletes. + +create or replace function delete_category(p_category_id uuid) +returns void +language plpgsql +security definer +set search_path = public +as $$ +declare + v_group_id uuid; +begin + select group_id into v_group_id + from categories + where id = p_category_id; + + if not found then + raise exception 'Category not found'; + end if; + -- Global defaults belong to every group; a single group can't delete them. + if v_group_id is null then + raise exception 'Default categories cannot be deleted'; + end if; + if not is_group_member(v_group_id) then + raise exception 'Not authorized'; + end if; + + -- Detach references explicitly so the delete succeeds even where the + -- ON DELETE SET NULL cascade from 0012 never ran. Only this group's own + -- rows can point at the category, and SECURITY DEFINER clears RLS. + update expenses set category_id = null where category_id = p_category_id; + update recurring_expenses set category_id = null where category_id = p_category_id; + update merchant_rules set category_id = null where category_id = p_category_id; + + delete from categories where id = p_category_id; +end; +$$; diff --git a/test/features/groups/group_money_invalidation_test.dart b/test/features/groups/group_money_invalidation_test.dart new file mode 100644 index 0000000..2ba4735 --- /dev/null +++ b/test/features/groups/group_money_invalidation_test.dart @@ -0,0 +1,66 @@ +import 'package:tally/features/balances/providers/balances_provider.dart'; +import 'package:tally/features/expenses/providers/expenses_provider.dart'; +import 'package:tally/features/groups/providers/groups_provider.dart'; +import 'package:tally/models/expense.dart'; +import 'package:tally/models/member_balance.dart'; +import 'package:tally/models/group_summary.dart'; +import 'package:flutter/material.dart'; +import 'package:flutter_riverpod/flutter_riverpod.dart'; +import 'package:flutter_test/flutter_test.dart'; + +// Regression: deleting (or adding/editing/importing) an expense used to refresh +// only the in-group providers, leaving groupSummariesProvider — the "you owe / +// you're owed" tally on the groups list — stale until a manual refresh. +// invalidateGroupMoney must hit all three. +void main() { + testWidgets('invalidateGroupMoney refetches the groups-list tally too', + (tester) async { + var expenseFetches = 0; + var balanceFetches = 0; + var summaryFetches = 0; + + late WidgetRef ref; + await tester.pumpWidget( + ProviderScope( + overrides: [ + expensesProvider('g1').overrideWith((ref) async { + expenseFetches++; + return []; + }), + balancesProvider('g1').overrideWith((ref) async { + balanceFetches++; + return []; + }), + groupSummariesProvider.overrideWith((ref) async { + summaryFetches++; + return []; + }), + ], + child: Consumer( + builder: (_, r, __) { + ref = r; + r.watch(expensesProvider('g1')); + r.watch(balancesProvider('g1')); + r.watch(groupSummariesProvider); + return const SizedBox(); + }, + ), + ), + ); + await tester.pumpAndSettle(); + expect( + [expenseFetches, balanceFetches, summaryFetches], + [1, 1, 1], + reason: 'each provider fetched once on first watch', + ); + + invalidateGroupMoney(ref, 'g1'); + await tester.pumpAndSettle(); + + expect( + [expenseFetches, balanceFetches, summaryFetches], + [2, 2, 2], + reason: 'all three refetch, including the groups-list summary', + ); + }); +}