Skip to content

UI review bug fixes - #388

Merged
eirvandelden merged 10 commits into
mainfrom
ui-review-bug-fixes
Oct 24, 2026
Merged

eirvandelden merged 10 commits into
mainfrom
ui-review-bug-fixes

Conversation

@eirvandelden

Copy link
Copy Markdown
Owner

Summary

The October 2026 UI review (artifact, slice 8 "clean-up") found small defects that are still present on main after #386:

  • On the user list (/users), an administrator cannot change a user between member and administrator: ticking the role box does nothing. The markup asks for a form Stimulus controller that exists neither in the app nor in appkit.
  • On your own profile edit page (reached from the menu), "Terug" sends you to the user list, a page the menu does not offer.
  • Labels on the transaction, category and chattel forms show English attribute names (Debitor, Creditor, Category, Note, Booked at, Interest at, Type, Original note, Direction, Parent category, Purchase transaction) for a user whose locale is Dutch or Italian.
  • The page has no <html lang>, so browsers and screen readers do not know the page language.
  • The accounts index writes its pager by hand, twice per table, while shared/_pagination exists but only knows the page parameter.

An administrator changes a role on /users with one click. Your own profile edit page has no misleading back link. Form labels speak the user's language. Every page declares its language. The accounts index uses the shared pager.

Implementation

Domain terms

No new domain terms; GLOSSARY.md stays unchanged. No word of the intent is on an Avoid: list. UI words used in this plan:

  • Role box: the role checkbox of one row on /users. It carries the hidden attribute; its <label> is what the user clicks.
  • Submit on change: a form that saves as soon as one of its fields changes, with no submit button.
  • Pager: the shared/_pagination partial. Page parameter: the query key one pager writes (page, own_page, external_page).
  • Page language: the lang attribute of <html>, equal to I18n.locale while the page renders.

Diagrams

sequenceDiagram
  actor Admin
  Admin ->> RoleBox: click label (change event)
  RoleBox ->> SubmitOnChangeController: submit()
  SubmitOnChangeController ->> UserForm: requestSubmit()
  UserForm ->> UsersController: PATCH /users/:id {user: {role}}
  UsersController -->> Admin: redirect users_url (Turbo visit, new role label)
Loading
classDiagram
  class UserRow["users/_user"] { form data-controller=submit-on-change }
  class SubmitOnChangeController { +submit() }
  class Pagination["shared/_pagination"] { locals: page, param = :page }
  class AccountsIndex["accounts/index"] { 2 tables x 2 pagers }
  class OtherCallers { 6 templates, 12 render sites, page only }
  class LoginLayout["layouts/login (host override)"] { html lang }
  UserRow ..> SubmitOnChangeController : data-action change->submit-on-change#submit
  AccountsIndex --> Pagination : param own_page / external_page
  OtherCallers --> Pagination : default param page
Loading

Before: users/_user names a form controller that does not exist, so the change event goes nowhere; accounts/index writes its own nav.pagination four times; layouts/application, layouts/admin and appkit's layouts/login have no lang.

