Merge lp:~gmb/launchpad/extract-subscription-view-bug-651240 into lp:launchpad

[Graham Binns]

The Bug:+subscribe functionality has been moved into its own view class.

This branch fixes bug 651240 by moving the +subscription-specific code
out of BugTaskView (where it has been since pretty much the dawn of
time) and into its own view. The majority of changes in this branch are
code moves as a result.

== lib/lp/bugs/browser/bugsubscription.py ==

 - I've added a new view, BugSubscriptionSubscribeSelfView. This does
   all the work that BugTask used to do when it was hit via the
   +subscription URL.

== lib/lp/bugs/browser/bugtask.py ==

 - There's an XXX here that I need to discuss with lifeless. Basically,
   the functionally empty for loop actually makes the view more
   efficient (in terms of queries run anyway) because, I presume, it
   causes the materialised objects to be cached by Storm. Removing it
   causes the test that ensures that querie counts remain reasonable to
   fail. I will hopefully remove this XXX-and-for-loop before landing,
   but it seems silly to block the review because of it.
 - I've moved all the +subscribe functionality out of BugTaskView.

== lib/lp/bugs/browser/configure.zcml ==

 - I've updated the ZCML to reflect the code move.

== lib/lp/bugs/templates/bug-subscription.pt ==

 - I've altered the form's action. For some reason the old version
   caused the view to break, though I've no idea why (basically it would
   never actually do anything with action=".". Suggestions as to reasons
   for this are welcome). Note that this form is going away in a
   subsequent branch and being replaced with the launchpadform macro.

[Robert Collins (lifeless)]

Graham, I have a few thoughts for you.

Firstly, consider (by consider I mean 'I really think you should do
this') writing specific tests for query scaling for the new view.

Secondly, the empty loop can be rephrased as
list(bug.getSubscribersForPerson(self.user))

This is eager loading the subscriber data.

*if* you arrange for the new view to be initialized before doing any
dereferencing of people in the main view, you can just trigger that
eager load in the subscribers view. If you can't arrange that, then
you've got two bugs:
1) you're adding a new query that isn't needed (you don't see this
because the scaling tests have a little fat - but check the counts
before and after, you'll see the extra query)
2) its being run too late to eager load anything.

you could make getSubscribersForPerson return a cached object, but
frankly I suspect that the problem is zope's wiring up of views and
subordinate things : death by a thousand cuts is a designed in feature
there, so moving all use of subscribers out of the main view is likely
the only way out of this.

Clearly this needs fixing - we don't want to make the page slower,
which this patch will do at the moment.

-Rob

[Abel Deuring (adeuring)]

review: Approve (code)

=== modified file 'lib/lp/bugs/browser/bugsubscription.py'
--- lib/lp/bugs/browser/bugsubscription.py	2010-09-24 14:13:50 +0000
+++ lib/lp/bugs/browser/bugsubscription.py	2010-10-07 15:32:53 +0000
@@ -10,9 +10,21 @@
     'BugSubscriptionAddView',
     ]
 
+import cgi
+
 from lazr.delegates import delegates
 from simplejson import dumps
 
+from zope.app.form import CustomWidgetFactory
+from zope.app.form.browser.itemswidgets import RadioWidget
+from zope.app.form.interfaces import IInputWidget
+from zope.app.form.utility import setUpWidget
+from zope.schema import Choice
+from zope.schema.vocabulary import (
+    SimpleTerm,
+    SimpleVocabulary,
+    )
+
 from canonical.launchpad.webapp import (
     action,
     canonical_url,
@@ -20,6 +32,8 @@
     LaunchpadView,
     )
 from canonical.launchpad.webapp.authorization import check_permission
+from canonical.launchpad.webapp.launchpadform import ReturnToReferrerMixin
+from canonical.launchpad.webapp.menu import structured
 from lp.bugs.browser.bug import BugViewMixin
 from lp.bugs.interfaces.bugsubscription import IBugSubscription
 
