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

On Sep 16, 2009, at 10:24 PM, Brad Crittenden wrote:

>The changes look good. Thanks for taking the time to make a lot of drive-by fixes.
>
>I've got a few questions below.

Thanks for the review 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.

Changed.

>
>> + 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?

Good call. This was cut-and-pasted from the original code, but .startswith()
is better, so I changed this.

>> @@ -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

Cut-and-paste, but good catch! Changed. Here's the incremental.

=== modified file 'lib/lp/registry/browser/person.py'
--- lib/lp/registry/browser/person.py 2009-09-16 21:19:47 +0000
+++ lib/lp/registry/browser/person.py 2009-09-16 23:50:16 +0000
@@ -4195,9 +4195,10 @@
         subscription = mailing_list.getSubscription(self.context)
         if subscription is None:
             return "Don't subscribe"
- if subscription.email_address is None:
+ elif subscription.email_address is None:
             return 'Preferred address'
- return subscription.email_address
+ else:
+ return subscription.email_address

     def _mailing_list_fields(self):
         """Creates a field for each mailing list the user can subscribe to.
@@ -4238,7 +4239,7 @@
         mailing_list_set = getUtility(IMailingListSet)
         widgets = []
         for widget in self.widgets:
- if 'field.subscription.' in widget.name:
+ if widget.name.startswith('field.subscription.'):
                 team_name = widget.label
                 mailing_list = mailing_list_set.get(team_name)
                 assert mailing_list is not None, 'Missing mailing list'

=== modified file 'lib/lp/registry/templates/person-editemails.pt'
--- lib/lp/registry/templates/person-editemails.pt 2009-09-16 21:00:11 +0000
+++ lib/lp/registry/templates/person-editemails.pt 2009-09-16 23:51:33 +0000
@@ -79,7 +79,7 @@

     <table class="listing" style="width: 45em;">
       <thead>
- <th style="white-space:nowrap">For mailing list</th>
+ <th style="white-space:nowrap;">For mailing list</th>
         <th>Subscribe with</th>
       </thead>
       <tbody>