Delete submissions and replies when a source is deleted #1440
Conversation
| @@ -4,7 +4,7 @@ | |||
| from cStringIO import StringIO | |||
| import unittest | |||
| import zipfile | |||
|
|
|||
| import pdb | |||
There was a problem hiding this comment.
Remove pdb import left over from debugging.
redshiftzero
force-pushed
the
pwplus-sub-src-delete
branch
from
fafd8ff to
cf16b69
Compare
| @@ -132,7 +132,8 @@ class Submission(Base): | |||
| "Source", | |||
| backref=backref( | |||
| 'submissions', | |||
| order_by=id)) | |||
| order_by=id, | |||
| cascade="delete")) | |||
There was a problem hiding this comment.
Nit: formatting. Either put all of the parameter to backref on one line, or left-align them. If you can fit them all of one line, that is preferable.
| # Submissions should be gone | ||
| results = db_session.query(Submission.source_id == source.id).all() | ||
| self.assertEqual(results, []) | ||
|
|
There was a problem hiding this comment.
I think you should also create a test reply as well, and verify that it is deleted as well (in order to test the change to class Reply).
There was a problem hiding this comment.
Ah, good point - I have now written this in a separate unit test (and added a helper function to setup test replies in the database).
redshiftzero
added
PR: Pending additional work
PR: Needs review
and removed
PR: Pending additional work
labels
|
Thanks for your review @garrettr! Let me know if these changes don't address your concerns. |
There was a problem hiding this comment.
#1467 and this PR conflict, but I think this one should be merged first--there are some useful helper functions here I can use in the tests there.
It doesn't appear that this code is actually removing the directories from the filesystem. At least in this development VM. I spent some time debugging why this might be--the store function looks right, so I suspect the problem might be with the Redis worker--I did notice in interactive testing the worker would fail because it was unable to find the store module, but that could be due to the options I passed rq worker being incorrect. Note that failures of Redis workers happen silently.
Once this is confirmed/resolved I believe you should check two more things to your tests:
- That submissions/replies are actually being removed from disk.
- The the source private and public keys are removed from the keyring (this seems to be worker, but tests would prevent future regressions).
| @@ -158,7 +158,10 @@ class Reply(Base): | |||
| order_by=id)) | |||
|
|
|||
| source_id = Column(Integer, ForeignKey('sources.id')) | |||
| source = relationship("Source", backref=backref('replies', order_by=id)) | |||
| source = relationship( | |||
There was a problem hiding this comment.
So, we've sort of discussed style in person, but I'll just share an opinion here, and maybe next time we're all in a room together someone will bring this up again. To me, using 4 lines here when you could be using 2
journalist = relationship("Journalist",
backref=backref('replies', order_by=id))
while still being at <80 chars, and clearly separating arguments, hurts readability of the source. My take is that you should separate expressions over the least lines possible as long as you respect these two things in the process. The idea is that you can fit more meaningful content on your screen at once.
To give some counterexamples, I feel like
journalist = relationship("Journalist", backref=backref('replies',
order_by=id))
while also respecting char limits, makes it less clear which arguments are to backref and which are to relationship. I don't believe in exact rules for this, and of course this is not some blocking thing for me, but I would like to hear others' ideas on this.
There was a problem hiding this comment.
I agree with your points here, and I definitely agree with you that the first is more readable than the second. I’ll note that:
source = relationship("Source",
backref=backref("replies", order_by=id, cascade="delete"))
is not PEP8 compliant due to limits on characters per line. Instead I decided to opt for the ‘hanging intent’ style used in many of the tests in #1417 for readability. Happy to discuss further.
Also: in order to address these kinds of style issues in the codebase, I suggest we tackle #47 and #886 soon and define and clean up the style in a few style-only PRs (I’m not going to change the lines you mentioned above in this PR since they’re not in the diff and I don’t want to confuse the history here, but the lines you point out are definitely something for a style PR once we’ve defined what constitutes good style for this project.)
This PR will delete submissions and replies linked to a source when a source is deleted.
redshiftzero
force-pushed
the
pwplus-sub-src-delete
branch
from
e4dab9e to
2461173
Compare
|
So it looks like the PGP key is indeed being deleted when the source is deleted (done in Check it out and let me know if this doesn’t address your concerns @fowlslegs! Also rebased on current develop. |
|
More good stuff @pwplus @redshiftzero!! Merging! |
This was an addition in #1440, but until we have a migration path for making changes like this to the database we cannot ship this to instances.
This PR is to merge in the work of @pwplus from #1190 which deletes submissions and replies when the source is deleted. I just added a unit test. Closes #1188.