Hi Johnathan. Thanks for undertaking this large effort. I reviewed the code and the UI. There is a lot that needs fixing, but I can help with some of it. Starting with the UI. I like your change to move country_dns_mirror to +edit, but per your original concern, it is not obvious that the mirror admin uses this page to change the state. We strive to move edit and view links into the page where the information is presented so that it is clear to the user that he can change what he is reading. This is a simple one-line addition to the template. However, the addition of a new portlet makes the layout worse. This page has known issues because it was one of the first to be changed for 3.0, but it was not updated as 3.0 style evolved. The registration and status information are not in their correct places: the registering slot and the Mirror information portlet. If they were, the addition of the new field would be easy and obvious. I tinkered with the template to fix the layout and I have a diff that you can apply to your branch to get these changes. In the fixed layout, the official country dns status appears higher on the page and I think that is good. Suggested layout: http://people.canonical.com/~curtis/mirror.png Diff to revise layout: http://people.canonical.com/~curtis/mirror.diff I do not like the info notifications. They make me worry that my other changes were not accepted. Since the form did what I asked it to, there is no need to take such an extraordinary measure. I think they should be removed. I pondered the addition of the star to the mirrors list. Your approach is just like badges that are applied to bugs, so I think it is correct. I think my misgivings are from my unfamiliarity with the star as an icon. I did learn it was official when I placed my mouse over it. I think we can leave it as is. BTW. There is a problem with this merge proposal. It must be merged into db-devel since the schema changes are incompatible with lpnet/edge. If you had proposed the merge with db-devel you would have seen that your db patch conflicts. You need to rename it and update it as suggested by Stuart before this can be tested properly. > === modified file 'lib/lp/registry/browser/distributionmirror.py' > --- lib/lp/registry/browser/distributionmirror.py 2009-12-12 22:15:31 +0000 > +++ lib/lp/registry/browser/distributionmirror.py 2010-01-05 15:14:20 +0000 ... > @@ -275,10 +278,66 @@ > """See `LaunchpadFormView`.""" > return canonical_url(self.context) > > + def validate(self, data): > + dns_mirror_choice = data.get('country_dns_mirror') > + > + # This mirror has not been marked as a country mirror. > + if dns_mirror_choice == True: > + # Safe-guard against multiple country mirrors for one country. > + current_country_mirror = ( > + getUtility(IDistributionMirrorSet).getCountryMirrorForCountry( > + self.context.country, self.context.content)) > + > + # User has decided to mark it as one. > + if current_country_mirror is None: > + # There are none - mark this one as the one. > + self.request.response.addInfoNotification( > + "The mirror %s has been set as the country DNS mirror for " > + "%s." % (self.context.title, self.context.country.name)) This message will be wrong if validation fails in the next few lines or the transaction fails. If this is needed, then it belongs in the action. I think you need to set an flag on the view that can be used by the action to add the notification. addInfoNotification() is use for to confirm the overall action /or/ to point out something extraordinary. This does not look extraordinary. The form is doing what the user asked it to do. > + elif current_country_mirror == self.context: > + return # We are editing the country DNS mirror. Launchpad does not use end comment because they are easy to mis and often require extra work when refactoring code. Move the comment before the early return. > + else: > + # Make sure that there isn't already a mirror for this > + # country already. > + self.setFieldError("country_dns_mirror", "%s already " > + "has a country DNS mirror set, please unset the country " > + "DNS mirror status of %s before continuing." % ( > + self.context.country.name, > + current_country_mirror.title)) The comment does not match the code. The code is clearly setting an error without checking. Is the message incomplete? Can the user unselect the other mirror, then select this mirror? Refrain from using 'Please' in Launchpad instructions per our style guide. > + elif dns_mirror_choice == False and self.context.country_dns_mirror: > + self.request.response.addInfoNotification( > + "The mirror %s has been unset as the country DNS mirror for " > + "%s." % (self.context.title, self.context.country.name)) As above, this is not the place to say something was changed when it has not been changed yet, and I do not think this change is warrants telling the user that only one part of the change was completed. > + def setUpFields(self): > + super(DistributionMirrorEditView, self).setUpFields() > + > + is_admin = check_permission('launchpad.Admin', self.context) > + > + mirror_qualifies = ( > + self.context.last_probe_record is not None > + and self.context.isOfficial() > + and self.context.http_base_url is not None) > + > + # User must be an admin and the mirror must pass the above. > + if not is_admin or not mirror_qualifies: > + self.form_fields = self.form_fields.omit('country_dns_mirror') Launchpad style would remove the empty lines here because there is no special break in execution. > @action(_("Save"), name="save") > def action_save(self, action, data): > - self.updateContextFromData(data) > - self.next_url = canonical_url(self.context) > + # Ensure that we are not changing an archive mirror to a releases one > + # and that it's a country mirror at the same time. > + if data.get('content') != self.context.content: > + if self.context.country_dns_mirror: > + self.setFieldError("content", "Changing the mirror content " > + "type of country DNS mirrors is forbidden.") > + elif data.get('country') != self.context.country: > + if self.context.country_dns_mirror: > + self.setFieldError("country", "Changing the country of country " > + "DNS mirrors is forbidden.") I do not think this validation code belongs here. I would move it to the validate() method. In general, we do not want to enter the action method if we know it can fail. ... > === modified file 'lib/lp/registry/interfaces/distributionmirror.py' > --- lib/lp/registry/interfaces/distributionmirror.py 2009-10-26 18:40:04 +0000 > +++ lib/lp/registry/interfaces/distributionmirror.py 2010-01-05 15:14:20 +0000 > @@ -330,8 +330,11 @@ > 'mirror of packages for this distribution.'), > vocabulary=MirrorContent) > official_candidate = Bool( > - title=_('Apply to be an official mirror of this distribution'), > + title=_('Apply to be an official mirror of this distribution.'), We do not use ending punctuation in field titles because they forms are generated with ':'. This change should be reverted. > required=False, readonly=False, default=True) > + country_dns_mirror = Bool( > + title=_('Set this mirror as an country DNS mirror.'), > + required=False, readonly=False, default=False) Remove the period from the title. > status = Choice( > title=_('Status'), required=True, readonly=False, > vocabulary=MirrorStatus) > @@ -536,6 +539,11 @@ > def getByRsyncUrl(url): > """Return the mirror with the given Rsync URL or None.""" > > + def getCountryMirrorForCountry(country, mirror_type): > + """Return the country mirror for a certain country for the specified > + mirror type. > + """ Python style guides demand that the doc string be a single line, or a line followed by an empty line then a paragraph. I think this will do: """Return the country mirror for a specified country and type.""" ... > === modified file 'lib/lp/registry/model/distributionmirror.py' > --- lib/lp/registry/model/distributionmirror.py 2009-10-26 18:40:04 +0000 > +++ lib/lp/registry/model/distributionmirror.py 2010-01-05 15:14:20 +0000 ... > @@ -402,6 +404,26 @@ > """See IDistributionMirrorSet""" > return DistributionMirror.get(mirror_id) > > + def getCountryMirrorForCountry(self, country, mirror_type): > + """See IDistributionMirrorSet.""" Use backticks for doc tools to know make links: `IDistributionMirrorSet` > + country_id = None > + if country is not None: > + country_id = country.id Launchpad style prefers: if country is None: country_id = None else: country_id = country.id > + base_query = AND( > + DistributionMirror.q.content == mirror_type, > + DistributionMirror.q.enabled == True, > + DistributionMirror.q.http_base_url != None, > + DistributionMirror.q.official_candidate == True, > + DistributionMirror.q.status == MirrorStatus.OFFICIAL, > + DistributionMirror.q.country_dns_mirror == True) > + > + query = AND(DistributionMirror.q.countryID == country_id, base_query) > + > + country_mirror = DistributionMirror.selectOne(query) > + > + return country_mirror Remove the superfluous empty lines. > === 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-01-05 15:14:20 +0000 > @@ -41,12 +41,16 @@ > Mirrors of Ubuntu Linux... > >>> print_mirrors_by_countries(browser.contents) > Antarctica: > - [(u'Archive-mirror2', u'http', u'128 Kbps', u'Six hours behind')] > + [(u'Davis Station', u'', u'http', u'100 Mbps', u'Last update unknown'), > + (u'Archive-mirror2', u'', 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')] > + [(u'Archive-404-mirror', u'', u'http', u'512 Kbps', u'Last update unknown'), Wrap this line so that it fits in 78 characters. > + (u'Archive-mirror', u'', u'http', u'128 Kbps', u'Last update unknown')] > + Germany: > + [(u'Technische Universit\xe4t Dresden', u'', u'http\nrsync', u'10 Gbps', > + u'Last update unknown')] > United Kingdom: > - [(u'Canonical-archive', u'http', u'100 Mbps', u'Last update unknown')] > + [(u'Canonical-archive', u'', u'http', u'100 Mbps', u'Last update unknown')] > >>> find_tags_by_class(browser.contents, 'distromirrorstatusSIXHOURSBEHIND') Wrap these lines so that they fit in 78 characters. ... > @@ -59,13 +63,160 @@ ... +=== Country DNS mirrors === + +Country DNS mirrors are official mirrors whose FQDN have been CNAME'd for +domain records such as gb.archive.ubuntu.com. They are highly reliable and set +during installation, depending on which country the user specified they were +in. + +Mirror listing administrators are the ones with the authority to set mirrors as +country mirrors. + + >>> browser = setupBrowser(auth='Basic