Code review comment for lp:~adeuring/launchpad/authentication-for-private-products-2

Revision history for this message
Curtis Hovey (sinzui) wrote :

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)

review: Approve (code)

« Back to merge proposal