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

pass object class name with a data attribute instead of parsing the input's name #52

Merged
merged 3 commits into from
Mar 16, 2015

Conversation

jamesmk
Copy link
Collaborator

@jamesmk jamesmk commented Mar 14, 2015

This fixes an issue I've run into with nested models, specifically from an engine. Passing anEngine::Userinstance through to simple_form or form_for will only create input names from the base class by default. This leave Judge trying to match Engine::User with User and returning a NullValidation.

My solution is to pass the class name directly through as a data attribute. Tests are passing and it works like a charm for nested and unested models. Let me know what you think.

thanks

@jamesmk
Copy link
Collaborator Author

jamesmk commented Mar 14, 2015

Well, rspec was passing :) I'll look into the JS test tomorrow.

@dannysperry
Copy link
Collaborator

Thanks @jamesmk, I'm curious what the returned name on your forms input is. I did a little testing with form_for/simple_form and it seems to be returning a name spaced class name. ex.

@object = NameSpace::User
simple_form_for @object do
  f.input :name
end

In my tests, @object would render an input.name of namespace_user[name]. Are you only getting user[name] while using an engine?

If Engines cause the element name to not be name spaced within FormBuilder or SimpleForm, It might make sense to have Judge keep its current classForName method and then look for data-judge-klass || classForName(el.name)

@jamesmk
Copy link
Collaborator Author

jamesmk commented Mar 15, 2015

@dannysperry thanks for the feedback.

I thought it might have been an issue with how the class was rendering in the element. It was assigned in the engine (you know the one) as @user = User.... I did a quick test and changed it to Namespace::User and it was still rendering user as the base. Might have been some class autoloading going on.

Even if Namespace::Model properly render as namespace_model, how would you parse that from UnderscoredModel which would render as underscored_model? That's why I though passing in the class directly was the most elegant solution.

Also, I didn't see a reason to keep classForName around as we can count on data-klass being present as much as data-validate so the method wouldn't be called and would just be code kruft.

@dannysperry
Copy link
Collaborator

Valid point on Namspace::Model. I agree there should be a constant klass attribute.

There are two helper methods called debracket and camelize. Those are no longer needed with the removal of classForName

Remove those and I can get this merged in.

@jamesmk
Copy link
Collaborator Author

jamesmk commented Mar 16, 2015

Good call, removed.

dannysperry added a commit that referenced this pull request Mar 16, 2015
pass object class name with a data attribute instead of parsing the input's name
@dannysperry dannysperry merged commit 8710f2c into judgegem:master Mar 16, 2015
@dannysperry
Copy link
Collaborator

+1 @jamesmk

@jamesmk
Copy link
Collaborator Author

jamesmk commented Mar 16, 2015

Thanks @dannysperry!

Since this gem is a dependency in my engine, there's no way link it directly to a repo. So for me to see the effect of this fix in production, I need a new version of the gem released. It looks like there's been a lot of activity since last release, any chance of getting an ETA on a new one? cc @joecorcoran

thanks again

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