Merge remote-tracking branch 'origin/feature/macdive-native-xml' into feature/macdive-sqlite

Author: ericgriffin

lib/features/universal_import/data/parsers/macdive_xml_parser.dart

@@ -109,11 +109,14 @@ class MacDiveXmlParser implements ImportParser {
       }
 
       if (dive.buddies.isNotEmpty) {
+        // Per-dive dedup: a dive with duplicate `<buddy>` entries would
+        // otherwise emit repeated refs. The buddy repo tolerates this by
+        // UPSERTing, but the redundant round-trips are wasted work.
         final buddyRefs = <String>[];
         for (final buddy in dive.buddies) {
           final trimmed = buddy.trim();
           if (trimmed.isEmpty) continue;
-          buddyRefs.add(trimmed);
+          if (!buddyRefs.contains(trimmed)) buddyRefs.add(trimmed);
           buddiesByName.putIfAbsent(
             trimmed,
             () => <String, dynamic>{'name': trimmed, 'uddfId': trimmed},
@@ -130,18 +133,34 @@ class MacDiveXmlParser implements ImportParser {
         }
       }
 
-      for (final g in dive.gear) {
-        final key = _gearKey(g);
-        if (key.isEmpty) continue;
-        gearByKey.putIfAbsent(key, () => _mapGear(g));
+      // Collect gear: record a per-dive `equipmentRefs` list keyed by the
+      // same composite key used for dedup, so `UddfEntityImporter` can link
+      // the imported equipment entities back to the dive via its
+      // `equipmentIdMapping` (uddfId -> newId) lookup. Without this, dedup
+      // still produces the entity list but the dive wouldn't reference any
+      // of it.
+      if (dive.gear.isNotEmpty) {
+        final equipmentRefs = <String>[];
+        for (final g in dive.gear) {
+          final key = _gearKey(g);
+          if (key.isEmpty) continue;
+          if (!equipmentRefs.contains(key)) equipmentRefs.add(key);
+          gearByKey.putIfAbsent(key, () => _mapGear(g, uddfId: key));
+        }
+        if (equipmentRefs.isNotEmpty) {
+          diveMap['equipmentRefs'] = equipmentRefs;
+        }
       }
 
       if (dive.tags.isNotEmpty) {
+        // Per-dive dedup: `dive_tags` has no UNIQUE(diveId, tagId)
+        // constraint, so duplicate `<tag>` entries would create duplicate
+        // junction rows that surface as a tag appearing twice on a dive.
         final tagNames = <String>[];
         for (final tag in dive.tags) {
           final trimmed = tag.trim();
           if (trimmed.isEmpty) continue;
-          tagNames.add(trimmed);
+          if (!tagNames.contains(trimmed)) tagNames.add(trimmed);
           tagsByName.putIfAbsent(
             trimmed,
             () => <String, dynamic>{'name': trimmed, 'uddfId': trimmed},
@@ -225,8 +244,8 @@ class MacDiveXmlParser implements ImportParser {
     if (entryMethod != null) map['entryMethod'] = entryMethod.name;
     if (d.computer != null) map['diveComputerModel'] = d.computer;
     if (d.serial != null) map['diveComputerSerial'] = d.serial;
-    // MacDive rating is a 0.0-5.0 float; Submersion stores 0-5 int.
-    if (d.rating != null) map['rating'] = d.rating!.clamp(0.0, 5.0).round();
+    final rating = MacDiveValueMapper.rating(d.rating);
+    if (rating != null) map['rating'] = rating;
 
     // Tanks: each <gas> becomes a tank map using the same key conventions as
     // the Subsurface and UDDF parsers so `UddfEntityImporter._buildTanks` can
@@ -290,8 +309,13 @@ class MacDiveXmlParser implements ImportParser {
     return map;
   }
 
-  Map<String, dynamic> _mapGear(MacDiveXmlGearItem g) {
-    final map = <String, dynamic>{};
+  Map<String, dynamic> _mapGear(
+    MacDiveXmlGearItem g, {
+    required String uddfId,
+  }) {
+    // `uddfId` is the gear's composite dedup key, referenced from each dive's
+    // `equipmentRefs`. UddfEntityImporter resolves it via `equipmentIdMapping`.
+    final map = <String, dynamic>{'uddfId': uddfId};
     if (g.name != null) map['name'] = g.name;
     if (g.manufacturer != null) map['brand'] = g.manufacturer;
     if (g.type != null) map['type'] = g.type;

lib/features/universal_import/data/services/macdive_xml_reader.dart

@@ -24,6 +24,17 @@ class MacDiveXmlReader {
     final doc = XmlDocument.parse(content);
     final root = doc.rootElement;
 
+    // Reject non-MacDive XML early: MacDive native XML must have <dives> at
+    // the root. Without this check, a user who forces a source override onto
+    // a UDDF or other dive XML would get a silent empty logbook because our
+    // `findElements('dive')` walk doesn't match anything at UDDF's top level.
+    // The parser's try/catch converts this into a user-visible ImportWarning.
+    if (root.name.local != 'dives') {
+      throw const FormatException(
+        'Not a MacDive native XML document: expected <dives> root element',
+      );
+    }
+
     final units = MacDiveUnitSystem.fromXml(_text(root, 'units'));
     final converter = MacDiveUnitConverter(units);
     final schemaVersion = _text(root, 'schema');

test/features/universal_import/data/parsers/macdive_xml_parser_test.dart

@@ -101,6 +101,17 @@ void main() {
       expect((profile[1] as Map)['timestamp'], 60);
     });
 
+    test('dive links gear via equipmentRefs with matching uddfId', () async {
+      final payload = await const MacDiveXmlParser().parse(bytes);
+      final dive = payload.entitiesOf(ImportEntityType.dives).first;
+      final equipment = payload.entitiesOf(ImportEntityType.equipment);
+      // Fixture has one gear item: manufacturer=Test, name=BCD1, no serial.
+      // Composite key "Test|BCD1|" lands on both sides.
+      expect(dive['equipmentRefs'], ['Test|BCD1|']);
+      expect(equipment.first['uddfId'], 'Test|BCD1|');
+      expect(equipment.first['name'], 'BCD1');
+    });
+
     test(
       'dive maps boat/diveOperator/weather into existing UDDF key names',
       () async {
@@ -213,9 +224,47 @@ void main() {
         final equipment = payload.entitiesOf(ImportEntityType.equipment);
         expect(equipment.length, 1, reason: 'only the populated item survives');
         expect(equipment.first['name'], 'BCD1');
+        // equipmentRefs reflects only the surviving item; skipped empty
+        // `<item>` elements do not leave phantom refs on the dive either.
+        final dive = payload.entitiesOf(ImportEntityType.dives).first;
+        expect(dive['equipmentRefs'], ['Test|BCD1|']);
       },
     );
 
+    test('duplicate <buddy> entries on one dive dedupe in buddyRefs', () async {
+      const xml = '''<?xml version="1.0"?>
+<dives><units>Metric</units><schema>2.2.0</schema>
+  <dive>
+    <date>2024-01-01 09:00:00</date><identifier>d1</identifier>
+    <maxDepth>20</maxDepth><duration>1800</duration>
+    <buddies><buddy>Alice</buddy><buddy>Alice</buddy><buddy>Bob</buddy></buddies>
+    <samples/>
+  </dive>
+</dives>''';
+      final bytes = Uint8List.fromList(utf8.encode(xml));
+      final payload = await const MacDiveXmlParser().parse(bytes);
+      final dive = payload.entitiesOf(ImportEntityType.dives).first;
+      expect(dive['buddyRefs'], ['Alice', 'Bob']);
+    });
+
+    test('duplicate <tag> entries on one dive dedupe in tagRefs', () async {
+      // dive_tags has no UNIQUE(diveId, tagId) constraint, so duplicates
+      // would surface as the same tag shown twice on a dive.
+      const xml = '''<?xml version="1.0"?>
+<dives><units>Metric</units><schema>2.2.0</schema>
+  <dive>
+    <date>2024-01-01 09:00:00</date><identifier>d1</identifier>
+    <maxDepth>20</maxDepth><duration>1800</duration>
+    <tags><tag>Reef</tag><tag>Reef</tag><tag>Photography</tag></tags>
+    <samples/>
+  </dive>
+</dives>''';
+      final bytes = Uint8List.fromList(utf8.encode(xml));
+      final payload = await const MacDiveXmlParser().parse(bytes);
+      final dive = payload.entitiesOf(ImportEntityType.dives).first;
+      expect(dive['tagRefs'], ['Reef', 'Photography']);
+    });
+
     test('multiple dives with overlapping buddies dedup', () async {
       const xml = '''<?xml version="1.0"?>
 <dives><units>Metric</units><schema>2.2.0</schema>

test/features/universal_import/data/parsers/macdive_xml_real_sample_test.dart

@@ -1,3 +1,16 @@
+/// MacDive XML real-sample regression suite.
+///
+/// These tests exercise a real MacDive native XML export that is not checked
+/// into the repository. To run them locally, point the [MACDIVE_XML_SAMPLE]
+/// compile-time environment variable at your local sample file:
+///
+///   flutter test \
+///     --dart-define=MACDIVE_XML_SAMPLE=/absolute/path/to/sample.xml \
+///     --run-skipped --tags=real-data \
+///     test/features/universal_import/data/parsers/macdive_xml_real_sample_test.dart
+///
+/// Without the env var (or when the file at that path does not exist), every
+/// test in this suite is cleanly skipped so CI and fresh clones stay green.
 @Tags(['real-data'])
 library;
 
@@ -11,23 +24,41 @@ import 'package:submersion/features/universal_import/data/models/import_enums.da
 import 'package:submersion/features/universal_import/data/models/import_warning.dart';
 import 'package:submersion/features/universal_import/data/parsers/macdive_xml_parser.dart';
 
-const _realSamplePath =
-    '/Users/ericgriffin/Documents/submersion development/submersion data/Macdive/Apr 4 no iPad Mini sync.xml';
+/// Compile-time env var that points at a local MacDive native XML sample.
+///
+/// Injected via `flutter test --dart-define=MACDIVE_XML_SAMPLE=...`.
+const _realSamplePathEnvVar = String.fromEnvironment('MACDIVE_XML_SAMPLE');
+
+String? _realSamplePath() {
+  if (_realSamplePathEnvVar.isEmpty) return null;
+  return _realSamplePathEnvVar;
+}
 
 void main() {
   group('MacDive XML real-sample regression', () {
     late Uint8List bytes;
+    var hasFixture = false;
 
     setUpAll(() async {
-      final file = File(_realSamplePath);
-      if (!file.existsSync()) {
-        markTestSkipped('Real sample not available in this environment');
-        return;
-      }
+      final path = _realSamplePath();
+      if (path == null) return;
+      final file = File(path);
+      if (!file.existsSync()) return;
       bytes = Uint8List.fromList(utf8.encode(await file.readAsString()));
+      hasFixture = true;
     });
 
+    bool skipIfNoFixture() {
+      if (hasFixture) return false;
+      markTestSkipped(
+        'Real sample not available. Set MACDIVE_XML_SAMPLE via '
+        '--dart-define and pass --run-skipped --tags=real-data to run.',
+      );
+      return true;
+    }
+
     test('parses 540 dives without errors', () async {
+      if (skipIfNoFixture()) return;
       final payload = await const MacDiveXmlParser().parse(bytes);
       expect(payload.entitiesOf(ImportEntityType.dives).length, 540);
       // No errors (warnings are ok).
@@ -40,6 +71,7 @@ void main() {
     });
 
     test('every dive has a sourceUuid from <identifier>', () async {
+      if (skipIfNoFixture()) return;
       final payload = await const MacDiveXmlParser().parse(bytes);
       final dives = payload.entitiesOf(ImportEntityType.dives);
       expect(
@@ -50,6 +82,7 @@ void main() {
     });
 
     test('tags imported (MacDive XML has tags, unlike MacDive UDDF)', () async {
+      if (skipIfNoFixture()) return;
       final payload = await const MacDiveXmlParser().parse(bytes);
       final tags = payload.entitiesOf(ImportEntityType.tags);
       expect(
@@ -65,6 +98,7 @@ void main() {
     });
 
     test('sites deduped by name', () async {
+      if (skipIfNoFixture()) return;
       final payload = await const MacDiveXmlParser().parse(bytes);
       final sites = payload.entitiesOf(ImportEntityType.sites);
       expect(sites, isNotEmpty);
@@ -77,6 +111,7 @@ void main() {
     });
 
     test('at least one dive has tagRefs populated', () async {
+      if (skipIfNoFixture()) return;
       final payload = await const MacDiveXmlParser().parse(bytes);
       final dives = payload.entitiesOf(ImportEntityType.dives);
       final withTags = dives.where(
@@ -86,6 +121,7 @@ void main() {
     });
 
     test('at least one dive has tanks + profile populated', () async {
+      if (skipIfNoFixture()) return;
       final payload = await const MacDiveXmlParser().parse(bytes);
       final dives = payload.entitiesOf(ImportEntityType.dives);
       final withTanks = dives.where(
@@ -99,6 +135,7 @@ void main() {
     });
 
     test('imperial sample: max depth values are plausible in meters', () async {
+      if (skipIfNoFixture()) return;
       final payload = await const MacDiveXmlParser().parse(bytes);
       final dives = payload.entitiesOf(ImportEntityType.dives);
       // After conversion, max depths should be mostly 5-80 meters (reasonable

test/features/universal_import/data/services/macdive_xml_reader_test.dart

@@ -172,6 +172,19 @@ void main() {
       expect(dive.boat, isNull);
     });
 
+    test('throws when root element is not <dives>', () {
+      // Guards against a source-override misuse where a user forces "MacDive
+      // XML" on a UDDF or other file: without this check, the reader would
+      // silently return an empty logbook.
+      const uddf =
+          '<?xml version="1.0"?>'
+          '<uddf version="3.2.0"><profiledata/></uddf>';
+      expect(
+        () => MacDiveXmlReader.parse(uddf),
+        throwsA(isA<FormatException>()),
+      );
+    });
+
     test('unknown units system passes through numerics unchanged', () {
       const xml = '''<?xml version="1.0"?>
 <dives>