Your merge proposal should show the output of make lint to verify your
changes did not have any cruft. It will also inform you of style mistakes
that must be fixed before making a merge proposal.

I have some ideas to improve the code, and I think the interface and model
are missing essential documentation and unit tests. I suspect that the
story tests (which are integration tests) are test what should be testing
as documentation and unit tests.

> === modified file 'lib/lp/registry/interfaces/distribution.py'
> --- lib/lp/registry/interfaces/distribution.py	2010-02-27 10:19:18 +0000
> +++ lib/lp/registry/interfaces/distribution.py	2010-03-06 00:09:35 +0000
> @@ -318,6 +318,16 @@
>          if it's not found.
>          """
>  
> +    @operation_parameters(
> +        country=copy_field(IDistributionMirror['country'], required=True),
> +        mirror_type=copy_field(IDistributionMirror['content'], required=True))
> +    @operation_returns_entry(IDistributionMirror)
> +    @export_read_operation()
> +    def getCountryMirror(country, mirror_type):
> +        """Return the country DNS mirror for a given country and content
> +        type.
> +        """
> +

This docstring does not follow PEP 257. The first sentence must be one line.
Subsequent sentences may follow after a blank line:
    http://www.python.org/dev/peps/pep-0257/

Think this fixes the issue:

        """Return the country DNS mirror for a country and content type."""

> === modified file 'lib/lp/registry/interfaces/distributionmirror.py'
> --- lib/lp/registry/interfaces/distributionmirror.py	2010-02-22 15:50:06 +0000
> +++ lib/lp/registry/interfaces/distributionmirror.py	2010-03-06 00:09:35 +0000
> @@ -6,21 +6,23 @@
>  __metaclass__ = type
>  
>  __all__ = [
> +'CannotTransitionToCountryMirror',
> +'CountryMirrorAlreadySet',
>  'IDistributionMirror',
> -'IDistributionMirrorAdminRestricted',
> -'IDistributionMirrorEditRestricted',
> -'IDistributionMirrorPublic',
>  'IMirrorDistroArchSeries',
>  'IMirrorDistroSeriesSource',
>  'IMirrorProbeRecord',
>  'IDistributionMirrorSet',
>  'IMirrorCDImageDistroSeries',
>  'PROBE_INTERVAL',
> -'UnableToFetchCDImageFileList',
>  'MirrorContent',
>  'MirrorFreshness',
> +'MirrorHasNoHTTPUrl',
> +'MirrorNotOfficial',
> +'MirrorNotProbed',
>  'MirrorSpeed',
> -'MirrorStatus']
> +'MirrorStatus',
> +'UnableToFetchCDImageFileList']
 
Per PEP 8, this list of single entries must each be indented and required a
trailing comma; the closing bracket on on a separate line to minimise diffs,
which I can see that this diff is already a victim:

    'MirrorStatus',
    'UnableToFetchCDImageFileList',
    ]
 
> @@ -47,6 +52,44 @@
>  PROBE_INTERVAL = 23
>  
> +class CannotTransitionToCountryMirror(Exception):
> +    """Root exception for transitions to country mirrors.
> +    """

The closing quotes belong on the previous line PEP 257.

> +    webservice_error(400) # HTTP Error: 'Bad Request'.

Launchpad style does not use trailing comments because they interfere with
refactoring. I do not think comments about HTTP codes are informative;
we are expect to know them.

> @@ -386,6 +406,33 @@
>      date_created = exported(Datetime(
>          title=_('Date Created'), required=True, readonly=True,
>          description=_("The date on which this mirror was registered.")))
> +    country_dns_mirror = exported(Bool(
> +        title=_('Country DNS Mirror'),
> +        description=_('Whether this is a country mirror in DNS.'),
> +        required=False, readonly=True, default=False))
> +
> +    reviewer = exported(PublicPersonChoice(
> +        title=_('Reviewer'), required=False, readonly=True,
> +        vocabulary='ValidPersonOrTeam', description=_(
> +            "The person who last reviewed this mirror.")))
> +    date_reviewed = exported(Datetime(
> +        title=_('Date reviewed'), required=False, readonly=True,
> +        description=_(
> +            "The date on which this mirror was last reviewed by a mirror admin.")))

Wrap the code at 78 characters.


=== modified file 'lib/lp/registry/model/distribution.py'
--- lib/lp/registry/model/distribution.py	2010-02-24 15:28:07 +0000
+++ lib/lp/registry/model/distribution.py	2010-03-06 00:09:35 +0000
@@ -399,6 +399,11 @@
         """See `IDistribution`."""
         return DistributionMirror.selectOneBy(distribution=self, name=name)
 
+    def getCountryMirror(self, country, mirror_type):
+        """See `IDistribution`."""
+        return DistributionMirror.selectOneBy(distribution=self,
+            country=country, content=mirror_type, country_dns_mirror=True)

This is using deprecated SQLObject. We want to use storm which provides
better control of caching. The find method take a tuple of types to cache,
and the query. You can then execute the result object using several methods
like: .config(distinct=True), .any(), ..order_by(), one()

    store = Store.of(self) 
    results =  store.find(
        DistributionMirror,
        distribution == self,
        country == country,
        content == mirror_type
        country_dns_mirror == True)
    return results.one()
        


> === modified file 'lib/lp/registry/model/distributionmirror.py'
> --- lib/lp/registry/model/distributionmirror.py	2009-11-15 19:52:54 +0000
> +++ lib/lp/registry/model/distributionmirror.py	2010-03-06 00:09:35 +0000

@@ -145,6 +149,54 @@
             "This mirror has been probed and thus can't be removed.")
         SQLBase.destroySelf(self)
 
> +    def verifyTransitionToCountryMirror(self):
> +        """Verify that a mirror can be set as a country mirror."""

This docstring does not explain how dangerous it can be to use it. I think
this should also be define on the interface or given a leading underscore.

    """Verify that a mirror can be set as a country mirror.

    Return True if valid, otherwise raise a subclass of
    CannotTransitionToCountryMirror.
    """

> +        if self.distribution.getCountryMirror(self.country,
> +            self.content):
> +            # Country already has a country mirror.
> +            raise CountryMirrorAlreadySet(
> +                "%s already has a country %s mirror set." % (self.country.name,
> +                self.content))

Wrap the code at 78 characters.

> +
> +        if not self.isOfficial():
> +            # Only official mirrors may be set as country mirrors.
> +            raise MirrorNotOfficial(
> +                "This mirror may not be set as a country mirror as it is not "
> +                "an official mirror.")
> +
> +        if self.http_base_url is None:
> +            # Country mirrors must have HTTP URLs set.
> +            raise MirrorHasNoHTTPUrl(
> +                "This mirror may not be set as a country mirror as it does "
> +                "not have an HTTP URL set.")
> +
> +        if not self.last_probe_record:
> +            # Only mirrors which have been probed may be set as country
> +            # mirrors.
> +            raise MirrorNotProbed(
> +                "This mirror may not be set as a country mirror as it has not "
> +                "been probed.")

Wrap the code at 78 characters.

> +        # Verification done.
> +        return True
> +
> +    def canTransitionToCountryMirror(self):
> +        """See if a mirror can be set as a country mirror or return False."""
> +        try:
> +            self.verifyTransitionToCountryMirror()
> +        except CannotTransitionToCountryMirror:
> +            return False
> +        return True

Since verifyTransitionToCountryMirror returns True, I think this can be
written:

        try:
            return self.verifyTransitionToCountryMirror()
        except CannotTransitionToCountryMirror:
            return False

> +
> +    def transitionToCountryMirror(self, country_dns_mirror):
> +        """See `IDistributionMirror`."""
> +        # Environment sanity checks.
> +        if country_dns_mirror:
> +            self.verifyTransitionToCountryMirror()
> +
> +        self.country_dns_mirror = country_dns_mirror

I am not sure I understand this. Can country_dns_mirror be None to unset it?
If so the guard should be written like this.

        if country_dns_mirror is not None:
            self.verifyTransitionToCountryMirror()

These model changes require a documentation test doc/distributionmirror.txt
to show the basic behaviour and document how we expect to use them.

I expect to see a unit test verify that each kind of exception is raised
for the state problem.

=== modified file 'lib/lp/registry/stories/distributionmirror/xx-distribution-mirrors.txt'
--- lib/lp/registry/stories/distributionmirror/xx-distribution-mirrors.txt	2009-12-10 23:32:13 +0000
+++ lib/lp/registry/stories/distributionmirror/xx-distribution-mirrors.txt	2010-03-06 00:09:35 +0000
@@ -40,13 +40,12 @@
     >>> print browser.title
     Mirrors of Ubuntu Linux...
     >>> print_mirrors_by_countries(browser.contents)
-    Antarctica:
-      [(u'Archive-mirror2',    u'http', u'128 Kbps', u'Six hours behind')]
+    Antarctica: [(u'Archive-mirror2', u'http', u'128 Kbps', u'Six hours behind')]
     France:
       [(u'Archive-404-mirror', u'http', u'512 Kbps', u'Last update unknown'),
-       (u'Archive-mirror',     u'http', u'128 Kbps', u'Last update unknown')]
-    United Kingdom:
-      [(u'Canonical-archive',  u'http', u'100 Mbps', u'Last update unknown')]
+       (u'Archive-mirror', u'http', u'128 Kbps', u'Last update unknown')]
+    United Kingdom: [(u'Canonical-archive', u'http', u'100 Mbps', u'Last update unknown')]

Wrap the narrative at 78 characters.
 

> === modified file 'lib/lp/registry/stories/webservice/xx-distribution-mirror.txt'
> --- lib/lp/registry/stories/webservice/xx-distribution-mirror.txt	2010-02-23 19:40:45 +0000
> +++ lib/lp/registry/stories/webservice/xx-distribution-mirror.txt	2010-03-06 00:09:35 +0000
> @@ -229,6 +236,219 @@
>      status: u'Unofficial'
>      whiteboard: u'This mirror is too shiny to be true'
>  
> += Country DNS mirrors =

We use rest headers now. We are converting all tests to rest when we find
that they are moin. eg.

Country DNS mirrors
-------------------

> +Country DNS mirrors are mirrors which have been assigned $CC.archive or
> +$CC.releases.ubuntu.com, these are tracked in Launchpad.
> +
> +    >>> from lp.registry.interfaces.distribution import IDistributionSet
> +    >>> login('<email address hidden>')
> +    >>> ubuntu_distro = getUtility(IDistributionSet).getByName('ubuntu')
> +    >>> de_archive = factory.makeMirror(ubuntu_distro,
> +    ...     "Technische Universitaet Dresden", country = 82,
> +    ...     http_url="http://ubuntu.mirror.tudos.de/ubuntu/")
> +    >>> davis_station = factory.makeMirror(ubuntu_distro,
> +    ...     "Davis Station", country=9,
> +    ...     http_url="http://mirror.davis.antarctica.org/ubuntu")
> +    >>> de_archive_prober_log = factory.makeMirrorProbeRecord(de_archive)
> +    >>> logout()
> +
> +    >>> review_patch = {
> +    ...     u'official_candidate': True,
> +    ...     u'status': 'Official'
> +    ... }
> +
> +    >>> de_archive_mirror = webservice.named_get(
> +    ...     ubuntu['self_link'], 'getMirrorByName',
> +    ...     name='ubuntu.mirror.tudos.de-archive').jsonBody()
> +
> +    >>> response = karl_webservice.patch(
> +    ...     de_archive_mirror['self_link'], 'application/json',
> +    ...     dumps(review_patch)).jsonBody()
> +
> +    >>> pprint_entry(response)
> +    content: u'Archive'
> +    country_dns_mirror: False
> +    country_link: u'http://.../+countries/DE'
> +    date_created: ...
> +    date_reviewed: None
> +    description: None
> +    displayname: u'Technische Universitaet Dresden'
> +    distribution_link: u'http://.../ubuntu'
> +    enabled: False
> +    ftp_base_url: None
> +    http_base_url: u'http://ubuntu.mirror.tudos.de/ubuntu/'
> +    name: u'ubuntu.mirror.tudos.de-archive'
> +    official_candidate: True
> +    owner_link: u'http://.../~ubuntu-team'
> +    resource_type_link: u'http://.../#distribution_mirror'
> +    reviewer_link: None
> +    rsync_base_url: None
> +    self_link: u'http://.../ubuntu/+mirror/ubuntu.mirror.tudos.de-archive'
> +    speed: u'256 Kbps'
> +    status: u'Official'
> +    whiteboard: None
> +
> +Normal users can access country_dns_mirror, however, they may not change it:
> +
> +    >>> patch = {
> +    ...     u'country_dns_mirror': True
> +    ... }
> +
> +    >>> response = test_webservice.patch(
> +    ...     de_archive_mirror['self_link'], 'application/json',
> +    ...     dumps(patch))
> +    >>> print response
> +    HTTP/1.1 401 Unauthorized
> +    Status: 401 Unauthorized
> +    ...
> +    Unauthorized: (<DistributionMirror ...>, 'transitionToCountryMirror',
> +      'launchpad.Admin')
> +    <BLANKLINE>
> +
> +Mirror listing administrators may change the status however:
> +
> +    >>> response = karl_webservice.patch(
> +    ...     de_archive_mirror['self_link'], 'application/json',
> +    ...     dumps(patch)).jsonBody()
> +    >>> pprint_entry(response)
> +    content: u'Archive'
> +    country_dns_mirror: True
> +    country_link: u'http://.../+countries/DE'
> +    ...
> +
> +And change it back:
> +
> +    >>> unmark_country_patch = {
> +    ...     u'country_dns_mirror': False
> +    ... }
> +    >>> response = karl_webservice.patch(
> +    ...     de_archive_mirror['self_link'], 'application/json',
> +    ...     dumps(unmark_country_patch)).jsonBody()
> +    >>> pprint_entry(response)
> +    content: u'Archive'
> +    country_dns_mirror: False
> +    country_link: u'http://.../+countries/DE'
> +    ...

I am not sure this last test is valuable. This looks like it is just
making the test run longer.

This section looks like an attempt at a unit test of the model, and I do
not think this is appropriate for a integration test.

> +There cannot be multiple country mirrors of one type for one country:
> +
> +    >>> login('<email address hidden>')
> +    >>> from lp.registry.interfaces.distributionmirror import IDistributionMirrorSet
> +    >>> davis_station_archive = getUtility(IDistributionMirrorSet).getByName(
> +    ...     'mirror.davis.antarctica.org-archive')
> +    >>> proberecord = factory.makeMirrorProbeRecord(davis_station_archive)
> +    >>> logout()
> +
> +    >>> davis_station_archive_json = webservice.named_get(
> +    ...     ubuntu['self_link'], 'getMirrorByName',
> +    ...     name="mirror.davis.antarctica.org-archive").jsonBody()
> +    >>> pprint_entry(davis_station_archive_json)
> +    content: u'Archive'
> +    country_dns_mirror: False
> +    country_link: u'http://.../+countries/AQ'
> +    ...
> +
> +    >>> archive_mirror2 = webservice.named_get(
> +    ...     ubuntu['self_link'], 'getMirrorByName',
> +    ...     name='archive-mirror2').jsonBody()
> +    >>> pprint_entry(archive_mirror2)
> +    content: u'Archive'
> +    country_dns_mirror: False
> +    country_link: u'http://.../+countries/AQ'
> +    ...
> +
> +    >>> # Mark davis_station_archive_json official.
> +    >>> response = karl_webservice.patch(
> +    ...     davis_station_archive_json['self_link'], 'application/json',
> +    ...     dumps(review_patch)).jsonBody()
> +    >>> # Mark davis_station_archive_json as country mirror.
> +    >>> response = karl_webservice.patch(
> +    ...     davis_station_archive_json['self_link'], 'application/json',
> +    ...     dumps(patch)).jsonBody()
> +    >>> response = karl_webservice.patch(
> +    ...     archive_mirror2['self_link'], 'application/json',
> +    ...     dumps(patch)).jsonBody()
> +    Traceback (most recent call last):
> +    ...
> +    ValueError: Antarctica already has a country Archive mirror set.
> +    <BLANKLINE>
> +    Traceback (most recent call last):
> +    ...
> +    CountryMirrorAlreadySet: Antarctica already has a country Archive mirror set.
> +    <BLANKLINE>
> +
> +Mirrors which have not been probed may not be marked as country mirrors:
> +
> +    >>> archive_mirror = webservice.named_get(
> +    ...     ubuntu['self_link'], 'getMirrorByName',
> +    ...     name='archive-mirror').jsonBody()
> +    >>> response = karl_webservice.patch(
> +    ...     archive_mirror['self_link'], 'application/json',
> +    ...     dumps(patch)).jsonBody()
> +    Traceback (most recent call last):
> +    ...
> +    ValueError: This mirror may not be set as a country mirror as it has not been probed.
> +    <BLANKLINE>
> +    Traceback (most recent call last):
> +    ...
> +    MirrorNotProbed: This mirror may not be set as a country mirror as it has not been probed.
> +
> +Mirrors which are not official or do not have an HTTP URL may not be set as
> +country mirrors:
> +
> +    >>> login('<email address hidden>')
> +    >>> us_mirror = factory.makeMirror(ubuntu_distro, "OSU Open Source Lab",
> +    ...     country=226,
> +    ...     ftp_url="ftp://ubuntu.osuosl.org/pub/ubuntu/")
> +    >>> logout()
> +
> +    >>> us_mirror_json = webservice.named_get(
> +    ...     ubuntu['self_link'], 'getMirrorByName',
> +    ...     name='ubuntu.osuosl.org-archive').jsonBody()
> +    >>> pprint_entry(us_mirror_json)
> +    content: u'Archive'
> +    country_dns_mirror: False
> +    country_link: u'http://.../+countries/US'
> +    ...
> +    ftp_base_url: u'ftp://ubuntu.osuosl.org/pub/ubuntu/'
> +    http_base_url: None
> +    ...
> +
> +    >>> response = karl_webservice.patch(
> +    ...     us_mirror_json['self_link'], 'application/json',
> +    ...     dumps(patch)).jsonBody()
> +    Traceback (most recent call last):
> +    ...
> +    ValueError: This mirror may not be set as a country mirror as it is not an official mirror.
> +    <BLANKLINE>
> +    Traceback (most recent call last):
> +    ...
> +    MirrorNotOfficial: This mirror may not be set as a country mirror as it is not an official mirror.
> +
> +    >>> response = karl_webservice.patch(
> +    ...     us_mirror_json['self_link'], 'application/json',
> +    ...     dumps(review_patch)).jsonBody()
> +    >>> pprint_entry(response)
> +    content: u'Archive'
> +    country_dns_mirror: False
> +    ...
> +    official_candidate: True
> +    ...
> +    status: u'Official'
> +    ...
> +
> +    >>> response = karl_webservice.patch(
> +    ...     us_mirror_json['self_link'], 'application/json',
> +    ...     dumps(patch)).jsonBody()
> +    Traceback (most recent call last):
> +    ...
> +    ValueError: This mirror may not be set as a country mirror as it does not have an HTTP URL set.
> +    <BLANKLINE>
> +    Traceback (most recent call last):
> +    ...
> +    MirrorHasNoHTTPUrl: This mirror may not be set as a country mirror as it does not have an HTTP URL set.

By testing this as a doc test and more importantly as a unit test we are
certain that the behaviour is intrinsic to the object. Unit test execute
much faster than integration tests. All the tests in stories are intended
to be simple verifications that a user can accomplish a task.

=== modified file 'lib/lp/registry/stories/webservice/xx-distribution.txt'
--- lib/lp/registry/stories/webservice/xx-distribution.txt	2010-02-23 17:36:27 +0000
+++ lib/lp/registry/stories/webservice/xx-distribution.txt	2010-03-06 00:09:35 +0000
@@ -123,6 +123,7 @@
     ...     name='canonical-releases').jsonBody()
     >>> pprint_entry(canonical_releases)
     content: u'CD Image'
+    country_dns_mirror: False
     country_link: u'http://.../+countries/GB'
     date_created: u'2006-10-16T18:31:43.434567+00:00'
     date_reviewed: None
@@ -142,3 +143,95 @@
     speed: u'100 Mbps'
     status: u'Official'
     whiteboard: None
> +
> +"getCountryMirror" returns the country DNS mirror for a given country;
> +returning None if there isn't one.
> +
> +    >>> # Prepare stuff.
> +    >>> from lp.registry.interfaces.distribution import IDistributionSet
> +    >>> from zope.component import getUtility
> +    >>> login('<email address hidden>')
> +    >>> ubuntu_distro = getUtility(IDistributionSet).getByName('ubuntu')
> +    >>> davis_station = factory.makeMirror(ubuntu_distro,
> +    ...     "Davis Station", country=9,
> +    ...     http_url="http://mirror.davis.antarctica.org/ubuntu")
> +    >>> davis_station_log = factory.makeMirrorProbeRecord(davis_station)
> +    >>> logout()
> +
> +    >>> # Prepare karl user.
> +    >>> from canonical.launchpad.testing.pages import webservice_for_person
> +    >>> from canonical.launchpad.webapp.interfaces import OAuthPermission
> +    >>> from lp.registry.interfaces.person import IPersonSet
> +    >>> from simplejson import dumps
> +    >>> login(ANONYMOUS)
> +    >>> karl_db = getUtility(IPersonSet).getByName('karl')
> +    >>> karl_webservice = webservice_for_person(karl_db,
> +    ...     permission=OAuthPermission.WRITE_PUBLIC)
> +    >>> logout()
> +
> +    >>> # Mark new mirror as official
> +    >>> officialness_patch = {
> +    ...     u'official_candidate': True,
> +    ...     u'status': 'Official'
> +    ... }
> +    >>> countryness_patch = {
> +    ...     u'country_dns_mirror': True
> +    ... }
> +    >>> antarctica_patch_target = webservice.named_get(
> +    ...     ubuntu['self_link'], 'getMirrorByName',
> +    ...     name='mirror.davis.antarctica.org-archive').jsonBody()
> +    ... )
> +    >>> response = karl_webservice.patch(
> +    ...     antarctica_patch_target['self_link'], 'application/json',
> +    ...     dumps(officialness_patch))
> +    >>> response = karl_webservice.patch(
> +    ...     antarctica_patch_target['self_link'], 'application/json',
> +    ...     dumps(countryness_patch))
> +
> +    >>> antarctica = webservice.get("/+countries/AQ").jsonBody()
> +    >>> antarctica_country_mirror_archive = webservice.named_get(
> +    ...     ubuntu['self_link'], 'getCountryMirror',
> +    ...     country=antarctica['self_link'],
> +    ...     mirror_type="Archive").jsonBody()
> +    >>> pprint_entry(antarctica_country_mirror_archive)
> +    content: u'Archive'
> +    country_dns_mirror: True
> +    country_link: u'http://.../+countries/AQ'
> +    date_created: ...
> +    date_reviewed: None
> +    description: None
> +    displayname: u'Davis Station'
> +    distribution_link: u'http://.../ubuntu'
> +    enabled: False
> +    ftp_base_url: None
> +    http_base_url: u'http://mirror.davis.antarctica.org/ubuntu/'
> +    name: u'mirror.davis.antarctica.org-archive'
> +    official_candidate: True
> +    owner_link: u'http://.../~ubuntu-team'
> +    resource_type_link: u'http://.../#distribution_mirror'
> +    reviewer_link: None
> +    rsync_base_url: None
> +    self_link: u'http://.../ubuntu/+mirror/mirror.davis.antarctica.org-archive'
> +    speed: u'256 Kbps'
> +    status: u'Official'
> +    whiteboard: None
> +
> +    >>> uk = webservice.get("/+countries/GB").jsonBody()
> +    >>> uk_country_mirror_archive = webservice.named_get(
> +    ...     ubuntu['self_link'], 'getCountryMirror',
> +    ...     country=uk['self_link'],
> +    ...     mirror_type="Archive")
> +    >>> print uk_country_mirror_archive.jsonBody()
> +    None
> +
> +For "getCountryMirror", the mirror_type parameter must be "Archive" or
> +"CD Images":
> +
> +    >>> uk_country_mirror_archive = webservice.named_get(
> +    ...     ubuntu['self_link'], 'getCountryMirror',
> +    ...     country=uk['self_link'],
> +    ...     mirror_type="Bogus")
> +    >>> print uk_country_mirror_archive.jsonBody()
> +    Traceback (most recent call last):
> +    ...
> +    ValueError: mirror_type: Invalid value "Bogus". Acceptable values are: Archive, CD Image

I think this too is out of scope. This looks like a documentation test.
the api test only needs to verify the method is exposed. Rigorous tests of
a method should be done in a unit test.