Add missing indexes from #7234 #7533

SuperTux88 · 2017-08-13T17:13:47Z

I added the indexes reviewed by @CSammy from #7234 (comment)

I also refactored the AccountDeleter to work with the person, that can be used in #6750 too.

I didn't add the unique index for the guid in the participations table. We already have an index there and we have a unique index for the target which should be enough. I planned to remove the guid column anyway soon, because we don't really need it, so there is no need to change the existing index to a unique index there.

cmrd-senya · 2017-08-13T17:19:20Z

lib/account_deleter.rb

@@ -69,10 +68,6 @@ def delete_standard_person_associations
    end
  end

-  def disconnect_contacts


Why did you remove this?

See commit-message: Also remove disconnect_contacts methods, because contacts are already removed with aspects memberships in before_destroy.

I had a problem after changing this to use person instead of diaspora_handle, because it tried to do this twice, but the second time it failed, because contact.user was nil.

And the tests ensures that contacts are empty after the AccountDeleter run, so that shouldn't be a problem when something changes in the future.

Actually before_destroy block in AspectMembership doesn't delete contacts if it's sharing: true. So it's not the same thing as disconnect_contacts in AccountDeleter. I have no idea why it worked for you, but it now fails with contacts not empty for me.

Here is my pry session output that shows what happens to a contact actually:

From: /home/senya/source/diaspora/app/models/aspect_membership.rb @ line 14 : 9: has_one :user, :through => :contact 10: has_one :person, :through => :contact 11: 12: before_destroy do 13: binding.pry => 14: if self.contact && self.contact.aspects.size == 1 15: self.user.disconnect(self.contact) 16: end 17: true 18: end 19: [2] pry(#<AspectMembership>)> self.contact.aspects.size => 1 [4] pry(#<AspectMembership>)> user.contacts => [#<Contact:0x0000000dfeff08 id: 1105, user_id: 1139, person_id: 4295, created_at: Mon, 14 Aug 2017 01:56:40 UTC +00:00, updated_at: Mon, 14 Aug 2017 01:56:40 UTC +00:00, sharing: true, receiving: true>] [5] pry(#<AspectMembership>)> next From: /home/senya/source/diaspora/app/models/aspect_membership.rb @ line 15 : 10: has_one :person, :through => :contact 11: 12: before_destroy do 13: binding.pry 14: if self.contact && self.contact.aspects.size == 1 => 15: self.user.disconnect(self.contact) 16: end 17: true 18: end 19: 20: def as_json(opts={}) [5] pry(#<AspectMembership>)> next From: /home/senya/source/diaspora/app/models/aspect_membership.rb @ line 17 : 12: before_destroy do 13: binding.pry 14: if self.contact && self.contact.aspects.size == 1 15: self.user.disconnect(self.contact) 16: end => 17: true 18: end 19: 20: def as_json(opts={}) 21: { 22: :id => self.id, [6] pry(#<AspectMembership>)> user.contacts.reload => [#<Contact:0x0000000f50cfa8 id: 1105, user_id: 1139, person_id: 4295, created_at: Mon, 14 Aug 2017 01:56:40 UTC +00:00, updated_at: Mon, 14 Aug 2017 01:57:25 UTC +00:00, sharing: true, receiving: false>]

Don't you mind me reverting the change with disconnect_contacts in the account deleter?

No, one half of the contacts are deleted via delete_contacts_of_me and the second half is deleted with deleting aspects in delete_standard_user_associations

No, delete_contacts_of_me are contacts of local people who have us in their aspects. The thing that isn't deleted are contacts where user is us, and person is someone else, while delete_contacts_of_me deletes contacts where we're a person. So that's a different thing.

You are right, when I add your change from the DataGenerator, it fails without disconnect_contacts ... We probably need better tests, because without the additional double-sided contact, it fails without .reload. I created #7538 to fix that, that should now cover both problems.

I need to sleep now, because I'm tired and I need to work tomorrow, but I'll have a closer look at this tomorrow, to improve the tests. Or if you have the time to improve the tests in your PR, you can also do it.

@denschub: can you merge #7538 as soon as travis is green?

We probably need better tests

What tests exactly? Some more tests for this method in spec/lib/account_deleter_spec.rb?

@SuperTux88, good night and thank you for your work!

In this case: tests with different combinations of contacts. Without disconnect_contacts it fails when you have double sided contacts, for me it failed without .reload. So something that covers that.

But I just tested, when I remove the .reload it now also fails with the updated DataGenerator, so it's maybe already enough. I just want to make sure that we don't remove the .reload or disconnect_contacts and something is broken again without a failing test.

But I'll have a closer look at this again tomorrow. #7538 should fix what I broke and as it looks it also makes the tests failing if I break something again, so for now that's good enough ;) 😴

