launchpad-reviewers team mailing list archive
-
launchpad-reviewers team
-
Mailing list archive
-
Message #00827
[Merge] lp:~bac/launchpad/bug-39212 into lp:launchpad/devel
Brad Crittenden has proposed merging lp:~bac/launchpad/bug-39212 into lp:launchpad/devel.
Requested reviews:
Launchpad code reviewers (launchpad-reviewers)
Related bugs:
#39212 +editwikinames needs validation of very large values
https://bugs.launchpad.net/bugs/39212
#54696 +editwikinames needs better unique validation.
https://bugs.launchpad.net/bugs/54696
= Summary =
Several of the person/+editthingy pages pre-date LaunchpadFormView. In
a misguided attempt to be super-friendly, the pages allowed in-place
editing of lots of data. Doing so ran the risk that internal
inconsistencies in the submitted batch would cause a failure.
== Proposed fix ==
Change +editwikinames to be a LaunchpadFormView and only allow existing
data to be displayed and marked for removal.
== Pre-implementation notes ==
None. Similar to the recent change to +editjabberids
== Implementation details ==
As above.
== Tests ==
bin/test -vvm lp.registry -t xx-person-edit-wikis.txt
== Demo and Q/A ==
https://launchpad.dev/people/+me/+editwikinames
= Launchpad lint =
Checking for conflicts and issues in changed files.
Linting changed files:
lib/lp/registry/templates/person-editwikinames.pt
lib/lp/registry/stories/person/xx-person-edit-wikis.txt
lib/lp/registry/browser/person.py
--
https://code.launchpad.net/~bac/launchpad/bug-39212/+merge/34196
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~bac/launchpad/bug-39212 into lp:launchpad/devel.
=== modified file 'lib/lp/registry/browser/person.py'
--- lib/lp/registry/browser/person.py 2010-08-30 06:38:53 +0000
+++ lib/lp/registry/browser/person.py 2010-08-31 13:20:59 +0000
@@ -219,6 +219,7 @@
LaunchpadRadioWidgetWithDescription,
LocationWidget,
PasswordChangeWidget,
+ URIWidget,
)
from canonical.widgets.image import ImageChangeWidget
from canonical.widgets.itemswidgets import LabeledMultiCheckBoxWidget
@@ -297,7 +298,10 @@
ITeamMembershipSet,
TeamMembershipStatus,
)
-from lp.registry.interfaces.wikiname import IWikiNameSet
+from lp.registry.interfaces.wikiname import (
+ IWikiName,
+ IWikiNameSet,
+ )
from lp.services.fields import LocationField
from lp.services.openid.adapters.openid import CurrentOpenIDEndPoint
from lp.services.openid.browser.openiddiscovery import (
@@ -3498,35 +3502,76 @@
sCoC_util.modifySignature(sig_id, self.user, comment, False)
-class PersonEditWikiNamesView(LaunchpadView):
+class PersonEditWikiNamesView(LaunchpadFormView):
"""View for ~person/+editwikinames"""
+ schema = IWikiName
+ fields = ['wiki', 'wikiname']
+ # Use custom widgets solely to get the width correct. The URIWidget has a
+ # CSS class that does not respect the displayWidth, thus the need to use a
+ # different cssClass.
+ custom_widget('wiki', URIWidget, displayWidth=40, cssClass="textType")
+ custom_widget('wikiname', TextWidget, displayWidth=40)
+
@property
def label(self):
return smartquote("%s's wiki names" % self.context.displayname)
@property
- def cancel_url(self):
- return canonical_url(self.context, view_name="+edit")
+ def next_url(self):
+ return canonical_url(self.context)
+
+ cancel_url = next_url
+
+ def setUpFields(self):
+ super(PersonEditWikiNamesView, self).setUpFields()
+ if self.context.wiki_names.count() > 0:
+ # Make the wiki and wiki_name entries optional on the edit page if
+ # one or more ids already exist, which allows the removal of ids
+ # without filling out the new wiki fields.
+ wiki_field = self.form_fields['wiki']
+ wikiname_field = self.form_fields['wikiname']
+ # Copy the fields so as not to modify the interface.
+ wiki_field.field = copy_field(wiki_field.field)
+ wiki_field.field.required = False
+ wikiname_field.field = copy_field(wikiname_field.field)
+ wikiname_field.field.required = False
def _validateWikiURL(self, url):
"""Validate the URL.
Make sure that the result is a valid URL with only the
- appropriate schemes.
+ appropriate schemes. The url is assumed to be a string.
"""
+ if url is None:
+ return
try:
uri = URI(url)
if uri.scheme not in ('http', 'https'):
- self.error_message = structured(
- 'The URL scheme "%(scheme)s" is not allowed. '
- 'Only http or https URLs may be used.', scheme=uri.scheme)
- return False
+ self.setFieldError(
+ 'wiki',
+ structured(
+ 'The URL scheme "%(scheme)s" is not allowed. '
+ 'Only http or https URLs may be used.',
+ scheme=uri.scheme))
except InvalidURIError:
- self.error_message = structured(
- '"%(url)s" is not a valid URL.', url=url)
- return False
- return True
+ self.setFieldError(
+ 'wiki',
+ structured(
+ '"%(url)s" is not a valid URL.', url=url))
+
+ def _validateWikiName(self, name):
+ """Ensure the wikiname is valid.
+
+ It must not be longer than 100 characters. Name is assumed to be a
+ string.
+ """
+ max_len = 100
+ if len(name) > max_len:
+ self.setFieldError(
+ 'wikiname',
+ structured(
+ 'The wiki name cannot exceed %d characters.' % max_len))
def _sanitizeWikiURL(self, url):
"""Strip whitespaces and make sure :url ends in a single '/'."""
@@ -3534,72 +3579,82 @@
return url
return '%s/' % url.strip().rstrip('/')
- def initialize(self):
- """Process the WikiNames form."""
- self.error_message = None
- if self.request.method != "POST":
- # Nothing to do
- return
-
- form = self.request.form
- context = self.context
+ def validate(self, data):
+ # If there are already form errors then just show them.
+ if self.errors:
+ return
+ wikiurl = self._sanitizeWikiURL(data.get('wiki'))
+ wikiname = data.get('wikiname')
+ if wikiurl or wikiname:
+ if not wikiurl:
+ self.setFieldError(
+ 'wiki',
+ structured(
+ 'The Wiki URL must be specified.'))
+ if not wikiname:
+ self.setFieldError(
+ 'wikiname',
+ structured(
+ 'The Wiki name must be specified.'))
+
+ if self.errors:
+ return
+
+ if wikiurl is not None:
+ self._validateWikiURL(wikiurl)
+ if wikiname is not None:
+ self._validateWikiName(wikiname)
+
+ if self.errors:
+ return
+
wikinameset = getUtility(IWikiNameSet)
-
- for w in context.wiki_names:
- # XXX: GuilhermeSalgado 2005-08-25:
- # We're exposing WikiName IDs here because that's the only
- # unique column we have. If we don't do this we'll have to
- # generate the field names using the WikiName.wiki and
- # WikiName.wikiname columns (because these two columns make
- # another unique identifier for WikiNames), but that's tricky and
- # not worth the extra work.
- if form.get('remove_%d' % w.id):
- w.destroySelf()
- else:
- wiki = self._sanitizeWikiURL(form.get('wiki_%d' % w.id))
- wikiname = form.get('wikiname_%d' % w.id)
- if not (wiki and wikiname):
- self.error_message = structured(
- "Neither Wiki nor WikiName can be empty.")
- return
- if not self._validateWikiURL(wiki):
- return
- w.wiki = wiki
- w.wikiname = wikiname
-
- wiki = self._sanitizeWikiURL(form.get('newwiki'))
- wikiname = form.get('newwikiname')
- if wiki or wikiname:
- if wiki and wikiname:
- existingwiki = wikinameset.getByWikiAndName(wiki, wikiname)
- if existingwiki and existingwiki.person != context:
- owner_name = urllib.quote(existingwiki.person.name)
- merge_url = (
- '%s/+requestmerge?field.dupe_person=%s'
- % (canonical_url(getUtility(IPersonSet)), owner_name))
- self.error_message = structured(
+ existingwiki = wikinameset.getByWikiAndName(wikiurl, wikiname)
+ if existingwiki:
+ if existingwiki.person != self.context:
+ owner_name = urllib.quote(existingwiki.person.name)
+ merge_url = (
+ '%s/+requestmerge?field.dupe_person=%s'
+ % (canonical_url(getUtility(IPersonSet)), owner_name))
+ self.setFieldError(
+ 'wikiname',
+ structured(
'The WikiName %s%s is already registered by '
'<a href="%s">%s</a>. If you think this is a '
'duplicated account, you can <a href="%s">merge it'
'</a> into your account.',
- wiki, wikiname, canonical_url(existingwiki.person),
- existingwiki.person.displayname, merge_url)
- return
-
- elif existingwiki:
- self.error_message = structured(
- 'The WikiName %s%s already belongs to you.',
- wiki, wikiname)
- return
- if not self._validateWikiURL(wiki):
- return
- wikinameset.new(context, wiki, wikiname)
+ wikiurl, wikiname,
+ canonical_url(existingwiki.person),
+ existingwiki.person.displayname, merge_url))
else:
- self.newwiki = wiki
- self.newwikiname = wikiname
- self.error_message = structured(
- "Neither Wiki nor WikiName can be empty.")
- return
+ # The person already has this wiki.
+ self.setFieldError(
+ 'wikiname',
+ 'The WikiName %s%s already belongs to you.' %
+ (wikiurl, wikiname))
+
+ def _save(self, wikiurl, wikiname):
+ """Given a wikiurl and wikiname, attempt to save it.
+
+ Verify someone else doesn't have it already.
+ """
+
+ @action(_("Save Changes"), name="save")
+ def save(self, action, data):
+ """Process the wiki names form."""
+ form = self.request.form
+ for obj in self.context.wiki_names:
+ if form.get('remove_%s' % obj.id):
+ obj.destroySelf()
+
+ if not self.errors:
+ wikiurl = self._sanitizeWikiURL(data.get('wiki'))
+ wikiname = data.get('wikiname')
+ # If either url or name are present then they both must be
+ # entered.
+ if wikiurl and wikiname:
+ wikinameset = getUtility(IWikiNameSet)
+ wikinameset.new(self.context, wikiurl, wikiname)
class PersonEditIRCNicknamesView(LaunchpadFormView):
@@ -3664,6 +3719,8 @@
# ids already exist, which allows the removal of ids without
# filling out the new jabberid field.
jabber_field = self.form_fields['jabberid']
+ # Copy the field so as not to modify the interface.
+ jabber_field.field = copy_field(jabber_field.field)
jabber_field.field.required = False
@property
=== modified file 'lib/lp/registry/stories/person/xx-person-edit-wikis.txt'
--- lib/lp/registry/stories/person/xx-person-edit-wikis.txt 2009-11-15 20:27:01 +0000
+++ lib/lp/registry/stories/person/xx-person-edit-wikis.txt 2010-08-31 13:20:59 +0000
@@ -1,109 +1,135 @@
-= Person's wikinames =
+==================
+Person's wikinames
+==================
A person can have any number of WikiNames registered in Launchpad, and
they can be managed on the +editwikinames page.
# A helper function for printing all wikinames of a person.
- >>> import re
- >>> def print_existing_wikinames(contents):
- ... soup = find_main_content(contents)
- ... wiki_url_inputs = soup.findAll(
- ... 'input', attrs={'name': re.compile(r'wiki_\d+')})
- ... wiki_urls = [input['value'] for input in wiki_url_inputs]
- ... wiki_name_inputs = soup.findAll(
- ... 'input', attrs={'name': re.compile(r'wikiname_\d+')})
- ... wiki_names = [input['value'] for input in wiki_name_inputs]
- ... wikis = zip(wiki_urls, wiki_names)
- ... print '\n'.join('%s%s' % (wiki_url, wiki_name)
- ... for wiki_url, wiki_name in wikis)
+ >>> def print_existing_wikiurls(contents):
+ ... trs = find_tags_by_class(contents, 'wikiurl')
+ ... wikis = []
+ ... for tr in trs:
+ ... td = tr.find('td')
+ ... wikis.append(td.contents[0])
+ ... print '\n'.join(wikis)
+
+ >>> def print_feedback(browser):
+ ... print "\n".join(get_feedback_messages(browser.contents))
Mark already has one WikiName registered.
>>> browser.addHeader('Authorization', 'Basic mark@xxxxxxxxxxx:test')
- >>> browser.open('http://localhost/~mark/+editwikinames')
- >>> print_existing_wikinames(browser.contents)
+ >>> browser.open('http://launchpad.dev/~mark/+editwikinames')
+ >>> print_existing_wikiurls(browser.contents)
https://wiki.ubuntu.com/MarkShuttleworth
But he wants to register another one.
- >>> browser.getControl(name='newwiki').value = 'http://foo.bar/wiki/'
- >>> browser.getControl(name='newwikiname').value = 'FooBar'
+ >>> browser.getControl(name='field.wiki').value = 'http://foo.bar/wiki/'
+ >>> browser.getControl(name='field.wikiname').value = 'FooBar'
>>> browser.getControl('Save Changes').click()
- >>> browser.url
- 'http://localhost/~mark/+editwikinames'
- >>> print_existing_wikinames(browser.contents)
+ >>> print_feedback(browser)
+ >>> print browser.url
+ http://launchpad.dev/~mark
+ >>> browser.open('http://launchpad.dev/~mark/+editwikinames')
+ >>> print_existing_wikiurls(browser.contents)
http://foo.bar/wiki/FooBar
https://wiki.ubuntu.com/MarkShuttleworth
He can't have two identical wiki names, though.
- >>> browser.getControl(name='newwiki').value = 'http://foo.bar/wiki/'
- >>> browser.getControl(name='newwikiname').value = 'FooBar'
+ >>> browser.getControl(name='field.wiki').value = 'http://foo.bar/wiki/'
+ >>> browser.getControl(name='field.wikiname').value = 'FooBar'
>>> browser.getControl('Save Changes').click()
- >>> browser.url
- 'http://localhost/~mark/+editwikinames'
+ >>> print browser.url
+ http://launchpad.dev/%7Emark/+editwikinames
>>> for message in find_tags_by_class(browser.contents, 'message'):
... print message.renderContents()
+ There is 1 error.
The WikiName http://foo.bar/wiki/FooBar already belongs to you.
Nor can he have a WikiName that is already registered in Launchpad.
- >>> browser.getControl(name='newwiki').value = 'https://wiki.ubuntu.com/'
- >>> browser.getControl(name='newwikiname').value = 'GuilhermeSalgado'
+ >>> browser.getControl(name='field.wiki').value = (
+ ... 'https://wiki.ubuntu.com/')
+ >>> browser.getControl(name='field.wikiname').value = 'GuilhermeSalgado'
>>> browser.getControl('Save Changes').click()
- >>> browser.url
- 'http://localhost/~mark/+editwikinames'
+ >>> print browser.url
+ http://launchpad.dev/%7Emark/+editwikinames
>>> print "\n".join(get_feedback_messages(browser.contents))
+ There is 1 error.
The WikiName https://wiki.ubuntu.com/GuilhermeSalgado is already
registered by Guilherme Salgado. If you think this is a duplicated
account, you can merge it into your account.
A WikiName's URL can't be empty nor invalid.
- >>> browser.getControl(name='newwiki').value = ''
- >>> browser.getControl(name='newwikiname').value = 'FooBar'
- >>> browser.getControl('Save Changes').click()
- >>> browser.url
- 'http://localhost/~mark/+editwikinames'
- >>> print "\n".join(get_feedback_messages(browser.contents))
- Neither Wiki nor WikiName can be empty.
-
- >>> browser.getControl(name='newwiki').value = '/this-is-not-a-url/'
- >>> browser.getControl(name='newwikiname').value = 'FooBar'
- >>> browser.getControl('Save Changes').click()
- >>> browser.url
- 'http://localhost/~mark/+editwikinames'
- >>> print "\n".join(get_feedback_messages(browser.contents))
- "/this-is-not-a-url/" is not a valid URL.
+ >>> browser.getControl(name='field.wiki').value = ''
+ >>> browser.getControl(name='field.wikiname').value = 'FooBar'
+ >>> browser.getControl('Save Changes').click()
+ >>> print browser.url
+ http://launchpad.dev/%7Emark/+editwikinames
+ >>> print "\n".join(get_feedback_messages(browser.contents))
+ There is 1 error.
+ The Wiki URL must be specified.
+
+ >>> browser.getControl(name='field.wiki').value = '/this-is-not-a-url/'
+ >>> browser.getControl(name='field.wikiname').value = 'FooBar'
+ >>> browser.getControl('Save Changes').click()
+ >>> print browser.url
+ http://launchpad.dev/%7Emark/+editwikinames
+ >>> print "\n".join(get_feedback_messages(browser.contents))
+ There is 1 error.
+ "/this-is-not-a-url/" is not a valid URI
+
+And it can't be incredibly long.
+
+ >>> browser.getControl(name='field.wiki').value = (
+ ... 'https://wiki.ubuntu.com/')
+ >>> wikiname = "z" * 101
+ >>> browser.getControl(name='field.wikiname').value = wikiname
+ >>> browser.getControl('Save Changes').click()
+ >>> print browser.url
+ http://launchpad.dev/%7Emark/+editwikinames
+ >>> print "\n".join(get_feedback_messages(browser.contents))
+ There is 1 error.
+ The wiki name cannot exceed 100 characters.
The invalid value is escaped using HTML entities when displayed back to
the user.
- >>> browser.getControl(name='newwiki').value = '<script>alert(1);</script>'
- >>> browser.getControl(name='newwikiname').value = 'FooBar'
+ >>> browser.getControl(name='field.wiki').value = (
+ ... '<script>alert(1);</script>')
+ >>> browser.getControl(name='field.wikiname').value = 'FooBar'
>>> browser.getControl('Save Changes').click()
- >>> browser.url
- 'http://localhost/~mark/+editwikinames'
+ >>> print browser.url
+ http://launchpad.dev/%7Emark/+editwikinames
>>> print "\n".join(get_feedback_messages(browser.contents))
- "<script>alert(1);</script>/" is not a valid URL.
+ There is 1 error.
+ "<script>alert(1);</script>" is not a valid URI
Only http and https URLs are allowed for wikis.
- >>> browser.getControl(name='newwiki').value = "javascript:void"
- >>> browser.getControl(name='newwikiname').value = 'FooBar'
+ >>> browser.getControl(name='field.wiki').value = "javascript:void"
+ >>> browser.getControl(name='field.wikiname').value = 'FooBar'
>>> browser.getControl('Save Changes').click()
- >>> browser.url
- 'http://localhost/~mark/+editwikinames'
+ >>> print browser.url
+ http://launchpad.dev/%7Emark/+editwikinames
>>> print "\n".join(get_feedback_messages(browser.contents))
- The URL scheme "javascript" is not allowed. Only http or https URLs may be
- used.
-
-Mark can remove any of his wiki names, and here he'll remove the one he
-just added.
-
+ There is 1 error.
+ The URI scheme "javascript" is not allowed. Only URIs with the following
+ schemes may be used: http, https
+
+Mark can remove any of his wiki names.
+
+ >>> browser.getControl(name='field.wiki').value = ""
+ >>> browser.getControl(name='field.wikiname').value = ''
>>> browser.getControl('Remove', index=0).selected = True
>>> browser.getControl('Save Changes').click()
- >>> browser.url
- 'http://localhost/~mark/+editwikinames'
- >>> print_existing_wikinames(browser.contents)
+ >>> print browser.url
+ http://launchpad.dev/~mark
+ >>> print "\n".join(get_feedback_messages(browser.contents))
+ >>> browser.open('http://launchpad.dev/~mark/+editwikinames')
+ >>> print_existing_wikiurls(browser.contents)
https://wiki.ubuntu.com/MarkShuttleworth
=== modified file 'lib/lp/registry/templates/person-editwikinames.pt'
--- lib/lp/registry/templates/person-editwikinames.pt 2009-09-17 15:00:38 +0000
+++ lib/lp/registry/templates/person-editwikinames.pt 2010-08-31 13:20:59 +0000
@@ -6,14 +6,12 @@
metal:use-macro="view/macro:page/main_only"
i18n:domain="launchpad"
>
- <body>
-
- <div metal:fill-slot="main">
- <p tal:condition="view/error_message"
- tal:content="structure view/error_message/escapedtext"
- class="error message" />
-
- <form name="edit_wikiname" action="" method="POST">
+<body>
+
+<div metal:fill-slot="main">
+<div metal:use-macro="context/@@launchpad_form/form">
+
+ <div metal:fill-slot="widgets">
<table id="wikinames">
<tal:XXX condition="nothing">
@@ -22,27 +20,19 @@
# there's a bug preventing us from # updating our version of storm
# to trunk.
</tal:XXX>
- <tal:block condition="context/wiki_names/any">
-
- <tr><td colspan="2"><h2>Wiki names</h2></td></tr>
-
- <tr>
- <td><label>Wiki base URL:</label></td>
- <td><label>Wiki name:</label></td>
- </tr>
-
- <tr tal:repeat="wiki context/wiki_names">
- <td>
- <input type="text" style="margin-bottom: 0.5em;"
- tal:attributes="name string:wiki_${wiki/id};
- value wiki/wiki" />
- </td>
- <td>
- <input type="text" size="15"
- tal:attributes="name string:wikiname_${wiki/id};
- value wiki/wikiname" />
- </td>
-
+
+ <tal:existing_wiki condition="context/wiki_names/any">
+
+ <tr>
+ <td><label>Existing wiki names</label></td>
+ </tr>
+
+ <tr>
+ <td><label>Wiki URL</label></td>
+ </tr>
+
+ <tr tal:repeat="wiki context/wiki_names" class="wikiurl">
+ <td tal:content="wiki/url"></td>
<td>
<label>
<input type="checkbox" value="Remove"
@@ -51,41 +41,35 @@
</label>
</td>
</tr>
-
- </tal:block>
-
- <tr>
- <td colspan="2"><h2>New wiki name</h2></td>
- </tr>
-
- <tr>
- <td><label>Wiki Base URL:</label></td>
- <td><label>Wiki Name:</label></td>
- </tr>
-
- <tr>
- <td>
- <input name="newwiki" type="text"
- tal:attributes="value view/newwiki|nothing" />
- </td>
- <td>
- <input name="newwikiname" type="text" size="15"
- tal:attributes="value view/newwikiname|nothing" />
- </td>
- </tr>
-
- <tr>
- <td class="formHelp">Example: https://wiki.ubuntu.com/</td>
- <td class="formHelp">Example: YourName.</td>
- </tr>
- </table>
-
- <div class="actions">
- <input type="submit" value="Save Changes" name="SAVE" />
- or <a tal:attributes="href view/cancel_url">Cancel</a>
- </div>
- </form>
- </div>
+ <tr style="height:2em;"><td></td></tr>
+ </tal:existing_wiki>
+
+
+ <tr>
+ <td><label>New wiki name</label></td>
+ </tr>
+
+ <tal:widget define="widget nocall:view/widgets/wiki">
+ <metal:block use-macro="context/@@launchpad_form/widget_row" />
+ </tal:widget>
+
+ <tr>
+ <td class="formHelp">Example: https://wiki.ubuntu.com/</td>
+ </tr>
+
+ <tal:widget define="widget nocall:view/widgets/wikiname">
+ <metal:block use-macro="context/@@launchpad_form/widget_row" />
+ </tal:widget>
+
+ <tr>
+ <td class="formHelp">Example: YourName.</td>
+ </tr>
+
+ </table>
+ </div>
+
+</div>
+</div>
</body>
</html>
