launchpad-reviewers team mailing list archive

[Aaron Bentley <aaron@xxxxxxxxxxxxx>]

Review: Approve

This looks very good to me.  I hope the Celery changes make sense.  There are a few quibbles:

In the tests using CeleryJobLayer, there is a risk that the transaction could be committed, causing Celery to run the job even when you weren't expecting that, and perhaps causing odd leakage into other tests.  To avoid this, I recommend using the FeatureFixture only in test cases that run the job via Celery.

I no longer believe that we need to have *Derived classes.  AFAICT, Storm permits one table to have many classes, so I believe RemoveSubscriptionsJob and the putative AddSubscriptionsJob could subclass SharingJob directly.  However, I have not tried this myself yet, so this is only a suggestion.  Perhaps something to try in a follow-up branch.

I am not sure whether oops prefixes are still required.

Ideally, each job type will have its own database user, so that any problems in production are easier to debug.
-- 
https://code.launchpad.net/~wallyworld/launchpad/revoke-access-delete-subscriptions-job/+merge/104198
Your team Launchpad code reviewers is subscribed to branch lp:launchpad.