← Back to team overview

launchpad-reviewers team mailing list archive

[Merge] lp:~stevenk/launchpad/structsub-private-bugs into lp:launchpad

 

Steve Kowalik has proposed merging lp:~stevenk/launchpad/structsub-private-bugs into lp:launchpad.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)

For more details, see:
https://code.launchpad.net/~stevenk/launchpad/structsub-private-bugs/+merge/115277

Now that visibility and subscription for private artifacts have been decoupled, we can make use of that to notify people about private bugs when they are filed. This required an awful lot of test changes, along with deleting xx-initial-bug-contacts.txt since every that horrid thing is testing will die in the next few weeks.

I have also changed test_sharingjob to clean up the granting and make use of factory.makeAccessArtifact(), which calls ensure() which will return an existing artifact; as well as TestNotificationRecipientsOfPrivateBugs to no longer pull in the LaunchpadObjectFactory and just make use of TestCaseWithFactory.
-- 
https://code.launchpad.net/~stevenk/launchpad/structsub-private-bugs/+merge/115277
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~stevenk/launchpad/structsub-private-bugs into lp:launchpad.
=== modified file 'database/schema/security.cfg'
--- database/schema/security.cfg	2012-07-16 13:02:41 +0000
+++ database/schema/security.cfg	2012-07-17 22:27:00 +0000
@@ -1512,8 +1512,10 @@
 [bugnotification]
 groups=script
 public.accessartifact                   = SELECT, INSERT, DELETE
+public.accessartifactgrant              = SELECT
 public.accesspolicy                     = SELECT
 public.accesspolicyartifact             = SELECT, INSERT, DELETE
+public.accesspolicygrant                = SELECT
 public.account                          = SELECT
 public.answercontact                    = SELECT
 public.archive                          = SELECT

=== modified file 'lib/lp/bugs/browser/tests/bugtarget-filebug-views.txt'
--- lib/lp/bugs/browser/tests/bugtarget-filebug-views.txt	2012-07-17 14:10:53 +0000
+++ lib/lp/bugs/browser/tests/bugtarget-filebug-views.txt	2012-07-17 22:27:00 +0000
@@ -387,18 +387,7 @@
     >>> filebug_view.added_bug.information_type
     <DBItem InformationType.USERDATA, (4) Private>
 
-Since the bug was marked private before it was filed, only the bug
-reporter has been subscribed to the bug and there should be no indirect
-subscribers.
-
-    >>> for subscriber in filebug_view.added_bug.getDirectSubscribers():
-    ...     print subscriber.displayname
-    No Privileges Person
-
-    >>> filebug_view.added_bug.getIndirectSubscribers()
-    []
-
-The user will be notified that the bug has been marked as private.
+he user will be notified that the bug has been marked as private.
 
     >>> print filebug_view.request.response.notifications[2].message
     This bug report has been marked private...
@@ -595,11 +584,6 @@
     Mark Shuttleworth
     No Privileges Person
 
-Since the bug is private, there should be no indirect subscribers.
-
-    >>> filebug_view.added_bug.getIndirectSubscribers()
-    []
-
 The user will be notified that Mark Shuttleworth has been subscribed to
 this bug and that the bug has been marked as private.
 

=== modified file 'lib/lp/bugs/doc/bug.txt'
--- lib/lp/bugs/doc/bug.txt	2012-06-19 02:14:21 +0000
+++ lib/lp/bugs/doc/bug.txt	2012-07-17 22:27:00 +0000
@@ -423,13 +423,6 @@
     >>> [subscriber.name for subscriber in private_bug.getDirectSubscribers()]
     [u'name16']
 
-Since it's private, there are no indirect subscribers.
-
-    >>> private_bug.getIndirectSubscribers()
-    []
-
-It's up to the submitter to subscribe the maintainer, if they so choose.
-
 This works similarly for distributions; in this case the
 "maintainer" is considered the person who maintains the applicable
 sourcepackage. E.g.
@@ -449,8 +442,6 @@
     >>> private_bug = bugset.get(added_bug.id)
     >>> [subscriber.name for subscriber in private_bug.getDirectSubscribers()]
     [u'name16']
-    >>> private_bug.getIndirectSubscribers()
-    []
 
 
 Prevent reporter from being subscribed to filed bugs

=== modified file 'lib/lp/bugs/doc/bugsubscription.txt'
--- lib/lp/bugs/doc/bugsubscription.txt	2012-07-17 06:34:59 +0000
+++ lib/lp/bugs/doc/bugsubscription.txt	2012-07-17 22:27:00 +0000
@@ -405,18 +405,6 @@
     Mark Shuttleworth
     Robert Collins
 
-A private bug never has indirect subscribers. Let's add an indirect subscriber
-to show that they still aren't included in the indirect subscriptions.
-
-    >>> linux_source_bug.bugtasks[0].transitionToAssignee(
-    ...     personset.getByName("martin-pitt"))
-
-    >>> linux_source_bug.getIndirectSubscribers()
-    []
-
-    >>> linux_source_bug.getSubscribersFromDuplicates()
-    ()
-
 Direct subscriptions always take precedence over indirect subscriptions.
 So, if we unmark the above bug as private, indirect_subscribers will include
 any subscribers not converted to direct subscribers.
