Skip to content

Label the store save button, which reddened development's e2e - #1697

Merged
rubenvdlinde merged 1 commit into
developmentfrom
fix/store-settings-save-label
Sep 3, 2026
Merged

Label the store save button, which reddened development's e2e#1697
rubenvdlinde merged 1 commit into
developmentfrom
fix/store-settings-save-label

Conversation

@rubenvdlinde

Copy link
Copy Markdown
Contributor

My regression, from #1677. StoreSettingsTab shipped a bare Save button, so pages.spec.ts hit a strict-mode violation on the admin page:

getByRole('button', { name: 'Save', exact: true }) resolved to 2 elements

The convention was already written down

In that spec's own comment:

The real admin surface renders every section, and four of them have their own labelled save ("Save mandate matrix settings", "Save consultation settings", …). The Configuration section's control is the bare one.

Mine took a name that was already spoken for. Renamed to Save store settings, joining the four siblings.

Labelled rather than the test scoped, because the duplicate is a real defect on its own terms: two buttons with the identical accessible name on one page give a screen-reader user no way to tell them apart. Fixing the label fixes both, and editing a test to accommodate new code would have hidden the accessibility problem.

Why this reached development at all

This is the part worth keeping. The e2e leg is skipped on PRs into development and runs only on the push. #1677 was green on 49 checks with E2E Tests (Playwright): skipping, so the PR could not have caught it, and the 19 local Playwright tests I ran did not include pages.spec.ts.

Any change touching the admin settings surface should run pages.spec.ts locally before merge, because CI will not.

Verified

The exact assertion that failed on development now passes against a live instance:

✓ pages.spec.ts:232 › Settings page › renders the configuration section and its save control

🤖 Generated with Claude Code

MY REGRESSION, from #1677. StoreSettingsTab shipped a bare "Save" button, so
`pages.spec.ts` hit a strict-mode violation:

  getByRole('button', { name: 'Save', exact: true }) resolved to 2 elements

The convention was already written down, in that spec's own comment: the admin
surface renders every section, four of them carry a LABELLED save ("Save
mandate matrix settings", "Save consultation settings", …), and the bare "Save"
belongs to the Configuration section alone. Mine took a name that was spoken
for.

Labelled rather than the test scoped, because the duplicate is a real defect in
its own right: two buttons with the identical accessible name on one page give
a screen-reader user no way to tell them apart. Fixing the label fixes both.

WHY THIS REACHED DEVELOPMENT AT ALL, and it is the useful part: the e2e leg is
SKIPPED on pull requests into development and runs only on the push. #1677 was
green on 49 checks with `E2E Tests (Playwright): skipping`, so the PR could not
have caught this. The 19 local Playwright tests I ran did not include
pages.spec.ts.

Verified against a live instance: the exact assertion that failed now passes.
@github-actions

github-actions Bot commented Sep 3, 2026

Copy link
Copy Markdown
Contributor

Quality Report — ConductionNL/dossiq @ deaf1b8

Check PHP Vue Security License Tests
lint
phpcs
phpmd
psalm
phpstan
phpmetrics
eslint
stylelint
build
check-manifest
check-vue3-compile
test-l10n
format
check-schema-l10n
check-l10n-js
composer ✅ 106/106
npm ✅ 540/540
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-03 05:02 UTC

Download the full PDF report from the workflow artifacts.

@rubenvdlinde
rubenvdlinde merged commit 4344c30 into development Sep 3, 2026
49 checks passed
@rubenvdlinde
rubenvdlinde deleted the fix/store-settings-save-label branch September 3, 2026 05:03
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