Launchpad itself

Code review comment for lp:~barry/launchpad/430065-person

  1. 430065-person
  2. Merge into devel
Revision history for this message
Brad Crittenden (bac) wrote :

Hi Barry,

The changes look good. Thanks for taking the time to make a lot of drive-by fixes.

I've got a few questions below.

--Brad

> === modified file 'lib/lp/registry/browser/person.py'
> --- lib/lp/registry/browser/person.py 2009-09-16 17:07:14 +0000
> +++ lib/lp/registry/browser/person.py 2009-09-16 21:19:47 +0000
> @@ -4086,6 +4086,8 @@
> custom_widget('mailing_list_auto_subscribe_policy',
> LaunchpadRadioWidgetWithDescription)
>
> + label = 'Change your e-mail settings'
> +
> def initialize(self):
> if self.context.is_team:
> # +editemails is not available on teams.
> @@ -4191,13 +4193,11 @@
> which the user is subscribed to this mailing list.
> """
> subscription = mailing_list.getSubscription(self.context)
> - if subscription is not None:
> - if subscription.email_address is None:
> - return "Preferred address"
> - else:
> - return subscription.email_address
> - else:
> + if subscription is None:
> return "Don't subscribe"
> + if subscription.email_address is None:

Hmm, I'd have used an elif.

> + return 'Preferred address'
> + return subscription.email_address
>
> def _mailing_list_fields(self):
> """Creates a field for each mailing list the user can subscribe to.
> @@ -4233,8 +4235,22 @@
> @property
> def mailing_list_widgets(self):
> """Return all the mailing list subscription widgets."""
> - return [widget for widget in self.widgets
> - if 'field.subscription.' in widget.name]
> + mailing_list_set = getUtility(IMailingListSet)
> + widgets = []
> + for widget in self.widgets:
> + if 'field.subscription.' in widget.name:

Why not use 'startswith'? Can it really be anywhere in the name?

> + team_name = widget.label
> + mailing_list = mailing_list_set.get(team_name)
> + assert mailing_list is not None, 'Missing mailing list'
> + widget_dict = dict(
> + team=mailing_list.team,
> + widget=widget,
> + )
> + widgets.append(widget_dict)
> + # We'll put the label in the first column, so don't include it
> + # in the second column.
> + widget.display_label = False
> + return widgets
>
> def _validate_selected_address(self, data, field='VALIDATED_SELECTED'):
> """A generic validator for this view's actions.

> === modified file 'lib/lp/registry/stories/foaf/xx-setpreferredemail.txt'
> --- lib/lp/registry/stories/foaf/xx-setpreferredemail.txt 2008-09-08 13:35:16 +0000
> +++ lib/lp/registry/stories/foaf/xx-setpreferredemail.txt 2009-09-16 21:00:11 +0000
> @@ -1,11 +1,14 @@
> -= A person's contact email address =
> +================================
> +A person's contact email address
> +================================

Thanks for converting this test.

> === modified file 'lib/lp/registry/stories/mailinglists/subscriptions.txt'
> --- lib/lp/registry/stories/mailinglists/subscriptions.txt 2009-09-14 01:28:41 +0000
> +++ lib/lp/registry/stories/mailinglists/subscriptions.txt 2009-09-16 21:00:11 +0000

> === removed file 'lib/lp/registry/templates/person-changepassword.pt'

Excellent!

> === modified file 'lib/lp/registry/templates/person-editemails.pt'
> --- lib/lp/registry/templates/person-editemails.pt 2009-07-17 17:59:07 +0000
> +++ lib/lp/registry/templates/person-editemails.pt 2009-09-16 21:00:11 +0000
> @@ -3,35 +3,25 @@
> xmlns:tal="http://xml.zope.org/namespaces/tal"
> xmlns:metal="http://xml.zope.org/namespaces/metal"
> xmlns:i18n="http://xml.zope.org/namespaces/i18n"
> - xml:lang="en"
> - lang="en"
> - dir="ltr"
> - metal:use-macro="view/macro:page/onecolumn"
> + metal:use-macro="view/macro:page/main_only"
> i18n:domain="launchpad"
> >
> <body>
> <div metal:fill-slot="main">
> <div metal:use-macro="context/@@launchpad_form/form">
> <metal:extra-info fill-slot="extra_info">
> - <h1>Change your e-mail settings</h1>
> <h2>Your e-mail addresses</h2>
> - <tal:block condition="context/preferredemail">
> - <p>Your preferred contact address for all Launchpad e-mail is:</p>
> - <ul id="preferred-email">
> - <li class="mail"><b tal:content="context/preferredemail/email" /></li>
> - </ul>
> - </tal:block>
> -
> - <tal:block tal:condition="not: context/preferredemail">
> - <p id="no-contact-address">
> - Currently you don't have a contact address in
> - Launchpad.
> - </p>
> - </tal:block>
> + <p tal:condition="context/preferredemail">
> + Your preferred contact address for all Launchpad e-mail is:
> + <b tal:content="context/preferredemail/email" />
> + </p>
> + <p tal:condition="not: context/preferredemail"
> + id="no-contact-address">
> + Currently you don't have a contact address in Launchpad.
> + </p>
> </metal:extra-info>
>
> <metal:widgets fill-slot="widgets">
> -
> <table class="form">
> <tal:validated tal:condition="view/validated_addresses">
> <tal:widget define="widget nocall:view/widgets/VALIDATED_SELECTED">
> @@ -87,24 +77,21 @@
> lists. You can subscribe to a team's mailing list using any of
> your verified email addresses.</p>
>
> - <table class="form">
> + <table class="listing" style="width: 45em;">
> <thead>
> - <th style="white-space:nowrap">For this mailing list...</th>
> - <td><strong>Subscribe with this address</strong></td>
> + <th style="white-space:nowrap">For mailing list</th>

Missing semi-colon

> + <th>Subscribe with</th>
> </thead>
> <tbody>
> - <metal:block tal:repeat="widget view/mailing_list_widgets">
> - <metal:block use-macro="context/@@launchpad_form/widget_row" />
> - </metal:block>
> - <tr>
> - <td></td>
> - <td>
> - <input tal:replace="structure
> - view/action_update_subscriptions/render" />
> - </td>
> - </tr>
> + <tr tal:repeat="widget view/mailing_list_widgets">
> + <td tal:content="structure widget/team/fmt:link">Team</td>
> + <td tal:content="structure widget/widget">Widget</td>
> + </tr>
> </tbody>
> </table>
> + <div style="padding-top: 1em; padding-bottom: 1em;">
> + <input tal:replace="structure view/action_update_subscriptions/render" />
> + </div>
> </form>
>
> <form action=""

review: Approve (code)

« Back to merge proposal