@@ -430,14 +418,12 @@
     Robert Collins
 
     >>> print_displayname(linux_source_bug.getIndirectSubscribers())
-    Martin Pitt
     No Privileges Person
     Sample Person
     Scott James Remnant
     Ubuntu Team
 
     >>> print_displayname(linux_source_bug.getAlsoNotifiedSubscribers())
-    Martin Pitt
     No Privileges Person
     Sample Person
     Scott James Remnant
@@ -781,20 +767,6 @@
     robertc@xxxxxxxxxxxxxxxxx
     test@xxxxxxxxxxxxx
 
-The upstream maintainer will be subscribed to security-related private
-bugs, because upstream has no security contact, in this case.
-
-    >>> firefox.security_contact is None
-    True
-
-    >>> params = CreateBugParams(
-    ...     title="a test bug", comment="a test description",
-    ...     owner=foobar, information_type=InformationType.PRIVATESECURITY)
-    >>> new_bug = firefox.createBug(params)
-
-    >>> getSubscribers(new_bug)
-    ['foo.bar@xxxxxxxxxxxxx', 'test@xxxxxxxxxxxxx']
-
 Now let's create a bug on a specific package, which has no package bug
 contacts:
 

=== modified file 'lib/lp/bugs/doc/security-teams.txt'
--- lib/lp/bugs/doc/security-teams.txt	2012-07-17 06:34:59 +0000
+++ lib/lp/bugs/doc/security-teams.txt	2012-07-17 22:27:00 +0000
@@ -67,9 +67,6 @@
     ...         bug.getIndirectSubscribers())
     ...     return sorted(subscriber.name for subscriber in subscribers)
 
-    >>> subscriber_names(bug)
-    [u'mark', u'name16']
-
 If the bug were not reported as security-related, only Foo Bar would
 have been subscribed:
 
@@ -104,9 +101,6 @@
     >>> bug.private
     True
 
-    >>> subscriber_names(bug)
-    [u'name16', u'ubuntu-team']
-
 Again, if the bug were not reported as security-related, the security
 contact, the Ubuntu Team, would not have been subscribed:
 

=== modified file 'lib/lp/bugs/mail/bugnotificationrecipients.py'
--- lib/lp/bugs/mail/bugnotificationrecipients.py	2012-06-13 21:01:11 +0000
+++ lib/lp/bugs/mail/bugnotificationrecipients.py	2012-07-17 22:27:00 +0000
@@ -56,7 +56,7 @@
         duplicateof parameter above and the addDupeSubscriber method.
         Don't confuse them!
         """
-        NotificationRecipientSet.__init__(self)
+        super(BugNotificationRecipients, self).__init__()
         self.duplicateof = duplicateof
         self.subscription_filters = set()
 

=== modified file 'lib/lp/bugs/model/bug.py'
--- lib/lp/bugs/model/bug.py	2012-07-17 14:40:15 +0000
+++ lib/lp/bugs/model/bug.py	2012-07-17 22:27:00 +0000
@@ -1134,24 +1134,34 @@
                                      level=None,
                                      include_master_dupe_subscribers=False):
         """See `IBug`."""
+        # Circular fail :(
+        from lp.bugs.model.bugtaskflat import BugTaskFlat
+        from lp.bugs.model.bugtasksearch import get_bug_privacy_filter_terms
+
         recipients = BugNotificationRecipients(duplicateof=duplicateof)
         self.getDirectSubscribers(recipients, level=level)
+        self.getIndirectSubscribers(recipients, level=level)
+
+        # Only include recipients who can see the bug, if it's private.
         if self.private:
-            assert self.getIndirectSubscribers() == [], (
-                "Indirect subscribers found on private bug. "
-                "A private bug should never have implicit subscribers!")
-        else:
-            self.getIndirectSubscribers(recipients, level=level)
-            if include_master_dupe_subscribers and self.duplicateof:
-                # This bug is a public duplicate of another bug, so include
-                # the dupe target's subscribers in the recipient list. Note
-                # that we only do this for duplicate bugs that are public;
-                # changes in private bugs are not broadcast to their dupe
-                # targets.
-                dupe_recipients = (
-                    self.duplicateof.getBugNotificationRecipients(
-                        duplicateof=self.duplicateof, level=level))
-                recipients.update(dupe_recipients)
+            ids = [person.id for person in recipients]
+            forbidden_recipients = IStore(Person).find(
+                Person,
+                Not(Or(*get_bug_privacy_filter_terms(Person.id))),
+                Person.id.is_in(ids), BugTaskFlat.bug_id == self.id)
+            for recipient in forbidden_recipients:
+                if recipient in recipients:
+                    recipients.remove(recipient)
+
+        if include_master_dupe_subscribers and self.duplicateof:
+            # This bug is a public duplicate of another bug, so include
+            # the dupe target's subscribers in the recipient list. Note
+            # that we only do this for duplicate bugs that are public;
+            # changes in private bugs are not broadcast to their dupe
+            # targets.
+            dupe_recipients = self.duplicateof.getBugNotificationRecipients(
+                duplicateof=self.duplicateof, level=level)
+            recipients.update(dupe_recipients)
         # XXX Tom Berger 2008-03-18:
         # We want to look up the recipients for `old_bug` too,
         # but for this to work, this code has to move out of the
@@ -2263,9 +2273,6 @@
     else:
         raise ValueError('First argument must be bug or bugtask')
 
-    if bug.private:
-        return []
-
     # Subscribers to exclude.
     exclude_subscribers = frozenset().union(
         info.direct_subscribers_at_all_levels, info.muted_subscribers)
@@ -2526,19 +2533,16 @@
 
         Excludes muted subscriptions.
         """
