Code review comment for lp:~jpds/launchpad/fix_517020

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 be an official mirror of this distribution'),
> required=False, readonly=False, default=True))
> status = exported(Choice(
> - title=_('Status'), required=True, readonly=False,
> + title=_('Status'), description=_("The current status of a mirror's "
> + "registration."), required=True, readonly=False,

If it's not already, this should be tested too (ie. who is allowed to write
to this field.

And same formatting issues as before.

> vocabulary=MirrorStatus))
>
> title = Attribute('The title of this mirror')
> @@ -355,17 +358,20 @@
> last_probe_record = Attribute(
> 'The last MirrorProbeRecord for this mirror.')
> all_probe_records = Attribute('All MirrorProbeRecords for this mirror.')
> - has_ftp_or_rsync_base_url = exported(Bool(
> - title=_('Does this mirror have a ftp or rsync base URL?')))
> + has_ftp_or_rsync_base_url = Bool(
> + title=_('Does this mirror have a FTP or Rsync base URL?'))
> base_url = Attribute('The HTTP or FTP base URL of this mirror')
> date_created = exported(Datetime(
> - title=_('Date Created'), required=True, readonly=True))
> - date_reviewed = Datetime(
> - title=_('Date reviewed'), required=False, readonly=False)
> - whiteboard = Whiteboard(
> + title=_('Date Created'), description=_("The date on which this mirror "
> + "was registered"), required=True, readonly=True))

Same formatting issues.

> + date_reviewed = exported(Datetime(
> + title=_('Date reviewed'), description=_("The date on which this "
> + "mirror was last reviewed by a mirror admin."), required=False,
> + readonly=True))

Same formatting issues.

> + whiteboard = exported(Whiteboard(
> title=_('Whiteboard'), required=False,
> description=_("Notes on the current status of the mirror (only "
> - "visible to admins and the mirror's registrant)."))
> + "visible to admins and the mirror's registrant).")))

It would be good to explicitely define whether htis is readonly.

>
> @invariant
> def mirrorMustHaveHTTPOrFTPURL(mirror):
>
> === modified file 'lib/lp/registry/stories/webservice/xx-distribution-mirror.txt'
> --- a/lib/lp/registry/stories/webservice/xx-distribution-mirror.txt 2010-02-01 23:17:27 +0000
> +++ b/lib/lp/registry/stories/webservice/xx-distribution-mirror.txt 2010-02-04 13:07:13 +0000
> @@ -13,21 +13,23 @@
> >>> pprint_entry(canonical_archive_json)
> content: u'Archive'
> 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://archive.ubuntu.com/ubuntu/'
> name: u'canonical-archive'
> official_candidate: True
> owner_link: u'http://.../~mark'
> resource_type_link: u'http://.../#distribution_mirror'
> + reviewer_link: None
> rsync_base_url: None
> self_link: u'http://.../ubuntu/+mirror/canonical-archive'
> speed: u'100 Mbps'
> status: u'Official'
> + whiteboard: None
>
> And CD image mirrors:
>
> @@ -37,21 +39,23 @@
> >>> pprint_entry(canonical_releases_json)
> 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'
> resource_type_link: u'http://.../#distribution_mirror'
> + reviewer_link: None
> rsync_base_url: None
> self_link: u'http://.../ubuntu/+mirror/canonical-releases'
> speed: u'100 Mbps'
> status: u'Official'
> + whiteboard: None
>
> = Distribution Mirror Custom Operations =
>
>

As mentioned, this documentation really should document which fields
are writeable and by whom.

Cheers