@@ -44,8 +58,8 @@
             message = '%s team has been subscribed to this bug.'
         else:
             message = '%s has been subscribed to this bug.'
-        self.request.response.addInfoNotification(message %
-                                                  person.displayname)
+        self.request.response.addInfoNotification(
+            message % person.displayname)
 
     @property
     def next_url(self):
@@ -60,6 +74,247 @@
     page_title = label
 
 
+class BugSubscriptionSubscribeSelfView(LaunchpadView,
+                                       ReturnToReferrerMixin):
+    """A view to handle the +subscribe page for a bug."""
+
+    @property
+    def next_url(self):
+        """Provided so returning to the page they came from works."""
+        referer = self.request.getHeader('referer')
+
+        # XXX bdmurray 2010-09-30 bug=98437: work around zope's test
+        # browser setting referer to localhost.
+        if referer and referer != 'localhost':
+            next_url = referer
+        else:
+            next_url = canonical_url(self.context)
+        return next_url
+
+    cancel_url = next_url
+
+    def initialize(self):
+        """Set up the needed widgets."""
+        bug = self.context.bug
+
+        # See render() for how this flag is used.
+        self._redirecting_to_bug_list = False
+
+        if self.user is None:
+            return
+
+        # Set up widgets in order to handle subscription requests.
+        subscription_terms = []
+        self_subscribed = False
+        for person in bug.getSubscribersForPerson(self.user):
+            if person.id == self.user.id:
+                subscription_terms.append(
+                    SimpleTerm(
+                        person, person.name,
+                        'Unsubscribe me from this bug'))
+                self_subscribed = True
+            else:
+                subscription_terms.append(
+                    SimpleTerm(
+                        person, person.name,
+                        'Unsubscribe <a href="%s">%s</a> from this bug' % (
+                            canonical_url(person),
+                            cgi.escape(person.displayname))))
+        if not self_subscribed:
+            subscription_terms.insert(0,
+                SimpleTerm(
+                    self.user, self.user.name, 'Subscribe me to this bug'))
+        subscription_vocabulary = SimpleVocabulary(subscription_terms)
+        person_field = Choice(
+            __name__='subscription',
+            vocabulary=subscription_vocabulary, required=True)
+        self.subscription_widget = CustomWidgetFactory(RadioWidget)
+        setUpWidget(
+            self, 'subscription', person_field, IInputWidget, value=self.user)
+
+        self.handleSubscriptionRequest()
+
+    def userIsSubscribed(self):
+        """Is the user subscribed to this bug?"""
+        return (
+            self.context.bug.isSubscribed(self.user) or
+            self.context.bug.isSubscribedToDupes(self.user))
+
+    def shouldShowUnsubscribeFromDupesWarning(self):
+        """Should we warn the user about unsubscribing and duplicates?
+
+        The warning should tell the user that, when unsubscribing, they
+        will also be unsubscribed from dupes of this bug.
+        """
+        if self.userIsSubscribed():
+            return True
+
+        bug = self.context.bug
+        for team in self.user.teams_participated_in:
+            if bug.isSubscribed(team) or bug.isSubscribedToDupes(team):
+                return True
+
+        return False
+
+    def render(self):
+        """Render the bug list if the user has permission to see the bug."""
+        # Prevent normal rendering when redirecting to the bug list
+        # after unsubscribing from a private bug, because rendering the
+        # bug page would raise Unauthorized errors!
+        if self._redirecting_to_bug_list:
+            return u''
+        elif self._isSubscriptionRequest() and self.request.get('next_url'):
+            self.request.response.redirect(self.request.get('next_url'))
+            return u''
+        else:
+            return LaunchpadView.render(self)
+
+    def handleSubscriptionRequest(self):
+        """Subscribe or unsubscribe the user from the bug, if requested."""
+        if not self._isSubscriptionRequest():
+            return
+
+        subscription_person = self.subscription_widget.getInputValue()
+
+        # 'subscribe' appears in the request whether the request is to
+        # subscribe or unsubscribe. Since "subscribe someone else" is
+        # handled by a different view we can assume that 'subscribe' +
+        # current user as a parameter means "subscribe the current
+        # user", and any other kind of 'subscribe' request actually
+        # means "unsubscribe". (Yes, this *is* very confusing!)
+        if ('subscribe' in self.request.form and
+            (subscription_person == self.user)):
+            self._handleSubscribe()
+        else:
+            self._handleUnsubscribe(subscription_person)
+
+    def _isSubscriptionRequest(self):
+        """Return True if the form contains subscribe/unsubscribe input."""
+        return (
+            self.user and
+            self.request.method == 'POST' and
+            'cancel' not in self.request.form and
+            self.subscription_widget.hasValidInput())
+
+    def _handleSubscribe(self):
+        """Handle a subscribe request."""
+        self.context.bug.subscribe(self.user, self.user)
+        self.request.response.addNotification(
+            "You have been subscribed to this bug.")
+
+    def _handleUnsubscribe(self, user):
+        """Handle an unsubscribe request."""
+        if user == self.user:
+            self._handleUnsubscribeCurrentUser()
+        else:
+            self._handleUnsubscribeOtherUser(user)
+
+    def _handleUnsubscribeCurrentUser(self):
+        """Handle the special cases for unsubscribing the current user.
+
+        when the bug is private. The user must be unsubscribed from all dupes
+        too, or they would keep getting mail about this bug!
+        """
+
+        # ** Important ** We call unsubscribeFromDupes() before
+        # unsubscribe(), because if the bug is private, the current user
+        # will be prevented from calling methods on the main bug after
+        # they unsubscribe from it!
+        unsubed_dupes = self.context.bug.unsubscribeFromDupes(
+            self.user, self.user)
+        self.context.bug.unsubscribe(self.user, self.user)
+
+        self.request.response.addNotification(
+            structured(
+                self._getUnsubscribeNotification(self.user, unsubed_dupes)))
+
+        # Because the unsubscribe above may change what the security policy
+        # says about the bug, we need to clear its cache.
+        self.request.clearSecurityPolicyCache()
+
+        if not check_permission("launchpad.View", self.context.bug):
+            # Redirect the user to the bug listing, because they can no
+            # longer see a private bug from which they've unsubscribed.
+            self.request.response.redirect(
+                canonical_url(self.context.target) + "/+bugs")
+            self._redirecting_to_bug_list = True
+
+    def _handleUnsubscribeOtherUser(self, user):
+        """Handle unsubscribing someone other than the current user."""
+        assert user != self.user, (
+            "Expected a user other than the currently logged-in user.")
+
+        # We'll also unsubscribe the other user from dupes of this bug,
+        # otherwise they'll keep getting this bug's mail.
+        self.context.bug.unsubscribe(user, self.user)
+        unsubed_dupes = self.context.bug.unsubscribeFromDupes(user, user)
+        self.request.response.addNotification(
+            structured(
+                self._getUnsubscribeNotification(user, unsubed_dupes)))
+
+    def _getUnsubscribeNotification(self, user, unsubed_dupes):
+        """Construct and return the unsubscribe-from-bug feedback message.
+
+        :user: The IPerson or ITeam that was unsubscribed from the bug.
+        :unsubed_dupes: The list of IBugs that are dupes from which the
+                        user was unsubscribed.
+        """
+        current_bug = self.context.bug
+        current_user = self.user
+        unsubed_dupes_msg_fragment = self._getUnsubscribedDupesMsgFragment(
+            unsubed_dupes)
+
+        if user == current_user:
+            # Consider that the current user may have been "locked out"
+            # of a bug if they unsubscribed themselves from a private
+            # bug!
+            if check_permission("launchpad.View", current_bug):
+                # The user still has permission to see this bug, so no
+                # special-casing needed.
+                return (
+                    "You have been unsubscribed from bug %d%s." % (
+                    current_bug.id, unsubed_dupes_msg_fragment))
+            else:
+                return (
+                    "You have been unsubscribed from bug %d%s. You no "
+                    "longer have access to this private bug.") % (
+                        current_bug.id, unsubed_dupes_msg_fragment)
+        else:
+            return "%s has been unsubscribed from bug %d%s." % (
+                cgi.escape(user.displayname), current_bug.id,
+                unsubed_dupes_msg_fragment)
+
+    def _getUnsubscribedDupesMsgFragment(self, unsubed_dupes):
+        """Return the duplicates fragment of the unsubscription notification.
+
+        This piece lists the duplicates from which the user was
+        unsubscribed.
+        """
+        if not unsubed_dupes:
+            return ""
+
+        dupe_links = []
+        for unsubed_dupe in unsubed_dupes:
+            dupe_links.append(
+                '<a href="%s" title="%s">#%d</a>' % (
+                canonical_url(unsubed_dupe), unsubed_dupe.title,
+                unsubed_dupe.id))
+        dupe_links_string = ", ".join(dupe_links)
+
+        num_dupes = len(unsubed_dupes)
+        if num_dupes > 1:
+            plural_suffix = "s"
+        else:
+            plural_suffix = ""
+
+        return (
+            " and %(num_dupes)d duplicate%(plural_suffix)s "
+            "(%(dupe_links_string)s)") % ({
+                'num_dupes': num_dupes,
+                'plural_suffix': plural_suffix,
+                'dupe_links_string': dupe_links_string})
+
+
 class BugPortletSubcribersContents(LaunchpadView, BugViewMixin):
     """View for the contents for the subscribers portlet."""
 