-        if self.bug.private:
-            return ()
-        else:
-            return IStore(BugSubscription).find(
-                BugSubscription,
-                BugSubscription.bug_notification_level >= self.level,
-                BugSubscription.bug_id == Bug.id,
-                Bug.duplicateof == self.bug,
-                Not(In(
-                    BugSubscription.person_id,
-                    Select(
-                        BugMute.person_id, BugMute.bug_id == Bug.id,
-                        tables=[BugMute]))))
+        return IStore(BugSubscription).find(
+            BugSubscription,
+            BugSubscription.bug_notification_level >= self.level,
+            BugSubscription.bug_id == Bug.id,
+            Bug.duplicateof == self.bug,
+            Not(In(
+                BugSubscription.person_id,
+                Select(
+                    BugMute.person_id, BugMute.bug_id == Bug.id,
+                    tables=[BugMute]))))
 
     @property
     def duplicate_subscribers(self):
@@ -2626,16 +2630,13 @@
     @cachedproperty
     def also_notified_subscribers(self):
         """All subscribers except direct, dupe, and muted subscribers."""
-        if self.bug.private:
-            return BugSubscriberSet()
-        else:
-            subscribers = BugSubscriberSet().union(
-                self.structural_subscribers,
-                self.all_pillar_owners_without_bug_supervisors,
-                self.all_assignees)
-            return subscribers.difference(
-                self.direct_subscribers_at_all_levels,
-                self.muted_subscribers)
+        subscribers = BugSubscriberSet().union(
+            self.structural_subscribers,
+            self.all_pillar_owners_without_bug_supervisors,
+            self.all_assignees)
+        return subscribers.difference(
+            self.direct_subscribers_at_all_levels,
+            self.muted_subscribers)
 
     @cachedproperty
     def indirect_subscribers(self):

=== modified file 'lib/lp/bugs/model/tests/test_bug.py'
--- lib/lp/bugs/model/tests/test_bug.py	2012-07-17 06:34:59 +0000
+++ lib/lp/bugs/model/tests/test_bug.py	2012-07-17 22:27:00 +0000
@@ -891,25 +891,6 @@
             bug.setPrivate(True, bug.owner)
         self.assertEqual(InformationType.USERDATA, bug.information_type)
 
-    def test_information_type_does_not_leak(self):
-        # Make sure that bug notifications for private bugs do not leak to
-        # people with a subscription on the product.
-        product = self.factory.makeProduct()
-        with person_logged_in(product.owner):
-            product.addSubscription(product.owner, product.owner)
-        reporter = self.factory.makePerson()
-        bug = self.factory.makeBug(
-            information_type=InformationType.USERDATA, product=product,
-            owner=reporter)
-        recipients = Store.of(bug).using(
-            BugNotificationRecipient,
-            Join(BugNotification, BugNotification.bugID == bug.id)).find(
-            BugNotificationRecipient,
-            BugNotificationRecipient.bug_notificationID ==
-                BugNotification.id)
-        self.assertEqual(
-            [reporter], [recipient.person for recipient in recipients])
-
     def test__reconcileAccess_handles_all_targets(self):
         # _reconcileAccess gets the pillar from any task
         # type.

=== modified file 'lib/lp/bugs/model/tests/test_bugsubscriptioninfo.py'
--- lib/lp/bugs/model/tests/test_bugsubscriptioninfo.py	2012-03-13 00:45:33 +0000
+++ lib/lp/bugs/model/tests/test_bugsubscriptioninfo.py	2012-07-17 22:27:00 +0000
@@ -273,18 +273,6 @@
         self.assertContentEqual(
             [duplicate_bug_subscription], found_subscriptions)
 
-    def test_duplicate_for_private_bug(self):
-        # The set of subscribers from duplicate bugs is always empty when the
-        # master bug is private.
-        duplicate_bug = self.factory.makeBug(product=self.target)
-        with person_logged_in(duplicate_bug.owner):
-            duplicate_bug.markAsDuplicate(self.bug)
-        with person_logged_in(self.bug.owner):
-            self.bug.setPrivate(True, self.bug.owner)
-        found_subscriptions = self.getInfo().duplicate_subscriptions
-        self.assertContentEqual([], found_subscriptions)
-        self.assertContentEqual([], found_subscriptions.subscribers)
-
     def test_duplicate_only(self):
         # The set of duplicate subscriptions where the subscriber has no other
         # subscriptions.
@@ -491,33 +479,6 @@
         found_subscribers = self.getInfo().also_notified_subscribers
         self.assertContentEqual([], found_subscribers)
 
