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

Avoid saving new translations that are empty, remove existing empty translations. #14

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

sepastian
Copy link

Activeadmin-globalize3 renders inputs for all possible translations. Consequently, params will contain translations for all attributes - even empty ones - and many empty translation records will be created.

This feature registers a before_filter in ActionController::Base that will

  1. remove new and empty translations from params before creating any empty records
  2. mark existing and empty translations in params for deletion

See #10, most credits go to codingluke and fabn, thanks!

… unsaved translations from params and b) mark empty saved translations for deletion.
Devise's Session does not translate, which led to an error in the original implementation.
@codingluke
Copy link
Contributor

Hi Sebastian, I like your Idea but im not so sure if this is the right way, cause with your code every ActiveRecord update/create triggers the before filter FilterEmptyTranslations.
Now this method just checks if in translations_attributes the First two attributes are empty. Is it like this it deletes the whole entry.

Now just imagine some guy has also an entry called "translations_attributes" but has not the structure form activeadmin-globalize3. Then it will delete the entries and the guy might be really confused ;)

I like to have the method centralized, but I think it should be configurable where the filter should trigger.

@codingluke
Copy link
Contributor

oh, I didn't see that commit. Great! in this case I like your implementation :)

sepastian@334c844

@sepastian
Copy link
Author

I added this later. But you are right, adding the filter to ActionController::Base is probably not the best of all options.

I will look into adding the filter automatically only to classes which do active_admin_translates.

use all? instead of mapping to boolean and checking for include?

use blank? instead of empty? on translated attributes
jdurand pushed a commit to jdurand/activeadmin-globalize3 that referenced this pull request Dec 3, 2013
… new translations that are empty, remove existing empty translations
jdurand pushed a commit to jdurand/activeadmin-globalize3 that referenced this pull request Dec 3, 2013
… new translations that are empty, remove existing empty translations
@jdurand
Copy link

jdurand commented Dec 3, 2013

This works pretty well. Any reason for this not being merged at this point?

@sepastian
Copy link
Author

I found at least one reason: ActiveAdmin::Globalize3::FilterEmptyTranslations#filter_empty_translations does not handle nested, translated attributes correctly.

I do not know out of my head how exactly the Rails params hash will look, if nested, translated attributes are used. However, looking at my implementation of filter_empty_translations, it becomes obvious that params[model][:translation_attributes] will not work for nested translations.

However, this could be fixed easily; all that is required is parsing the params hash recursively for :translation_attributes.

@fabn fabn mentioned this pull request Jun 7, 2014
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.

3 participants