=== modified file 'lib/lp/bugs/browser/bugtask.py'
--- lib/lp/bugs/browser/bugtask.py	2010-10-06 18:27:57 +0000
+++ lib/lp/bugs/browser/bugtask.py	2010-10-07 15:32:53 +0000
@@ -115,7 +115,6 @@
     )
 from zope.schema.vocabulary import (
     getVocabularyRegistry,
-    SimpleTerm,
     SimpleVocabulary,
     )
 from zope.security.interfaces import Unauthorized
@@ -162,7 +161,6 @@
 from canonical.launchpad.webapp.batching import TableBatchNavigator
 from canonical.launchpad.webapp.breadcrumb import Breadcrumb
 from canonical.launchpad.webapp.interfaces import ILaunchBag
-from canonical.launchpad.webapp.launchpadform import ReturnToReferrerMixin
 from canonical.launchpad.webapp.menu import structured
 from canonical.lazr.interfaces import IObjectPrivacy
 from canonical.lazr.utils import smartquote
@@ -694,39 +692,13 @@
             bug, 'title', canonical_url(self.context, view_name='+edit'),
             id="bug-title", title="Edit this summary")
 
-        if self.user is None:
-            return
-
-        # Set up widgets in order to handle subscription requests.
-        subscription_terms = []
-        self_subscribed = False
-        for person in bug.getSubscribersForPerson(self.user):
-            if person.id == self.user.id:
-                subscription_terms.append(
-                    SimpleTerm(
-                        person, person.name,
-                        'Unsubscribe me from this bug'))
-                self_subscribed = True
-            else:
-                subscription_terms.append(
-                    SimpleTerm(
-                        person, person.name,
-                        'Unsubscribe <a href="%s">%s</a> from this bug' % (
-                            canonical_url(person),
-                            cgi.escape(person.displayname))))
-        if not self_subscribed:
-            subscription_terms.insert(0,
-                SimpleTerm(
-                    self.user, self.user.name, 'Subscribe me to this bug'))
-        subscription_vocabulary = SimpleVocabulary(subscription_terms)
-        person_field = Choice(
-            __name__='subscription',
-            vocabulary=subscription_vocabulary, required=True)
-        self.subscription_widget = CustomWidgetFactory(RadioWidget)
-        setUpWidget(
-            self, 'subscription', person_field, IInputWidget, value=self.user)
-
-        self.handleSubscriptionRequest()
+        # XXX 2010-10-05 gmb bug=655597:
+        #     This line of code keeps the view's query count down,
+        #     possibly using witchcraft. It should be rewritten to be
+        #     useful or removed in favour of making other queries more
+        #     efficient.
+        if self.user is not None:
+            list(bug.getSubscribersForPerson(self.user))
 
     def userIsSubscribed(self):
         """Is the user subscribed to this bug?"""
