launchpad-reviewers team mailing list archive

[Colin Watson <mp+423249@xxxxxxxxxxxxxxxxxx>]

Colin Watson has proposed merging ~cjwatson/launchpad:sshkeys-trailing-newline into launchpad:master.

Commit message:
Add a trailing newline to Person:+sshkeys

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)

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

It's annoying for text endpoints to return output without a trailing newline.  For instance, it meant that running `curl` on this endpoint produced confusing output because the next shell prompt appeared immediately after the last SSH key with no intervening line break.

(In fact, POSIX requires text files to have a terminating newline.  See https://pubs.opengroup.org/onlinepubs/9699919799/basedefs/V1_chap03.html#tag_03_403 and https://pubs.opengroup.org/onlinepubs/9699919799/basedefs/V1_chap03.html#tag_03_206.)
-- 
Your team Launchpad code reviewers is requested to review the proposed merge of ~cjwatson/launchpad:sshkeys-trailing-newline into launchpad:master.

diff --git a/lib/lp/registry/browser/person.py b/lib/lp/registry/browser/person.py
index 390221b..e1c31b3 100644
--- a/lib/lp/registry/browser/person.py
+++ b/lib/lp/registry/browser/person.py
@@ -1897,7 +1897,7 @@ class PersonView(LaunchpadView, FeedsMixin, ContactViaWebLinksMixin):
         """Return a data structure used for display of raw SSH keys"""
         self.request.response.setHeader('Content-Type', 'text/plain')
         keys = [key.getFullKeyText() for key in self.context.sshkeys]
-        return "\n".join(keys)
+        return "".join(key + "\n" for key in keys)
 
     @cachedproperty
     def archive_url(self):
diff --git a/lib/lp/registry/browser/tests/test_person.py b/lib/lp/registry/browser/tests/test_person.py
index 5db6ff0..9effeef 100644
--- a/lib/lp/registry/browser/tests/test_person.py
+++ b/lib/lp/registry/browser/tests/test_person.py
@@ -2316,4 +2316,28 @@ class TestPersonRdfView(BrowserTestCase):
             browser.headers['Content-type'])
 
 
+class TestPersonViewSSHKeys(BrowserTestCase):
+    """Tests for Person:+sshkeys."""
+
+    layer = DatabaseFunctionalLayer
+
+    def test_no_keys(self):
+        person = self.factory.makePerson()
+        browser = self.getViewBrowser(person, view_name="+sshkeys")
+        self.assertEqual(
+            "text/plain;charset=utf-8", browser.headers["Content-Type"])
+        self.assertEqual("", browser.contents)
+
+    def test_keys(self):
+        person = self.factory.makePerson()
+        with person_logged_in(person):
+            keys = [self.factory.makeSSHKey(person) for _ in range(2)]
+        browser = self.getViewBrowser(person, view_name="+sshkeys")
+        self.assertEqual(
+            "text/plain;charset=utf-8", browser.headers["Content-Type"])
+        self.assertContentEqual(
+            [key.getFullKeyText() + "\n" for key in keys],
+            re.findall(r".*\n", browser.contents))
+
+
 load_tests = load_tests_apply_scenarios