[E-Document] Data Exchange v2 import handler (bridge pattern)

[Groenbech96]

Summary

• Add IStructuredFormatReader implementation for Data Exchange format (codeunit 6407)
• Bridge existing Data Exchange Definition infrastructure to v2 import pipeline staging tables
• Register as enum value 5 "Data Exchange" on E-Doc. Read into Draft
• Namespace-based definition matching (replaces v1 trial-and-error auto-detection)
• Run Data Handling Codeunit only (skip Pre-Mapping codeunit 6156 — vendor/GL resolution deferred to Prepare Draft)
• Map Intermediate Data Import records to E-Document Purchase Header / E-Document Purchase Line via field-ID bridge
• Process base64 attachments from Document Attachment intermediate records
• XPath supplement for Company Information fields not in intermediate data
• Integration events for partner extensibility

New codeunit

ID	Name	Purpose
6407	E-Document Data Exch. Handler	IStructuredFormatReader for Data Exchange format

Key design decisions

• No Commit() — ReadIntoDraft runs inside pipeline try-function context
• No EDocument.Modify() — record passed by value; local variables used
• Namespace matching — matches XML root namespace against DataExchLineDef.Namespace instead of v1's trial-and-error (which requires Commit())
• Data Handling only — runs codeunit 1214 to populate Intermediate Data Import, skips Pre-Mapping (6156) since v2 Prepare Draft handles vendor/item resolution

Test plan

• Invoice ReadIntoDraft: header fields mapped (vendor name, invoice no, dates, amounts)
• Invoice ReadIntoDraft: line fields mapped (description, quantity, unit price, line numbers)
• Invoice returns "Purchase Invoice" process draft type
• CreditNote returns "Purchase Credit Memo" process draft type
• Total VAT computed correctly
• Attachments decoded from base64 and stored
• Currency code blank when matching LCY

AB#630822
AB#599123

[github-actions]

AL Documentation Audit

Documentation gaps were detected in the following apps:

• E-Document-Core-Tests: 0% documentation coverage
• E-Document-Core: 45% documentation coverage

To generate documentation, run /al-docs init or /al-docs update using GitHub Copilot CLI or Claude Code.
This review is for awareness to help keep documentation in sync with code changes. It is okay to dismiss this request.

[github-actions]

AL Documentation Audit

Documentation gaps were detected in the following apps:

• E-Document-Core-Tests: 0% documentation coverage
• E-Document-Core: 59% documentation coverage

To generate documentation, run /al-docs init or /al-docs update using GitHub Copilot CLI or Claude Code.
This review is for awareness to help keep documentation in sync with code changes. It is okay to dismiss this request.

[github-actions]

AL Documentation Audit

Documentation gaps were detected in the following apps:

• E-Document-Core-Tests: 0% documentation coverage

To generate documentation, run /al-docs init or /al-docs update using GitHub Copilot CLI or Claude Code.
This review is for awareness to help keep documentation in sync with code changes. It is okay to dismiss this request.

[github-actions]

AL Documentation Audit

Documentation gaps were detected in the following apps:

• E-Document-Core-Tests: 0% documentation coverage

To generate documentation, run /al-docs init or /al-docs update using GitHub Copilot CLI or Claude Code.
This review is for awareness to help keep documentation in sync with code changes. It is okay to dismiss this request.

[github-actions]

$\textbf{🟡\ Medium\ Severity\ —\ Performance} \quad \color{gray}{\texttt{\small Iteration\ 1}}$

Redundant RecordRef SetTable/GetTable calls per field

Inside the inner loop of MapIntermediateToLines, lines 223–224 call RecordRef.SetTable(EDocumentPurchaseLine) followed immediately by RecordRef.GetTable(EDocumentPurchaseLine) for every single intermediate field. This double round-trip per field is unnecessary and adds overhead for documents with many lines and fields.

Recommendation:

• Move the RecordRef.SetTable call to only occur once per record group boundary (after the loop or at the start of a new record), rather than per-field. The intermediate GetTable after each field assignment can be replaced by working entirely with FieldRef until the record needs to be committed.

Suggested change

	RecordRef.SetTable(EDocumentPurchaseLine);
	until IntermediateDataImport.Next() = 0;
	// Insert last line - only set table once at the end
	RecordRef.SetTable(EDocumentPurchaseLine);
	PostProcessLine(EDocumentPurchaseLine);
	EDocumentPurchaseLine.Insert();

