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

Set updated_at when executing counter_culture_fix_counts #210

Closed
AlexVPopov opened this issue Jan 25, 2018 · 6 comments
Closed

Set updated_at when executing counter_culture_fix_counts #210

AlexVPopov opened this issue Jan 25, 2018 · 6 comments

Comments

@AlexVPopov
Copy link
Contributor

AlexVPopov commented Jan 25, 2018

class Foo < AR::Base
  has_many :bars
end

class Bar < AR::Base
  belongs_to :foo
  counter_culture :foo, touch: true
end

and

Bar.counter_culture_fix_counts

I think in this case also the updated_at field of the foos needs to be updated.

Does this make sense to you?

@AlexVPopov
Copy link
Contributor Author

Currently, as I workaround, in a migration, I'd put Foo.find_each(&:touch).

@magnusvk
Copy link
Owner

magnusvk commented Feb 6, 2018

Yeah, that makes sense. Feel free to submit a PR. 😄

@AlexVPopov
Copy link
Contributor Author

@magnusvk I'm willing to submit a PR. What I've done so far:

  • forked the project;
  • cloned locally the fork;
  • ran rspec spec

I get the following failure:

Failures:

  1) CounterCulture should fix a STI counter cache correctly
     Failure/Error: expect(company.twitter_reviews_count).to eq(1)

       expected: 1
            got: 0

       (compared using ==)
     # ./spec/counter_culture_spec.rb:1018:in `block (2 levels) in <top (required)>'

Finished in 2.31 seconds (files took 1.55 seconds to load)
35 examples, 1 failure

Failed examples:

rspec ./spec/counter_culture_spec.rb:1003 # CounterCulture should fix a STI counter cache correctly

After some investigation I found out that the failure is cause by the after_create callback in Review. When I comment out this callback, the test passes. Also, if I put the callback after:

# TwitterReview
counter_culture [:user, :manages_company]

that is, I move it from Review to TwitterReview (for the sake of executing it after counter_culter has finished updating the counters) the test also passes. I am currently investigating this, but it appears that after after_create :update_some_text is ran, @_counter_culture_active is not properly set to false in _wrap_in_counter_culture_active(&block), so that when it gets to decide whether to update the counter for twitter_reviews_count in Company, @_counter_culture_active is still true and thus does not execute the block.

Before I continue my investigation - can you tell me if the test fails on your side as well?

I'm running ruby 2.4.1 and rails 5.1.5.

https://github.com/magnusvk/counter_culture/blob/master/lib/counter_culture/extensions.rb#L78

@AlexVPopov
Copy link
Contributor Author

O.K., this does not happen on 5.1.4., it only happens on 5.1.5. I think the problem is somehow related to rails/rails#27780

@magnusvk
Copy link
Owner

Yeah, this used to work fine with Rails 5.1. I haven’t had a chance to look into this yet but will try to soon. Thanks for digging. For purposes of your PR feel free to just test against 5.1.4 for now where the test is green.

@magnusvk
Copy link
Owner

magnusvk commented May 2, 2018

Fixed by #212

@magnusvk magnusvk closed this as completed May 2, 2018
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

No branches or pull requests

2 participants