Fix error state is not visible when translation is missing#16157
Fix error state is not visible when translation is missing#16157andreslucena merged 13 commits intodecidim:developfrom
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughDispatches an initial "change" event from the language-change controller on connect, marks the first locale with errors as the active/selected locale in language selectors, adds tests for the controller and admin organization form validation, and updates the spelling exceptions list. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Select as "Locale Select"
participant Controller as "LanguageChangeController"
participant Panels as "Locale Panels"
User->>Select: has current locale selected
Select->>Controller: controller connects
Controller->>Select: add "change" listener
Controller->>Select: dispatch "change" event
Controller->>Panels: activate panel for selected locale
Panels-->>User: render active panel, set aria-hidden accordingly
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Pull request overview
This PR fixes a UX issue where validation errors on multilingual form fields are not visible when the user is viewing the form in a locale different from the one with the error. The solution automatically switches the language selector dropdown to the first locale with an error, making the error immediately visible to the user.
Changes:
- Modified language selector dropdown to mark error locales as
selected - Added automatic tab switching on page load by dispatching a change event when the controller connects
- Added system test to verify error locale is automatically displayed
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| decidim-core/lib/decidim/form_builder.rb | Sets selected attribute on dropdown options for locales with errors |
| decidim-core/app/packs/src/decidim/controllers/language_change/controller.js | Dispatches change event on connect to activate the selected tab |
| decidim-admin/spec/system/admin_manages_organization_spec.rb | Adds system test verifying automatic error locale selection |
| .github/actions/spelling/expect.txt | Adds Catalan words used in the test |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
decidim-core/app/packs/src/decidim/controllers/language_change/controller.js
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
decidim-admin/spec/system/admin_manages_organization_spec.rb (2)
108-111: Consider adding an assertion for the(error!)suffix in the dropdown option.The
is_errorbranch inlanguage_selector_selectchanges the option title to"name_with_error"(e.g., "English (error!)"). The test confirms the panel is visible and the field shows the blank-error message, but doesn't verify that the dropdown option itself is re-labeled. Addinghave_select("organization-name-tabs", with_options: [/English.*error/i])(or the exact translated string) would fully exercise the title-change logic.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@decidim-admin/spec/system/admin_manages_organization_spec.rb` around lines 108 - 111, Test currently checks panel visibility and blank-error message but omits asserting the dropdown option label change from the language_selector_select is_error branch; update the spec in admin_manages_organization_spec.rb to also assert the language tab select (e.g., "organization-name-tabs") contains the error-marked option by adding an expectation like have_select("organization-name-tabs", with_options: [/English.*error/i] or the exact translated "English (error!)") so the name_with_error path in language_selector_select is exercised and verified.
96-96: Test description doesn't reflect what's actually being tested.
"displays the form for official language"is ambiguous. The test is specifically validating that after submitting with a missing English translation while in the Catalan locale, the error is surfaced by auto-selecting the English panel.📝 Suggested rename
- it "displays the form for official language" do + it "auto-selects the errored locale's panel after submitting with a missing primary-locale translation" do🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@decidim-admin/spec/system/admin_manages_organization_spec.rb` at line 96, The example description for the spec example in the it block currently reads "displays the form for official language" but is ambiguous; rename the it description to clearly state the behavior being validated (e.g., "selects the English translation panel when English translation is missing after submitting in Catalan locale") so readers know the test checks that submitting with a missing English translation in the Catalan locale auto-selects the English panel; update only the example string in the it block in admin_manages_organization_spec.rb to the new clearer description.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@decidim-core/lib/decidim/form_builder.rb`:
- Around line 805-816: Precompute the first locale that has an error before
generating options so only that option gets selected: call
error?(name_with_locale(name, locale)) or similar to find the first errored
locale, store it (e.g., first_error_locale), then in the loop that builds option
tags (the block using locales.each_with_index and content_tag(:option, ...)) set
selected: true only when locale == first_error_locale; keep using
name_with_locale, error?, sanitize_tabs_selector and tabs_id as in the current
code to build values and ids.
- Around line 807-810: The form builder
(decidim-core/lib/decidim/form_builder.rb) calls I18n.t("name_with_error",
scope: "locale") when rendering a language selector with errors, but 37 locale
YAMLs are missing that key; add a name_with_error entry under the same "locale"
scope in each of the listed locale files (match the structure and style of the
existing "name" key in those files) so that I18n.t("name_with_error", scope:
"locale") returns a proper translated string instead of "translation missing".
---
Nitpick comments:
In `@decidim-admin/spec/system/admin_manages_organization_spec.rb`:
- Around line 108-111: Test currently checks panel visibility and blank-error
message but omits asserting the dropdown option label change from the
language_selector_select is_error branch; update the spec in
admin_manages_organization_spec.rb to also assert the language tab select (e.g.,
"organization-name-tabs") contains the error-marked option by adding an
expectation like have_select("organization-name-tabs", with_options:
[/English.*error/i] or the exact translated "English (error!)") so the
name_with_error path in language_selector_select is exercised and verified.
- Line 96: The example description for the spec example in the it block
currently reads "displays the form for official language" but is ambiguous;
rename the it description to clearly state the behavior being validated (e.g.,
"selects the English translation panel when English translation is missing after
submitting in Catalan locale") so readers know the test checks that submitting
with a missing English translation in the Catalan locale auto-selects the
English panel; update only the example string in the it block in
admin_manages_organization_spec.rb to the new clearer description.
There was a problem hiding this comment.
🧹 Nitpick comments (1)
decidim-core/app/packs/src/decidim/controllers/language_change/language_change.test.js (1)
76-87: LGTM — test correctly covers the newconnect()dispatch-change behavior.One optional consistency note: the sibling
handleChangetest at line 92 usesselectElement.value = "#update_organization-name-tabs-name-panel-1"to set the selection. Using the same idiom here would make the two tests easier to compare at a glance.♻️ Optional style alignment
- const options = selectElement.querySelectorAll("option"); - options[1].selected = true; + selectElement.value = "#update_organization-name-tabs-name-panel-1";🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@decidim-core/app/packs/src/decidim/controllers/language_change/language_change.test.js` around lines 76 - 87, The test sets selection by toggling options[1].selected, which is inconsistent with the sibling test that uses selectElement.value; update this test to set selectElement.value = "#update_organization-name-tabs-name-panel-1" before calling controller.disconnect() and controller.connect() so it matches the idiom used in the handleChange test and still triggers controller.connect() behavior affecting panel0/panel1 and their ariaHidden/is-active assertions.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In
`@decidim-core/app/packs/src/decidim/controllers/language_change/language_change.test.js`:
- Around line 76-87: The test sets selection by toggling options[1].selected,
which is inconsistent with the sibling test that uses selectElement.value;
update this test to set selectElement.value =
"#update_organization-name-tabs-name-panel-1" before calling
controller.disconnect() and controller.connect() so it matches the idiom used in
the handleChange test and still triggers controller.connect() behavior affecting
panel0/panel1 and their ariaHidden/is-active assertions.
|
@alecslupu I'd expect that the behavior is the same for the organizations that have less than 4 locales: 
Can you fix that please? Thanks |
|
This is also solving #16195 |
f190df3
|
@andreslucena this is ready for another round |
|
Removing the v0.30 release label, as we would need the Stimulus controller a. we don't have Stimulus So, too much time to invest for fixing this for v0.30. We will skip this. |
🎩 What? Why?
In this PR i am fixing the an issue when there is an error on a i18n field and not visible if the user has other locale.
📌 Related Issues
Link your PR to an issue
Testing
📷 Screenshots
Summary by CodeRabbit
Improvements
Tests
Chores