_{👍 useful · ❤️ especially valuable · 👎 wrong - reply with why}

[github-actions]

$\textbf{🟡\ Medium\ Severity\ —\ Performance} \quad \color{gray}{\texttt{\small Iteration\ 1}}$

AttachmentTempBlob not cleared between attachment records

AttachmentTempBlob is a single local codeunit instance reused across all attachment records in the loop. When CreateOutStream is called for a second attachment, the first attachment's data may not be fully flushed before the blob is reused, risking data leakage between attachment records.

Recommendation:

• Call Clear(AttachmentTempBlob) at the start of each new attachment record iteration, before calling CreateOutStream, to ensure each attachment starts with a clean blob.

Suggested change

	AttachmentTempBlob: Codeunit "Temp Blob";
	if CurrRecordNo <> IntermediateDataImport."Record No." then begin
	if CurrRecordNo <> -1 then
	if FileName <> '' then begin
	AttachmentTempBlob.CreateInStream(InStream);
	EDocAttachmentProcessor.Insert(EDocument, InStream, FileName);
	FileName := '';
	Clear(AttachmentTempBlob);
	end;
	CurrRecordNo := IntermediateDataImport."Record No.";
	end;

_{👍 useful · ❤️ especially valuable · 👎 wrong - reply with why}

[github-actions]

$\textbf{🟠\ High\ Severity\ —\ Security} \quad \color{gray}{\texttt{\small Iteration\ 1}}$

Invoice lookup missing vendor filter in ResolveAppliesToFromExtInvoiceNo

PurchInvHeader.SetRange("Vendor Invoice No.", ExtInvoiceNo) searches all posted purchase invoices without filtering by vendor. Because vendor invoice numbers are only unique per vendor, this can match an invoice from a different vendor, causing the credit memo to incorrectly apply against another vendor's invoice.

Recommendation:

• Add a filter on Buy-from Vendor No. (taken from the PurchaseHeader) before calling FindFirst() to restrict the lookup to the correct vendor.

Suggested change

	PurchInvHeader.SetRange("Vendor Invoice No.", ExtInvoiceNo);
	local procedure ResolveAppliesToFromExtInvoiceNo(ExtInvoiceNo: Text[100]; var PurchaseHeader: Record "Purchase Header")
	var
	PurchInvHeader: Record "Purch. Inv. Header";
	begin
	PurchInvHeader.SetRange("Buy-from Vendor No.", PurchaseHeader."Buy-from Vendor No.");
	PurchInvHeader.SetRange("Vendor Invoice No.", ExtInvoiceNo);
	if PurchInvHeader.FindFirst() then begin
	PurchaseHeader."Applies-to Doc. Type" := PurchaseHeader."Applies-to Doc. Type"::Invoice;
	PurchaseHeader."Applies-to Doc. No." := PurchInvHeader."No.";
	end;
	end;

_{👍 useful · ❤️ especially valuable · 👎 wrong - reply with why}

[github-actions]

$\textbf{🟡\ Medium\ Severity\ —\ Style} \quad \color{gray}{\texttt{\small Iteration\ 1}}$

Hard-coded string literal instead of Label in View

The View procedure calls Error('A view is not implemented for this handler.') using a raw string literal. This string is not localizable and violates the AL convention of using Label for all user-visible text.

Recommendation:

• Declare a Label variable for the error message and reference it in the Error call.

Suggested change

	Error('A view is not implemented for this handler.');
	var
	ViewNotImplementedErr: Label 'A view is not implemented for this handler.';
	procedure View(EDocument: Record "E-Document"; TempBlob: Codeunit "Temp Blob")
	begin
	Error(ViewNotImplementedErr);
	end;

_{👍 useful · ❤️ especially valuable · 👎 wrong - reply with why}

[github-actions]

$\textbf{🟡\ Medium\ Severity\ —\ Upgrade} \quad \color{gray}{\texttt{\small Iteration\ 1}}$

Table field rename breaks existing extensions and data

Field 40 is renamed from "Vendor Invoice No." to "Applies-to Ext. Invoice No.". Renaming a published table field is a breaking change: any existing extension or report that references "Vendor Invoice No." on this table will fail to compile, and any data exchange definitions targeting the old field name will stop working.

