launchpad-reviewers team mailing list archive
-
launchpad-reviewers team
-
Mailing list archive
-
Message #02807
[Merge] lp:~thumper/launchpad/fix-mantis-warnings-bug-218384 into lp:launchpad
Tim Penhey has proposed merging lp:~thumper/launchpad/fix-mantis-warnings-bug-218384 into lp:launchpad.
Requested reviews:
Launchpad code reviewers (launchpad-reviewers)
Related bugs:
#214982 HTTPErrror raised in Mantis ExternalBugTracker
https://bugs.launchpad.net/bugs/214982
#218384 OOPS processing Mantis bugwatches
https://bugs.launchpad.net/bugs/218384
For more details, see:
https://code.launchpad.net/~thumper/launchpad/fix-mantis-warnings-bug-218384/+merge/51679
This branch fixes two old Mantis issues.
HTTPError when trying to determine if the remote site can
handle batches, and the oops handled when trying to use
a non-existant warning method.
The HTTPError is handled by wrapping the urlopen method with
the _fetchPage method, which translates the urllib2 errors to
a BugTrackerConnectionError.
Fixing the warning usage was trivial, but I refactored the
code somewhat to make testing easier. A class was created
that is responsible for parsing the CVS data.
Also an unused method (_getStatusFromCSV) was deleted.
--
https://code.launchpad.net/~thumper/launchpad/fix-mantis-warnings-bug-218384/+merge/51679
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~thumper/launchpad/fix-mantis-warnings-bug-218384 into lp:launchpad.
=== modified file 'lib/lp/bugs/externalbugtracker/base.py'
--- lib/lp/bugs/externalbugtracker/base.py 2011-02-23 06:42:12 +0000
+++ lib/lp/bugs/externalbugtracker/base.py 2011-03-01 02:35:00 +0000
@@ -238,13 +238,13 @@
"""
return None
- def _fetchPage(self, page):
+ def _fetchPage(self, page, data=None):
"""Fetch a page from the remote server.
A BugTrackerConnectError will be raised if anything goes wrong.
"""
try:
- return self.urlopen(page)
+ return self.urlopen(page, data)
except (urllib2.HTTPError, urllib2.URLError), val:
raise BugTrackerConnectError(self.baseurl, val)
@@ -260,7 +260,7 @@
def _post(self, url, data):
"""Post to a given URL."""
request = urllib2.Request(url, headers={'User-agent': LP_USER_AGENT})
- return self.urlopen(request, data=data)
+ return self._fetchPage(request, data=data)
def _postPage(self, page, form, repost_on_redirect=False):
"""POST to the specified page and form.
=== modified file 'lib/lp/bugs/externalbugtracker/mantis.py'
--- lib/lp/bugs/externalbugtracker/mantis.py 2011-01-23 09:34:35 +0000
+++ lib/lp/bugs/externalbugtracker/mantis.py 2011-03-01 02:35:00 +0000
@@ -8,6 +8,7 @@
import cgi
import csv
+import logging
import urllib
import urllib2
from urlparse import urlunparse
@@ -98,6 +99,70 @@
self, request, fp, code, msg, hdrs, self.rewrite_url(new_url))
+class MantisBugBatchParser:
+ """A class that parses the batch of bug data.
+
+ Using the CSV reader is pretty much essential since the data that comes
+ back can include title text which can in turn contain field separators.
+ You don't want to handle the unquoting yourself.
+ """
+
+ def __init__(self, csv_data, logger):
+ # Clean out stray, unquoted newlines inside csv_data to avoid the CSV
+ # module blowing up. IDEA: perhaps if the size of csv_data is large
+ # in the future, this could be moved into a generator.
+ csv_data = [s.replace("\r", "") for s in csv_data]
+ csv_data = [s.replace("\n", "") for s in csv_data]
+ self.reader = csv.reader(csv_data)
+ self.logger = logger
+
+ def processCSVBugLine(self, bug_line, headers):
+ """Processes a single line of the CSV."""
+ bug = {}
+ for index, header in enumerate(headers):
+ try:
+ data = bug_line[index]
+ except IndexError:
+ self.logger.warning("Line %r incomplete." % bug_line)
+ return None
+ bug[header] = data
+ try:
+ bug['id'] = int(bug['id'])
+ except ValueError:
+ self.logger.warning("Encountered invalid bug ID: %r." % bug['id'])
+ return None
+ return bug
+
+ def parseHeaderLine(self, reader):
+ # The first line of the CSV file is the header. We need to read
+ # it because different Mantis instances have different header
+ # ordering and even different columns in the export.
+ try:
+ headers = [h.lower() for h in reader.next()]
+ except StopIteration:
+ raise UnparsableBugData("Missing header line")
+ missing_headers = [
+ name for name in ('id', 'status', 'resolution')
+ if name not in headers]
+ if missing_headers:
+ raise UnparsableBugData(
+ "CSV header %r missing fields: %r" % (
+ headers, missing_headers))
+ return headers
+
+ def getBugs(self):
+ headers = self.parseHeaderLine(self.reader)
+ bugs = {}
+ try:
+ for bug_line in self.reader:
+ bug = self.processCSVBugLine(bug_line, headers)
+ if bug is not None:
+ bugs[bug['id']] = bug
+ return bugs
+ except csv.Error, error:
+ raise UnparsableBugData("Exception parsing CSV file: %s." % error)
+
+
class Mantis(ExternalBugTracker):
"""An `ExternalBugTracker` for dealing with Mantis instances.
@@ -114,6 +179,7 @@
self._cookie_handler = urllib2.HTTPCookieProcessor()
self._opener = urllib2.build_opener(
self._cookie_handler, MantisLoginHandler())
+ self._logger = logging.getLogger()
@ensure_no_transaction
def urlopen(self, request, data=None):
@@ -178,7 +244,10 @@
'search': '',
'filter': 'Apply Filter',
}
- self.page = self._postPage("view_all_set.php?f=3", data)
+ try:
+ self._postPage("view_all_set.php?f=3", data)
+ except BugTrackerConnectError:
+ return None
# Finally grab the full CSV export, which uses the
# MANTIS_VIEW_ALL_COOKIE set in the previous step to specify
@@ -275,65 +344,8 @@
if not csv_data:
raise UnparsableBugData("Empty CSV for %s" % self.baseurl)
- # Clean out stray, unquoted newlines inside csv_data to avoid
- # the CSV module blowing up.
- csv_data = [s.replace("\r", "") for s in csv_data]
- csv_data = [s.replace("\n", "") for s in csv_data]
-
- # The first line of the CSV file is the header. We need to read
- # it because different Mantis instances have different header
- # ordering and even different columns in the export.
- self.headers = [h.lower() for h in csv_data.pop(0).split(",")]
- if len(self.headers) < 2:
- raise UnparsableBugData("CSV header mangled: %r" % self.headers)
-
- if not csv_data:
- # A file with a header and no bugs is also useless.
- raise UnparsableBugData("CSV for %s contained no bugs!"
- % self.baseurl)
-
- try:
- bugs = {}
- # Using the CSV reader is pretty much essential since the
- # data that comes back can include title text which can in
- # turn contain field separators -- you don't want to handle
- # the unquoting yourself.
- for bug_line in csv.reader(csv_data):
- bug = self._processCSVBugLine(bug_line)
- bugs[int(bug['id'])] = bug
-
- return bugs
-
- except csv.Error, error:
- raise UnparsableBugData("Exception parsing CSV file: %s." % error)
-
- def _processCSVBugLine(self, bug_line):
- """Processes a single line of the CSV.
-
- Adds the bug it represents to self.bugs.
- """
- required_fields = ['id', 'status', 'resolution']
- bug = {}
- for header in self.headers:
- try:
- data = bug_line.pop(0)
- except IndexError:
- self.warning("Line '%r' incomplete." % bug_line)
- return
- bug[header] = data
- for field in required_fields:
- if field not in bug:
- self.warning("Bug %s lacked field '%r'." % (bug['id'], field))
- return
- try:
- # See __init__ for an explanation of why we use integer
- # IDs in the internal data structure.
- bug_id = int(bug['id'])
- except ValueError:
- self.warning("Encountered invalid bug ID: %r." % bug['id'])
- return
-
- return bug
+ parser = MantisBugBatchParser(csv_data, self._logger)
+ return parser.getBugs()
def _checkForApplicationError(self, page_soup):
"""If Mantis does not find the bug it still returns a 200 OK
@@ -467,14 +479,6 @@
# it makes display of the data nicer.
return "%(status)s: %(resolution)s" % bug
- def _getStatusFromCSV(self, bug_id):
- try:
- bug = self.bugs[int(bug_id)]
- except KeyError:
- raise BugNotFound(bug_id)
- else:
- return bug['status'], bug['resolution']
-
def convertRemoteImportance(self, remote_importance):
"""See `ExternalBugTracker`.
=== modified file 'lib/lp/bugs/externalbugtracker/tests/test_externalbugtracker.py'
--- lib/lp/bugs/externalbugtracker/tests/test_externalbugtracker.py 2011-01-18 20:46:11 +0000
+++ lib/lp/bugs/externalbugtracker/tests/test_externalbugtracker.py 2011-03-01 02:35:00 +0000
@@ -6,17 +6,25 @@
__metaclass__ = type
from StringIO import StringIO
+import urllib2
from zope.interface import implements
-from lp.bugs.externalbugtracker.base import ExternalBugTracker
+from canonical.testing.layers import ZopelessLayer
+from lp.bugs.externalbugtracker.base import (
+ BugTrackerConnectError,
+ ExternalBugTracker,
+ )
from lp.bugs.externalbugtracker.debbugs import DebBugs
from lp.bugs.interfaces.externalbugtracker import (
ISupportsBackLinking,
ISupportsCommentImport,
ISupportsCommentPushing,
)
-from lp.testing import TestCase
+from lp.testing import (
+ monkey_patch,
+ TestCase,
+ )
from lp.testing.fakemethod import FakeMethod
@@ -144,3 +152,21 @@
self.assertEqual(2, bugtracker._post.call_count)
last_args, last_kwargs = bugtracker._post.calls[-1]
self.assertEqual((fake_form.url, ), last_args)
+
+
+class TestExternalBugTracker(TestCase):
+ """Tests for various methods of the ExternalBugTracker."""
+
+ layer = ZopelessLayer
+
+ def test_post_raises_on_404(self):
+ # When posting, a 404 is converted to a BugTrackerConnectError.
+ base_url = "http://example.com/";
+ bugtracker = ExternalBugTracker(base_url)
+ def raise404(request, data):
+ raise urllib2.HTTPError('url', 404, 'Not Found', None, None)
+ with monkey_patch(urllib2, urlopen=raise404):
+ self.assertRaises(
+ BugTrackerConnectError,
+ bugtracker._post,
+ 'some-url', {'post-data': 'here'})
=== added file 'lib/lp/bugs/externalbugtracker/tests/test_mantis.py'
--- lib/lp/bugs/externalbugtracker/tests/test_mantis.py 1970-01-01 00:00:00 +0000
+++ lib/lp/bugs/externalbugtracker/tests/test_mantis.py 2011-03-01 02:35:00 +0000
@@ -0,0 +1,110 @@
+# Copyright 2011 Canonical Ltd. This software is licensed under the
+# GNU Affero General Public License version 3 (see the file LICENSE).
+
+"""Tests for the Mantis BugTracker."""
+
+__metaclass__ = type
+
+from testtools.matchers import Equals, Is
+import urllib2
+
+from canonical.testing.layers import ZopelessLayer
+from lp.bugs.externalbugtracker import UnparsableBugData
+from lp.bugs.externalbugtracker.mantis import (
+ Mantis,
+ MantisBugBatchParser,
+ )
+from lp.testing import (
+ monkey_patch,
+ TestCase,
+ )
+from lp.services.log.logger import BufferLogger
+
+
+class TestMantisBugBatchParser(TestCase):
+ """Test the MantisBugBatchParser class."""
+
+ def setUp(self):
+ super(TestMantisBugBatchParser, self).setUp()
+ self.logger = BufferLogger()
+
+ def test_empty(self):
+ data = []
+ parser = MantisBugBatchParser(data, self.logger)
+ exc = self.assertRaises(
+ UnparsableBugData,
+ parser.getBugs)
+ self.assertThat(
+ str(exc), Equals("Missing header line"))
+
+ def test_missing_headers(self):
+ data = ['some,headers']
+ parser = MantisBugBatchParser(data, self.logger)
+ exc = self.assertRaises(
+ UnparsableBugData,
+ parser.getBugs)
+ self.assertThat(
+ str(exc),
+ Equals("CSV header ['some', 'headers'] missing fields:"
+ " ['id', 'status', 'resolution']"))
+
+ def test_missing_some_headers(self):
+ data = ['some,headers,status,resolution']
+ parser = MantisBugBatchParser(data, self.logger)
+ exc = self.assertRaises(
+ UnparsableBugData,
+ parser.getBugs)
+ self.assertThat(
+ str(exc),
+ Equals("CSV header ['some', 'headers', 'status', 'resolution'] "
+ "missing fields: ['id']"))
+
+ def test_no_bugs(self):
+ data = ['other,fields,id,status,resolution']
+ parser = MantisBugBatchParser(data, self.logger)
+ self.assertThat(parser.getBugs(), Equals({}))
+
+ def test_passing(self):
+ data = ['ignored,id,resolution,status',
+ 'foo,42,not,complete',
+ 'boo,13,,confirmed']
+ parser = MantisBugBatchParser(data, self.logger)
+ bug_42 = dict(
+ id=42, status='complete', resolution='not', ignored='foo')
+ bug_13 = dict(
+ id=13, status='confirmed', resolution='', ignored='boo')
+ self.assertThat(parser.getBugs(), Equals({42: bug_42, 13: bug_13}))
+
+ def test_incomplete_line(self):
+ data = ['ignored,id,resolution,status',
+ '42,not,complete']
+ parser = MantisBugBatchParser(data, self.logger)
+ self.assertThat(parser.getBugs(), Equals({}))
+ log = self.logger.getLogBuffer()
+ self.assertThat(
+ log, Equals("WARNING Line ['42', 'not', 'complete'] incomplete.\n"))
+
+ def test_non_integer_id(self):
+ data = ['ignored,id,resolution,status',
+ 'foo,bar,not,complete']
+ parser = MantisBugBatchParser(data, self.logger)
+ self.assertThat(parser.getBugs(), Equals({}))
+ log = self.logger.getLogBuffer()
+ self.assertThat(
+ log, Equals("WARNING Encountered invalid bug ID: 'bar'.\n"))
+
+
+class TestMantisBugTracker(TestCase):
+ """Tests for various methods of the Manits bug tracker."""
+
+ layer = ZopelessLayer
+
+ def test_csv_data_on_post_404(self):
+ # If the 'view_all_set.php' request raises a 404, then the csv_data
+ # attribute is None.
+ base_url = "http://example.com/";
+ def raise404(self, request, data):
+ raise urllib2.HTTPError('url', 404, 'Not Found', None, None)
+ with monkey_patch(Mantis, urlopen=raise404):
+ bugtracker = Mantis(base_url)
+ self.assertThat(bugtracker.csv_data, Is(None))
