launchpad-reviewers team mailing list archive

[Robert Collins <robertc@xxxxxxxxxxxxxxxxx>]

Robert Collins has proposed merging lp:~lifeless/python-oops-tools/amqp-polish into lp:python-oops-tools.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)
Related bugs:
  Bug #884571 in python-oops-tools: "amqp2disk oopses with lower case hashes are not accessible via their suffix"
  https://bugs.launchpad.net/python-oops-tools/+bug/884571

For more details, see:
https://code.launchpad.net/~lifeless/python-oops-tools/amqp-polish/+merge/81083

The code path that the linked bug thought was broken isn't. This changes the test environment a little to cover the case we exercise (which already worked).
-- 
https://code.launchpad.net/~lifeless/python-oops-tools/amqp-polish/+merge/81083
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~lifeless/python-oops-tools/amqp-polish into lp:python-oops-tools.

=== modified file 'src/oopstools/NEWS.txt'
--- src/oopstools/NEWS.txt	2011-11-02 20:25:00 +0000
+++ src/oopstools/NEWS.txt	2011-11-02 23:29:23 +0000
@@ -8,9 +8,8 @@
 * Flush stdout when logging an OOPS receipt in amqp2disk.
   (Robert Collins, #884569)
 
-* Force received OOPS ids to uppercase in amqp2disk to fit in with the UI
-  - otherwise they cannot be accessed.
-  (Robert Collins, #884571)
+* Mixed case OOPS reports can now be looked up in the web UI without their
+  OOPS-prefix (if they had one). (Robert Collins, #884571)
 
 * OOPS reports that don't meet the normal rules for req_vars are handled
   a bit better (Robert Collins, William Grant, #885416)

=== modified file 'src/oopstools/oops/test/bug-672984.txt'
--- src/oopstools/oops/test/bug-672984.txt	2011-10-13 20:18:51 +0000
+++ src/oopstools/oops/test/bug-672984.txt	2011-11-02 23:29:23 +0000
@@ -19,7 +19,7 @@
     >>> pprint(
     ...     [(oops, oops.http_method) for oops in
     ...      sorted(found_oopses, key=lambda oops: oops.date)])
-    [(<Oops: OOPS-1308X1>, u'1234567890')] 
+    [(<Oops: OOPS-1308x1>, u'1234567890')] 
 
 Reset the database.
 

=== modified file 'src/oopstools/oops/test/db-summary.txt'
--- src/oopstools/oops/test/db-summary.txt	2011-10-24 05:21:01 +0000
+++ src/oopstools/oops/test/db-summary.txt	2011-11-02 23:29:23 +0000
@@ -45,10 +45,10 @@
     >>> print get_section_contents("Top 10 Durations", webapp_summary)
     26.46s  OOPS-689S4      FooBar:+questions
     20.67s  OOPS-689S6      FooBar:+bug-text
-    0.28s  OOPS-1308X1      Unknown
+    0.28s  OOPS-1308x1      Unknown
 
     >>> print get_section_contents("Top 10 Statement Counts", webapp_summary)
-    14  OOPS-1308X1     Unknown
+    14  OOPS-1308x1     Unknown
     4 OOPS-689S6        FooBar:+bug-text
     3  OOPS-689S4       FooBar:+questions
 

=== modified file 'src/oopstools/oops/test/files/oops-sample/dir2/2009-07-31/72358.X1'
--- src/oopstools/oops/test/files/oops-sample/dir2/2009-07-31/72358.X1	2011-10-13 20:18:51 +0000
+++ src/oopstools/oops/test/files/oops-sample/dir2/2009-07-31/72358.X1	2011-11-02 23:29:23 +0000
@@ -1,4 +1,4 @@
-Oops-Id: OOPS-1308X1
+Oops-Id: OOPS-1308x1
 Exception-Type: NotFound
 Exception-Value: Object: <canonical.launchpad.webapp.publisher.RootObject object at 0x40b83fec>, name: u'foobar'
 Date: 2009-07-31T20:05:58.274049+00:00

=== modified file 'src/oopstools/oops/test/oops.txt'
--- src/oopstools/oops/test/oops.txt	2011-10-13 20:18:51 +0000
+++ src/oopstools/oops/test/oops.txt	2011-11-02 23:29:23 +0000
@@ -262,7 +262,7 @@
 Information about which Storm store is being used by the application is
 recorded together with the SQL statement.
 
-    >>> oops_with_db_id = Oops.objects.get(oopsid__exact="OOPS-1308X1")
+    >>> oops_with_db_id = Oops.objects.get(oopsid__exact="OOPS-1308x1")
     >>> for start, stop, db_id, statement in oops_with_db_id.statements:
     ...     print start, stop, db_id, statement
     4 5 session UPDATE SessionData SET last_accessed ...

=== modified file 'src/oopstools/oops/test/pagetest.txt'
--- src/oopstools/oops/test/pagetest.txt	2011-10-13 20:18:51 +0000
+++ src/oopstools/oops/test/pagetest.txt	2011-11-02 23:29:23 +0000
@@ -25,15 +25,15 @@
 
 Using a valid OOPS id the OOPS page is correctly rendered.
 
-    >>> b.getControl(name="oopsid").value = "OOPS-689S4"
+    >>> b.getControl(name="oopsid").value = "OOPS-1308X1"
     >>> b.getControl("Search OOPS").click()
 
     >>> b.url
-    'http://localhost/oops/?oopsid=OOPS-689S4'
+    'http://localhost/oops/?oopsid=OOPS-1308X1'
 
     >>> print b.contents
     <...
-    ...<title>OOPS-689S4</title>...
+    ...<title>OOPS-1308x1</title>...
     ...
     ...<div id="request_variables">...
     ...
@@ -43,30 +43,55 @@
 It also accepts the OOPS id without the "OOPS-" part.
 
     >>> b.open('http://localhost/oops/')
-    >>> b.getControl(name="oopsid").value = "689S4"
+    >>> b.getControl(name="oopsid").value = "1308X1"
     >>> b.getControl("Search OOPS").click()
 
     >>> b.url
-    'http://localhost/oops/?oopsid=689S4'
+    'http://localhost/oops/?oopsid=1308X1'
 
     >>> print b.contents
     <...
-    ...<title>OOPS-689S4</title>...
+    ...<title>OOPS-1308x1</title>...
     ...
 
 It doesn't care about case.
 
     >>> b.open('http://localhost/oops/')
-    >>> b.getControl(name="oopsid").value = "oops-689S4"
-    >>> b.getControl("Search OOPS").click()
-
-    >>> b.url
-    'http://localhost/oops/?oopsid=oops-689S4'
-
-    >>> print b.contents
-    <...
-    ...<title>OOPS-689S4</title>...
-    ...
+    >>> b.getControl(name="oopsid").value = "oops-1308x1"
+    >>> b.getControl("Search OOPS").click()
+
+    >>> b.url
+    'http://localhost/oops/?oopsid=oops-1308x1'
+
+    >>> print b.contents
+    <...
+    ...<title>OOPS-1308x1</title>...
+    ...
+
+Even when just the suffix is used:
+
+First we need to make sure that the oops can't be found by the fallback
+search-disk path (otherwise the test is invalid as amqp oopses will be outside
+the disk search path).
+
+    >>> from django.conf import settings
+    >>> old_root = settings.OOPSDIR
+    >>> settings.OOPSDIR = '/doesnotexist/'
+
+    >>> b.open('http://localhost/oops/')
+    >>> b.getControl(name="oopsid").value = "1308x1"
+    >>> b.getControl("Search OOPS").click()
+
+    >>> settings.OOPSDIR = old_root
+
+    >>> b.url
+    'http://localhost/oops/?oopsid=1308x1'
+
+    >>> print b.contents
+    <...
+    ...<title>OOPS-1308x1</title>...
+    ...
+
 
 It will also allow lower-case IDs with no OOPS- prefix, as used in ISD.
 

=== modified file 'src/oopstools/oops/views.py'
--- src/oopstools/oops/views.py	2011-10-13 20:18:51 +0000
+++ src/oopstools/oops/views.py	2011-11-02 23:29:23 +0000
@@ -29,24 +29,35 @@
 
 
 def index(request):
-    # Check both the given ID and a fixed-up version of it.
-    oopsids = [request.GET.get('oopsid', '')]
-    oopsid_fixed = oopsids[0].upper()
-    if not oopsid_fixed.startswith('OOPS-'):
-        oopsid_fixed = 'OOPS-' + oopsid_fixed
-    if oopsid_fixed not in oopsids:
-        oopsids.append(oopsid_fixed)
+    # For any OOPS we search for:
+    # - the literal string
+    # - an upper() version of the string in case it was previously upper cased
+    # on insertion into the DB.
+    # - a lower() version of the string in case it wasn't upper cased but the
+    # user's reporting tool has upper cased it.
+    # - those three things with OOPS- prefixed, if they do not have that prefix
+    # already in case the url the user was given had had the OOPS- prefixed
+    # removed for some unknown reason, and the report has an OOPS- prefix in
+    # its id.
+    query_str = request.GET.get('oopsid', '')
+    oopsids = set([query_str])
+    oopsids.add(query_str.upper())
+    oopsids.add(query_str.lower())
+    if not query_str.upper().startswith('OOPS-'):
+        with_oops = 'OOPS-' + query_str
+        oopsids.add(with_oops)
+        oopsids.add(with_oops.upper())
+        oopsids.add(with_oops.lower())
 
     # Check each ID in both the database and filesystem
     # (should maybe have an API option for this, instead of doing it manually?)
-    oops = None
-    for oopsid in oopsids:
-        try:
-            oops = Oops.objects.get(oopsid__exact=oopsid)
-            break
-        except Oops.DoesNotExist:
-            # Try to find the OOPS report in the filesystem.
-            store = OopsStore(settings.OOPSDIR)
+    try:
+        oops = Oops.objects.get(oopsid__in=oopsids)
+    except Oops.DoesNotExist:
+        # Try to find the OOPS report in the filesystem.
+        oops = None
+        store = OopsStore(settings.OOPSDIR)
+        for oopsid in oopsids:
             try:
                 oops = store.find_oops(oopsid)
                 break

=== modified file 'src/oopstools/scripts/amqp2disk.py'
--- src/oopstools/scripts/amqp2disk.py	2011-11-01 03:06:49 +0000
+++ src/oopstools/scripts/amqp2disk.py	2011-11-02 23:29:23 +0000
@@ -120,8 +120,6 @@
     # the first publisher will either inherit or assign, so this should be
     # impossible.
     assert report['id'] is not None
-    # Fit in with the upper-case assumption in oops-tools.
-    report['id'] = report['id'].upper()
     # Some fallback methods could lead to duplicate paths into the DB: exit
     # early if the OOPS is already loaded.
     try: