Fix MacDive UDDF import (Milestone 1 of 4)#252
Merged
Conversation
Contributor
There was a problem hiding this comment.
Pull request overview
Milestone 1 for improving MacDive UDDF import support by expanding UDDF parsing (standard gear location, additional dive/site metadata, gas-switch markers, source UUID capture) and persisting new metadata to the database with schema migrations and regression tests.
Changes:
- Extend UDDF parsing to capture MacDive/standard UDDF fields (gear under
<diver><owner><equipment>,<dive id>assourceUuid, richer dive/site metadata, improved equipment refs, gas-switch markers). - Persist new MacDive metadata into new DB columns and persist per-dive
sourceUuidintodive_data_sources.source_uuid; bump schema to v71 with v70/v71 migrations + tests. - Add integration and real-sample regression tests; introduce a
real-datatest tag default-skipped viadart_test.yaml.
Reviewed changes
Copilot reviewed 15 out of 15 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| test/features/dive_import/uddf_import_source_uuid_test.dart | New integration test verifying dive_data_sources.source_uuid persistence for UDDF <dive id>. |
| test/features/dive_import/uddf_import_macdive_fields_test.dart | New integration tests verifying MacDive metadata persistence into new dives/dive_sites columns. |
| test/core/services/export/uddf/uddf_macdive_real_sample_test.dart | New gated “real-data” regression suite against a large MacDive UDDF export. |
| test/core/services/export/uddf/uddf_macdive_import_test.dart | Adds unit tests validating new MacDive/UDDF parsing behaviors (metadata, equipment refs, gear, switchmix markers, infinity surface interval). |
| test/core/database/migration_v71_test.dart | New migration tests ensuring v71 columns exist and upgrade v70→v71 is idempotent. |
| test/core/database/migration_v70_test.dart | New migration tests ensuring source_uuid exists and upgrade v69→v70 works. |
| lib/features/dive_sites/data/repositories/site_repository_impl.dart | Adds repository method to apply importer-only site column updates and mark pending sync. |
| lib/features/dive_log/data/repositories/dive_repository_impl.dart | Adds repository method to apply importer-only dive column updates and mark pending sync. |
| lib/features/dive_import/data/services/uddf_entity_importer.dart | Persists MacDive parsed metadata into new DB columns; persists sourceUuid into dive_data_sources. |
| lib/core/services/export/uddf/uddf_full_import_service.dart | Enhances UDDF parsing for standard gear, site/dive source UUIDs, richer metadata, equipment refs, and switchmix sample markers. |
| lib/core/domain/models/incoming_dive_data.dart | Extends normalized incoming-dive model to carry new MacDive metadata and sourceUuid. |
| lib/core/database/database.dart | Adds new columns + migrations (v70 source_uuid, v71 MacDive metadata columns) and bumps schema version to 71. |
| docs/superpowers/plans/2026-04-21-macdive-uddf-gap-fill.md | Updates milestone plan/status documentation to reflect landed scope and rationale. |
| dart_test.yaml | Adds default-skipped real-data tag configuration. |
| CHANGELOG.md | Adds Unreleased changelog entries for MacDive UDDF improvements and dedup behavior. |
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
This was referenced Apr 21, 2026
ericgriffin
added a commit
that referenced
this pull request
Apr 22, 2026
Drift's Value(null) sets the column to NULL - it does NOT mean 'leave this field untouched'. Re-importing a MacDive UDDF whose new data was missing fields the existing row already had would silently wipe those fields. All nullable-field writes to DivesCompanion and DiveSitesCompanion now use Value.absent() when the source value is null, preserving existing data on partial re-imports. Addresses Copilot review comments 1 and 2 on PR #252.
ericgriffin
added a commit
that referenced
this pull request
Apr 22, 2026
- Path is now read from MACDIVE_UDDF_SAMPLE (compile-time env var set via --dart-define). No more hard-coded developer username. - Each test checks hasFixture and calls markTestSkipped/return if the sample isn't available. --run-skipped --tags=real-data now cleanly skips rather than crashing on uninitialized state. - Added top-of-file comment documenting the env-var workflow. Addresses Copilot review comments 3 and 4 on PR #252.
Matches existing per-source architecture (raw_fingerprint, source_format, source_file_name, computer_serial already live on dive_data_sources). One column, one table, one migration. Non-dive entities stay on content-based dedup for now.
- Rename to migration_v70_test.dart matching v58/v66/v68/v69 pattern - Use addTearDown(db.close) for leak-safe cleanup - Add v69 -> v70 migration-path test seeding pre-migration schema
Pre-cursor to Task 3 which will replace positional <link ref> resolution in informationbeforedive with kind-based binding.
Investigation showed UddfFullImportService already uses map-based link-ref disambiguation, not positional order. Prescribed test passes on unmodified code. Documented the finding and the real (unrelated) gap for trip/center/course UUIDs which will surface in Milestone 3 if at all.
Verifies that MacDive's <infinity/> first-dive marker does not produce a spurious surfaceInterval=0. The current parser handles this correctly via int.tryParse; this test is a regression guard.
…ator, ...) and source UUID Adds 10 new optional fields that MacDive emits but the parser previously ignored: weather, surfaceConditions, boatName, boatCaptain, diveOperator, personalMode, altitudeMode, signature, diveNumberOfDay, and the dive element's id attribute as sourceUuid. Threaded through IncomingDiveData.
…and source UUID MacDive emits these site-level fields but the current parser ignored them. Extract into the site data map alongside name, country, and coordinates.
…are captured Previously only scanned informationbeforedive for <equipmentused> refs. Now merges refs from both informationbeforedive and informationafterdive into equipmentRefs, supporting both <equipmentref> elements and <link ref="..."/> attributes.
MacDive emits <switchmix ref="gas-UUID"/> inside waypoints to mark gas changes during a dive; the parser now captures this ref as gasMixRef on the sample map so downstream consumers (deco display, gas stats) can see the change event. Regression guard test added to prevent future breaks of this feature per the ScubaBoard 'gas switches for deco not imported' complaint.
Closes the pipe from parser to DB for the source UUID plumbing added in Tasks 1 and 5. MacDive, Shearwater Cloud, and any other source with a stable per-dive ID can now round-trip through import and be matched on re-import. Other dive-level MacDive fields (weather, boat, operator, …) remain in the dive map but have no persistence target yet; deferred to a later milestone that adds display for them.
… source_uuid Previous test re-exercised Task 5's UddfFullImportService parser but never hit UddfEntityImporter, leaving the one-line DB write at line 1425 untested. This test now spins up an in-memory AppDatabase via setUpTestDatabase, runs UddfEntityImporter.import with real repositories, and queries dive_data_sources.source_uuid directly to verify both the populated and null-attribute cases.
Runs the user's 29MB MacDive UDDF through the parser and asserts dive/site/buddy/gear counts plus spot-checks for sourceUuid, equipmentRefs, gasMixRef, and the country-nesting dialect fix. Gated behind @tags(['real-data']) so CI stays fast.
…location MacDive and other UDDF-compliant exporters emit gear under <diver><owner><equipment> with child elements like <variouspieces>, <suit>, <divecomputer>, <regulator>, <bcd>. The parser previously only scanned Submersion's private <applicationdata><submersion> extension, so real-world UDDF imports produced zero gear - the root cause of the 'my gear didn't import' complaint. Now scans both locations, deduping by source UUID. Real-sample test restored to assert >=20 items.
Removes parser extraction and IncomingDiveData fields for MacDive fields we've decided not to persist: personalMode, altitudeMode, signature, site flag. These are niche MacDive-specific concepts or redundant with existing columns. Also removes LinkRefKind and LinkRefIndex — added for a bug that turned out not to exist (Task 3 investigation) and never wired up.
Adds 7 new columns on dives and dive_sites for previously extract-but-discard fields: - dives: dive_number_of_day, boat_name, boat_captain, dive_operator, surface_conditions - dive_sites: water_type, body_of_water Also wires the existing weather_description column for UDDF imports (populated from MacDive <weather>). The importer calls narrow new applyImportedMetadata helpers on DiveRepository and SiteRepository so the new columns ride along with the existing repository-created rows without needing entity field churn (copyWith, equatable, sync serialiser). Schema v70 -> v71 with an idempotent migration guarded by PRAGMA introspection, matching the v69/v70 house style. Closes the parser-to-DB gap for the MacDive extended fields extracted in Milestone 1 Tasks 5-6.
Drift's Value(null) sets the column to NULL - it does NOT mean 'leave this field untouched'. Re-importing a MacDive UDDF whose new data was missing fields the existing row already had would silently wipe those fields. All nullable-field writes to DivesCompanion and DiveSitesCompanion now use Value.absent() when the source value is null, preserving existing data on partial re-imports. Addresses Copilot review comments 1 and 2 on PR #252.
- Path is now read from MACDIVE_UDDF_SAMPLE (compile-time env var set via --dart-define). No more hard-coded developer username. - Each test checks hasFixture and calls markTestSkipped/return if the sample isn't available. --run-skipped --tags=real-data now cleanly skips rather than crashing on uninitialized state. - Added top-of-file comment documenting the env-var workflow. Addresses Copilot review comments 3 and 4 on PR #252.
Earlier commits on this branch added DiveRepository.applyImportedMetadata and SiteRepository.applyImportedMetadata without regenerating the dependent mockito mocks. This picks up that drift across 9 .mocks.dart files.
dive_number_of_day had zero readers -- parsed from <divenumberofday>, stored, never queried. It's trivially derivable from dive_date_time via ROW_NUMBER() OVER (PARTITION BY DATE(dive_date_time) ORDER BY dive_date_time), which self-heals when dives are added manually between imported ones. Removes the column from the v71 migration (PR not merged yet, safe to amend), the schema definition, IncomingDiveData, the UDDF parser, and the entity importer. Updates CHANGELOG and the three tests that referenced it.
Copilot review flagged drift between the plan's status section ("7 new
columns including dive_number_of_day") and the actual v71 schema (6
columns, no dive_number_of_day). Status section now lists only the
columns that shipped; a new "Post-completion revision" subsection
documents why dive_number_of_day was dropped (trivially derivable from
diveDateTime, no reader, desyncs on manually-interleaved dives). Deeper
task sections are left as historical record of the original plan.
f8994f0 to
90b3c44
Compare
ericgriffin
added a commit
that referenced
this pull request
Apr 23, 2026
Drift's Value(null) sets the column to NULL - it does NOT mean 'leave this field untouched'. Re-importing a MacDive UDDF whose new data was missing fields the existing row already had would silently wipe those fields. All nullable-field writes to DivesCompanion and DiveSitesCompanion now use Value.absent() when the source value is null, preserving existing data on partial re-imports. Addresses Copilot review comments 1 and 2 on PR #252.
ericgriffin
added a commit
that referenced
this pull request
Apr 23, 2026
- Path is now read from MACDIVE_UDDF_SAMPLE (compile-time env var set via --dart-define). No more hard-coded developer username. - Each test checks hasFixture and calls markTestSkipped/return if the sample isn't available. --run-skipped --tags=real-data now cleanly skips rather than crashing on uninitialized state. - Added top-of-file comment documenting the env-var workflow. Addresses Copilot review comments 3 and 4 on PR #252.
ericgriffin
added a commit
that referenced
this pull request
Apr 23, 2026
PR #252 (UDDF gap-fill M1) landed on main and was followed by two refactors that this branch needs to absorb: - `dive_number_of_day` column removed (derivable from `diveDateTime`). Dropped from `database.dart`, `IncomingDiveData`, `uddf_entity_importer` write path, `uddf_full_import_service` parser, and the MacDive XML parser's tank-map output. Removed assertions from schema + integration tests. - `Value.absent()` for missing companion fields in `uddf_entity_importer` applyImportedMetadata paths (site + dive). Also swapped the hard-coded real-sample path in `uddf_macdive_real_sample_test.dart` for main's env-var / skip-safe version so the suite runs cleanly on CI and fresh clones. Tests: 1539 passing, 5 skipped (gated real-data suites).
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
First of four planned milestones addressing MacDive import issues (closes #178 in part, foundational for #179).
Fixed: gear doesn't import from MacDive UDDF. Parser was only scanning Submersion's private
<applicationdata><submersion><equipment>extension, missing the standard UDDF gear location at<diver><owner><equipment>where MacDive and every other compliant exporter puts it. Now extracts from both locations (dedup'd by source UUID). Real-sample test confirms 29/29 equipment items land.Fixed: gas switches for deco dives dropped at import.
<switchmix ref>markers on waypoints updated internal state but never populated the sample map. Fixed at the parser; persistence todive_profile_samplesdeferred to M1.5 (needs an events-table design decision).Fixed:
<equipmentused><link ref>only captured from one section. Now captured from bothinformationbeforediveandinformationafterdive, with dedup.Fixed:
<surfaceintervalbeforedive><infinity/></…>(MacDive first-dive marker) is now explicitly handled rather than relying on silentint.tryParsefailure.Added: richer dive metadata extraction + persistence. Schema bumped 69 → 70 → 71. New columns on
dives(boat_name, boat_captain, dive_operator, surface_conditions) anddive_sites(water_type, body_of_water). Weather flows into the existingweather_descriptioncolumn; site difficulty into the existingdifficultycolumn.dive_number_of_daywas originally planned as a stored column but dropped in favor of deriving it on demand fromdiveDateTime(no current reader and self-healing as manually-logged dives are interleaved with imports).Added: cross-format import deduplication via
source_uuidon the existingdive_data_sourcessidecar. Preserves stable per-dive IDs from MacDive, Shearwater Cloud, and Subsurface SSRF. Re-importing the same dives in a different format no longer creates duplicates.Added: gated real-sample regression test (
@Tags(['real-data'])) against the 29MB MacDive UDDF export, asserting 540 dives, 350+ sites, 30+ buddies, 20+ gear, and spot-checks forsourceUuid,equipmentRefs,gasMixRef, and country-nesting.Dropped from scope (parse-side extraction removed)
personalMode,altitudeMode,signature, siteflag— niche MacDive-specific concepts or redundant with existing columns (we already have numericaltitude).LinkRefKind/LinkRefIndexutility types — added in support of a bug that turned out not to exist in the codebase (the parser already uses map-based ref disambiguation, not positional).Deferred to future milestones
gasMixRefpersistence (parsed in memory; needs events-table design).Commit narrative
17 commits tell the story top-to-bottom: schema foundation → parser utilities → per-field extractions → bug fixes → integration tests → cleanup. Each commit is individually reviewable.
Notable: Task 3 was investigated and explicitly skipped after the implementer discovered the diagnosed bug didn't exist in the code. Task 7 was descoped based on user guidance (only preserve stable IDs for sources that actually emit them — which led to the narrow
dive_data_sources.source_uuiddesign instead of 9 new columns).Test plan
flutter test— 7112 tests passflutter analyze— cleandart format— cleantest/core/database/migration_v70_test.dart)test/core/database/migration_v71_test.dart)source_uuidand MacDive extended fieldsUser-reported issues closed
.xmlformat not recognized → deferred to M2