Merge lp:~michael.nelson/ubuntu-webcatalog/1002262-dont-insert-errors-via-ajax into lp:ubuntu-webcatalog

[Michael Nelson]

Handle multiple apps per packagename/distroseries and other potential view and api errors when requesting reviews.

Overview
========

Contrary to the branch name, the issue was not related to any API call or ajax response. See bug 1002262 for the details, but in summary, we've now got multiple applications with the same packagename and distroseries in the DB (skype, unity-lens-utilities) - perhaps as a result of importing from both extras and myapps [1].

`fab test`

[1] Note: due to another bug, even if we've since removed the app from the published apps on myapps, the corresponding record won't have been removed in uwc - we only import new ones - bug 985446.

[Łukasz Czyżykowski (lukasz-czyzykowski)]

LGTM

review: Approve

[Michael Nelson (michael.nelson)]

I've just pushed up some further changes that:

1) Ensures that if in the future, the reviews view 500's for some other reason, we won't display the result, and
2) Ensures that if in the future, the rnr api does return an error, we catch it rather than also erroring.

[ISD Branch Mangler (isd-branches-mangler)]

There are additional revisions which have not been approved in review. Please seek review and approval of these new revisions.

=== modified file 'src/webcatalog/forms.py'
--- src/webcatalog/forms.py	2012-05-03 13:06:53 +0000
+++ src/webcatalog/forms.py	2012-05-25 08:57:18 +0000
@@ -141,7 +141,7 @@
             tagline, _, description = app_data['description'].partition('\n')
             app_data['comment'] = tagline
             app_data['description'] = description
-        if 'debtags' in  app_data and app_data['debtags']:
+        if 'debtags' in app_data and app_data['debtags']:
             app_data['debtags'] = json.dumps([get_hw_short_description(x)
                                    for x in app_data['debtags']])
         app_data['application_id'] = app_data.get('id')

=== modified file 'src/webcatalog/templates/webcatalog/application_detail.html'
--- src/webcatalog/templates/webcatalog/application_detail.html	2012-05-08 17:03:43 +0000
+++ src/webcatalog/templates/webcatalog/application_detail.html	2012-05-25 08:57:18 +0000
@@ -14,10 +14,12 @@
     var reviews_uri = "{% url wc-package-reviews-ajax application.distroseries.code_name application.package_name %}";
     Y.io(reviews_uri, {
         on: {
-            complete: function(id, obj){
-                var reviews_html = obj.responseText;
-                var reviews_div = Y.one('#reviews_placeholder');
-                reviews_div.set("innerHTML", reviews_html);
+            complete: function(id, response){
+                if (response.status == 200) {
+                    var reviews_html = response.responseText;
+                    var reviews_div = Y.one('#reviews_placeholder');
+                    reviews_div.set("innerHTML", reviews_html);
+                }
             }
         }
     });
@@ -26,9 +28,11 @@
         var recommendations_uri = "{% url wc-package-recommends application.package_name %}";
         Y.io(recommendations_uri, {
             on: {
-                complete: function(id, obj){
-                    var recommends_html = obj.responseText;
-                    recommends_div.set("innerHTML", recommends_html);
+                complete: function(id, response){
+                    if (response.status == 200) {
+                        var recommends_html = response.responseText;
+                        recommends_div.set("innerHTML", recommends_html);
+                    }
                 }
             }
         });

=== modified file 'src/webcatalog/tests/test_views.py'
--- src/webcatalog/tests/test_views.py	2012-05-23 23:11:26 +0000
+++ src/webcatalog/tests/test_views.py	2012-05-25 08:57:18 +0000
@@ -34,6 +34,7 @@
 from django.test import TestCase
 
 from mock import patch
+from piston_mini_client.failhandlers import APIError
 from rnrclient import ReviewDetails
 
 from webcatalog.models import (
@@ -1043,6 +1044,25 @@
 
         self.assertEqual(200, response.status_code)
 
+    def test_handles_multiple_apps_for_same_distroseries(self):
+        ds = self.factory.make_distroseries()
+        self.factory.make_application(package_name='skype', distroseries=ds)
+        self.factory.make_application(package_name='skype', distroseries=ds)
+
+        response = self.client.get(reverse('wc-package-reviews',
+            args=[ds.code_name, 'skype']))
+
+        self.assertEqual(200, response.status_code)
+
+    def test_handles_api_error(self):
+        app = self.factory.make_application()
+        self.mock_get_reviews.side_effect = APIError('500', 'error')
+
+        response = self.client.get(reverse('wc-package-reviews',
+            args=[app.distroseries.code_name, app.package_name]))
+
+        self.assertEqual(200, response.status_code)
+
 
 class ApplicationScreenshotsTestCase(TestCaseWithFactory):
     def setUp(self):

=== modified file 'src/webcatalog/utilities.py'
--- src/webcatalog/utilities.py	2012-04-20 19:15:00 +0000
+++ src/webcatalog/utilities.py	2012-05-25 08:57:18 +0000
@@ -79,7 +79,7 @@
         try:
             fresh_reviews = self.rnr_api.get_reviews(packagename=package_name,
                 distroseries=distroseries, page=page)
-        except ValidationException:
+        except (ValidationException, APIError):
             fresh_reviews = []
         cache.set(key, fresh_reviews, settings.REVIEWS_CACHE_TIMEOUT)
         return fresh_reviews

=== modified file 'src/webcatalog/views.py'
--- src/webcatalog/views.py	2012-05-23 22:28:39 +0000
+++ src/webcatalog/views.py	2012-05-25 08:57:18 +0000
@@ -37,6 +37,7 @@
     HttpResponse,
     )
 from django.shortcuts import (
+    get_list_or_404,
     get_object_or_404,
     render_to_response,
     )
@@ -224,8 +225,15 @@
 
 
 def application_reviews(request, package_name, distro, ajax=False, page=1):
-    app = get_object_or_404(Application, package_name=package_name,
+    # XXX 2012-05-25 bug=1002262 michaeln Multiple apps for distroseries.
+    # It seems that we have a few applications which are present in
+    # multiple archives for the same distroseries (skype,
+    # unity-lens-utilities, probably via a myapps PPA as well as extras?).
+    # Don't break when this happens.
+    apps = get_list_or_404(Application, package_name=package_name,
                             distroseries__code_name=distro)
+    app = apps[0]
+
     # XXX michaeln 2011-09-15 bug=851662 Better review language options.
     reviews = WebServices().get_reviews_for_package(package_name,
         distroseries=distro, page=page)