Merge lp://staging/~gary/launchpad/move-events-to-filters into lp://staging/launchpad/db-devel

A worry: target.addSubscription did result in a subscription that had a level of NOTHING. We never exposed that, to my knowledge, instead relying publicly on addBugSubscription, which always explicitly set a level of COMMENTS. Now, this branch creates subscriptions without filters, which means addBugSubscription has the same result, but addSubscription does not. I don't think it is an issue, but I'd like a sanity check from an expert (gmb? :-D )

There's one more thing I hope will get someone's attention: the XXX I added. I intend to remove it after discussion and resolution in this review.

Here's the concern. The filter class (lp.bugs.model.bugsubscriptionfilter.BugSubscriptionFilter) has an explicit "delete" method that will delete all of its subobjects (that is, records in the database in a many to one relationship with the filter class). However, the structural subscriptions do not have any similar code for the filters, which are themselves many to one with the structural subscriptions. Why? Is the delete method in BugSubscriptionFilter overkill because we have cascading delete in the database, or do I need to do something similar to the delete method on structural subscriptions? This seems like an elementary question for LP development, but not something I've encountered yet.

Thanks.

Fine. patch-2208-37-0.sql

> A worry: target.addSubscription did result in a subscription that had a level
> of NOTHING. We never exposed that, to my knowledge, instead relying publicly
> on addBugSubscription, which always explicitly set a level of COMMENTS. Now,
> this branch creates subscriptions without filters, which means
> addBugSubscription has the same result, but addSubscription does not. I don't
> think it is an issue, but I'd like a sanity check from an expert (gmb? :-D )

I don't think that's going to be an issue (and I'd like to think that tests will blow up if it is).

> There's one more thing I hope will get someone's attention: the XXX I added.
> I intend to remove it after discussion and resolution in this review.
>
> Here's the concern. The filter class
> (lp.bugs.model.bugsubscriptionfilter.BugSubscriptionFilter) has an explicit
> "delete" method that will delete all of its subobjects (that is, records in
> the database in a many to one relationship with the filter class). However,
> the structural subscriptions do not have any similar code for the filters,
> which are themselves many to one with the structural subscriptions. Why? Is
> the delete method in BugSubscriptionFilter overkill because we have cascading
> delete in the database, or do I need to do something similar to the delete
> method on structural subscriptions? This seems like an elementary question
> for LP development, but not something I've encountered yet.

I don't know the answer to this, I'm afraid. I don't see anything in the DB that suggests we have a cascading delete set up in the database, and IIRC you'll get an OOPS if you try to delete something that has other DB rows hanging off it. My best suggestion would be to write a test for this and see whether smoke comes out.

Hi Gary,

So, I'm broadly-speaking +1 on this branch landing (note that I'm on a fairly narrow bandwidth at the moment, so I've gone straight for understanding the code and not worried too much about formatting issues). However, I don't feel that I have enough knowledge about the way that filters work at the moment to offer a definite r=me. I'd suggest you ask Gavin to cast a weather eye over it too, since the filters were at least in part his work.

Anyway, approval from me. Nice work; I don't think I can see a way to break it up without causing some heartache, so don't worry about the branch size.

This is really cool :) I have an answer for your specific question in
[2], and one other trivial comment.

[1]

+ LaunchpadEditFormView.setUpFields(self)

Consider using super() here.

[2]

+ # XXX Delete all associated filters.
         store = Store.of(subscription_to_remove)
         store.remove(subscription_to_remove)

Mucking about in bin/iharness shows that the filters need to be
explicitly deleted too.

> Is the delete method in BugSubscriptionFilter overkill because we
> have cascading delete in the database, or do I need to do something
> similar to the delete method on structural subscriptions?

Yes :) I never thought to put it in before.

I talked briefly with Aaron about this before. There isn't a clear
story for deleting stuff in Launchpad. We have destroySelf() in some
places, a hang-over from SQLObject, and delete() in other places, and
many places do store.remove(thing) without thinking about it.

It might work to have a __storm_remove__ hook so that things like
BugSubscriptionFilter can take responsibility of its
dependencies. Then we would always use store.remove(). However, that
doesn't work from view code which must not speak of the store. I
suspect a mishmash will rule.

Thank you for the reviews!

I have changed to use super (I was too lazy at the time to verify that the base class was new-style; in the LP code base, I suppose I should have assumed yes).

In regards to the XXX, I want to do this in a separate branch, since this one was so gigantic. It also is a bug outside of the code I added in this branch, which I'm using to further rationalize the decision. :-) I created bug 711362 and will be beginning a branch for it (with tests as Graham suggested and a delete method as Gavin suggested) within the hour.