@@ -734,22 +706,6 @@
             self.context.bug.isSubscribed(self.user) or
             self.context.bug.isSubscribedToDupes(self.user))
 
-    def shouldShowUnsubscribeFromDupesWarning(self):
-        """Should we warn the user about unsubscribing and duplicates?
-
-        The warning should tell the user that, when unsubscribing, they
-        will also be unsubscribed from dupes of this bug.
-        """
-        if self.userIsSubscribed():
-            return True
-
-        bug = self.context.bug
-        for team in self.user.teams_participated_in:
-            if bug.isSubscribed(team) or bug.isSubscribedToDupes(team):
-                return True
-
-        return False
-
     def render(self):
         """Render the bug list if the user has permission to see the bug."""
         # Prevent normal rendering when redirecting to the bug list
@@ -757,155 +713,9 @@
         # bug page would raise Unauthorized errors!
         if self._redirecting_to_bug_list:
             return u''
-        elif self._isSubscriptionRequest() and self.request.get('next_url'):
-            self.request.response.redirect(self.request.get('next_url'))
-            return u''
         else:
             return LaunchpadView.render(self)
 
-    def handleSubscriptionRequest(self):
-        """Subscribe or unsubscribe the user from the bug, if requested."""
-        if not self._isSubscriptionRequest():
-            return
-
-        subscription_person = self.subscription_widget.getInputValue()
-
-        # 'subscribe' appears in the request whether the request is to
-        # subscribe or unsubscribe. Since "subscribe someone else" is
-        # handled by a different view we can assume that 'subscribe' +
-        # current user as a parameter means "subscribe the current
-        # user", and any other kind of 'subscribe' request actually
-        # means "unsubscribe". (Yes, this *is* very confusing!)
-        if ('subscribe' in self.request.form and
-            (subscription_person == self.user)):
-            self._handleSubscribe()
-        else:
-            self._handleUnsubscribe(subscription_person)
-
-    def _isSubscriptionRequest(self):
-        """Return True if the form contains subscribe/unsubscribe input."""
-        return (
-            self.user and
-            self.request.method == 'POST' and
-            'cancel' not in self.request.form and
-            self.subscription_widget.hasValidInput())
-
-    def _handleSubscribe(self):
-        """Handle a subscribe request."""
-        self.context.bug.subscribe(self.user, self.user)
-        self.request.response.addNotification("You have been subscribed to this bug.")
-
-    def _handleUnsubscribe(self, user):
-        """Handle an unsubscribe request."""
-        if user == self.user:
-            self._handleUnsubscribeCurrentUser()
-        else:
-            self._handleUnsubscribeOtherUser(user)
-
-    def _handleUnsubscribeCurrentUser(self):
-        """Handle the special cases for unsubscribing the current user.
-
-        when the bug is private. The user must be unsubscribed from all dupes
-        too, or they would keep getting mail about this bug!
-        """
-
-        # ** Important ** We call unsubscribeFromDupes() before
-        # unsubscribe(), because if the bug is private, the current user
-        # will be prevented from calling methods on the main bug after
-        # they unsubscribe from it!
-        unsubed_dupes = self.context.bug.unsubscribeFromDupes(
-            self.user, self.user)
-        self.context.bug.unsubscribe(self.user, self.user)
-
-        self.request.response.addNotification(
-            structured(
-                self._getUnsubscribeNotification(self.user, unsubed_dupes)))
-
-        # Because the unsubscribe above may change what the security policy
-        # says about the bug, we need to clear its cache.
-        self.request.clearSecurityPolicyCache()
-
-        if not check_permission("launchpad.View", self.context.bug):
-            # Redirect the user to the bug listing, because they can no
-            # longer see a private bug from which they've unsubscribed.
-            self.request.response.redirect(
-                canonical_url(self.context.target) + "/+bugs")
-            self._redirecting_to_bug_list = True
-
-    def _handleUnsubscribeOtherUser(self, user):
-        """Handle unsubscribing someone other than the current user."""
-        assert user != self.user, (
-            "Expected a user other than the currently logged-in user.")
-
-        # We'll also unsubscribe the other user from dupes of this bug,
-        # otherwise they'll keep getting this bug's mail.
-        self.context.bug.unsubscribe(user, self.user)
-        unsubed_dupes = self.context.bug.unsubscribeFromDupes(user, user)
-        self.request.response.addNotification(
-            structured(
-                self._getUnsubscribeNotification(user, unsubed_dupes)))
-
-    def _getUnsubscribeNotification(self, user, unsubed_dupes):
-        """Construct and return the unsubscribe-from-bug feedback message.
-
-        :user: The IPerson or ITeam that was unsubscribed from the bug.
-        :unsubed_dupes: The list of IBugs that are dupes from which the
-                        user was unsubscribed.
-        """
-        current_bug = self.context.bug
-        current_user = self.user
-        unsubed_dupes_msg_fragment = self._getUnsubscribedDupesMsgFragment(
-            unsubed_dupes)
-
-        if user == current_user:
-            # Consider that the current user may have been "locked out"
-            # of a bug if they unsubscribed themselves from a private
-            # bug!
-            if check_permission("launchpad.View", current_bug):
-                # The user still has permission to see this bug, so no
-                # special-casing needed.
-                return (
-                    "You have been unsubscribed from bug %d%s." % (
-                    current_bug.id, unsubed_dupes_msg_fragment))
-            else:
-                return (
-                    "You have been unsubscribed from bug %d%s. You no "
-                    "longer have access to this private bug.") % (
-                        current_bug.id, unsubed_dupes_msg_fragment)
-        else:
-            return "%s has been unsubscribed from this bug%s." % (
-                cgi.escape(user.displayname), unsubed_dupes_msg_fragment)
-
-    def _getUnsubscribedDupesMsgFragment(self, unsubed_dupes):
-        """Return the duplicates fragment of the unsubscription notification.
-
-        This piece lists the duplicates from which the user was
-        unsubscribed.
-        """
-        if not unsubed_dupes:
-            return ""
-
-        dupe_links = []
-        for unsubed_dupe in unsubed_dupes:
-            dupe_links.append(
-                '<a href="%s" title="%s">#%d</a>' % (
-                canonical_url(unsubed_dupe), unsubed_dupe.title,
-                unsubed_dupe.id))
-        dupe_links_string = ", ".join(dupe_links)
-
-        num_dupes = len(unsubed_dupes)
-        if num_dupes > 1:
-            plural_suffix = "s"
-        else:
-            plural_suffix = ""
-
-        return (
-            " and %(num_dupes)d duplicate%(plural_suffix)s "
-            "(%(dupe_links_string)s)") % ({
-                'num_dupes': num_dupes,
-                'plural_suffix': plural_suffix,
-                'dupe_links_string': dupe_links_string})
-
     def _nominateBug(self, series):
         """Nominate the bug for the series and redirect to the bug page."""
         self.context.bug.addNomination(self.user, series)

