Fix some security issues and make the traversal adapter work with OFS.Traversable. Requires a name, so we now call it ++conversation++default

svn path=/plone.app.discussion/trunk/; revision=27059

plone/app/discussion/browser/configure.zcml

@@ -4,7 +4,7 @@
     i18n_domain="plone.app.discussion">
     
     <!-- Traversal adapter -->
-    <adapter factory=".traversal.ConversationNamespace" name="comment" />
+    <adapter factory=".traversal.ConversationNamespace" name="conversation" />
 
     <!-- Comments viewlet -->
     <browser:viewlet

plone/app/discussion/browser/traversal.py

@@ -4,7 +4,7 @@ into an actual comment object.
 """
 
 from zope.interface import Interface, implements
-from zope.component import adapts
+from zope.component import adapts, queryAdapter
 
 from zope.traversing.interfaces import ITraversable, TraversalError
 from zope.publisher.interfaces.browser import IBrowserRequest
@@ -12,8 +12,13 @@ from zope.publisher.interfaces.browser import IBrowserRequest
 from plone.app.discussion.interfaces import IConversation
 
 class ConversationNamespace(object):
-    """Allow traversal into a conversation
+    """Allow traversal into a conversation via a ++conversation++name
+    namespace. The name is the name of an adapter from context to
+    IConversation. The special name 'default' will be taken as the default
+    (unnamed) adapter. This is to work around a bug in OFS.Traversable which
+    does not allow traversal to namespaces with an empty string name.
     """
+    
     implements(ITraversable)
     adapts(Interface, IBrowserRequest)
     
@@ -23,8 +28,11 @@ class ConversationNamespace(object):
         
     def traverse(self, name, ignore):
         
-        conversation = IConversation(self.context, None)
+        if name == "default":
-        if conversation is None:
+            name = u""
-            raise TraversalError('++comment++')
         
-        return conversation.__of__(self.context)
+        conversation = queryAdapter(self.context, IConversation, name=name)
+        if conversation is None:
+            raise TraversalError(name)
+        
+        return conversation

plone/app/discussion/conversation.py

@@ -58,7 +58,9 @@ class Conversation(Traversable, Persistent, Explicit):
     
     implements(IConversation)
     
-    def __init__(self, id="++comment++"):
+    __allow_access_to_unprotected_subobjects__ = True
+    
+    def __init__(self, id="++conversation++default"):
         self.id = id
         
         # username -> count of comments; key is removed when count reaches 0
@@ -71,7 +73,8 @@ class Conversation(Traversable, Persistent, Explicit):
         self._children = LOBTree()
         
     def getId(self):
-        """Get the id of 
+        """Get the id of the conversation. This is used to construct a
+        URL.
         """
         return self.id
     
@@ -216,7 +219,8 @@ class Conversation(Traversable, Persistent, Explicit):
 @implementer(IConversation)
 @adapter(IAnnotatable)
 def conversationAdapterFactory(content):
-    """Adapter factory to fetch a conversation from annotations
+    """Adapter factory to fetch the default conversation from annotations.
+    Will create the conversation if it does not exist.
     """
     annotions = IAnnotations(content)
     if not ANNOTATION_KEY in annotions:

plone/app/discussion/tests/test_comment.py

@@ -6,7 +6,6 @@ from Products.PloneTestCase.ptc import PloneTestCase
 from plone.app.discussion.tests.layer import DiscussionLayer
 
 from plone.app.discussion.interfaces import IComment, IConversation
-from plone.app.discussion.comment import Comment
 
 class CommentTest(PloneTestCase):
     
@@ -19,30 +18,44 @@ class CommentTest(PloneTestCase):
         typetool.constructContent('Document', self.portal, 'doc1')
     
     def test_factory(self):
-        # test with createObject()
+        comment1 = createObject('plone.Comment')
-        pass
+        self.assert_(IComment.providedBy(comment1))
         
     def test_id(self):
-        # relationship between id, getId(), __name__
+        comment1 = createObject('plone.Comment')
-        pass
+        comment1.comment_id = 123
+        self.assertEquals('123', comment1.id)
+        self.assertEquals('123', comment1.getId())
+        self.assertEquals(u'123', comment1.__name__)
         
     def test_title(self):
-        pass
+        comment1 = createObject('plone.Comment')
+        comment1.title = "New title"
+        self.assertEquals("New title", comment1.Title())
     
     def test_creator(self):
-        pass
+        comment1 = createObject('plone.Comment')
+        comment1.creator = "Jim"
+        self.assertEquals("Jim", comment1.Creator())
         
     def test_traversal(self):
         # make sure comments are traversable, have an id, absolute_url and physical path
         
-        # XXX - traversal doesn't work without a name?
+        conversation = IConversation(self.portal.doc1).__of__(self.portal.doc1)
-        conversation = self.portal.doc1.restrictedTraverse('++comment++1')        
-        self.assert_(IConversation.providedBy(conversation))
         
-        # TODO: Test adding comments, traversing to them
+        comment1 = createObject('plone.Comment')
+        comment1.title = 'Comment 1'
+        comment1.text = 'Comment text'
+
+        new_comment1_id = conversation.addComment(comment1)
+        
+        comment = self.portal.doc1.restrictedTraverse('++conversation++default/%s' % new_comment1_id)
+        self.assert_(IComment.providedBy(comment))
+        self.assertEquals('Comment 1', comment.title)
+        
+        self.assertEquals(('', 'plone', 'doc1', '++conversation++default', str(new_comment1_id)), comment.getPhysicalPath())
+        self.assertEquals('plone/doc1/%2B%2Bconversation%2B%2Bdefault/' + str(new_comment1_id), comment.absolute_url())
         
-        pass
-    
     def test_workflow(self):
         # ensure that we can assign a workflow to the comment type and perform
         # workflow operations

plone/app/discussion/tests/test_conversation.py

@@ -47,7 +47,7 @@ class ConversationTest(PloneTestCase):
         self.assertEquals(conversation.total_comments, 1)
         self.assert_(conversation.last_comment_date - datetime.now() < timedelta(seconds=1))
 
-    def test_delete(self):
+    def test_delete_comment(self):
         pass
 
     def test_dict_operations(self):
@@ -103,6 +103,15 @@ class ConversationTest(PloneTestCase):
 
     def test_get_threads_batched(self):
         pass
+    
+    def test_traversal(self):
+        # make sure we can traverse to conversations and get a URL and path
+        
+        conversation = self.portal.doc1.restrictedTraverse('++conversation++default')        
+        self.assert_(IConversation.providedBy(conversation))
+        
+        self.assertEquals(('', 'plone', 'doc1', '++conversation++default'), conversation.getPhysicalPath())
+        self.assertEquals('plone/doc1/%2B%2Bconversation%2B%2Bdefault', conversation.absolute_url())
 
 class RepliesTest(PloneTestCase):