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

Add tests for PR 1699 #1782

Merged
merged 3 commits into from
Dec 3, 2012
Merged

Conversation

ptn
Copy link
Contributor

@ptn ptn commented Nov 21, 2012

Pull request #1699 added the ability to work with two-levels-deep has_many relationships via the form builder. This adds tests for that.

@pcreux
Copy link
Contributor

pcreux commented Nov 28, 2012

Two level deep... Whoah!

Anyone can test this and confirm that it works?

def js_for_has_many(association, form_block, template)
association_reflection = object.class.reflect_on_association(association)
association_human_name = association_reflection.klass.model_name.human
placeholder = "NEW_#{association_human_name.upcase}_RECORD"
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@danielwestendorf It looks like what you fixed in #1789 is still present here, right? I'll make sure to fix it.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, I was going to comment on this pull but you beat me to it!

@ptn
Copy link
Contributor Author

ptn commented Nov 28, 2012

@pcreux I will provide a sample Rails app that demonstrates the feature.

@pcreux
Copy link
Contributor

pcreux commented Nov 28, 2012

@ptn This would be awesome!

ptn added 3 commits December 1, 2012 18:37
When generating the html that form builder's #has_many replaces via JS,
the placeholder that it looks for cannot contain spaces, otherwise, JS's
replace method won't replace all that it was supposed to.

This was originally implemented by @danielwestendorf in PR activeadmin#1789
@ptn
Copy link
Contributor Author

ptn commented Dec 2, 2012

@pcreux The sample app is here: https://github.com/ptn/aa-sample-1782

In the app, an Adventure has many Days which have many Activities; that's the 2-level nesting. To test:

  1. Clone the repo
  2. Run bundle install
  3. Run the migrations
  4. Start the rails server and go to /admin
  5. Create an adventure. You'll see that you can nest days inside adventures, and activities inside days. You'll have to visit the edit page of an adventure to verify that everything worked.

Since @jpmckinney merged the original code, maybe he'd like to read this too?

@pcreux
Copy link
Contributor

pcreux commented Dec 3, 2012

Awesome!

pcreux added a commit that referenced this pull request Dec 3, 2012
@pcreux pcreux merged commit fa6cfe0 into activeadmin:master Dec 3, 2012
@aakashd
Copy link

aakashd commented May 7, 2013

Hey @pcreux @ptn

I am facing the same issue. I checked out the sample rails application and it perfectly works on local environment.

I also downgraded all my gems (rails, activeadmin, jquery-rails) to match the ones in the sample application, but no luck. Here is the Gemfile.lock and the admin form config I am using https://gist.github.com/aakashd/5531908.

Can someone please take a look at this and let me know if I am missing something?

@ptn
Copy link
Contributor Author

ptn commented May 7, 2013

Did you test on master?
On May 7, 2013 6:20 AM, "aakash dharmadhikari" notifications@github.com
wrote:

Hey @pcreux https://github.com/pcreux @ptn https://github.com/ptn

I am facing the same issue. I checked out the sample rails application and
it perfectly works on local environment.

I also downgraded all my gems (rails, activeadmin, jquery-rails) to match
the ones in the sample application, but no luck. Here is the Gemfile.lock
and the admin form config I am using
https://gist.github.com/aakashd/5531908.

Can someone please take a look at this and let me know if I am missing
something?


Reply to this email directly or view it on GitHubhttps://github.com//pull/1782#issuecomment-17536471
.

@aakashd
Copy link

aakashd commented May 7, 2013

Yes, I tested this on master. Is there a particular part of the codebase that you would want me to check or debug?

@ptn
Copy link
Contributor Author

ptn commented May 7, 2013

I don't understand what issue it is that you are facing :) Try asking here:

#1311

and here:

#1699

Since a lot more people were active there.

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.

4 participants