Recommendation:

• For published tables, prefer adding a new field rather than renaming. If rename is required, verify all callers across all dependent apps/extensions are updated and consider an upgrade codeunit to migrate existing data.

Suggested change

	field(40; "Vendor Invoice No."; Text[100])
	// Option: Keep old field as obsolete and add new field
	field(40; "Vendor Invoice No."; Text[100])
	{
	Caption = 'Vendor Invoice No.';
	DataClassification = CustomerContent;
	ObsoleteReason = 'Use Applies-to Ext. Invoice No. instead.';
	ObsoleteState = Pending;
	ObsoleteTag = '26.0';
	}
	field(41; "Applies-to Ext. Invoice No."; Text[100])
	{
	Caption = 'Applies-to Ext. Invoice No.';
	DataClassification = CustomerContent;
	}

_{👍 useful · ❤️ especially valuable · 👎 wrong - reply with why}

[github-actions]

$\textbf{🟠\ High\ Severity\ —\ Performance} \quad \color{gray}{\texttt{\small Iteration\ 1}}$

DataExch record leaks on pipeline error

In RunPipelineAndBridge, a DataExch record is inserted and modified but there is no error handling around the subsequent ImportToDataExch, ProcessDataExchange, BridgeToStagingTables, or DeleteIntermediateData calls. If any of those steps throws an error, the DataExch record and its intermediate data remain permanently in the database (orphaned rows in "Data Exch." and "Intermediate Data Import").

Recommendation:

• Wrap the pipeline in a try-function or add a commit-free cleanup on failure. At minimum, call DataExch.Delete(true) in an error handler so intermediate data is rolled back or cleaned up.

Suggested change

	DeleteIntermediateData(DataExch);
	local procedure RunPipelineAndBridge(EDocument: Record "E-Document"; var TempBlob: Codeunit "Temp Blob"; DataExchDefCode: Code[20])
	begin
	// ... setup DataExch record ...
	if not TryRunPipeline(EDocument, DataExch, DataExchDef) then begin
	DataExch.Delete(true);
	Error(GetLastErrorText());
	end;
	end;
	[TryFunction]
	local procedure TryRunPipeline(...)
	begin
	DataExch.ImportToDataExch(DataExchDef);
	DataExchDef.ProcessDataExchange(DataExch);
	BridgeToStagingTables(EDocument, DataExch);
	DeleteIntermediateData(DataExch);
	end;

_{👍 useful · ❤️ especially valuable · 👎 wrong - reply with why}

[github-actions]

$\textbf{🟡\ Medium\ Severity\ —\ Performance} \quad \color{gray}{\texttt{\small Iteration\ 1}}$

FindFirst without index on Vendor Invoice No.

ResolveAppliesToFromExtInvoiceNo calls PurchInvHeader.SetRange("Vendor Invoice No.", ExtInvoiceNo) then FindFirst() without calling SetCurrentKey. If "Vendor Invoice No." is not the primary key or part of a sorted index, this triggers a full-table scan on Purch. Inv. Header, which can be very slow for companies with large invoice history.

Recommendation:

• Add PurchInvHeader.SetCurrentKey("Vendor Invoice No.") before the SetRange, and consider also filtering by "Buy-from Vendor No." to avoid applying the wrong vendor's invoice.

local procedure ResolveAppliesToFromExtInvoiceNo(ExtInvoiceNo: Text[100]; var PurchaseHeader: Record "Purchase Header")
var
    PurchInvHeader: Record "Purch. Inv. Header";
begin
    PurchInvHeader.SetCurrentKey("Vendor Invoice No.");
    PurchInvHeader.SetRange("Vendor Invoice No.", ExtInvoiceNo);
    PurchInvHeader.SetRange("Buy-from Vendor No.", PurchaseHeader."Buy-from Vendor No.");
    if PurchInvHeader.FindFirst() then begin
        PurchaseHeader."Applies-to Doc. Type" := PurchaseHeader."Applies-to Doc. Type"::Invoice;
        PurchaseHeader."Applies-to Doc. No." := PurchInvHeader."No.";
    end;
end;

Line mapping was unavailable, so this was posted as an issue comment.

_{👍 useful · ❤️ especially valuable · 👎 wrong - reply with why}

[github-actions]

