From 5843fda2d602d5d5ab98aecef8dc7091f9f98dcd Mon Sep 17 00:00:00 2001 From: Tim <43742253+TQxTim@users.noreply.github.com> Date: Thu, 4 Jun 2026 11:07:14 +0200 Subject: [PATCH] QA: harden DTFB player sync receiver Applies 6 fixes to sync.php found during QA of the player-sync feature: 1. Normalise non-UTF-8 (latin1/Win-1252) payloads -> fixes silent 0-row imports 2. Fail loudly (success=false) when N rows parse but nothing is added/updated 3. Remove dead \ block (undefined-variable notice) 4. Gate mass-deactivation: skip the sweep when a payload carries < 50% of an org's currently-active members (configurable via sync_deactivation_min_ratio, default 0.5); adds/updates still proceed, skipped sweeps return warnings 5. Use a single DB clock (NOW()) for staging session id/cleanup 6. Enforce Passnummer format ^[0-9]{2}-[0-9]{4,6}\$ (parity with manual import) Adds tests/dtfb-player-sync/FINDINGS.md documenting the findings and fixes. End-to-end validation is to be done on the staging environments. --- .../components/com_sportsmanager/sync.php | 111 ++++++++++++++---- tests/dtfb-player-sync/FINDINGS.md | 23 ++++ 2 files changed, 112 insertions(+), 22 deletions(-) create mode 100644 tests/dtfb-player-sync/FINDINGS.md diff --git a/src/structure/components/com_sportsmanager/sync.php b/src/structure/components/com_sportsmanager/sync.php index ba9b617..5c30922 100644 --- a/src/structure/components/com_sportsmanager/sync.php +++ b/src/structure/components/com_sportsmanager/sync.php @@ -283,6 +283,15 @@ function syncReceiveSpielerImport(string $csvData): array $db = getDatabase(); + // Normalise the payload to UTF-8. The automatic/semi-automatic sync path is + // UTF-8 end to end, but a legacy/manual export (e.g. from TFVHH) can be + // latin1/Windows-1252 encoded. Without this, non-ASCII bytes (e.g. "ß" in an + // organisation name) get truncated when staged into the utf8mb4 import table, + // causing the organisation match to fail and every row to be skipped silently. + if (!mb_check_encoding($csvData, 'UTF-8')) { + $csvData = mb_convert_encoding($csvData, 'UTF-8', 'Windows-1252'); + } + $lines = explode("\n", str_replace("\r", "", $csvData)); if (count($lines) < 2) { return [ @@ -378,7 +387,14 @@ function syncReceiveSpielerImport(string $csvData): array } $lineIdx++; - $session_id = date('Y-m-d H:i:s'); + // Source the staging session id from the database clock, not PHP's. The + // stale-row cleanup below compares session_id against the database NOW(); if + // PHP and the database run in different timezones, a PHP-generated timestamp + // can fall outside the window and the just-inserted rows get deleted mid-import. + $session_id = loadResult($db, "SELECT DATE_FORMAT(NOW(), '%Y-%m-%d %H:%i:%s')"); + if (empty($session_id)) { + $session_id = date('Y-m-d H:i:s'); + } $organisations = []; $rows_to_insert = []; @@ -442,11 +458,17 @@ function syncReceiveSpielerImport(string $csvData): array $geschlecht = isset($spalte["geschlecht"]) && !empty($daten[$spalte["geschlecht"]]) ? (($daten[$spalte["geschlecht"]][0] === "M" || $daten[$spalte["geschlecht"]][0] === "m" || $daten[$spalte["geschlecht"]][0] === "H" || $daten[$spalte["geschlecht"]][0] === "h") ? "M" : "W") : "M"; $spielernr = isset($daten[$spalte["spielernr"]]) ? trim($daten[$spalte["spielernr"]]) : ""; - if (!empty($spielernr) && !ctype_digit(substr($spielernr, strlen($spielernr) - 1, 1))) { + // Validate the Passnummer with the same format the manual import enforces + // (NN-NNNN[NN]). Invalid values are dropped rather than aborting the whole + // automated feed, keeping the player but treating them as having no pass. + if (!empty($spielernr) && !preg_match('/^[0-9]{2}-[0-9]{4,6}$/', $spielernr)) { $spielernr = ""; } $spielernr_alt = isset($spalte["spielernr_alt"]) && isset($daten[$spalte["spielernr_alt"]]) ? trim($daten[$spalte["spielernr_alt"]]) : ""; + if (!empty($spielernr_alt) && !preg_match('/^[0-9]{2}-[0-9]{4,6}$/', $spielernr_alt)) { + $spielernr_alt = ""; + } $lizenznr = isset($spalte["lizenznr"]) && isset($daten[$spalte["lizenznr"]]) ? $daten[$spalte["lizenznr"]] : ""; if (!empty($lizenznr) && !ctype_digit(substr($lizenznr, strlen($lizenznr) - 1, 1))) { $lizenznr = ""; @@ -546,8 +568,52 @@ function syncReceiveSpielerImport(string $csvData): array . "\n WHERE session_id = '" . $db->escape($session_id) . "'"; $spieler_import = loadObjectList($db, $query); - // Deactivate all memberships for involved organisations temporarily + // Count how many active players the incoming payload provides per organisation. + // The mass-deactivation below is only safe when the payload is a *full* roster; + // a partial CSV would otherwise silently deactivate every member not listed. + $incoming_per_org = []; + foreach ($rows_to_insert as $row) { + $o = trim($row['organisation']); + if ($o !== "") { + $incoming_per_org[$o] = ($incoming_per_org[$o] ?? 0) + 1; + } + } + // Minimum fraction of the currently-active roster the payload must contain + // before the sweep is allowed to run. Configurable; defaults to 0.5. + $deactivation_min_ratio = (float) (einstellungswert("sync_deactivation_min_ratio") ?? 0.5); + if ($deactivation_min_ratio <= 0 || $deactivation_min_ratio > 1) { + $deactivation_min_ratio = 0.5; + } + + $warnings = []; + $deactivated_total = 0; + + // Deactivate all memberships for involved organisations temporarily. The + // players present in the payload are reactivated further below; anyone not + // listed stays deactivated (i.e. is treated as having left the organisation). foreach ($org_map as $orgName => $veranstalterId) { + $aktiv_vorher = (int) loadResult( + $db, + "SELECT COUNT(*) FROM #__sportsmanager_mitglied_von_verein" + . " INNER JOIN #__sportsmanager_verein USING (verein_id)" + . " WHERE veranstalter_id = " . $veranstalterId + . " AND NOT #__sportsmanager_mitglied_von_verein.ausgetreten" + ); + $eingehend = $incoming_per_org[$orgName] ?? 0; + + // Guard: skip the sweep when the payload looks like a partial roster + // (far fewer players than are currently active). This prevents a partial + // export from wiping an entire organisation's memberships. + if ($aktiv_vorher > 0 && $eingehend < $aktiv_vorher * $deactivation_min_ratio) { + $warnings[] = sprintf( + 'Massen-Deaktivierung für "%s" übersprungen: nur %d von %d aktiven Mitgliedern in den Daten (mögliche Teil-Liste).', + $orgName, + $eingehend, + $aktiv_vorher + ); + continue; + } + $query = "UPDATE #__sportsmanager_mitglied_von_verein INNER JOIN #__sportsmanager_verein USING (verein_id)" . "\n SET mitgliedsstatus = 0," . "\n #__sportsmanager_mitglied_von_verein.ausgetreten = TRUE" @@ -559,6 +625,7 @@ function syncReceiveSpielerImport(string $csvData): array 'message' => 'Fehler beim Deaktivieren der alten Vereinsmitgliedschaften.' ]; } + $deactivated_total += $aktiv_vorher; } $spieler_updated = 0; @@ -632,24 +699,6 @@ function syncReceiveSpielerImport(string $csvData): array $spieler_id = $db->insertid(); $spielerIdsHinzugefuegt[$spielernr] = $spieler_id; - if ($spielernr === $naechste_spielernr) { - do { - for ($idx = strlen($naechste_spielernr) - 1; $idx >= 0; $idx--) { - if ($naechste_spielernr[$idx] < '0' || $naechste_spielernr[$idx] > '9') { - $naechste_spielernr = substr($naechste_spielernr, 0, $idx + 1) . "1" . substr($naechste_spielernr, $idx + 1); - break; - } - if ($naechste_spielernr[$idx] <= '8') { - $naechste_spielernr[$idx] = $naechste_spielernr[$idx] + 1; - break; - } - $naechste_spielernr[$idx] = '0'; - } - if ($idx < 0) { - $naechste_spielernr = "1" . $naechste_spielernr; - } - } while (isset($spielerIdsHinzugefuegt[$naechste_spielernr])); - } $spieler_added++; } @@ -724,6 +773,22 @@ function syncReceiveSpielerImport(string $csvData): array $db->setQuery($query); $db->execute(); + // Fail loudly on a zero-effect import: if valid rows were parsed but nothing + // was added or updated, the data almost certainly failed to map (e.g. an + // encoding mismatch corrupting organisation names). Reporting success here + // would silently hide data loss. + if (count($rows_to_insert) > 0 && $spieler_added === 0 && $spieler_updated === 0) { + return [ + 'success' => false, + 'message' => 'Import ergab keine Änderungen trotz ' . count($rows_to_insert) + . ' gültiger Zeilen – mögliche Encoding- oder Zuordnungsfehler.', + 'spieler_count' => count($rows_to_insert), + 'spieler_updated' => 0, + 'spieler_added' => 0, + 'warnings' => $warnings + ]; + } + aktuellerVereinAktualisieren(); ranglisteAktualisieren(); einstufungAktualisieren(); @@ -732,7 +797,9 @@ function syncReceiveSpielerImport(string $csvData): array 'success' => true, 'spieler_count' => count($rows_to_insert), 'spieler_updated' => $spieler_updated, - 'spieler_added' => $spieler_added + 'spieler_added' => $spieler_added, + 'deactivated' => $deactivated_total, + 'warnings' => $warnings ]; } diff --git a/tests/dtfb-player-sync/FINDINGS.md b/tests/dtfb-player-sync/FINDINGS.md new file mode 100644 index 0000000..46fadb4 --- /dev/null +++ b/tests/dtfb-player-sync/FINDINGS.md @@ -0,0 +1,23 @@ +# DTFB Player Sync — QA findings & applied fixes + +Concise record of the defects found while reviewing the player-sync receiver +(`syncReceiveSpielerImport()` in `sync.php`) and the six fixes applied in this PR. +The exploratory test harness used to find these has been removed — the +authoritative next test is the **staging end-to-end sync** (see the PR description). + +## The six issues fixed + +| # | Issue | Impact | Fix in `sync.php` | +|---|-------|--------|-------------------| +| 1 | **Receiver did not normalise input encoding.** A latin1 / Windows-1252 CSV (the legacy manual export) has `ß` as the single byte `0xDF`. Staging the org name into the utf8mb4 table truncated it at that byte, so the org lookup missed and **every row was skipped**. | Critical — silent data loss (e.g. 2964 rows parsed, 0 imported, `success=true`). | Transcode the payload to UTF-8 when it is not already valid UTF-8, before staging. | +| 2 | **A zero-effect import reported success.** When N rows parsed but nothing was added or updated, the function returned `success=true`. | Critical — masks encoding/mapping failures. | Return `success=false` with a diagnostic message when `rows>0` but `added==0 && updated==0`. | +| 3 | **Dead `$naechste_spielernr` block** referenced an undefined variable in the insert branch. | Runtime notice; incoming rows already carry their Passnummer. | Removed the block. | +| 4 | **Unconditional mass-deactivation.** A partial CSV deactivated every member not listed. | High — a broken/partial export could wipe an org's roster. | Per organisation, skip the deactivation sweep when the incoming count is below `sync_deactivation_min_ratio` (default 0.5) of the org's currently-active members; adds/updates still proceed and a warning is returned. | +| 5 | **Split clock for staging.** `session_id` came from PHP `date()` while stale-row cleanup used MySQL `NOW()`; a PHP/MySQL timezone gap could delete in-flight staging rows. | Medium — another silent 0-row path. | Derive `session_id` from the DB clock (`NOW()`), with `date()` fallback. | +| 6 | **No Passnummer format check.** The manual import UI enforces `^[0-9]{2}-[0-9]{4,6}$`; the sync receiver did not. | Medium — inconsistent data quality vs the manual flow. | Apply the same regex to `spielernr` / `spielernr_alt` (blank/reject on mismatch). | + +## Confirmed by design (not bugs) +- **No contact / personal data** (email, phone, address) is ever exported or imported. +- Existing players keep their `lizenznr` and `geburtsjahr` on update; only name / sex / Passnummer-driven fields change. +- An unknown organisation aborts the whole import with no mutation. +- The sync path itself (export → cURL push → receive) is UTF-8 end-to-end; the encoding defect (#1) only affected ingesting a legacy latin1 *manual* file.