Repository navigation
Conversation
- an application can get a technical name: lowercase letters, numbers and
underscores, unique per owner, set on create and update; an empty name
removes it
- /apps/tables/app/{name} opens it; the owner's own application wins over
one shared with the same name, and others' applications are linked by id
- the app navigation links the owner's applications by their name
- the name travels with an exported scheme
Signed-off-by: Remko <remko@conduction.nl>
Assisted-by: ClaudeCode:claude-opus-5-5
- the create and edit dialogs take an optional technical name - the overview shows and opens the application at its readable address for its owner, and by id for everyone else Signed-off-by: Remko <remko@conduction.nl> Assisted-by: ClaudeCode:claude-opus-5-5
| <div class="col-4"> | ||
| {{ t('tables', 'Technical name') }} | ||
| </div> | ||
| <input v-model="technicalName" type="text" data-cy="editContextTechnicalName" |
There was a problem hiding this comment.
🟢 Minor — the technical-name inputs have no label or inline validation
The text "Technical name" is a sibling <div>, not a <label for>, so the input's only accessible name is its placeholder. The naming rules also live in that placeholder, which disappears once you type. A malformed or taken name is only reported after submit, as the generic toast plus the server's untranslated English message. The same applies to CreateContext.vue.
Impact: screen readers announce no label, and users learn the rules only by failing.
Suggested fix: a real <label>, and a client-side check of ^[a-z][a-z0-9_]*$ with helper text, in both dialogs.
| return null; | ||
| } | ||
| MenuItemInput::assertTechnicalNameValid($technicalName); | ||
| if ($this->contextMapper->isTechnicalNameTaken($technicalName, $ownerId, $contextId)) { |
There was a problem hiding this comment.
🟡 Concern — Ownership transfer bypasses the per-owner uniqueness check
The name is only checked on create, update and import. transfer() (unchanged, line ~634) sets the new owner and keeps technical_name without checking it. If the new owner already has an application called intake, they now own two.
Impact: addToNavigation() links both by name, so both menu entries open the same (oldest) application. findIdByTechnicalName() never reaches the second one. The "unique per owner" rule in the PR body no longer holds.
Fix: in transfer(), call isTechnicalNameTaken($name, $newOwnerId, $contextId) and clear the name (or refuse with a BadRequestError). Add a unit test for it.
Done when:
- sed -n '/public function transfer/,/^\t}/p' lib/Service/ContextService.php | grep -q isTechnicalNameTaken
- A ContextServiceTest case transfers a named application to a user who already uses that name and asserts the name is cleared (or a BadRequestError is thrown); it goes red when the check is removed
|
|
||
| /** | ||
| * Applications (contexts) get an optional technical name, which addresses them at | ||
| * /apps/tables/app/{technicalName}. It is unique per owner, which the service checks. |
There was a problem hiding this comment.
🟡 Concern — Uniqueness is a check-then-write, with no DB constraint
"Unique per owner" relies on isTechnicalNameTaken() only. In create() that check runs before atomic() starts (ContextService.php:152). Two concurrent creates, or a double submit, can both pass and store the same name. The menu items in this same stack use a unique index (tables_ctx_menu_name_uq, Version2400Date20261012020000.php:45). This layer does not.
Impact: the same two-apps-one-name state as with a transfer: both navigation entries open the same application.
Fix: add $table->addUniqueIndex(['owner_id', 'technical_name'], 'tables_ctx_owner_name_uq') (24 chars, so within the Oracle limit). NULL names stay allowed on sqlite, mysql, mariadb and pgsql, because those databases treat NULLs as distinct. Oracle does not: it treats rows with the same owner_id and a NULL name as duplicates. If Oracle is in scope, that needs a decision first; moving the service check into the transaction alone does not stop two concurrent creates. Either way, map a unique-constraint violation to the same 400.
Done when:
- grep -q "addUniqueIndex(['owner_id', 'technical_name']" lib/Migration/Version2400Date20261014000000.php
- CI green on sqlite, mysql, mariadb and pgsql with the index
| } | ||
|
|
||
| return $this->update($contextId, $userId, $scheme->getName(), $scheme->getIcon() ?? '', $scheme->getDescription() ?? '', $resolvedNodes, $this->resolveMenuItems($scheme->getMenuItems())); | ||
| return $this->update($contextId, $userId, $scheme->getName(), $scheme->getIcon() ?? '', $scheme->getDescription() ?? '', $resolvedNodes, $this->resolveMenuItems($scheme->getMenuItems()), $scheme->getTechnicalName()); |
There was a problem hiding this comment.
🟡 Concern — Importing a same-owner copy fails, preview does not warn
An exported scheme now carries the name. Export application A (intake) and import it into application B of the same owner, the usual way to copy an application, and update() throws "already exists". The controller rolls back the whole import and returns 400. compareSchemeChanges() never looks at the name, so the preview reports nothing first. The import dialog has no field to change the name. The only workaround is editing the JSON.
Fix (backend): in importScheme(), pass the scheme's name only when the owner does not already use it on another application. Otherwise keep the target's name. Or report the conflict from compareSchemeChanges() so the preview shows it. Add a test for whichever you choose.
Done when:
- A test imports a scheme whose technicalName the owner uses on another context and asserts the chosen behaviour (import succeeds with the target's name kept, or the preview lists the conflict)
| $this->assertSame('intake', $updated->getTechnicalName()); | ||
| } | ||
|
|
||
| public function testCreateStoresTheTechnicalName(): void { |
There was a problem hiding this comment.
🟢 Minor — Untested branches behind the PR's claims
Several claims have no test that goes red when they break:
- create with a taken name (only update is tested),
- the 400 mapping in the controller for create, update and import (the service tests stop at InvalidArgumentException),
- import with a taken name rolls back,
PageController::context(): digits go to the id, a name goes to the lookup, an unknown name goes toindex()(the Playwright test covers only the happy path by name),- lookup through a group share (the mapper test stubs
getGroupIdsForUserto[]).
Add a create-taken case here and a small PageController test for the three branches.
Done when:
- ContextServiceTest has a create case that expects InvalidArgumentException on a taken name
- A PageController test covers id, name and unknown name
rjzondervan
left a comment
rjzondervan
left a comment
There was a problem hiding this comment.
Verdict: Approve (Thorough)
Technical names for applications are well scoped: the lookup only reaches applications the user owns or that are shared with them, ids and names cannot collide, and update keeps or clears the name as described. The concerns are about keeping "unique per owner" true on every path, and one import case that now fails. None blocks under Thorough.
🟡 Concerns
- Ownership transfer bypasses the per-owner uniqueness check
- Uniqueness is a check-then-write, with no DB constraint
- Importing a same-owner copy fails, preview does not warn
🟢 Minor
✅ Verified clean
- Migration: nullable
technical_name, length 200 (matches the menu-item limit),hasTable/hasColumnguards, no data loss. - Update: an absent or null name keeps it,
''or whitespace clears it; the taken check excludes the application itself, so the edit dialog can resend its current name. /app/{name}: resolved throughapplyOwnedOrSharedQuery(group shares included), returns an id only; names must start with a letter and the route sends digits to the id path.- Import runs in the controller's transaction, so a malformed or taken name rolls everything back with a 400; export carries the name.
Mode: Thorough · Head: bb2b3a96 (reviewed at 6383311f; the newer commits only merge base fixes to Context.vue and two e2e specs)
CI: green except Playwright. 3 failures are shared with the base branch. The new test "An application with a technical name is opened at its readable address" is flaky on Playwright (master) only: the first two tries got a 500 from PUT /api/2/contexts/{id}, retry 2 passed. The server log was not captured, so the cause is not confirmed; it looks like the SQLite lock race seen on the menu layer.
Not run: local PHPUnit and mutations.
3 participants
An application can get a technical name, so it opens at a readable address like
/apps/tables/app/intakeinstead of/apps/tables/app/28.What changes
tables_contexts_contextgets a nullabletechnical_name./apps/tables/app/{name}opens the application. The user's own application wins over one shared with them under the same name. Because names are only unique per owner, applications of others are linked by id, both in the app navigation and on the overview. An unknown name opens the Tables start page.technicalNameis added to the context response and to the create, update and import scheme endpoints (OpenAPI and TS types regenerated).🖼️ Screenshots
🏁 Checklist
/backport to stableX.X🤖 AI (if applicable)