Fixed fields_for with has_many associations #516

Merged
merged 1 commit into from Apr 2, 2013

Projects

None yet

5 participants

@arr-ee
Contributor
arr-ee commented Mar 29, 2013

Fixes #504.
Kudos to @romanbsd for his tips on this at #462.

It's a lot of hoops jumping, maybe it'd be better to somehow (more reliably) map fields to validators on server side. Data attributes or something.

@coveralls

Coverage remained the same when pulling 6644783 on arr-ee:fix_fields_for_with_has_many_relation into b674e02 on bcardarella:3-2-stable.

View Details

@bcardarella
Contributor

@arr-ee you've got push/pull access make the magic happen! :)

@arr-ee
Contributor
arr-ee commented Mar 29, 2013

@bcardarella, I'm not quite sure on this one. Would love to get some comments from you and/or @romanbsd.

Also, thanks for the faith in me! I'll try to not fuck everything up in here :neckbeard:

@bcardarella
Contributor

This is as good a place as any to go over this. I've really come to the conclusion that the approach that ClientSideValidaitons takes is incorrect. The idea is very good, but how the validation happens on the client is not. A very large mess of inspecting input fields that may or may not be the correct ones, relying upon name attributes and the problem even gets worse when you start to include nested attributes and associations.

After having worked in Ember for the past 2 months I am convinced that having the data represented by models on the client is the only way to go. There is too much of a mess attempting to parse the data out of form elements. Most of that form parsing code in ClientSideValdiations is hackey at best.

So, where does this leave things? I'm not quite sure. We could continue to head down the rabbit hole, fixing the bugs as the gem does work for most use cases. But I am wanting to push towards a model based approach on the client. I need more time to consider how this would work in the absence of a MVC on the client.

@arr-ee
Contributor
arr-ee commented Mar 29, 2013

@bcardarella, I saw the same words from you somewhere on the issue tracker and yes, I guess that would be a better approach.

I need more time to consider how this would work in the absence of a MVC on the client.

We always can use some MVC framework (say, Backbone). It's pretty small and if it fits the bill then why not.

I don't have a lot of experience with client-side MVC, but I'll try to go through dockyard/ember-validations and see what're you doing there.

As for this PR, I'd merge it for now if you don't mind, since it's better get fixed now — nobody knows how long the migration will take.

@arr-ee arr-ee merged commit 5f4552e into DavyJonesLocker:3-2-stable Apr 2, 2013

1 check passed

default The Travis build passed
Details
@jeroenj
jeroenj commented Jun 19, 2013

I think there is still an issue with deeply nested has_many attributes. I'll try to explain it:

So for this example I have an Account object that has many Applications which have many Subdomains.

The attributes in this example would be defined as account[applications_attributes][0][subdomains_attributes][1371632493689][url]. In this case client_side_validations would define the validators for account[applications_attributes][][subdomains_attributes][][url] in the script tag in the form.

However this change in the regexp would transform it to account[applications_attributes][subdomains_attributes][][url].

I'm guessing that you have tested against a has_many and then has_one associations?

Anyway, for now I am using a simple regexp to make it work with multiple has_many associations: name.replace(/\[(\w+_attributes)\]\[(\w+)\]/g, "[$1][]");. However this will break for has_many-has_one validations.

I'll have a look to see what's going one with has_one associations and how we could fix it for both at once.

@jeroenj
jeroenj commented Jun 19, 2013

What could work is using /\[(\w+_attributes)\]\[(\d+)\] as the regexp: using \d for the id part. Is there any usecase where you would have a string as an id for the nested attributes array?

@romanbsd
Contributor

Like @bcardarella pointed out - mongo's ids are strings.

@jeroenj
jeroenj commented Jun 19, 2013

Okay, totally forgot about that. I have a feeling that it is going to be impossible to find out the correct attribute names.

The longer I have used this gem, the more I agree that it isn't the right approach.

@romanbsd
Contributor

Well, I tend to agree. But for mongo's id one could use /\A[0-9a-f]{24}\Z/

@jeroenj
jeroenj commented Jun 19, 2013

Okay, so basically this regexp could work: \[(\w+_attributes)\]\[(\d+|[0-9a-f]{24})\]. However, if you would have 24 character a-f attributes it would again be wrong.
Anyway, I've switched to the \d notation in this project for now since we don't use mongo for this one.

@romanbsd
Contributor

Yeah, we also switched to \d+ for the same reason

@jeroenj
jeroenj commented Jun 19, 2013

I'd say there is a very small chance that somebody would ever have such an attribute name.

Maybe it is a better fix than what is happening now, since I guess more people are using nested has_many's than 24 character a-f attribute names.

@jeroenj
jeroenj commented Jun 19, 2013

@romanbsd I'd be happy to hear your opinion about my fix in #557.

@romanbsd
Contributor

@jeroenj 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment