Code review comment for lp://staging/~gary/launchpad/move-events-to-filters

Revision history for this message
Gavin Panella (allenap) wrote :

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.

review: Approve (code)

« Back to merge proposal