Code review comment for lp:~andrea.corbellini/launchpad/blueprint-comments-part2

Hi Andrea,

Thanks again for working on this. Your branch looks very good and is almost ready to land. There are a few things I'd like you to fix, and I also have a couple of questions, so I'm marking this Needs Fixing for now, but it should be very easy to fix.

> === modified file 'lib/lp/blueprints/configure.zcml'
> --- lib/lp/blueprints/configure.zcml 2009-07-13 18:15:02 +0000
> +++ lib/lp/blueprints/configure.zcml 2009-08-10 09:34:05 +0000
> @@ -219,6 +219,28 @@
> <allow
> interface="lp.blueprints.interfaces.specification.ISpecificationDelta"/>
> </class>
> +
> + <!-- SpecificationMessage -->
> +
> + <class
> +
> class="lp.blueprints.model.specificationmessage.SpecificationMessage">
> + <allow
> + interface="lp.blueprints.interfaces.specificationmessage.ISpe
> cificationMessage"/>
> + </class>
> +
> + <!-- SpecificationMessageSet -->
> +
> + <class
> +
> class="lp.blueprints.model.specificationmessage.SpecificationMessageSet">
> + <allow
> + interface="lp.blueprints.interfaces.specificationmessage.ISpe
> cificationMessageSet"/>
> + </class>
> + <securedutility
> +
> class="lp.blueprints.model.specificationmessage.SpecificationMessageSet"
> + provides="lp.blueprints.interfaces.specificationmessage.ISpecific
> ationMessageSet">
> + <allow
> + interface="lp.blueprints.interfaces.specificationmessage.ISpe
> cificationMessageSet"/>
> + </securedutility>
> </facet>
> <subscriber
> for="lp.blueprints.interfaces.specification.ISpecification
> lazr.lifecycle.interfaces.IObjectModifiedEvent"
>
> === added file 'lib/lp/blueprints/doc/specificationmessage.txt'
> --- lib/lp/blueprints/doc/specificationmessage.txt 1970-01-01 00:00:00
> +0000
> +++ lib/lp/blueprints/doc/specificationmessage.txt 2009-08-10 09:34:05
> +0000
> @@ -0,0 +1,38 @@
> += Specification Messages =
> +
> +Specification messages are messages associated with blueprints. A specifiction
> +message is described by the ISpecificationMessage interface.

Narrative in doctests should wrap around 72 characters.

> +
> +One IMessage can be associated with many ISpecification, but one
> +ISpecificationMessage is always associated with exactly one specification.

That's techincally true, but I don't think very relevant. You will never want
to do that, so I think this bit can be removed.

> +
> + >>> from lp.blueprints.interfaces.specification import ISpecificationSet
> + >>> from lp.blueprints.interfaces.specificationmessage import (
> + ... ISpecificationMessageSet)
> + >>> specset = getUtility(ISpecificationSet)
> + >>> specmessageset = getUtility(ISpecificationMessageSet)

It's a bit odd to do this assignment at the end of a test (though I understand
why you did it). I think it's better to merge the code part of this section
with the next one.

> +

Please add an extra blank line between sections (so there are two blank lines).

> +== Creating specification messages ==
> +
> +To create a specification message, use
> ISpecificationMessageSet.createMessage:
> +
> + >>> from canonical.launchpad.interfaces import IPersonSet
> + >>> sample_person = getUtility(IPersonSet).get(12)
> + >>> spec = specset.getByURL('http://developer.mozilla.org/en/docs/SVG')
> + >>> test_message = specmessageset.createMessage(
> + ... subject="test message subject",
> + ... content="text message content",
> + ... owner=sample_person,
> + ... spec=spec)
> + >>> test_message.message.subject
> + u'test message subject'
> +

Extra blank line.

> +== Retrieving specification messages ==
> +
> +ISpecificationMessageSet represents the set of all ISpecificationMessages in
> +the system. An individual ISpecificationMessage can be retrieved with
> +ISpecificationMessageSet.get:
> +
> + >>> specmessage_one = specmessageset.get(1)
> + >>> specmessage_one.message.subject
> + u'test message subject'
>
> === added file 'lib/lp/blueprints/interfaces/specificationmessage.py'
> --- lib/lp/blueprints/interfaces/specificationmessage.py 1970-01-01
> 00:00:00 +0000
> +++ lib/lp/blueprints/interfaces/specificationmessage.py 2009-08-10
> 09:34:05 +0000
> @@ -0,0 +1,58 @@
> +# Copyright 2009 Canonical Ltd. This software is licensed under the
> +# GNU Affero General Public License version 3 (see the file LICENSE).
> +
> +# pylint: disable-msg=E0211,E0213

