Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Display translations on failed validation #2166

Closed
wants to merge 18 commits into from

Conversation

garethrees
Copy link
Member

HOTFIX (Moved from #2154)

Merge in to rails-3-develop

ac907d7 built the translations in the controller rather than the view.

In a request that contains no params for a translation and fails
validation the translation is no longer available to the view. This
commit builds the translations in the case of a failed validation.

Fixes issue brought up in #2140 (comment)

Also fixes #2112, #2113

@garethrees
Copy link
Member Author

I think this works in most cases now, but if the PublicBody fails validation the PublicBody::Translation is persisted without a public_body_id. #2142 should prevent this, but we'd have to include both in the hotfix.

@mysociety-pusher mysociety-pusher force-pushed the 2140-translations-failed-validation branch from 81227a0 to a5a20d3 Compare February 25, 2015 16:24
@mysociety-pusher mysociety-pusher force-pushed the 2140-translations-failed-validation branch from e4e1e33 to 4ff39fa Compare February 26, 2015 17:37
@@ -60,7 +83,17 @@ def update
def create
I18n.with_locale(I18n.default_locale) do
@category = PublicBodyCategory.new(params[:public_body_category])
# Build a translation for the default locale as it isn't available
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we can just call @category.translation here which seems to add the default locale in to the translations array:

> category = FactoryGirl.build(:public_body_category)
=> #<PublicBodyCategory id: nil, title: "Example Public Body Category 6", category_tag: "example_tag_6", description: "Example Public body Description 6">
> category.translations
=> []
> category.translation
=> #<PublicBodyCategory::Translation id: nil, public_body_category_id: nil, locale: "en", description: nil, title: nil, created_at: nil, updated_at: nil>
> category.translations
=> [#<PublicBodyCategory::Translation id: nil, public_body_category_id: nil, locale: "en", description: nil, title: nil, created_at: nil, updated_at: nil>]

@mysociety-pusher mysociety-pusher force-pushed the 2140-translations-failed-validation branch 3 times, most recently from e3f5521 to 205000f Compare March 3, 2015 16:53
Fixes submission of form containing both existing and new translations
Fixes submission of form containing both existing and new
translations
@mysociety-pusher mysociety-pusher force-pushed the 2140-translations-failed-validation branch from 205000f to a14ce58 Compare March 4, 2015 14:41
<% @category.ordered_translations.each do |translation| %>
<li>
<a href="#div-locale-<%= translation.locale.to_s %>" data-toggle="tab" >
<%= locale_name(translation.locale.to_s) || _("Default locale") %>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Probably translation.locale.to_s would be a better fallback here than _("Default locale") as this is not necessarily the default locale.

@crowbot
Copy link
Member

crowbot commented Mar 5, 2015

My only question here now is whether it's clear enough what the problem is when the category heading translations in particular fail validation. I've made some commits in #2219 that I think might help. 7929e1b seems like an obvious improvement to the admin interface in general. 533e99b breaks out the error messages and highlights the translation they belong to. ac207f7 fixes the issue that the fields on the default locale aren't getting highlighted because the errors are on the model itself, but probably going back to a pattern like 6b0a79a#diff-e9be0b7887688275035d3346ae523a01R18 is a cleaner fix.

@garethrees
Copy link
Member Author

going back to a pattern like 6b0a79a#diff-e9be0b7887688275035d3346ae523a01R18 is a cleaner fix.

Yeah, done.

Also highlighted the tab 3b0bfb5

@garethrees
Copy link
Member Author

I don't think these if statements are needed - should be handled by built-in validation

Ah yep, removed.

I'll drop 3b0bfb5, merge the fixups and make a PR against rails-3-develop

@garethrees
Copy link
Member Author

See #2166

@garethrees garethrees closed this Mar 18, 2015
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.

2 participants