Closed
Bug 1112628
Opened 9 years ago
Closed 7 years ago
disallow deletion of releases that have a rule.mapping or rule.whitelist pointing at them
Categories
(Release Engineering Graveyard :: Applications: Balrog (backend), defect)
Release Engineering Graveyard
Applications: Balrog (backend)
Tracking
(Not tracked)
RESOLVED
FIXED
People
(Reporter: bhearsum, Assigned: ninad101, Mentored)
References
Details
(Whiteboard: [lang=python][good first bug])
Attachments
(1 file)
5.21 KB,
patch
|
bhearsum
:
feedback+
|
Details | Diff | Splinter Review |
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.
Reporter | ||
Updated•7 years ago
|
Mentor: bhearsum
Whiteboard: [lang=python]
Reporter | ||
Updated•7 years ago
|
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
Comment 1•7 years ago
|
||
Not too sure if it is desired this way. But still would like to get some review.
Attachment #8724159 -
Flags: review?(bhearsum)
Reporter | ||
Comment 2•7 years ago
|
||
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+
Reporter | ||
Updated•7 years ago
|
Assignee: nobody → shailrishabh
Reporter | ||
Comment 3•7 years ago
|
||
Kumar, are you still interested in working on this?
Flags: needinfo?(shailrishabh)
Reporter | ||
Comment 4•7 years ago
|
||
(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
Flags: needinfo?(shailrishabh)
Reporter | ||
Updated•7 years ago
|
Whiteboard: [lang=python] → [lang=python][good first bug]
Comment 5•7 years ago
|
||
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
Reporter | ||
Updated•7 years ago
|
Assignee: nobody → bhat.ninadmb
Reporter | ||
Comment 6•7 years ago
|
||
This is in production now, thanks Ninad!
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Updated•3 years ago
|
Product: Release Engineering → Release Engineering Graveyard
You need to log in
before you can comment on or make changes to this bug.
Description
•