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

all check_boxes checked if collection given as array #674

Merged
merged 4 commits into from
Sep 10, 2011
Merged

all check_boxes checked if collection given as array #674

merged 4 commits into from
Sep 10, 2011

Conversation

cthiel
Copy link
Contributor

@cthiel cthiel commented Sep 5, 2011

I'm running into an issue where all check_boxes are being checked no mather what the object actually is.

The bug seems to be due to https://github.com/justinfrench/formtastic/blob/master/lib/formtastic/inputs/check_boxes_input.rb#L156
which only tries to set selected_items if collection is not given or is not an array.

I have something like this for my users, which used to work before:

<%= f.input :roles, :as => :check_boxes, :collection => User::ROLES.collect {|r| [r.humanize, r]} %>

Now @user.roles could be ["admin"] and User::ROLES.collect {|r| [r.humanize, r]} is
[["Admin", "admin"], ["User", "user"]] -- still both check_boxes are checked.

I'm running the latests formtastic from master on Rails 3.1, Ruby 1.8.7.

@justinfrench
Copy link
Member

Hi,

If you're currently using master (bundling from git), could you please try bundling one of the previous RC gems (e.g. 2.0.0.rc4 and/or rc3)? There have been some changes on master since rc5 shipped very much related to this part of the system, and it'd be helpful to narrow it down.

@cthiel
Copy link
Contributor Author

cthiel commented Sep 4, 2011

It's broken in 2.0.0.rc5 and working as expected in 2.0.0.rc4. Want me to git bisect to find the commit that broke it?

@justinfrench
Copy link
Member

Sure! How about a patch too :) I have an extra busy week, so any help I can get hunting this down would be great.

@cthiel
Copy link
Contributor Author

cthiel commented Sep 4, 2011

OK, I'll look into it tomorrow :)

@cthiel
Copy link
Contributor Author

cthiel commented Sep 5, 2011

commit 65a1916 introduced this issue.

@cthiel
Copy link
Contributor Author

cthiel commented Sep 5, 2011

@timothyklim made more changes to that code. Maybe he can explain what https://github.com/justinfrench/formtastic/blob/master/lib/formtastic/inputs/check_boxes_input.rb#L156 is about?

@cthiel
Copy link
Contributor Author

cthiel commented Sep 5, 2011

Check out my proposed fix. Unfortunately the specs for check_boxes are all failing (also before changing this), so I can hardly verify the fix with them.

I have tested this in my app, where it is working fine now!

@timothyklim
Copy link
Contributor

Hi! Check this #675

@cthiel
Copy link
Contributor Author

cthiel commented Sep 5, 2011

Well, it's up to Justin now to decide on which pull request to take ;)

@cthiel
Copy link
Contributor Author

cthiel commented Sep 5, 2011

IMO, it would be best to back this up with specs, but that requires getting all the checkboxes specs to pass first.

@justinfrench
Copy link
Member

@Thiel, it's your bug report, and I've got a massive week at work, so if you're up for it, I'd like to defer it to you to tell me what to merge in and when. "many thanks" (or "no problem") in advance :)

@cthiel
Copy link
Contributor Author

cthiel commented Sep 5, 2011

Well, I have tested both pull requests (#675 and this one) and both fix the issue I reported. Let's defer merging for a bit until specs are fixed up for check_boxes. I saw references to that in #673.

P.S.: you wanted to say "@cthiel" in your last comment :)

@justinfrench
Copy link
Member

@cthiel #673's reference to check boxes input was a mistake... he meant hidden inputs. We're also not planning to merge that stuff in 'till after 2.0 most likely, so whatever is up with check boxes is new… what's the deal?

@cthiel
Copy link
Contributor Author

cthiel commented Sep 5, 2011

My first fix wasn't correct, that's why other tests were failing. I now pulled in the fix from @timothyklim and also added a spec for this issue. Should be ready to merge now.

@cthiel
Copy link
Contributor Author

cthiel commented Sep 9, 2011

So? How do we proceed with this?

@justinfrench
Copy link
Member

Sorry, big week. If this request is fine to merge, I'll do it straight away.

@cthiel
Copy link
Contributor Author

cthiel commented Sep 10, 2011

IMO it's ready.

justinfrench added a commit that referenced this pull request Sep 10, 2011
Fix all check_boxes being checked if collection given as array
@justinfrench justinfrench merged commit a77267c into formtastic:master Sep 10, 2011
@cthiel
Copy link
Contributor Author

cthiel commented Sep 10, 2011

Many thanks!

@justinfrench
Copy link
Member

No, Thank you!

jcf pushed a commit to jcf/formtastic that referenced this pull request Jan 27, 2012
jcf pushed a commit to jcf/formtastic that referenced this pull request Jan 27, 2012
… passed to :collection"

This reverts commit bd218ed.
jcf pushed a commit to jcf/formtastic that referenced this pull request Jan 27, 2012
jcf pushed a commit to jcf/formtastic that referenced this pull request Jan 27, 2012
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants