launchpad-reviewers team mailing list archive

["j.c.sackett" <jonathan.sackett@xxxxxxxxxxxxx>]

j.c.sackett has proposed merging lp:~jcsackett/launchpad/bloody-question-api-redirect into lp:launchpad.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)

For more details, see:
https://code.launchpad.net/~jcsackett/launchpad/bloody-question-api-redirect/+merge/59248

Summary
=======
When trying to call some methods on questions over the API (i.e. setCommentVisibility) an error occurs because the API is annoyed at a redirect provided by the questionset toplevel. Other toplevel resource (e.g. Bugs) don't use a redirect when being used by the API. This adds the same sort of behavior to questionset.

Preimplemenation
================
Spoke with Curtis Hovey.

Implemenation
=============
lib/lp/answers/browser/question.py
----------------------------------
Put in a check to determine if this was a webservice request, which specifies a 'version' attribute, in which case we simply return the question and let traversal sort out the rest. If it's not an API request we return a redirect to the canonical url, as before.

lib/lp/answers/tests/test_question_webservice.py
------------------------------------------------
Updated tests to assert a 200, rather than 301, when accessing a question via the toplevel resource.

QA
==
{{{
    q = lp.load('/questions/$question_id/')
    try:
        q.setCommentVisibility(comment_number=$comment_index, visible=True)
    except:
        print ':-)'
    else:
        print ':-('
}}}

Tests
=====
bin/test -vvct test_question_webservice

Lint
====
= Launchpad lint =

Checking for conflicts and issues in changed files.

Linting changed files:
  lib/lp/answers/browser/question.py
  lib/lp/answers/tests/test_question_webservice.py

-- 
https://code.launchpad.net/~jcsackett/launchpad/bloody-question-api-redirect/+merge/59248
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~jcsackett/launchpad/bloody-question-api-redirect into lp:launchpad.

=== modified file 'lib/lp/answers/browser/question.py'
--- lib/lp/answers/browser/question.py	2011-04-26 16:39:33 +0000
+++ lib/lp/answers/browser/question.py	2011-04-27 16:01:11 +0000
@@ -227,6 +227,7 @@
 
 class QuestionSetNavigation(Navigation):
     """Navigation for the IQuestionSet."""
+
     usedfor = IQuestionSet
 
     def traverse(self, name):
@@ -237,7 +238,13 @@
             question = None
         if question is None:
             raise NotFoundError(name)
-        return redirection(canonical_url(question, self.request), status=301)
+        # We need to check if this is an API request, as we don't want to
+        # send a redirect in that instance (it breaks launchpadlib).
+        if hasattr(self.request, 'version'):
+            return question
+        else:
+            return redirection(
+                canonical_url(question, self.request), status=301)
 
 
 class QuestionBreadcrumb(Breadcrumb):

=== modified file 'lib/lp/answers/tests/test_question_webservice.py'
--- lib/lp/answers/tests/test_question_webservice.py	2011-04-18 12:12:33 +0000
+++ lib/lp/answers/tests/test_question_webservice.py	2011-04-27 16:01:11 +0000
@@ -20,7 +20,7 @@
     launchpadlib_for,
     logout,
     person_logged_in,
-    ws_object
+    ws_object,
     )
 
 
@@ -51,8 +51,7 @@
         # a question by id via redirect without url hacking.
         response = self.webservice.get(
             '/questions/%s' % self.question.id, 'application/xhtml+xml')
-        self.assertEqual(response.status, 301)
-
+        self.assertEqual(response.status, 200)
 
     def test_GET_xhtml_representation(self):
         # A question's xhtml representation is available on the api.
@@ -107,14 +106,6 @@
         """Convenience function to get the api question reference."""
         # End any open lplib instance.
         logout()
-        if user is not None:
-            lp = launchpadlib_for("test", user)
-        else:
-            lp = launchpadlib_for("test")
-
-        question_entry = lp.load(
-            '/%s/+question/%d/' % (
-                self.question.target.name, self.question.id))
         lp = launchpadlib_for("test", user)
         return ws_object(lp, self.question)