Optimize db queries in several places

[fatkodima]

This PR reduces and avoids db queries at several places.
Primarily this is done by replacing COUNT.. by SELECT 1..., using already calculated values and replacing many queries by one (like .destroy vs delete).

[fatkodima]

ping @ryanto

[ryanto]

This looks good and I'm happy to merge.

There are some merge conflicts, do you mind fixing them?

[fatkodima]

Rebased.
CI hasn't started for some reason.

[ryanto]

Awesome thanks!

Looks like it's queued up here: https://travis-ci.org/github/ryanto/acts_as_votable/builds/744302627

I'll check on it in a few hours

Thanks again!

[ryanto]

Ok I think this one is ready to be merged! If you want to rebase again (sorry!) to get the new CI running that would be helpful.

[fatkodima]

Rebased.

[ryanto]

Thanks! Released as 0.13.1

[marckohlbrugge]

This PR broke our implementation of the gem.

We have extended ActsAsVotable::Vote such that a notification is created when a record is upvoted. When the record is destroyed, we also want to destroy the associated notifications. This is a simplified example of the code we use:

module ActsAsVotable
  class Vote
    has_many :notifications, as: :notifiable, dependent: :destroy
    after_commit :create_notifications, on: [:create]

    private

    def create_notifications
      # …
    end
  end
end

With the changes in this PR, the associated notifications no longer get destroyed when the vote is removed.

I think is due to the change of dependent: :destroy to dependent: :delete_all on Votable and Voter models. This deletes the Vote without running any of its callbacks or subsequent association destroyings.

What's the recommended way to implement such functionality with this PR's new behavior?

[fatkodima]

Do you need to remove notifications right after the vote deletion (to not show it somewhere or something like that) or just to cleanup the table?
If the second, you can create a periodic activity (rake task via cron, or a background job) to run something like

Notification.where.missing(:vote).delete_all

or something like that.

[marckohlbrugge]

@fatkodima I need to destroy it immediately, as otherwise we might try and show a notification with a missing association somewhere. Now I could add a check to see if the vote still exists, but that would really complicate the code a lot.

If there's no official solution, I'm leaning towards monkey-patching ActsAsVotable::Vote to revert back to the old behavior. But that could break again in future updates.

[fatkodima]

Got it. Yes, the monkey patching is the way to go or otherwise you can manually select and destroy notifications in the place where Voter or Votable or Vote (if there is a place where it is deleted not through the association) is deleted.

Something like

ActiveRecord::Base.transaction do
  notifications = # select notifications for user votes
  user.destroy
  notifications.delete_all
end