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>