Skip to content

feat(flow): anchor a task read to one run, and give a run an address - #3462

Merged
rubenvdlinde merged 1 commit into
developmentfrom
feat/run-scoped-tasks-and-run-route
Sep 6, 2026
Merged

feat(flow): anchor a task read to one run, and give a run an address#3462
rubenvdlinde merged 1 commit into
developmentfrom
feat/run-scoped-tasks-and-run-route

Conversation

@rubenvdlinde

Copy link
Copy Markdown
Contributor

Why

Building the flow editor's run view turned up two things the server did not
offer. Both were found by trying to build the tab, not by reading the code.

A run-anchored task read

task#index had no runUuid parameter at all, and it defaults to
scope=assigned. A client-side filter would therefore have answered "what
did this run ask me"
— an empty list for every run whose performer is a
group, which reads as a run that asked nobody.

So runUuid anchors rather than filters:

  • it replaces the scope narrowing, because the run's view asks what the
    run asked;
  • it lifts the external exclusion, exactly as the objectUuid anchor
    already does: a run that asked a resident through the portal did ask
    somebody, and hiding that ask would report the run as waiting on nothing;
  • it does not relax visibility. A non-admin is still held to one of the
    sanctioned relationships, and for a run's own tasks that relationship is
    requester, which the engine stamps with the run's acting identity. The
    run's owner therefore sees all of it and a passer-by sees none, without
    the anchor knowing anything about ownership.

An empty ?runUuid= is read as absent. A query string carries it as '',
never as null, and '' applied as a real predicate matches no row: a tab
that forgot to fill the uuid in would have reported the run as having asked
nobody, rather than reporting that it asked nothing of the server.

An address for a run

/apps/openregister/flow-runs/{uuid}, on the same terms as the task deep
link beside it. A run gets no screen of its own — the flow editor already
shows the replay, the steps, the objects and the log — but without an
address, "open this run in a new tab" cannot be a link, and what is not a
link cannot be middle-clicked, bookmarked, or pasted into a ticket.

The page is a resolver: it reads the run, learns its flow, and replaces
itself with that flow's editor holding the run inspected. replace, never
push, so a redirect nobody chose does not sit in their history. It renders
a real failure state rather than bouncing to the dashboard, because a dead
link that lands you somewhere plausible is the one that cannot be diagnosed.

The consuming half is nc-vue's: the Runs list renders the <a href>, and the
editor reads ?run=. This is the server side of that contract.

How it was verified

  • The five new mapper tests were each seen RED first, by neutering the
    scope anchor and then the filter in turn (4 of 5 red, then 1 of 5 red).
  • PHPCS, Psalm and PHPStan clean on the changed files.
  • 347 Task unit tests green.
  • Both l10n gates green, all 36 locales at parity. The two new strings are
    derived from the sibling "No such task" strings, so each locale keeps its
    own established terms for a run and for a flow.
  • Exercised against a live instance: the anchored read returns the run's
    group-assigned task that scope=assigned hides; the unanchored read is
    unchanged; the empty anchor falls back to unanchored rather than to zero.

Two things reviewers should know

