From 22ae84735b37416551806f41c015ee12cf292498 Mon Sep 17 00:00:00 2001 From: Martin Aspeli Date: Mon, 18 May 2009 14:16:48 +0000 Subject: [PATCH] - specify dependencies - set profile version to 1 (profile version != package version) - complete dict APIs and IReplies adapters (not yet fully tested) - tidy up addComment() and __delitem__ w.r.t events - sync interfaces with actual code - move tests to use collective.testcaselayer svn path=/plone.app.discussion/trunk/; revision=27010 --- plone/app/discussion/TODO.txt | 37 +++ plone/app/discussion/comment.py | 11 +- plone/app/discussion/configure.zcml | 19 +- plone/app/discussion/conversation.py | 214 ++++++++++++++---- plone/app/discussion/interfaces.py | 66 ++++-- .../discussion/profiles/default/metadata.xml | 2 +- plone/app/discussion/tests/base.py | 29 --- plone/app/discussion/tests/layer.py | 12 + plone/app/discussion/tests/test_api.py | 25 +- plone/app/discussion/tests/test_tool.py | 28 +-- plone/app/discussion/tool.py | 12 +- setup.py | 11 +- 12 files changed, 318 insertions(+), 148 deletions(-) create mode 100644 plone/app/discussion/TODO.txt delete mode 100644 plone/app/discussion/tests/base.py create mode 100644 plone/app/discussion/tests/layer.py diff --git a/plone/app/discussion/TODO.txt b/plone/app/discussion/TODO.txt new file mode 100644 index 0000000..8202da5 --- /dev/null +++ b/plone/app/discussion/TODO.txt @@ -0,0 +1,37 @@ +========================== +plone.app.discussion to-do +========================== + + [ ] Thread building in conversation.getThreads() + [ ] Batching in conversation.getComments() + + [ ] ++comments++ namespace traversal adapter + + [ ] Acquisition wrapping - should it happen on: + + - Conversation retrieval methods (get, __getitem__, values etc)? + - IConversation adapter lookup? + + [ ] Implement plone.indexer indexers for comments, filling standard metadata + + - Note discrepancy between Python datetime and indexing expecting a Zope 2 + DateTime field + + [ ] Implement plone.indexer indexers for commented-upon content + + - Unique set of commentators + - Number of comments + - Date/time of most recent comment + + Needs to reindex when comment is added/removed (IContainerModifiedEvent) + + [ ] Add tests for conversation dict API + [ ] Add tests for IReplies adapters + + [ ] Make sure a catalog Clear & Rebuild doesn't lose all comments + + [ ] Add BBB support for the existing portal_discussion interface + + - implement in BBB package + - mix into tool.CommentingTool + - emit deprecation warnings diff --git a/plone/app/discussion/comment.py b/plone/app/discussion/comment.py index 9b849b2..d7c3dc2 100644 --- a/plone/app/discussion/comment.py +++ b/plone/app/discussion/comment.py @@ -1,6 +1,5 @@ """The default comment class and factory. """ -import time from datetime import datetime from zope.interface import implements from zope.component.factory import Factory @@ -26,8 +25,8 @@ class Comment(Explicit, Traversable, RoleManager, Owned): __parent__ = None - comment_id = None # int - in_reply_to = None # int + comment_id = None # long + in_reply_to = None # long title = u"" @@ -44,7 +43,7 @@ class Comment(Explicit, Traversable, RoleManager, Owned): author_email = None def __init__(self, conversation=None, **kw): - self.comment_id = long(time.time() * 1e6) + self.comment_id = None # will be set by IConversation.addComment() self.__parent__ = conversation self.creation_date = self.modification_date = datetime.now() @@ -54,11 +53,11 @@ class Comment(Explicit, Traversable, RoleManager, Owned): @property def __name__(self): - return unicode(self.comment_id) + return self.comment_id and unicode(self.comment_id) or None @property def id(self): - return str(self.comment_id) + return self.comment_id and str(self.comment_id) or None def getId(self): """The id of the comment, as a string diff --git a/plone/app/discussion/configure.zcml b/plone/app/discussion/configure.zcml index 06e0875..c49086f 100644 --- a/plone/app/discussion/configure.zcml +++ b/plone/app/discussion/configure.zcml @@ -8,12 +8,13 @@ + name="default" + title="Plone Discussions" + description="Commenting infrastructure for Plone" + directory="profiles/default" + provides="Products.GenericSetup.interfaces.EXTENSION" + for="Products.CMFPlone.interfaces.IPloneSiteRoot" + /> @@ -36,14 +37,14 @@ - + diff --git a/plone/app/discussion/conversation.py b/plone/app/discussion/conversation.py index 19705a9..4a83127 100644 --- a/plone/app/discussion/conversation.py +++ b/plone/app/discussion/conversation.py @@ -10,6 +10,8 @@ manipulating the comments directly in reply to a particular comment or at the top level of the conversation. """ +import time + from persistent import Persistent from zope.interface import implements, implementer @@ -17,18 +19,22 @@ from zope.component import adapts, adapter from zope.annotation.interfaces import IAnnotations from zope.event import notify -from zope.app.container.interfaces import IObjectAddedEvent + +from Acquisition import Explicit +from OFS.Traversable import Traversable + from OFS.event import ObjectWillBeAddedEvent from OFS.event import ObjectWillBeRemovedEvent + from zope.app.container.contained import ContainerModifiedEvent + from zope.app.container.contained import ObjectAddedEvent from zope.app.container.contained import ObjectRemovedEvent from zope.annotation.interfaces import IAnnotatable from BTrees.OIBTree import OIBTree -from BTrees.IOBTree import IOBTree -from BTrees.IIBTree import IIBTree, IISet + try: # These exist in new versions, but not in the one that comes with Zope 2.10. from BTrees.LOBTree import LOBTree @@ -37,13 +43,13 @@ except ImportError: from BTrees.OOBTree import OOBTree as LOBTree from BTrees.OOBTree import OOSet as LLSet - -from Acquisition import Explicit from plone.app.discussion.interfaces import IConversation, IComment, IReplies +from Acquisition import aq_base + ANNO_KEY = 'plone.app.discussion:conversation' -class Conversation(Persistent, Explicit): +class Conversation(Traversable, Persistent, Explicit): """A conversation is a container for all comments on a content object. It manages internal data structures for comment threading and efficient @@ -57,7 +63,6 @@ class Conversation(Persistent, Explicit): # username -> count of comments; key is removed when count reaches 0 self._commentators = OIBTree() - self._last_comment_date = None # id -> comment - find comment by id self._comments = LOBTree() @@ -66,55 +71,76 @@ class Conversation(Persistent, Explicit): self._children = LOBTree() def getId(self): - """ + """Get the id of """ return self.id @property def enabled(self): - # TODO + # TODO - check __parent__'s settings + global settings return True @property def total_comments(self): - # TODO return len(self._comments) @property def last_comment_date(self): - return self._last_comment_date + try: + return self._comments[self._comments.maxKey()].creation_date + except (ValueError, KeyError, AttributeError,): + return None @property def commentators(self): - # TODO: - return set() + return self._commentators.keys() def getComments(self, start=0, size=None): + """Get unthreaded comments + """ + # TODO - batching return self._comments.values() def getThreads(self, start=0, size=None, root=None, depth=None): - # TODO: + """Get threaded comments + """ + # TODO - build threads return self._comments.values() def addComment(self, comment): - id = comment.comment_id - if id in self._comments: - id = max(self._comments.keys()) + 1 + """Add a new comment. The parent id should have been set already. The + comment id may be modified to find a free key. The id used will be + returned. + """ + + # Make sure we don't have a wrapped object + + comment = aq_base(comment) + + id = long(time.time() * 1e6) + while id in self._comments: + id += 1 + + comment.comment_id = id notify(ObjectWillBeAddedEvent(comment, self, id)) self._comments[id] = comment - comment.comment_id = id - commentator = comment.creator - if not commentator in self._commentators: - self._commentators[commentator] = 0 - self._commentators[commentator] += 1 - - self._last_comment_date = comment.creation_date + # for logged in users only + commentator = comment.author_username + if commentator: + if not commentator in self._commentators: + self._commentators[commentator] = 0 + self._commentators[commentator] += 1 reply_to = comment.in_reply_to + if not reply_to: + # top level comments are in reply to the faux id 0 + comment.in_reply_to = reply_to = 0 + if not reply_to in self._children: self._children[reply_to] = LLSet() self._children[reply_to].insert(id) + # Notify that the object is added. The object must here be # acquisition wrapped or the indexing will fail. notify(ObjectAddedEvent(comment.__of__(self), self, id)) @@ -122,27 +148,63 @@ class Conversation(Persistent, Explicit): # Dict API - def __getitem__(self, key): - return self._comments[key] + def __len__(self): + return len(self._comments) + + def __contains__(self, key): + return long(key) in self._comments + + # TODO: Should __getitem__, get, __iter__, values(), items() and iter* return aq-wrapped comments? + + def __getitem__(self, key): + """Get an item by its long key + """ + return self._comments[long(key)] - def __setitem__(self, key, value): - # XXX Check that it implements the commenting interface - if value.comment_id in self._comments: - raise ValueError("Can not replace an existing comment") - # Note that we ignore the key completely: - self.addComment(comment) - def __delitem__(self, key): - # TODO unindex everything - return self._comments.remove(key) + """Delete an item by its long key + """ + + key = long(key) + + comment = self[key] + commentator = comment.author_username + + notify(ObjectWillBeRemovedEvent(comment, self, key)) + self._comments.remove(key) + notify(ObjectRemovedEvent(comment, self, key)) + + if commentator and commentator in self._commentators: + if self._commentators[commentator] <= 1: + del self._commentators[commentator] + else: + self._commentators[commentator] -= 1 + + notify(ContainerModifiedEvent(self)) + + def __iter__(self): + return iter(self._comments) + + def get(self, key, default=None): + return self._comments.get(long(key), default) def keys(self): return self._comments.keys() - def getPhysicalPath(self): - return self.aq_parent.getPhysicalPath() + (self.id,) + def items(self): + return self._comments.items() - # TODO: Update internal data structures when items added or removed + def values(self): + return self._comments.values() + + def iterkeys(self): + return self._comments.iterkeys() + + def itervalues(self): + return self._comments.itervalues() + + def iteritems(self): + return self._comments.iteritems() @implementer(IConversation) @adapter(IAnnotatable) @@ -155,7 +217,6 @@ def conversationAdapterFactory(content): conversation._parent_uid = content.UID() annotions[ANNO_KEY] = conversation conversation = annotions[ANNO_KEY] - # Probably this needs an acquisition wrapper return conversation class ConversationReplies(object): @@ -165,14 +226,70 @@ class ConversationReplies(object): """ implements(IReplies) - adapts(Conversation) + adapts(Conversation) # relies on implementation details def __init__(self, context): self.conversation = context - self.root = 0 + self.children = self.conversation._children.get(0, LLSet()) - # TODO: dict interface - generalise to work with any starting point, so - # that the subclassing below works + def addComment(self, comment): + comment.in_reply_to = None + return self.conversation.addComment(comment) + + # Dict API + + def __len__(self): + return len(self.children) + + def __contains__(self, key): + return long(key) in self.children + + # TODO: Should __getitem__, get, __iter__, values(), items() and iter* return aq-wrapped comments? + + def __getitem__(self, key): + """Get an item by its long key + """ + key = long(key) + if key not in self.children: + raise KeyError(key) + return self.conversation[key] + + def __delitem__(self, key): + """Delete an item by its long key + """ + key = long(key) + if key not in self.children: + raise KeyError(key) + del self.conversation[key] + + def __iter__(self): + return iter(self.children) + + def get(self, key, default=None): + key = long(key) + if key not in self.children: + return default + return self.conversation.get(key) + + def keys(self): + return self.children + + def items(self): + return [(k, self.conversation[k]) for k in self.children] + + def values(self): + return [self.conversation[k] for k in self.children] + + def iterkeys(self): + return iter(self.children) + + def itervalues(self): + for key in self.children: + yield self.conversation[key] + + def iteritems(self): + for key in self.children: + yield (key, self.conversation[key],) class CommentReplies(ConversationReplies): """An IReplies adapter for comments. @@ -184,5 +301,12 @@ class CommentReplies(ConversationReplies): adapts(IComment) def __init__(self, context): - self.conversation = context.__parent__ - self.root = context.comment_id + self.comment = context + self.comment_id = context.comment_id + self.children = self.conversation._children.get(0, LLSet()) + + def addComment(self, comment): + comment.in_reply_to = self.comment_id + return self.conversation.addComment(comment) + + # Dict API is inherited diff --git a/plone/app/discussion/interfaces.py b/plone/app/discussion/interfaces.py index 3009f91..d2bc859 100644 --- a/plone/app/discussion/interfaces.py +++ b/plone/app/discussion/interfaces.py @@ -1,5 +1,5 @@ from zope.interface import Interface -from zope.interface.common.mapping import IIterableMapping, IWriteMapping +from zope.interface.common.mapping import IIterableMapping from zope import schema from zope.i18nmessageid import MessageFactory @@ -22,13 +22,16 @@ class IDiscussionSettings(Interface): "the standard search tools."), default=True) -class IConversation(IIterableMapping, IWriteMapping): +class IConversation(IIterableMapping): """A conversation about a content object. This is a persistent object in its own right and manages all comments. The dict interface allows access to all comments. They are stored by - integer key, in the order they were added. + long integer key, in the order they were added. + + Note that __setitem__() is not supported - use addComment() instead. + However, comments can be deleted using __delitem__(). To get replies at the top level, adapt the conversation to IReplies. @@ -46,6 +49,16 @@ class IConversation(IIterableMapping, IWriteMapping): last_comment_date = schema.Date(title=_(u"Date of the most recent comment"), readonly=True) commentators = schema.Set(title=_(u"The set of unique commentators (usernames)"), readonly=True) + def __delitem__(key): + """Delete the comment with the given key. The key is a long id. + """ + + def addComment(comment): + """Adds a new comment to the list of comments, and returns the + comment id that was assigned. The comment_id property on the comment + will be set accordingly. + """ + def getComments(start=0, size=None): """Return a batch of comment objects for rendering. The 'start' parameter is the id of the comment from which to start the batch. @@ -73,19 +86,24 @@ class IConversation(IIterableMapping, IWriteMapping): in order to give enough context to show the full lineage of the starting comment. """ - - def addComment(comment): - """Adds a new comment to the list of comments, and returns the - comment id that was assigned. - """ -class IReplies(IIterableMapping, IWriteMapping): +class IReplies(IIterableMapping): """A set of related comments in reply to a given content object or another comment. Adapt a conversation or another comment to this interface to obtain the direct replies. """ + + def addComment(comment): + """Adds a new comment as a child of this comment, and returns the + comment id that was assigned. The comment_id property on the comment + will be set accordingly. + """ + + def __delitem__(key): + """Delete the comment with the given key. The key is a long id. + """ class IComment(Interface): """A comment. @@ -120,16 +138,30 @@ class IComment(Interface): class ICommentingTool(Interface): """A tool that indexes all comments for usage by the management interface. - This was the management interface can still work even though we don't + This means the management interface can still work even though we don't index the comments in portal_catalog. + + The default implementation of this interface simply defers to + portal_catalog, but a custom version of the tool can be used to provide + an alternate indexing mechanism. """ - def index(comment): - """Indexes a comment""" + def indexObject(comment): + """Indexes a comment + """ - def unindex(comment): - """Removes a comment from the indexes""" + def reindexObject(comment): + """Reindex a comment + """ - def search(username=None, wfstate=None): - """Get all comments with a certain username of wfstate""" - \ No newline at end of file + def unindexObject(comment): + """Removes a comment from the indexes + """ + + def uniqueValuesFor(name): + """Get unique values for FieldIndex name + """ + + def searchResults(REQUEST=None, **kw): + """Perform a search over all indexed comments. + """ diff --git a/plone/app/discussion/profiles/default/metadata.xml b/plone/app/discussion/profiles/default/metadata.xml index bb6eb60..6d5e9a9 100644 --- a/plone/app/discussion/profiles/default/metadata.xml +++ b/plone/app/discussion/profiles/default/metadata.xml @@ -1,3 +1,3 @@ - 1.0a1 + 1 \ No newline at end of file diff --git a/plone/app/discussion/tests/base.py b/plone/app/discussion/tests/base.py deleted file mode 100644 index 98a2c0c..0000000 --- a/plone/app/discussion/tests/base.py +++ /dev/null @@ -1,29 +0,0 @@ -import unittest - -from zope.testing import doctestunit -from zope.component import testing, getMultiAdapter -from zope.publisher.browser import TestRequest -from zope.publisher.interfaces.browser import IBrowserView -from Testing import ZopeTestCase as ztc - -from Products.Five import zcml -from Products.Five import fiveconfigure -from Products.PloneTestCase import PloneTestCase as ptc -from Products.PloneTestCase.layer import PloneSite -ptc.setupPloneSite(extension_profiles=['plone.app.discussion:default']) - -import plone.app.discussion - -class TestCase(ptc.PloneTestCase): - class layer(PloneSite): - @classmethod - def setUp(cls): - fiveconfigure.debug_mode = True - zcml.load_config('configure.zcml', - plone.app.discussion) - fiveconfigure.debug_mode = False - - @classmethod - def tearDown(cls): - pass - diff --git a/plone/app/discussion/tests/layer.py b/plone/app/discussion/tests/layer.py new file mode 100644 index 0000000..2670b7f --- /dev/null +++ b/plone/app/discussion/tests/layer.py @@ -0,0 +1,12 @@ +from Products.PloneTestCase import ptc +import collective.testcaselayer.ptc + +ptc.setupPloneSite() + +class Layer(collective.testcaselayer.ptc.BasePTCLayer): + """Install collective.flowplayer""" + + def afterSetUp(self): + self.addProfile('plone.app.discussion:default') + +DiscussionLayer = Layer([collective.testcaselayer.ptc.ptc_layer]) \ No newline at end of file diff --git a/plone/app/discussion/tests/test_api.py b/plone/app/discussion/tests/test_api.py index dc2642c..984af14 100644 --- a/plone/app/discussion/tests/test_api.py +++ b/plone/app/discussion/tests/test_api.py @@ -1,23 +1,17 @@ import unittest from datetime import datetime, timedelta -from base import TestCase -from zope.testing import doctestunit -from zope.component import testing, getMultiAdapter -from zope.publisher.browser import TestRequest -from zope.publisher.interfaces.browser import IBrowserView -from Testing import ZopeTestCase as ztc - -from Products.Five import zcml -from Products.Five import fiveconfigure -from Products.PloneTestCase import PloneTestCase as ptc -from Products.PloneTestCase.layer import PloneSite +from Products.PloneTestCase.ptc import PloneTestCase +from plone.app.discussion.tests.layer import DiscussionLayer from plone.app.discussion.conversation import Conversation from plone.app.discussion.comment import Comment from plone.app.discussion.interfaces import ICommentingTool, IConversation -class ConversationTest(TestCase): +class ConversationTest(PloneTestCase): + + layer = DiscussionLayer + def afterSetUp(self): # XXX If we make this a layer, it only get run once... # First we need to create some content. @@ -48,9 +42,4 @@ class ConversationTest(TestCase): self.assert_(conversation.last_comment_date - datetime.now() < timedelta(seconds=1)) def test_suite(): - return unittest.TestSuite([ - unittest.makeSuite(ConversationTest), - ]) - -if __name__ == '__main__': - unittest.main(defaultTest='test_suite') + return unittest.defaultTestLoader.loadTestsFromName(__name__) \ No newline at end of file diff --git a/plone/app/discussion/tests/test_tool.py b/plone/app/discussion/tests/test_tool.py index 7c7126f..619a6e8 100644 --- a/plone/app/discussion/tests/test_tool.py +++ b/plone/app/discussion/tests/test_tool.py @@ -1,23 +1,17 @@ import unittest -from datetime import datetime, timedelta -from base import TestCase -from zope.testing import doctestunit -from zope.component import testing, getMultiAdapter, getUtility -from zope.publisher.browser import TestRequest -from zope.publisher.interfaces.browser import IBrowserView -from Testing import ZopeTestCase as ztc +from zope.component import getUtility -from Products.Five import zcml -from Products.Five import fiveconfigure -from Products.PloneTestCase import PloneTestCase as ptc -from Products.PloneTestCase.layer import PloneSite +from Products.PloneTestCase.ptc import PloneTestCase +from plone.app.discussion.tests.layer import DiscussionLayer -from plone.app.discussion.conversation import Conversation from plone.app.discussion.comment import Comment from plone.app.discussion.interfaces import ICommentingTool, IConversation -class ToolTest(TestCase): +class ToolTest(PloneTestCase): + + layer = DiscussionLayer + def afterSetUp(self): # XXX If we make this a layer, it only get run once... # First we need to create some content. @@ -46,12 +40,6 @@ class ToolTest(TestCase): self.assert_(len(comment) == 1, "There is only one comment, but we got" " %s results in the search" % len(comment)) self.assertEquals(comment[0].Title, 'Comment 1') - def test_suite(): - return unittest.TestSuite([ - unittest.makeSuite(ToolTest), - ]) - -if __name__ == '__main__': - unittest.main(defaultTest='test_suite') + return unittest.defaultTestLoader.loadTestsFromName(__name__) \ No newline at end of file diff --git a/plone/app/discussion/tool.py b/plone/app/discussion/tool.py index c37a011..fb55bf1 100644 --- a/plone/app/discussion/tool.py +++ b/plone/app/discussion/tool.py @@ -1,9 +1,14 @@ -import time +"""The portal_discussion tool, usually accessed via +getUtility(ICommentingTool). The default implementation delegates to the +standard portal_catalog for indexing comments. + +BBB support for the old portal_discussion is provided in the bbb package. +""" + from zope import interface from zope.component import getUtility from interfaces import ICommentingTool, IComment -# The commenting tool, which is a local utility from Products.CMFCore.utils import UniqueObject, getToolByName from OFS.SimpleItem import SimpleItem @@ -41,12 +46,14 @@ class CommentingTool(UniqueObject, SimpleItem): """ catalog = getToolByName(self, 'portal_catalog') object_provides = [IComment.__identifier__] + if 'object_provides' in kw: kw_provides = kw['object_provides'] if isinstance(str, kw_provides): object_provides.append(kw_provides) else: object_provides.extend(kw_provides) + if REQUEST is not None and 'object_provides' in REQUEST.form: rq_provides = REQUEST.form['object_provides'] del REQUEST.form['object_provides'] @@ -54,6 +61,7 @@ class CommentingTool(UniqueObject, SimpleItem): object_provides.append(rq_provides) else: object_provides.extend(rq_provides) + kw['object_provides'] = object_provides return catalog.searchResults(REQUEST, **kw) diff --git a/setup.py b/setup.py index 8e57269..d2abc3b 100644 --- a/setup.py +++ b/setup.py @@ -26,8 +26,17 @@ setup(name='plone.app.discussion', install_requires=[ 'setuptools', 'collective.autopermission', + 'collective.testcaselayer', + 'plone.indexer', + 'ZODB3', + 'zope.interface', + 'zope.component', + 'zope.annotation', + 'zope.event', + 'zope.app.container', # XXX: eventually should change to zope.container ], entry_points=""" - # -*- Entry points: -*- + [z3c.autoinclude.plugin] + target = plone """, )