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

Check for #390 to prevent application breakage. #451

Merged
merged 1 commit into from
Jan 13, 2014

Conversation

seuros
Copy link
Collaborator

@seuros seuros commented Jan 3, 2014

No description provided.

@@ -29,7 +29,7 @@ def self.columns
@acts_as_taggable_on_counter_columns ||= begin
db_columns = super
if _has_tags_counter_column?(db_columns)
Copy link
Collaborator

Choose a reason for hiding this comment

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

But, now I'm wondering why I wrote this _has_tags_counter_column method as the column is on Tag, not Tagging. And I guess the tests were passing for the wrong reason-- that the column existed, not that the counter_cache was turned on.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Though I suppose the columns call is still the best place for this check, as we can use it as a signal that we have db connectivity, rather than the value of it, itself.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I was half asleep when i did this PR, let me fix that.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Ditto :)

@bf4
Copy link
Collaborator

bf4 commented Jan 3, 2014

Looks good. I'd like to add a test for the counter_cache being added. The one that was added as part of this feature doesn't actually test it. It only tests for the presence of the column. Your thoughts?

@bf4
Copy link
Collaborator

bf4 commented Jan 9, 2014

I'd like to merge this.. can you help me finish it?

@seuros
Copy link
Collaborator Author

seuros commented Jan 9, 2014

Sorry, i forgot. I will do that right now.

@bf4
Copy link
Collaborator

bf4 commented Jan 10, 2014

Could you rebase to remove the extra commit?

bf4 added a commit that referenced this pull request Jan 13, 2014
[Fix] counter_cache, Check for #390 to prevent application breakage.
@bf4 bf4 merged commit f891175 into mbleigh:dgilperez-counter_cache Jan 13, 2014
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