fix(#575): apply pre-PR review, stop claiming unconfirmed import success

Adversarial review raised three findings. Two survived verification.

Rejected: a claimed RangeError when abbreviating a short public key. The
code already guards with a length check before either substring, so the
crash it describes cannot occur.

Rejected as described, fixed as found: contact position (0, 0) was said to
be dropped on import. It is not. The frame builder writes the position block
whenever lastModified is present, which it always is here, so a suppressed
position still writes 0/0 and the bytes are identical either way. The
hasPosition conditional was therefore doing nothing except misleading a
reader, which is exactly what happened. Removed, and the behavior is pinned
with a test.

Accepted: the service added sections to 'applied' after sendFrame returned,
which only means the frame left our side. A lost or refused frame was
indistinguishable from success, so the user could be told an import worked
when nothing changed on the device.

A correct fix needs an awaited per-command acknowledgement in the connector,
keyed on command code so concurrent commands cannot steal each other's OK.
That is real connector work and is filed as #584. What is fixed here is the
overclaim: 'applied' and the two counts are documented as sent rather than
confirmed, and the result string now reads 'Sent N contacts and M channels
to the device'. The code no longer states something it cannot know.
pull/701/head
Strycher 1 month ago committed by Benjamin Wiechel
parent 56649e437c
commit 9741c3399d

@ -3002,9 +3002,9 @@
"@importConfig_resultTitle": {
"description": "Title of the dialog summarising what an import did (#576)."
},
"importConfig_resultCounts": "{contacts} contacts written, {channels} channels added.",
"importConfig_resultCounts": "Sent {contacts} contacts and {channels} channels to the device.",
"@importConfig_resultCounts": {
"description": "Summary counts after an import (#576).",
"description": "Summary counts after an import. Deliberately says sent, not applied: apart from identity these commands are not confirmed by the device yet (#576).",
"placeholders": {
"contacts": { "type": "int" },
"channels": { "type": "int" }

@ -8710,10 +8710,10 @@ abstract class AppLocalizations {
/// **'Import finished'**
String get importConfig_resultTitle;
/// Summary counts after an import (#576).
/// Summary counts after an import. Deliberately says sent, not applied: apart from identity these commands are not confirmed by the device yet (#576).
///
/// In en, this message translates to:
/// **'{contacts} contacts written, {channels} channels added.'**
/// **'Sent {contacts} contacts and {channels} channels to the device.'**
String importConfig_resultCounts(int contacts, int channels);
/// Heading above the list of channels that were skipped (#576).

@ -5127,7 +5127,7 @@ class AppLocalizationsBg extends AppLocalizations {
@override
String importConfig_resultCounts(int contacts, int channels) {
return '$contacts contacts written, $channels channels added.';
return 'Sent $contacts contacts and $channels channels to the device.';
}
@override

@ -5144,7 +5144,7 @@ class AppLocalizationsDe extends AppLocalizations {
@override
String importConfig_resultCounts(int contacts, int channels) {
return '$contacts contacts written, $channels channels added.';
return 'Sent $contacts contacts and $channels channels to the device.';
}
@override

@ -5050,7 +5050,7 @@ class AppLocalizationsEn extends AppLocalizations {
@override
String importConfig_resultCounts(int contacts, int channels) {
return '$contacts contacts written, $channels channels added.';
return 'Sent $contacts contacts and $channels channels to the device.';
}
@override

@ -5132,7 +5132,7 @@ class AppLocalizationsEs extends AppLocalizations {
@override
String importConfig_resultCounts(int contacts, int channels) {
return '$contacts contacts written, $channels channels added.';
return 'Sent $contacts contacts and $channels channels to the device.';
}
@override

@ -5160,7 +5160,7 @@ class AppLocalizationsFr extends AppLocalizations {
@override
String importConfig_resultCounts(int contacts, int channels) {
return '$contacts contacts written, $channels channels added.';
return 'Sent $contacts contacts and $channels channels to the device.';
}
@override

@ -5149,7 +5149,7 @@ class AppLocalizationsHu extends AppLocalizations {
@override
String importConfig_resultCounts(int contacts, int channels) {
return '$contacts contacts written, $channels channels added.';
return 'Sent $contacts contacts and $channels channels to the device.';
}
@override

@ -5137,7 +5137,7 @@ class AppLocalizationsIt extends AppLocalizations {
@override
String importConfig_resultCounts(int contacts, int channels) {
return '$contacts contacts written, $channels channels added.';
return 'Sent $contacts contacts and $channels channels to the device.';
}
@override

@ -4903,7 +4903,7 @@ class AppLocalizationsJa extends AppLocalizations {
@override
String importConfig_resultCounts(int contacts, int channels) {
return '$contacts contacts written, $channels channels added.';
return 'Sent $contacts contacts and $channels channels to the device.';
}
@override

@ -4904,7 +4904,7 @@ class AppLocalizationsKo extends AppLocalizations {
@override
String importConfig_resultCounts(int contacts, int channels) {
return '$contacts contacts written, $channels channels added.';
return 'Sent $contacts contacts and $channels channels to the device.';
}
@override

@ -5112,7 +5112,7 @@ class AppLocalizationsNl extends AppLocalizations {
@override
String importConfig_resultCounts(int contacts, int channels) {
return '$contacts contacts written, $channels channels added.';
return 'Sent $contacts contacts and $channels channels to the device.';
}
@override

@ -5149,7 +5149,7 @@ class AppLocalizationsPl extends AppLocalizations {
@override
String importConfig_resultCounts(int contacts, int channels) {
return '$contacts contacts written, $channels channels added.';
return 'Sent $contacts contacts and $channels channels to the device.';
}
@override

@ -5125,7 +5125,7 @@ class AppLocalizationsPt extends AppLocalizations {
@override
String importConfig_resultCounts(int contacts, int channels) {
return '$contacts contacts written, $channels channels added.';
return 'Sent $contacts contacts and $channels channels to the device.';
}
@override

@ -5143,7 +5143,7 @@ class AppLocalizationsRu extends AppLocalizations {
@override
String importConfig_resultCounts(int contacts, int channels) {
return '$contacts contacts written, $channels channels added.';
return 'Sent $contacts contacts and $channels channels to the device.';
}
@override

@ -5109,7 +5109,7 @@ class AppLocalizationsSk extends AppLocalizations {
@override
String importConfig_resultCounts(int contacts, int channels) {
return '$contacts contacts written, $channels channels added.';
return 'Sent $contacts contacts and $channels channels to the device.';
}
@override

@ -5107,7 +5107,7 @@ class AppLocalizationsSl extends AppLocalizations {
@override
String importConfig_resultCounts(int contacts, int channels) {
return '$contacts contacts written, $channels channels added.';
return 'Sent $contacts contacts and $channels channels to the device.';
}
@override

@ -5082,7 +5082,7 @@ class AppLocalizationsSv extends AppLocalizations {
@override
String importConfig_resultCounts(int contacts, int channels) {
return '$contacts contacts written, $channels channels added.';
return 'Sent $contacts contacts and $channels channels to the device.';
}
@override

@ -5144,7 +5144,7 @@ class AppLocalizationsUk extends AppLocalizations {
@override
String importConfig_resultCounts(int contacts, int channels) {
return '$contacts contacts written, $channels channels added.';
return 'Sent $contacts contacts and $channels channels to the device.';
}
@override

@ -4777,7 +4777,7 @@ class AppLocalizationsZh extends AppLocalizations {
@override
String importConfig_resultCounts(int contacts, int channels) {
return '$contacts contacts written, $channels channels added.';
return 'Sent $contacts contacts and $channels channels to the device.';
}
@override

@ -68,14 +68,27 @@ class StockConfigImportResult {
required this.channelsAdded,
});
/// Sections whose commands were **sent** to the device without error.
///
/// This is not device confirmation. Apart from the identity import, which
/// waits for a real OK or ERR, the firmware's generic OK for these commands
/// is not currently awaited, so a frame lost or refused after it left our
/// side would still land here. Wiring per-command confirmation is tracked
/// separately; until then this set means "sent", and the UI says so rather
/// than claiming more than we know.
final Set<StockConfigSection> applied;
final Map<StockConfigSection, StockConfigImportIssue> failed;
/// Channels in the file that were not written, each with a reason. Shown to
/// the user; never discarded.
final List<SkippedChannel> skippedChannels;
/// Contacts whose write command was sent. See [applied] for why this is a
/// send count and not a confirmed count.
final int contactsWritten;
/// Channels whose write command was sent. Same caveat as [contactsWritten].
final int channelsAdded;
bool get isComplete => failed.isEmpty && skippedChannels.isEmpty;
@ -280,7 +293,12 @@ class StockConfigImportService {
var written = 0;
for (final contact in contacts) {
final path = contact.outPath;
final hasPosition = contact.latitude != 0 || contact.longitude != 0;
// Position is always passed through, including (0, 0). Suppressing zero
// would be pointless here: the frame builder emits the position block
// whenever lastModified is present, which it always is below, so a
// suppressed position writes 0/0 anyway. Passing it straight through
// means the file's value is what lands, with no special case to
// misread.
await _connector.sendFrame(
buildUpdateContactPathFrame(
contact.publicKey,
@ -291,8 +309,8 @@ class StockConfigImportService {
type: contact.type,
flags: contact.flags,
name: contact.name,
lat: hasPosition ? contact.latitude : null,
lon: hasPosition ? contact.longitude : null,
lat: contact.latitude,
lon: contact.longitude,
lastModified: DateTime.fromMillisecondsSinceEpoch(
contact.lastModified * 1000,
),

@ -155,6 +155,19 @@ void main() {
expect(stock.outPath, isNull);
});
test('a zero position stays zero rather than becoming absent', () {
// Review flagged (0, 0) as a lost position on import. It is not: the
// frame builder writes the position block whenever lastModified is
// present, so zero lands as zero. Pinned here so the export side of the
// pair cannot start emitting something else.
final stock = toStockContact(makeContact(latitude: 0, longitude: 0));
expect(stock.latitude, 0);
expect(stock.longitude, 0);
expect(stock.toJson()['latitude'], '0.0');
expect(stock.toJson()['longitude'], '0.0');
});
test('flags and type pass through untouched', () {
final stock = toStockContact(makeContact(type: 3, flags: 15));

Loading…
Cancel
Save

Powered by TurnKey Linux.