Skip to content

fix(import): When a imported site has detected a duplicate give the option to over…#325

Open
netdata-be wants to merge 1 commit into
submersion-app:mainfrom
netdata-be:import-overwrite-site
Open

fix(import): When a imported site has detected a duplicate give the option to over…#325
netdata-be wants to merge 1 commit into
submersion-app:mainfrom
netdata-be:import-overwrite-site

Conversation

@netdata-be

Copy link
Copy Markdown

When importing sites and an existing site has been detected give the option to overwrite the current site

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Adds an “overwrite existing site” path to the Universal import duplicate-resolution flow, allowing users to replace an existing dive site with imported site data when a duplicate is detected.

Changes:

  • Adds a new duplicate action (DuplicateAction.replaceSource) to the Universal adapter and review UI.
  • Captures site overwrite selections as siteOverrides (import index → existing site ID) and applies them during UDDF site import by updating the existing site in place.
  • Updates duplicate UI styling/badges to reflect the new overwrite action.

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 4 comments.

File Description
lib/features/import_wizard/presentation/widgets/entity_review_list.dart Adds “Overwrite” UI option (button + badge/border styling) for duplicate entities.
lib/features/import_wizard/data/adapters/universal_adapter.dart Exposes replaceSource as a supported duplicate action and builds siteOverrides for site overwrites.
lib/features/dive_import/data/services/uddf_entity_importer.dart Extends site import to support overwriting existing sites based on siteOverrides.

Comment on lines 140 to 144
Set<DuplicateAction> get supportedDuplicateActions => const {
DuplicateAction.skip,
DuplicateAction.importAsNew,
DuplicateAction.replaceSource,
};
Comment on lines +724 to +731
_EntityActionButton(
label: 'Overwrite',
subtitle: 'Replace existing entry',
isSelected: selectedAction == DuplicateAction.replaceSource,
color: Colors.blue.shade700,
onPressed: () =>
onActionChanged(DuplicateAction.replaceSource),
),
Comment on lines +811 to +829
final overwrittenSite = DiveSite(
id: existingId,
diverId: diverId,
name: name,
description: siteData['description'] as String? ?? '',
location: (lat != null && lon != null) ? GeoPoint(lat, lon) : null,
minDepth: siteData['minDepth'] as double?,
maxDepth: siteData['maxDepth'] as double?,
difficulty: difficulty,
country: country,
region: region,
rating: siteData['rating'] as double?,
notes: siteData['notes'] as String? ?? '',
hazards: siteData['hazards'] as String?,
accessNotes: siteData['accessNotes'] as String?,
mooringNumber: siteData['mooringNumber'] as String?,
parkingInfo: siteData['parkingInfo'] as String?,
altitude: siteData['altitude'] as double?,
);
Comment on lines +774 to +852
// Handle overwrite (replaceSource): update existing sites in place.
for (final entry in overrides.entries) {
final i = entry.key;
final existingId = entry.value;
if (i >= items.length) continue;
final siteData = items[i];
final name = siteData['name'] as String?;
if (name == null || name.isEmpty) continue;

final existing = existingById[existingId];
if (existing == null) continue;

final uddfId = siteData['uddfId'] as String?;
final lat = siteData['latitude'] as double?;
final lon = siteData['longitude'] as double?;

String? country = siteData['country'] as String?;
String? region = siteData['region'] as String?;

if (lat != null && lon != null && (country == null || region == null)) {
try {
final geocodeResult = await LocationService.instance.reverseGeocode(
lat,
lon,
);
country ??= geocodeResult.country;
region ??= geocodeResult.region;
} catch (_) {
// Geocoding is best-effort
}
}

final difficultyStr = siteData['difficulty'] as String?;
final difficulty = difficultyStr != null
? SiteDifficulty.fromString(difficultyStr)
: null;

final overwrittenSite = DiveSite(
id: existingId,
diverId: diverId,
name: name,
description: siteData['description'] as String? ?? '',
location: (lat != null && lon != null) ? GeoPoint(lat, lon) : null,
minDepth: siteData['minDepth'] as double?,
maxDepth: siteData['maxDepth'] as double?,
difficulty: difficulty,
country: country,
region: region,
rating: siteData['rating'] as double?,
notes: siteData['notes'] as String? ?? '',
hazards: siteData['hazards'] as String?,
accessNotes: siteData['accessNotes'] as String?,
mooringNumber: siteData['mooringNumber'] as String?,
parkingInfo: siteData['parkingInfo'] as String?,
altitude: siteData['altitude'] as double?,
);

await repository.updateSite(overwrittenSite);

final waterType = siteData['waterType'] as String?;
final bodyOfWater = siteData['bodyOfWater'] as String?;
if (waterType != null || bodyOfWater != null) {
await repository.applyImportedMetadata(
existingId,
DiveSitesCompanion(
waterType: waterType != null
? Value(waterType)
: const Value.absent(),
bodyOfWater: bodyOfWater != null
? Value(bodyOfWater)
: const Value.absent(),
),
);
}

if (uddfId != null) idMapping[uddfId] = overwrittenSite;
count++;
onProgress?.call(ImportPhase.sites, count, totalWork);
}
@ericgriffin

Copy link
Copy Markdown
Member

A few things worth fixing:

  1. Badge wording is inconsistent across the two card variants. This PR's _SimpleActionBadge (in entity_review_list.dart:884) uses 'OVERWRITE' for replaceSource. But duplicate_action_card.dart:311 already uses 'REPLACE' for the exact same enum value. The expanded button label here says "Overwrite", the bulk-action key is universalImport_bulk_replaceSourceAll, and review_step.dart:373 uses universalImport_label_replaceSource. Pick one user-facing word and use it everywhere. I'd lean toward 'REPLACE' to match the existing dive card.

  2. No tests cover the new override branch. _importSites gains ~50 lines of non-trivial behaviour (existing-by-id lookup, conditional reverse-geocoding, conditional metadata application, progress totals that include both buckets), plus several continue-and-silently-no-op branches (i >= items.length, name == null, existing == null). test/features/dive_import/data/services/uddf_entity_importer_test.dart doesn't touch siteOverrides. Given the silent failure paths, this is the place I'd most want a couple of unit tests before shipping.

  3. Hardcoded English in the action button. 'Overwrite' / 'Replace existing entry' are inline strings at entity_review_list.dart:725-728. This codebase has a strict project rule about translating new strings into all 10 non-en locales, and l10n keys already exist for replaceSource (universalImport_label_replaceSource, universalImport_bulk_replaceSourceAll). The sibling 'Skip' and 'Import as New' buttons in the same widget are also hardcoded, so this PR is consistent with existing debt, but it's perpetuating it rather than fixing it.

  4. The two-write sequence isn't transactional. The new path calls repository.updateSite(...) then optionally repository.applyImportedMetadata(...). If the second call throws, the row is left with the new core fields but stale waterType/bodyOfWater. The create path has the same shape, so this isn't a regression, but expanding the surface area without wrapping the pair in _db.transaction(...) doubles the rows at risk.

@codecov

codecov Bot commented Jun 12, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 21.12676% with 56 lines in your changes missing coverage. Please review.

Files with missing lines Patch % Lines
...ive_import/data/services/uddf_entity_importer.dart 13.04% 40 Missing ⚠️
...izard/presentation/widgets/entity_review_list.dart 12.50% 14 Missing ⚠️
...import_wizard/data/adapters/universal_adapter.dart 77.77% 2 Missing ⚠️

📢 Thoughts on this report? Let us know!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Status: Backlog

Development

Successfully merging this pull request may close these issues.

3 participants