Merge lp:~jcsackett/launchpad/filter-milestone-vocabulary-for-projectgroups into lp:launchpad

[j.c.sackett]

Updates project group milestone filtering to not display milestones from associated private products if the user shouldn't see them.

Summary
=======
As it currently stands, when milestones are fetched for a projectgroup, the
milestones from all associated products are fetched, regardless of whether or
not they belong to a private product the current user cannot see.

Projectgroup must be updated to not show milestones the user can't see, using
our existing filtering clause.

Preimp
======
None.

Implementation
==============
The method `ProjectGroup`.`_getMilestoneCondition`, which creates the query to
fetch milestones, is updated to pull the current user from the launchbag and
create the privacy filtering clause from that. It then adds that clause to the
existing query.

Tests have also been updated for this case.

Tests
=====
bin/test -vvct TestMilestoneVocabulary

QA
==
Ensure the milestones listed for a project does not include anything from
associated private products.

LoC
===
Part of private projects.

Lint
====

Checking for conflicts and issues in changed files.

Linting changed files:
  lib/lp/registry/model/projectgroup.py
  lib/lp/registry/tests/test_milestone_vocabularies.py

[Abel Deuring (adeuring)]

Looks good.

But I am wondering if we need to change the expression returned by ProductSet.getProductPrivacyFilter():

Users may have an artifact grant for a bug/branch/blueprint of a private product but not a policy grant for the product itself; I suspect that we should add a clause like APGF.artifact == None to address this case.

But this affects of course other callsites of getProductPrivacyFilter(), so we should probably talk about it in our standup call on Monday.

review: Approve (code)

[j.c.sackett (jcsackett)]

> Users may have an artifact grant for a bug/branch/blueprint of a private
> product but not a policy grant for the product itself; I suspect that we
> should add a clause like APGF.artifact == None to address this case.
>
> But this affects of course other callsites of getProductPrivacyFilter(), so we
> should probably talk about it in our standup call on Monday.

I agree, especially after dicsussion on IRC. Let's bring this up in the next standup.

=== modified file 'lib/lp/registry/model/projectgroup.py'
--- lib/lp/registry/model/projectgroup.py	2012-10-19 03:01:32 +0000
+++ lib/lp/registry/model/projectgroup.py	2012-11-16 20:43:23 +0000
@@ -407,8 +407,12 @@
 
     def _getMilestoneCondition(self):
         """See `HasMilestonesMixin`."""
-        return And(Milestone.productID == Product.id,
-                   Product.projectID == self.id)
+        user = getUtility(ILaunchBag).user
+        privacy_filter = ProductSet.getProductPrivacyFilter(user)
+        return And(
+            Milestone.productID == Product.id,
+            Product.projectID == self.id,
+            privacy_filter)
 
     def _getMilestones(self, user, only_active):
         """Return a list of milestones for this project group.
 
=== modified file 'lib/lp/registry/tests/test_milestone_vocabularies.py'
--- lib/lp/registry/tests/test_milestone_vocabularies.py	2012-10-11 04:57:59 +0000
+++ lib/lp/registry/tests/test_milestone_vocabularies.py	2012-11-16 20:43:23 +0000
@@ -5,10 +5,9 @@
 
 __metaclass__ = type
 
-from unittest import TestCase
-
 from zope.component import getUtility
 
+from lp.app.enums import InformationType
 from lp.blueprints.interfaces.specification import ISpecificationSet
 from lp.registry.interfaces.distribution import IDistributionSet
 from lp.registry.interfaces.person import IPersonSet
@@ -18,20 +17,47 @@
 from lp.testing import (
     login,
     logout,
+    person_logged_in,
+    TestCaseWithFactory,
     )
 from lp.testing.layers import DatabaseFunctionalLayer
 
 
-class TestMilestoneVocabulary(TestCase):
+class TestMilestoneVocabulary(TestCaseWithFactory):
     """Test that the MilestoneVocabulary behaves as expected."""
+
     layer = DatabaseFunctionalLayer
 
     def setUp(self):
+        super(TestMilestoneVocabulary, self).setUp()
         login('[e-mail]')
 
     def tearDown(self):
+        super(TestMilestoneVocabulary, self).tearDown()
         logout()
 
+    def test_project_group_does_not_show_nonpublic_products(self):
+        # Milestones for a projectgroup should not include those on an
+        # associated private product.
+        owner = self.factory.makePerson()
+        group = self.factory.makeProject(owner=owner)
+        public = self.factory.makeProduct(project=group, owner=owner)
+        private = self.factory.makeProduct(project=group, owner=owner,
+            information_type=InformationType.PROPRIETARY)
+        with person_logged_in(owner):
+            m1 = self.factory.makeMilestone(name='public', product=public)
+            m2 = self.factory.makeMilestone(name='private', product=private)
+        vocabulary = MilestoneVocabulary(group)
+        expected = [m1.title]
+        listing = [term.title for term in vocabulary]
+        self.assertEqual(expected, listing)
+
+        with person_logged_in(owner):
+            vocabulary = MilestoneVocabulary(group)
+            expected = [m1.title, m2.title]
+            listing = [term.title for term in vocabulary]
+            self.assertEqual(expected, listing)
+
     def testProductMilestoneVocabulary(self):
         """Test of MilestoneVocabulary for a product."""
         firefox = getUtility(IProductSet).getByName('firefox')