FIX never unpay an invoice that carries real payments

[Pichinov-Jose]

Bug

setPaidFlag(false) (Invoice MainTrait) reopens a closed invoice whenever the source announces not paid — even when the invoice carries real, sometimes bank-reconciled, payment entries. The e-commerce side's opinion of the payment state is allowed to outrank recorded money.

Combined with a source that mis-reports its paid flag (see SplashSync/Wordpress#19: ispaid derived from the delivery workflow), invoices flip-flop between paid and unpaid on every sync, and accounting states drift.

Fix

Before unpaying, count the invoice's payment lines: if any exist, keep the invoice paid and log a warning. Mirrors the philosophy of the existing payments-list protections (a source cannot destroy richer local payment knowledge).

🤖 Generated with Claude Code

[Pichinov-Jose]

Two adaptations were needed to run this on a live 2.23.3 install — worth folding into the branch, because the diff as written does not execute.

1. $this->db does not exist in this module.

Invoice extends AbstractObject (Splash's own model), which carries no $db property, and a grep over splash/src/Objects/ returns no $this->db anywhere. $this->db->query($sqlNbPay) is therefore a call on null. The module's own idiom is global $db; — ItemsTrait::… and PaymentsTrait::loadPayments() / deletePayment() all do exactly that. setPaidFlag() already declares global $user;, so it is a one-word change.

2. This trait also serves supplier invoices.

MainTrait is shared: buildMainFields() branches on is_a($this, Local::CLASS_SUPPLIER_INVOICE) for date_echeance vs date_lim_reglement, and loadPayments() switches tables the same way. Supplier payment links live in llx_paiementfourn_facturefourn keyed on fk_facturefourn, not in llx_paiement_facture keyed on fk_facture.

Counting customer payment links by a supplier invoice id matches unrelated rows — the two id sequences are independent, so a supplier invoice would be kept paid because some unrelated customer invoice happens to share its rowid. That is the opposite of the intent.

What is running here:

private function setPaidFlag($data): bool
{
    global $user, $db;
    ...
    } else {
        $isSupplier = is_a($this, Local::CLASS_SUPPLIER_INVOICE);
        $sqlNbPay = "SELECT COUNT(*) as nb FROM ".MAIN_DB_PREFIX
            .($isSupplier ? "paiementfourn_facturefourn" : "paiement_facture")
            ." WHERE ".($isSupplier ? "fk_facturefourn" : "fk_facture")." = ".((int) $this->object->id);
        $resNbPay = $db->query($sqlNbPay);
        $objNbPay = $resNbPay ? $db->fetch_object($resNbPay) : null;
        if ($objNbPay && ((int) $objNbPay->nb) > 0) {
            dol_syslog(...);

            return true;
        }
        //====================================================================//
        // Set UnPaid using Dolibarr Function
        ...

Local is already imported by the file, so no new use is needed.

Deployed in production on a Dolibarr 23.0.0 shop on 12/09. Verified against the case it is meant to protect: 173 orders change their announced payment status under SplashSync/Wordpress#19, 80 of them have a Dolibarr invoice, 59 are closed — and all 59 carry at least one payment, so this guard covers every one of them. Without it, #19 alone reopens 136 refunded invoices.

Also opened #31, the symmetric half: never close an invoice the recorded payments do not cover. The two touch different branches of the same if/else and merge cleanly in either order.

[Pichinov-Jose]

Correction to my own note above, on point 2.

I checked the claim properly after @seriousupgrade pushed back, and they are right: the supplier-invoice concern does not apply here. Invoice\MainTrait is used by exactly two classes, Objects/Invoice.php and Objects/CreditNote.php — there is no SupplierInvoice class in this repository at all, and Local::CLASS_SUPPLIER_INVOICE points at a class that lives elsewhere.

I generalised from the is_a($this, Local::CLASS_SUPPLIER_INVOICE) branches inside the trait (buildMainFields(), and the table switch in PaymentsTrait::loadPayments()) and assumed the trait was shared. It anticipates supplier invoices; it never actually serves one. So the table switch is defensive hardening, not a correctness fix, and setPaidFlag() as written on this branch cannot match unrelated rows. My "opposite of the intent" wording was wrong.

Point 1 stands unchanged and is the one that matters: $this->db does not exist on these objects, so $this->db->query() is a call on null. global $db; is the module's own idiom.

Apologies for the noise on the second point — better to have it on the record than to leave an incorrect claim standing in the thread.