$\textbf{🟡\ Medium\ Severity\ —\ Performance} \quad \color{gray}{\texttt{\small Iteration\ 1}}$

GetDocumentRootNamespace creates new InStream on each call

GetDocumentRootNamespace calls TempBlob.CreateInStream(Stream) and reads the entire XML document to extract the root namespace. FindBestDataExchDef then calls this, and afterwards RunPipelineAndBridge calls TempBlob.CreateInStream(Stream) again — reading the blob a second time. TempBlob InStreams are position-based; creating a new InStream rewinds to the start, so this works correctly, but it means the XML is read/parsed twice.

Recommendation:

• Cache the namespace result or pass it through to avoid double-parsing. This is especially relevant for large PEPPOL documents with many attachments.

Suggested change

	TempBlob.CreateInStream(Stream);
	// Pass namespace through or cache:
	local procedure ReadIntoDraft(EDocument: Record "E-Document"; TempBlob: Codeunit "Temp Blob"): Enum "E-Doc. Process Draft"
	var
	BestDefCode: Code[20];
	BestDocType: Enum "E-Document Type";
	begin
	FindBestDataExchDef(TempBlob, BestDefCode, BestDocType); // reads namespace
	RunPipelineAndBridge(EDocument, TempBlob, BestDefCode); // TempBlob rewound automatically
	exit(MapDocumentTypeToProcessDraft(BestDocType));
	end; // acceptable as-is if TempBlob rewind is cheap; add comment to clarify

_{👍 useful · ❤️ especially valuable · 👎 wrong - reply with why}

[github-actions]

$\textbf{🟡\ Medium\ Severity\ —\ Performance} \quad \color{gray}{\texttt{\small Iteration\ 1}}$

BuildEndpointIdentifiers does redundant SetRange reuse

BuildEndpointIdentifiers calls DataExchColumnDef.SetRange(Name, 'CustomerEndpointSchemeID') without first clearing the previous SetRange(Name, 'CustomerEndpointID') filter. The field Name is reset via SetRange so this works correctly for the column lookup, but then DataExchField.SetRange re-uses the same variable after changing "Column No." filter without resetting the previous range — meaning if the first FindFirst() for EndpointValue did not find anything, DataExchField still has stale filter state.

Recommendation:

• Call DataExchField.Reset() or explicitly DataExchField.SetRange("Data Exch. No.", DataExch."Entry No.") before the second SetRange("Column No.", SchemeColNo) to avoid unexpected filter accumulation.

Suggested change

	FieldRef := RecordRef.Field(IntermediateDataImport."Field ID");
	DataExchField.SetRange("Data Exch. No.", DataExch."Entry No.");
	DataExchField.SetRange("Column No.", ValueColNo);
	if DataExchField.FindFirst() then
	EndpointValue := DataExchField.Value;
	DataExchField.SetRange("Column No.", SchemeColNo); // Column No. is reset; Data Exch. No. stays
	if DataExchField.FindFirst() then
	EndpointScheme := DataExchField.Value;

_{👍 useful · ❤️ especially valuable · 👎 wrong - reply with why}

[github-actions]

$\textbf{🟡\ Medium\ Severity\ —\ Performance} \quad \color{gray}{\texttt{\small Iteration\ 1}}$

MapIntermediateToLines doesn't reset RecordRef between records

MapIntermediateToLines uses a RecordRef obtained from a new EDocumentPurchaseLine record but never calls Clear() or reinitialises the RecordRef when processing a new Record No. If a field was set in a previous iteration and the current iteration's intermediate data omits that field, the stale value from the previous line will silently carry forward.

Recommendation:

• For each new Record No., initialise a fresh EDocumentPurchaseLine and reset the RecordRef before assigning fields.

Suggested change

	if DataExchField.FindFirst() then
	if CurrRecordNo <> IntermediateDataImport."Record No." then begin
	if CurrRecordNo <> 0 then begin
	RecordRef.SetTable(EDocumentPurchaseLine);
	EDocumentPurchaseLine.Insert();
	end;
	CurrRecordNo := IntermediateDataImport."Record No.";
	Clear(EDocumentPurchaseLine);
	EDocumentPurchaseLine."E-Document Entry No." := EDocument."Entry No.";
	RecordRef.GetTable(EDocumentPurchaseLine);
	end;

