launchpad-reviewers team mailing list archive

[Ian Booth <ian.booth@xxxxxxxxxxxxx>]

Ian Booth has proposed merging lp:~wallyworld/launchpad/retarget-private-bug-863098 into lp:launchpad.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)
Related bugs:
  Bug #863098 in Launchpad itself: "OOPS when retargeting a private bug to a context the user cannot see private bugs by default"
  https://bugs.launchpad.net/launchpad/+bug/863098

For more details, see:
https://code.launchpad.net/~wallyworld/launchpad/retarget-private-bug-863098/+merge/77895

The branch fixes a corner case which comes up with the disclosure.private_bug_visibility_rules.enabled feature flag enabled. See bug 863098 for details.

== Implementation ==

In the "Save changes" action for the BugTaskEditView, if a change is recorded as having been made and the bug is no longer visible to the user, set the next_url to the bugs index page for the (new) bugtask pillar with a notification message.

To make this work, I had to clear the known_users cached property on the bug, otherwise userCanView() would incorrect;y still return True after the pillar is retargetted. I feel dirty about manipulating model code in the view, but it's being done already in several other places in this View and there was no easy way around it. I tried to clear the known users cache in the bugtask transitionToTarget() method. However, this broke subsequent processing in the save action, eg adding change actions to the bug. So what's been done is the simplest approach to make it work given the current code structure.

== Tests ==

Add a new test to TestBugTaskEditView:
  - test_retarget_private_bug()

== Lint ==

Checking for conflicts and issues in changed files.

Linting changed files:
  lib/lp/bugs/browser/bugtask.py
  lib/lp/bugs/browser/tests/test_bugtask.py


-- 
https://code.launchpad.net/~wallyworld/launchpad/retarget-private-bug-863098/+merge/77895
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~wallyworld/launchpad/retarget-private-bug-863098 into lp:launchpad.

=== modified file 'lib/lp/bugs/browser/bugtask.py'
--- lib/lp/bugs/browser/bugtask.py	2011-10-03 07:49:31 +0000
+++ lib/lp/bugs/browser/bugtask.py	2011-10-03 10:43:27 +0000
@@ -265,7 +265,10 @@
 from lp.registry.model.personroles import PersonRoles
 from lp.registry.vocabularies import MilestoneVocabulary
 from lp.services.fields import PersonChoice
-from lp.services.propertycache import cachedproperty
+from lp.services.propertycache import (
+    cachedproperty,
+    get_property_cache,
+    )
 
 vocabulary_registry = getVocabularyRegistry()
 
@@ -1268,6 +1271,10 @@
     custom_widget('bugwatch', BugTaskBugWatchWidget)
     custom_widget('assignee', BugTaskAssigneeWidget)
 
+    def __init__(self, context, request):
+        super(BugTaskEditView, self).__init__(context, request)
+        self.next_url = canonical_url(self.context)
+
     def initialize(self):
         # Initialize user_is_subscribed, if it hasn't already been set.
         if self.user_is_subscribed is None:
@@ -1352,11 +1359,6 @@
         return self.context.bug.getQuestionCreatedFromBug() is not None
 
     @property
-    def next_url(self):
-        """See `LaunchpadFormView`."""
-        return canonical_url(self.context)
-
-    @property
     def initial_values(self):
         """See `LaunchpadFormView.`"""
         field_values = {}
@@ -1701,6 +1703,19 @@
                     object_before_modification=bugtask_before_modification,
                     edited_fields=field_names))
 
+            # We clear the known views cache because the bug may not be
+            # viewable anymore by the current user. If the bug is not
+            # viewable, then we redirect to the current bugtask's pillar's
+            # bug index page with a message.
+            get_property_cache(bugtask.bug)._known_viewers = set()
+            if not bugtask.bug.userCanView(self.user):
+                self.request.response.addWarningNotification(
+                    "The bug you have just updated is now a private bug for "
+                    "%s. You do not have permission to view such bugs."
+                    % bugtask.pillar.displayname)
+                self.next_url = canonical_url(
+                    new_target.pillar, rootsite='bugs')
+
         if (bugtask.sourcepackagename and (
             self.widgets.get('target') or
             self.widgets.get('sourcepackagename'))):

=== modified file 'lib/lp/bugs/browser/tests/test_bugtask.py'
--- lib/lp/bugs/browser/tests/test_bugtask.py	2011-09-26 07:33:00 +0000
+++ lib/lp/bugs/browser/tests/test_bugtask.py	2011-10-03 10:43:27 +0000
@@ -47,6 +47,10 @@
     IBugTask,
     IBugTaskSet,
     )
+from lp.services.features.model import (
+    FeatureFlag,
+    getFeatureStore,
+    )
 from lp.services.propertycache import get_property_cache
 from lp.soyuz.interfaces.component import IComponentSet
 from lp.testing import (
@@ -908,6 +912,42 @@
         notifications = view.request.response.notifications
         self.assertEqual(0, len(notifications))
 
+    def test_retarget_private_bug(self):
+        # If a private bug is re-targetted such that the bug is no longer
+        # visible to the user, they are redirected to the pillar's bug index
+        # page with a suitable message. This corner case can occur when the
+        # disclosure.private_bug_visibility_rules.enabled feature flag is on
+        # and a bugtask is re-targetted to a pillar for which the user is not
+        # authorised to see any private bugs.
+        first_product = self.factory.makeProduct(name='bunny')
+        with person_logged_in(first_product.owner):
+            bug = self.factory.makeBug(product=first_product, private=True)
+            bug_task = bug.bugtasks[0]
+        second_product = self.factory.makeProduct(name='duck')
+        getFeatureStore().add(FeatureFlag(
+            scope=u'default', value=u'on', priority=1,
+            flag=u'disclosure.private_bug_visibility_rules.enabled'))
+
+        # The first product owner can see the private bug. We will re-target
+        # it to second_product where it will not be visible to that user.
+        with person_logged_in(first_product.owner):
+            form = {
+                'bunny.target': 'product',
+                'bunny.target.product': 'duck',
+                'bunny.actions.save': 'Save Changes',
+                }
+            view = create_initialized_view(
+                bug_task, name='+editstatus', form=form)
+            self.assertEqual(
+                canonical_url(bug_task.pillar, rootsite='bugs'),
+                view.next_url)
+        self.assertEqual([], view.errors)
+        self.assertEqual(second_product, bug_task.target)
+        notifications = view.request.response.notifications
+        self.assertEqual(1, len(notifications))
+        expected = ('The bug you have just updated is now a private bug for')
+        self.assertTrue(notifications.pop().message.startswith(expected))
+
 
 class TestProjectGroupBugs(TestCaseWithFactory):
     """Test the bugs overview page for Project Groups."""