-    def test_also_notified_subscribers_for_private_bug(self):
-        # The set of also notified subscribers is always empty when the master
-        # bug is private.
-        assignee = self.factory.makePerson()
-        bugtask = self.bug.default_bugtask
-        with person_logged_in(bugtask.pillar.bug_supervisor):
-            bugtask.transitionToAssignee(assignee)
-        with person_logged_in(self.bug.owner):
-            self.bug.setPrivate(True, self.bug.owner)
-        found_subscribers = self.getInfo().also_notified_subscribers
-        self.assertContentEqual([], found_subscribers)
-
-    def test_indirect_subscribers(self):
-        # The set of indirect subscribers is the union of also notified
-        # subscribers and subscribers to duplicates.
-        assignee = self.factory.makePerson()
-        bugtask = self.bug.default_bugtask
-        with person_logged_in(bugtask.pillar.bug_supervisor):
-            bugtask.transitionToAssignee(assignee)
-        duplicate_bug = self.factory.makeBug(product=self.target)
-        with person_logged_in(duplicate_bug.owner):
-            duplicate_bug.markAsDuplicate(self.bug)
-        found_subscribers = self.getInfo().indirect_subscribers
-        self.assertContentEqual(
-            [assignee, duplicate_bug.owner],
-            found_subscribers)
-
 
 class TestBugSubscriptionInfoPermissions(TestCaseWithFactory):
 
@@ -645,9 +606,9 @@
         self.make_duplicate_bug()
         with person_logged_in(self.bug.owner):
             self.bug.setPrivate(True, self.bug.owner)
-        with self.exactly_x_queries(0):
+        with self.exactly_x_queries(1):
             self.info.duplicate_subscriptions
-        with self.exactly_x_queries(0):
+        with self.exactly_x_queries(1):
             self.info.duplicate_subscriptions.subscribers
 
     def add_structural_subscriber(self):

