Code review comment for lp:~wallyworld/launchpad/unshare-team-members-subscriptions2-1009358

Thanks for the review!

On Thu 14 Jun 2012 14:45:23 EST, William Grant wrote:
> Review: Approve code
>
> As discussed on IRC, this branch makes the jobs completely unsuitable for product. Most notably, the jobs in a lot of cases end up verifying the subscription consistency of hundreds of thousands or millions of unrelated bugs. However, it's a good intermediate step, since the jobs remain disabled on production.
>

I'll add it pillar filtering prior to landing.

> Since it doesn't make a huge amount of sense to review the functionality, there's just a couple of style comments. In addition to the below, I also propose http://pastebin.ubuntu.com/1040252/ as a bit of a cleanup around the admin special case.

Looks good I think.

>
> 185 bug_ids = [
> 186 - bug.id for bug in bugs
> 187 + bug.id for bug in bugs or []
> 188 + ]
> 189 + information_types = [
> 190 + info_type.value for info_type in information_types or []
> 191 ]
>
> Can't those list comprehensions be on one line each?
>

The first one can. The second won't fit. To me it looked better if they
were both done the same way. But I've changed it.

> 202 + @property
> 203 + def information_types(self):
> 204 + return [
> 205 + enumerated_type_registry[InformationType.name].items[value]
> 206 + for value in self.metadata['information_types']]
>
> Why does this use the registry? You are looking the enum up by the name you got from the enum :)
>

I think you misread it :-) It's pulling serialised values (the names)
from the stored json and looking them up in an items list associated
with the InformationType.name attribute.

> 495 +# def test_revokeAccessGrantsBranches(self):
>
> I'm not sure a commented test is useful, particularly when we have the work planned and the test won't ever work as written.

The test worked fine in the previous branch. I had left it in as a
reminder so it could be adapted when the next job was added but will
remove it.