Merge lp:~jpds/launchpad/fix_517020 into lp:launchpad

= Summary =

This branch fixes various problems with the newly exposed distribution_mirror's as described in bug #517020.

It also exposes various other things whose permissions to access are restricted by Zope's configuration.

Hi Jonathan,

It's great that you're so quick to get in there and fix his. Other than some formatting issues, the only point that I think needs addressing here is testing the writable of the fields you're changing/exporting.

> === modified file 'lib/lp/registry/interfaces/distributionmirror.py'
> --- a/lib/lp/registry/interfaces/distributionmirror.py 2010-02-01 23:17:27 +0000
> +++ b/lib/lp/registry/interfaces/distributionmirror.py 2010-02-04 13:07:13 +0000
> @@ -287,17 +287,19 @@
>
> id = Int(title=_('The unique id'), required=True, readonly=True)
> owner = exported(PublicPersonChoice(
> - title=_('Owner'), required=False, readonly=True,
> - vocabulary='ValidOwner'))
> - reviewer = PublicPersonChoice(
> - title=_('Reviewer'), required=False, readonly=False,
> - vocabulary='ValidPersonOrTeam')
> + title=_('Owner'), description=_("The person who is set as the "
> + "current administrator of this mirror."), required=True,
> + readonly=False, vocabulary='ValidOwner'))

So the owner attribute is no-longer readonly? If that's intentional, we really
should have that tested/documented (ie. who can/can't write to it).

Also, just a few small indentation things looking at
the style guide. It might be easier to format this like:

    title=_('Owner'), description=_(
 "The person who is set as the current administrator of this mirror."),
    required=True, readonly=False, vocabulary='ValidOwner')

or

    title=_('Owner'), required=True, readonly=False, vocabulary='ValidOwner'
    description=_(
 "The person who is set as the current administrator of this mirror."))

> + reviewer = exported(PublicPersonChoice(
> + title=_('Reviewer'), description=_("The person who last reviewed this "
> + "mirror."), required=False, readonly=True,
> + vocabulary='ValidPersonOrTeam'))

So the main change is that readonly is now true. Please ensure this is tested
also.

Similar formatting issues here. Although, I can't see anything in the
style-guide, but I thought that concatenated strings needed to be aligned (if
you have to break them).

> distribution = exported(
> Reference(
> Interface,
> # Really IDistribution, circular import fixed in
> # _schema_circular_imports.
> - title=_("Distribution"), required=True,
> + title=_("Distribution"), required=True, readonly=True,

Again, this needs to be tested (so it doesn't happen again).

> description=_("The distribution that is mirrored")))
> name = exported(DistributionMirrorNameField(
> title=_('Name'), required=True, readonly=False,
> @@ -325,7 +327,7 @@
> description=_('e.g.: rsync://archive.ubuntu.com/ubuntu/')))
> enabled = exported(Bool(
> title=_('This mirror was probed successfully.'),
> - required=False, readonly=False, default=False))
> + required=False, readonly=True, default=False))

And here too.

> speed = exported(Choice(
> title=_('Link Speed'), required=True, readonly=False,
> vocabulary=MirrorSpeed))
> @@ -343,7 +345,8 @@
> title=_('Apply to...

> === modified file 'lib/lp/registry/interfaces/distributionmirror.py'
> > --- a/lib/lp/registry/interfaces/distributionmirror.py 2010-02-01 23:17:27 +0000
> > +++ b/lib/lp/registry/interfaces/distributionmirror.py 2010-02-04 13:07:13 +0000
> > @@ -287,17 +287,19 @@
> >
> > id = Int(title=_('The unique id'), required=True, readonly=True)
> > owner = exported(PublicPersonChoice(
> > - title=_('Owner'), required=False, readonly=True,
> > - vocabulary='ValidOwner'))
> > - reviewer = PublicPersonChoice(
> > - title=_('Reviewer'), required=False, readonly=False,
> > - vocabulary='ValidPersonOrTeam')
> > + title=_('Owner'), description=_("The person who is set as the "
> > + "current administrator of this mirror."), required=True,
> > + readonly=False, vocabulary='ValidOwner'))
>
> So the owner attribute is no-longer readonly? If that's intentional, we really
> should have that tested/documented (ie. who can/can't write to it).

This is now tested in registry/stories/webservice/xx-distribution-mirror.txt.

> Also, just a few small indentation things looking at
> the style guide. It might be easier to format this like:
>
> title=_('Owner'), description=_(
> "The person who is set as the current administrator of this mirror."),
> required=True, readonly=False, vocabulary='ValidOwner')
>
> or
>
> title=_('Owner'), required=True, readonly=False, vocabulary='ValidOwner'
> description=_(
> "The person who is set as the current administrator of this mirror."))

I've changed all the fields to the latter.

> > + reviewer = exported(PublicPersonChoice(
> > + title=_('Reviewer'), description=_("The person who last reviewed this "
> > + "mirror."), required=False, readonly=True,
> > + vocabulary='ValidPersonOrTeam'))
>
> So the main change is that readonly is now true. Please ensure this is tested
> also.

Also done.

> Similar formatting issues here. Although, I can't see anything in the
> style-guide, but I thought that concatenated strings needed to be aligned (if
> you have to break them).
>
> > distribution = exported(
> > Reference(
> > Interface,
> > # Really IDistribution, circular import fixed in
> > # _schema_circular_imports.
> > - title=_("Distribution"), required=True,
> > + title=_("Distribution"), required=True, readonly=True,
>
> Again, this needs to be tested (so it doesn't happen again).

This is now tested.

> > description=_("The distribution that is mirrored")))
> > name = exported(DistributionMirrorNameField(
> > title=_('Name'), required=True, readonly=False,
> > @@ -325,7 +327,7 @@
> > description=_('e.g.: rsync://archive.ubuntu.com/ubuntu/')))
> > enabled = exported(Bool(
> > title=_('This mirror was probed successfully.'),
> > - required=False, readonly=False, default=False))
> > + required=False, readonly=True, default=False))
>
> And here too.

