Conversation
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.
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.
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.
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.
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.
…r 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 [email protected] each" matched as an address and the price was destroyed. Guarded on the candidate containing a letter instead, which also keeps "[email protected]" 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 <[email protected]> Claude-Session: https://claude.ai/code/session_01JfrVZbbtaC2hLoFh7T3qFs
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 <[email protected]> Claude-Session: https://claude.ai/code/session_01JfrVZbbtaC2hLoFh7T3qFs
|
Thanks for this. I've reviewed it with AI assistance, so treat the findings below as a starting point rather than gospel — push back on anything that looks wrong. Fixes pushed to Authorization. I think the right model here is Administrator-only. As written the endpoint returns every device from every approved group to any authenticated caller, and as you noted, Redaction. Three gaps, all with tests:
Config.
The other two deferred items look right to me as they stand. One thing I want to think about rather than change: |
The docblock explaining why the shared fixture is an Administrator was written onto apiToken() in ed667c5, then backdate() and touchRow() were inserted above it in f9e2e84, leaving it stranded on backdate() - which already carries its own one-line docblock. Comment placement only; no test behaviour changes.
|
I have added both commits into the branch. The Administrator gate is the right call and I've got no pushback. I originally thought about adding it but didn't want to overtighten things before it was confirmed. Given we'd want to mirror the Zapier path, ORA pulling with an admin token makes sense. I checked and confirmed the redaction gaps. The
Same with Thanks for doing There's also one small commit of mine on top, On We hit the same question in our fork and went with opt-in: |
edwh
changed the title
|
The failing check here is not caused by anything in this PR, and it is now fixed on The failure is: That test built a CircleCI builds a fork PR at its own head commit rather than a merge with the base, so this branch needs Nothing else is failing — 616 tests, that one error. (AI-assisted triage by Claude, posted with maintainer supervision.) |
testGeocode built a Geocoder directly and geocoded a real address against the live Maps API. When that call failed - network, key or quota - the helper returned false and the test errored with "Trying to access array offset on value of type bool", as it did on the TheRestartProject#900 run. Nothing was wrong with the branch: a unit test simply depended on a third party being reachable. Move the HTTP call behind a protected fetch(), the same idea as the existing googleKey() seam, so the parsing can be tested with a canned response. The assertions on the parsed latitude, longitude and country code are unchanged. Also covers the paths that had no test: no results, a failed request (the one that produced the error above), and the ForceGeocodeFailure short circuit. GeocoderMock overrides geocode() outright, so tests resolving the geocoder from the container are unaffected. Co-Authored-By: Claude Opus 5 (1M context) <[email protected]>
|
Regarding the For info, we also do additional checks and scrubs of all data at the ORA level. I'm happy with Admin scope for now, likely better than having it wide open. Correct me if I'm wrong - as it stands, one would either need to ask an admin of an instance for an export, or be turned into an admin of that instance. Take the example of me accessing the ORA data from community.ifixit.com - I'm assuming I'm not an admin there :) - so I guess I accessed it before this change was introduced? In future (e.g. next year), I'd need to ask an admin at community.ifixit.com for the data. Right? That's not a big deal - it's good to be in touch with data providers. But one day perhaps we might consider another permission available to specific non-admin users, so you could say - Neil and Monique at the Open Repair Alliance, we're happy for them to access this endpoint, though we don't want them as admins of our instance. Anyway - very happy for this to be merged in. Though the toggles related to problem are still to be added on this PR - right? If so can you add them @edwh or @ardelato ? @ardelato I'll repeat what I said on Slack - thanks for adding this, hugely appreciated! |
`devices.problem` is volunteer free text. The scrubber takes out contact details and identifiers but cannot take out personal names, and a group name plus an event date narrows the field on its own, so publishing it is a decision an instance should make separately from turning the endpoint on. The flag defaults off: an instance that upgrades without setting anything stops publishing free text rather than starting to. The column is still emitted when the flag is off. ORDS is a fixed 14-column shape and a consumer reading it positionally would break if the key disappeared, so opting out empties the value instead. configuredFlag() reads the value rather than casting it. Laravel only casts the literals it recognises, so ORDS_INCLUDE_PROBLEM=on arrives as the string "on" and (bool) would make any non-empty typo truthy. Anything filter_var cannot read falls back to the private side. The feature test's setUp turns the flag on, because it ships off and the column would otherwise be empty in every other test in the file.
An instance that already reviews this data downstream may not want it redacted twice, so scrubbing is separable from publishing. It defaults on and only applies once ORDS_INCLUDE_PROBLEM has let the column through, so turning it off cannot publish anything on its own. With it off the column ships exactly as the volunteer typed it, HTML and contact details included. That is the point of the flag, but it is why the default is the other way and why the config comment says an instance should have a specific reason. The flag is scoped to `problem`. `item_type` and `brand` are volunteer free text with no vocabulary behind them and have no opt-out at all, so they keep redacting either way. Unreadable values fail closed in the opposite direction from `include`: scrubbing stays on rather than switching off.
EnsureAPIToken mints a token for every logged-in user and the log is the only trace of an export afterwards, so it already carries the caller, the filters, the row count and the redaction tallies. Whether free text went out at all, and whether it was redacted on the way, cannot be inferred from any of those after the fact. Both flags now go in alongside the tallies.
|
I pushed the PROBLEM flags change/feature.
Not quite. We are using an API token auth system for those endpoints in the fork, so you don't need to have an actual account or be an admin to use the API there. |
|
Thanks for the problem-flag commits. A heads-up on CI before you merge
We'd push the merge for you, but GitHub doesn't allow maintainer pushes to organisation-owned forks. (AI-assisted triage by Claude, posted with maintainer supervision.) |
|
|
@edwh it looks like maybe you could merge develop in to the fork? In which case, is this good for me to merge into develop? |
This branch has not been deployed
3 participants
Description
Adds
GET /api/v2/repairs, which exports repair records in Open Repair Data Standard v0.3. The Open Repair Alliance asked for a codified export rather than a periodic database extract, and there's no endpoint for it in the codebase today.This started in our fork at iFixit, where it's merged and running: iFixit/restarters#58. It isn't a straight cherry-pick. That version sits on an API-client auth layer, a feature flag and Helm config that are specific to our deployment, none of which apply here, so this is rebuilt on
developagainst your conventions with a handful of fixes on top. Happy to adjust anything that doesn't fit.Changes
GET /api/v2/repairs, in the existingv2group behindauth:api, the same way the Zapier and Repair Together endpoints are already served. JSON or CSV, withupdated_since,event_start,event_end,powered,pageandper_page. Per-page tops out at 1000 rather than 100 because it's a bulk export. Only approved events on approved groups are visible, and soft-deleted events are excluded. Files:API/RepairController.php,routes/api.php.year_of_manufactureisn't stored, so it's derived from the event year minusdevices.age.countryconvertsgroups.country_codefrom alpha-2 to alpha-3, andproduct_category_idis a name lookup becauseidcategoriesdoesn't line up with the standard's ids. Files:Services/Ords/OrdsRecordMapper.php,Helpers/Iso3166.php.config/ords.phpfollows ORA's published data rather than theirtableschema.json, which is stale in places. The standard collapses the screen-size and laptop-size category splits, carriesUnknownas a realrepair_statusvalue, and drops the "the" from "No way to open the product" as seeded in2018_11_12_135805.partner_product_categoryuses the<category> ~ <item_type>shape from your own published rows.devices.problem,devices.item_typeanddevices.brandare all volunteer-written, anditem_typeis validated as a barestringinDeviceControllerwith no vocabulary behind it. All three go throughProblemTextScrubber, which strips HTML and redacts emails, phone numbers, digit runs of 8 or more, and URL query strings.ORDS_ID_PREFIXandORDS_DATA_PROVIDERhave no defaults, and the endpoint returns 503 while either is blank. The id is the stable key ORA upserts on across releases, so publishing under an unassigned namespace would overwrite another provider's rows. Nothing is exported until you set them.EnsureAPITokenmints a token for every logged-in user, so any account can reach this, and that log is the only trace afterwards if someone asks who pulled what.Output was validated against ORA's published
tableschema.json. Column names and order match exactly and every declared constraint passes, except theiridregex, which uses a hyphen while all of their own published rows use an underscore.On the free-text columns
The scrubber is best-effort. It takes out contact details and identifiers, but personal names aren't pattern-detectable and stay in. Two bypasses turned up during review, both fixed with regression tests: whitespace or inline markup inside an address (
owner@\nexample.com,owner@ex<b>ample</b>.com) defeated the email pattern, and a>inside a quoted HTML attribute ended the tag early and left the address whole.As written,
problemships on every export with no toggle. That's the call I'd flag hardest for you, since it's your community's data under your terms, not ours. If you'd rather it were opt-in, or dropped entirely for a structured-fields-only export, that's a small change and I'm happy to make it.Deferred
Three gaps, all of which I'm happy to split out or fold in, whichever you prefer.
updated_sincekeys offdevices.updated_at, so approving an old event leaves its devices invisible to an incremental consumer. Widening it toevents.updated_atandgroups.updated_atwould fix it.Pagination is offset-based, so a row leaving the set mid-crawl shifts later pages, and a deep crawl is quadratic. Keyset pagination on
devices.iddeviceswould fix both.The route inherits the app-wide 300/min bucket from
RouteServiceProvider. That's generous for a request that runs three full passes over thedevices/events/groups/categoriesjoin, but I didn't want to guess a limit that might throttle ORA's own crawl.QA Notes
Run
php artisan l5-swagger:generateafter pulling.TestCase::get()validates every/api/v2response against the generated schema, so the suite depends on it.To exercise it, set
ORDS_ID_PREFIXandORDS_DATA_PROVIDERand pull a full export against the spec. The prefix carries its own separator:ORDS_ID_PREFIX=restarters_producesrestarters_12345. Worth confirming with ORA which prefix is assigned to you before publishing under it, since it's the upsert key.Tests: 69 covering the config guard, column mapping, both vocabularies, visibility, filters, pagination, CSV output and redaction.