Why are you disabling these lint messages?

> +
> +"""Specification message interfaces."""
> +
> +__metaclass__ = type
> +__all__ = [
> + 'ISpecificationMessage',
> + 'ISpecificationMessageSet',
> + ]
> +
> +from zope.interface import Attribute, Interface
> +from zope.schema import Bool, Bytes, Int, Object, Text, TextLine

You are importing things you don't need. You should always run `make lint` to
get help finding things like this.

> +
> +from canonical.launchpad.fields import Title
> +from lp.blueprints.interfaces.specification import ISpecification
> +from canonical.launchpad.interfaces.message import IMessage
> +from canonical.launchpad.validators.attachment import (
> + attachment_size_constraint)
> +
> +
> +class ISpecificationMessage(Interface):
> + """A link between a specification and a message."""
> +
> + specification = Object(schema=ISpecification, title=u"The specification.")

It would be better to use a Reference field (which you can, later on, expose
via the web service API.

> + messageID = Int(title=u"The message id.", readonly=True)

Why is this attribute needed? We only very rarely need to work with IDs. Normally
Storm should take care of references for us.

> + message = Object(schema=IMessage, title=u"The message.")

Likewise, a Reference field will be a better choice.

> + visible = Bool(title=u"This message is visible or not.", required=False,
> + default=True)

I think "Is this message visible?" would be a better title.

> +
> +
> +class ISpecificationMessageSet(Interface):
> + """The set of all ISpecificationMessages."""
> +
> + def createMessage(subject, specification, owner, content=None):
> + """Create an ISpecificationMessage.
> +
> + title -- a string
> + specification -- an ISpecification
> + owner -- an IPerson
> + content -- a string
> +
> + The created message will have the specification's initial message as
> + its parent.
> +
> + Returns the created ISpecificationMessage.
> + """
> +
> + def get(specificationmessageid):
> + """Retrieve an ISpecificationMessage by its ID."""
> +
> + def getBySpecificationAndMessage(specification, message):
> + """Return the corresponding ISpecificationMesssage.
> +
> + Return None if no such ISpecificationMesssage exists.
> + """
>
> === added file 'lib/lp/blueprints/model/specificationmessage.py'
> --- lib/lp/blueprints/model/specificationmessage.py 1970-01-01 00:00:00
> +0000
> +++ lib/lp/blueprints/model/specificationmessage.py 2009-08-10 09:34:05
> +0000
> @@ -0,0 +1,60 @@
> +# Copyright 2009 Canonical Ltd. This software is licensed under the
> +# GNU Affero General Public License version 3 (see the file LICENSE).
> +
> +# pylint: disable-msg=E0611,W0212
> +
> +__metaclass__ = type
> +__all__ = [
> + 'SpecificationMessage',
> + 'SpecificationMessageSet'
> + ]
> +
> +from email.Utils import make_msgid
> +
> +from zope.interface import implements
> +
> +from sqlobject import BoolCol, ForeignKey, StringCol
> +from storm.store import Store
> +
> +from canonical.database.sqlbase import SQLBase, sqlvalues
> +from lp.blueprints.interfaces.specificationmessage import (
> + ISpecificationMessage, ISpecificationMessageSet)
> +from canonical.launchpad.database.message import Message, MessageChunk
> +
> +
> +class SpecificationMessage(SQLBase):
> + """A table linking specifictions and messages."""
> +
> + implements(ISpecificationMessage)
> +
> + _table = 'SpecificationMessage'
> +
> + specification = ForeignKey(
> + dbName='specification', foreignKey='Specification', notNull=True)
> + message = ForeignKey(dbName='message', foreignKey='Message', notNull=True)
> + visible = BoolCol(notNull=True, default=True)
> +
> +
> +class SpecificationMessageSet:
> + """See ISpecificationMessageSet."""
> +
> + implements(ISpecificationMessageSet)
> +
> + def createMessage(self, subject, spec, owner, content=None):
> + """See ISpecificationMessageSet."""
> + msg = Message(
> + owner=owner, rfc822msgid=make_msgid('blueprint'), subject=subject)
> + chunk = MessageChunk(message=msg, content=content, sequence=1)
> + specmsg = SpecificationMessage(specification=spec, message=msg)
> +
> + Store.of(specmsg).flush()

Why is it necessary to flush here?

> + return specmsg
> +
> + def get(self, specmessageid):
> + """See ISpecificationMessageSet."""
> + return SpecificationMessage.get(specmessageid)
> +
> + def getBySpecificationAndMessage(self, spec, message):
> + """See ISpecificationMessageSet."""
> + return SpecificationMessage.selectOneBy(
> + specification=spec, message=message)