Likewise.

> > speed = exported(Choice(
> > title=_('Link Speed'), required=True, readonly=False,
> > vocabul...

Thanks for the changes Jonathan. I've approved the review, but do have a couple of things below that I'm interested to hear back on (they could be just oversights on my part).

> Done and full diff since r10278:
> === modified file 'lib/lp/registry/stories/webservice/xx-distribution-
> mirror.txt'
> --- lib/lp/registry/stories/webservice/xx-distribution-mirror.txt
> 2010-02-04 12:22:19 +0000
> +++ lib/lp/registry/stories/webservice/xx-distribution-mirror.txt
> 2010-02-05 11:32:41 +0000
> @@ -57,6 +57,88 @@
> status: u'Official'
> whiteboard: None
>
> += Security checks =
> +
> +People who are not mirror listing admins or the mirrors registrar may not
> +change the owner's of mirrors:
> +
> + >>> from canonical.launchpad.testing.pages import webservice_for_person
> + >>> from canonical.launchpad.webapp.interfaces import OAuthPermission
> + >>> from zope.component import getUtility
> + >>> from lp.registry.interfaces.person import IPersonSet
> + >>> from simplejson import dumps
> + >>> login(ANONYMOUS)
> + >>> karl_db = getUtility(IPersonSet).getByName('karl')
> + >>> test_db = getUtility(IPersonSet).getByName('name12')
> + >>> karl_webservice = webservice_for_person(karl_db,
> + ... permission=OAuthPermission.WRITE_PUBLIC)
> + >>> test_webservice = webservice_for_person(test_db,
> + ... permission=OAuthPermission.WRITE_PUBLIC)
> + >>> logout()
> + >>> karl = webservice.get("/~karl").jsonBody()
> + >>> patch = {
> + ... u'owner_link': karl['self_link']
> + ... }
> +
> +Now trying to set the owner using Sample Person's webservice is not
> authorized.
> +
> + >>> response = test_webservice.patch(
> + ... canonical_archive['self_link'], 'application/json', dumps(patch))
> + >>> print response.getheader('status')
> + 401 Unauthorized
> +
> +But if we use Karl, the mirror listing admin's, webservice, we can update the

s/admin's,/admin's

> owner.
> +
> + >>> response = karl_webservice.patch(
> + ... canonical_archive['self_link'], 'application/json', dumps(patch))
> + >>> print response.getheader('status')
> + 209 Content Returned
> +
> + >>> patched_canonical_archive = response.jsonBody()
> + >>> print patched_canonical_archive['owner_link']
> + http://.../~karl
> +
> +Some attributes are read-only via the API:
> +
> + >>> distros = webservice.get("/distros").jsonBody()
> + >>> distro = distros['entries'][3]
> + >>> debian = webservice.get(distro['self_link']).jsonBody()

I haven't checked, but is the above line necessary? you're getting distro['self_link'] which should be equivalent to distro? (ie. distro == debian?). Actually, you only seem to use debian['self_link'] below anyway (which should be equivalent to distro['self_link'] unless I'm missing something).

> + >>> patch = {
> + ... u'date_reviewed' : u'2010-02-04T17:19:16.424198+00:00',
> + ... u'distribution_link' : debian['self_link'],
> + ... u'enabled' : False,
> + ... u'reviewer_link' : karl['self_link']
> + ... }
> + >>> response = karl_webservice.patch(
> + ... canonical_releases['self_link'], 'application/json',
> dum...

The branch failed a test on distribution.txt, as I was just testing distribution-mirror.txt. The last commit fixes this and this is the diff:

=== modified file 'lib/lp/registry/stories/webservice/xx-distribution.txt'
--- lib/lp/registry/stories/webservice/xx-distribution.txt 2010-02-01 23:17:27 +0000
+++ lib/lp/registry/stories/webservice/xx-distribution.txt 2010-02-05 18:18:28 +0000
@@ -124,18 +124,20 @@
     >>> pprint_entry(canonical_releases)
     content: u'CD Image'
     date_created: u'2006-10-16T18:31:43.434567+00:00'
+ date_reviewed: None
     description: None
     displayname: None
     distribution_link: u'http://.../ubuntu'
     enabled: True
     ftp_base_url: None
- has_ftp_or_rsync_base_url: False
     http_base_url: u'http://releases.ubuntu.com/'
     name: u'canonical-releases'
     official_candidate: True
     owner_link: u'http://.../~mark'
+ reviewer_link: None
     resource_type_link: u'http://.../#distribution_mirror'
     rsync_base_url: None
     self_link: u'http://.../ubuntu/+mirror/canonical-releases'
     speed: u'100 Mbps'
     status: u'Official'
+ whiteboard: None

>> >>> is_official_mirror =
>> webservice.named_get(canonical_releases['self_link'],
>> ... 'isOfficial').jsonBody()
>> >>> print is_official_mirror
>> - True
>> + False
>
> I can't see anything that should have changed the expected value of this test? Why did it
> change here?

For the record:

<noodles> Also, I can't see why the expected value of is_official_mirror changed at the end of your diff?
<jpds> 13:47:01 <jpds> is_o_m> IT changes because I patch it with: u'status' : 'Unofficial'

Your additional test changes look good. Thanks.