[Framework-Team] Re: Review for PLIP 179 - commenting

[Martin Aspeli]

Hi Kai,

> >   - user informationis duplicated in all comments
>
> Yes, that's true. A possible solution could be just to save the
> member id for a authenticated member, but then we should think about
> the case a member will be deleted.

This is true, but storing member id is probably sufficient. I'd
suggest that you store the user name, and look up full name etc
(mtool.getMemberInfo) - if possible. If that returns nothing, then
just display the user id as a fallback.

> >   - searching for comments by site member is not possible
>
> Yes. See above.

This would be quite good.

There ought to be some thought about how the easy commenting
interfaces coudl be adapted to the Ploneboard interfaces. One of the
goals of Ploneboard is to let you create some kind of "forum" space
where you can see threads based on the comments created around the
portal. If EC comments were searchable (i.e. you could find them using
a catalog search), we should be able to write adapters to
IConversation and IComment in Ploneboard (look at
http://svn.plone.org/svn/collective/Ploneboard/trunk/interfaces.py).

> >   IComment should probably be modified so
> >   it can deal with something that refers to actual users.
> >
> > - the interface does not allow for nested comments. Reusing the
> > Ploneboard
> >   interfaces would add the ability to support nested comments, as
> > well as fix
> >   the former issue.
>
> Yes. nested comments are on the top of my todo list.

You may be able to re-use the Ploneboard interfaces directly, or model
something closely on them. A lot of thought have gone into them. :)

> Generally, we should decide, whether a comment should be a AT or just
> a simpler datatype like a dict or somethink like that. I think former
> would bring a lot of advantages:
>
>         - easy realisation of nested comments
>         - easy using of workflows, hence easy realisation of moderation
>         - easier to search (?)
>         - using of default CRUD mechanism
>
> My first approach was realised with an AT, but then optilude remarks,
> that this could be a little bit overloaded (http://tinyurl.com/
> fckj5), so I tried an approach with dicts. But as I write these lines
> I would prefer to go back to AT. What do you think?

Neither. :)

They should be plain Python objects, or rather, Zope 3 content types.

class IComment(Interface):
   ...

class Comment(Persistent):
    implements(IComment)

I would suggest that you look at Ploneboard, where we have a
conversation that is a container for comments. In Ploneboard these are
AT objects, but you don't really need that. For example, you could:

 - Let a conversation be an ordered Zope 3 container
 - Store this container in the annotations of an object

So long as you use a Persistent object, you can manage it however you
like. You don't need a Plone UI or AT's base_edit or anything of the
sort for managing comments, so AT is overkill. With proper interfaces,
though, yuo have the option of using formlib for generating forms,
which again makes things so much easier.

You may be interested in looking at plone.contentrules and
plone.app.contentrules to see how I used annotations and containers
there. They are in http://svn.plone.org/svn/plone.

> > - there is a lot of HTML-specific information in
> > ICommentingOptions; it
> >   may be better to implement these using configurable filters.
> > Ploneboard
> >   already has a few implementations for these which can be reused.
>
> Okay, I will check this, too. But this could lead to a problem if one
> want to use different options on different objects or products which
> uses all plone.commenting. Or do I miss a point? With
> ICommentingOptions approach one could write own adapters, which would
> then use side wide options or own ones for every product or even object.
>
> > - the ICommentingOptions interface produced a number of user message;
> >   should not these be in the user interface itself? A status
> > message or
> >   text in the template are possible options.
>
> The initial idea was to provide a generic, easy to use component
> which could be easily reused in different contextes. So my thought
> was, that a user of the commenting framework is able to change
> message for a special "comment" type. For example:
>
>         - Comment
>         - Blog Entry
>         - Guestbook Entry
>         - Note
>         - ...
>
> And I don't know how this could be solved for example with the i18n-
> framework, if the component is used more the one times in different
> contextes in one Plone site. But there might be other, better ways,
> of course.
>
> > - the viewThreadsAtBottom template assumes that the user interface for
> >   all implementation is the same. This is not necessarily true and
> >   warrants a more flexible approach. Perhaps viewlets can be used
> > here?
>
> Yes, I see, as I have heard a talk about viewlets from Phillipp, just
> today :-).

We need a viewlet manager in that space, definitely, also for things
like ratings, imho.

> > Commenting implementation
> > =========================
> >
> > - there is a basic test suite; it should be expanded to at least cover
> >   all methods exposed through the defined interfaces
>
> In planning.
>
> > - comments are not included in the SearchableText index, so
> > searching for
> >   comments is not possible using the default plone search option.
> > If that
> >   is desirable or not will probably depend on the specific use-
> > case, so this
> >   is not necessarily a problem.
> >
> > - there is a bunch of (ArchgenXML generated?) template code
> > throughout the
> >   product that looks like it can safely be removed.
>
> Yes it can. I'll do that, as I've decided to remove ArchGenXML code
> from all of my products, recently.
>
> > - The code uses formlib directlry. This should be changed to use
> > plone.app.form
> >   instead: this allows you to write cleaner code (you can use
> > zope.formlib
> >   directly) and the UI will look much more plonish.
>
> Have to confess, that I never heard of it before, but I will dive in.

Again, the code in plone.app.contentrules may be useful to you. Note
that I don't think you really use plone.app.form, it just patches its
way into Zope to make regular formlib forms looke Plone-ish.

> > Small issues
> > ============
> >
> > While reviewing I ran into a few minor issues. These are certainly no
> > showstoppers for merging this bundle but should be fixed before a
> > final
> > release:
> >
> > - the toplevel __init__.py still contains a couple of debug statements
> >   (such as allow_module("pdb"))
> > - the descriptions for the fields in IConfigletFormFields are
> > incorrect
>
> Oha, that's true :-(
>
> > - The implementation in adapters.py has does not support translation
> >   of these messages.
>
> The support for translations is in the AddCommentForm at the moment.
> Do you consider this as wrong/bad style?
>
> > - the skin layer has a readme.txt and tool.gif which should not be
> > there
>
> All this issues are easily to fix.
>
> > Verdict
> > =======
> > When merging a product into plone core it should be a real in all
> > possible
> > ways. Currently easycommenting fails this in a few areas which need
> > to be
> > fixed:
> >
> > - core interfaces which can be used to hook into standard
> > functionality
> >   should be in the plone namespace
> > - configlets should be listed under plone configuration, not under
> > Add-On
> >   products
> >
> > The first one is a a definite showstopper and must be fixed before
> > this
> > bundle can be merged. Additionally I strongly feel the interfaces
> > need to be revisited. The interfaces from Ploneboard should be a good
> > basis here.
>
> Both could be easily fixed from my point of view.
>
> Hope that helps. Generally spoken, I'm keen to improve easycommenting
> or better plone.commenting to land it in Plone. I would propose that
> I start immediately with the following steps as these should be done
> without general decisions:
>
>         - Change namespace to plone.commenting
>         - Remove ArchGenXML code
>         - Fix bugs as described in small issues above
>         - Change to plone.app.form
>         - Adding more and better tests
>
> Then we need some decisions about the content type for comments and
> which approach we want to use for options and/or HTML handling.
> What do you mean?

I'm very keen on seeing this progress, too, and those steps look good.
I'm pretty sure that you need to manage minimal Persistent objects
with well-defined interfaces, and not dicts or AT objects, and that
you need to store things in annotations. It'd also be very helpful if
your interfaces were adaptable to Ploneboard's interfaces (you don't
need to write an adapter, but just think about whether anything in
your design would make that impossible).

HTH,
Martin