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

Fixed bug with Rails 4 and association#to_sql #367

Merged
merged 1 commit into from
Aug 4, 2013
Merged

Fixed bug with Rails 4 and association#to_sql #367

merged 1 commit into from
Aug 4, 2013

Conversation

rsl
Copy link
Contributor

@rsl rsl commented Jun 7, 2013

You can see the issue here: https://coderwall.com/p/45ombq [as well as how I fixed it]. This problem raises itself in acts-as-taggable-on when you try something like:

Host.first.entries.all_tags
# OR
Host.first.entries.tag_counts_on(:foo)

I didn't include tests because when I ran the suite via rake appraisal:rails-4 there was a lot of errors and I wasn't sure whether committing to a red suite was any better. If you guys want, I totally can write tests for this.

@tilsammans
Copy link
Contributor

The tests in Travis are passing, so it looks good. But yes please provide tests for this, as I want to make sure it remains fixed in the future.

@rsl
Copy link
Contributor Author

rsl commented Jun 13, 2013

are the appraisal tests for rails-4 passing for you?

working on those tests.

@rsl
Copy link
Contributor Author

rsl commented Jun 13, 2013

two things. mostly separate.

  1. https://gist.github.com/rsl/06bca2322e9f8ae3a0fb is an example of what i get when i try to run rake appraisal:rails-4
  2. other than assert_nothing_raised i'm not sure what to test to be honest as i'm not introducing a single bit of new functionality but fixing a call to rails code. what are you looking for for a test for this? sorry if i'm being obtuse.

@tilsammans
Copy link
Contributor

Travis says it's good to merge: https://travis-ci.org/mbleigh/acts-as-taggable-on/builds/7885639

Unfortunately I have literally packed my bags to go on holidays. Will be back June 28 and can only take a proper look then I'm afraid.

@rsl
Copy link
Contributor Author

rsl commented Jun 13, 2013

all good. let's pick this back up then. i'm never sure what to do on tests for commits like this. thanks.

@tilsammans
Copy link
Contributor

👍

@ryanwood
Copy link

ryanwood commented Aug 4, 2013

Any progress on this?

tilsammans added a commit that referenced this pull request Aug 4, 2013
Fixed bug with Rails 4 and association#to_sql
@tilsammans tilsammans merged commit 98aecdd into mbleigh:master Aug 4, 2013
@tilsammans
Copy link
Contributor

Thanks, looks good.

These kinds of issues are probably solved using ARel throughout as well.

@ryanwood
Copy link

ryanwood commented Aug 4, 2013

Thanks @tilsammans!

tekniklr pushed a commit to tekniklr/acts-as-taggable-on that referenced this pull request Mar 19, 2021
Fixed bug with Rails 4 and association#to_sql
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.

3 participants