-
Notifications
You must be signed in to change notification settings - Fork 313
clean up endorsements with no valid skills #285
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
Conversation
@@ -0,0 +1,11 @@ | |||
class CleanupEndorsements < ActiveRecord::Migration | |||
def up | |||
Endorsement.find_each(batch_size: 100) do |endo| |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we could use delete_all(condition)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I thought finding in batches wouldn't clog the memory.. Not sure about delete_all
way of handling though. You want me to rewrite it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think delete_all
would be the way to go since it would be a single SQL query without having to build up any ruby objects. But you'll have to change the query to something like:
Endorsement.where(endorsing_user_id: nil).delete_all
Not sure if that covers the case where there's a broken endorsing_user_id
foreign key.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Endorsement.delete_all(endorsing_user_id: [nil,''])
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
cool.. i ll rewrite then..
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.. created new migration just incase..
LGTM |
Nice work. Could you squash your commits ? |
Sure.. |
Alright.. So I squashed em and it is showing as only one commit in my local repo, but adds more and more while I push.. messed up somewhere.. Shall I ignore this PR altogether and start a new one since I am afraid the history would rewritten if I try squashing again.. Anyone has suggestions? |
Fine.. I am getting a fresh copy and would send a new PR.. let's dump this.. |
@avinoth: git push -f |
The rebase messed up.. got a clean copy and did force push.. 👍 |
Hello, I would rather implement find_in_batches inside a task instead of a migration: http://apidock.com/rails/ActiveRecord/Batches/find_in_batches Also with find_in_batches you can define the size of the batch to process. Cheers, Juan. |
…f corresponding skill is deleted
All set now.. changed the title and comments accordingly.. |
clean up endorsements with no valid skills
There are some rows in Endorsement where the Skills is not valid.
Make Endorsements delete if corresponding skill is deleted.
https://assembly.com/coderwall/bounties/491