cmrd-senya · 2017-08-13T17:21:45Z

Your refactorings conflict with the changes from #6750. What do you think we should merge first?

SuperTux88 · 2017-08-13T17:26:07Z

Your refactorings conflict with the changes from #6750. What do you think we should merge first?

I know, that's why I already mentioned it in the PR description. Since you changed AccountDeleter in #6750 to handle persons too, you can use the change here, and you don't need this if anymore there. So I think it's cleaner to merge this first, and the conflicts should be easy to resolve (I only removed diaspora_handle, and since you don't rely on that, that shouldn't be a problem for your PR).

SuperTux88 · 2017-08-13T17:30:24Z

Oh, I broke the tests last minute, because pronto told me so :( I'll fix that ...

cmrd-senya · 2017-08-13T17:35:55Z

Usage of person in AccountDeleter is what I originally was trying to do in #6783 (which I later included to #6726).

@denschub disapproved this change back then: #6783 (comment)

SuperTux88 · 2017-08-13T17:39:47Z

Hmm, I don't see why we shouldn't use person here, we already have it and can continue using it. It doesn't make sense to query an entity again.

cmrd-senya · 2017-08-13T17:43:20Z

I know, that's why I already mentioned it in the PR description. Since you changed AccountDeleter in #6750 to handle persons too, you can use the change here, and you don't need this if anymore there. So I think it's cleaner to merge this first, and the conflicts should be easy to resolve (I only removed diaspora_handle, and since you don't rely on that, that shouldn't be a problem for your PR).

Okay, I need to review this PR then to check it doesn't affect anything that I use in migration.

#6750 also changes some associations in objects and in account_deleter (method normal_ar_person_associates_to_delete and friends) so that's not only diaspora_handle. I should check this I think.

cmrd-senya · 2017-08-13T17:44:48Z

Hmm, I don't see why we shouldn't use person here, we already have it and can continue using it. It doesn't make sense to query an entity again.

That's what I thought either, but I changed it to this if approach just because of this comment.

denschub · 2017-08-13T17:48:31Z

Hmm, I don't see why we shouldn't use person here, we already have it and can continue using it.

If we already have inside the current worker, that's totally fine, but passing around objects between worker causes them to get serialized to redis, which is a bit of a bummer. However, it does not seem like we end up with that here, so I may made a brainfart in the comment back then.

SuperTux88 · 2017-08-13T17:49:27Z

Okay, I need to review this PR then to check it doesn't affect anything that I use in migration.

I checked while editing this (but that was before your latest push), and you extacted the close_user to a new method, but that is an easy change and can be resolved (you need to remove disconnect_contacts there too).

I didn't change anything normal_ar_person_associates_to_delete, but I changed from %i() to %i[], because it was really hard to read because rubymine adds curly yellow lines when there is a rubocop error, and I don't see if there is a "_" or a " " ;) I but I can revert that back if you like.

SuperTux88 · 2017-08-13T17:52:46Z

Also, tests fixed (travis is still busy with your PR) :)

denschub

Take this as approved once travis is green.

cmrd-senya · 2017-08-13T18:00:09Z

spec/integration/account_deletion_spec.rb

    subject.reload
  end

  context "of local user" do
