Closed Bug 1112628 Opened 10 years ago Closed 8 years ago

disallow deletion of releases that have a rule.mapping or rule.whitelist pointing at them


(Release Engineering Graveyard :: Applications: Balrog (backend), defect)

Not set


(Not tracked)



(Reporter: bhearsum, Assigned: ninad101, Mentored)



(Whiteboard: [lang=python][good first bug])


(1 file)

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.
Mentor: bhearsum
Whiteboard: [lang=python]
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]

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.
Attachment #8724159 - Flags: review?(bhearsum) → feedback+
Assignee: nobody → shailrishabh
Kumar, are you still interested in working on this?
Flags: needinfo?(shailrishabh)
(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)
Whiteboard: [lang=python] → [lang=python][good first bug]
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
Assignee: nobody → bhat.ninadmb
Depends on: 1304054
This is in production now, thanks Ninad!
Closed: 8 years ago
Resolution: --- → FIXED
Product: Release Engineering → Release Engineering Graveyard
You need to log in before you can comment on or make changes to this bug.