launchpad-reviewers team mailing list archive

[Curtis Hovey <curtis.hovey@xxxxxxxxxxxxx>]

Review: Approve code

This looks good. Thank you. I think one test can be simplified (and it looks like it is testing the wrong user at the end). this test duplicates code performed by 'celebrity_logged_in'. We also want to avoid sampledata since Foo Bar is more than just an Admin.

203	+    def test_access_for_persons_with_special_permissions(self):
204	+        # Admins have access all products, including inactive and propretary
205	+        # products.
206	+        self.check_admin_access(getUtility(IPersonSet).getByName('name16'))
207	+        # Registry experts can access to all products.
208	+        registry_expert = self.factory.makePerson()
209	+        registry = getUtility(ILaunchpadCelebrities).registry_experts
210	+        with person_logged_in(registry.teamowner):
211	+            registry.addMember(registry_expert, registry.teamowner)
212	+        self.check_admin_access(registry_expert)
213	+        # Commercial admins have access to all products.
214	+        commercial_admin = self.factory.makePerson()
215	+        commercial_admins = getUtility(ILaunchpadCelebrities).commercial_admin
216	+        with person_logged_in(commercial_admins.teamowner):
217	+            commercial_admins.addMember(
218	+                commercial_admin, commercial_admins.teamowner)
219	+        self.check_admin_access(registry_expert)

Could be
    def test_access_for_persons_with_special_permissions(self):
        # Admins have access all products, including inactive and propretary
        # products.
        with celebrity_logged_in('admin') as admin:
            self.check_admin_access(admin)
        with celebrity_logged_in('registry_experts') as registry_expert:
            self.check_admin_access(registry_expert)
        with celebrity_logged_in('commercial_admins') as commercial_admin:
            self.check_admin_access(commercial_admin)

-- 
https://code.launchpad.net/~adeuring/launchpad/authentication-for-private-products-2/+merge/129459
Your team Launchpad code reviewers is subscribed to branch lp:launchpad.