launchpad-reviewers team mailing list archive

[Colin Watson <cjwatson@xxxxxxxxxxxxx>]

Colin Watson has proposed merging ~cjwatson/launchpad:answers-api-redirects into launchpad:master.

Commit message:
Send proper webservice redirects for questions

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)

For more details, see:
https://code.launchpad.net/~cjwatson/launchpad/+git/launchpad/+merge/391821

launchpadlib handles this correctly nowadays, but only if we send redirects to the correct target host (e.g. api.launchpad.net rather than answers.launchpad.net).
-- 
Your team Launchpad code reviewers is requested to review the proposed merge of ~cjwatson/launchpad:answers-api-redirects into launchpad:master.

diff --git a/lib/lp/answers/browser/question.py b/lib/lp/answers/browser/question.py
index e660cb2..491ff16 100644
--- a/lib/lp/answers/browser/question.py
+++ b/lib/lp/answers/browser/question.py
@@ -1,4 +1,4 @@
-# Copyright 2009-2018 Canonical Ltd.  This software is licensed under the
+# Copyright 2009-2020 Canonical Ltd.  This software is licensed under the
 # GNU Affero General Public License version 3 (see the file LICENSE).
 
 """Question views."""
@@ -253,13 +253,8 @@ class QuestionSetNavigation(Navigation):
             question = None
         if question is None:
             raise NotFoundError(name)
-        # 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 self.redirectSubTree(
-                canonical_url(question, self.request), status=301)
+        return self.redirectSubTree(
+            canonical_url(question, self.request), status=301)
 
 
 class QuestionBreadcrumb(Breadcrumb):
diff --git a/lib/lp/answers/browser/questiontarget.py b/lib/lp/answers/browser/questiontarget.py
index a80b36f..ef277c0 100644
--- a/lib/lp/answers/browser/questiontarget.py
+++ b/lib/lp/answers/browser/questiontarget.py
@@ -1,4 +1,4 @@
-# Copyright 2009-2018 Canonical Ltd.  This software is licensed under the
+# Copyright 2009-2020 Canonical Ltd.  This software is licensed under the
 # GNU Affero General Public License version 3 (see the file LICENSE).
 
 """IQuestionTarget browser views."""
@@ -887,7 +887,8 @@ class QuestionTargetTraversalMixin:
         question = getUtility(IQuestionSet).get(question_id)
         if question is None:
             raise NotFoundError(name)
-        return self.redirectSubTree(canonical_url(question))
+        return self.redirectSubTree(
+            canonical_url(question, request=self.request))
 
     @stepto('+ticket')
     def redirect_ticket(self):
@@ -896,7 +897,9 @@ class QuestionTargetTraversalMixin:
         It will take care of the remaining steps and query URL.
         """
         target = urlappend(
-            canonical_url(self.context, rootsite='answers'), '+question')
+            canonical_url(
+                self.context, request=self.request, rootsite='answers'),
+            '+question')
         return self.redirectSubTree(target)
 
 
diff --git a/lib/lp/answers/stories/question-compatibility-urls.txt b/lib/lp/answers/stories/question-compatibility-urls.txt
index 42116c3..29eaf2a 100644
--- a/lib/lp/answers/stories/question-compatibility-urls.txt
+++ b/lib/lp/answers/stories/question-compatibility-urls.txt
@@ -38,6 +38,10 @@ terminology. We provide redirect from the old names to the new ones.
     >>> print(browser.url)
     http://answers.launchpad.test/firefox/+question/1
 
+    >>> browser.open('http://api.launchpad.test/devel/firefox/+ticket/1')
+    >>> print(browser.url)
+    http://api.launchpad.test/devel/firefox/+question/1
+
 == Person Questions Listing ==
 
     >>> browser.open('http://launchpad.test/~name12/+tickets')
diff --git a/lib/lp/answers/stories/question-overview.txt b/lib/lp/answers/stories/question-overview.txt
index 6da1923..f9150ad 100644
--- a/lib/lp/answers/stories/question-overview.txt
+++ b/lib/lp/answers/stories/question-overview.txt
@@ -225,6 +225,12 @@ the proper context where the question can be found:
     >>> print(find_main_content(browser.contents).find('h1').renderContents())
     Firefox cannot render Bank Site
 
+This also works on the webservice.
+
+    >>> browser.open('http://api.launchpad.test/devel/questions/1')
+    >>> print(browser.url)
+    http://api.launchpad.test/devel/firefox/+question/1
+
 Asking for a non-existent question or an invalid ID will still raise a
 404 though:
 
@@ -246,6 +252,10 @@ useful after a question was retargeted.)
     >>> print(browser.url)
     http://answers.launchpad.test/firefox/+question/1
 
+    >>> browser.open('http://api.launchpad.test/devel/ubuntu/+question/1')
+    >>> print(browser.url)
+    http://api.launchpad.test/devel/firefox/+question/1
+
 It also works with pages below that URL:
 
     >>> browser.open(
diff --git a/lib/lp/answers/tests/test_question_webservice.py b/lib/lp/answers/tests/test_question_webservice.py
index 11fc14d..1dd2e42 100644
--- a/lib/lp/answers/tests/test_question_webservice.py
+++ b/lib/lp/answers/tests/test_question_webservice.py
@@ -1,4 +1,4 @@
-# Copyright 2011-2019 Canonical Ltd.  This software is licensed under the
+# Copyright 2011-2020 Canonical Ltd.  This software is licensed under the
 # GNU Affero General Public License version 3 (see the file LICENSE).
 
 """Webservice unit tests related to Launchpad Questions."""
@@ -118,7 +118,11 @@ class TestQuestionRepresentation(TestCaseWithFactory):
         # 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, 200)
+        self.assertEqual(response.status, 301)
+        self.assertEqual(
+            'http://api.launchpad.test/devel/%s/+question/%d' % (
+                self.target_name, self.question.id),
+            response.getheader('Location'))
 
     def test_GET_xhtml_representation(self):
         # A question's xhtml representation is available on the api.