The l10n diff is large and almost entirely whitespace. l10n-ai.js add
rewrote all 37 bundles into the canonical Nextcloud/Transifex layout (4-space
indent, " : " separator), which is not the layout they were committed in. I
verified per locale that the only content change is the two new keys: every
other line differs by separator spacing alone, every plural array is
byte-identical, and every nplurals header survives. l10n/*.js must never
be hand-edited, so this is the tool's output, not a choice.

Ten unrelated unit errors exist in this environment, in
FlowClaimMapperTest and FlowRunStepOrderingMapperTest. They mock
IQueryBuilder::forUpdate, which the vendored nextcloud/ocp stub has and
the local server checkout's lib/public copy does not. This PR touches
neither file, and the failure is a stale checkout rather than a defect.

The flow editor's run view needs two things the server did not offer, and
both were found by trying to build the tab rather than by reading the code.

A RUN-ANCHORED TASK READ. `task#index` had no `runUuid` at all, and
defaults to `scope=assigned`, so a client-side filter would have answered
'what did this run ask ME' — an empty list for every run whose performer is
a group, which reads as a run that asked nobody. `runUuid` therefore
ANCHORS rather than filters: it replaces the scope narrowing and lifts the
external exclusion (a run that asked a resident through the portal did ask
somebody), exactly as the `objectUuid` anchor already does. Visibility is
the one thing it does not relax — a non-admin is still held to a sanctioned
relationship, and for a run's own tasks that relationship is `requester`,
which the engine stamps with the run's acting identity.

An empty `?runUuid=` is read as absent. A query string carries it as '',
never as null, and '' applied as a real predicate matches no row: a tab that
forgot to fill the uuid in would have reported the run as having asked
nobody instead of reporting that it asked nothing of the server.

AN ADDRESS FOR A RUN. `/apps/openregister/flow-runs/{uuid}`, on the same
terms as the task deep link beside it. A run gets no screen of its own — the
flow editor already shows the replay, the steps, the objects and the log —
but without an address, 'open this run in a new tab' cannot be a link, and
what is not a link cannot be middle-clicked, bookmarked or pasted into a
ticket. The page resolves the run to its flow and hands over, with a real
failure state rather than a silent bounce to the dashboard, because a dead
link that lands you somewhere plausible is the one that cannot be diagnosed.

Verified: the five new mapper tests were each seen RED first, by neutering
the anchor and then the filter in turn; PHPCS, Psalm and PHPStan clean on
the changed files; both l10n gates green with all 36 locales at parity, the
two new strings derived from the sibling task strings so each locale keeps
its own established terms for a run and a flow. Exercised live on :8080:
anchored read returns the run's group-assigned task that `scope=assigned`
hides, the unanchored read is unchanged at 7 rows, and the empty anchor
falls back to unanchored rather than to zero.
@rubenvdlinde
rubenvdlinde merged commit 801cc02 into development Sep 6, 2026
34 of 36 checks passed
@github-actions

github-actions Bot commented Sep 6, 2026

Copy link
Copy Markdown
Contributor

Quality Report — ConductionNL/openregister @ 2f263ff

Check PHP Vue Security License Tests
lint
phpcs
phpmd
psalm
phpstan
phpmetrics
eslint
stylelint
build
check-specs
test-l10n
test-l10n-parity
format
check-schema-l10n
check-l10n-js
composer ✅ 174/174
npm ✅ 543/543
app:check-code ⏭️
info.xml
REUSE
PHPUnit
Newman
Playwright ⏭️ deferred — runs on the promotion into beta/main, not on a pull request into development
Hydra gates

Quality workflow — 2026-09-06 07:38 UTC

Download the full PDF report from the workflow artifacts.

rubenvdlinde added a commit that referenced this pull request Sep 6, 2026
…rings in them (#3467)

`check:l10n-js` has been red on development since #3462, reporting all 37
browser catalogues as stale. The generator that check runs,
scripts/build-l10n-js.js, landed in #3428 the day before, and the .js
files committed since predate it, so they are written in the older
transifex layout the generator no longer emits.

Rebuilding alone would have been wrong. #3462 added two strings

    No such run
    The run does not exist, or it is not yours to see. Deleting a flow
    deletes its runs.

straight into the 37 .js catalogues, translated, without ever adding them
to the .json sources the generator reads. A plain `npm run l10n:build`
therefore deleted 37 languages' worth of two real translations, and the
UI would have fallen back to English wherever they appear.

So the two strings are harvested out of the committed .js files into the
matching .json, appended in the order those files already use, and only
then is everything rebuilt.

Checked, because a catalogue rebuild is exactly the kind of change whose
diff is too large to read:

- a no-op json round trip reproduces every source file byte for byte
  before anything is written, so adding two strings cannot reformat 37
  catalogues as a side effect. The json diff is +3 -1 lines per file.
- every key in every old .js is present in the new one with an identical
  value. Across all 37 locales: nothing lost, nothing altered.
- `node scripts/build-l10n-js.js --check` exits 0, as do test:l10n,
  test:l10n:parity and check:schema-l10n.
- scripts/check-l10n.js reports 788 issues both before and after. That is
  pre-existing dynamic-`t()` debt in dashboard and settings views, and it
  is not one of the checks CI runs.

A first count said 2,907 keys had been lost per file. That was my own
regex assuming four-space indentation, not the data: the generator emits
eight. The real delta was two.

Co-authored-by: Conduction Release Bot <release-bot@conduction.nl>
Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
rubenvdlinde added a commit that referenced this pull request Sep 6, 2026
…has no baseline (#3471)

Hydra Gates has been red on development since #3462, on two findings that
are the same new file:

    [gate-16] spec-coverage:   1 changed method missing @SPEC
    [gate-26] visual-coverage: 1 new page component missing a visual baseline

Both are src/views/flows/FlowRunDetail.vue.

gate-16: `resolve()` now carries @SPEC pointing at the flow-runs-subject-scope
spec, which is where the deep link this page implements is written down: "the
run uuid in the row is the deep link to it". The path was opened and read
rather than guessed. A tag the gate accepts but that resolves to nothing
clears the finding without covering anything, and that is worth stating
because the gate cannot tell the difference.

gate-26: an @visual exclude with its reason, rather than a baseline. This
page has no screen. It is a resolver: the spinner is replaced by the flow
editor as soon as the run resolves, and the failure state is one
NcEmptyContent whose pixels say nothing the flow editor's own baselines do
not already cover.

WHAT IS STILL MISSING, PLAINLY. The behaviour here is worth a test and does
not have one: that a cold load of /flow-runs/{uuid} reaches the flow rather
than the manifest's catch-all, that Back does not bounce because the handover
replaces rather than pushes, and that an absent run says so at its own
address. I wrote that spec and could not run it: this workstation's instance
carries 30-odd apps and the shared global setup times out logging in at 30s,
twice, before any test starts. Rather than ship a playwright spec whose only
evidence is that it reads correctly, the exclude says what is not covered and
the comment in the file says what to write. The gap is named, not hidden.

Co-authored-by: Conduction Release Bot <release-bot@conduction.nl>
Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant