From f92350bbc32bc8a58fe3a1e5088fc68f382b9d81 Mon Sep 17 00:00:00 2001 From: Angel de la Torre Date: Tue, 11 Aug 2026 10:51:06 -0700 Subject: [PATCH 1/8] feat(ords): add ISO 3166 alpha-2 to alpha-3 helper The Open Repair Data Standard requires alpha-3 country codes, but groups.country_code is alpha-2. Checked in as a static map rather than adding a dependency, so the exporter stays self-contained for instances that pick it up. The only existing country helper returns translated display names, not codes. --- app/Helpers/Iso3166.php | 280 ++++++++++++++++++++++++++++++++++++++++ 1 file changed, 280 insertions(+) create mode 100644 app/Helpers/Iso3166.php diff --git a/app/Helpers/Iso3166.php b/app/Helpers/Iso3166.php new file mode 100644 index 0000000000..c538b39957 --- /dev/null +++ b/app/Helpers/Iso3166.php @@ -0,0 +1,280 @@ + 'AND', + 'AE' => 'ARE', + 'AF' => 'AFG', + 'AG' => 'ATG', + 'AI' => 'AIA', + 'AL' => 'ALB', + 'AM' => 'ARM', + 'AO' => 'AGO', + 'AQ' => 'ATA', + 'AR' => 'ARG', + 'AS' => 'ASM', + 'AT' => 'AUT', + 'AU' => 'AUS', + 'AW' => 'ABW', + 'AX' => 'ALA', + 'AZ' => 'AZE', + 'BA' => 'BIH', + 'BB' => 'BRB', + 'BD' => 'BGD', + 'BE' => 'BEL', + 'BF' => 'BFA', + 'BG' => 'BGR', + 'BH' => 'BHR', + 'BI' => 'BDI', + 'BJ' => 'BEN', + 'BL' => 'BLM', + 'BM' => 'BMU', + 'BN' => 'BRN', + 'BO' => 'BOL', + 'BQ' => 'BES', + 'BR' => 'BRA', + 'BS' => 'BHS', + 'BT' => 'BTN', + 'BV' => 'BVT', + 'BW' => 'BWA', + 'BY' => 'BLR', + 'BZ' => 'BLZ', + 'CA' => 'CAN', + 'CC' => 'CCK', + 'CD' => 'COD', + 'CF' => 'CAF', + 'CG' => 'COG', + 'CH' => 'CHE', + 'CI' => 'CIV', + 'CK' => 'COK', + 'CL' => 'CHL', + 'CM' => 'CMR', + 'CN' => 'CHN', + 'CO' => 'COL', + 'CR' => 'CRI', + 'CU' => 'CUB', + 'CV' => 'CPV', + 'CW' => 'CUW', + 'CX' => 'CXR', + 'CY' => 'CYP', + 'CZ' => 'CZE', + 'DE' => 'DEU', + 'DJ' => 'DJI', + 'DK' => 'DNK', + 'DM' => 'DMA', + 'DO' => 'DOM', + 'DZ' => 'DZA', + 'EC' => 'ECU', + 'EE' => 'EST', + 'EG' => 'EGY', + 'EH' => 'ESH', + 'ER' => 'ERI', + 'ES' => 'ESP', + 'ET' => 'ETH', + 'FI' => 'FIN', + 'FJ' => 'FJI', + 'FK' => 'FLK', + 'FM' => 'FSM', + 'FO' => 'FRO', + 'FR' => 'FRA', + 'GA' => 'GAB', + 'GB' => 'GBR', + 'GD' => 'GRD', + 'GE' => 'GEO', + 'GF' => 'GUF', + 'GG' => 'GGY', + 'GH' => 'GHA', + 'GI' => 'GIB', + 'GL' => 'GRL', + 'GM' => 'GMB', + 'GN' => 'GIN', + 'GP' => 'GLP', + 'GQ' => 'GNQ', + 'GR' => 'GRC', + 'GS' => 'SGS', + 'GT' => 'GTM', + 'GU' => 'GUM', + 'GW' => 'GNB', + 'GY' => 'GUY', + 'HK' => 'HKG', + 'HM' => 'HMD', + 'HN' => 'HND', + 'HR' => 'HRV', + 'HT' => 'HTI', + 'HU' => 'HUN', + 'ID' => 'IDN', + 'IE' => 'IRL', + 'IL' => 'ISR', + 'IM' => 'IMN', + 'IN' => 'IND', + 'IO' => 'IOT', + 'IQ' => 'IRQ', + 'IR' => 'IRN', + 'IS' => 'ISL', + 'IT' => 'ITA', + 'JE' => 'JEY', + 'JM' => 'JAM', + 'JO' => 'JOR', + 'JP' => 'JPN', + 'KE' => 'KEN', + 'KG' => 'KGZ', + 'KH' => 'KHM', + 'KI' => 'KIR', + 'KM' => 'COM', + 'KN' => 'KNA', + 'KP' => 'PRK', + 'KR' => 'KOR', + 'KW' => 'KWT', + 'KY' => 'CYM', + 'KZ' => 'KAZ', + 'LA' => 'LAO', + 'LB' => 'LBN', + 'LC' => 'LCA', + 'LI' => 'LIE', + 'LK' => 'LKA', + 'LR' => 'LBR', + 'LS' => 'LSO', + 'LT' => 'LTU', + 'LU' => 'LUX', + 'LV' => 'LVA', + 'LY' => 'LBY', + 'MA' => 'MAR', + 'MC' => 'MCO', + 'MD' => 'MDA', + 'ME' => 'MNE', + 'MF' => 'MAF', + 'MG' => 'MDG', + 'MH' => 'MHL', + 'MK' => 'MKD', + 'ML' => 'MLI', + 'MM' => 'MMR', + 'MN' => 'MNG', + 'MO' => 'MAC', + 'MP' => 'MNP', + 'MQ' => 'MTQ', + 'MR' => 'MRT', + 'MS' => 'MSR', + 'MT' => 'MLT', + 'MU' => 'MUS', + 'MV' => 'MDV', + 'MW' => 'MWI', + 'MX' => 'MEX', + 'MY' => 'MYS', + 'MZ' => 'MOZ', + 'NA' => 'NAM', + 'NC' => 'NCL', + 'NE' => 'NER', + 'NF' => 'NFK', + 'NG' => 'NGA', + 'NI' => 'NIC', + 'NL' => 'NLD', + 'NO' => 'NOR', + 'NP' => 'NPL', + 'NR' => 'NRU', + 'NU' => 'NIU', + 'NZ' => 'NZL', + 'OM' => 'OMN', + 'PA' => 'PAN', + 'PE' => 'PER', + 'PF' => 'PYF', + 'PG' => 'PNG', + 'PH' => 'PHL', + 'PK' => 'PAK', + 'PL' => 'POL', + 'PM' => 'SPM', + 'PN' => 'PCN', + 'PR' => 'PRI', + 'PS' => 'PSE', + 'PT' => 'PRT', + 'PW' => 'PLW', + 'PY' => 'PRY', + 'QA' => 'QAT', + 'RE' => 'REU', + 'RO' => 'ROU', + 'RS' => 'SRB', + 'RU' => 'RUS', + 'RW' => 'RWA', + 'SA' => 'SAU', + 'SB' => 'SLB', + 'SC' => 'SYC', + 'SD' => 'SDN', + 'SE' => 'SWE', + 'SG' => 'SGP', + 'SH' => 'SHN', + 'SI' => 'SVN', + 'SJ' => 'SJM', + 'SK' => 'SVK', + 'SL' => 'SLE', + 'SM' => 'SMR', + 'SN' => 'SEN', + 'SO' => 'SOM', + 'SR' => 'SUR', + 'SS' => 'SSD', + 'ST' => 'STP', + 'SV' => 'SLV', + 'SX' => 'SXM', + 'SY' => 'SYR', + 'SZ' => 'SWZ', + 'TC' => 'TCA', + 'TD' => 'TCD', + 'TF' => 'ATF', + 'TG' => 'TGO', + 'TH' => 'THA', + 'TJ' => 'TJK', + 'TK' => 'TKL', + 'TL' => 'TLS', + 'TM' => 'TKM', + 'TN' => 'TUN', + 'TO' => 'TON', + 'TR' => 'TUR', + 'TT' => 'TTO', + 'TV' => 'TUV', + 'TW' => 'TWN', + 'TZ' => 'TZA', + 'UA' => 'UKR', + 'UG' => 'UGA', + 'UM' => 'UMI', + 'US' => 'USA', + 'UY' => 'URY', + 'UZ' => 'UZB', + 'VA' => 'VAT', + 'VC' => 'VCT', + 'VE' => 'VEN', + 'VG' => 'VGB', + 'VI' => 'VIR', + 'VN' => 'VNM', + 'VU' => 'VUT', + 'WF' => 'WLF', + 'WS' => 'WSM', + 'YE' => 'YEM', + 'YT' => 'MYT', + 'ZA' => 'ZAF', + 'ZM' => 'ZMB', + 'ZW' => 'ZWE', + ]; + + public static function alpha3(?string $alpha2): ?string + { + if ($alpha2 === null) { + return null; + } + + $key = strtoupper(trim($alpha2)); + + return self::ALPHA2_TO_ALPHA3[$key] ?? null; + } +} From 77668b14f39aa1e9eeda3c6fabfbb892e0d09bf4 Mon Sep 17 00:00:00 2001 From: Angel de la Torre Date: Tue, 11 Aug 2026 10:51:06 -0700 Subject: [PATCH 2/8] feat(ords): add problem text scrubber Redacts emails, phone numbers, long digit runs and URL query strings from volunteer-written free text before it is published. Whitespace is normalised before the redaction passes rather than after, because an address broken by a newline or a non-breaking space is still reconstructible by a reader. Inline HTML tags are deleted rather than replaced with a space for the same reason, and the tag pattern tracks quoted attributes so a > inside one cannot end the tag early. Personal names are not pattern-detectable and are not removed. --- app/Services/Ords/ProblemTextScrubber.php | 233 +++++++++++++++++ tests/Unit/Ords/ProblemTextScrubberTest.php | 271 ++++++++++++++++++++ 2 files changed, 504 insertions(+) create mode 100644 app/Services/Ords/ProblemTextScrubber.php create mode 100644 tests/Unit/Ords/ProblemTextScrubberTest.php diff --git a/app/Services/Ords/ProblemTextScrubber.php b/app/Services/Ords/ProblemTextScrubber.php new file mode 100644 index 0000000000..5a797cfdf3 --- /dev/null +++ b/app/Services/Ords/ProblemTextScrubber.php @@ -0,0 +1,233 @@ + '[email removed]', + self::PHONE => '[phone removed]', + self::LONG_DIGITS => '[number removed]', + ]; + + /** @var array */ + private array $counts = []; + + public function __construct() + { + $this->reset(); + } + + public function reset(): void + { + $this->counts = [ + self::HTML => 0, + self::URL_QUERY => 0, + self::EMAIL => 0, + self::PHONE => 0, + self::LONG_DIGITS => 0, + ]; + } + + /** @return array */ + public function counts(): array + { + return $this->counts; + } + + public function totalRedactions(): int + { + return array_sum($this->counts); + } + + public function scrub(?string $text): string + { + if ($text === null || trim($text) === '') { + return ''; + } + + $text = $this->stripHtml($text); + // Before the redaction passes, not after: "owner@\nexample.com" and + // "owner@\u{00A0}example.com" are reconstructible addresses, and the + // redaction patterns cannot see through the break while it is still there. + $text = $this->normaliseWhitespace($text); + // Must run before the digit/phone passes, or they chew through tracking params. + $text = $this->stripUrlQueryStrings($text); + $text = $this->redactEmails($text); + $text = $this->redactPhoneNumbers($text); + $text = $this->redactLongDigitRuns($text); + + // Again, because the placeholders above are inserted with their own spacing. + return $this->normaliseWhitespace($text); + } + + /** + * Block-level tags mark a real word boundary and become a space; inline + * tags do not and are deleted outright. Substituting a space for every tag + * would split "owner@example.com" into something no pattern matches + * but a reader can still reassemble. + * + * Not strip_tags(): it truncates after an unterminated "<". + */ + private const BLOCK_TAGS = [ + 'address', 'article', 'aside', 'blockquote', 'br', 'dd', 'div', 'dl', 'dt', + 'fieldset', 'figcaption', 'figure', 'footer', 'form', 'h1', 'h2', 'h3', 'h4', + 'h5', 'h6', 'header', 'hr', 'li', 'main', 'nav', 'ol', 'p', 'pre', 'section', + 'table', 'tbody', 'td', 'tfoot', 'th', 'thead', 'tr', 'ul', + ]; + + private function stripHtml(string $text): string + { + $decoded = html_entity_decode($text, ENT_QUOTES | ENT_HTML5, 'UTF-8'); + + $count = 0; + $result = preg_replace_callback( + // The attribute alternation matters: a bare [^>]* stops at the first + // ">" even when it sits inside a quoted attribute value, which ends + // the tag early and leaves the rest of the attribute as text. That + // turned owner@example.com into owner@">example.com, + // with the address intact. + '~<\s*/?\s*([a-z!][^\s>/]*)(?:"[^"]*"|\'[^\']*\'|[^\'">])*>~iu', + function (array $match) use (&$count) { + $count++; + + return in_array(strtolower($match[1]), self::BLOCK_TAGS, true) ? ' ' : ''; + }, + $decoded + ); + + return $this->record($result, $count, self::HTML); + } + + private function stripUrlQueryStrings(string $text): string + { + return $this->replace( + '~(https?://[^\s<>"\']+?)[?#][^\s<>"\']*~iu', + '$1', + $text, + self::URL_QUERY + ); + } + + /** + * Exclusion-based match, not an ASCII allow-list, so international addresses redact whole. + * + * Two passes. The strict one takes unbroken addresses. The tolerant one then + * takes addresses a single space still runs through, which survive because a + * volunteer wrapped a line or pasted from a client that inserted one. Its + * top-level domain excludes digits so it cannot swallow "cost 10 @ 2.50 each". + * Runs after normaliseWhitespace, so one optional space is enough. + */ + private function redactEmails(string $text): string + { + // Fullwidth (U+FF20)/small (U+FE6B) commercial-at also render as "@" and must be normalised first. + $normalised = str_replace(["\u{FF20}", "\u{FE6B}"], '@', $text); + + $strict = $this->replace( + '~[^\s@<>"\'()\[\],;:]+@[^\s@<>"\'()\[\],;:]+\.[^\s@<>"\'()\[\],;:.]{2,}~u', + self::PLACEHOLDERS[self::EMAIL], + $normalised, + self::EMAIL + ); + + return $this->replace( + '~[^\s@<>"\'()\[\],;:]+ ?@ ?[^\s@<>"\'()\[\],;:]+ ?\. ?[^\s@<>"\'()\[\],;:.\d]{2,}~u', + self::PLACEHOLDERS[self::EMAIL], + $strict, + self::EMAIL + ); + } + + /** + * Separator-bearing sequences only; bare runs go to the digit pass below, + * so an IMEI isn't mislabelled as a phone. Nine-digit floor: rpm ranges, + * part numbers, firmware versions and year ranges sit at 8 digits or + * fewer, while a dialable number needs 9+ once an area/country code is + * present. A 7-digit local number is missed by design -- cheaper than + * destroying ranges and part numbers. + */ + private function redactPhoneNumbers(string $text): string + { + $count = 0; + + $result = preg_replace_callback( + // Bounded on digits only: \b would let "phone-555-123-4567" through untouched. + '~(?record($result, $count, self::PHONE); + } + + private function redactLongDigitRuns(string $text): string + { + return $this->replace( + '~(?record($result, $count, $countKey); + } + + /** Null result = regex engine failed; must never leak the original, so text is dropped and the failure counted. */ + private function record(?string $result, int $count, string $countKey): string + { + if ($result === null) { + $this->counts[$countKey]++; + + return ''; + } + + $this->counts[$countKey] += $count; + + return $result; + } + + /** + * \p{Zs} as well as \s: under /u alone PCRE leaves U+00A0 and the other + * Unicode spaces out of \s, so a non-breaking space would survive and keep + * an address readable but unmatchable. + */ + private function normaliseWhitespace(string $text): string + { + return trim(preg_replace('/[\s\p{Zs}]+/u', ' ', $text) ?? $text); + } +} diff --git a/tests/Unit/Ords/ProblemTextScrubberTest.php b/tests/Unit/Ords/ProblemTextScrubberTest.php new file mode 100644 index 0000000000..75d2982bfd --- /dev/null +++ b/tests/Unit/Ords/ProblemTextScrubberTest.php @@ -0,0 +1,271 @@ +scrubber = new ProblemTextScrubber(); + } + + public function test_strips_html_tags(): void + { + // ORA's own published data contains

tags, and `devices.problem` has + // no Purify mutator so anything a volunteer pastes lands raw. + $this->assertEquals( + 'Screen flickers Replaced the inverter', + $this->scrubber->scrub('

Screen flickers

Replaced the inverter

') + ); + } + + public function test_strips_escaped_html_tags(): void + { + $this->assertEquals( + 'Broken hinge', + $this->scrubber->scrub('<b>Broken hinge</b>') + ); + } + + /** + * strip_tags() discards everything after an unterminated "<", and repair + * notes routinely compare against a threshold, a price or a value. + */ + public function test_keeps_text_containing_a_bare_less_than(): void + { + $this->assertEquals( + 'temp <100C and rising, unit dead', + $this->scrubber->scrub('temp <100C and rising, unit dead') + ); + + $this->assertEquals( + 'price was <5 pounds', + $this->scrubber->scrub('price was <5 pounds') + ); + + $this->assertEquals( + 'reads ascrubber->scrub('reads aassertEquals( + 'serviced on 2024-06-15 by the owner', + $this->scrubber->scrub('serviced on 2024-06-15 by the owner') + ); + $this->assertEquals( + 'logged 2024-06-15 14:30 at the bench', + $this->scrubber->scrub('logged 2024-06-15 14:30 at the bench') + ); + $this->assertEquals(0, $this->scrubber->counts()[ProblemTextScrubber::PHONE]); + } + + /** + * Widening the phone pattern to catch "phone-555-123-4567" also made it + * greedy enough to eat hyphenated numeric substance. The nine-digit floor + * is what holds these apart, so both sides of it are pinned here. + */ + public function test_leaves_hyphenated_numeric_substance_alone(): void + { + foreach ([ + 'spins at 1000-2000 rpm now', + 'firmware 1.2.3-4567 installed', + 'part no. 12-345-678 ordered', + 'fault seen 2023-2024 repeatedly', + 'drop from 240-110 volts', + ] as $input) { + $this->scrubber->reset(); + $this->assertEquals($input, $this->scrubber->scrub($input)); + $this->assertEquals(0, $this->scrubber->counts()[ProblemTextScrubber::PHONE], $input); + } + } + + /** + * The first two run onto a preceding word, which a word-character + * lookbehind used to let through completely unredacted. + */ + public function test_still_redacts_numbers_long_enough_to_dial(): void + { + foreach ([ + 'phone-555-123-4567' => 'phone-[phone removed]', + 'mob-07700-900123' => 'mob-[phone removed]', + 'call 555-123-4567' => 'call [phone removed]', + 'ring +1 (555) 123-4567' => 'ring [phone removed]', + 'owner 020 7946 0958' => 'owner [phone removed]', + ] as $input => $expected) { + $this->scrubber->reset(); + $this->assertEquals($expected, $this->scrubber->scrub($input)); + $this->assertEquals(1, $this->scrubber->counts()[ProblemTextScrubber::PHONE]); + } + } + + public function test_redacts_non_ascii_and_homoglyph_email_addresses(): void + { + // An ASCII-only pattern left the first two untouched or, worse, redacted + // only the ASCII tail and published the name fragment ahead of it. + $this->assertEquals('[email removed]', $this->scrubber->scrub('john@münchen.de')); + $this->assertEquals('[email removed]', $this->scrubber->scrub('josé.garcía@example.com')); + // Fullwidth commercial at (U+FF20). + $this->assertEquals('[email removed]', $this->scrubber->scrub("john\u{FF20}example.com")); + + $this->assertEquals(3, $this->scrubber->counts()[ProblemTextScrubber::EMAIL]); + } + + public function test_redacts_email_addresses(): void + { + $result = $this->scrubber->scrub('Owner is jane.doe+repairs@example.co.uk, will follow up'); + + $this->assertEquals('Owner is [email removed], will follow up', $result); + $this->assertEquals(1, $this->scrubber->counts()[ProblemTextScrubber::EMAIL]); + } + + /** + * Regression: every one of these exported a reconstructible address. The + * email pattern cannot cross whitespace, whitespace was only normalised + * after redaction had already run, and stripHtml substituted a space for + * every tag, which manufactured the break in the inline-tag case. + * + * @dataProvider brokenEmailProvider + */ + public function test_redacts_email_addresses_broken_by_whitespace_or_markup(string $problem): void + { + $result = $this->scrubber->scrub($problem); + + $this->assertStringNotContainsString('john.smith', $result); + $this->assertStringNotContainsString('example.com', $result); + $this->assertStringContainsString('[email removed]', $result); + } + + public static function brokenEmailProvider(): array + { + return [ + 'newline in address' => ["contact john.smith@\nexample.com"], + 'non-breaking space' => ["contact john.smith@\u{00A0}example.com"], + 'plain space' => ['contact john.smith@ example.com'], + 'inline tag inside address' => ['contact john.smith@example.com'], + 'tab before domain' => ["contact john.smith@\texample.com"], + // A ">" inside a quoted attribute used to end the tag early, so the + // rest of the attribute stayed as text and the address survived + // whole: this exported as john.smith@">example.com. + 'gt inside a double-quoted attribute' => ['contact john.smith@example.com'], + 'gt inside a single-quoted attribute' => ["contact john.smith@example.com"], + 'gt mid-attribute' => ['contact john.smith@example.com'], + ]; + } + + /** The tolerant pass must not treat prices, measurements or citations as addresses. */ + public function test_does_not_redact_at_signs_that_are_not_addresses(): void + { + foreach (['cost 10 @ 2.50 each', '5 @ 3 . 2 volts', 'see p . 4', 'met @ the cafe . nice'] as $kept) { + $this->assertEquals($kept, $this->scrubber->scrub($kept)); + } + + $this->assertEquals(0, $this->scrubber->counts()[ProblemTextScrubber::EMAIL]); + } + + /** Inline tags close up; block tags still mark a word boundary. */ + public function test_block_tags_separate_words_but_inline_tags_do_not(): void + { + $this->assertEquals('word one word two', $this->scrubber->scrub('

word one

word two

')); + $this->assertEquals('bold', $this->scrubber->scrub('bold')); + } + + public function test_strips_url_query_strings_but_keeps_the_bare_url(): void + { + // Modelled on published record fixitclinic_584, which carries a full + // affiliate URL with gclid and sfdr_ptcid tracking parameters. + $result = $this->scrubber->scrub( + 'Part at https://www.example.com/parts/motor?gclid=ABC123xyz&sfdr_ptcid=99887766 ordered' + ); + + $this->assertEquals('Part at https://www.example.com/parts/motor ordered', $result); + $this->assertEquals(1, $this->scrubber->counts()[ProblemTextScrubber::URL_QUERY]); + } + + public function test_strips_url_fragments(): void + { + $this->assertEquals( + 'See https://example.com/guide', + $this->scrubber->scrub('See https://example.com/guide#step-4-user-jane') + ); + } + + public function test_redacts_long_digit_runs(): void + { + $result = $this->scrubber->scrub('IMEI 356938035643809 does not match the label'); + + $this->assertEquals('IMEI [number removed] does not match the label', $result); + } + + public function test_leaves_short_numbers_alone(): void + { + // Model numbers, years, capacities and measurements are the substance + // of a repair note; over-redacting would gut the dataset. + $this->assertEquals( + 'Bosch WAE24166GB from 2011, 1200 rpm, blew a 13 amp fuse', + $this->scrubber->scrub('Bosch WAE24166GB from 2011, 1200 rpm, blew a 13 amp fuse') + ); + } + + public function test_handles_multiple_redaction_types_in_one_string(): void + { + $result = $this->scrubber->scrub( + '

Contact me@example.com or 555-123-4567.

Serial 123456789012. ' + .'Guide https://example.com/x?utm_source=email' + ); + + $this->assertStringNotContainsString('me@example.com', $result); + $this->assertStringNotContainsString('555-123-4567', $result); + $this->assertStringNotContainsString('123456789012', $result); + $this->assertStringNotContainsString('utm_source', $result); + $this->assertStringContainsString('https://example.com/x', $result); + + $counts = $this->scrubber->counts(); + $this->assertEquals(1, $counts[ProblemTextScrubber::EMAIL]); + $this->assertEquals(1, $counts[ProblemTextScrubber::PHONE]); + $this->assertEquals(1, $counts[ProblemTextScrubber::LONG_DIGITS]); + $this->assertEquals(1, $counts[ProblemTextScrubber::URL_QUERY]); + // Occurrences, like every other counter: the fixture opens and closes one

. + $this->assertEquals(2, $counts[ProblemTextScrubber::HTML]); + $this->assertEquals(6, $this->scrubber->totalRedactions()); + } + + public function test_counts_accumulate_across_records_until_reset(): void + { + $this->scrubber->scrub('a@example.com'); + $this->scrubber->scrub('b@example.com'); + + $this->assertEquals(2, $this->scrubber->counts()[ProblemTextScrubber::EMAIL]); + + $this->scrubber->reset(); + + $this->assertEquals(0, $this->scrubber->counts()[ProblemTextScrubber::EMAIL]); + $this->assertEquals(0, $this->scrubber->totalRedactions()); + } + + public function test_handles_null_and_blank_input(): void + { + $this->assertEquals('', $this->scrubber->scrub(null)); + $this->assertEquals('', $this->scrubber->scrub('')); + $this->assertEquals('', $this->scrubber->scrub(' ')); + $this->assertEquals(0, $this->scrubber->totalRedactions()); + } + + public function test_collapses_whitespace(): void + { + $this->assertEquals( + 'Fixed the switch', + $this->scrubber->scrub(" Fixed the\n\tswitch ") + ); + } +} From e759e905d26a8dd17f609fb74a965ec3411b5176 Mon Sep 17 00:00:00 2001 From: Angel de la Torre Date: Tue, 11 Aug 2026 10:51:18 -0700 Subject: [PATCH 3/8] feat(ords): add record mapper and vocabulary config Maps a device row onto the standard's fourteen columns in spec order. year_of_manufacture is not stored so it is derived from the event year minus the item's age, and product_category_id is a name lookup because our category ids do not match the standard's. The vocabulary follows the Open Repair Alliance's published data rather than its tableschema.json, which is stale in places. ORDS_ID_PREFIX and ORDS_DATA_PROVIDER have no defaults: the id is a stable key the consumer upserts on across releases, so a borrowed namespace would overwrite another provider's rows. --- app/Services/Ords/OrdsRecordMapper.php | 299 +++++++++++++++++++++++++ config/ords.php | 126 +++++++++++ 2 files changed, 425 insertions(+) create mode 100644 app/Services/Ords/OrdsRecordMapper.php create mode 100644 config/ords.php diff --git a/app/Services/Ords/OrdsRecordMapper.php b/app/Services/Ords/OrdsRecordMapper.php new file mode 100644 index 0000000000..b82c112426 --- /dev/null +++ b/app/Services/Ords/OrdsRecordMapper.php @@ -0,0 +1,299 @@ + */ + private readonly array $poweredCategories; + + /** @var array */ + private readonly array $unpoweredCategories; + + private readonly mixed $unpoweredFallback; + + /** @var array */ + private readonly array $repairStatuses; + + private readonly mixed $repairStatusUnknown; + + /** @var array */ + private readonly array $barrierVocabulary; + + /** + * Config is resolved once here rather than per row. map() runs up to 1000 + * times per request and each config() call copies a whole vocabulary array + * out by value to read one key. + */ + public function __construct(private readonly ProblemTextScrubber $scrubber) + { + $this->idPrefix = trim((string) config('ords.id_prefix')); + $this->dataProvider = config('ords.data_provider'); + $this->poweredCategories = config('ords.categories_powered'); + $this->unpoweredCategories = config('ords.categories_unpowered'); + $this->unpoweredFallback = config('ords.categories_unpowered_fallback'); + $this->repairStatuses = config('ords.repair_status'); + $this->repairStatusUnknown = config('ords.repair_status_unknown'); + $this->barrierVocabulary = config('ords.barriers'); + } + + public function resetRedactions(): void + { + $this->scrubber->reset(); + } + + /** @return array */ + public function redactionCounts(): array + { + return $this->scrubber->counts(); + } + + /** @return array keyed in ORDS column order */ + public function map(Device $device): array + { + $eventDate = $this->eventDate($device); + $productAge = $this->productAge($device); + [$productCategory, $productCategoryId] = $this->productCategory($device); + + return [ + // Trimmed to match the controller's guard: untrimmed, " ifixit_" would + // pass validation and emit ids with a leading space. + 'id' => $this->idPrefix.$device->iddevices, + 'data_provider' => $this->dataProvider, + 'country' => Iso3166::alpha3($device->ords_country_code), + 'partner_product_category' => $this->partnerProductCategory($device), + 'product_category' => $productCategory, + 'product_category_id' => $productCategoryId, + 'brand' => $this->scrubbed($device->brand), + 'year_of_manufacture' => $this->yearOfManufacture($eventDate, $productAge), + 'product_age' => $productAge, + 'repair_status' => $this->repairStatus($device), + 'repair_barrier_if_end_of_life' => $this->repairBarrier($device), + 'group_identifier' => $this->nullIfBlank($device->ords_group_name), + 'event_date' => $eventDate?->toDateString(), + 'problem' => $this->problem($device), + ]; + } + + /** + * " ~ " when an item type is present, bare category + * otherwise -- follows The Restart Project's own published row convention. + */ + private function partnerProductCategory(Device $device): ?string + { + // Category name comes from our own `categories` table and is not + // free text; item_type is typed by a volunteer at the event. + $category = $this->nullIfBlank($device->ords_category_name); + $itemType = $this->scrubbed($device->item_type); + + if ($category === null) { + return $itemType; + } + + return $itemType === null ? $category : "{$category} ~ {$itemType}"; + } + + /** @return array{0: ?string, 1: ?int} [product_category, product_category_id] */ + private function productCategory(Device $device): array + { + $name = $this->nullIfBlank($device->ords_category_name); + + if (! $device->ords_category_powered) { + // ORA's unpowered dataset carries no product_category_id. + return [ + $this->unpoweredCategories[$name] ?? $this->unpoweredFallback, + null, + ]; + } + + // Unmapped powered category is a vocabulary gap, not a data error: fall + // back to our own name with a null id rather than dropping the record. + return $this->poweredCategories[$name] ?? [$name, null]; + } + + /** + * `devices.age` is DECIMAL(5,2) UNSIGNED ZEROFILL NOT NULL DEFAULT 0, so + * MySQL returns it zero-padded ("005.00") and 0 means "not recorded", not + * a real age. is_numeric also covers instances still on the old free-text + * VARCHAR column. + */ + private function productAge(Device $device): int|float|null + { + $age = $device->age; + + if ($age === null || ! is_numeric($age)) { + return null; + } + + $age = (float) $age; + + if ($age <= 0) { + return null; + } + + return $age == (int) $age ? (int) $age : $age; + } + + /** + * Not stored, so derived: the year the event ran minus the item's age. + * ORDS wants a 4-digit string; anything we cannot derive is omitted. + */ + private function yearOfManufacture(?Carbon $eventDate, int|float|null $productAge): ?string + { + if ($eventDate === null || $productAge === null) { + return null; + } + + $year = (int) round($eventDate->year - $productAge); + + // ORDS constrains this to ^[0-9]{4}$; clamp rather than emit a rejected value. + if ($year < 1000 || $year > 9999) { + return null; + } + + return (string) $year; + } + + private function eventDate(Device $device): ?Carbon + { + $startUtc = $device->ords_event_start_utc; + + if (empty($startUtc)) { + return null; + } + + $date = Carbon::parse($startUtc, 'UTC'); + $timezone = $this->nullIfBlank($device->ords_event_timezone); + + if ($timezone !== null) { + try { + $date = $date->setTimezone($timezone); + } catch (\Throwable) { + // Unrecognised timezone: keep UTC rather than drop a required column. + } + } + + return $date; + } + + private function repairStatus(Device $device): string + { + return $this->repairStatuses[$device->repair_status] + ?? $this->repairStatusUnknown; + } + + /** Devices can carry several barriers; ORDS has one column, so the first wins. */ + private function repairBarrier(Device $device): ?string + { + if ((int) $device->repair_status !== Device::REPAIR_STATUS_ENDOFLIFE) { + return null; + } + + $barrier = $device->barriers->first(); + + if ($barrier === null) { + return null; + } + + return $this->barrierVocabulary[$barrier->barrier] ?? null; + } + + /** + * Always emitted and always scrubbed. The scrubber removes contact details + * and identifiers but cannot remove personal names, which are not + * pattern-detectable; see ProblemTextScrubber. + */ + private function problem(Device $device): ?string + { + return $this->scrubbed($device->problem); + } + + /** + * Every volunteer-written column goes through here. `problem` is the + * obvious one, but `item_type` and `brand` are typed by hand at an event + * with no vocabulary behind them, and owner-identifying text turns up in + * both ("Jane's kettle"). All three are published, so all three redact. + */ + private function scrubbed(?string $value): ?string + { + return $this->nullIfBlank($this->scrubber->scrub($value)); + } + + private function nullIfBlank(?string $value): ?string + { + if ($value === null) { + return null; + } + + $trimmed = trim($value); + + return $trimmed === '' ? null : $trimmed; + } +} diff --git a/config/ords.php b/config/ords.php new file mode 100644 index 0000000000..01590b323e --- /dev/null +++ b/config/ords.php @@ -0,0 +1,126 @@ + env('ORDS_ID_PREFIX', OrdsRecordMapper::UNASSIGNED_ID_PREFIX), + + // Organisation name emitted on every record. Undefaulted for the same + // reason as the prefix: a blank credits the data to nobody. + 'data_provider' => env('ORDS_DATA_PROVIDER', ''), + + // Bulk export, so a higher ceiling than the interactive v2 endpoints. + 'pagination' => [ + 'default_per_page' => 100, + 'max_per_page' => 1000, + ], + + // Deliberately spelled out rather than reusing Device::REPAIR_STATUS_*_STR: + // these are the standard's strings, not ours, and they must not follow ours + // if we ever reword them. + // + // Unknown is a real value in the published data, not a blank, and is what + // the 0 default on `devices.repair_status` maps to. + 'repair_status' => [ + Device::REPAIR_STATUS_FIXED => 'Fixed', + Device::REPAIR_STATUS_REPAIRABLE => 'Repairable', + Device::REPAIR_STATUS_ENDOFLIFE => 'End of life', + ], + + 'repair_status_unknown' => 'Unknown', + + // The five barriers seeded by 2018_11_12_135805_additional_device_fields. + // They match the published vocabulary except for the "the" in "No way to + // open the product". A barrier seeded without a mapping here exports as + // blank, which OrdsRepairsApiTest asserts against. + 'barriers' => [ + 'Spare parts not available' => 'Spare parts not available', + 'Spare parts too expensive' => 'Spare parts too expensive', + 'No way to open the product' => 'No way to open product', + 'Repair information not available' => 'Repair information not available', + 'Lack of equipment' => 'Lack of equipment', + ], + + // Keyed on `categories.name` where `categories.powered` is true. + // + // Names mostly pass straight through, but the ids do not line up (our + // "Desktop computer" is idcategories 11, product_category_id 4) and the + // standard collapses our screen-size and laptop-size splits into one + // category each, which is confirmed against The Restart Project's own + // published rows. + 'categories_powered' => [ + 'Desktop computer' => ['Desktop computer', 4], + 'Flat screen 15-17"' => ['Flat screen', 8], + 'Flat screen 19-20"' => ['Flat screen', 8], + 'Flat screen 22-24"' => ['Flat screen', 8], + 'Flat screen 26-30"' => ['Flat screen', 8], + 'Flat screen 32-37"' => ['Flat screen', 8], + 'Laptop large' => ['Laptop', 16], + 'Laptop medium' => ['Laptop', 16], + 'Laptop small' => ['Laptop', 16], + 'Paper shredder' => ['Paper shredder', 21], + 'PC accessory' => ['PC accessory', 22], + 'Printer/scanner' => ['Printer/scanner', 25], + 'Digital compact camera' => ['Digital compact camera', 5], + 'DSLR/video camera' => ['DSLR/video camera', 6], + 'Handheld entertainment device' => ['Handheld entertainment device', 10], + 'Headphones' => ['Headphones', 11], + 'Mobile' => ['Mobile', 19], + 'Tablet' => ['Tablet', 30], + 'Hi-Fi integrated' => ['Hi-Fi integrated', 12], + 'Hi-Fi separates' => ['Hi-Fi separates', 13], + 'Musical instrument' => ['Musical instrument', 20], + 'Portable radio' => ['Portable radio', 23], + 'Projector' => ['Projector', 26], + 'TV and gaming-related accessories' => ['TV and gaming-related accessories', 33], + 'Aircon/dehumidifier' => ['Aircon/dehumidifier', 1], + 'Decorative or safety lights' => ['Decorative or safety lights', 3], + 'Fan' => ['Fan', 7], + 'Hair & beauty item' => ['Hair & beauty item', 9], + 'Kettle' => ['Kettle', 14], + 'Lamp' => ['Lamp', 15], + 'Power tool' => ['Power tool', 24], + 'Small kitchen item' => ['Small kitchen item', 29], + 'Toaster' => ['Toaster', 31], + 'Toy' => ['Toy', 32], + 'Vacuum' => ['Vacuum', 34], + 'Misc' => ['Misc', 18], + + // Added by 2021_08_13_000439_update_lca_unpowered_categories. + 'Games console' => ['Games console', 38], + 'Watch/clock' => ['Watch/clock', 35], + 'Sewing machine' => ['Sewing machine', 27], + 'Iron' => ['Iron', 40], + 'Coffee maker' => ['Coffee maker', 36], + ], + + // Keyed on `categories.name` where `categories.powered` is false. Unpowered + // repairs are published as a separate four-column dataset with no + // product_category_id, so these carry the "Unpowered - X" name and a null + // id. Callers wanting the 14-column aggregate shape should pass powered=1. + 'categories_unpowered' => [ + 'Furniture' => 'Unpowered - Furniture', + 'Bicycle' => 'Unpowered - Bicycle', + 'Clothing/textile' => 'Unpowered - Textile', + 'Jewellery' => 'Unpowered - Jewellery', + 'Misc' => 'Unpowered - Other', + // No published equivalent; "Household" is the nearest but overstates it. + 'Hand tool' => 'Unpowered - Other', + ], + + 'categories_unpowered_fallback' => 'Unpowered - Other', + +]; From a6359d957ddf6692e92afe1a0e9a427401271de3 Mon Sep 17 00:00:00 2001 From: Angel de la Torre Date: Tue, 11 Aug 2026 10:51:18 -0700 Subject: [PATCH 4/8] feat(api): add ORDS v0.3 repair export endpoint GET /api/v2/repairs, behind auth:api alongside the other external-consumer endpoints. Serves JSON or CSV with updated_since, event_start, event_end, powered, page and per_page filters. Only approved events on approved groups are visible and soft-deleted events are excluded. The per-page ceiling is 1000 rather than the interactive endpoints' 100 because this is a bulk export. Returns 503 until the id namespace and data provider are configured. --- .env.example | 6 + app/Http/Controllers/API/RepairController.php | 371 ++++++++++++++++++ routes/api.php | 4 + 3 files changed, 381 insertions(+) create mode 100644 app/Http/Controllers/API/RepairController.php diff --git a/.env.example b/.env.example index 49d7bf8be2..40c4dda1ba 100644 --- a/.env.example +++ b/.env.example @@ -107,3 +107,9 @@ HONEYPOT_DISABLE= L5_SWAGGER_GENERATE_ALWAYS=true SUPPORT_EMAIL_ADDRESS="support@your.site" + +# Open Repair Data Standard export. No defaults; the endpoint returns 503 +# until both are set. See config/ords.php for why. Include the separator: +# ORDS_ID_PREFIX=restarters_ produces ids like restarters_12345. +ORDS_ID_PREFIX= +ORDS_DATA_PROVIDER= diff --git a/app/Http/Controllers/API/RepairController.php b/app/Http/Controllers/API/RepairController.php new file mode 100644 index 0000000000..80317f0ae7 --- /dev/null +++ b/app/Http/Controllers/API/RepairController.php @@ -0,0 +1,371 @@ +guardExportConfig()) { + return $guard; + } + + $this->normalisePoweredInput($request); + + $validated = $request->validate([ + 'format' => ['nullable', 'string', 'in:json,csv'], + 'updated_since' => ['nullable', 'date'], + 'event_start' => ['nullable', 'date'], + 'event_end' => ['nullable', 'date'], + 'powered' => ['nullable', 'boolean'], + 'page' => ['nullable', 'integer', 'min:1'], + 'per_page' => ['nullable', 'integer', 'min:1', 'max:'.(int) config('ords.pagination.max_per_page')], + ]); + + $query = $this->buildBaseRepairQuery(); + $this->applyFilters($query, $validated); + + // Formatted once here rather than in each responder: CSV and JSON carry + // the same watermark, they just carry it in different places. + $rawMaxUpdatedAt = (clone $query)->max('devices.updated_at'); + $maxUpdatedAt = $rawMaxUpdatedAt ? Carbon::parse($rawMaxUpdatedAt)->toIso8601String() : null; + + $perPage = (int) ($validated['per_page'] ?? config('ords.pagination.default_per_page')); + $paginator = $query->paginate($perPage); + + $this->mapper->resetRedactions(); + $records = $paginator->getCollection() + ->map(fn (Device $device) => $this->mapper->map($device)) + ->values(); + + $this->logExport($request, $validated, $paginator); + + if (($validated['format'] ?? 'json') === 'csv') { + return $this->csvResponse($records->all(), $paginator, $maxUpdatedAt); + } + + return $this->jsonResponse($records->all(), $paginator, $maxUpdatedAt); + } + + /** Laravel's `boolean` rule rejects "true"/"false"; normalise those before validation. */ + private function normalisePoweredInput(Request $request): void + { + if (! $request->has('powered')) { + return; + } + + $raw = $request->input('powered'); + + // filter_var maps "" and null to false rather than firing + // FILTER_NULL_ON_FAILURE, so an empty `?powered=` would narrow the + // export to unpowered items. Null instead, which applyFilters skips. + if ($raw === null || $raw === '') { + $request->merge(['powered' => null]); + + return; + } + + $normalised = filter_var($raw, FILTER_VALIDATE_BOOL, FILTER_NULL_ON_FAILURE); + + if ($normalised !== null) { + $request->merge(['powered' => $normalised]); + } + } + + /** + * `id_prefix` and `data_provider` are instance-specific with no safe + * default. Both are checked for emptiness, not just presence, because an + * env var set to "" yields an empty string rather than falling back to + * any default. + */ + private function guardExportConfig(): ?JsonResponse + { + $prefix = trim((string) config('ords.id_prefix')); + + if ($prefix === '' || $prefix === OrdsRecordMapper::UNASSIGNED_ID_PREFIX) { + return response()->json([ + 'message' => 'ORDS export is not configured: no partner id namespace has been assigned.', + ], 503); + } + + if (trim((string) config('ords.data_provider')) === '') { + return response()->json([ + 'message' => 'ORDS export is not configured: no data provider name has been set.', + ], 503); + } + + return null; + } + + private function jsonResponse(array $records, LengthAwarePaginator $paginator, ?string $maxUpdatedAt): JsonResponse + { + return response()->json([ + 'data' => $records, + 'meta' => [ + 'page' => $paginator->currentPage(), + 'per_page' => $paginator->perPage(), + 'total' => $paginator->total(), + 'last_page' => $paginator->lastPage(), + 'standard' => OrdsRecordMapper::STANDARD, + 'columns' => OrdsRecordMapper::COLUMNS, + ], + 'sync' => [ + 'generated_at' => Carbon::now()->toIso8601String(), + 'max_updated_at' => $maxUpdatedAt, + ], + ]); + } + + /** No envelope for pagination/sync metadata in CSV, so it travels in headers instead. */ + private function csvResponse(array $records, LengthAwarePaginator $paginator, ?string $maxUpdatedAt): StreamedResponse + { + return response()->streamDownload(function () use ($records) { + $csv = Writer::createFromStream(fopen('php://output', 'w')); + // `problem` is free text; a cell opening with = + - @ executes as a formula on open. + $csv->addFormatter([new EscapeFormula(), 'escapeRecord']); + $csv->insertOne(OrdsRecordMapper::COLUMNS); + + foreach ($records as $record) { + // ORDS uses an empty string for every missing value. + $csv->insertOne(array_map( + fn ($value) => $value === null ? '' : $value, + $record + )); + } + }, 'ords-repairs.csv', [ + 'Content-Type' => 'text/csv; charset=UTF-8', + 'X-Total-Count' => (string) $paginator->total(), + 'X-Page' => (string) $paginator->currentPage(), + 'X-Per-Page' => (string) $paginator->perPage(), + 'X-Last-Page' => (string) $paginator->lastPage(), + 'X-Max-Updated-At' => $maxUpdatedAt ?? '', + ]); + } + + private function buildBaseRepairQuery(): Builder + { + return Device::query() + ->join('events', 'events.idevents', '=', 'devices.event') + ->join('groups', 'groups.idgroups', '=', 'events.group') + ->join('categories', 'categories.idcategories', '=', 'devices.category') + // Only events are soft-deletable; a removed group takes its rows + // out of the inner join. + ->whereNull('events.deleted_at') + ->where('events.approved', true) + ->where('groups.approved', true) + // Ordered: the mapper emits only the first barrier, and an unordered + // relation would let a device publish a different one between exports. + ->with(['barriers' => fn ($q) => $q->orderBy('barriers.id')]) + // Pinned, not devices.*: the exported surface should be a property + // of this query rather than an accident of what the mapper happens + // to read. It also keeps the mediumText `notes` column, which the + // export never uses, out of a 1000-row hydration. + ->select( + 'devices.iddevices', + 'devices.brand', + 'devices.item_type', + 'devices.age', + 'devices.repair_status', + 'devices.problem', + 'devices.updated_at', + 'categories.name as ords_category_name', + 'categories.powered as ords_category_powered', + 'groups.name as ords_group_name', + 'groups.country_code as ords_country_code', + 'events.event_start_utc as ords_event_start_utc', + 'events.timezone as ords_event_timezone', + ) + // Stable ordering so pagination can't skip or repeat rows between pages. + ->orderBy('devices.iddevices', 'asc'); + } + + private function applyFilters(Builder $query, array $validated): void + { + if (! empty($validated['updated_since'])) { + $updatedSince = Carbon::parse($validated['updated_since'])->setTimezone('UTC')->toDateTimeString(); + $query->where('devices.updated_at', '>=', $updatedSince); + } + + if (! empty($validated['event_start'])) { + // toDateTimeString, not toIso8601String, and the same for the end + // bound below: `events.event_start_utc` is a plain DATETIME and the + // connection sets no session timezone, so MySQL 8.0.19+ reads a + // trailing "+00:00" as an offset and shifts the bound into the + // server's timezone. `updated_since` already binds this way. + $start = Carbon::parse($validated['event_start'])->setTimezone('UTC')->toDateTimeString(); + $query->where('events.event_start_utc', '>=', $start); + } + + if (! empty($validated['event_end'])) { + $end = Carbon::parse($validated['event_end']); + + // A date-only bound reads as "include that day"; left at 00:00 it'd exclude it entirely. + if ($end->format('H:i:s') === '00:00:00') { + $end = $end->endOfDay(); + } + + $query->where('events.event_start_utc', '<=', $end->setTimezone('UTC')->toDateTimeString()); + } + + // Unfiltered by default: ORA publishes powered/unpowered as separate datasets. + if (array_key_exists('powered', $validated) && $validated['powered'] !== null) { + $query->where('categories.powered', (bool) $validated['powered']); + } + } + + /** + * Logged on every export, not only when something was redacted. This is a + * bulk export of volunteer-written text reachable by any account with a + * token, so who pulled how much and when is the only record available + * afterwards for a subject-access request or an abuse report. The redaction + * counters carry no text, only tallies. + * + * @param array $validated + */ + private function logExport(Request $request, array $validated, LengthAwarePaginator $paginator): void + { + Log::info('ORDS export served', [ + 'user_id' => $request->user()?->id, + 'format' => $validated['format'] ?? 'json', + 'page' => $paginator->currentPage(), + 'per_page' => $paginator->perPage(), + 'records' => $paginator->count(), + 'total' => $paginator->total(), + 'filters' => array_intersect_key($validated, array_flip([ + 'updated_since', 'event_start', 'event_end', 'powered', + ])), + 'redactions' => $this->mapper->redactionCounts(), + ]); + } +} diff --git a/routes/api.php b/routes/api.php index 5564ad0dcc..35f716a05d 100644 --- a/routes/api.php +++ b/routes/api.php @@ -129,5 +129,9 @@ Route::patch('{id}', [API\DeviceController::class, 'updateDevicev2']); Route::delete('{id}', [API\DeviceController::class, 'deleteDevicev2']); }); + + // Open Repair Data Standard export. Returns 503 until ORDS_ID_PREFIX and + // ORDS_DATA_PROVIDER are set; see config/ords.php for why they have no defaults. + Route::get('/repairs', [API\RepairController::class, 'listRepairsv2'])->middleware('auth:api'); }); }); \ No newline at end of file From 0947a7a17477a46895b777c77027e2a73dfe7adb Mon Sep 17 00:00:00 2001 From: Angel de la Torre Date: Tue, 11 Aug 2026 10:51:25 -0700 Subject: [PATCH 5/8] test(ords): add repair export feature tests Covers the config guard, the fourteen columns and their mapping, the barrier and category vocabularies, visibility rules, filters, pagination, CSV output and problem-text redaction. Unauthenticated requests are asserted as refused rather than as 401, because this codebase renders any JSON exception without a status code as a 500 and that behaviour is not this endpoint's to change. --- tests/Feature/Ords/OrdsRepairsApiTest.php | 840 ++++++++++++++++++++++ 1 file changed, 840 insertions(+) create mode 100644 tests/Feature/Ords/OrdsRepairsApiTest.php diff --git a/tests/Feature/Ords/OrdsRepairsApiTest.php b/tests/Feature/Ords/OrdsRepairsApiTest.php new file mode 100644 index 0000000000..163971fb08 --- /dev/null +++ b/tests/Feature/Ords/OrdsRepairsApiTest.php @@ -0,0 +1,840 @@ + 'testinstance_', + 'ords.data_provider' => 'Test Repair Org', + ]); + } + + // ---------------------------------------------------------------- auth + + /** + * Note: Handler::render returns 500 for any JSON exception that carries no + * status code, and AuthenticationException is one, so an unauthenticated + * call to any endpoint on this API surfaces as a 500 rather than a 401. + * These tests assert that the request is refused rather than asserting the + * status, so they cover the auth gate without baking in that pre-existing + * behaviour. + */ + public function test_requires_authentication(): void + { + $this->withExceptionHandling(); + + $response = $this->getJson('/api/v2/repairs'); + + $this->assertFalse($response->isSuccessful()); + $this->assertSame('Unauthenticated.', $response->json('message')); + } + + public function test_rejects_an_unknown_token(): void + { + $this->withExceptionHandling(); + + $response = $this->getJson('/api/v2/repairs?api_token=not_a_real_token'); + + $this->assertFalse($response->isSuccessful()); + $this->assertSame('Unauthenticated.', $response->json('message')); + } + + // ------------------------------------------------------ id namespace + + public function test_refuses_to_serve_under_an_unassigned_id_namespace(): void + { + // ORDS ids are a stable key the Open Repair Alliance updates in place + // across releases, so publishing under a namespace we were not assigned + // would overwrite another provider's rows. + config(['ords.id_prefix' => OrdsRecordMapper::UNASSIGNED_ID_PREFIX]); + + $this->seedRepair(); + + $response = $this->getJson($this->url()); + + $response->assertStatus(503); + $this->assertStringContainsString('id namespace', $response->json('message')); + } + + public function test_refuses_to_serve_under_a_blank_id_namespace(): void + { + // An env var set to "" yields an empty string rather than falling back + // to the config default, so a deployment shipping ORDS_ID_PREFIX="" must + // not be mistaken for a configured export. + config(['ords.id_prefix' => '']); + + $this->seedRepair(); + + $this->getJson($this->url())->assertStatus(503); + } + + public function test_refuses_to_serve_without_a_data_provider(): void + { + // data_provider is the attribution carried on every published row. + config(['ords.data_provider' => '']); + + $this->seedRepair(); + + $response = $this->getJson($this->url()); + + $response->assertStatus(503); + $this->assertStringContainsString('data provider', $response->json('message')); + } + + public function test_emits_ids_under_the_assigned_namespace(): void + { + config(['ords.id_prefix' => 'restarters_']); + + $device = $this->seedRepair(); + + $this->assertEquals( + 'restarters_'.$device->iddevices, + $this->fetchRecords()[0]['id'] + ); + } + + public function test_a_padded_id_prefix_is_trimmed_before_emission(): void + { + // The config guard validates the trimmed prefix, so the emitted id has + // to be trimmed too or " restarters_ " passes the guard and publishes + // ids carrying a leading space. + config(['ords.id_prefix' => ' restarters_ ']); + + $device = $this->seedRepair(); + + $this->assertSame('restarters_'.$device->iddevices, $this->fetchRecords()[0]['id']); + } + + // ------------------------------------------------------------ columns + + public function test_emits_the_fourteen_ords_columns_in_spec_order(): void + { + $this->seedRepair(); + + $this->assertEquals([ + 'id', + 'data_provider', + 'country', + 'partner_product_category', + 'product_category', + 'product_category_id', + 'brand', + 'year_of_manufacture', + 'product_age', + 'repair_status', + 'repair_barrier_if_end_of_life', + 'group_identifier', + 'event_date', + 'problem', + ], array_keys($this->fetchRecords()[0])); + } + + public function test_maps_every_column_from_the_fixture(): void + { + $device = $this->seedRepair([ + 'brand' => 'Acme', + 'item_type' => 'Tower PC', + 'age' => 5, + 'problem' => 'Would not power on. Replaced the PSU.', + 'repair_status' => Device::REPAIR_STATUS_FIXED, + ]); + + $record = $this->fetchRecords()[0]; + + $this->assertEquals('testinstance_'.$device->iddevices, $record['id']); + $this->assertEquals('Test Repair Org', $record['data_provider']); + // groups.country_code is alpha-2; ORDS requires alpha-3. + $this->assertEquals('GBR', $record['country']); + // The Restart Project's own published rows use " ~ ". + $this->assertEquals('Desktop computer ~ Tower PC', $record['partner_product_category']); + $this->assertEquals('Desktop computer', $record['product_category']); + // Our idcategories is 11; the standard publishes Desktop computer as 4. + $this->assertEquals(4, $record['product_category_id']); + $this->assertEquals('Acme', $record['brand']); + // 2024 event minus a 5 year old item. + $this->assertEquals('2019', $record['year_of_manufacture']); + $this->assertEquals(5, $record['product_age']); + $this->assertEquals('Fixed', $record['repair_status']); + $this->assertNull($record['repair_barrier_if_end_of_life']); + $this->assertEquals('ORDS Group', $record['group_identifier']); + $this->assertEquals('2024-06-15', $record['event_date']); + $this->assertEquals('Would not power on. Replaced the PSU.', $record['problem']); + } + + public function test_event_date_uses_the_events_local_timezone(): void + { + // 2024-06-16 06:00 UTC is still 2024-06-15 in Los Angeles. + $this->seedRepair([], ['start_utc' => '2024-06-16 06:00:00', 'timezone' => 'America/Los_Angeles']); + + $this->assertEquals('2024-06-15', $this->fetchRecords()[0]['event_date']); + } + + public function test_repair_status_falls_back_to_unknown(): void + { + // `devices.repair_status` is NOT NULL DEFAULT 0, and ORDS carries + // Unknown as a real enum value rather than a blank. + $this->seedRepair(['repair_status' => 0]); + + $this->assertEquals('Unknown', $this->fetchRecords()[0]['repair_status']); + } + + public function test_year_of_manufacture_and_age_are_omitted_when_age_is_not_recorded(): void + { + // `devices.age` is DECIMAL NOT NULL DEFAULT 0, so 0 is how "we did not + // record an age" is stored rather than a real age of zero. + $this->seedRepair(['age' => 0]); + + $record = $this->fetchRecords()[0]; + $this->assertNull($record['year_of_manufacture']); + $this->assertNull($record['product_age']); + } + + public function test_fractional_age_is_preserved(): void + { + $this->seedRepair(['age' => 2.5]); + + $record = $this->fetchRecords()[0]; + $this->assertEquals(2.5, $record['product_age']); + // 2024 minus 2.5 years, rounded to a whole year. + $this->assertEquals('2022', $record['year_of_manufacture']); + } + + // ----------------------------------------------------------- barriers + + public function test_barrier_is_emitted_only_for_end_of_life_and_uses_ords_wording(): void + { + // We seed "No way to open the product"; the standard publishes it + // without the "the". + $barrier = Barrier::where('barrier', 'No way to open the product')->firstOrFail(); + + $device = $this->seedRepair(['repair_status' => Device::REPAIR_STATUS_ENDOFLIFE]); + $device->barriers()->attach($barrier->id); + + $this->assertEquals( + 'No way to open product', + $this->fetchRecords()[0]['repair_barrier_if_end_of_life'] + ); + } + + public function test_barrier_choice_is_stable_across_exports(): void + { + // ORDS has one barrier column but a device can carry several, so the + // mapper takes the first. The record id is a stable key the consumer + // upserts on, so an unordered relation would republish a different + // barrier at random. + $first = Barrier::where('barrier', 'Lack of equipment')->firstOrFail(); + $second = Barrier::where('barrier', 'Spare parts too expensive')->firstOrFail(); + [$lower, $higher] = $first->id < $second->id ? [$first, $second] : [$second, $first]; + + $device = $this->seedRepair(['repair_status' => Device::REPAIR_STATUS_ENDOFLIFE]); + // Attached highest-first so insertion order cannot be what makes this pass. + $device->barriers()->attach($higher->id); + $device->barriers()->attach($lower->id); + + // Local literals, not config('ords.barriers')[...]: deriving the + // expectation from the same map the mapper reads would leave only the + // barrier-choice half of this assertion load-bearing. + $vocabulary = [ + 'Lack of equipment' => 'Lack of equipment', + 'Spare parts too expensive' => 'Spare parts too expensive', + ]; + + $this->assertEquals( + $vocabulary[$lower->barrier], + $this->fetchRecords()[0]['repair_barrier_if_end_of_life'] + ); + } + + public function test_an_unmapped_barrier_is_omitted(): void + { + // A barrier outside the ORDS vocabulary is a mapping gap; emitting our + // own wording would put an invalid value in a constrained column. + $barrier = Barrier::create(['barrier' => 'Not an ORDS barrier']); + + try { + $device = $this->seedRepair(['repair_status' => Device::REPAIR_STATUS_ENDOFLIFE]); + $device->barriers()->attach($barrier->id); + + $this->assertNull($this->fetchRecords()[0]['repair_barrier_if_end_of_life']); + } finally { + // TestCase::setUp does not truncate `barriers`, so without this the + // row survives into test_every_seeded_barrier_has_a_vocabulary_mapping + // and fails it. In finally, or a failure here becomes two failures. + // Barrier's inverse relation is itself named barriers(); see App\Barrier. + $barrier->barriers()->detach(); + $barrier->delete(); + } + } + + public function test_barrier_is_suppressed_when_the_item_was_repaired(): void + { + $barrier = Barrier::where('barrier', 'Lack of equipment')->firstOrFail(); + + $device = $this->seedRepair(['repair_status' => Device::REPAIR_STATUS_FIXED]); + $device->barriers()->attach($barrier->id); + + $this->assertNull($this->fetchRecords()[0]['repair_barrier_if_end_of_life']); + } + + public function test_every_seeded_barrier_has_a_vocabulary_mapping(): void + { + // An unmapped barrier exports as null rather than as our own wording, + // so a barrier added to the seed without a mapping degrades quietly. + $map = config('ords.barriers'); + + foreach (Barrier::all() as $barrier) { + $this->assertArrayHasKey( + $barrier->barrier, + $map, + "seeded barrier \"{$barrier->barrier}\" has no ORDS mapping" + ); + } + } + + // --------------------------------------------------------- categories + + public function test_unpowered_categories_use_the_ords_unpowered_vocabulary(): void + { + $this->seedCategory(501, 'Clothing/textile', false); + + $this->seedRepair(['category' => 501, 'category_creation' => 501, 'item_type' => null]); + + $record = $this->fetchRecords()[0]; + $this->assertEquals('Unpowered - Textile', $record['product_category']); + // The standard's unpowered dataset carries no product_category_id. + $this->assertNull($record['product_category_id']); + $this->assertEquals('Clothing/textile', $record['partner_product_category']); + } + + public function test_an_unmapped_powered_category_falls_back_to_our_own_name(): void + { + // A category outside the ORDS vocabulary is a mapping gap, not a data + // error. The record stays usable and the gap is visible in the export. + $this->seedCategory(504, '3D printer', true); + + $this->seedRepair(['category' => 504, 'category_creation' => 504, 'item_type' => null]); + + $record = $this->fetchRecords()[0]; + $this->assertEquals('3D printer', $record['product_category']); + $this->assertNull($record['product_category_id']); + $this->assertEquals('3D printer', $record['partner_product_category']); + } + + /** + * ORDS requires alpha-3. `groups.country_code` is alpha-2 and nullable, and + * nothing constrains it to a real code. Split per case: asserted together, + * the unmappable-code half passes on the null fixture's leftover row. + */ + public function test_an_unmapped_unpowered_category_uses_the_generic_fallback(): void + { + // The reason categories_unpowered_fallback exists: an unpowered category + // outside the standard's vocabulary still has to land somewhere valid. + $this->seedCategory(506, 'Unlisted unpowered thing', false); + + $this->seedRepair(['category' => 506, 'category_creation' => 506, 'item_type' => null]); + + $record = $this->fetchRecords()[0]; + $this->assertEquals(config('ords.categories_unpowered_fallback'), $record['product_category']); + $this->assertNull($record['product_category_id']); + } + + public function test_year_of_manufacture_is_omitted_when_the_derived_year_is_implausible(): void + { + // ORDS constrains this to ^[0-9]{4}$, so a derived year outside 1000-9999 + // is dropped rather than emitted and rejected downstream. + // + // Reached via an old event date, not a huge age: `devices.age` is + // DECIMAL(5,2) so it tops out at 999.99, and a present-day event minus + // 999.99 years still lands in the 1000s. + $this->seedRepair(['age' => 999], ['start_utc' => '1990-06-15 18:00:00']); + + $record = $this->fetchRecords()[0]; + $this->assertNull($record['year_of_manufacture']); + $this->assertEquals(999, $record['product_age']); + } + + public function test_an_unrecognised_event_timezone_falls_back_to_utc(): void + { + // events.timezone is not constrained to a real zone. Rather than drop a + // required column, the mapper keeps the UTC date. + $this->seedRepair([], ['start_utc' => '2024-06-15 18:00:00', 'timezone' => 'Not/AZone']); + + $this->assertEquals('2024-06-15', $this->fetchRecords()[0]['event_date']); + } + + public function test_country_is_null_when_the_group_has_no_country_code(): void + { + $this->seedRepair([], ['country_code' => null]); + + $this->assertNull($this->fetchRecords()[0]['country']); + } + + public function test_country_is_null_when_the_code_is_not_a_real_country(): void + { + $this->seedRepair([], ['group' => 'Unknown Country Group', 'country_code' => 'ZZ']); + + $records = $this->fetchRecords(); + $this->assertCount(1, $records); + $this->assertNull($records[0]['country']); + } + + // ---------------------------------------------------------- visibility + + public function test_excludes_unapproved_events_unapproved_groups_and_deleted_events(): void + { + // Events are soft-deletable, groups are not: a removed group takes its + // rows out of the inner join, so only events need the explicit filter. + $visible = $this->seedRepair(); + + $unapprovedEventDevice = $this->seedRepair([], ['group' => 'Second Group', 'approve_event' => false]); + $unapprovedGroupDevice = $this->seedRepair([], ['group' => 'Third Group', 'approve_group' => false]); + + $deletedEventDevice = $this->seedRepair([], ['group' => 'Fourth Group']); + Party::findOrFail($deletedEventDevice->event)->delete(); + + $ids = array_column($this->fetchRecords(), 'id'); + + $this->assertContains('testinstance_'.$visible->iddevices, $ids); + $this->assertNotContains('testinstance_'.$unapprovedEventDevice->iddevices, $ids); + $this->assertNotContains('testinstance_'.$unapprovedGroupDevice->iddevices, $ids); + $this->assertNotContains('testinstance_'.$deletedEventDevice->iddevices, $ids); + } + + // ------------------------------------------------------------ filters + + public function test_powered_filter_selects_each_dataset(): void + { + $this->seedCategory(502, 'Bicycle', false); + + $powered = $this->seedRepair(); + $unpowered = $this->seedRepair(['category' => 502, 'category_creation' => 502], ['reuse' => true]); + + $poweredIds = array_column($this->fetchRecords(['powered' => 1]), 'id'); + $this->assertContains('testinstance_'.$powered->iddevices, $poweredIds); + $this->assertNotContains('testinstance_'.$unpowered->iddevices, $poweredIds); + + $unpoweredIds = array_column($this->fetchRecords(['powered' => 0]), 'id'); + $this->assertContains('testinstance_'.$unpowered->iddevices, $unpoweredIds); + $this->assertNotContains('testinstance_'.$powered->iddevices, $unpoweredIds); + + // Unfiltered returns both: the standard publishes them as separate + // datasets, but the endpoint does not silently drop half the data. + $this->assertCount(2, $this->fetchRecords()); + } + + public function test_powered_filter_accepts_true_and_false_spellings(): void + { + // Laravel's `boolean` rule takes only true/false/0/1, so `?powered=true` + // has to be normalised before validation or it is rejected outright. + $this->seedCategory(502, 'Bicycle', false); + + $powered = $this->seedRepair(); + $unpowered = $this->seedRepair(['category' => 502, 'category_creation' => 502], ['reuse' => true]); + + $trueIds = array_column($this->fetchRecords(['powered' => 'true']), 'id'); + $this->assertContains('testinstance_'.$powered->iddevices, $trueIds); + $this->assertNotContains('testinstance_'.$unpowered->iddevices, $trueIds); + + $falseIds = array_column($this->fetchRecords(['powered' => 'false']), 'id'); + $this->assertContains('testinstance_'.$unpowered->iddevices, $falseIds); + $this->assertNotContains('testinstance_'.$powered->iddevices, $falseIds); + } + + public function test_an_empty_powered_param_returns_both_datasets(): void + { + // An empty param is how plenty of clients spell an unset one. + $this->seedCategory(502, 'Bicycle', false); + + $powered = $this->seedRepair(); + $unpowered = $this->seedRepair(['category' => 502, 'category_creation' => 502], ['reuse' => true]); + + $ids = array_column($this->fetchRecords(['powered' => '']), 'id'); + + $this->assertContains('testinstance_'.$powered->iddevices, $ids); + $this->assertContains('testinstance_'.$unpowered->iddevices, $ids); + } + + public function test_a_date_only_event_end_includes_that_whole_day(): void + { + // The fixture event runs at 18:00 on 2024-06-15. A caller asking for a + // window ending on that date means to include it. + $this->seedRepair(); + + $this->assertCount(1, $this->fetchRecords(['event_end' => '2024-06-15'])); + $this->assertEmpty($this->fetchRecords(['event_end' => '2024-06-14'])); + } + + public function test_updated_since_filter(): void + { + $device = $this->seedRepair(); + $device->timestamps = false; + $device->updated_at = '2000-01-01 00:00:00'; + $device->save(); + + $this->assertEmpty($this->fetchRecords(['updated_since' => '2010-01-01T00:00:00+00:00'])); + $this->assertCount(1, $this->fetchRecords(['updated_since' => '1999-01-01T00:00:00+00:00'])); + } + + public function test_event_window_filters(): void + { + $this->seedRepair(); + + $this->assertCount(1, $this->fetchRecords([ + 'event_start' => '2024-01-01T00:00:00+00:00', + 'event_end' => '2024-12-31T00:00:00+00:00', + ])); + + $this->assertEmpty($this->fetchRecords(['event_start' => '2025-01-01T00:00:00+00:00'])); + $this->assertEmpty($this->fetchRecords(['event_end' => '2023-01-01T00:00:00+00:00'])); + } + + // --------------------------------------------------------- pagination + + public function test_per_page_ceiling_is_one_thousand(): void + { + $this->withExceptionHandling(); + $this->seedRepair(); + + $this->getJson($this->url(['per_page' => 1000])) + ->assertSuccessful() + ->assertJsonPath('meta.per_page', 1000); + + $this->getJson($this->url(['per_page' => 1001]))->assertStatus(422); + } + + public function test_pages_cover_every_record_exactly_once(): void + { + // A bulk consumer walks this endpoint page by page, so the pages must + // partition the result set: no row skipped, none served twice. + $devices = [ + $this->seedRepair(), + $this->seedRepair([], ['reuse' => true]), + $this->seedRepair([], ['reuse' => true]), + ]; + + $expected = array_map(fn (Device $d) => 'testinstance_'.$d->iddevices, $devices); + + $first = $this->getJson($this->url(['per_page' => 2])); + $first->assertSuccessful() + ->assertJsonPath('meta.total', 3) + ->assertJsonPath('meta.last_page', 2); + + $second = $this->getJson($this->url(['per_page' => 2, 'page' => 2])); + $second->assertSuccessful()->assertJsonPath('meta.page', 2); + + $firstIds = array_column($first->json('data'), 'id'); + $secondIds = array_column($second->json('data'), 'id'); + $all = array_merge($firstIds, $secondIds); + + $this->assertCount(2, $firstIds); + $this->assertCount(1, $secondIds); + $this->assertSame($all, array_unique($all)); + $this->assertEqualsCanonicalizing($expected, $all); + } + + public function test_rejects_an_unknown_format(): void + { + $this->withExceptionHandling(); + + $this->getJson($this->url(['format' => 'xml']))->assertStatus(422); + } + + public function test_reports_the_standard_and_columns_in_meta(): void + { + $this->seedRepair(); + + $this->getJson($this->url()) + ->assertSuccessful() + // Pinned as a literal: this is the standard version the record + // shape implements, so a change to the constant should fail here. + ->assertJsonPath('meta.standard', 'Open Repair Data Standard v0.3') + // Spelled out rather than compared to OrdsRecordMapper::COLUMNS, + // which the controller emits verbatim: that would assert a constant + // equals itself and survive any edit to it. + ->assertJsonPath('meta.columns', [ + 'id', 'data_provider', 'country', 'partner_product_category', + 'product_category', 'product_category_id', 'brand', + 'year_of_manufacture', 'product_age', 'repair_status', + 'repair_barrier_if_end_of_life', 'group_identifier', + 'event_date', 'problem', + ]) + ->assertJsonStructure(['sync' => ['generated_at', 'max_updated_at']]); + } + + public function test_response_matches_the_published_openapi_schema(): void + { + // TestCase::get validates every /api/v2 response against the generated + // schema, so this covers the annotation as well as the payload. + $this->seedRepair(); + + $this->get($this->url())->assertSuccessful(); + } + + // ---------------------------------------------------------------- CSV + + public function test_csv_output_matches_the_ords_column_order(): void + { + $device = $this->seedRepair([ + 'brand' => 'Acme', + 'item_type' => 'Tower PC', + 'age' => 5, + 'repair_status' => Device::REPAIR_STATUS_FIXED, + 'problem' => 'Would not power on.', + ]); + + $rows = $this->fetchCsvRows(); + + $this->assertEquals(OrdsRecordMapper::COLUMNS, $rows[0]); + $this->assertEquals([ + 'testinstance_'.$device->iddevices, + 'Test Repair Org', + 'GBR', + 'Desktop computer ~ Tower PC', + 'Desktop computer', + '4', + 'Acme', + '2019', + '5', + 'Fixed', + '', + 'ORDS Group', + '2024-06-15', + 'Would not power on.', + ], $rows[1]); + } + + public function test_csv_writes_empty_strings_for_missing_values(): void + { + // ORDS declares "" as the missing value for every optional column. + $this->seedRepair(['brand' => null, 'age' => 0, 'problem' => '']); + + $columns = array_combine(OrdsRecordMapper::COLUMNS, $this->fetchCsvRows()[1]); + + $this->assertSame('', $columns['brand']); + $this->assertSame('', $columns['year_of_manufacture']); + $this->assertSame('', $columns['product_age']); + $this->assertSame('', $columns['problem']); + $this->assertSame('', $columns['repair_barrier_if_end_of_life']); + } + + public function test_csv_neutralises_spreadsheet_formulas(): void + { + // `problem` is volunteer free text, and Excel and Sheets execute a cell + // opening with = + - @ as a formula the moment the file is opened. + $this->seedRepair(['problem' => '=HYPERLINK("http://example.com","click")']); + + $columns = array_combine(OrdsRecordMapper::COLUMNS, $this->fetchCsvRows()[1]); + + $this->assertSame( + '\'=HYPERLINK("http://example.com","click")', + $columns['problem'] + ); + } + + public function test_csv_carries_the_pagination_metadata_in_headers(): void + { + // CSV has no envelope for `meta`/`sync`, so without these a bulk + // consumer cannot tell that a second page exists. + $this->seedRepair(); + $this->seedRepair([], ['reuse' => true]); + $this->seedRepair([], ['reuse' => true]); + + $response = $this->fetchCsvResponse(['per_page' => 2]); + + // Without this the endpoint could serve CSV bytes labelled as JSON and + // every other CSV assertion here would still pass. + $response->assertHeader('Content-Type', 'text/csv; charset=UTF-8'); + $this->assertSame('3', $response->headers->get('X-Total-Count')); + $this->assertSame('1', $response->headers->get('X-Page')); + $this->assertSame('2', $response->headers->get('X-Per-Page')); + $this->assertSame('2', $response->headers->get('X-Last-Page')); + $this->assertNotEmpty($response->headers->get('X-Max-Updated-At')); + } + + // ---------------------------------------------------------- redaction + + public function test_problem_text_is_scrubbed(): void + { + $this->seedRepair([ + 'problem' => '

