launchpad-reviewers team mailing list archive
-
launchpad-reviewers team
-
Mailing list archive
-
Message #04002
[Merge] lp:~wallyworld/launchpad/blueprint-subscription-forms into lp:launchpad
Ian Booth has proposed merging lp:~wallyworld/launchpad/blueprint-subscription-forms into lp:launchpad with lp:~wallyworld/launchpad/blueprint-subscriptions-tales-refactor as a prerequisite.
Requested reviews:
Launchpad code reviewers (launchpad-reviewers)
Related bugs:
Bug #50875 in Launchpad itself: "It is not possible to unsubscribe a team from a blueprint"
https://bugs.launchpad.net/launchpad/+bug/50875
For more details, see:
https://code.launchpad.net/~wallyworld/launchpad/blueprint-subscription-forms/+merge/65184
This branch builds on the blueprint refactoring work in the previous branch to add support for unsubscribing teams from blueprints. It also replaces old blueprint form code with the preferred LaunchpadFormView based infrastructure.
== Implementation ==
Blueprint subscription forms were still implemented using the old form infrastructure which relied on processing html posts in the view initialise() method. The code was modernised to use @action and LaunchpadFormView for managing subscriptions. A few artifacts of the implementation:
- replace two different forms and code used to modify a subscription with a single implementation
- split the "all in one" form used for managing one's own subscriptions into separate add/modify/delete forms
- use the same forms as above to handle subscribing someone else
- consistent use of "Subscribe", "Unsubscribe" etc for the form buttons as opposed to "Subscribe"/"Continue" in different places
- better informational messages - the name of the "someone else" is included in the info message when a different user of subscribed or unsubscribed
The above html changes will make it easier to do the ajax implementation consistent with the rest of lp. To reiterate - this branch does the work to provide the html forms based implementation.
For both the current logged in user, and any allowed teams, unsubscribing is done by clicking the red "remove" icon to the right of the user name. Editing a subscription is done by clicking the icon to the left of the user name.
== Demo ==
http://people.canonical.com/~ianb/blueprint-subscription-portal1.png
The screenshot shows the result of subscribing the Launchpad Administrators team. See the info message and the entry in the portal. The admins team can be unsubscribed hence the remove icon, but the other team and user shown in the portal cannot be subscribed so there's no option to do it.
== Tests ==
Blueprint subscriptions are tested by doc tests in subscribing.txt. These were modified to account for the changed button names as well as adding a test for unsubscribing teams and tests for the better informational messages.
== Lint ==
Checking for conflicts and issues in changed files.
Linting changed files:
lib/lp/blueprints/browser/configure.zcml
lib/lp/blueprints/browser/specification.py
lib/lp/blueprints/browser/specificationsubscription.py
lib/lp/blueprints/stories/standalone/subscribing.txt
lib/lp/blueprints/templates/specification-subscriber-row.pt
lib/lp/blueprints/templates/specification-subscription-delete.pt
lib/lp/blueprints/templates/specification-subscription.pt
./lib/lp/blueprints/browser/specification.py
203: E251 no spaces around keyword / parameter equals
./lib/lp/blueprints/stories/standalone/subscribing.txt
128: source exceeds 78 characters.
246: source exceeds 78 characters.
--
https://code.launchpad.net/~wallyworld/launchpad/blueprint-subscription-forms/+merge/65184
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~wallyworld/launchpad/blueprint-subscription-forms into lp:launchpad.
=== modified file 'lib/lp/blueprints/browser/configure.zcml'
--- lib/lp/blueprints/browser/configure.zcml 2011-06-20 11:53:21 +0000
+++ lib/lp/blueprints/browser/configure.zcml 2011-06-20 11:53:22 +0000
@@ -335,10 +335,16 @@
<browser:page
for="lp.blueprints.interfaces.specification.ISpecification"
name="+subscribe"
- class="lp.blueprints.browser.specification.SpecificationSubscriptionView"
+ class="lp.blueprints.browser.specificationsubscription.SpecificationSubscriptionAddView"
permission="launchpad.AnyPerson"
template="../templates/specification-subscription.pt"/>
<browser:page
+ for="lp.blueprints.interfaces.specification.ISpecificationSubscription"
+ name="+unsubscribe"
+ class="lp.blueprints.browser.specificationsubscription.SpecificationSubscriptionDeleteView"
+ permission="launchpad.Edit"
+ template="../templates/specification-subscription-delete.pt"/>
+ <browser:page
for="lp.blueprints.interfaces.specification.ISpecification"
name="+givefeedback"
class="lp.blueprints.browser.specificationfeedback.SpecificationFeedbackClearingView"
@@ -347,7 +353,7 @@
<browser:page
name="+addsubscriber"
for="lp.blueprints.interfaces.specification.ISpecification"
- class="lp.blueprints.browser.specificationsubscription.SpecificationSubscriptionAddView"
+ class="lp.blueprints.browser.specificationsubscription.SpecificationSubscriptionAddSubscriberView"
permission="launchpad.AnyPerson"
template="../../app/templates/generic-edit.pt"/>
<browser:page
=== modified file 'lib/lp/blueprints/browser/specification.py'
--- lib/lp/blueprints/browser/specification.py 2011-06-20 11:53:21 +0000
+++ lib/lp/blueprints/browser/specification.py 2011-06-20 11:53:22 +0000
@@ -30,7 +30,6 @@
'SpecificationProductSeriesGoalProposeView',
'SpecificationRetargetingView',
'SpecificationSprintAddView',
- 'SpecificationSubscriptionView',
'SpecificationSupersedingView',
'SpecificationTreePNGView',
'SpecificationTreeImageTag',
@@ -473,15 +472,14 @@
"""Return the 'Edit Subscription' Link."""
user = self.user
if user is None:
- text = 'Edit subscription'
- icon = 'edit'
- elif self.context.isSubscribed(user):
- text = 'Update subscription'
- icon = 'edit'
+ return Link('+subscribe', 'Edit subscription', icon='edit')
+
+ if self.context.isSubscribed(user):
+ return Link(
+ '+subscription/%s' % user.name,
+ 'Update subscription', icon='edit')
else:
- text = 'Subscribe'
- icon = 'add'
- return Link('+subscribe', text, icon=icon)
+ return Link('+subscribe', 'Subscribe', icon='add')
@enabled_with_permission('launchpad.AnyPerson')
def linkbug(self):
@@ -539,13 +537,6 @@
return []
return list(self.context.getFeedbackRequests(self.user))
- @property
- def subscription(self):
- """whether the current user has a subscription to the spec."""
- if self.user is None:
- return None
- return self.context.subscription(self.user)
-
@cachedproperty
def has_dep_tree(self):
return self.context.dependencies or self.context.blocked_specs
@@ -578,25 +569,6 @@
if not self.user:
return
- request = self.request
- if request.method == 'POST':
- # establish if a subscription form was posted.
- sub = request.form.get('subscribe')
- upd = request.form.get('update')
- unsub = request.form.get('unsubscribe')
- essential = request.form.get('essential') == 'yes'
- if sub is not None:
- self.context.subscribe(self.user, self.user, essential)
- self.notices.append(
- "You have subscribed to this blueprint.")
- elif upd is not None:
- self.context.subscribe(self.user, self.user, essential)
- self.notices.append('Your subscription has been updated.')
- elif unsub is not None:
- self.context.unsubscribe(self.user, self.user)
- self.notices.append(
- "You have unsubscribed from this blueprint.")
-
if self.feedbackrequests:
msg = "You have %d feedback request(s) on this blueprint."
msg %= len(self.feedbackrequests)
@@ -680,15 +652,6 @@
header='Change approval of basic direction')
-class SpecificationSubscriptionView(SpecificationView):
-
- @property
- def label(self):
- if self.subscription is not None:
- return "Modify subscription"
- return "Subscribe to blueprint"
-
-
class SpecificationEditSchema(ISpecification):
"""Provide overrides for the implementaion and definition status."""
=== modified file 'lib/lp/blueprints/browser/specificationsubscription.py'
--- lib/lp/blueprints/browser/specificationsubscription.py 2011-06-20 11:53:21 +0000
+++ lib/lp/blueprints/browser/specificationsubscription.py 2011-06-20 11:53:22 +0000
@@ -6,6 +6,7 @@
__metaclass__ = type
__all__ = [
'SpecificationSubscriptionAddView',
+ 'SpecificationSubscriptionAddSubscriberView',
'SpecificationSubscriptionEditView',
]
@@ -34,36 +35,102 @@
class SpecificationSubscriptionAddView(LaunchpadFormView):
+ """Used to subscribe the current user to a blueprint."""
schema = ISpecificationSubscription
+ field_names = ['essential']
+ label = 'Subscribe to blueprint'
+
+ @property
+ def cancel_url(self):
+ return canonical_url(self.context)
+
+ next_url = cancel_url
+
+ def _subscribe(self, person, essential):
+ self.context.subscribe(person, self.user, essential)
+
+ @action(_('Subscribe'), name='subscribe')
+ def subscribe_action(self, action, data):
+ self._subscribe(self.user, data['essential'])
+ self.request.response.addInfoNotification(
+ "You have subscribed to this blueprint.")
+
+
+class SpecificationSubscriptionAddSubscriberView(
+ SpecificationSubscriptionAddView):
+ """Used to subscribe someone else to a blueprint."""
+
field_names = ['person', 'essential']
label = 'Subscribe someone else'
for_input = True
- @action(_('Continue'), name='continue')
- def continue_action(self, action, data):
- self.context.subscribe(data['person'], self.user, data['essential'])
- self.next_url = canonical_url(self.context)
+ @action(_('Subscribe'), name='subscribe')
+ def subscribe_action(self, action, data):
+ person = data['person']
+ self._subscribe(person, data['essential'])
+ self.request.response.addInfoNotification(
+ "%s has been subscribed to this blueprint." % person.displayname)
+
+
+class SpecificationSubscriptionDeleteView(LaunchpadFormView):
+ """Used to unsubscribe someone from a blueprint."""
+
+ schema = ISpecificationSubscription
+ field_names = []
+
+ @property
+ def label(self):
+ return ("Unsubscribe %s from %s"
+ % (self.context.person.displayname,
+ self.context.specification.title))
+
+ page_title = label
@property
def cancel_url(self):
- return canonical_url(self.context)
+ return canonical_url(self.context.specification)
+
+ next_url = cancel_url
+
+ @action('Unsubscribe', name='unsubscribe')
+ def unsubscribe_action(self, action, data):
+ self.context.specification.unsubscribe(self.context.person, self.user)
+ if self.context.person == self.user:
+ self.request.response.addInfoNotification(
+ "You have unsubscribed from this blueprint.")
+ else:
+ self.request.response.addInfoNotification(
+ "%s has been unsubscribed from this blueprint."
+ % self.context.person.displayname)
class SpecificationSubscriptionEditView(LaunchpadEditFormView):
schema = ISpecificationSubscription
field_names = ['essential']
- label = 'Edit subscription'
+
+ @property
+ def label(self):
+ return "Modify subscription %s" % self.context.specification.title
+
+ @property
+ def cancel_url(self):
+ return canonical_url(self.context.specification)
+
+ next_url = cancel_url
@action(_('Change'), name='change')
def change_action(self, action, data):
self.updateContextFromData(data)
- self.next_url = canonical_url(self.context.specification)
-
- @property
- def cancel_url(self):
- return canonical_url(self.context.specification)
+ is_current_user_subscription = self.user == self.context.person
+ if is_current_user_subscription:
+ self.request.response.addInfoNotification(
+ "Your subscription has been updated.")
+ else:
+ self.request.response.addInfoNotification(
+ "The subscription for %s has been updated."
+ % self.context.person.displayname)
class SpecificationPortletSubcribersContents(LaunchpadView):
=== modified file 'lib/lp/blueprints/stories/standalone/subscribing.txt'
--- lib/lp/blueprints/stories/standalone/subscribing.txt 2011-06-07 04:39:03 +0000
+++ lib/lp/blueprints/stories/standalone/subscribing.txt 2011-06-20 11:53:22 +0000
@@ -68,10 +68,6 @@
the link to modify the subscription. It should currently be checked.
>>> submod_link.click()
- >>> print browser.title
- Modify subscription : Support E4X in EcmaScript :
- Blueprints : Mozilla Firefox
-
>>> essential = browser.getControl('essential')
>>> essential.selected
True
@@ -79,7 +75,7 @@
We will unset the essential flag and resubmit:
>>> essential.selected = False
- >>> browser.getControl('Update').click()
+ >>> browser.getControl('Change').click()
>>> 'Your subscription has been updated' in browser.contents
True
@@ -99,15 +95,13 @@
True
We don't really want to be subscribed, so lets unsubscribe from that
-spec. We load the subscription page, and now the button says
-"Unsubscribe".
-
- >>> browser.getLink('Update subscription').click()
- >>> unsubit = browser.getControl(name='unsubscribe')
- >>> unsubit.value
- 'Unsubscribe'
-
+spec. We click the remove icon in the subscribers list, and now the
+unsubscribe confirmation page loads.
+
+ >>> unsubit = browser.getLink(id='unsubscribe-subscriber-13')
>>> unsubit.click()
+ >>> confirm = browser.getControl('Unsubscribe')
+ >>> confirm.click()
>>> 'You have unsubscribed from this blueprint.' in browser.contents
True
@@ -130,7 +124,9 @@
'http://blueprints.launchpad.dev/firefox/+spec/e4x'
>>> browser.getControl('Subscriber').value = 'stub'
- >>> browser.getControl('Continue').click()
+ >>> browser.getControl('Subscribe').click()
+ >>> 'Stuart Bishop has been subscribed to this blueprint.' in browser.contents
+ True
When we subscribe someone else to a blueprint, they get notified by
email.
@@ -157,7 +153,14 @@
>>> browser.getLink('Subscribe someone else').click()
>>> browser.getControl('Subscriber').value = 'stub'
>>> browser.getControl(name='field.essential').value = 'yes'
- >>> browser.getControl('Continue').click()
+ >>> browser.getControl('Subscribe').click()
+
+Or we can click the icon next to their name to get to the subscription edit
+page.
+
+ >>> browser.getLink(url='/+subscription/stub').click()
+ >>> browser.getControl(name='field.essential').value = 'no'
+ >>> browser.getControl('Change').click()
When we change a user's subscription, they get notified by email. Teams
can be subscribed to a blueprint too
@@ -197,7 +200,7 @@
>>> browser.getLink('Subscribe someone else').click()
>>> browser.getControl('Subscriber').value = 'admins'
>>> browser.getControl(name='field.essential').value = None
- >>> browser.getControl('Continue').click()
+ >>> browser.getControl('Subscribe').click()
We created a subscription for the Launchpad Admins, but because the team
does not have a preferred email address, an email is sent to each active
@@ -213,7 +216,7 @@
>>> browser.getLink('Subscribe someone else').click()
>>> browser.getControl('Subscriber').value = 'admins'
>>> browser.getControl(name='field.essential').value = 'yes'
- >>> browser.getControl('Continue').click()
+ >>> browser.getControl('Subscribe').click()
We modified the Launchpad Admins team's subscription and again, an email
is sent to each active member.
@@ -230,7 +233,18 @@
>>> browser.getLink('Subscribe someone else').click()
>>> browser.getControl('Subscriber').value = 'ubuntu-team'
>>> browser.getControl(name='field.essential').value = None
- >>> browser.getControl('Continue').click()
+ >>> browser.getControl('Subscribe').click()
+
+Because the current logged in user carlos is a member of the admins team it is
+possible to unsubscribe the team. We click the remove icon in the subscribers
+list, and now the unsubscribe confirmation page loads.
+
+ >>> unsubit = browser.getLink(id='unsubscribe-subscriber-25')
+ >>> unsubit.click()
+ >>> confirm = browser.getControl('Unsubscribe')
+ >>> confirm.click()
+ >>> 'Launchpad Administrators has been unsubscribed from this blueprint.' in browser.contents
+ True
We subscribe the Ubuntu Team and an email is sent to the team's
preferred email address.
@@ -245,7 +259,7 @@
>>> browser.getLink('Subscribe someone else').click()
>>> browser.getControl('Subscriber').value = 'ubuntu-team'
>>> browser.getControl(name='field.essential').value = 'yes'
- >>> browser.getControl('Continue').click()
+ >>> browser.getControl('Subscribe').click()
We modified the Ubuntu Team's subscription and again, an email is sent
to the team's preferred email address.
=== modified file 'lib/lp/blueprints/templates/specification-subscriber-row.pt'
--- lib/lp/blueprints/templates/specification-subscriber-row.pt 2011-06-20 11:53:21 +0000
+++ lib/lp/blueprints/templates/specification-subscriber-row.pt 2011-06-20 11:53:22 +0000
@@ -39,10 +39,10 @@
/>
<a tal:define="user request/lp:person"
tal:condition="python: subscription.canBeUnsubscribedByUser(user)"
- href="+subscribe"
tal:attributes="
title string:Unsubscribe ${subscription/person/fmt:displayname};
- id string:unsubscribe-${subscription/css_name};"
+ id string:unsubscribe-${subscription/css_name};
+ href string:${subscription/fmt:url}/+unsubscribe;"
>
<img class="unsub-icon" src="/@@/remove" alt="Remove"
tal:attributes="id string:unsubscribe-icon-${subscription/css_name}" />
=== added file 'lib/lp/blueprints/templates/specification-subscription-delete.pt'
--- lib/lp/blueprints/templates/specification-subscription-delete.pt 1970-01-01 00:00:00 +0000
+++ lib/lp/blueprints/templates/specification-subscription-delete.pt 2011-06-20 11:53:22 +0000
@@ -0,0 +1,18 @@
+<html
+ xmlns="http://www.w3.org/1999/xhtml";
+ xmlns:tal="http://xml.zope.org/namespaces/tal";
+ xmlns:metal="http://xml.zope.org/namespaces/metal";
+ xmlns:i18n="http://xml.zope.org/namespaces/i18n";
+ metal:use-macro="view/macro:page/main_only"
+ i18n:domain="launchpad">
+ <body>
+ <div metal:fill-slot="main">
+ <p>
+ Are you sure you want to delete the subscription for
+ <strong tal:content="context/person/displayname">Person</strong>?
+ </p>
+ <div metal:use-macro="context/@@launchpad_form/form">
+ </div>
+ </div>
+ </body>
+</html>
=== modified file 'lib/lp/blueprints/templates/specification-subscription.pt'
--- lib/lp/blueprints/templates/specification-subscription.pt 2011-05-15 19:31:31 +0000
+++ lib/lp/blueprints/templates/specification-subscription.pt 2011-06-20 11:53:22 +0000
@@ -15,70 +15,8 @@
<div metal:fill-slot="main">
- <div class="top-portlet" tal:condition="view/subscription">
-
- <p>
- Choose “Unsubscribe” to remove your subscription to this
- blueprint, or “Cancel” to return to the blueprint page.
- </p>
-
- </div>
-
- <div class="top-portlet" tal:condition="not: view/subscription">
-
- <p>
- Choose “Subscribe” to subscribe to this blueprint,
- or “Cancel” to return to the blueprint page.
- </p>
-
- </div>
-
- <form action="." method="POST">
-
- <div class="field">
-
- <tal:subscribed condition="view/subscription">
- <input type="checkbox" id="essential" name="essential"
- value="yes" checked="yes"
- tal:condition="view/subscription/essential" />
- <input type="checkbox" id="essential" name="essential"
- value="yes"
- tal:condition="not: view/subscription/essential" />
- </tal:subscribed>
- <tal:not_subscribed condition="not: view/subscription">
- <input type="checkbox" id="essential" name="essential"
- value="yes" />
- </tal:not_subscribed>
-
- <label for="essential">Participation essential</label>
-
- <p class="formHelp">
- Check this box only if you are required to be included in all discussions about
- this feature when you are attending sprints or meetings with this
- feature on the agenda.
- </p>
-
- </div>
-
- <div class="actions">
- <input tal:condition="not: view/subscription"
- type="submit"
- name="subscribe"
- value="Subscribe" />
- <input tal:condition="view/subscription"
- type="submit"
- name="update"
- value="Update" />
- <input tal:condition="view/subscription"
- type="submit"
- name="unsubscribe"
- value="Unsubscribe" />
- <input type="submit"
- name="cancel"
- value="Cancel" />
- </div>
- </form>
-
+ <div metal:use-macro="context/@@launchpad_form/form">
+ </div>
</div>
</body>
=== modified file 'lib/lp/blueprints/tests/test_webservice.py'
--- lib/lp/blueprints/tests/test_webservice.py 2011-06-20 11:53:21 +0000
+++ lib/lp/blueprints/tests/test_webservice.py 2011-06-20 11:53:22 +0000
@@ -300,7 +300,7 @@
db_spec = self.factory.makeBlueprint()
db_person = self.factory.makePerson()
db_spec.subscribe(person=db_person)
- launchpad = self.factory.makeLaunchpadService()
+ launchpad = self.factory.makeLaunchpadService(person=db_person)
spec = ws_object(launchpad, db_spec)
person = ws_object(launchpad, db_person)
