Merge lp:~andrea.corbellini/launchpad/blueprint-comments-part3 into lp:launchpad

[Andrea Corbellini]

[Andrea Corbellini (andrea.corbellini)]

This branch contains the model work needed for blueprint comments.

* I created two new interfaces: ISpecificationComment and ISpecificationMessageAddForm. Currently they are not used but will be needed for displaying in the web UI.
* I also added some new attributes to ISpecification: messages, newMessage, linkMessage, getMessageChunks.
* Finally I added tests at the end of lib/lp/blueprints/doc/specification.txt.

To test the changes: bin/test -t specification.txt

[Eleanor Berger (intellectronica)]

Andrea, thanks again for taking on this work. And well done, the branch looks in great shape. We can't land it just yet, since the tree is closed, but I'm marking it approved and you can continue with the UI branch building on top of this one.

review: Approve

[lp:~andrea.corbellini/launchpad/blueprint-comments-part3]

10065. By Andrea Corbellini on 2010-01-29

Merge with devel.

10066. By Andrea Corbellini on 2010-01-29

Allow missing attributes.

10067. By Andrea Corbellini on 2010-02-16

Merge with devel.

[Martin Pool (mbp)]

Can we land this now?

[Andrea Corbellini (andrea.corbellini)]

Unfortunately no, sorry. The PQM said some tests failed. I'm almost sure the problem is caused by the fact that some attributes are missing from the <allow> directives in the configure.zcml file.

I'd fix the errors, but unfortunately in this period I don't have the time to run the full test suite.

[Brad Crittenden (bac)]

I am changing the status of this branch to Work in progress as it will now need significant work before landing and will need a re-review. Andrea I hope you can pick this work up again when you have time.

review: Needs Resubmitting (code)

[Robert Collins (lifeless)]

It may not need significant work.

Running it through ec2 once will return a subunit file listing all of
the failing tests, allowing it to be worked on much more simply than
by running the full test suite.

Unmerged revisions

10067. By Andrea Corbellini on 2010-02-16

Merge with devel.

10066. By Andrea Corbellini on 2010-01-29

Allow missing attributes.

10065. By Andrea Corbellini on 2010-01-29

Merge with devel.

10064. By Andrea Corbellini on 2009-12-17

Lint code.

10063. By Andrea Corbellini on 2009-12-17

Adjust permissions.

10062. By Andrea Corbellini on 2009-12-17

Add tests.

10061. By Andrea Corbellini on 2009-12-17

Add the missing imports.

10060. By Andrea Corbellini on 2009-12-16

Add the code for the new attributes and methods.

10059. By Andrea Corbellini on 2009-12-16

Set permissions for the new methods.

10058. By Andrea Corbellini on 2009-12-16

Add `linkMessage()` to ISpecification.

=== modified file 'lib/lp/blueprints/configure.zcml'
--- lib/lp/blueprints/configure.zcml	2009-09-23 03:36:55 +0000
+++ lib/lp/blueprints/configure.zcml	2010-02-16 12:43:20 +0000
@@ -157,7 +157,84 @@
         <class
             class="lp.blueprints.model.specification.Specification">
             <allow
-                interface="lp.blueprints.interfaces.specification.ISpecification"/>
+                attributes="
+                    linkBranch
+                    unlinkBranch
+                    unqueue
+                    getBranchLink
+                    messages
+                    all_blocked
+                    goal_proposer
+                    getSubscriptionByName
+                    owner
+                    linked_branches
+                    productseries
+                    getFeedbackRequests
+                    id
+                    createDependency
+                    sprints
+                    queue
+                    man_days
+                    removeDependency
+                    goal
+                    linkMessage
+                    date_goal_proposed
+                    priority
+                    date_goal_decided
+                    declineBy
+                    date_started
+                    has_accepted_goal
+                    goalstatus
+                    is_started
+                    getSprintSpecification
+                    product
+                    getDelta
+                    subscriptions
+                    sprint_links
+                    feedbackrequests
+                    retarget
+                    unlinkSprint
+                    dependencies
+                    unsubscribe
+                    subscribers
+                    milestone
+                    linkSprint
+                    updateLifecycleStatus
+                    is_blocked
+                    completer
+                    is_incomplete
+                    starter
+                    subscription
+                    notificationRecipientAddresses
+                    is_complete
+                    date_completed
+                    direction_approved
+                    subscribe
+                    isSubscribed
+                    superseded_by
+                    distroseries
+                    whiteboard
+                    proposeGoal
+                    datecreated
+                    blocked_specs
+                    all_deps
+                    distribution
+                    getMessageChunks
+                    informational
+                    goal_decider
+                    implementation_status
+                    acceptBy
+                    name
+                    title
+                    approver
+                    definition_status
+                    summary
+                    assignee
+                    specurl
+                    drafter
+                    target
+                    specification_branch
+                    mentoring_offers"/>
 
             <!-- We allow any authenticated person to update the whiteboard -->
 
