launchpad-reviewers team mailing list archive
-
launchpad-reviewers team
-
Mailing list archive
-
Message #01395
[Merge] lp:~gmb/launchpad/extract-subscription-view-bug-651240 into lp:launchpad/devel
Graham Binns has proposed merging lp:~gmb/launchpad/extract-subscription-view-bug-651240 into lp:launchpad/devel.
Requested reviews:
Launchpad code reviewers (launchpad-reviewers): code
Related bugs:
#651240 Bug:+subscribe should have its own view
https://bugs.launchpad.net/bugs/651240
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.
--
https://code.launchpad.net/~gmb/launchpad/extract-subscription-view-bug-651240/+merge/37713
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~gmb/launchpad/extract-subscription-view-bug-651240 into lp:launchpad/devel.
=== 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-06 08:47:49 +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-04 14:20:00 +0000
+++ lib/lp/bugs/browser/bugtask.py 2010-10-06 08:47:49 +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 lp.app.browser.tales import (
FormattersAPI,
@@ -693,39 +691,11 @@
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
+ # XXX 2010-10-04 gmb
+ # This has to go, *surely*.
+ # HAHAAHAHAHA.
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()
+ continue
def userIsSubscribed(self):
"""Is the user subscribed to this bug?"""
@@ -733,22 +703,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
@@ -756,155 +710,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-06 08:47:49 +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/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-06 08:47:49 +0000
@@ -34,7 +34,7 @@
duplicates.
</p>
- <form action="." method="POST">
+ <form action="" method="POST">
<div class="field">
<div>
Follow ups
