launchpad-reviewers team mailing list archive

[Ian Booth <ian.booth@xxxxxxxxxxxxx>]

> Ok Ian, this was a little rough to get through, but I think I've got a couple

Sorry. I owe you a beer or three. Thanks for getting through it.

> Line 346 removal, was that unused but left hanging?
>

Yes. Unused.

> With the tests I'd prefer to see one safe case, where the job was run with
> things that should not be effected so that we can make sure that it's safe to
> run with a control group.
>

The tests create both users who will be unsubscribed and those who shouldn't and do the relevant checks. I could split this up but it would add more lines of code. I'll stake another look.
 
> #980 you no longer use the flat table for the query? Is this intentional?
> 

The flat table is now used by default so the parameter is unneeded.

> #1127 you add the user to the deletePillarSharee, but I don't see that the arg
> is added to the actual method.

Very observant. This was a drive by fix for a previous branch in the pipeline. So far there are 8 branches in this pipe waiting for celery to hit production before I can land.
-- 
https://code.launchpad.net/~wallyworld/launchpad/bug-unsubscribe-pillar-infotype-change/+merge/106792
Your team Launchpad code reviewers is subscribed to branch lp:launchpad.