Merge pull request #248 from Apoplexi/fix/tank-data-lost-after-editing-dive

ericgriffin · web-flow · commit 518ceae12c1b · 2026-05-12T11:11:22.000-04:00
Replace delete &amp; re-insert approach for updating tank data with a real update mechanism
diff --git a/lib/features/dive_log/data/repositories/dive_repository_impl.dart b/lib/features/dive_log/data/repositories/dive_repository_impl.dart
@@ -753,6 +753,21 @@ class DiveRepository {
   Future<void> updateDive(domain.Dive dive) async {
     try {
       _log.info('Updating dive: ${dive.id}');
+
+      // Validate that all tanks have non-empty IDs on update to prevent data loss
+      // (If a tank ID is empty, it would generate a new UUID and cause the old tank to be deleted)
+      final emptyTankIndices = dive.tanks
+          .asMap()
+          .entries
+          .where((e) => e.value.id.isEmpty)
+          .map((e) => e.key);
+      if (emptyTankIndices.isNotEmpty) {
+        throw ArgumentError(
+          'Cannot update dive: tank(s) at index(es) ${emptyTankIndices.join(', ')} '
+          'have empty IDs. All tanks must have valid IDs when updating a dive.',
+        );
+      }
+
       final now = DateTime.now().millisecondsSinceEpoch;
 
       await (_db.update(_db.dives)..where((t) => t.id.equals(dive.id))).write(
@@ -848,44 +863,90 @@ class DiveRepository {
         localUpdatedAt: now,
       );
 
-      // Update tanks: delete and re-insert
-      final existingTanks = await (_db.select(
+      // Update tanks:
+      // Try to match existing tanks by ID to do updates instead of delete+insert when possible,
+      // to preserve sync records and avoid unnecessary deletions/insertions in the sync engine.
+      // Delete tanks that are no longer present.
+      // This is more complex but should result in better sync behavior and performance when tanks
+      // are edited but not completely changed (which is more likely).
+
+      // Get existing tank IDs for this dive
+      final existingTankRows = await (_db.select(
         _db.diveTanks,
       )..where((t) => t.diveId.equals(dive.id))).get();
-      await (_db.delete(
-        _db.diveTanks,
-      )..where((t) => t.diveId.equals(dive.id))).go();
-      for (final tank in existingTanks) {
-        await _syncRepository.logDeletion(
-          entityType: 'diveTanks',
-          recordId: tank.id,
-        );
-      }
+      final existingTankIds = existingTankRows.map((t) => t.id).toSet();
+      final updatedTankIds = <String>{};
+
+      // Update or insert tanks
       for (final tank in dive.tanks) {
-        final tankId = tank.id.isNotEmpty ? tank.id : _uuid.v4();
-        await _db
-            .into(_db.diveTanks)
-            .insert(
-              DiveTanksCompanion(
-                id: Value(tankId),
-                diveId: Value(dive.id),
-                volume: Value(tank.volume),
-                workingPressure: Value(tank.workingPressure),
-                startPressure: Value(tank.startPressure),
-                endPressure: Value(tank.endPressure),
-                o2Percent: Value(tank.gasMix.o2),
-                hePercent: Value(tank.gasMix.he),
-                tankOrder: Value(tank.order),
-                tankRole: Value(tank.role.name),
-                tankMaterial: Value(tank.material?.name),
-                tankName: Value(tank.name),
-                presetName: Value(tank.presetName),
-              ),
-            );
-        await _syncRepository.markRecordPending(
+        // Tank ID is guaranteed to be non-empty by validation at start of method
+        final tankId = tank.id;
+        updatedTankIds.add(tankId);
+
+        if (existingTankIds.contains(tankId)) {
+          // Update existing tank
+          await (_db.update(
+            _db.diveTanks,
+          )..where((t) => t.id.equals(tankId))).write(
+            DiveTanksCompanion(
+              volume: Value(tank.volume),
+              workingPressure: Value(tank.workingPressure),
+              startPressure: Value(tank.startPressure),
+              endPressure: Value(tank.endPressure),
+              o2Percent: Value(tank.gasMix.o2),
+              hePercent: Value(tank.gasMix.he),
+              tankOrder: Value(tank.order),
+              tankRole: Value(tank.role.name),
+              tankMaterial: Value(tank.material?.name),
+              tankName: Value(tank.name),
+              presetName: Value(tank.presetName),
+            ),
+          );
+          // Log as pending update (assuming sync handles updates)
+          await _syncRepository.markRecordPending(
+            entityType: 'diveTanks',
+            recordId: tankId,
+            localUpdatedAt: now,
+          );
+        } else {
+          // Insert new tank
+          await _db
+              .into(_db.diveTanks)
+              .insert(
+                DiveTanksCompanion(
+                  id: Value(tankId),
+                  diveId: Value(dive.id),
+                  volume: Value(tank.volume),
+                  workingPressure: Value(tank.workingPressure),
+                  startPressure: Value(tank.startPressure),
+                  endPressure: Value(tank.endPressure),
+                  o2Percent: Value(tank.gasMix.o2),
+                  hePercent: Value(tank.gasMix.he),
+                  tankOrder: Value(tank.order),
+                  tankRole: Value(tank.role.name),
+                  tankMaterial: Value(tank.material?.name),
+                  tankName: Value(tank.name),
+                  presetName: Value(tank.presetName),
+                ),
+              );
+          await _syncRepository.markRecordPending(
+            entityType: 'diveTanks',
+            recordId: tankId,
+            localUpdatedAt: now,
+          );
+        }
+      }
+
+      // Delete tanks that are no longer present
+      // This will cascade to both tank_pressure_profiles and gas_switches for removed tanks
+      final tanksToDelete = existingTankIds.difference(updatedTankIds);
+      for (final tankId in tanksToDelete) {
+        await (_db.delete(
+          _db.diveTanks,
+        )..where((t) => t.id.equals(tankId))).go();
+        await _syncRepository.logDeletion(
           entityType: 'diveTanks',
           recordId: tankId,
-          localUpdatedAt: now,
         );
       }
 
diff --git a/test/features/dive_log/data/repositories/dive_repository_error_test.dart b/test/features/dive_log/data/repositories/dive_repository_error_test.dart
@@ -128,6 +128,36 @@ void main() {
       );
     });
 
+    test(
+      'should throw ArgumentError when updating dive with a tank that has an empty id',
+      () async {
+        final createdDive = await repository.createDive(
+          Dive(
+            id: 'test-dive',
+            dateTime: DateTime.now(),
+            tanks: const [
+              DiveTank(
+                id: '',
+                volume: 12.0,
+                startPressure: 200,
+                endPressure: 50,
+              ),
+            ],
+          ),
+        );
+
+        final updatedDive = createdDive.copyWith(
+          tanks: [createdDive.tanks[0].copyWith(id: '')],
+          notes: 'Attempt update with empty tank id',
+        );
+
+        await expectLater(
+          repository.updateDive(updatedDive),
+          throwsA(isA<ArgumentError>()),
+        );
+      },
+    );
+
     test(
       'methods that return defaults return correct values on error',
       () async {
diff --git a/test/features/dive_log/data/repositories/dive_repository_test.dart b/test/features/dive_log/data/repositories/dive_repository_test.dart
@@ -1,6 +1,9 @@
+import 'package:drift/drift.dart' hide isNull, isNotNull;
 import 'package:flutter_test/flutter_test.dart';
 import 'package:submersion/core/constants/enums.dart';
+import 'package:submersion/core/database/database.dart' as db;
 import 'package:submersion/core/performance/perf_timer.dart';
+import 'package:submersion/core/services/database_service.dart';
 import 'package:submersion/features/buddies/data/repositories/buddy_repository.dart';
 import 'package:submersion/features/buddies/domain/entities/buddy.dart';
 import 'package:submersion/features/dive_log/data/repositories/dive_repository_impl.dart';
@@ -238,10 +241,16 @@ void main() {
           ),
         );
 
-        final updatedDive = dive.copyWith(
+        // Fetch the dive to get the generated tank IDs
+        final fetchedDive = await repository.getDiveById(dive.id);
+        expect(fetchedDive, isNotNull);
+        expect(fetchedDive!.tanks.isNotEmpty, isTrue);
+        final tankId = fetchedDive.tanks[0].id;
+
+        final updatedDive = fetchedDive.copyWith(
           tanks: [
-            const DiveTank(
-              id: '',
+            DiveTank(
+              id: tankId,
               volume: 15.0,
               startPressure: 210,
               endPressure: 40,
@@ -256,6 +265,140 @@ void main() {
         expect(result.tanks[0].volume, equals(15.0));
         expect(result.tanks[0].startPressure, equals(210));
       });
+
+      test(
+        'should throw error when updating dive with empty tank IDs',
+        () async {
+          final dive = await repository.createDive(
+            createTestDive(
+              tanks: [
+                const DiveTank(
+                  id: '',
+                  volume: 12.0,
+                  startPressure: 200,
+                  endPressure: 50,
+                ),
+              ],
+            ),
+          );
+
+          // Try to update with tanks that have empty IDs (which should fail)
+          final updatedDive = dive.copyWith(
+            tanks: [
+              const DiveTank(
+                id: '', // Empty ID - this should trigger validation error
+                volume: 15.0,
+                startPressure: 210,
+                endPressure: 40,
+              ),
+            ],
+          );
+
+          expect(
+            () => repository.updateDive(updatedDive),
+            throwsA(
+              isA<ArgumentError>().having(
+                (e) => e.message,
+                'message',
+                contains('tank(s) at index(es) 0 have empty IDs'),
+              ),
+            ),
+          );
+        },
+      );
+
+      test('should preserve tank_pressure_profiles and gas_switches', () async {
+        final database = DatabaseService.instance.database;
+
+        // Create a dive with one tank
+        final dive = await repository.createDive(
+          createTestDive(
+            tanks: [
+              const DiveTank(
+                id: '',
+                volume: 12.0,
+                startPressure: 200,
+                endPressure: 50,
+              ),
+            ],
+          ),
+        );
+
+        // Get the actual tank ID
+        final createdDive = await repository.getDiveById(dive.id);
+        expect(createdDive!.tanks.length, equals(1));
+        final tankId = createdDive.tanks[0].id;
+
+        // Insert tank pressure profiles
+        await database
+            .into(database.tankPressureProfiles)
+            .insert(
+              db.TankPressureProfilesCompanion(
+                id: const Value('profile1'),
+                diveId: Value(dive.id),
+                tankId: Value(tankId),
+                timestamp: const Value(0),
+                pressure: const Value(200.0),
+              ),
+            );
+
+        // Insert gas switches
+        await database
+            .into(database.gasSwitches)
+            .insert(
+              db.GasSwitchesCompanion(
+                id: const Value('switch1'),
+                diveId: Value(dive.id),
+                tankId: Value(tankId),
+                timestamp: const Value(300),
+                createdAt: Value(DateTime.now().millisecondsSinceEpoch),
+              ),
+            );
+
+        // Verify initial data
+        var pressureProfiles = await (database.select(
+          database.tankPressureProfiles,
+        )..where((t) => t.diveId.equals(dive.id))).get();
+        expect(pressureProfiles.length, equals(1));
+
+        var gasSwitches = await (database.select(
+          database.gasSwitches,
+        )..where((t) => t.diveId.equals(dive.id))).get();
+        expect(gasSwitches.length, equals(1));
+
+        // Update the dive WITHOUT changing tanks (just update dive fields)
+        final updatedDive = createdDive.copyWith(
+          maxDepth: 25.0,
+          notes: 'Updated dive notes',
+          rating: 5,
+        );
+
+        await repository.updateDive(updatedDive);
+
+        // Verify that tank_pressure_profiles and gas_switches are still present
+        pressureProfiles = await (database.select(
+          database.tankPressureProfiles,
+        )..where((t) => t.diveId.equals(dive.id))).get();
+        expect(
+          pressureProfiles.length,
+          equals(1),
+          reason:
+              'Tank pressure profiles should not be deleted when updating dive',
+        );
+
+        gasSwitches = await (database.select(
+          database.gasSwitches,
+        )..where((t) => t.diveId.equals(dive.id))).get();
+        expect(
+          gasSwitches.length,
+          equals(1),
+          reason: 'Gas switches should not be deleted when updating dive',
+        );
+
+        // Verify the data is unchanged
+        expect(pressureProfiles[0].pressure, equals(200.0));
+        expect(gasSwitches[0].timestamp, equals(300));
+      });
     });
 
     group('deleteDive', () {
