Keep payments the remote source could not have created - #25
Open
Pichinov-Jose wants to merge 3 commits into
Open
Pichinov-Jose wants to merge 3 commits into
Pichinov-Jose wants to merge 3 commits into
Conversation
setPaymentLineFields() pairs local payments with remote lines by position
(array_shift), then deletes everything left over. A payment entered by hand
in Dolibarr, for a movement the source never knew about, is therefore either
reused for an unrelated remote line or silently destroyed on the next sync,
together with its bank entry.
Two kinds of payment can never have come from the source, and are now set
aside before any pairing happens:
- a payment method with no Splash equivalent (PaymentMethods::getSplashCode
returns the raw Dolibarr code when it knows no mapping), such as holiday
vouchers or any site-specific method;
- a payment whose bank entry is already reconciled with a statement.
Both are removed from the working list, so they are neither reused nor
deleted, and a warning names the payment that was left alone. Payments using
a mapped method stay fully managed by Splash: existing behaviour is unchanged
for everything the source can actually express.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Pichinov-Jose
marked this pull request as ready for review
September 5, 2026 00:06
Pichinov-Jose
marked this pull request as ready for review
September 5, 2026 00:06
Pichinov-Jose
marked this pull request as draft
September 5, 2026 00:50
Pichinov-Jose
marked this pull request as ready for review
September 5, 2026 00:51
The two previous guards ask "could the source have created this payment?". That misses the most damaging case, and the one the report opens with: a four-cheque instalment plan, entered by hand, using a payment method the source knows perfectly well. A source that models one payment per order cannot describe a schedule. When the local side holds several payments that already cover the invoice, pairing them one-by-one with the source's shorter list keeps the first and deletes the rest — the schedule is replaced by a single line, for the same total. There is nothing to write in that case: the local set is a refinement of the very fact the source is reporting. The invoice is left untouched, neither paired, deleted, nor added to, and a warning records the decision. Only triggers with at least two local payments, more than the source declares, and a local sum that already covers the invoice. A single local payment, or a source that describes as many lines as the target holds, is unaffected. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Two gaps found while replaying the guards against the affected install. Credit notes were not counted as settlement. An invoice closed by payments plus a discount was treated as under-paid, so the richer-decomposition guard did not apply to it. One real case: two payments entered by hand, 407,00 of 419,21, the remaining 12,21 cancelled by a credit note. Fully settled, and unprotected. getSumCreditNotesUsed() and getSumDepositsUsed() now count. A payment whose method the source does not declare is now kept. Cash recorded locally against an order the shop believes was paid by card is not another version of the same movement — it is a movement the source knows nothing about. Pairing them by position rewrites the local one to the source's amount and method, which is how a partial local settlement gets destroyed: enter ten euros in cash, let the shop mark the order complete, and the entry comes back as a card payment for the full total. This is the case the previous guards missed. The richer-decomposition guard needs the local set to already cover the invoice, so it cannot help while a settlement is still incomplete — precisely when the local record is the only one that exists. The trade-off is explicit: Splash can no longer change the payment method of an existing payment to one it did not previously declare. Correcting a method becomes a local operation. Given that the alternative silently destroys offline settlements, that seems the right way round. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
This was referenced Sep 5, 2026
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.
The problem
setPaymentLineFields()pairs local payments with remote lines by position —setPaymentLineData()starts with
array_shift($this->payments)— and then deletes every payment left over:The only guard is for a payment spread over several invoices. So a payment entered by hand in
Dolibarr, for a movement the source never knew about, is either reused for an unrelated remote line
or deleted outright — and
Paiement::delete()takes thellx_bankentry with it.Nothing in the source ever held that information, so it is simply lost.
This is not theoretical
On a production install, Dolibarr's tamper-evident log (
llx_blockedlog, module Archive) records113
PAYMENT_CUSTOMER_DELETEevents for one season, 93 of them performed by the connector's APIuser. Cross-referencing each deletion with its creation event shows 11 payments entered by a human
operator that the connector deleted, totalling 2 259,93 €.
It does not even wait for the operator to finish. On 12 March 2026 they were entering a four-cheque
instalment plan on one invoice, then a three-cheque plan on the next:
Four of the eleven were French ANCV holiday vouchers — a payment method that exists only offline
and can never reach the source. The log preserves the payload, and the type is explicit:
{"ref":"PAY2511-0777","type_code":"ANCV","payment_part":{"1":{"amount":"393.46", ...}}}Those were later replaced by card payments carrying no gateway reference, because no card transaction
ever existed. The true payment method, date and bank account were all lost.
The change
Three guards, applied before any pairing happens.
The invoice is left untouched when the local decomposition is richer than the source's. A source
that models one payment per order cannot describe an instalment plan. When the local side holds
several payments that already cover the invoice, and the source declares fewer lines, there is
nothing to write: the local set is a refinement of the very fact being reported. Nothing is paired,
deleted or added. This is the guard that covers the four-cheque plan above — those payments use
CHQ, a method the source expresses perfectly well, so no per-payment test can save them.Three further kinds of payment can never have come from the source, and are set aside individually:
shop believes was paid by card is not another version of the same movement — it is a movement the
source knows nothing about. This is what protects a settlement still in progress, where the local
record is the only one that exists.
PaymentMethods::getSplashCode()returns the rawDolibarr code when it knows no mapping, so an unmapped method is never a key of
PaymentMethods::KNOWN. Holiday vouchers, and any site-specific method, fall here.rapproornum_releveset). The accounting is closed; a sync has no business rewriting it.
Each decision is logged as a warning, so the behaviour is visible rather than silent.
Settlement is counted the way the ERP counts it:
getSumCreditNotesUsed()andgetSumDepositsUsed()are added to the payments. An invoice closed by payments plus a discount issettled just as surely as one closed by payments alone, and the first guard must see that.
Scope
The first guard needs at least two local payments, strictly more than the source declares, and a
local total that already settles the invoice. A single local payment, or a source describing as many
lines as the target holds, is unaffected — so an ordinary order syncs exactly as before.
A payment using the method the source declares, not reconciled, on an invoice with a single payment
line stays fully managed by Splash. No configuration is added: the guards only ever protect data the
source could not own, could not express, or never mentioned.
One trade-off is explicit: Splash can no longer change an existing payment's method to one it did
not previously declare — correcting a method becomes a local operation. Given that the alternative
silently destroys offline settlements, that seems the right way round, but it is a behaviour change
and worth a maintainer's opinion.
clearPayments()is deliberately untouched, as it is documented as debug-only for PHPUnit.Verified on the affected install
Replaying the guards against the six invoices from the incident:
CHQCHQCB+ 2 ×CHQANCVANCVCB+VIR, plus a 12,21 € credit noteCBThe last row is the refunded-order case: a single mapped payment, which this PR deliberately does
not cover. That one belongs to the source declaring it in the first place — see the related issue.
Related
Filed separately as SplashSync/Wordpress#13: a WooCommerce order moving to
refundedstopsdeclaring its payment, which makes this same loop delete a legitimately settled payment. That issue
is what led here, but the loop needs a guard of its own regardless of what any source declares.