_{👍 useful · ❤️ especially valuable · 👎 wrong - reply with why}

[github-actions]

$\textbf{🟡\ Medium\ Severity\ —\ Performance} \quad \color{gray}{\texttt{\small Iteration\ 1}}$

AttachmentTempBlob not reset between attachment records

In ProcessAttachments, the AttachmentTempBlob variable is never cleared between iterations of different Record No. groups. If one attachment record writes blob data and a subsequent attachment record sets only the FileName (e.g. the Document Reference ID field is missing or processed later), the new attachment will be associated with the previous record's binary data.

Recommendation:

• Reset AttachmentTempBlob at the start of each new record group by declaring a new variable or explicitly calling Clear(AttachmentTempBlob).

Suggested change

	EDocAttachmentProcessor.Insert(EDocument, InStream, FileName);
	if CurrRecordNo <> IntermediateDataImport."Record No." then begin
	// flush previous
	if (CurrRecordNo <> -1) and (FileName <> '') then begin
	AttachmentTempBlob.CreateInStream(InStream);
	EDocAttachmentProcessor.Insert(EDocument, InStream, FileName);
	end;
	CurrRecordNo := IntermediateDataImport."Record No.";
	FileName := '';
	Clear(AttachmentTempBlob); // reset blob for new record
	end;

_{👍 useful · ❤️ especially valuable · 👎 wrong - reply with why}

[github-actions]

$\textbf{🟡\ Medium\ Severity\ —\ Performance} \quad \color{gray}{\texttt{\small Iteration\ 1}}$

DeleteIntermediateData called only on success path

DeleteIntermediateData is the last call in RunPipelineAndBridge and cleans up DataExchField and IntermediateDataImport rows. If BridgeToStagingTables throws an error, this cleanup is never reached, leaving intermediate import rows in the database indefinitely.

Recommendation:

• Ensure DeleteIntermediateData is called even on failure, either by wrapping the pipeline in a try-function with finally-equivalent cleanup, or by calling it inside an error handler.

Suggested change

	DataExchField.DeleteAll();
	local procedure RunPipelineAndBridge(...)
	begin
	// ... setup ...
	if not TryRunBridge(EDocument, DataExch, DataExchDef) then begin
	DeleteIntermediateData(DataExch);
	DataExch.Delete(true);
	Error(GetLastErrorText());
	end;
	DeleteIntermediateData(DataExch);
	end;

_{👍 useful · ❤️ especially valuable · 👎 wrong - reply with why}

[github-actions]

$\textbf{🟡\ Medium\ Severity\ —\ Security} \quad \color{gray}{\texttt{\small Iteration\ 1}}$

Base64 decoded attachment data not validated

In ProcessAttachments, the Base64-decoded attachment data from IntermediateDataImport.GetValue() is decoded and inserted directly via EDocAttachmentProcessor.Insert without any validation of file type, size, or content. A malformed or oversized attachment embedded in an e-document XML could cause unexpected behavior or storage issues.

Recommendation:

• Add a size check on the decoded blob and consider validating the file extension against an allowlist before inserting the attachment.

Suggested change

	// After decoding:
	Base64Convert.FromBase64(Base64Data, OutStream);
	// Validate size (example: 10 MB max)
	AttachmentTempBlob.CreateInStream(InStream);
	if AttachmentTempBlob.Length() > 10 * 1024 * 1024 then
	Error('Attachment exceeds maximum allowed size.');

_{👍 useful · ❤️ especially valuable · 👎 wrong - reply with why}

[github-actions]

$\textbf{🟡\ Medium\ Severity\ —\ Style} \quad \color{gray}{\texttt{\small Iteration\ 1}}$

View() uses hardcoded non-localized error string

Error('A view is not implemented for this handler.') uses a string literal instead of a labeled text constant. This is not translatable and violates AL localization requirements — all user-facing strings must be declared as Label constants.

Recommendation:

• Declare the message as a locked or translatable Label and reference it in the Error call.

Suggested change

	var
	ViewNotImplementedErr: Label 'A view is not implemented for this handler.', Locked = true;
	procedure View(EDocument: Record "E-Document"; TempBlob: Codeunit "Temp Blob")
	begin
	Error(ViewNotImplementedErr);
	end;

_{👍 useful · ❤️ especially valuable · 👎 wrong - reply with why}

[github-actions]

