launchpad-reviewers team mailing list archive

[Curtis Hovey <curtis.hovey@xxxxxxxxxxxxx>]

Curtis Hovey has proposed merging lp:~sinzui/launchpad/system-bug-comments-api into lp:launchpad.

Commit message:
Do not make links to a message's parent message.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)
Related bugs:
  Bug #808952 in Launchpad itself: "NoCanonicalUrl using api to fetch bug comments"
  https://bugs.launchpad.net/launchpad/+bug/808952

For more details, see:
https://code.launchpad.net/~sinzui/launchpad/system-bug-comments-api/+merge/137308

A bug message is accessible over the API via two approaches. Iterating
over bug.messages or directly loading the message via its URL
(eg, +bug/1004834/comments/2). The latter case can oops with
NoCanonicalUrl (OOPS-2f0df95d0ba20796d46cfe2a1b3c3fa1) because the parent
message is not one of the bug's messages, it it does not have an index number.
bug.message returns IIndexedMessage which sets the parent to None to enforce
non-threaded messaging in bugs, but the latter case an IMessage which is
threaded. Both IIndexedMessage and IMessage claim to be bug messages, but
we know that the DB is storing all messages in the table.

--------------------------------------------------------------------

RULES

    Pre-implementation: no one
    * IMessage was exported as an accident of exporting bug messages, and
      they are now permanently in the API. They claim to just be bug messages
      as can be seen by their URL.
    * Since bug messages are not threaded, and we cannot unexport the
      parent attribute, we can follow IIndexedMessage's example by ensuring
      None is the value returned.
    * Export a replacement parent attr on the API that retrofits the beta
      api to return None.

QA

    * visit https://api.qastaging.launchpad.net/devel/ubuntu/+source/request-tracker3.8/+bug/1004834/comments/2
    * Verify the page loads.

LoC

    I have a 16,000 line credit this week.

LINT

    lib/lp/bugs/tests/test_bug_messages_webservice.py
    lib/lp/services/messages/interfaces/message.py
    lib/lp/services/messages/model/message.py

TEST

    ./bin/test -vvc -t MessageTraversal lp.bugs.tests.test_bug_messages_webservice

IMPLEMENTATION

I added getAPIParent to IMessage and exported it using accessor_for(parent)
to ensure the API never sees a parent message.
    lib/lp/bugs/tests/test_bug_messages_webservice.py
    lib/lp/services/messages/interfaces/message.py
    lib/lp/services/messages/model/message.py
-- 
https://code.launchpad.net/~sinzui/launchpad/system-bug-comments-api/+merge/137308
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~sinzui/launchpad/system-bug-comments-api into lp:launchpad.

=== modified file 'lib/lp/bugs/tests/test_bug_messages_webservice.py'
--- lib/lp/bugs/tests/test_bug_messages_webservice.py	2012-09-18 18:36:09 +0000
+++ lib/lp/bugs/tests/test_bug_messages_webservice.py	2012-11-30 20:01:21 +0000
@@ -50,6 +50,26 @@
             messages,
             expected_messages)
 
+    def test_message_with_parent(self):
+        # The API exposes the parent attribute IMessage that is hidden by
+        # IIndexedMessage. The representtion cannot make a link to the
+        # parent message because it is might switch to another context
+        # object that is not exposed or the user may not have access to.
+        bug = self.factory.makeBug()
+        message_1 = self.factory.makeMessage()
+        message_2 = self.factory.makeMessage()
+        message_2.parent = message_1
+        bug.linkMessage(message_2)
+        transaction.commit()
+        user = self.factory.makePerson()
+        lp_bug = self.wsObject(bug, user)
+        for lp_message in lp_bug.messages:
+            # An IIndexedMessage's representation.
+            self.assertIs(None, lp_message.parent)
+        # An IMessage's representation.
+        lp_message = self.wsObject(message_2, user)
+        self.assertIs(None, lp_message.parent)
+
 
 class TestSetCommentVisibility(TestCaseWithFactory):
     """Tests who can successfully set comment visibility."""

=== modified file 'lib/lp/services/messages/interfaces/message.py'
--- lib/lp/services/messages/interfaces/message.py	2012-04-27 19:03:32 +0000
+++ lib/lp/services/messages/interfaces/message.py	2012-11-30 20:01:21 +0000
@@ -23,8 +23,11 @@
 
 from lazr.delegates import delegates
 from lazr.restful.declarations import (
+    accessor_for,
     export_as_webservice_entry,
+    export_read_operation,
     exported,
+    operation_for_version,
     )
 from lazr.restful.fields import (
     CollectionField,
@@ -108,6 +111,12 @@
     def __iter__():
         """Iterate over all the message chunks."""
 
+    @accessor_for(parent)
+    @export_read_operation()
+    @operation_for_version('beta')
+    def getAPIParent():
+        """Return None because messages are not threaded over the API."""
+
 
 # Fix for self-referential schema.
 IMessage['parent'].schema = IMessage

=== modified file 'lib/lp/services/messages/model/message.py'
--- lib/lp/services/messages/model/message.py	2012-04-27 19:03:32 +0000
+++ lib/lp/services/messages/model/message.py	2012-11-30 20:01:21 +0000
@@ -163,6 +163,10 @@
     # interface because it is used as a UI field in MessageAddView
     content = None
 
+    def getAPIParent(self):
+        """See `IMessage`."""
+        return None
+
 
 def get_parent_msgids(parsed_message):
     """Returns a list of message ids the mail was a reply to.