=== removed file 'lib/lp/bugs/stories/initial-bug-contacts/xx-initial-bug-contacts.txt'
--- lib/lp/bugs/stories/initial-bug-contacts/xx-initial-bug-contacts.txt	2012-07-17 06:34:59 +0000
+++ lib/lp/bugs/stories/initial-bug-contacts/xx-initial-bug-contacts.txt	1970-01-01 00:00:00 +0000
@@ -1,237 +0,0 @@
-To set a bug supervisor for a distribution, we need to go to the Bug page
-of that distribution:
-
-  >>> browser.open('http://launchpad.dev/ubuntu/+bugs')
-
-But the link is not available if you are not logged in with permission
-to change the bug supervisor.
-
-  >>> browser.getLink("Change bug supervisor")
-  Traceback (most recent call last):
-  ...
-  LinkNotFoundError
-
-Colin is an Ubuntu owner and can set the bug supervisor role.
-
-  >>> colin_browser = setupBrowser(
-  ...     auth='Basic colin.watson@xxxxxxxxxxxxxxx:test')
-  >>> colin_browser.open('http://bugs.launchpad.dev/ubuntu/+bugs')
-
-...and he can see that the link on Ubuntu's bugs page.
-
-  >>> bug_supervisor_link = colin_browser.getLink("Change bug supervisor")
-  >>> bug_supervisor_link.url
-  'http://bugs.launchpad.dev/ubuntu/+bugsupervisor'
-
-Anyone with launchpad.Edit permission can edit the distribution bug
-supervisor, but most users can select only themselves and the teams they
-administer. In this example, Colin will set himself as the distribution
-bug supervisor.
-
-  >>> bug_supervisor_link.click()
-  >>> colin_browser.url
-  'http://bugs.launchpad.dev/ubuntu/+bugsupervisor'
-
-The bug supervisor page takes just one simple value: the bug supervisor email
-or nickname. Let's set colin.watson@xxxxxxxxxxxxxxx as the bug supervisor for
-Ubuntu.
-
-  >>> colin_browser.getControl("Bug Supervisor").value = (
-  ...     ' colin.watson@xxxxxxxxxxxxxxx ')
-  >>> colin_browser.getControl("Change").click()
-
-And then Colin is redirected to the distribution bugs page.
-
-    >>> print extract_text(find_tag_by_id(
-    ...     colin_browser.contents, 'bug-supervisor'))
-    Bug supervisor: Colin Watson
-
-== Setting Upstream Bug Supervisor ==
-
-Setting the bug supervisor for an upstream requires launchpad.Edit
-permission on the product. But regular users can only appoint
-themselves as bug supervisors and teams they administer.
-
-    >>> sample_browser = setupBrowser()
-    >>> sample_browser.addHeader("Authorization",
-    ...                          "Basic test@xxxxxxxxxxxxx:test")
-
-    >>> sample_browser.open(
-    ...     "http://bugs.launchpad.dev/firefox/+bugsupervisor";)
-    >>> sample_browser.getControl(name="field.bug_supervisor").value = (
-    ...                           "test@xxxxxxxxxxxxx")
-    >>> sample_browser.getControl("Change").click()
-
-He is now redirected to the main product page, and he sees a confirmation
-message.
-
-    >>> print extract_text(find_tag_by_id(
-    ...     sample_browser.contents, 'bug-supervisor'))
-    Bug supervisor: Sample Person
-
-Another example, this time with a team that has no "preferred email" set.
-
-    >>> sample_browser.open(
-    ...     "http://bugs.launchpad.dev/firefox/+bugsupervisor";)
-    >>> sample_browser.getControl(name="field.bug_supervisor").value = (
-    ...     "landscape-developers")
-    >>> sample_browser.getControl("Change").click()
-    >>> print extract_text(find_tag_by_id(
-    ...     sample_browser.contents, 'bug-supervisor'))
-    Bug supervisor: Landscape Developers
-
-Launchpad administrators can appoint anybody.
-
-    >>> admin_browser = setupBrowser()
-    >>> admin_browser.addHeader("Authorization",
-    ...                          "Basic foo.bar@xxxxxxxxxxxxx:test")
-
-    >>> admin_browser.open("http://bugs.launchpad.dev/firefox/+bugsupervisor";)
-    >>> admin_browser.getControl(name="field.bug_supervisor").value = (
-    ...                           "robertc@xxxxxxxxxxxxxxxxx")
-    >>> admin_browser.getControl("Change").click()
-    >>> print extract_text(find_tag_by_id(
-    ...     admin_browser.contents, 'bug-supervisor'))
-    Bug supervisor: Robert Collins
-
-Filing a public bug on an upstream will subscribe the bug supervisor, as
-well.
-
-    >>> browser.addHeader("Authorization", "Basic mark@xxxxxxxxxxx:test")
-
-    >>> browser.open("http://launchpad.dev/firefox/+filebug";)
-
-    >>> browser.getControl(name="field.title", index=0).value = "bug supervisor test"
-    >>> browser.getControl('Continue').click()
-
-    >>> browser.getControl(name="field.comment").value = "a public bug"
-    >>> browser.getControl("Submit Bug Report").click()
-
-    >>> bug_id = browser.url.split("/")[-1]
-    >>> print browser.url.replace(bug_id, "BUG-ID")
-    http://bugs.launchpad.dev/firefox/+bug/BUG-ID
-
-Now mark (because he's the bug reporter), Sample Person (a former bug
-supervisor), Landscape Developers (another former bug supervisor) and
-Robert Collins (the current bug supervisor) are subscribed to this bug:
-
-    >>> from itertools import chain
-    >>> from zope.component import getUtility
-    >>> from lp.bugs.interfaces.bug import IBugSet
-    >>> from lp.testing import login, logout, ANONYMOUS
-
-    >>> def subscriber_names(bug):
-    ...     subscribers = chain(
-    ...         bug.getDirectSubscribers(),
-    ...         bug.getIndirectSubscribers())
-    ...     return sorted(subscriber.displayname for subscriber in subscribers)
-
-    >>> login(ANONYMOUS)
-    >>> bugset = getUtility(IBugSet)
-    >>> subscriber_names(bugset.get(bug_id))
-    [u'Landscape Developers', u'Mark Shuttleworth', u'Robert Collins',
-    u'Sample Person']
-
-For a security bug, only the reporter and the registrant gets
-subscribed, because Firefox does not have a security contact.
-
-    >>> from lp.registry.interfaces.product import IProductSet
-
-    >>> login(ANONYMOUS)
-    >>> firefox = getUtility(IProductSet).getByName("firefox")
-    >>> firefox.security_contact is None
-    True
-    >>> logout()
-
-    >>> browser.open("http://launchpad.dev/firefox/+filebug";)
-
-    >>> browser.getControl(name="field.title", index=0).value = "bug supervisor test"
-    >>> browser.getControl('Continue').click()
-
-    >>> browser.getControl(name="field.comment").value = "a PRIVATE bug"
-    >>> browser.getControl("Private Security").selected = True
-    >>> browser.getControl("Submit Bug Report").click()
-
-    >>> other_bug_id = browser.url.split("/")[-1]
-    >>> print browser.url.replace(other_bug_id, "BUG-ID")
-    http://bugs.launchpad.dev/firefox/+bug/BUG-ID
-
-    >>> login("mark@xxxxxxxxxxx")
-
-    >>> subscriber_names(bugset.get(other_bug_id))
-    [u'Mark Shuttleworth', u'Sample Person']
-
-    >>> logout()
-
-Filing a public bug on a distribution source package subscribes the bug
-reporter, the distribution bug supervisor, if there is one, and all the
-package subscribers, if there are any.
-
-    >>> browser.addHeader("Authorization", "Basic mark@xxxxxxxxxxx:test")
-
-    >>> browser.open(
-    ...     "http://localhost:9000/ubuntu/+source/mozilla-firefox/";
-    ...     "+filebug")
-
-    >>> browser.getControl(name="field.title", index=0).value = "a public bug"
-    >>> browser.getControl('Continue').click()
-
-    >>> browser.getControl(name="field.comment").value = (
-    ...     "anyone can see this")
-    >>> browser.getControl("Submit Bug Report").click()
-
-    >>> bug_id = browser.url.split("/")[-1]
-    >>> print browser.url.replace(bug_id, "BUG-ID")
-    http://bugs.launchpad.dev/ubuntu/+source/mozilla-firefox/+bug/BUG-ID
-
-We should have three subscribers now. The bug reporter (also a package
-subscriber), mark, the distro bug supervisor kamion, and foobar, who is
-subscribed to the distribution.
-
-    >>> from zope.component import getUtility
-    >>> from lp.bugs.interfaces.bug import IBugSet
-    >>> from lp.testing import login, logout, ANONYMOUS
-
-    >>> def subscriber_names(bug):
-    ...     subscribers = chain(
-    ...         bug.getDirectSubscribers(),
-    ...         bug.getIndirectSubscribers())
-    ...     return sorted(subscriber.name for subscriber in subscribers)
-
-    >>> login(ANONYMOUS)
-
-    >>> bugset = getUtility(IBugSet)
-    >>> subscriber_names(bugset.get(bug_id))
-    [u'kamion', u'mark', u'name16']
-
-
-When filing a security bug, only the bug reporter and registrant are explicitly
-Cc'd, because the Ubuntu distribution does not have a security contact.
-
-    >>> from lp.registry.interfaces.distribution import IDistributionSet
-
-    >>> ubuntu = getUtility(IDistributionSet).getByName("ubuntu")
-    >>> ubuntu.security_contact is None
-    True
-    >>> logout()
-
-    >>> browser.open(
-    ...     "http://localhost:9000/ubuntu/+source/mozilla-firefox/";
-    ...     "+filebug")
-    >>> browser.getControl(name="field.title", index=0).value = "a PRIVATE bug"
-    >>> browser.getControl('Continue').click()
-
-    >>> browser.getControl(name="field.comment").value = "top sekrit"
-    >>> browser.getControl("Private Security").selected = True
-    >>> browser.getControl("Submit Bug Report").click()
-
-    >>> other_bug_id = browser.url.split("/")[-1]
-    >>> print browser.url.replace(other_bug_id, "BUG-ID")
-    http://bugs.launchpad.dev/ubuntu/+source/mozilla-firefox/+bug/BUG-ID
-
-    >>> login("mark@xxxxxxxxxxx")
-
-    >>> subscriber_names(bugset.get(other_bug_id))
-    [u'mark', u'ubuntu-team']
-
-    >>> logout()

