launchpad-reviewers team mailing list archive

[William Grant <me@xxxxxxxxxxxxxxxxxx>]

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.

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.

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?

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 :)

495	+# def test_revokeAccessGrantsBranches(self):
496	+# # Users with launchpad.Edit can delete all access for a sharee.
497	+# owner = self.factory.makePerson()
498	+# product = self.factory.makeProduct(owner=owner)
499	+# login_person(owner)
500	+# branch = self.factory.makeBranch(
501	+# product=product, owner=owner,
502	+# information_type=InformationType.USERDATA)
503	+# self._assert_revokeTeamAccessGrants(distro, [bug], None)

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.
-- 
https://code.launchpad.net/~wallyworld/launchpad/unshare-team-members-subscriptions2-1009358/+merge/110215
Your team Launchpad code reviewers is subscribed to branch lp:launchpad.