mirror of
https://github.com/Deutscher-Tischfussballbund/com_sportsmanager.git
synced 2026-06-10 06:27:52 +00:00
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.
This commit is contained in:
@@ -283,6 +283,15 @@ function syncReceiveSpielerImport(string $csvData): array
|
|||||||
|
|
||||||
$db = getDatabase();
|
$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));
|
$lines = explode("\n", str_replace("\r", "", $csvData));
|
||||||
if (count($lines) < 2) {
|
if (count($lines) < 2) {
|
||||||
return [
|
return [
|
||||||
@@ -378,7 +387,14 @@ function syncReceiveSpielerImport(string $csvData): array
|
|||||||
}
|
}
|
||||||
|
|
||||||
$lineIdx++;
|
$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 = [];
|
$organisations = [];
|
||||||
$rows_to_insert = [];
|
$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";
|
$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"]]) : "";
|
$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 = "";
|
||||||
}
|
}
|
||||||
|
|
||||||
$spielernr_alt = isset($spalte["spielernr_alt"]) && isset($daten[$spalte["spielernr_alt"]]) ? trim($daten[$spalte["spielernr_alt"]]) : "";
|
$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"]] : "";
|
$lizenznr = isset($spalte["lizenznr"]) && isset($daten[$spalte["lizenznr"]]) ? $daten[$spalte["lizenznr"]] : "";
|
||||||
if (!empty($lizenznr) && !ctype_digit(substr($lizenznr, strlen($lizenznr) - 1, 1))) {
|
if (!empty($lizenznr) && !ctype_digit(substr($lizenznr, strlen($lizenznr) - 1, 1))) {
|
||||||
$lizenznr = "";
|
$lizenznr = "";
|
||||||
@@ -546,8 +568,52 @@ function syncReceiveSpielerImport(string $csvData): array
|
|||||||
. "\n WHERE session_id = '" . $db->escape($session_id) . "'";
|
. "\n WHERE session_id = '" . $db->escape($session_id) . "'";
|
||||||
$spieler_import = loadObjectList($db, $query);
|
$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) {
|
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)"
|
$query = "UPDATE #__sportsmanager_mitglied_von_verein INNER JOIN #__sportsmanager_verein USING (verein_id)"
|
||||||
. "\n SET mitgliedsstatus = 0,"
|
. "\n SET mitgliedsstatus = 0,"
|
||||||
. "\n #__sportsmanager_mitglied_von_verein.ausgetreten = TRUE"
|
. "\n #__sportsmanager_mitglied_von_verein.ausgetreten = TRUE"
|
||||||
@@ -559,6 +625,7 @@ function syncReceiveSpielerImport(string $csvData): array
|
|||||||
'message' => 'Fehler beim Deaktivieren der alten Vereinsmitgliedschaften.'
|
'message' => 'Fehler beim Deaktivieren der alten Vereinsmitgliedschaften.'
|
||||||
];
|
];
|
||||||
}
|
}
|
||||||
|
$deactivated_total += $aktiv_vorher;
|
||||||
}
|
}
|
||||||
|
|
||||||
$spieler_updated = 0;
|
$spieler_updated = 0;
|
||||||
@@ -632,24 +699,6 @@ function syncReceiveSpielerImport(string $csvData): array
|
|||||||
$spieler_id = $db->insertid();
|
$spieler_id = $db->insertid();
|
||||||
$spielerIdsHinzugefuegt[$spielernr] = $spieler_id;
|
$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++;
|
$spieler_added++;
|
||||||
}
|
}
|
||||||
|
|
||||||
@@ -724,6 +773,22 @@ function syncReceiveSpielerImport(string $csvData): array
|
|||||||
$db->setQuery($query);
|
$db->setQuery($query);
|
||||||
$db->execute();
|
$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();
|
aktuellerVereinAktualisieren();
|
||||||
ranglisteAktualisieren();
|
ranglisteAktualisieren();
|
||||||
einstufungAktualisieren();
|
einstufungAktualisieren();
|
||||||
@@ -732,7 +797,9 @@ function syncReceiveSpielerImport(string $csvData): array
|
|||||||
'success' => true,
|
'success' => true,
|
||||||
'spieler_count' => count($rows_to_insert),
|
'spieler_count' => count($rows_to_insert),
|
||||||
'spieler_updated' => $spieler_updated,
|
'spieler_updated' => $spieler_updated,
|
||||||
'spieler_added' => $spieler_added
|
'spieler_added' => $spieler_added,
|
||||||
|
'deactivated' => $deactivated_total,
|
||||||
|
'warnings' => $warnings
|
||||||
];
|
];
|
||||||
}
|
}
|
||||||
|
|
||||||
|
|||||||
@@ -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.
|
||||||
Reference in New Issue
Block a user