launchpad-reviewers team mailing list archive

[William Grant <me@xxxxxxxxxxxxxxxxxx>]

Review: Needs Fixing



Diff comments:

> === modified file 'lib/lp/bugs/browser/buglisting.py'
> --- lib/lp/bugs/browser/buglisting.py	2014-11-29 02:16:28 +0000
> +++ lib/lp/bugs/browser/buglisting.py	2015-01-08 03:46:20 +0000
> @@ -1229,13 +1229,21 @@
>  
>          if data:
>              searchtext = data.get("searchtext")
> -            if searchtext and searchtext.isdigit():
> -                try:
> -                    bug = getUtility(IBugSet).get(searchtext)
> -                except NotFoundError:
> -                    pass
> -                else:
> -                    self.request.response.redirect(canonical_url(bug))
> +            if searchtext:
> +                bug_number = ""
> +                if searchtext.isdigit():
> +                    bug_number = searchtext
> +                if len(searchtext.split(' ')) == 1 and searchtext[0] == '#' and \
> +                searchtext[1:].isdigit():
> +                    bug_number = searchtext[1:]

That's slightly awkward code, and we don't use backslashes for long lines, preferring parentheses.

Consider something like this:

if searchtext:
    maybe_number = searchtext
    if maybe_number.startswith('#'):
        maybe_number = maybe_number[1:]
    if maybe_number.isdigit():
        blah blah blah

> +
> +                if bug_number:
> +                    try:
> +                        bug = getUtility(IBugSet).get(bug_number)
> +                    except NotFoundError:
> +                        pass
> +                    else:
> +                        self.request.response.redirect(canonical_url(bug))
>  
>              assignee_option = self.request.form.get("assignee_option")
>              if assignee_option == "none":
> 
> === modified file 'lib/lp/bugs/browser/tests/test_buglisting.py'
> --- lib/lp/bugs/browser/tests/test_buglisting.py	2015-01-07 03:07:22 +0000
> +++ lib/lp/bugs/browser/tests/test_buglisting.py	2015-01-08 03:46:20 +0000
> @@ -249,6 +249,30 @@
>  
>          self.assertEqual(default_bugtask_url, browser.url)
>  
> +    def test_redirects_to_bug_from_search_form_with_hash(self):
> +        bug = self.factory.makeBug()
> +        login_person(bug.owner)
> +        default_bugtask_url = canonical_url(bug.default_bugtask, rootsite='bugs')
> +
> +        browser = self.getUserBrowser("http://bugs.launchpad.dev/";)
> +        input_field = browser.getControl(name='field.searchtext')
> +        input_field.value = "#%d" % bug.id
> +        browser.getControl(name='search').click()
> +
> +        self.assertEqual(default_bugtask_url, browser.url)
> +
> +    def test_doesnt_redirect_to_bug_from_search_form_with_multiple_terms(self):
> +        bug = self.factory.makeBug()
> +        login_person(bug.owner)
> +        default_bugtask_url = canonical_url(bug.default_bugtask, rootsite='bugs')
> +
> +        browser = self.getUserBrowser("http://bugs.launchpad.dev/";)
> +        input_field = browser.getControl(name='field.searchtext')
> +        input_field.value = "#%d otherterm" % bug.id
> +        browser.getControl(name='search').click()
> +
> +        self.assertNotEqual(default_bugtask_url, browser.url)

That's three tests that are almost precisely identical. Can you factor something out to reduce duplicated code?

> +
>  
>  class BugTargetTestCase(TestCaseWithFactory):
>      """Test helpers for setting up `IBugTarget` tests."""
> 


-- 
https://code.launchpad.net/~thomir/launchpad/devel-fix-bug-redirect/+merge/245816
Your team Launchpad code reviewers is subscribed to branch lp:launchpad.