Last Comment Bug 1112628 - disallow deletion of releases that have a rule.mapping or rule.whitelist pointing at them
: disallow deletion of releases that have a rule.mapping or rule.whitelist poin...
[lang=python][good first bug]
Product: Release Engineering
Classification: Other
Component: Balrog: Backend (show other bugs)
: unspecified
: All All
-- normal (vote)
: ---
Assigned To: Ninad Bhat[:ninad101]
: Ben Hearsum (:bhearsum)
Mentors: Ben Hearsum (:bhearsum)
Depends on: 1304054
  Show dependency treegraph
Reported: 2014-12-17 08:35 PST by Ben Hearsum (:bhearsum)
Modified: 2016-09-21 13:11 PDT (History)
2 users (show)
See Also:
Crash Signature:
QA Whiteboard:
Iteration: ---
Points: ---

0001-bug-1140262-disallow-deletion-of-releases-that-have-.patch (5.21 KB, patch)
2016-02-26 11:05 PST, Kumar Rishabh
bhearsum: feedback+
Details | Diff | Splinter Review

Description User image Ben Hearsum (:bhearsum) 2014-12-17 08:35:26 PST
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.
Comment 1 User image Kumar Rishabh 2016-02-26 11:05:28 PST
Created attachment 8724159 [details] [diff] [review]

Not too sure if it is desired this way. But still would like to get some review.
Comment 2 User image Ben Hearsum (:bhearsum) 2016-02-26 11:56:54 PST
Comment on attachment 8724159 [details] [diff] [review]

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/
@@ +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 Either way, you should add tests both at the web layer (which you're doing) and the database layer (like

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/
@@ +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:

..but there might be other cases to cover as well.
Comment 3 User image Ben Hearsum (:bhearsum) 2016-06-10 08:27:01 PDT
Kumar, are you still interested in working on this?
Comment 4 User image Ben Hearsum (:bhearsum) 2016-06-21 07:51:08 PDT
(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.
Comment 5 User image [github robot] 2016-09-19 11:48:50 PDT
Commit pushed to master at
Bug 1112628: disallow deletion of releases that have a rule.mapping or rule.whitelist pointing at them (#126). r=bhearsum
Comment 6 User image Ben Hearsum (:bhearsum) 2016-09-21 13:11:40 PDT
This is in production now, thanks Ninad!

Note You need to log in before you can comment on or make changes to this bug.