Launchpad itself

Merge lp:~gmb/launchpad/launchpadformify-subscribe-view-bug-651240 into lp:launchpad

  1. launchpadformify-subscribe-view-bug-651240
  2. Merge into devel
Proposed by Graham Binns
Status: Merged
Approved by: Gavin Panella
Approved revision: no longer in the source branch.
Merged at revision: 11724
Proposed branch: lp:~gmb/launchpad/launchpadformify-subscribe-view-bug-651240
Merge into: lp:launchpad
Prerequisite: lp:~gmb/launchpad/extract-subscription-view-bug-651240
Diff against target: 255 lines (+53/-88)
3 files modified
lib/lp/bugs/browser/bugsubscription.py (+48/-58)
lib/lp/bugs/stories/bugs/xx-bug-personal-subscriptions.txt (+1/-1)
lib/lp/bugs/templates/bug-subscription.pt (+4/-29)
To merge this branch: bzr merge lp:~gmb/launchpad/launchpadformify-subscribe-view-bug-651240
Reviewer Review Type Date Requested Status
Gavin Panella (community) Approve
Review via email: mp+37850@code.launchpad.net

Commit message

BugSubscriptionSubscribeSelfView is now a LaunchpadFormView.

Description of the change

This branch fixes bug 651240 by turning BugSubscriptionSubscribeSelfView from a LaunchpadView into a LaunchpadFormView. The work here is pretty straightforward - converting everything to use FormFields instead of custom widget attributes on the view - and will be used in subsequent branches to add new features to BugSubscriptions.

To post a comment you must log in.
Revision history for this message
Gavin Panella (allenap) wrote :

Looks good. Took me a while before I realised that it's mostly moved
code. Because of that, this is landable as-is, though my comments
below still apply.

[1]

+ if referer and referer not in ('localhost', self.request.getURL()):

Implicit booleanism. You could express this in another way:

        if referer in (None, '', 'localhost', self.request.getURL()):
            next_url = canonical_url(self.context)
        else:
            next_url = referer

[2]

+ persons_for_user = {}
+ person_count = 0
+ bug = self.context.bug
+ for person in bug.getSubscribersForPerson(self.user):
+ if person.id not in persons_for_user:
+ persons_for_user[person.id] = person
+ person_count += 1
+
+ self._subscriber_count_for_current_user = person_count
+ return persons_for_user.values()

I think that person_count is equivalent to len(persons_for_user).

Having said that, _subscriber_count_for_current_user doesn't appear to
be used anywhere.

Also, based on how it's used, _subscribers_for_current_user could be
simplified to:

    @cachedproperty
    def _subscribers_for_current_user(self):
        """Return a dict of the subscribers for the current user."""
        return set(bug.getSubscribersForPerson(self.user))

[3]

+ 'Unsubscribe <a href="%s">%s</a> from this bug' % (
+ canonical_url(person),

The canonical_url(person) may need to be cgi.escape()d too.

[4]

+ 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.widgets['subscription'].hasValidInput())

Could this be replaced by referring to
self.subscribe_action.submitted()?

Actually, it doesn't seem to be used anymore.

Okay, I'll stop going through this stuff so finely as I now see that
it's mostly moved code.

review: Approve
Revision history for this message
Gavin Panella (allenap) wrote :

Oh, flaming turds, I missed the prerequisite branch! Well, the review still stands.

Revision history for this message
Graham Binns (gmb) wrote :

<gmb> allenap: Thanks.
 allenap: So _subscribers_for_current_user is used quite a bit in the subsequent branch, so I'm happier to leave that as-is.
 allenap: And _isSubscriptionRequest() is vestigial and no longer in use.
