launchpad-reviewers team mailing list archive

[William Grant <me@xxxxxxxxxxxxxxxxxx>]

William Grant has proposed merging lp:~wgrant/launchpad/bug-1105543 into lp:launchpad.

Commit message:
Fix get_comments_for_bugtask to not skip some attachments when handling slices of comments.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)
Related bugs:
  Bug #1105543 in Launchpad itself: "attachment added within a comment is sometimes hidden"
  https://bugs.launchpad.net/launchpad/+bug/1105543

For more details, see:
https://code.launchpad.net/~wgrant/launchpad/bug-1105543/+merge/267603

get_comments_for_bugtask assumed that it could stop looking for attachments on the bug whenever it found an attachment that was for a message that it wasn't interested in. But that assumed it was only ever fetching a chronological prefix of the comments and attachments -- not true when incrementally loading comments on a bug page, or viewing a single comment. As a result, attachments were missing from the API and web UI in some circumstances: bug #1105543, bug #1348148, bug #1483283.

I also tweaked the function to avoid querying each attachment's message where that was unnecessary, saving a few queries.
-- 
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~wgrant/launchpad/bug-1105543 into lp:launchpad.

=== modified file 'lib/lp/bugs/browser/bugtask.py'
--- lib/lp/bugs/browser/bugtask.py	2015-07-09 20:06:17 +0000
+++ lib/lp/bugs/browser/bugtask.py	2015-08-11 01:21:04 +0000
@@ -242,12 +242,10 @@
         get_property_cache(comment._message).bugattachments = []
 
     for attachment in bugtask.bug.attachments_unpopulated:
-        message_id = attachment.message.id
-        # All attachments are related to a message, so we can be
-        # sure that the BugComment is already created.
+        message_id = attachment._messageID
         if message_id not in comments:
             # We are not showing this message.
-            break
+            continue
         if attachment.type == BugAttachmentType.PATCH:
             comments[message_id].patches.append(attachment)
         cache = get_property_cache(attachment.message)

=== modified file 'lib/lp/bugs/interfaces/bugattachment.py'
--- lib/lp/bugs/interfaces/bugattachment.py	2015-01-30 18:24:07 +0000
+++ lib/lp/bugs/interfaces/bugattachment.py	2015-08-11 01:21:04 +0000
@@ -117,6 +117,7 @@
         Bytes(title=_("The attachment content."),
               required=True,
               readonly=True))
+    _messageID = Int(title=_("Message ID"))
     message = exported(
         Reference(IMessage, title=_("The message that was created when we "
                                     "added this attachment.")))

=== modified file 'lib/lp/bugs/tests/test_bug_messages_webservice.py'
--- lib/lp/bugs/tests/test_bug_messages_webservice.py	2012-11-30 20:11:54 +0000
+++ lib/lp/bugs/tests/test_bug_messages_webservice.py	2015-08-11 01:21:04 +0000
@@ -6,6 +6,7 @@
 __metaclass__ = type
 
 from lazr.restfulclient.errors import HTTPError
+from testtools.matchers import HasLength
 import transaction
 from zope.component import getUtility
 from zope.security.management import endInteraction
@@ -16,13 +17,20 @@
 from lp.registry.interfaces.accesspolicy import IAccessPolicySource
 from lp.registry.interfaces.person import IPersonSet
 from lp.testing import (
+    admin_logged_in,
+    api_url,
     launchpadlib_for,
     login_celebrity,
+    logout,
     person_logged_in,
     TestCaseWithFactory,
     WebServiceTestCase,
     )
-from lp.testing.layers import DatabaseFunctionalLayer
+from lp.testing.layers import (
+    DatabaseFunctionalLayer,
+    LaunchpadFunctionalLayer,
+    )
+from lp.testing.pages import LaunchpadWebServiceCaller
 
 
 class TestMessageTraversal(WebServiceTestCase):
@@ -70,6 +78,41 @@
         self.assertIs(None, lp_message.parent)
 
 
+class TestBugMessage(TestCaseWithFactory):
+
+    layer = LaunchpadFunctionalLayer
+
+    def test_attachments(self):
+        # A bug's attachments and the union of its messages' attachments
+        # are the same set.
+        with admin_logged_in():
+            bug = self.factory.makeBug()
+            created_attachment_ids = set(
+                self.factory.makeBugAttachment(bug).id for i in range(3))
+            bug_url = api_url(bug)
+        self.assertThat(created_attachment_ids, HasLength(3))
+        logout()
+
+        webservice = LaunchpadWebServiceCaller('test', None)
+        bug_attachments = webservice.get(
+            bug_url + '/attachments').jsonBody()['entries']
+        bug_attachment_ids = set(
+            int(att['self_link'].rsplit('/', 1)[1]) for att in bug_attachments)
+        self.assertContentEqual(created_attachment_ids, bug_attachment_ids)
+
+        messages = webservice.get(bug_url + '/messages').jsonBody()['entries']
+        message_attachments = []
+        for message in messages[1:]:
+            attachments_url = message['bug_attachments_collection_link']
+            attachments = webservice.get(attachments_url).jsonBody()['entries']
+            self.assertThat(attachments, HasLength(1))
+            message_attachments.append(attachments[0])
+        message_attachment_ids = set(
+            int(att['self_link'].rsplit('/', 1)[1])
+            for att in message_attachments)
+        self.assertContentEqual(bug_attachment_ids, message_attachment_ids)
+
+
 class TestSetCommentVisibility(TestCaseWithFactory):
     """Tests who can successfully set comment visibility."""