=== modified file 'lib/lp/bugs/browser/configure.zcml'
--- lib/lp/bugs/browser/configure.zcml	2010-10-03 15:30:06 +0000
+++ lib/lp/bugs/browser/configure.zcml	2010-10-07 15:32:53 +0000
@@ -725,7 +725,7 @@
         <browser:page
             for="lp.bugs.interfaces.bugtask.IBugTask"
             name="+subscribe"
-            class="lp.bugs.browser.bugtask.BugTaskView"
+            class="lp.bugs.browser.bugsubscription.BugSubscriptionSubscribeSelfView"
             permission="launchpad.AnyPerson"
             template="../templates/bug-subscription.pt"/>
         <browser:page

=== modified file 'lib/lp/bugs/stories/bugs/xx-bug-personal-subscriptions.txt'
--- lib/lp/bugs/stories/bugs/xx-bug-personal-subscriptions.txt	2010-10-04 14:20:00 +0000
+++ lib/lp/bugs/stories/bugs/xx-bug-personal-subscriptions.txt	2010-10-07 15:32:53 +0000
@@ -124,7 +124,7 @@
 
   >>> for tag in find_tags_by_class(browser.contents, 'informational message'):
   ...   print tag.renderContents()
-  Launchpad Developers has been unsubscribed from this bug.
+  Launchpad Developers has been unsubscribed from bug 1.
 
   >>> browser.open(
   ...     'http://bugs.launchpad.dev/bugs/1/+bug-portlet-subscribers-content')