@@ -185,6 +262,14 @@
                 attributes="
                     linkBug
                     unlinkBug"/>
+            <require
+                permission="launchpad.Edit"
+                attributes="
+                    newMessage"/>
+            <require
+                permission="launchpad.Admin"
+                attributes="
+                    setCommentVisibility"/>
         </class>
         <class
             class="lp.blueprints.model.specificationbug.SpecificationBug">
@@ -232,6 +317,8 @@
             class="lp.blueprints.model.specificationmessage.SpecificationMessage">
             <allow
                 interface="lp.blueprints.interfaces.specificationmessage.ISpecificationMessage"/>
+            <require permission="launchpad.Admin"
+                set_attributes="visible"/>
         </class>
 
         <!-- SpecificationMessageSet -->

=== modified file 'lib/lp/blueprints/doc/specification.txt'
--- lib/lp/blueprints/doc/specification.txt	2009-08-13 19:03:36 +0000
+++ lib/lp/blueprints/doc/specification.txt	2010-02-16 12:43:20 +0000
@@ -531,3 +531,52 @@
  False
  >>> canvas.date_completed is not None
  False
+
+
+== Specification comments ==
+
+A comment can be added to a specification using the `newMessage()` method:
+
+    >>> spec = specset.new(
+    ...     'spec-for-comments', 'Spec for comments',
+    ...     'http://www.example.com/SpecForComments',
+    ...     'Specification used to test comments',
+    ...     SpecificationDefinitionStatus.APPROVED, mark,
+    ...     distribution=ubuntu)
+    >>> message = spec.newMessage(mark, 'Subject', 'First comment.')
+
+All messages are stored in `messages`.
+
+    >>> message in spec.messages
+    True
+
+The message chunks associated to a blueprint can be obtained with the method
+`getMessageChunks()`.
+
+    >>> chunks = spec.getMessageChunks()
+    >>> len(chunks)
+    1
+    >>> chunk = chunks[0]
+    >>> chunk.message.subject
+    u'Subject'
+    >>> chunk.content
+    u'First comment.'
+
+All comments are visible by default, but they can be hidden by an
+administrator using `setCommentVisibility()`.
+
+    >>> from zope.component import getUtility
+    >>> from lp.blueprints.interfaces.specificationmessage import (
+    ...     ISpecificationMessageSet)
+    >>> message_set = getUtility(ISpecificationMessageSet)
+    >>> comment = message_set.getBySpecificationAndMessage(
+    ...     spec, spec.messages[0])
+
+    >>> comment.visible
+    True
+    >>> spec.setCommentVisibility(mark, 0, False)
+    >>> comment.visible
+    False
+
+    >>> check_permission('launchpad.Admin', spec.setCommentVisibility)
+    True

=== modified file 'lib/lp/blueprints/interfaces/specification.py'
--- lib/lp/blueprints/interfaces/specification.py	2009-07-17 00:26:05 +0000
+++ lib/lp/blueprints/interfaces/specification.py	2010-02-16 12:43:20 +0000
@@ -26,10 +26,7 @@
     ]
 
 
-from lazr.restful.declarations import (
-    REQUEST_USER, call_with, export_as_webservice_entry,
-    export_write_operation, operation_parameters, operation_returns_entry)
-from lazr.restful.fields import Reference
+from lazr.restful.declarations import export_as_webservice_entry
 from zope.interface import Interface, Attribute
 from zope.component import getUtility
 
@@ -40,7 +37,6 @@
     ContentNameField, PublicPersonChoice, Summary, Title)
 from canonical.launchpad.validators import LaunchpadValidationError
 from lp.registry.interfaces.role import IHasOwner
-from lp.code.interfaces.branch import IBranch
 from lp.code.interfaces.branchlink import IHasLinkedBranches
 from lp.registry.interfaces.mentoringoffer import ICanBeMentored
 from canonical.launchpad.interfaces.validation import valid_webref
@@ -879,6 +875,22 @@
     def getBranchLink(branch):
         """Return the SpecificationBranch link for the branch, or None."""
 
+    # comments
+    messages = Attribute("The specification messages related to this object.")
+
+    def newMessage(owner, subject, content):
+        """Create a new message and link it to this specification."""
+
+    def linkMessage(message, user):
+        """Link a message to this specification."""
+
+    def getMessageChunks():
+        """Return MessageChunks linked to comments made on this specification
+        """
+
+    def setCommentVisibility(user, comment_number, visible):
+        """Set the `visible` attribute on a specification comment."""
+
 
 # Interfaces for containers
 class ISpecificationSet(IHasSpecifications):

