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
=== modified file 'lib/lp/bugs/browser/bugsubscription.py'
--- lib/lp/bugs/browser/bugsubscription.py 2010-10-09 14:27:51 +0000
+++ lib/lp/bugs/browser/bugsubscription.py 2010-10-09 14:27:52 +0000
@@ -15,6 +15,7 @@
15from lazr.delegates import delegates15from lazr.delegates import delegates
16from simplejson import dumps16from simplejson import dumps
1717
18from zope import formlib
18from zope.app.form import CustomWidgetFactory19from zope.app.form import CustomWidgetFactory
19from zope.app.form.browser.itemswidgets import RadioWidget20from zope.app.form.browser.itemswidgets import RadioWidget
20from zope.app.form.interfaces import IInputWidget21from zope.app.form.interfaces import IInputWidget
@@ -25,9 +26,11 @@
25 SimpleVocabulary,26 SimpleVocabulary,
26 )27 )
2728
29from canonical.launchpad import _
28from canonical.launchpad.webapp import (30from canonical.launchpad.webapp import (
29 action,31 action,
30 canonical_url,32 canonical_url,
33 custom_widget,
31 LaunchpadFormView,34 LaunchpadFormView,
32 LaunchpadView,35 LaunchpadView,
33 )36 )
@@ -36,6 +39,9 @@
36from canonical.launchpad.webapp.menu import structured39from canonical.launchpad.webapp.menu import structured
37from lp.bugs.browser.bug import BugViewMixin40from lp.bugs.browser.bug import BugViewMixin
38from lp.bugs.interfaces.bugsubscription import IBugSubscription41from lp.bugs.interfaces.bugsubscription import IBugSubscription
42from lp.registry.enum import BugNotificationLevel
43from lp.services import features
44from lp.services.propertycache import cachedproperty
3945
4046
41class BugSubscriptionAddView(LaunchpadFormView):47class BugSubscriptionAddView(LaunchpadFormView):
@@ -74,10 +80,11 @@
74 page_title = label80 page_title = label
7581
7682
77class BugSubscriptionSubscribeSelfView(LaunchpadView,83class BugSubscriptionSubscribeSelfView(LaunchpadFormView):
78 ReturnToReferrerMixin):
79 """A view to handle the +subscribe page for a bug."""84 """A view to handle the +subscribe page for a bug."""
8085
86 schema = IBugSubscription
87
81 @property88 @property
82 def next_url(self):89 def next_url(self):
83 """Provided so returning to the page they came from works."""90 """Provided so returning to the page they came from works."""
@@ -85,7 +92,7 @@
8592
86 # XXX bdmurray 2010-09-30 bug=98437: work around zope's test93 # XXX bdmurray 2010-09-30 bug=98437: work around zope's test
87 # browser setting referer to localhost.94 # browser setting referer to localhost.
88 if referer and referer != 'localhost':95 if referer and referer not in ('localhost', self.request.getURL()):
89 next_url = referer96 next_url = referer
90 else:97 else:
91 next_url = canonical_url(self.context)98 next_url = canonical_url(self.context)
@@ -93,20 +100,30 @@
93100
94 cancel_url = next_url101 cancel_url = next_url
95102
96 def initialize(self):103 @cachedproperty
97 """Set up the needed widgets."""104 def _subscribers_for_current_user(self):
98 bug = self.context.bug105 """Return a dict of the subscribers for the current user."""
99106 persons_for_user = {}
100 # See render() for how this flag is used.107 person_count = 0
101 self._redirecting_to_bug_list = False108 bug = self.context.bug
102109 for person in bug.getSubscribersForPerson(self.user):
110 if person.id not in persons_for_user:
111 persons_for_user[person.id] = person
112 person_count += 1
113
114 self._subscriber_count_for_current_user = person_count
115 return persons_for_user.values()
116
117 def setUpFields(self):
118 """See `LaunchpadFormView`."""
119 super(BugSubscriptionSubscribeSelfView, self).setUpFields()
120 bug = self.context.bug
103 if self.user is None:121 if self.user is None:
104 return122 return
105123
106 # Set up widgets in order to handle subscription requests.
107 subscription_terms = []124 subscription_terms = []
108 self_subscribed = False125 self_subscribed = False
109 for person in bug.getSubscribersForPerson(self.user):126 for person in self._subscribers_for_current_user:
110 if person.id == self.user.id:127 if person.id == self.user.id:
111 subscription_terms.append(128 subscription_terms.append(
112 SimpleTerm(129 SimpleTerm(
@@ -125,16 +142,18 @@
125 SimpleTerm(142 SimpleTerm(
126 self.user, self.user.name, 'Subscribe me to this bug'))143 self.user, self.user.name, 'Subscribe me to this bug'))
127 subscription_vocabulary = SimpleVocabulary(subscription_terms)144 subscription_vocabulary = SimpleVocabulary(subscription_terms)
128 person_field = Choice(145 default_subscription_value = subscription_vocabulary.getTermByToken(
129 __name__='subscription',146 self.user.name).value
130 vocabulary=subscription_vocabulary, required=True)147 subscription_field = Choice(
131 self.subscription_widget = CustomWidgetFactory(RadioWidget)148 __name__='subscription', title=_("Subscription options"),
132 setUpWidget(149 vocabulary=subscription_vocabulary, required=True,
133 self, 'subscription', person_field, IInputWidget, value=self.user)150 default=default_subscription_value)
134151 self.form_fields += formlib.form.Fields(subscription_field)
135 self.handleSubscriptionRequest()152 self.form_fields['subscription'].custom_widget = CustomWidgetFactory(
136153 RadioWidget)
137 def userIsSubscribed(self):154
155 @cachedproperty
156 def user_is_subscribed(self):
138 """Is the user subscribed to this bug?"""157 """Is the user subscribed to this bug?"""
139 return (158 return (
140 self.context.bug.isSubscribed(self.user) or159 self.context.bug.isSubscribed(self.user) or
@@ -146,7 +165,7 @@
146 The warning should tell the user that, when unsubscribing, they165 The warning should tell the user that, when unsubscribing, they
147 will also be unsubscribed from dupes of this bug.166 will also be unsubscribed from dupes of this bug.
148 """167 """
149 if self.userIsSubscribed():168 if self.user_is_subscribed:
150 return True169 return True
151170
152 bug = self.context.bug171 bug = self.context.bug
@@ -156,45 +175,16 @@
156175
157 return False176 return False
158177
159 def render(self):178 @action('Continue', name='continue')
160 """Render the bug list if the user has permission to see the bug."""179 def subscribe_action(self, action, data):
161 # Prevent normal rendering when redirecting to the bug list180 """Handle subscription requests."""
162 # after unsubscribing from a private bug, because rendering the181 subscription_person = self.widgets['subscription'].getInputValue()
163 # bug page would raise Unauthorized errors!182 if (not self.user_is_subscribed and
164 if self._redirecting_to_bug_list:
165 return u''
166 elif self._isSubscriptionRequest() and self.request.get('next_url'):
167 self.request.response.redirect(self.request.get('next_url'))
168 return u''
169 else:
170 return LaunchpadView.render(self)
171
172 def handleSubscriptionRequest(self):
173 """Subscribe or unsubscribe the user from the bug, if requested."""
174 if not self._isSubscriptionRequest():
175 return
176
177 subscription_person = self.subscription_widget.getInputValue()
178
179 # 'subscribe' appears in the request whether the request is to
180 # subscribe or unsubscribe. Since "subscribe someone else" is
181 # handled by a different view we can assume that 'subscribe' +
182 # current user as a parameter means "subscribe the current
183 # user", and any other kind of 'subscribe' request actually
184 # means "unsubscribe". (Yes, this *is* very confusing!)
185 if ('subscribe' in self.request.form and
186 (subscription_person == self.user)):183 (subscription_person == self.user)):
187 self._handleSubscribe()184 self._handleSubscribe()
188 else:185 else:
189 self._handleUnsubscribe(subscription_person)186 self._handleUnsubscribe(subscription_person)
190187 self.request.response.redirect(self.next_url)
191 def _isSubscriptionRequest(self):
192 """Return True if the form contains subscribe/unsubscribe input."""
193 return (
194 self.user and
195 self.request.method == 'POST' and
196 'cancel' not in self.request.form and
197 self.subscription_widget.hasValidInput())
198188
199 def _handleSubscribe(self):189 def _handleSubscribe(self):
200 """Handle a subscribe request."""190 """Handle a subscribe request."""
201191
=== modified file 'lib/lp/bugs/stories/bugs/xx-bug-personal-subscriptions.txt'
--- lib/lp/bugs/stories/bugs/xx-bug-personal-subscriptions.txt 2010-10-09 14:27:51 +0000
+++ lib/lp/bugs/stories/bugs/xx-bug-personal-subscriptions.txt 2010-10-09 14:27:52 +0000
@@ -68,7 +68,7 @@
68 >>> submit.click()68 >>> submit.click()
6969
70 >>> browser.url70 >>> browser.url
71 'http://bugs.launchpad.dev/firefox/+bug/1/'71 'http://bugs.launchpad.dev/firefox/+bug/1'
7272
73 >>> for tag in find_tags_by_class(browser.contents, 'informational message'):73 >>> for tag in find_tags_by_class(browser.contents, 'informational message'):
74 ... print tag.renderContents()74 ... print tag.renderContents()
7575
=== modified file 'lib/lp/bugs/templates/bug-subscription.pt'
--- lib/lp/bugs/templates/bug-subscription.pt 2010-10-09 14:27:51 +0000
+++ lib/lp/bugs/templates/bug-subscription.pt 2010-10-09 14:27:52 +0000
@@ -15,13 +15,13 @@
1515
16 <div id="maincontent">16 <div id="maincontent">
17 <div id="nonportlets" class="readable">17 <div id="nonportlets" class="readable">
18 <h1 tal:define="subscribed view/userIsSubscribed">18 <h1 tal:define="subscribed view/user_is_subscribed">
19 <tal:action condition="subscribed">Unsubscribe from</tal:action>19 <tal:action condition="subscribed">Unsubscribe from</tal:action>
20 <tal:action condition="not:subscribed">Subscribe to</tal:action>20 <tal:action condition="not:subscribed">Subscribe to</tal:action>
21 bug #<span tal:replace="context/bug/id" />21 bug #<span tal:replace="context/bug/id" />
22 </h1>22 </h1>
2323
24 <p tal:condition="not: view/userIsSubscribed">24 <p tal:condition="not: view/user_is_subscribed">
25 If you subscribe to a bug it shows up on your25 If you subscribe to a bug it shows up on your
26 personal pages, you receive copies of all comments by email,26 personal pages, you receive copies of all comments by email,
27 and notification of any changes to the status of the bug upstream27 and notification of any changes to the status of the bug upstream
@@ -34,33 +34,8 @@
34 duplicates.34 duplicates.
35 </p>35 </p>
3636
37 <form action="" method="POST">37 <div metal:use-macro="context/@@launchpad_form/form">
3838 </div>
39 <div class="field">
40 <div>
41 <div tal:content="structure view/subscription_widget">
42 <input type="radio" />
43 </div>
44 </div>
45
46 </div>
47
48 <div class="actions">
49 <input type="hidden" name="next_url"
50 tal:condition="view/next_url | nothing"
51 tal:attributes="value view/next_url" />
52 <input tal:condition="not: view/userIsSubscribed"
53 type="submit"
54 name="subscribe"
55 value="Continue" />
56 <input tal:condition="view/userIsSubscribed"
57 type="submit"
58 name="unsubscribe"
59 value="Continue" />
60 or <a tal:condition="view/cancel_url | nothing"
61 tal:attributes="href view/cancel_url">Cancel</a>
62 </div>
63 </form>
6439
65 </div>40 </div>
66 </div>41 </div>