Signatures

  • app/javascript/controllers/submit_on_change_controller.js (new): submits the form it is attached to. Eager-loaded as submit-on-change by the existing eagerLoadControllersFrom("controllers", application).
    • submit() // (): void — calls this.element.requestSubmit(), so Turbo submits the form with its _method=patch.
  • app/views/users/_user.html.erb: form_with ... data: { controller: "submit-on-change" }; the role box gets data: { action: "change->submit-on-change#submit" }. hidden, id and the disabled rule stay exactly as they are.
  • app/views/shared/_pagination.html.erb: renders previous/next links for one page parameter. Strict locals become <%# locals: (page:, param: :page) %>; both merge(page: ...) calls become merge(param => ...). The six other templates that render it (twelve render sites: receipts/index, products/index, admin/users/index, shared/_recent_transactions, transactions/_table, todos/index) do not change.
  • app/views/accounts/index.html.erb: each of the two hand-written <nav class="pagination"> blocks becomes <%= render "shared/pagination", page: table[:page], param: table[:page_param] %>.
  • app/views/users/profiles/edit.html.erb: the link_to t("common.back"), users_path line goes; the title-bar block stays and yields nothing, so shared/_title_bar renders no <nav>.
  • app/views/layouts/application.html.erb, app/views/layouts/admin.html.erb: <html lang="<%= I18n.locale %>" ...>.
  • app/views/layouts/login.html.erb (new): host override of appkit's layouts/login, copied verbatim from the locked appkit revision (bundle show appkit, revision 9b5b30d), plus lang="<%= I18n.locale %>" on <html>. Host views win the lookup over engine views.
  • config/locales/{en,nl,it}.yml, under activerecord.attributes:
    • transaction: booked_at, category_id, id, creditor, debitor, interest_at, note, original_balance_after_mutation, original_note, original_tag, type.
    • category: direction, parent_category.
    • chattel: purchase_transaction_id.
    • Debit, Credit, Transfer resolve to the transaction keys through STI lookup_ancestors; the f.label calls stay as they are.

Words:

key en nl it
transaction.id Id ID ID
transaction.debitor Debitor Debiteur Debitore
transaction.creditor Creditor Crediteur Creditore
transaction.category_id Category Categorie Categoria
transaction.note Note Opmerking Nota
transaction.booked_at Booked at Boekingsdatum Data registrazione
transaction.interest_at Interest at Rentedatum Data valuta
transaction.type Type Type Tipo
transaction.original_note Original note Oorspronkelijke omschrijving Descrizione originale
transaction.original_balance_after_mutation Original balance after mutation Oorspronkelijk saldo na mutatie Saldo originale dopo il movimento
transaction.original_tag Original tag Oorspronkelijk label Etichetta originale
category.direction Direction Richting Direzione
category.parent_category Parent category Hoofdcategorie Categoria principale
chattel.purchase_transaction_id Purchase transaction Aankooptransactie Transazione d'acquisto

The nl/it words for debitor, creditor, note, booked_at and interest_at are the ones transactions.table already uses. Hoofdcategorie matches the existing must_be_parent error text. The en values equal today's humanized output, so English pages do not change.

Not checked: journal_administration has no Steepfile, and this change adds no Ruby methods.

Complexity

  • Role box: one PATCH per change through the existing UsersController#update (find + update), then one redirect to the index. A disabled box fires no event and sends nothing.
  • Labels, page language and pager add no queries. The pager reuses the page objects the controller already loads.

