There are some small issues with the current nested attributes and
autosave implementation that I would like to address until Rails 3.
1) The first one was already dissused on the mailing list, which is
the use of "_delete" in accepts_nested_attributes_for when both the
DSL (:allow_destroy) and the behavior are destroy. I''ve created a
patch that just checks for _destroy instead of _delete with backwards
compatibility.
https://rails.lighthouseapp.com/projects/8994/tickets/2889
This is to avoid confusion and open room for more improvements in the
future.
2) How messages are created with nested attributes are hard to
translate and does not work for other languages. For instance:
model_name + " " + attribute_name + " " + message
Is not grammatically correct in portuguese and japanese. There is a
ticket about it on Lighthouse:
https://rails.lighthouseapp.com/projects/8994/tickets/2210
Although the ticket does not solve the whole problem. It only deals
with attributes at the beginning of the message, but not with the
errors messages. For instance, when you have a project which has many
tasks, it will search for messages at:
+ activerecord.errors.messages.project.tasks.name
And it does not fallback to:
+ activerecord.errors.messages.task.name
So if I customize the error message, I will have to do that in two
places.
3) Another problem that has bitten me frequently when using autosave
is that I don''t want error messages to be copied to the parent when
I''m giving both parent and child to error_messages_for. For that, I
usually need to create an after_validate callback that delete the
messages in the parent:
def after_validate
self.errors.each do |key, value|
next unless key.to_s =~ /child_/
self.errors.instance_variable_get(''@errors'').delete(key)
end
end
Right now, we deal with parent-child errors in two ways:
+ We copy the child errors to the parent (default for autosave);
+ We add an error to the parent saying that the child is invalid
(default for validate => true);
I would like to add a third one:
+ Do nothing with the error messages;
The patch would be simple, the tricky is how the DSL is going to be. A
suggestion (which definitely needs improvement):
has_many :tasks, :validate => :copy_errors # copy all messages
to the parent
has_many :account, :validate => true # add one message
that the parent is invalid
has_one :account, :validate => :skip_errors # validate but skip
the error messages
has_one :account, :validate => false # does not
validate
I already have a patch for (1), working on another patch for (2) and
would like to have some discussion on the DSL for (3) before working
on it.
What are your thoughts?
I discussed the problem with Eloy and we reached a solution that would
simplify both #2 and #3 cases.
The main problem is that ActiveRecord is copying the messages from the
child to the parent on autosave, while it shouldn''t.
error_messages_for should be the one responsible with showing those
messages properly:
error_messages_for :project, :association => :tasks
It would show errors in the project and in the associated tasks
objects. The I18n part could be dealt as:
activerecord:
errors:
format:
association: "{{association}} {{attribute}} {{message}}"
Which defaults to:
Tasks name can''t be blank
It''s simpler and faster since it does not rely on a fallback mechanism
for I18n.
On Jul 10, 11:37 am, José Valim <jose.va...@gmail.com>
wrote:> There are some small issues with the current nested attributes and
> autosave implementation that I would like to address until Rails 3.
>
> 1) The first one was already dissused on the mailing list, which is
> the use of "_delete" in accepts_nested_attributes_for when both
the
> DSL (:allow_destroy) and the behavior are destroy. I''ve created a
> patch that just checks for _destroy instead of _delete with backwards
> compatibility.
>
> https://rails.lighthouseapp.com/projects/8994/tickets/2889
>
> This is to avoid confusion and open room for more improvements in the
> future.
>
> 2) How messages are created with nested attributes are hard to
> translate and does not work for other languages. For instance:
>
> model_name + " " + attribute_name + " " + message
>
> Is not grammatically correct in portuguese and japanese. There is a
> ticket about it on Lighthouse:
>
> https://rails.lighthouseapp.com/projects/8994/tickets/2210
>
> Although the ticket does not solve the whole problem. It only deals
> with attributes at the beginning of the message, but not with the
> errors messages. For instance, when you have a project which has many
> tasks, it will search for messages at:
>
> + activerecord.errors.messages.project.tasks.name
>
> And it does not fallback to:
>
> + activerecord.errors.messages.task.name
>
> So if I customize the error message, I will have to do that in two
> places.
>
> 3) Another problem that has bitten me frequently when using autosave
> is that I don''t want error messages to be copied to the parent
when
> I''m giving both parent and child to error_messages_for. For that,
I
> usually need to create an after_validate callback that delete the
> messages in the parent:
>
> def after_validate
> self.errors.each do |key, value|
> next unless key.to_s =~ /child_/
>
self.errors.instance_variable_get(''@errors'').delete(key)
> end
> end
>
> Right now, we deal with parent-child errors in two ways:
>
> + We copy the child errors to the parent (default for autosave);
> + We add an error to the parent saying that the child is invalid
> (default for validate => true);
>
> I would like to add a third one:
>
> + Do nothing with the error messages;
>
> The patch would be simple, the tricky is how the DSL is going to be. A
> suggestion (which definitely needs improvement):
>
> has_many :tasks, :validate => :copy_errors # copy all messages
> to the parent
> has_many :account, :validate => true # add one message
> that the parent is invalid
> has_one :account, :validate => :skip_errors # validate but skip
> the error messages
> has_one :account, :validate => false # does not
> validate
>
> I already have a patch for (1), working on another patch for (2) and
> would like to have some discussion on the DSL for (3) before working
> on it.
>
> What are your thoughts?
> It''s simpler and faster since it does not rely on a fallback mechanism > for I18n.Sounds good to me, will eloy merge that into his branch? -- Cheers Koz
Koz,
The second step was talking with Sven Fuchs from I18n.
The string concatenation "{{association}} {{attribute}} {{message}}"
is a no-go in I18n and they are trying to eliminate it.
A solution that is still valid would be to nest messages on
error_messages_for. Let''s suppose that projects has many tasks, and
project has invalid title when task has an invalid name. When invoked
as:
error_messages_for :projects, :association => :tasks
It would print something as:
+ Title can''t be blank
+ Tasks
+ Name can''t be blank
It''s the same solution as above, just how we will show the error
messages has changed (which can be easily customized by overwriting
it).
It still sounds good to you? :)
On Jul 12, 6:04 am, Michael Koziarski <mich...@koziarski.com>
wrote:> > It''s simpler and faster since it does not rely on a fallback
mechanism
> > for I18n.
>
> Sounds good to me, will eloy merge that into his branch?
>
> --
> Cheers
>
> Koz
Yup, once José is done with it I will merge it in my branch so you can pull it in before 2.3.4. Eloy On 12 jul 2009, at 12:33, José Valim wrote:> > Koz, > > The second step was talking with Sven Fuchs from I18n. > The string concatenation "{{association}} {{attribute}} {{message}}" > is a no-go in I18n and they are trying to eliminate it. > > A solution that is still valid would be to nest messages on > error_messages_for. Let''s suppose that projects has many tasks, and > project has invalid title when task has an invalid name. When invoked > as: > > error_messages_for :projects, :association => :tasks > > It would print something as: > > + Title can''t be blank > + Tasks > + Name can''t be blank > > It''s the same solution as above, just how we will show the error > messages has changed (which can be easily customized by overwriting > it). > > It still sounds good to you? :) > > On Jul 12, 6:04 am, Michael Koziarski <mich...@koziarski.com> wrote: >>> It''s simpler and faster since it does not rely on a fallback >>> mechanism >>> for I18n. >> >> Sounds good to me, will eloy merge that into his branch? >> >> -- >> Cheers >> >> Koz > >
@koz and @eloy, here is the patch: https://rails.lighthouseapp.com/projects/8994-ruby-on-rails/tickets/2904-avoid-copying-errors-from-child-to-parent-on-autosave On Jul 12, 2:01 pm, Eloy Duran <eloy.de.en...@gmail.com> wrote:> Yup, once José is done with it I will merge it in my branch so you can > pull it in before 2.3.4. > > Eloy > > On 12 jul 2009, at 12:33, José Valim wrote: > > > > > Koz, > > > The second step was talking with Sven Fuchs from I18n. > > The string concatenation "{{association}} {{attribute}} {{message}}" > > is a no-go in I18n and they are trying to eliminate it. > > > A solution that is still valid would be to nest messages on > > error_messages_for. Let''s suppose that projects has many tasks, and > > project has invalid title when task has an invalid name. When invoked > > as: > > > error_messages_for :projects, :association => :tasks > > > It would print something as: > > > + Title can''t be blank > > + Tasks > > + Name can''t be blank > > > It''s the same solution as above, just how we will show the error > > messages has changed (which can be easily customized by overwriting > > it). > > > It still sounds good to you? :) > > > On Jul 12, 6:04 am, Michael Koziarski <mich...@koziarski.com> wrote: > >>> It''s simpler and faster since it does not rely on a fallback > >>> mechanism > >>> for I18n. > > >> Sounds good to me, will eloy merge that into his branch? > > >> -- > >> Cheers > > >> Koz