=== modified file 'lib/lp/blueprints/interfaces/specificationmessage.py'
--- lib/lp/blueprints/interfaces/specificationmessage.py	2009-08-10 12:34:20 +0000
+++ lib/lp/blueprints/interfaces/specificationmessage.py	2010-02-16 12:43:20 +0000
@@ -12,8 +12,9 @@
     ]
 
 from lazr.restful.fields import Reference
-from zope.interface import Interface
-from zope.schema import Bool, Int
+from zope.interface import Attribute, Interface
+from zope.schema import Bool, Int, Text
+from canonical.launchpad.fields import Title
 
 from lp.blueprints.interfaces.specification import ISpecification
 from canonical.launchpad.interfaces.message import IMessage
@@ -54,3 +55,27 @@
 
         Return None if no such ISpecificationMesssage exists.
         """
+
+
+class ISpecificationComment(IMessage):
+    """A specification comment for displaying in the web UI."""
+
+    specification = Attribute(u"The specification the comments belongs to.")
+    show_for_admin = Bool(
+        title=u"A hidden comment still displayed for admins.",
+        readonly=True)
+    index = Int(title=u"The comment number.", required=True, readonly=True)
+    was_truncated = Bool(
+        title=u"Whether the displayed text was truncated for display.",
+        readonly=True)
+    text_for_display = Text(
+        title=u"The comment text to be displayed in the UI.", readonly=True)
+    display_title = Attribute(u"Whether or not to show the title.")
+
+
+class ISpecificationMessageAddForm(Interface):
+    """Schema used to build the add form for specification comment.
+    """
+
+    subject = Title(title=u"Subject", required=True)
+    comment = Text(title=u"Comment", required=False)

=== modified file 'lib/lp/blueprints/model/specification.py'
--- lib/lp/blueprints/model/specification.py	2009-09-21 14:56:07 +0000
+++ lib/lp/blueprints/model/specification.py	2010-02-16 12:43:20 +0000
@@ -10,8 +10,11 @@
     'SpecificationSet',
     ]
 
+from email.Utils import make_msgid
+
 from storm.store import Store
 
+from zope.component import getUtility
 from zope.interface import implements
 from zope.event import notify
 
@@ -33,6 +36,7 @@
 from canonical.database.constants import DEFAULT, UTC_NOW
 from canonical.database.datetimecol import UtcDateTimeCol
 from canonical.database.enumcol import EnumCol
+from canonical.launchpad.database.message import Message, MessageChunk
 
 from canonical.launchpad.helpers import (
     get_contact_email_addresses, shortlist)
@@ -55,6 +59,10 @@
     SprintSpecification)
 from lp.blueprints.model.sprint import Sprint
 
+from lp.blueprints.model.specificationmessage import SpecificationMessage
+from lp.blueprints.interfaces.specificationmessage import (
+    ISpecificationMessageSet)
+
 from lazr.lifecycle.objectdelta import ObjectDelta
 from lp.blueprints.adapters import SpecificationDelta
 
@@ -652,6 +660,54 @@
         spec_branch = self.getBranchLink(branch)
         spec_branch.destroySelf()
 
+    # comments
+    messages = SQLRelatedJoin(
+        'Message', joinColumn='specification', otherColumn='message',
+        intermediateTable='SpecificationMessage', prejoins=['owner'],
+        orderBy=['datecreated', 'id'])
+
+    def newMessage(self, owner, subject=None, content=None):
+        """See `ISpecification`."""
+        if subject is None:
+            subject = self.followup_subject()
+        msg = Message(
+            owner=owner, subject=subject,
+            rfc822msgid=make_msgid('blueprints'))
+        MessageChunk(message=msg, content=content, sequence=1)
+        specmsg = self.linkMessage(msg)
+        if not specmsg:
+            return
+        notify(ObjectCreatedEvent(specmsg, user=owner))
+        return specmsg.message
+
+    def linkMessage(self, message, user=None):
+        """See `ISpecification`."""
+        if message not in self.messages:
+            if user is None:
+                user = message.owner
+            return SpecificationMessage(specification=self, message=message)
+
+    def getMessageChunks(self):
+        """See `ISpecification`."""
+        query = """
+            SpecificationMessage.specification = %s AND
+            Message.id = MessageChunk.message AND
+            SpecificationMessage.message = Message.id
+            """ % sqlvalues(self)
+        chunks = MessageChunk.select(
+            query, clauseTables=['SpecificationMessage', 'Message'],
+            prejoinClauseTables=['Message'],
+            orderBy=['Message.datecreated', 'Message.id',
+                     'MessageChunk.sequence'])
+        return list(chunks)
+
+    def setCommentVisibility(self, user, comment_number, visible):
+        """See `ISpecification`."""
+        message_set = getUtility(ISpecificationMessageSet)
+        message = message_set.getBySpecificationAndMessage(
+            self, self.messages[comment_number])
+        message.visible = visible
+
 
 class HasSpecificationsMixin:
     """A mixin class that implements many of the common shortcut properties