Review: Needs Fixing
Diff comments:
> === modified file 'lib/lp/app/validators/name.py'
> --- lib/lp/app/validators/name.py 2012-11-29 05:52:36 +0000
> +++ lib/lp/app/validators/name.py 2019-05-07 00:19:42 +0000
> @@ -19,6 +19,7 @@
> valid_name_pattern = re.compile(r"^[a-z0-9][a-z0-9\+\.\-]+$")
> valid_bug_name_pattern = re.compile(r"^[a-z][a-z0-9\+\.\-]+$")
> invalid_name_pattern = re.compile(r"^[^a-z0-9]+|[^a-z0-9\\+\\.\\-]+")
> +valid_username_pattern =
> re.compile(r"(?!^[\d-]+$)^[a-z\d](-?[a-z\d]){2,31}$")
You're going to need an equivalent of sanitize_name too, and to use it in
lib/lp/registry/model/person.py:generate_nick. For the latter, please
particularly check that it copes with cases that are now more difficult, such
as an email address with a local part that's right up against the length limit
and that ends with either "-" or a character that's replaced with "-".
>
>
> def sanitize_name(name):
> @@ -97,3 +145,18 @@
>
> raise LaunchpadValidationError(structured(message))
> return True
> +
> +
> +def username_validator(name):
> + """Return True if the `Person.name` is valid, or raise a
> + LaunchpadValidationError.
> + """
> + if not valid_username(name):
> + message = _(dedent("""
> + Invalid username '${name}'. Usernames must be at least two
> characters long
> + and start with a letter or number. All letters must be
> lower-case.
This doesn't seem quite accurate. "Usernames must be at least three characters
long, and must both start and end with a letter or number", perhaps?
> + The character <samp>-</samp> is allowed after the first
> character."""),
Maybe something about the non-consecutive requirement?
> + mapping={'name': html_escape(name)})
> +
> + raise LaunchpadValidationError(structured(message))
> + return True
>
> === modified file 'lib/lp/bugs/model/bugtracker.py'
> --- lib/lp/bugs/model/bugtracker.py 2019-02-23 08:15:45 +0000
> +++ lib/lp/bugs/model/bugtracker.py 2019-05-07 00:19:42 +0000
> @@ -645,9 +645,18 @@
> if bugtracker_person is not None:
> return bugtracker_person.person
>
> - # Generate a valid Launchpad name for the Person.
> - base_canonical_name = (
> - "%s-%s" % (sanitize_name(display_name.lower()), self.name))
> + # Generate a valid Launchpad name for the Person extracting an
> + # username from the given display_name smaller enough to be joined
> + # with the tracker name and up to 2 digits (99) for disambiguation
> + # and fit in 32 characters limit and avoid consecutive hyphens.
> + remaining = 32 - len(self.name) - 2
> + username = sanitize_name(display_name.lower())[:remaining]
> + username = username if not username.endswith('-') else username[:-1]
> +
> + # XXX cprov 2019-05-06: bugtracker names may be longer than 32-chars,
> + # we need extra name-cropping if this simple strategy gets accepted.
I think we should probably just require bugtracker names only to comply with
the older rules instead, at least for now. We certainly shouldn't limit their
length as strictly, IMO.
For internal use such as this, it may be necessary to have a flag when creating
a person that allows using the name validator rather than the username
validator (or it could maybe just select validation rules based on the person
creation rationale, if that turns out to be good enough).
> +
> + base_canonical_name = "%s-%s" % (username, self.name)
> canonical_name = base_canonical_name
>
> person_set = getUtility(IPersonSet)
>
> === modified file 'lib/lp/registry/interfaces/person.py'
> --- lib/lp/registry/interfaces/person.py 2018-08-02 16:12:57 +0000
> +++ lib/lp/registry/interfaces/person.py 2019-05-07 00:19:42 +0000
> @@ -662,11 +665,11 @@
> name = exported(
> PersonNameField(
> title=_('Name'), required=True, readonly=False,
> - constraint=name_validator,
> + constraint=username_validator,
> description=_(
> "A short unique name, beginning with a lower-case "
s/beginning/beginning and ending/
> "letter or number, and containing only letters, "
> - "numbers, dots, hyphens, or plus signs.")))
> + "numbers and non-consecutive hyphens.")))
> display_name = exported(
> StrippedTextLine(
> title=_('Display Name'), required=True, readonly=False,
>
> === modified file 'lib/lp/registry/tests/test_nickname.py'
> --- lib/lp/registry/tests/test_nickname.py 2015-10-26 14:54:43 +0000
> +++ lib/lp/registry/tests/test_nickname.py 2019-05-07 00:19:42 +0000
> @@ -36,14 +36,14 @@
> # valid nick that doesn't start with symbols.
> parts = ['---bar', 'foo.bar', 'foo-bar', 'foo+bar']
> nicks = [generate_nick("%[email protected]" % part) for part in parts]
> - self.assertEqual(['bar', 'foo-bar', 'foo-bar', 'foo+bar'], nicks)
> + self.assertEqual(['bar', 'foo-bar', 'foo-bar', 'foo-bar'], nicks)
>
> def test_enforces_minimum_length(self):
> # Nicks must be a minimum length. generate_nick enforces this by
> # adding random suffixes to the required length.
> - self.assertIs(None, getUtility(IPersonSet).getByName('i'))
> - nick = generate_nick('[email protected]')
> - self.assertEqual('i-b', nick)
> + self.assertIs(None, getUtility(IPersonSet).getByName('hi'))
> + nick = generate_nick('[email protected]')
> + self.assertEqual('hi-5', nick)
As indicated above, I'd like to see at least a few more tests here for corner
cases of the new rules.
>
> def test_can_create_noncolliding_nicknames(self):
> # Given the same email address, generate_nick doesn't recreate the
>
> === modified file
> 'lib/lp/translations/stories/standalone/xx-person-activity.txt'
> --- lib/lp/translations/stories/standalone/xx-person-activity.txt
> 2018-06-02 22:39:54 +0000
> +++ lib/lp/translations/stories/standalone/xx-person-activity.txt
> 2019-05-07 00:19:42 +0000
> @@ -51,13 +51,18 @@
> URL-escaped user names
> ----------------------
>
> -Since the user's name is included in the URL, and user names can contain
> -some slightly weird characters, it is escaped especially for this usage.
> +Since the user's name is included in the URL, and legacy user names can
> +contain some slightly weird characters, it is escaped especially for this
> +usage.
>
> -For instance, here's a user called a+b.
> +For instance, here's a legacy user called "a+b".
>
> >>> login('[email protected]')
> - >>> ab = factory.makePerson(name='a+b')
> + >>> ab = factory.makePerson()
> + >>> from zope.security.proxy import removeSecurityProxy
> + >>> naked_person = removeSecurityProxy(ab)
> + >>> naked_person.name = 'a+b'
> + >>> naked_person.display_name = 'A+b'
If you follow my suggestion above to have a flag or a creation-rationale-based
indication for which validation rules to use when creating a person, then it
ought to be possible to refactor this test to avoid having to remove the
security proxy.
> >>> sr_pofile = factory.makePOFile('sr')
> >>> message = factory.makeCurrentTranslationMessage(
> ... pofile=sr_pofile, translator=ab)
--
https://code.launchpad.net/~cprov/launchpad/strict-usernames/+merge/366985
Your team Launchpad code reviewers is subscribed to branch lp:launchpad.
_______________________________________________
Mailing list: https://launchpad.net/~launchpad-reviewers
Post to : [email protected]
Unsubscribe : https://launchpad.net/~launchpad-reviewers
More help : https://help.launchpad.net/ListHelp