Owner jane@example.com, call 020 7946 0958.

' + .'Serial 123456789012. Part https://example.com/p?gclid=ABC123', + ]); + + $problem = $this->fetchRecords()[0]['problem']; + + $this->assertStringNotContainsString('jane@example.com', $problem); + $this->assertStringNotContainsString('020 7946 0958', $problem); + $this->assertStringNotContainsString('123456789012', $problem); + $this->assertStringNotContainsString('gclid', $problem); + $this->assertStringNotContainsString('

', $problem); + $this->assertStringContainsString('https://example.com/p', $problem); + } + + public function test_problem_is_always_exported_and_always_scrubbed(): void + { + // There is no toggle: the column ships on every export and the scrubber + // always runs. Personal names are not pattern-detectable and are not + // removed, which is a known limit of the redaction rather than a bug. + $this->seedRepair(['problem' => 'Reported by jane@example.com, screen cracked']); + + $problem = $this->fetchRecords()[0]['problem']; + + $this->assertNotNull($problem); + $this->assertStringNotContainsString('jane@example.com', $problem); + $this->assertStringContainsString('screen cracked', $problem); + } + + // ------------------------------------------------------------ helpers + + /** @param array $query */ + private function url(array $query = []): string + { + return '/api/v2/repairs?'.http_build_query( + array_merge(['api_token' => $this->apiToken()], $query) + ); + } + + /** + * CategoryFactory::definition() is empty, so every column is supplied here. + * TestCase::setUp truncates `categories`, so the id only has to avoid the + * six it re-seeds. + */ + private function seedCategory(int $id, string $name, bool $powered): void + { + Category::factory()->create([ + 'idcategories' => $id, + 'name' => $name, + 'revision' => 2, + 'aggregate' => 0, + 'powered' => $powered ? 1 : 0, + ]); + } + + private function apiToken(): string + { + if ($this->apiToken === null) { + $this->apiToken = User::factory()->create()->ensureAPIToken(); + } + + return $this->apiToken; + } + + /** + * @param array $deviceAttributes + * @param array $context + */ + private function seedRepair(array $deviceAttributes = [], array $context = []): Device + { + if (! empty($context['reuse'])) { + $eventId = Party::query()->orderBy('idevents', 'desc')->firstOrFail()->idevents; + } else { + // Groups and events are built straight from the factories rather + // than through TestCase::createGroup/createEvent, which post to the + // v2 API. This fixture needs an exact country_code, event start and + // timezone anyway, so the round trip would only be overwritten. + $group = Group::factory()->create([ + 'name' => $context['group'] ?? 'ORDS Group', + // array_key_exists, not ??, so a test can ask for a null code. + 'country_code' => array_key_exists('country_code', $context) ? $context['country_code'] : 'GB', + 'approved' => $context['approve_group'] ?? true, + 'latitude' => 51.5074, + 'longitude' => -0.1278, + 'location' => 'London', + ]); + + $startUtc = $context['start_utc'] ?? self::EVENT_START_UTC; + + $event = Party::factory()->create([ + 'group' => $group->idgroups, + 'approved' => $context['approve_event'] ?? true, + 'event_start_utc' => $startUtc, + 'event_end_utc' => Carbon::parse($startUtc)->addHours(2)->toDateTimeString(), + 'timezone' => $context['timezone'] ?? 'Europe/London', + ]); + + $eventId = $event->idevents; + } + + return Device::create(array_merge([ + 'event' => $eventId, + // 11 is "Desktop computer", seeded by TestCase::setUp. + 'category' => 11, + 'category_creation' => 11, + 'brand' => 'Acme', + 'item_type' => 'Tower PC', + 'age' => 5, + 'problem' => 'Would not power on.', + 'repair_status' => Device::REPAIR_STATUS_FIXED, + ], $deviceAttributes)); + } + + /** + * @param array $query + * @return array> + */ + private function fetchRecords(array $query = []): array + { + $response = $this->getJson($this->url($query)); + $response->assertSuccessful(); + + return $response->json('data'); + } + + /** + * Not TestCase::get, which would try to validate the CSV body against the + * OpenAPI schema, and not getJson, which would claim an Accept this request + * does not want. The route is selected by the `format` param either way. + * + * @param array $query + */ + private function fetchCsvResponse(array $query = []): \Illuminate\Testing\TestResponse + { + $response = $this->call('GET', $this->url(array_merge(['format' => 'csv'], $query))); + $response->assertSuccessful(); + + return $response; + } + + /** @return array> */ + private function fetchCsvRows(): array + { + $response = $this->fetchCsvResponse(); + + $handle = fopen('php://temp', 'r+'); + fwrite($handle, trim($response->streamedContent())); + rewind($handle); + + $rows = []; + while (($row = fgetcsv($handle, 0, ',', '"', '')) !== false) { + $rows[] = $row; + } + fclose($handle); + + return $rows; + } +} From ed667c5251dc807af477a7f86c4b260467773604 Mon Sep 17 00:00:00 2001 From: edwh Date: Thu, 20 Aug 2026 16:32:45 +0100 Subject: [PATCH 6/8] Gate the ORDS export on Administrator, and close four redaction/filter gaps Every other endpoint in the auth:api group either requires Administrator (the three Zapier bulk exports) or scopes its result set to the caller's own networks (Repair Together). This one returned every device from every approved group to any authenticated caller, and EnsureAPIToken mints a forever token for every logged-in user, so that was the whole site. Gated the same way UserController::changes is, message string included; the middleware stays auth:api, which was already correct. ProblemTextScrubber: - The phone candidate class admitted only space, parentheses, dot and hyphen, so "020/7946/0958" and "020,7946,0958" were published verbatim - each group is too short for the bare-digit pass to catch. Added "/" and ",", and extended the date guard to day-first and slash/dot separated forms so dates are still held back. - Typographic dashes and fullwidth digits render as a readable number that the ASCII-bounded patterns could not see. Folded to ASCII up front, in the same place the fullwidth commercial-at was already handled. - The strict email pass had no digit exclusion, so "cost 10@2.50 each" matched as an address and the price was destroyed. Guarded on the candidate containing a letter instead, which also keeps "user@192.168.1.10" redacting - a non-numeric TLD rule would not have. - Query-string stripping required an explicit scheme, so a schemeless "www.example.com/x?gclid=..." kept its tracking parameters. The bare-host arm requires a path segment so prose is not truncated at a question mark. RepairController: - event_end decided "date-only" from the parsed value's local H:i:s, which reads 00:00:00 for a full timestamp landing on midnight in its own offset. event_end=2024-06-15T00:00:00-05:00 means 05:00 UTC, and widening it to end-of-day admitted a further 24 hours of events. Decided from the raw input now. - Filter parsing passes 'UTC' explicitly, as the mapper already did, so a bare bound no longer depends on config('app.timezone') being UTC. OrdsRecordMapper: - id_prefix and data_provider are read and normalised in one place, so the guard and the emitted value cannot drift. Non-strings count as unset: ORDS_ID_PREFIX=true reaches config() as a boolean and "(string) true" is "1", which used to pass the guard and publish under a one-character namespace. "false", "null" and "empty" already failed closed. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01JfrVZbbtaC2hLoFh7T3qFs --- app/Http/Controllers/API/RepairController.php | 46 ++++-- app/Services/Ords/OrdsRecordMapper.php | 35 ++++- app/Services/Ords/ProblemTextScrubber.php | 113 ++++++++++++-- routes/api.php | 8 +- tests/Feature/Ords/OrdsRepairsApiTest.php | 68 ++++++++- tests/Unit/Ords/ProblemTextScrubberTest.php | 142 +++++++++++++++++- 6 files changed, 377 insertions(+), 35 deletions(-) diff --git a/app/Http/Controllers/API/RepairController.php b/app/Http/Controllers/API/RepairController.php index 80317f0ae7..a8b5231260 100644 --- a/app/Http/Controllers/API/RepairController.php +++ b/app/Http/Controllers/API/RepairController.php @@ -35,10 +35,10 @@ public function __construct(private readonly OrdsRecordMapper $mapper) * operationId="listRepairsv2", * tags={"Repairs"}, * summary="Export repair records in Open Repair Data Standard v0.3", - * description="Bulk export of repair records for the Open Repair Alliance. Covers approved events on approved groups only. Returns JSON by default, or CSV when format=csv.", + * description="Bulk export of repair records for the Open Repair Alliance. Covers approved events on approved groups only. Returns JSON by default, or CSV when format=csv. Administrator role required.", * @OA\Parameter( * name="api_token", - * description="A valid user API token", + * description="A valid user API token. The caller must hold the Administrator role.", * required=true, * in="query", * @OA\Schema(type="string") @@ -127,6 +127,14 @@ public function __construct(private readonly OrdsRecordMapper $mapper) * ) * ), * @OA\Response( + * response=401, + * description="Unauthenticated", + * ), + * @OA\Response( + * response=403, + * description="Forbidden - the authenticated user is not an Administrator", + * ), + * @OA\Response( * response=503, * description="Export is not configured: no id namespace or data provider name has been set", * @OA\JsonContent( @@ -137,6 +145,12 @@ public function __construct(private readonly OrdsRecordMapper $mapper) */ public function listRepairsv2(Request $request): Response { + // Checked before the config guard so the export's configuration state + // is not readable by an account that cannot use the export anyway. + if (! $request->user()->hasRole('Administrator')) { + return abort(403, 'The authenticated user is not authorized to access this resource'); + } + if ($guard = $this->guardExportConfig()) { return $guard; } @@ -207,11 +221,12 @@ private function normalisePoweredInput(Request $request): void * `id_prefix` and `data_provider` are instance-specific with no safe * default. Both are checked for emptiness, not just presence, because an * env var set to "" yields an empty string rather than falling back to - * any default. + * any default. Normalisation lives on the mapper so the value checked here + * is exactly the value emitted on every row. */ private function guardExportConfig(): ?JsonResponse { - $prefix = trim((string) config('ords.id_prefix')); + $prefix = OrdsRecordMapper::configuredIdPrefix(); if ($prefix === '' || $prefix === OrdsRecordMapper::UNASSIGNED_ID_PREFIX) { return response()->json([ @@ -219,7 +234,7 @@ private function guardExportConfig(): ?JsonResponse ], 503); } - if (trim((string) config('ords.data_provider')) === '') { + if (OrdsRecordMapper::configuredDataProvider() === '') { return response()->json([ 'message' => 'ORDS export is not configured: no data provider name has been set.', ], 503); @@ -313,7 +328,10 @@ private function buildBaseRepairQuery(): Builder private function applyFilters(Builder $query, array $validated): void { if (! empty($validated['updated_since'])) { - $updatedSince = Carbon::parse($validated['updated_since'])->setTimezone('UTC')->toDateTimeString(); + // 'UTC' passed explicitly, as OrdsRecordMapper::eventDate does: an + // offset in the input still wins, but a bare value no longer depends + // on config('app.timezone') happening to be UTC. + $updatedSince = Carbon::parse($validated['updated_since'], 'UTC')->setTimezone('UTC')->toDateTimeString(); $query->where('devices.updated_at', '>=', $updatedSince); } @@ -323,15 +341,21 @@ private function applyFilters(Builder $query, array $validated): void // connection sets no session timezone, so MySQL 8.0.19+ reads a // trailing "+00:00" as an offset and shifts the bound into the // server's timezone. `updated_since` already binds this way. - $start = Carbon::parse($validated['event_start'])->setTimezone('UTC')->toDateTimeString(); + $start = Carbon::parse($validated['event_start'], 'UTC')->setTimezone('UTC')->toDateTimeString(); $query->where('events.event_start_utc', '>=', $start); } if (! empty($validated['event_end'])) { - $end = Carbon::parse($validated['event_end']); - - // A date-only bound reads as "include that day"; left at 00:00 it'd exclude it entirely. - if ($end->format('H:i:s') === '00:00:00') { + $raw = trim((string) $validated['event_end']); + $end = Carbon::parse($raw, 'UTC'); + + // A date-only bound reads as "include that day"; left at 00:00 it'd + // exclude it entirely. Decided from the raw input rather than from + // the parsed value's local H:i:s, which reads "00:00:00" for a full + // timestamp that happens to land on midnight in its own offset: + // event_end=2024-06-15T00:00:00-05:00 means 05:00 UTC, and widening + // that to end-of-day admitted a further 24 hours of events. + if (! preg_match('~\d:\d~', $raw)) { $end = $end->endOfDay(); } diff --git a/app/Services/Ords/OrdsRecordMapper.php b/app/Services/Ords/OrdsRecordMapper.php index b82c112426..128b3280d1 100644 --- a/app/Services/Ords/OrdsRecordMapper.php +++ b/app/Services/Ords/OrdsRecordMapper.php @@ -69,7 +69,7 @@ class OrdsRecordMapper private readonly string $idPrefix; - private readonly mixed $dataProvider; + private readonly string $dataProvider; /** @var array */ private readonly array $poweredCategories; @@ -94,8 +94,8 @@ class OrdsRecordMapper */ public function __construct(private readonly ProblemTextScrubber $scrubber) { - $this->idPrefix = trim((string) config('ords.id_prefix')); - $this->dataProvider = config('ords.data_provider'); + $this->idPrefix = self::configuredIdPrefix(); + $this->dataProvider = self::configuredDataProvider(); $this->poweredCategories = config('ords.categories_powered'); $this->unpoweredCategories = config('ords.categories_unpowered'); $this->unpoweredFallback = config('ords.categories_unpowered_fallback'); @@ -104,6 +104,30 @@ public function __construct(private readonly ProblemTextScrubber $scrubber) $this->barrierVocabulary = config('ords.barriers'); } + /** + * The one place either setting is read and normalised, so the controller's + * guard and the value emitted here cannot drift apart. + * + * A non-string is treated as unset. Laravel's env() casts the literal + * "true" in a .env file to a boolean, and "(string) true" is "1", so + * ORDS_ID_PREFIX=true would otherwise sail past the guard and publish + * every record under a one-character namespace. "false", "null" and + * "empty" already fail closed; only "true" did not. + */ + public static function configuredIdPrefix(): string + { + $prefix = config('ords.id_prefix'); + + return is_string($prefix) ? trim($prefix) : ''; + } + + public static function configuredDataProvider(): string + { + $provider = config('ords.data_provider'); + + return is_string($provider) ? trim($provider) : ''; + } + public function resetRedactions(): void { $this->scrubber->reset(); @@ -123,8 +147,9 @@ public function map(Device $device): array [$productCategory, $productCategoryId] = $this->productCategory($device); return [ - // Trimmed to match the controller's guard: untrimmed, " ifixit_" would - // pass validation and emit ids with a leading space. + // Normalised by the same helper the controller's guard uses, so + // " restarters_ " cannot pass validation and then emit ids with a + // leading space. 'id' => $this->idPrefix.$device->iddevices, 'data_provider' => $this->dataProvider, 'country' => Iso3166::alpha3($device->ords_country_code), diff --git a/app/Services/Ords/ProblemTextScrubber.php b/app/Services/Ords/ProblemTextScrubber.php index 5a797cfdf3..e945031b7a 100644 --- a/app/Services/Ords/ProblemTextScrubber.php +++ b/app/Services/Ords/ProblemTextScrubber.php @@ -61,6 +61,10 @@ public function scrub(?string $text): string // "owner@\u{00A0}example.com" are reconstructible addresses, and the // redaction patterns cannot see through the break while it is still there. $text = $this->normaliseWhitespace($text); + // Also before the redaction passes: the patterns below are bounded on + // ASCII digits and separators, so a fullwidth digit or a typographic + // dash leaves a number perfectly readable but unmatchable. + $text = $this->normaliseConfusables($text); // Must run before the digit/phone passes, or they chew through tracking params. $text = $this->stripUrlQueryStrings($text); $text = $this->redactEmails($text); @@ -109,10 +113,19 @@ function (array $match) use (&$count) { return $this->record($result, $count, self::HTML); } + /** + * Strips the query string and fragment, which is where tracking and + * identifying parameters live. + * + * Three shapes, because a volunteer rarely types the scheme: an explicit + * http(s):// URL, a "www." host, and a bare "host.tld/path". The last + * requires a path segment so ordinary prose ("did you mean file.txt?") is + * not mistaken for a URL and truncated mid-sentence. + */ private function stripUrlQueryStrings(string $text): string { return $this->replace( - '~(https?://[^\s<>"\']+?)[?#][^\s<>"\']*~iu', + '~((?:https?://|www\.)[^\s<>"\']+?|(?:[a-z0-9-]+\.)+[a-z]{2,}/[^\s<>"\']*?)[?#][^\s<>"\']*~iu', '$1', $text, self::URL_QUERY @@ -126,20 +139,36 @@ private function stripUrlQueryStrings(string $text): string * takes addresses a single space still runs through, which survive because a * volunteer wrapped a line or pasted from a client that inserted one. Its * top-level domain excludes digits so it cannot swallow "cost 10 @ 2.50 each". - * Runs after normaliseWhitespace, so one optional space is enough. + * Runs after normaliseConfusables and normaliseWhitespace, so the fullwidth + * commercial-at is already an "@" and one optional space is enough. + * + * The strict pass carries the same protection differently: it is guarded on + * the candidate containing a letter rather than on a non-numeric top-level + * domain, because excluding digits there would also stop "user@192.168.1.10" + * redacting. Without the guard "cost 10@2.50 each" matched as an address. */ private function redactEmails(string $text): string { - // Fullwidth (U+FF20)/small (U+FE6B) commercial-at also render as "@" and must be normalised first. - $normalised = str_replace(["\u{FF20}", "\u{FE6B}"], '@', $text); + $strictCount = 0; - $strict = $this->replace( + $strict = preg_replace_callback( '~[^\s@<>"\'()\[\],;:]+@[^\s@<>"\'()\[\],;:]+\.[^\s@<>"\'()\[\],;:.]{2,}~u', - self::PLACEHOLDERS[self::EMAIL], - $normalised, - self::EMAIL + function (array $match) use (&$strictCount) { + // No letter anywhere means a numeric expression ("10@2.50"), + // not an address; a real local part or domain always has one. + if (! preg_match('~\p{L}~u', $match[0])) { + return $match[0]; + } + + $strictCount++; + + return self::PLACEHOLDERS[self::EMAIL]; + }, + $text ); + $strict = $this->record($strict, $strictCount, self::EMAIL); + return $this->replace( '~[^\s@<>"\'()\[\],;:]+ ?@ ?[^\s@<>"\'()\[\],;:]+ ?\. ?[^\s@<>"\'()\[\],;:.\d]{2,}~u', self::PLACEHOLDERS[self::EMAIL], @@ -149,8 +178,15 @@ private function redactEmails(string $text): string } /** - * Separator-bearing sequences only; bare runs go to the digit pass below, - * so an IMEI isn't mislabelled as a phone. Nine-digit floor: rpm ranges, + * Separator-bearing sequences only; bare runs go to the digit pass below. + * The separator set includes "/" and "," as well as space, parentheses, dot + * and hyphen: "020/7946/0958" is a standard continental format, and without + * them a number written that way was published verbatim -- each group is too + * short for the bare-digit pass to catch. One consequence is that a + * slash-grouped serial now redacts as a phone rather than a number; the + * label is wrong but the digits are gone, which is the point. + * + * Nine-digit floor: rpm ranges, * part numbers, firmware versions and year ranges sit at 8 digits or * fewer, while a dialable number needs 9+ once an area/country code is * present. A 7-digit local number is missed by design -- cheaper than @@ -162,18 +198,21 @@ private function redactPhoneNumbers(string $text): string $result = preg_replace_callback( // Bounded on digits only: \b would let "phone-555-123-4567" through untouched. - '~(? '@', + "\u{FE6B}" => '@', + // Hyphen, figure/en/em dash, horizontal bar, minus, and small/fullwidth forms. + "\u{2010}" => '-', + "\u{2011}" => '-', + "\u{2012}" => '-', + "\u{2013}" => '-', + "\u{2014}" => '-', + "\u{2015}" => '-', + "\u{2212}" => '-', + "\u{FE58}" => '-', + "\u{FE63}" => '-', + "\u{FF0D}" => '-', + // Separators that appear in dialling and serial formats. + "\u{FF0F}" => '/', + "\u{FF0C}" => ',', + "\u{FF0E}" => '.', + // Fullwidth digits. + "\u{FF10}" => '0', + "\u{FF11}" => '1', + "\u{FF12}" => '2', + "\u{FF13}" => '3', + "\u{FF14}" => '4', + "\u{FF15}" => '5', + "\u{FF16}" => '6', + "\u{FF17}" => '7', + "\u{FF18}" => '8', + "\u{FF19}" => '9', + ]; + + private function normaliseConfusables(string $text): string + { + return strtr($text, self::CONFUSABLES); + } + /** * \p{Zs} as well as \s: under /u alone PCRE leaves U+00A0 and the other * Unicode spaces out of \s, so a non-breaking space would survive and keep diff --git a/routes/api.php b/routes/api.php index 35f716a05d..6c1a5cd737 100644 --- a/routes/api.php +++ b/routes/api.php @@ -130,8 +130,12 @@ Route::delete('{id}', [API\DeviceController::class, 'deleteDevicev2']); }); - // Open Repair Data Standard export. Returns 503 until ORDS_ID_PREFIX and - // ORDS_DATA_PROVIDER are set; see config/ords.php for why they have no defaults. + // Open Repair Data Standard export. Administrator only, gated in the + // controller the same way UserController::changes is - this is a bulk + // export of volunteer-written text across every approved group, and + // auth:api alone only establishes who the caller is, not whether they + // should have it. Returns 503 until ORDS_ID_PREFIX and + // ORDS_DATA_PROVIDER are set; see config/ords.php for why. Route::get('/repairs', [API\RepairController::class, 'listRepairsv2'])->middleware('auth:api'); }); }); \ No newline at end of file diff --git a/tests/Feature/Ords/OrdsRepairsApiTest.php b/tests/Feature/Ords/OrdsRepairsApiTest.php index 163971fb08..4e3103b285 100644 --- a/tests/Feature/Ords/OrdsRepairsApiTest.php +++ b/tests/Feature/Ords/OrdsRepairsApiTest.php @@ -64,6 +64,22 @@ public function test_rejects_an_unknown_token(): void $this->assertSame('Unauthenticated.', $response->json('message')); } + public function test_refuses_a_caller_who_is_not_an_administrator(): void + { + // This is a bulk export of volunteer-written text across every approved + // group, so it is gated the same way the other global exports are. + $this->withExceptionHandling(); + + $this->seedRepair(); + + foreach (['restarter', 'host', 'networkCoordinator'] as $role) { + $token = User::factory()->{$role}()->create()->ensureAPIToken(); + + $this->getJson('/api/v2/repairs?'.http_build_query(['api_token' => $token])) + ->assertStatus(403); + } + } + // ------------------------------------------------------ id namespace public function test_refuses_to_serve_under_an_unassigned_id_namespace(): void @@ -93,6 +109,28 @@ public function test_refuses_to_serve_under_a_blank_id_namespace(): void $this->getJson($this->url())->assertStatus(503); } + public function test_refuses_a_prefix_that_env_casting_turned_into_a_boolean(): void + { + // ORDS_ID_PREFIX=true in a .env file reaches config() as boolean true, + // and "(string) true" is "1", which used to pass the guard and publish + // every record under a one-character namespace. "false", "null" and + // "empty" already failed closed; only "true" did not. + config(['ords.id_prefix' => true]); + + $this->seedRepair(); + + $this->getJson($this->url())->assertStatus(503); + } + + public function test_refuses_a_data_provider_that_env_casting_turned_into_a_boolean(): void + { + config(['ords.data_provider' => true]); + + $this->seedRepair(); + + $this->getJson($this->url())->assertStatus(503); + } + public function test_refuses_to_serve_without_a_data_provider(): void { // data_provider is the attribution carried on every published row. @@ -479,6 +517,28 @@ public function test_an_empty_powered_param_returns_both_datasets(): void $this->assertContains('testinstance_'.$unpowered->iddevices, $ids); } + /** + * Regression: "date-only" was decided from the parsed value's local + * H:i:s, which reads "00:00:00" for a full timestamp that lands on midnight + * in its own offset. event_end=2024-06-15T00:00:00-05:00 means 05:00 UTC, + * and widening it to end-of-day admitted a further 24 hours of events. + */ + public function test_a_midnight_event_end_carrying_an_offset_is_taken_as_given(): void + { + // Seeded event starts 2024-06-15 18:00:00 UTC. + $this->seedRepair(); + + // 2024-06-15T00:00:00-05:00 is 2024-06-15 05:00:00 UTC, before the event. + $this->assertEmpty($this->fetchRecords(['event_end' => '2024-06-15T00:00:00-05:00'])); + + // 2024-06-16T00:00:00-05:00 is 2024-06-16 05:00:00 UTC, after it. + $this->assertCount(1, $this->fetchRecords(['event_end' => '2024-06-16T00:00:00-05:00'])); + + // An explicit UTC midnight is likewise a boundary, not a whole day. + $this->assertEmpty($this->fetchRecords(['event_end' => '2024-06-15T00:00:00+00:00'])); + $this->assertEmpty($this->fetchRecords(['event_end' => '2024-06-15 00:00:00'])); + } + public function test_a_date_only_event_end_includes_that_whole_day(): void { // The fixture event runs at 18:00 on 2024-06-15. A caller asking for a @@ -735,10 +795,16 @@ private function seedCategory(int $id, string $name, bool $powered): void ]); } + /** + * An Administrator, because the export is gated on that role the same way + * UserController::changes is. Every other fixture in this file goes through + * here, so a regression in the gate fails the whole class rather than one + * test. + */ private function apiToken(): string { if ($this->apiToken === null) { - $this->apiToken = User::factory()->create()->ensureAPIToken(); + $this->apiToken = User::factory()->administrator()->create()->ensureAPIToken(); } return $this->apiToken; diff --git a/tests/Unit/Ords/ProblemTextScrubberTest.php b/tests/Unit/Ords/ProblemTextScrubberTest.php index 75d2982bfd..bf10a286d0 100644 --- a/tests/Unit/Ords/ProblemTextScrubberTest.php +++ b/tests/Unit/Ords/ProblemTextScrubberTest.php @@ -109,6 +109,70 @@ public function test_still_redacts_numbers_long_enough_to_dial(): void } } + /** + * Regression: every one of these was exported byte-for-byte. The candidate + * class only admitted space, parentheses, dot and hyphen, so a number + * grouped any other way fell to the bare-digit pass, which needs eight + * contiguous digits and never saw them either. + * + * @dataProvider separatedNumberProvider + */ + public function test_redacts_numbers_grouped_by_other_separators(string $problem): void + { + $result = $this->scrubber->scrub($problem); + + $this->assertStringNotContainsString('7946', $result); + $this->assertStringNotContainsString('0958', $result); + $this->assertStringContainsString('[phone removed]', $result); + $this->assertEquals(1, $this->scrubber->counts()[ProblemTextScrubber::PHONE]); + } + + public static function separatedNumberProvider(): array + { + return [ + // Standard continental grouping, and the reason this matters here: + // the instance serves fr-BE and the Repair Together integration. + 'slash' => ['owner on 020/7946/0958 most days'], + 'comma' => ['ring 020,7946,0958 please'], + 'en dash' => ["call 020\u{2013}7946\u{2013}0958 today"], + 'em dash' => ["call 020\u{2014}7946\u{2014}0958 today"], + 'non-breaking hyphen' => ["call 020\u{2011}7946\u{2011}0958 today"], + 'minus sign' => ["call 020\u{2212}7946\u{2212}0958 today"], + 'mixed slash and space' => ['owner on 020/7946 0958 most days'], + ]; + } + + /** + * \d is ASCII-only under /u without PCRE_UCP, so fullwidth digits render as + * a perfectly readable number that no pattern could see. + */ + public function test_redacts_numbers_written_with_fullwidth_digits(): void + { + $result = $this->scrubber->scrub("call \u{FF10}\u{FF12}\u{FF10} 7946 0958 back"); + + $this->assertEquals('call [phone removed] back', $result); + $this->assertEquals(1, $this->scrubber->counts()[ProblemTextScrubber::PHONE]); + } + + /** + * Admitting "/" and "," to the candidate class brings day-first and + * slash-separated dates within reach of the phone pattern, so the date + * guard has to cover them too. + */ + public function test_leaves_dates_in_other_separators_alone(): void + { + foreach ([ + 'logged 15/06/2024 14:30 at the bench', + 'logged 2024/06/15 14:30 at the bench', + 'logged 15.06.2024 14:30 at the bench', + 'logged 1/2/2024 09:15 at the bench', + ] as $input) { + $this->scrubber->reset(); + $this->assertEquals($input, $this->scrubber->scrub($input)); + $this->assertEquals(0, $this->scrubber->counts()[ProblemTextScrubber::PHONE], $input); + } + } + public function test_redacts_non_ascii_and_homoglyph_email_addresses(): void { // An ASCII-only pattern left the first two untouched or, worse, redacted @@ -166,11 +230,33 @@ public static function brokenEmailProvider(): array /** The tolerant pass must not treat prices, measurements or citations as addresses. */ public function test_does_not_redact_at_signs_that_are_not_addresses(): void { - foreach (['cost 10 @ 2.50 each', '5 @ 3 . 2 volts', 'see p . 4', 'met @ the cafe . nice'] as $kept) { + foreach ([ + 'cost 10 @ 2.50 each', + '5 @ 3 . 2 volts', + 'see p . 4', + 'met @ the cafe . nice', + // Regression: the strict pass had no digit exclusion, so the + // unspaced form matched as an address and the price was destroyed. + 'cost 10@2.50 each', + 'ratio 3@1.75 measured', + ] as $kept) { + $this->scrubber->reset(); $this->assertEquals($kept, $this->scrubber->scrub($kept)); + $this->assertEquals(0, $this->scrubber->counts()[ProblemTextScrubber::EMAIL], $kept); } + } - $this->assertEquals(0, $this->scrubber->counts()[ProblemTextScrubber::EMAIL]); + /** + * The guard that keeps "10@2.50" is "contains a letter", not "the top-level + * domain is non-numeric", precisely so this still redacts. + */ + public function test_redacts_an_address_at_a_bare_ip(): void + { + $this->assertEquals( + 'logs went to [email removed] overnight', + $this->scrubber->scrub('logs went to user@192.168.1.10 overnight') + ); + $this->assertEquals(1, $this->scrubber->counts()[ProblemTextScrubber::EMAIL]); } /** Inline tags close up; block tags still mark a word boundary. */ @@ -200,6 +286,58 @@ public function test_strips_url_fragments(): void ); } + /** + * Regression: the pattern required an explicit scheme, and a volunteer + * rarely types one, so the tracking parameters this pass exists to remove + * were published whole. + * + * @dataProvider schemelessUrlProvider + */ + public function test_strips_query_strings_from_schemeless_urls(string $problem, string $expected): void + { + $this->assertEquals($expected, $this->scrubber->scrub($problem)); + $this->assertEquals(1, $this->scrubber->counts()[ProblemTextScrubber::URL_QUERY]); + } + + public static function schemelessUrlProvider(): array + { + return [ + 'www host' => [ + 'Part at www.example.com/parts/motor?gclid=ABC123 ordered', + 'Part at www.example.com/parts/motor ordered', + ], + 'bare host with a path' => [ + 'Part at example.com/parts/motor?gclid=ABC123 ordered', + 'Part at example.com/parts/motor ordered', + ], + 'www host with no path' => [ + 'Listed on www.example.co.uk?ref=jane ordered', + 'Listed on www.example.co.uk ordered', + ], + 'schemeless fragment' => [ + 'See www.example.com/guide#step-4-user-jane', + 'See www.example.com/guide', + ], + ]; + } + + /** + * The bare-host arm requires a path segment so that ordinary prose is not + * mistaken for a URL and truncated at the first question mark. + */ + public function test_leaves_prose_containing_a_question_mark_alone(): void + { + foreach ([ + 'did you mean file.txt?yes', + 'is it the M.2 SSD?', + 'replaced fuse. Still dead?', + ] as $kept) { + $this->scrubber->reset(); + $this->assertEquals($kept, $this->scrubber->scrub($kept)); + $this->assertEquals(0, $this->scrubber->counts()[ProblemTextScrubber::URL_QUERY], $kept); + } + } + public function test_redacts_long_digit_runs(): void { $result = $this->scrubber->scrub('IMEI 356938035643809 does not match the label'); From f9e2e84d08fde6f98909c2163aceb6b454319e73 Mon Sep 17 00:00:00 2001 From: edwh Date: Thu, 20 Aug 2026 16:44:42 +0100 Subject: [PATCH 7/8] Widen updated_since to the event and group rows Keyed on devices.updated_at alone, an incremental consumer never sees work that only touched a parent row. An event sits unapproved, its devices are filtered out of every export, and approving it makes them exportable without changing devices.updated_at - so they stay invisible for good. Filter and watermark both move to GREATEST(devices.updated_at, events.updated_at, groups.updated_at), so a consumer resuming from max_updated_at resumes on the same clock it was filtered on. All three columns are NOT NULL DEFAULT CURRENT_TIMESTAMP ON UPDATE CURRENT_TIMESTAMP (2018_05_10_085751_rename_modified_at), so there is nothing to maintain and no null handling needed. test_updated_since_filter now backdates all three rows rather than the device alone, because "unchanged since" means something wider than it did. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01JfrVZbbtaC2hLoFh7T3qFs --- app/Http/Controllers/API/RepairController.php | 25 +++++- tests/Feature/Ords/OrdsRepairsApiTest.php | 77 ++++++++++++++++++- 2 files changed, 96 insertions(+), 6 deletions(-) diff --git a/app/Http/Controllers/API/RepairController.php b/app/Http/Controllers/API/RepairController.php index a8b5231260..e311ef4e06 100644 --- a/app/Http/Controllers/API/RepairController.php +++ b/app/Http/Controllers/API/RepairController.php @@ -10,6 +10,7 @@ use Illuminate\Http\JsonResponse; use Illuminate\Http\Request; use Illuminate\Pagination\LengthAwarePaginator; +use Illuminate\Support\Facades\DB; use Illuminate\Support\Facades\Log; use League\Csv\EscapeFormula; use League\Csv\Writer; @@ -25,6 +26,24 @@ */ class RepairController extends Controller { + /** + * What "changed" means to an incremental consumer. + * + * Keying off `devices.updated_at` alone hides work that only ever touched + * a parent row: approving an old event, or approving the group it belongs + * to, makes devices exportable that every earlier pull filtered out, while + * their own updated_at stands still. Those devices would never appear in + * an incremental crawl again. + * + * All three columns are NOT NULL DEFAULT CURRENT_TIMESTAMP ON UPDATE + * CURRENT_TIMESTAMP (see 2018_05_10_085751_rename_modified_at), so GREATEST + * needs no null handling and the values maintain themselves. + * + * The filter and the watermark both use this, so a consumer that resumes + * from max_updated_at resumes from the same clock it was filtered on. + */ + private const CHANGED_AT = 'GREATEST(devices.updated_at, events.updated_at, groups.updated_at)'; + public function __construct(private readonly OrdsRecordMapper $mapper) { } @@ -52,7 +71,7 @@ public function __construct(private readonly OrdsRecordMapper $mapper) * ), * @OA\Parameter( * name="updated_since", - * description="Only records whose device row changed at or after this time", + * description="Only records whose device, event or group row changed at or after this time", * required=false, * in="query", * @OA\Schema(type="string", format="date-time") @@ -172,7 +191,7 @@ public function listRepairsv2(Request $request): Response // Formatted once here rather than in each responder: CSV and JSON carry // the same watermark, they just carry it in different places. - $rawMaxUpdatedAt = (clone $query)->max('devices.updated_at'); + $rawMaxUpdatedAt = (clone $query)->max(DB::raw(self::CHANGED_AT)); $maxUpdatedAt = $rawMaxUpdatedAt ? Carbon::parse($rawMaxUpdatedAt)->toIso8601String() : null; $perPage = (int) ($validated['per_page'] ?? config('ords.pagination.default_per_page')); @@ -332,7 +351,7 @@ private function applyFilters(Builder $query, array $validated): void // offset in the input still wins, but a bare value no longer depends // on config('app.timezone') happening to be UTC. $updatedSince = Carbon::parse($validated['updated_since'], 'UTC')->setTimezone('UTC')->toDateTimeString(); - $query->where('devices.updated_at', '>=', $updatedSince); + $query->whereRaw(self::CHANGED_AT.' >= ?', [$updatedSince]); } if (! empty($validated['event_start'])) { diff --git a/tests/Feature/Ords/OrdsRepairsApiTest.php b/tests/Feature/Ords/OrdsRepairsApiTest.php index 4e3103b285..a1a9b0e999 100644 --- a/tests/Feature/Ords/OrdsRepairsApiTest.php +++ b/tests/Feature/Ords/OrdsRepairsApiTest.php @@ -551,15 +551,63 @@ public function test_a_date_only_event_end_includes_that_whole_day(): void public function test_updated_since_filter(): void { + // All three rows are backdated: "unchanged since" now means the device + // and both of its parents have stood still, not the device alone. $device = $this->seedRepair(); - $device->timestamps = false; - $device->updated_at = '2000-01-01 00:00:00'; - $device->save(); + $this->backdate($device, '2000-01-01 00:00:00'); $this->assertEmpty($this->fetchRecords(['updated_since' => '2010-01-01T00:00:00+00:00'])); $this->assertCount(1, $this->fetchRecords(['updated_since' => '1999-01-01T00:00:00+00:00'])); } + /** + * The case that made incremental consumers miss data: an event sits + * unapproved for months, the devices on it are exported by nobody, and + * approving it never touches devices.updated_at. Keyed on the device row + * alone those devices stay invisible for good. + */ + public function test_updated_since_sees_a_device_whose_event_changed_later(): void + { + $device = $this->seedRepair(); + $this->backdate($device, '2000-01-01 00:00:00'); + + $this->touchRow(Party::class, $device->event, 'idevents', '2020-06-01 12:00:00'); + + $this->assertCount(1, $this->fetchRecords(['updated_since' => '2010-01-01T00:00:00+00:00'])); + } + + public function test_updated_since_sees_a_device_whose_group_changed_later(): void + { + $device = $this->seedRepair(); + $this->backdate($device, '2000-01-01 00:00:00'); + + $groupId = Party::findOrFail($device->event)->group; + $this->touchRow(Group::class, $groupId, 'idgroups', '2020-06-01 12:00:00'); + + $this->assertCount(1, $this->fetchRecords(['updated_since' => '2010-01-01T00:00:00+00:00'])); + } + + /** + * The watermark has to run on the same clock as the filter, or a consumer + * resuming from it would skip the parent-row changes the filter just + * started returning. + */ + public function test_the_sync_watermark_covers_the_parent_rows_too(): void + { + $device = $this->seedRepair(); + $this->backdate($device, '2000-01-01 00:00:00'); + + $this->touchRow(Party::class, $device->event, 'idevents', '2020-06-01 12:00:00'); + + $response = $this->getJson($this->url()); + $response->assertSuccessful(); + + $this->assertEquals( + '2020-06-01T12:00:00+00:00', + Carbon::parse($response->json('sync.max_updated_at'))->utc()->toIso8601String() + ); + } + public function test_event_window_filters(): void { $this->seedRepair(); @@ -801,6 +849,29 @@ private function seedCategory(int $id, string $name, bool $powered): void * here, so a regression in the gate fails the whole class rather than one * test. */ + /** Backdates the device and both parent rows past the auto-maintained columns. */ + private function backdate(Device $device, string $when): void + { + $device->timestamps = false; + $device->updated_at = $when; + $device->save(); + + $event = Party::findOrFail($device->event); + $this->touchRow(Party::class, $event->idevents, 'idevents', $when); + $this->touchRow(Group::class, $event->group, 'idgroups', $when); + } + + /** + * `events` and `groups` carry ON UPDATE CURRENT_TIMESTAMP, which only fires + * when updated_at is left out of the statement, so an explicit value wins. + * + * @param class-string<\Illuminate\Database\Eloquent\Model> $model + */ + private function touchRow(string $model, int $id, string $key, string $when): void + { + $model::query()->where($key, $id)->update(['updated_at' => $when]); + } + private function apiToken(): string { if ($this->apiToken === null) { From f1df87468695bea765c677f3039aad5c0ce65829 Mon Sep 17 00:00:00 2001 From: Angel de la Torre Date: Fri, 21 Aug 2026 11:47:57 -0700 Subject: [PATCH 8/8] docs(ords): reattach the apiToken docblock to its method The docblock explaining why the shared fixture is an Administrator was written onto apiToken() in ed667c525, then backdate() and touchRow() were inserted above it in f9e2e84d0, leaving it stranded on backdate() - which already carries its own one-line docblock. Comment placement only; no test behaviour changes. --- tests/Feature/Ords/OrdsRepairsApiTest.php | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/tests/Feature/Ords/OrdsRepairsApiTest.php b/tests/Feature/Ords/OrdsRepairsApiTest.php index a1a9b0e999..361d5fb9b7 100644 --- a/tests/Feature/Ords/OrdsRepairsApiTest.php +++ b/tests/Feature/Ords/OrdsRepairsApiTest.php @@ -843,12 +843,6 @@ private function seedCategory(int $id, string $name, bool $powered): void ]); } - /** - * An Administrator, because the export is gated on that role the same way - * UserController::changes is. Every other fixture in this file goes through - * here, so a regression in the gate fails the whole class rather than one - * test. - */ /** Backdates the device and both parent rows past the auto-maintained columns. */ private function backdate(Device $device, string $when): void { @@ -872,6 +866,12 @@ private function touchRow(string $model, int $id, string $key, string $when): vo $model::query()->where($key, $id)->update(['updated_at' => $when]); } + /** + * An Administrator, because the export is gated on that role the same way + * UserController::changes is. Every other fixture in this file goes through + * here, so a regression in the gate fails the whole class rather than one + * test. + */ private function apiToken(): string { if ($this->apiToken === null) {