=== modified file 'lib/lp/bugs/tests/test_bugchanges.py'
--- lib/lp/bugs/tests/test_bugchanges.py	2012-07-17 21:40:48 +0000
+++ lib/lp/bugs/tests/test_bugchanges.py	2012-07-17 22:27:00 +0000
@@ -31,7 +31,6 @@
 from lp.services.webapp.publisher import canonical_url
 from lp.testing import (
     api_url,
-    celebrity_logged_in,
     launchpadlib_for,
     login_person,
     person_logged_in,
@@ -1046,83 +1045,6 @@
             expected_activity=expected_activity,
             expected_notification=expected_notification)
 
-    def _test_retarget_private_security_bug_to_product(self,
-                                                       bug, maintainer,
-                                                       bug_supervisor=None):
-        # When a private security related bug has a bugtask retargetted to a
-        # different product, a notification is sent to the new bug supervisor
-        # and maintainer. If they are the same person, only one notification
-        # is sent. They only get notifications if they can see the bug.
-
-        # Create the private bug.
-        bug_task = bug.bugtasks[0]
-        new_target = self.factory.makeProduct(
-            owner=maintainer, bug_supervisor=bug_supervisor)
-        self.saveOldChanges(bug)
-
-        bug_task_before_modification = Snapshot(
-            bug_task, providing=providedBy(bug_task))
-        bug_task.transitionToTarget(new_target, self.user)
-        notify(ObjectModifiedEvent(
-            bug_task, bug_task_before_modification,
-            ['target', 'product'], user=self.user))
-
-        expected_activity = {
-            'person': self.user,
-            'whatchanged': 'affects',
-            'oldvalue': bug_task_before_modification.bugtargetname,
-            'newvalue': bug_task.bugtargetname,
-            }
-
-        expected_recipients = [self.user]
-        expected_reasons = ['Subscriber']
-        if bug.userCanView(maintainer):
-            expected_recipients.append(maintainer)
-            expected_reasons.append('Maintainer')
-        if (bug_supervisor and not bug_supervisor.inTeam(maintainer)
-                and bug.userCanView(bug_supervisor)):
-            expected_recipients.append(bug_supervisor)
-            expected_reasons.append('Bug Supervisor')
-        expected_notification = {
-            'text': u"** Project changed: %s => %s" % (
-                bug_task_before_modification.bugtargetname,
-                bug_task.bugtargetname),
-            'person': self.user,
-            'recipients': expected_recipients,
-            'recipient_reasons': expected_reasons
-            }
-        self.assertRecordedChange(
-            expected_activity=expected_activity,
-            expected_notification=expected_notification, bug=bug)
-
-    def test_retarget_private_security_bug_to_product(self):
-        # A series of tests for re-targetting a private bug task.
-        bug = self.factory.makeBug(
-            product=self.product, owner=self.user,
-            information_type=InformationType.USERDATA)
-        maintainer = self.factory.makePerson()
-        bug_supervisor = self.factory.makePerson()
-
-        # Test with no bug supervisor
-        self._test_retarget_private_security_bug_to_product(bug, maintainer)
-        # Test with bug supervisor = maintainer.
-        self._test_retarget_private_security_bug_to_product(
-            bug, maintainer, maintainer)
-        # Test with different bug supervisor
-        self._test_retarget_private_security_bug_to_product(
-            bug, maintainer, bug_supervisor)
-
-        # Now make the bug visible to the bug supervisor and re-test.
-        with celebrity_logged_in('admin'):
-            bug.default_bugtask.transitionToAssignee(bug_supervisor)
-
-        # Test with bug supervisor = maintainer.
-        self._test_retarget_private_security_bug_to_product(
-            bug, maintainer, maintainer)
-        # Test with different bug supervisor
-        self._test_retarget_private_security_bug_to_product(
-            bug, maintainer, bug_supervisor)
-
     def test_target_bugtask_to_sourcepackage(self):
         # When a bugtask's target is changed, BugActivity and
         # BugNotification get updated.