<allenap> gmb: Yeah, those comments of mine need a lot of salt today.
<gmb> (Which I just noticed)
 allenap: Fair enough. I'll remove one and leave the other, then.

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'lib/lp/bugs/browser/bugsubscription.py'
2--- lib/lp/bugs/browser/bugsubscription.py 2010-10-09 14:27:51 +0000
3+++ lib/lp/bugs/browser/bugsubscription.py 2010-10-09 14:27:52 +0000
4@@ -15,6 +15,7 @@
5 from lazr.delegates import delegates
6 from simplejson import dumps
7
8+from zope import formlib
9 from zope.app.form import CustomWidgetFactory
10 from zope.app.form.browser.itemswidgets import RadioWidget
11 from zope.app.form.interfaces import IInputWidget
12@@ -25,9 +26,11 @@
13 SimpleVocabulary,
14 )
15
16+from canonical.launchpad import _
17 from canonical.launchpad.webapp import (
18 action,
19 canonical_url,
20+ custom_widget,
21 LaunchpadFormView,
22 LaunchpadView,
23 )
24@@ -36,6 +39,9 @@
25 from canonical.launchpad.webapp.menu import structured
26 from lp.bugs.browser.bug import BugViewMixin
27 from lp.bugs.interfaces.bugsubscription import IBugSubscription
28+from lp.registry.enum import BugNotificationLevel
29+from lp.services import features
30+from lp.services.propertycache import cachedproperty
31
32
33 class BugSubscriptionAddView(LaunchpadFormView):
34@@ -74,10 +80,11 @@
35 page_title = label
36
37
38-class BugSubscriptionSubscribeSelfView(LaunchpadView,
39- ReturnToReferrerMixin):
40+class BugSubscriptionSubscribeSelfView(LaunchpadFormView):
41 """A view to handle the +subscribe page for a bug."""
42
43+ schema = IBugSubscription
44+
45 @property
46 def next_url(self):
47 """Provided so returning to the page they came from works."""
48@@ -85,7 +92,7 @@
49
50 # XXX bdmurray 2010-09-30 bug=98437: work around zope's test
51 # browser setting referer to localhost.
52- if referer and referer != 'localhost':
53+ if referer and referer not in ('localhost', self.request.getURL()):
54 next_url = referer
55 else:
56 next_url = canonical_url(self.context)
57@@ -93,20 +100,30 @@
58
59 cancel_url = next_url
60
61- def initialize(self):
62- """Set up the needed widgets."""
63- bug = self.context.bug
64-
65- # See render() for how this flag is used.
66- self._redirecting_to_bug_list = False
67-
68+ @cachedproperty
69+ def _subscribers_for_current_user(self):
70+ """Return a dict of the subscribers for the current user."""
71+ persons_for_user = {}
72+ person_count = 0
73+ bug = self.context.bug
74+ for person in bug.getSubscribersForPerson(self.user):
75+ if person.id not in persons_for_user:
76+ persons_for_user[person.id] = person
77+ person_count += 1
78+
79+ self._subscriber_count_for_current_user = person_count
80+ return persons_for_user.values()
81+
82+ def setUpFields(self):
83+ """See `LaunchpadFormView`."""
84+ super(BugSubscriptionSubscribeSelfView, self).setUpFields()
85+ bug = self.context.bug
86 if self.user is None:
87 return
88
89- # Set up widgets in order to handle subscription requests.
90 subscription_terms = []
91 self_subscribed = False
92- for person in bug.getSubscribersForPerson(self.user):
93+ for person in self._subscribers_for_current_user:
94 if person.id == self.user.id:
95 subscription_terms.append(
96 SimpleTerm(
97@@ -125,16 +142,18 @@
98 SimpleTerm(
99 self.user, self.user.name, 'Subscribe me to this bug'))
100 subscription_vocabulary = SimpleVocabulary(subscription_terms)
101- person_field = Choice(
102- __name__='subscription',
103- vocabulary=subscription_vocabulary, required=True)
104- self.subscription_widget = CustomWidgetFactory(RadioWidget)
105- setUpWidget(
106- self, 'subscription', person_field, IInputWidget, value=self.user)
107-
108- self.handleSubscriptionRequest()
109-
110- def userIsSubscribed(self):
111+ default_subscription_value = subscription_vocabulary.getTermByToken(
112+ self.user.name).value
113+ subscription_field = Choice(
114+ __name__='subscription', title=_("Subscription options"),
115+ vocabulary=subscription_vocabulary, required=True,
116+ default=default_subscription_value)
117+ self.form_fields += formlib.form.Fields(subscription_field)
118+ self.form_fields['subscription'].custom_widget = CustomWidgetFactory(
119+ RadioWidget)
120+
121+ @cachedproperty
122+ def user_is_subscribed(self):
123 """Is the user subscribed to this bug?"""
124 return (
125 self.context.bug.isSubscribed(self.user) or
126@@ -146,7 +165,7 @@
127 The warning should tell the user that, when unsubscribing, they
128 will also be unsubscribed from dupes of this bug.
129 """
130- if self.userIsSubscribed():
131+ if self.user_is_subscribed:
132 return True
133
134 bug = self.context.bug
135@@ -156,45 +175,16 @@
136
137 return False
138
139- def render(self):
140- """Render the bug list if the user has permission to see the bug."""
141- # Prevent normal rendering when redirecting to the bug list
142- # after unsubscribing from a private bug, because rendering the
143- # bug page would raise Unauthorized errors!
144- if self._redirecting_to_bug_list:
145- return u''
146- elif self._isSubscriptionRequest() and self.request.get('next_url'):
147- self.request.response.redirect(self.request.get('next_url'))
148- return u''
149- else:
150- return LaunchpadView.render(self)
151-
152- def handleSubscriptionRequest(self):
153- """Subscribe or unsubscribe the user from the bug, if requested."""
154- if not self._isSubscriptionRequest():
155- return
156-
157- subscription_person = self.subscription_widget.getInputValue()
158-
159- # 'subscribe' appears in the request whether the request is to
160- # subscribe or unsubscribe. Since "subscribe someone else" is
161- # handled by a different view we can assume that 'subscribe' +
162- # current user as a parameter means "subscribe the current
163- # user", and any other kind of 'subscribe' request actually
164- # means "unsubscribe". (Yes, this *is* very confusing!)
165- if ('subscribe' in self.request.form and
166+ @action('Continue', name='continue')
167+ def subscribe_action(self, action, data):
168+ """Handle subscription requests."""
169+ subscription_person = self.widgets['subscription'].getInputValue()
170+ if (not self.user_is_subscribed and
171 (subscription_person == self.user)):
172 self._handleSubscribe()
173 else:
174 self._handleUnsubscribe(subscription_person)
175-
176- def _isSubscriptionRequest(self):
177- """Return True if the form contains subscribe/unsubscribe input."""
178- return (
179- self.user and
180- self.request.method == 'POST' and
181- 'cancel' not in self.request.form and
182- self.subscription_widget.hasValidInput())
183+ self.request.response.redirect(self.next_url)
184
185 def _handleSubscribe(self):
186 """Handle a subscribe request."""
187
188=== modified file 'lib/lp/bugs/stories/bugs/xx-bug-personal-subscriptions.txt'
189--- lib/lp/bugs/stories/bugs/xx-bug-personal-subscriptions.txt 2010-10-09 14:27:51 +0000
190+++ lib/lp/bugs/stories/bugs/xx-bug-personal-subscriptions.txt 2010-10-09 14:27:52 +0000
191@@ -68,7 +68,7 @@
192 >>> submit.click()
193
194 >>> browser.url
195- 'http://bugs.launchpad.dev/firefox/+bug/1/'
196+ 'http://bugs.launchpad.dev/firefox/+bug/1'
197
198 >>> for tag in find_tags_by_class(browser.contents, 'informational message'):
199 ... print tag.renderContents()
200
201=== modified file 'lib/lp/bugs/templates/bug-subscription.pt'
202--- lib/lp/bugs/templates/bug-subscription.pt 2010-10-09 14:27:51 +0000
203+++ lib/lp/bugs/templates/bug-subscription.pt 2010-10-09 14:27:52 +0000
204@@ -15,13 +15,13 @@
205
206 <div id="maincontent">
207 <div id="nonportlets" class="readable">
208- <h1 tal:define="subscribed view/userIsSubscribed">
209+ <h1 tal:define="subscribed view/user_is_subscribed">
210 <tal:action condition="subscribed">Unsubscribe from</tal:action>
211 <tal:action condition="not:subscribed">Subscribe to</tal:action>
212 bug #<span tal:replace="context/bug/id" />
213 </h1>
214
215- <p tal:condition="not: view/userIsSubscribed">
216+ <p tal:condition="not: view/user_is_subscribed">
217 If you subscribe to a bug it shows up on your
218 personal pages, you receive copies of all comments by email,
219 and notification of any changes to the status of the bug upstream
220@@ -34,33 +34,8 @@
221 duplicates.
222 </p>
223
224- <form action="" method="POST">
225-
226- <div class="field">
227- <div>
228- <div tal:content="structure view/subscription_widget">
229- <input type="radio" />
230- </div>
231- </div>
232-
233- </div>
234-
235- <div class="actions">
236- <input type="hidden" name="next_url"
237- tal:condition="view/next_url | nothing"
238- tal:attributes="value view/next_url" />
239- <input tal:condition="not: view/userIsSubscribed"
240- type="submit"
241- name="subscribe"
242- value="Continue" />
243- <input tal:condition="view/userIsSubscribed"
244- type="submit"
245- name="unsubscribe"
246- value="Continue" />
247- or <a tal:condition="view/cancel_url | nothing"
248- tal:attributes="href view/cancel_url">Cancel</a>
249- </div>
250- </form>
251+ <div metal:use-macro="context/@@launchpad_form/form">
252+ </div>
253
254 </div>
255 </div>