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. _______________________________________________ Mailing list: https://launchpad.net/~launchpad-reviewers Post to : [email protected] Unsubscribe : https://launchpad.net/~launchpad-reviewers More help : https://help.launchpad.net/ListHelp