@@ -1174,21 +1096,6 @@
             expected_notification=expected_notification,
             bug=source_package_bug)
 
-    def test_private_bug_target_change_doesnt_add_everyone(self):
-        # Retargeting a private bug doesn't add all subscribers for the
-        # target.
-        old_product = self.factory.makeProduct()
-        new_product = self.factory.makeProduct()
-        subscriber = self.factory.makePerson()
-        new_product.addBugSubscription(subscriber, subscriber)
-        owner = self.factory.makePerson()
-        bug = self.factory.makeBug(
-            product=old_product, owner=owner,
-            information_type=InformationType.USERDATA)
-        bug.default_bugtask.transitionToTarget(new_product, owner)
-        self.assertNotIn(subscriber, bug.getDirectSubscribers())
-        self.assertNotIn(subscriber, bug.getIndirectSubscribers())
-
     def test_add_bugwatch_to_bugtask(self):
         # Adding a BugWatch to a bug task records an entry in
         # BugActivity and BugNotification.

=== modified file 'lib/lp/bugs/tests/test_bugnotification.py'
--- lib/lp/bugs/tests/test_bugnotification.py	2012-05-02 05:25:11 +0000
+++ lib/lp/bugs/tests/test_bugnotification.py	2012-07-17 22:27:00 +0000
@@ -7,7 +7,6 @@
 
 from datetime import datetime
 from itertools import chain
-import unittest
 
 from lazr.lifecycle.event import ObjectModifiedEvent
 from lazr.lifecycle.snapshot import Snapshot
@@ -33,6 +32,7 @@
     )
 from lp.bugs.model.bugsubscriptionfilter import BugSubscriptionFilterMute
 from lp.registry.enums import InformationType
+from lp.registry.interfaces.accesspolicy import IAccessArtifactGrantSource
 from lp.services.config import config
 from lp.services.messages.interfaces.message import IMessageSet
 from lp.services.messages.model.message import MessageSet
@@ -42,7 +42,6 @@
     TestCaseWithFactory,
     )
 from lp.testing.dbuser import switch_dbuser