@@ -337,7 +337,7 @@
   >>> for tag in find_tags_by_class(
   ...     foobar_browser.contents, 'informational message'):
   ...   print tag.renderContents()
-  Ubuntu Team has been unsubscribed from this bug and 1 duplicate (<a...#2</a>)...
+  Ubuntu Team has been unsubscribed from bug 3 and 1 duplicate (<a...#2</a>)...
 
 (ubuntu-team is no longer an indirect subscriber.)
 

=== modified file 'lib/lp/bugs/templates/bug-subscription.pt'
--- lib/lp/bugs/templates/bug-subscription.pt	2010-09-29 20:33:19 +0000
+++ lib/lp/bugs/templates/bug-subscription.pt	2010-10-07 15:32:53 +0000
@@ -34,7 +34,7 @@
           duplicates.
         </p>
 
-        <form action="." method="POST">
+        <form action="" method="POST">
 
           <div class="field">
             <div>

=== modified file 'lib/lp/registry/browser/person.py'
--- lib/lp/registry/browser/person.py	2010-10-04 20:46:55 +0000
+++ lib/lp/registry/browser/person.py	2010-10-07 15:32:53 +0000
@@ -2368,7 +2368,13 @@
 
         for task in bug_tasks:
             if task.bug not in sub_bugs:
-                sub_bug_tasks.append(task)
+                # We order the bugtasks by date_last_updated but we
+                # always display the default task for the bug. This is
+                # to avoid ordering issues in tests and also prevents
+                # user confusion (because nothing is more confusing than
+                # your subscription targets changing seemingly at
+                # random).
+                sub_bug_tasks.append(task.bug.default_bugtask)
                 sub_bugs.append(task.bug)
         return BatchNavigator(sub_bug_tasks, self.request)
 

=== modified file 'lib/lp/registry/stories/person/xx-person-subscriptions.txt'
--- lib/lp/registry/stories/person/xx-person-subscriptions.txt	2010-10-05 16:17:07 +0000
+++ lib/lp/registry/stories/person/xx-person-subscriptions.txt	2010-10-07 15:32:53 +0000
@@ -110,7 +110,7 @@
     In
     1
     Firefox does not support SVG
-    mozilla-firefox (Ubuntu)
+    Mozilla Firefox
 
 Sample Person now chooses to unsubscribe HWDB team from bug 1.