Closed Bug 1112628 Opened 6 years ago Closed 4 years ago
disallow deletion of releases that have a rule
.mapping or rule .whitelist pointing at them
Right now it's possible to delete releases that still have rules pointing to them. This is bad because it leaves the rules in a very weird state. One could easily remove a release that is active without knowing it because the UI doesn't even give any sort of warning. We should probably disallow this at the database schema level with a foreign key. IIRC this wasn't implemented originally because sqlite doesn't support them. I'm sure we can find a way to work around this.
Summary: disallow deletion of releases that have a rule.mapping pointing at them → disallow deletion of releases that have a rule.mapping or rule.whitelist pointing at them
Not too sure if it is desired this way. But still would like to get some review.
Attachment #8724159 - Flags: review?(bhearsum)
Comment on attachment 8724159 [details] [diff] [review] 0001-bug-1140262-disallow-deletion-of-releases-that-have-.patch Review of attachment 8724159 [details] [diff] [review]: ----------------------------------------------------------------- This is a very good start! It looks like sqlite actually supports foreign keys as of 3.6.19 (which is ~6 years old). I'd prefer database level enforcement of this because it minimizes the possibility of a bug in our code, could you give that a shot? A couple other things in line: ::: auslib/admin/views/releases.py @@ +276,4 @@ > return changeRelease(release, changed_by, transaction, exists, commit, self.log) > > @requirelogin > + def _check_rule_count(self, name, changed_by): The web layer isn't the right place to do this, even if you end up implementing it yourself instead of using a foreign key. If the foreign key doesn't work out, this should go in https://github.com/mozilla/balrog/blob/master/auslib/db.py#L1089. Either way, you should add tests both at the web layer (which you're doing) and the database layer (like https://github.com/mozilla/balrog/blob/master/auslib/test/test_db.py#L1020) I think the queries below aren't quite right either, but let's see if the foreign key works out before worrying about that. ::: auslib/test/admin/views/test_releases.py @@ +132,5 @@ > ret = self._delete("/releases/a", username="bob") > self.assertStatusCode(ret, 401) > > + def testDeleteWithRulesExisting(self): > + # Test the _delete method first mocking check_rule_count helper I don't think you should be doing any mocking in these tests, regardless of whether this gets implemented as a foreign key or manually. There's no external services we're depending on either (the database is in-memory sqlite, so it's fine to depend on that). I think you're also testing multiple cases in one test...they should be separated out into individual tests. I see at least: testDeleteWithRuleMapping testDeleteWithRuleWhitelist ..but there might be other cases to cover as well.
Attachment #8724159 - Flags: review?(bhearsum) → feedback+
Kumar, are you still interested in working on this?
(In reply to Ben Hearsum (:bhearsum) from comment #3) > Kumar, are you still interested in working on this? Looks like not. Feel free to reassign yourself if you like, though.
Assignee: shailrishabh → nobody
Whiteboard: [lang=python] → [lang=python][good first bug]
Commit pushed to master at https://github.com/mozilla/balrog https://github.com/mozilla/balrog/commit/38830e39e9c7d28c280b85b78ea66df6ef1bae66 Bug 1112628: disallow deletion of releases that have a rule.mapping or rule.whitelist pointing at them (#126). r=bhearsum
This is in production now, thanks Ninad!
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.