-from lp.testing.factory import LaunchpadObjectFactory
 from lp.testing.layers import (
     DatabaseFunctionalLayer,
     LaunchpadFunctionalLayer,
@@ -51,33 +50,34 @@
 from lp.testing.matchers import Contains
 
 
-class TestNotificationRecipientsOfPrivateBugs(unittest.TestCase):
+class TestNotificationRecipientsOfPrivateBugs(TestCaseWithFactory):
     """Test who get notified of changes to private bugs."""
 
     layer = LaunchpadFunctionalLayer
 
     def setUp(self):
+        super(TestNotificationRecipientsOfPrivateBugs, self).setUp()
         login('foo.bar@xxxxxxxxxxxxx')
-        factory = LaunchpadObjectFactory()
-        self.product_owner = factory.makePerson(name="product-owner")
-        self.product = factory.makeProduct(owner=self.product_owner)
-        self.product_subscriber = factory.makePerson(
+        self.product_owner = self.factory.makePerson(name="product-owner")
+        self.product = self.factory.makeProduct(owner=self.product_owner)
+        self.product_subscriber = self.factory.makePerson(
             name="product-subscriber")
         self.product.addBugSubscription(
             self.product_subscriber, self.product_subscriber)
-        self.bug_subscriber = factory.makePerson(name="bug-subscriber")
-        self.bug_owner = factory.makePerson(name="bug-owner")
-        self.private_bug = factory.makeBug(
+        self.bug_subscriber = self.factory.makePerson(name="bug-subscriber")
+        self.bug_owner = self.factory.makePerson(name="bug-owner")
+        self.private_bug = self.factory.makeBug(
             product=self.product, owner=self.bug_owner,
             information_type=InformationType.USERDATA)
         self.reporter = self.private_bug.owner
         self.private_bug.subscribe(self.bug_subscriber, self.reporter)
         [self.product_bugtask] = self.private_bug.bugtasks
-        self.direct_subscribers = set(
+        self.subscribers = set(
             person.name for person in [self.bug_subscriber, self.reporter])
 
     def test_status_change(self):
-        # Status changes are sent to the direct subscribers only.
+        # Status changes are sent to direct and indirect subscribers if they
+        # have access to the bug.
         bugtask_before_modification = Snapshot(
             self.product_bugtask, providing=providedBy(self.product_bugtask))
         self.product_bugtask.transitionToStatus(
@@ -89,20 +89,41 @@
         notified_people = set(
             recipient.person.name
             for recipient in latest_notification.recipients)
-        self.assertEqual(self.direct_subscribers, notified_people)
+        expected_subscribers = set(
+            person.name for person in [self.bug_subscriber, self.reporter,
+            self.product_subscriber])
+        self.assertEqual(expected_subscribers, notified_people)
 
     def test_add_comment(self):
-        # Comment additions are sent to the direct subscribers only.
-        self.private_bug.newMessage(
-            self.reporter, subject='subject', content='content')
-        latest_notification = BugNotification.selectFirst(orderBy='-id')
-        notified_people = set(
-            recipient.person.name
-            for recipient in latest_notification.recipients)
-        self.assertEqual(self.direct_subscribers, notified_people)
+        # Comment additions are sent to direct and indirect subscribers if
+        # they have access to the bug.
+        self.private_bug.newMessage(
+            self.reporter, subject='subject', content='content')
+        latest_notification = BugNotification.selectFirst(orderBy='-id')
+        notified_people = set(
+            recipient.person.name
+            for recipient in latest_notification.recipients)
+        self.assertEqual(self.subscribers, notified_people)
+
+    def test_add_comment_with_access(self):
+        # If product_subscriber is given access, they are notified.
+        artifact = self.factory.makeAccessArtifact(concrete=self.private_bug)
+        getUtility(IAccessArtifactGrantSource).grant(
+            [(artifact, self.product_subscriber, self.private_bug.owner)])
+        self.private_bug.newMessage(
+            self.reporter, subject='subject', content='content')
+        latest_notification = BugNotification.selectFirst(orderBy='-id')
+        notified_people = set(
+            recipient.person.name
+            for recipient in latest_notification.recipients)
+        expected_subscribers = set(
+            person.name for person in [self.bug_subscriber, self.reporter,
+            self.product_subscriber])
+        self.assertEqual(expected_subscribers, notified_people)
 
     def test_bug_edit(self):
-        # Bug edits are sent to direct the subscribers only.
+        # Bug edits are sent to direct and indirect subscribers if they have
+        # access to the bug.
         bug_before_modification = Snapshot(
             self.private_bug, providing=providedBy(self.private_bug))
         self.private_bug.description = 'description'
@@ -113,7 +134,7 @@
         notified_people = set(
             recipient.person.name
             for recipient in latest_notification.recipients)
-        self.assertEqual(self.direct_subscribers, notified_people)
+        self.assertEqual(self.subscribers, notified_people)
 
 
 class TestNotificationsSentForBugExpiration(TestCaseWithFactory):

=== modified file 'lib/lp/registry/tests/test_sharingjob.py'
--- lib/lp/registry/tests/test_sharingjob.py	2012-07-17 06:34:59 +0000
+++ lib/lp/registry/tests/test_sharingjob.py	2012-07-17 22:27:00 +0000
@@ -19,7 +19,6 @@
 from lp.registry.enums import InformationType
 from lp.registry.interfaces.accesspolicy import (
     IAccessArtifactGrantSource,
-    IAccessArtifactSource,
     IAccessPolicySource,
     )
 from lp.registry.interfaces.person import TeamSubscriptionPolicy
@@ -182,9 +181,9 @@
         job, job_type = create_job(distro, bug, grantee, owner)
         # Subscribing grantee has created an artifact grant so we need to
         # revoke that to test the job.
+        artifact = self.factory.makeAccessArtifact(concrete=bug)
         getUtility(IAccessArtifactGrantSource).revokeByArtifact(
-            getUtility(IAccessArtifactSource).find(
-                [bug]), [grantee])
+            [artifact], [grantee])
         transaction.commit()
 
         out, err, exit_code = run_script(
@@ -295,9 +294,9 @@
             CodeReviewNotificationLevel.NOEMAIL, owner)
         # Subscribing policy_team_grantee has created an artifact grant so we
         # need to revoke that to test the job.
+        artifact = self.factory.makeAccessArtifact(concrete=bug)
         getUtility(IAccessArtifactGrantSource).revokeByArtifact(
-            getUtility(IAccessArtifactSource).find(
-                [bug]), [policy_team_grantee])
+            [artifact], [policy_team_grantee])
 
         # policy grantees are subscribed because the job has not been run yet.
         subscribers = removeSecurityProxy(bug).getDirectSubscribers()
@@ -367,9 +366,9 @@
         bug.subscribe(artifact_indirect_grantee, owner)
         # Subscribing policy_team_grantee has created an artifact grant so we
         # need to revoke that to test the job.
+        artifact = self.factory.makeAccessArtifact(concrete=branch)
         getUtility(IAccessArtifactGrantSource).revokeByArtifact(
-            getUtility(IAccessArtifactSource).find(
-                [branch]), [policy_team_grantee])
+            [artifact], [policy_team_grantee])
 
         # policy grantees are subscribed because the job has not been run yet.
         #subscribers = removeSecurityProxy(branch).subscribers
@@ -432,10 +431,9 @@
             bug.subscribe(grantee, owner)
         # Subscribing grantee to bug creates an access grant so we need to
         # revoke that for our test.
-        accessartifact_source = getUtility(IAccessArtifactSource)
-        accessartifact_grant_source = getUtility(IAccessArtifactGrantSource)
-        accessartifact_grant_source.revokeByArtifact(
-            accessartifact_source.find([bug]), [grantee])
+        artifact = self.factory.makeAccessArtifact(concrete=bug)
+        getUtility(IAccessArtifactGrantSource).revokeByArtifact(
+            [artifact], [grantee])
 
         return bug, owner
 


Follow ups