Invariants

  • A change to an enabled role box saves the opposite role with no other click; enforced by submit_on_change_controller.js and its wiring in users/_user.
  • Your own role box, and every role box a member sees, stay disabled; enforced by the existing disabled: expression in users/_user (and ensure_can_administer on UsersController#update).
  • A pager link changes only its own page parameter and keeps every other query parameter; enforced by request.query_parameters.merge(param => ...) in shared/_pagination.
  • With param left out, the pager writes page, so the twelve other render sites render the same links as today.
  • <html lang> equals I18n.locale on the application, admin and login layouts; ApplicationController#switch_locale sets the locale (user locale, else I18n.default_locale, en). Appkit's session, first-run and transfer controllers inherit ::ApplicationController, so the login layout gets the same locale.
  • en, nl and it hold the same activerecord.attributes keys for the three forms; test/i18n_test.rb (missing keys, normalized files) enforces it.

Alternatives

  • A. Generic form_controller.js (smallest): add a form controller with submit(), so the existing data-controller="form" markup starts to work with no view change. Rejected: form says nothing about the behaviour, and the markup relies on Stimulus' default input event for a checkbox. One line of view change buys an intention-revealing name and an explicit change event.

  • B. Host submit-on-change controller (chosen): as above. One repository, no gem bump, eslint-checked. It can move to appkit later without a markup change beyond the registration.

  • C. Appkit submit-on-change controller, Appkit::FormsHelper#submit_on_change_form_with, and lang in appkit's login layout: most reusable, and no layout copy in the host. Rejected for this PR: it needs a separate appkit change and a bundle update appkit before the host commits can go green, which the intent's constraints make stop-and-ask, and it adds public appkit API that must then stay stable. Kept as the recommended follow-up for the login layout (see ## Design decisions).

  • app/javascript/controllers/submit_on_change_controller.js — new; submits its form on change.

  • app/views/users/_user.html.erb — wire the role box to submit-on-change.

  • app/views/users/profiles/edit.html.erb — drop the Terug link.

  • config/locales/en.yml, config/locales/nl.yml, config/locales/it.yml — the 14 attribute keys above.

  • app/views/layouts/application.html.erb, app/views/layouts/admin.html.erb — lang.

  • app/views/layouts/login.html.erb — new host override with lang.

  • app/views/shared/_pagination.html.erb — optional param local.

  • app/views/accounts/index.html.erb — render the shared pager.

  • Tests: test/system/user_roles_test.rb (new), test/integration/users_index_test.rb, test/integration/users/profiles_test.rb, test/integration/form_labels_test.rb (new), test/integration/page_language_test.rb (new), test/integration/pagination_navigation_test.rb.

Proof

  • An administrator on /users ticks the role box of a member; that member is now an administrator, without pressing another button → test/system/user_roles_test.rb test "an administrator makes a member an administrator by ticking the role box"
  • An administrator on /users unticks the role box of another administrator; that user is now a member → test/system/user_roles_test.rb test "an administrator makes another administrator a member by unticking the role box"
  • On /users the role box of your own row stays disabled, and a member sees every role box disabled → test/integration/users_index_test.rb test "your own role box is disabled", test "a member sees every role box disabled"
  • On your own profile edit page there is no "Terug" link; Annuleren still leads to your profile → test/integration/users/profiles_test.rb test "your own profile edit page has no back link", existing test "the edit form ends in an action row of Annuleren and Opslaan"
  • On another user's profile show page, "Terug" still leads to /users → test/integration/users/profiles_test.rb test "another user's profile page links back to the users list"
  • A user with locale nl opening a transaction's edit page sees Dutch labels for debitor, creditor, category, note, booked at, interest at, type and the original import fields (and id) → test/integration/form_labels_test.rb test "the transaction form labels its fields in Dutch"
  • A user with locale nl sees Dutch labels for direction and parent category on the category form, and for purchase transaction on the chattel form → test/integration/form_labels_test.rb test "the category form labels direction and parent category in Dutch", test "the chattel form labels the purchase transaction in Dutch"
  • A user with locale it sees Italian labels for the same fields → test/integration/form_labels_test.rb test "the transaction form labels its fields in Italian", test "the category form labels direction and parent category in Italian", test "the chattel form labels the purchase transaction in Italian"
  • A user with locale nl gets <html lang="nl">; a user with locale en gets <html lang="en">; a signed-out visitor gets the default locale → test/integration/page_language_test.rb test "a Dutch user gets lang nl", test "an English user gets lang en", test "the admin layout declares the user's language", test "a signed-out visitor gets the default locale"
  • Invariant: en, nl and it hold the same label keys, enforced by i18n-tasks → test/i18n_test.rb test_no_missing_keys, test_files_are_normalized

eirvandelden and others added 10 commits October 24, 2026 08:56
Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
The host copy of appkit's login layout wrote four data-* theme
attributes even when theme_attributes is empty, so a signed-out page
carried data-theme="" and mvpa.css's :root:not([data-theme]) dark-mode
rules stopped matching. tag.html writes only the attributes that exist.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@eirvandelden
eirvandelden marked this pull request as ready for review October 24, 2026 07:23
@eirvandelden
eirvandelden merged commit bc7eccb into main Oct 24, 2026
7 checks passed
@eirvandelden
eirvandelden deleted the ui-review-bug-fixes branch October 24, 2026 07:32
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