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

Fixes issue #5625 #5640

Merged
merged 1 commit into from Feb 20, 2015

Conversation

@zachrab
Copy link
Contributor

commented Feb 9, 2015

@jhass Please let me know if this is all that's needed for this fix.

@jhass

This comment has been minimized.

Copy link
Member

commented Feb 9, 2015

Okay, please do two more things:

Add a spec here that starts with a user which has the field set to false and asserts that after calling the method, it's set to true. Maybe also backfill a test for the fields that are set to false there.

Add a migration that sets it to true for all already closed accounts. See here for how to select these (the blank username is indicating an old invite, not a closed account).

@zachrab

This comment has been minimized.

Copy link
Contributor Author

commented Feb 9, 2015

@jhass What do you mean by "backfill a test for the fields that are set to false there"?

@@ -485,10 +485,10 @@ def clear_account!
self[field] = nil
end
[:getting_started,
:disable_mail,
:show_community_spotlight_in_stream].each do |field|

This comment has been minimized.

Copy link
@jhass

jhass Feb 10, 2015

Member

I can't seem to spot an existing test for these fields, that's what I mean with backfilling ;)

closed_users = User.joins(:person).where(people: {closed_account: true})
closed_users.each do |closed_user|
closed_user.update_attribute(:disable_mail, true)
end

This comment has been minimized.

Copy link
@jhass

jhass Feb 10, 2015

Member

Let's use update_all on the relation here, that should be much faster.

@zachrab zachrab force-pushed the zachrab:5625-disable-mail-to-deleted-user branch from 5f0ec2c to 2d5203e Feb 10, 2015

@jhass jhass added this to the next-major milestone Feb 11, 2015

@jhass

This comment has been minimized.

Copy link
Member

commented Feb 11, 2015

Please update db/schema.rb to the new migration. Since the migration contains no schema changes, you shouldn't commit them if you have any. I also think it's small enough to be rebased into a single commit:

git remote add upstream git://github.com/diaspora/diaspora.git
git fetch upstream
git checkout 5625-disable-mail-to-deleted-user
git rebase -i upstream/develop
# Leave the first line at pick, choose squash for the others
bin/rake db:migrate
git commit --amend -p db/schema.rb
git push -f origin 5625-disable-mail-to-deleted-user

The code itself looking good.

@jhass

This comment has been minimized.

Copy link
Member

commented Feb 18, 2015

@zachrab ping, still interested in getting this merged ;)

Set disable_mail to true
Add #clear_account! disable mail spec

Add migration for disabling mail for all closed accounts

Change migration to use #update_all for disable_mail attribute

Add #clear_account! false fields spec

@zachrab zachrab force-pushed the zachrab:5625-disable-mail-to-deleted-user branch from 2d5203e to f695b5d Feb 20, 2015

@zachrab

This comment has been minimized.

Copy link
Contributor Author

commented Feb 20, 2015

@jhass sorry for the delay!

@jhass

This comment has been minimized.

Copy link
Member

commented Feb 20, 2015

Thank you!

@jhass jhass merged commit f695b5d into diaspora:develop Feb 20, 2015

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
hound Hound has reviewed the changes.

jhass added a commit that referenced this pull request Feb 20, 2015

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.