$\textbf{🟡\ Medium\ Severity\ —\ Style} \quad \color{gray}{\texttt{\small Iteration\ 1}}$

Error label missing translator Comment

ProcessFailedErr: Label 'Failed to process the file with data exchange.' has no Comment property. Translators need context comments to understand where and why the message appears, especially for technical messages involving "data exchange".

Recommendation:

• Add a Comment parameter to the Label declaration to describe the context for translators.

ProcessFailedErr: Label 'Failed to process the file with data exchange.', Comment = 'Error shown when PEPPOL BIS 3 document processing via Data Exchange fails — shown to the user in the E-Document processing page.';

Line mapping was unavailable, so this was posted as an issue comment.

_{👍 useful · ❤️ especially valuable · 👎 wrong - reply with why}

[github-actions]

$\textbf{🟠\ High\ Severity\ —\ Upgrade} \quad \color{gray}{\texttt{\small Iteration\ 1}}$

DataExchDef deleted before re-import without rollback

ImportInvoiceV2XML (and ImportCreditMemoV2XML) unconditionally delete the existing Data Exchange Definition record before reimporting it. If NavApp.GetResource or Xmlport.Import subsequently fails, the definition is permanently destroyed — there is no rollback or restore mechanism.

Recommendation:

• Only delete the old definition after a successful import, or use a transaction-safe pattern: import into a temp-check first, then replace. Alternatively, check whether a delete-and-reimport is actually needed at install time vs. a conditional insert.

Suggested change

	DataExchDef.Delete(true);
	// Do not pre-delete; let the Xmlport handle upsert, or:
	// 1. Import into a temporary staging area
	// 2. Only delete existing definition if staging import succeeded
	if DataExchDef.Get('EDOCPEPINVIMPV2') then begin
	NavApp.GetResource('DataExchange/eDocPEPPOLInvoiceImportV2.xml', ResInStream);
	// ... import ...
	DataExchDef.Delete(true); // delete AFTER successful import
	end;

_{👍 useful · ❤️ especially valuable · 👎 wrong - reply with why}

[github-actions]

$\textbf{🟠\ High\ Severity\ —\ Upgrade} \quad \color{gray}{\texttt{\small Iteration\ 1}}$

Field 40 renamed without deprecation

Field 40 on table "E-Document Purchase Header" was renamed from "Vendor Invoice No." to "Applies-to Ext. Invoice No." without an intermediate ObsoleteState::Pending period. Any extension referencing the old name — including the existing EDocumentPEPPOLHandler before this PR — will break at compile time.

Recommendation:

• Introduce ObsoleteState = Pending on the old field and add a new field with the new name and number, or if this table is still in preview, document the breaking change explicitly in release notes and ensure all callers in the same PR are updated.

field(40; "Vendor Invoice No."; Text[100])
{
    ObsoleteState = Pending;
    ObsoleteReason = 'Use "Applies-to Ext. Invoice No." instead.';
    ObsoleteTag = '26.0';
}
field(41; "Applies-to Ext. Invoice No."; Text[100])
{
    Caption = 'Applies-to Ext. Invoice No.';
}

Line mapping was unavailable, so this was posted as an issue comment.

_{👍 useful · ❤️ especially valuable · 👎 wrong - reply with why}

[github-actions]

$\textbf{🟡\ Medium\ Severity\ —\ Upgrade} \quad \color{gray}{\texttt{\small Iteration\ 1}}$

New enum value ordinal may conflict with extensions

A new enum value value(5; "PEPPOL Data Exchange") is inserted into "E-Doc. Read into Draft". If any partner extension has already added a value with ordinal 5 to this enum, the two definitions will conflict at compile time. There is no way for Microsoft to know this without checking the extension ecosystem.

Recommendation:

• Use a higher, partner-unlikely ordinal (e.g. 100+ or following an established spacing convention) for platform-reserved new enum values, or document the ordinal reservation scheme so partners know which ranges to avoid.

Suggested change

	{
	// Consider using a higher ordinal to avoid collision:
	value(100; "PEPPOL Data Exchange")
	{
	Caption = 'PEPPOL BIS 3 - Data Exchange';
	Implementation = IStructuredFormatReader = "E-Doc. PEPPOL DX Handler";
	}

_{👍 useful · ❤️ especially valuable · 👎 wrong - reply with why}