-    subject(:user) { FactoryGirl.create(:user_with_aspect) }


I'd rahter not change this part here since it will be hard to rebase. Maybe it's better to add this change on top of my PR instead?

(this applies to most of the changes in this file, maybe excluding line 3)

Also remove `disconnect_contacts` methods, because contacts are already removed with aspects memberships in `before_destroy`.

See diaspora#7234

Add missing indexes from #7234

SuperTux88 added this to the 0.7.0.0 milestone Aug 13, 2017

cmrd-senya reviewed Aug 13, 2017

View reviewed changes

SuperTux88 force-pushed the add-indexes branch from aca64a1 to 75b69a6 Compare August 13, 2017 17:33

Remove diaspora_handle from AccountDeletions and add unique index

245ad9e

SuperTux88 force-pushed the add-indexes branch from 75b69a6 to d535ab9 Compare August 13, 2017 17:43

denschub approved these changes Aug 13, 2017

View reviewed changes

cmrd-senya reviewed Aug 13, 2017

View reviewed changes

SuperTux88 added 4 commits August 13, 2017 20:10

Use person for AccountDeleter

7ffd787

Also remove `disconnect_contacts` methods, because contacts are already removed with aspects memberships in `before_destroy`.

Add missing indexes

0577bf8

See diaspora#7234

Cleanup invalid aspects and set unique index

1d5460c

Add missing unique indexes from diaspora#7234

dbde75a

SuperTux88 force-pushed the add-indexes branch from d535ab9 to dbde75a Compare August 13, 2017 18:10

cmrd-senya approved these changes Aug 13, 2017

View reviewed changes

SuperTux88 merged commit dbde75a into diaspora:develop Aug 13, 2017

SuperTux88 added a commit that referenced this pull request Aug 13, 2017

Merge pull request #7533 from SuperTux88/add-indexes

9adcca2

Add missing indexes from #7234

SuperTux88 deleted the add-indexes branch August 13, 2017 18:59

SuperTux88 mentioned this pull request Aug 14, 2017

Re-add disconnect_contacts in AccountDeleter #7538

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add missing indexes from #7234 #7533

Add missing indexes from #7234 #7533

SuperTux88 commented Aug 13, 2017

cmrd-senya Aug 13, 2017

SuperTux88 Aug 13, 2017

SuperTux88 Aug 13, 2017

cmrd-senya Aug 14, 2017

cmrd-senya Aug 14, 2017

cmrd-senya Aug 14, 2017

SuperTux88 Aug 14, 2017

cmrd-senya Aug 14, 2017

cmrd-senya Aug 14, 2017

SuperTux88 Aug 14, 2017

cmrd-senya commented Aug 13, 2017

SuperTux88 commented Aug 13, 2017

SuperTux88 commented Aug 13, 2017

cmrd-senya commented Aug 13, 2017

SuperTux88 commented Aug 13, 2017

cmrd-senya commented Aug 13, 2017

cmrd-senya commented Aug 13, 2017

denschub commented Aug 13, 2017

SuperTux88 commented Aug 13, 2017 •

edited

Loading

SuperTux88 commented Aug 13, 2017

denschub left a comment

cmrd-senya Aug 13, 2017

cmrd-senya Aug 13, 2017

Add missing indexes from #7234 #7533

Add missing indexes from #7234 #7533

Conversation

SuperTux88 commented Aug 13, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cmrd-senya commented Aug 13, 2017

SuperTux88 commented Aug 13, 2017

SuperTux88 commented Aug 13, 2017

cmrd-senya commented Aug 13, 2017

SuperTux88 commented Aug 13, 2017

cmrd-senya commented Aug 13, 2017

cmrd-senya commented Aug 13, 2017

denschub commented Aug 13, 2017

SuperTux88 commented Aug 13, 2017 • edited Loading

SuperTux88 commented Aug 13, 2017

denschub left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SuperTux88 commented Aug 13, 2017 •

edited

Loading