Comments module refactoring

Topics: Announcements, Core
Developer
May 17, 2012 at 10:07 AM
Edited Jun 4, 2012 at 12:21 PM

With nightwolf226 we currently start to refactor the Comments module. Currently we're narrowing down the aims (many listed in this discussion still stand).

  1. First of all comments should have dynamically built display and editor (also) so the type can be actually altered. Editor and display rendering is currently hard-coded.
  2. The usage of Projector queries could be considered instead of building the list of comments, but this option should be carefully examined.
  3. Integrating the new Antispam module.
  4. The comment body could be saved utilizing the new TextField field. This would help extensibility but migration from the current system (where the message is stored in a part's property) could be slightly complicated.
  5. Refactor so that the UserName and email will be only saved separately if the user is not logged in (the current schema would remain, just the fields would be empty if a logged in user has written the comment, then the users table would be joined).
  6. Fix that one can save a comment in someone other's name.
  7. Make comments searchable (this would be given if the message would be in a TextField).
  8. Possibly save the datetime of the last comment in CommmentPart (and so in CommentsPartRecord) so it can be used with Projector queries.
  9. CommentsService and its tests should be adapted if needed.

Because of the expensive shape building dynamic display building could be a performance issue (as discussed also on yesterday's Steering Committee Meeting), but this should be measured first. Anyway, paging with a reasonable amount default amount of items per page should prevent this to be noticeable.

Coordinator
May 17, 2012 at 6:26 PM

Could you envision to handle multi-threaded comments ?

I wouldn't recommend to use the TextField. If you do a module like comments, you should handle everything internally, no surprise.

If you use the MiniProfiler module, you can see the time spent in rendering shapes. It didn't look so long to me yesterday when I checked.

Developer
Jun 4, 2012 at 12:20 PM

Threaded comments would be cool, but it would widen the current scope very much and I don't think we could do it for 1.5. Let's see if we manage to complete the refactoring alone. I've opened an issue for it here to further elaborate the question.

I've crossed out TextField considering your arguments on the last Steering Committee Meeting. Migration would be messy anyway.

Jun 4, 2012 at 1:21 PM
Edited Jun 4, 2012 at 6:24 PM

You could always 'just' store the 'parent' comment already and not do anything with it by default.

Then using alternates people could 'unlock' the threading capabilities.

Just an idea ;)

No idea what went wrong with my sentence... :/

Developer
Jun 4, 2012 at 3:27 PM

Would you finish the sentence? :-)

Coordinator
Jun 4, 2012 at 7:56 PM

True, we could have the column in there and not do anything with it.

Jun 4, 2012 at 9:34 PM

i think the first step is to release simpler new version, because there is still active issue for anon post in the 1.42.

Piedone, could you release it in mid of this month as you expected ago?

thanks

andy

Developer
Jun 6, 2012 at 10:49 AM

After the core aims are done we'll start looking at the threading feature.

infofromca: We said we'll _start_ working on this in mid-June. This is still the expected date as we currently have other duties on the university, like not failing examinations :-).

Developer
Jun 24, 2012 at 8:04 AM

We have moved to another fork to avoid any conflicts with the latest source.

Developer
Jul 1, 2012 at 9:30 PM

See this discussion about the concept of an AnonymOwnerPart.

Developer
Jul 3, 2012 at 8:32 PM

Now the refactoring is at a state that only editor and display shapes are used. But there is the question of how the comment should be saved.

With the current implementation the comment is posted to a controller, where it's saved; if there are validation errors, those are added to the notifier and the filled out data is sent back to the form through TempData; lastly the action redirects to where the comment was posted from.

The usage of TempData with a type that can be altered dynamically is problematic. After UpdateEditor is called on a new Comment content item to fill parts with POST data, the resulting editor shape (that is populated by the already filled out data by ContentManager) should somehow be swapped with the default, empty editor shape of the comment.

Now this is normally done by not doing a redirect on validation errors, but returning the editor shape (ShapeResult) from the action, thus the user will see the populated form with the corresponding validation messages. I think this can't be done here as with this technique here the whole content item's display containing the comments should be built.

Another approach would be the following:

  1. Post the comment form exclusively through AJAX to CommentsController.
  2. There:
    • If there's no validation error, return an empty OK response (maybe instead of status code 200, 201, 202 or 204 could be used?)
    • If there is one, return the editor shape in a ShapeResult (which would mean the full html of the form, together with validation messages and populated data)
  3. Depending on the response (here a distinction of the empty result with a different status code would come handy) in the JS waiting for the AJAX response, do:
    • if there was no validation error, do a reload
    • if there was one, swap the editor form to the one got from the response.

Downsides: would not work without JS. Although who cares? :-)

Anybody with a better idea?

Developer
Jul 4, 2012 at 10:57 AM

Is there actually a need for ICommentValidator and AkismetCommentValidator after the integration of the Antispam module?

Coordinator
Jul 4, 2012 at 8:31 PM

Akismet should be out, and I would say commentvalidator as well. Sebastien?

I'm really not sure about the Ajax thing. It's probably fine, and may even do a little spam fighting of its own, but I don't like us departing from the super-simple model (from a user's pov) we had before.

Developer
Jul 4, 2012 at 11:18 PM

It should work without JavaScript. Can we apply Ajax once the non JavaScript version is complete? 

Coordinator
Jul 5, 2012 at 2:47 AM

Well, Disqus doesn't work without JavaScript if I'm not mistaken, and it doesn't bother anyone except maybe for spammers.

Coordinator
Jul 5, 2012 at 5:39 PM

Disqus doesn't bother anyone, they just tell people who care about it not to use it. Actually I'd rather have a  javascript-less implementation.

You can remove anything related to validation. Just ensure that associating a SpamPart or a reCaptchaPart to the comment will work.

Developer
Jul 11, 2012 at 11:10 PM
Edited Jul 11, 2012 at 11:11 PM

Okay, then that validator will go. Didn't like it anyway :-).

The issue is that I can't think of anything else besides the AJAX solution outlined above that would work well, i.e.: post comment -> tryupdate -> refresh if valid, present editor filled with typed-in data otherwise. Here it is needed that after the new comment content item is updated with IContentManager.UpdateEditor(), the resulting shape (what the method returns) is displayed to the user if validation fails (again meaning TryUpdate).

Now in the current implementation TempData is used to transfer individual field contents from the action where the comment is posted to to the page it's redirected back. This approach can't work if the content type is dynamic (as it should be), but only with fixed parts.

I've thought about passing the whole editor shape around in TempData, but I don't like this approach, the AJAX one seems cleaner.

Developer
Jul 14, 2012 at 8:46 PM

Now the refactored module is more or less working at a basic level (see fork). If the comment is invalid, the editor shape is passed back through TempData.

I have two new questions:

  1. Currently an e-mail address is not required for a comment, but if one is present, it's validated. Should this stay, or should it be required too?
  2. I asked this formerly, but again CommentContainerPart confuses me. Is it needed? Isn't it partially redundant with CommentsPart?

Please consider the AnonymOwnerPart idea again; we're not in a hurry with Comments, it most possibly coming for 1.6, so this new feature could well be included too. How corresponding users are stored for comments should be refactored anyway. It's hurting for me that this feature is well implemented in Comments but could have a good use elsewhere too :-).

I've created a wiki page with an up-to-date list of remaining tasks, in order of precedence.

Please comment.

Developer
Jul 20, 2012 at 9:26 AM

Sebastien, I'm looking at you, please reply!

Coordinator
Jul 20, 2012 at 6:19 PM

Sorry Zoltan, I am heads down other things right now and need to focus seriously on this one, would waste your time if I just spent 5 minutes on this one. Will do, promised, but not yet. (1.5.1, harvest, ...)

Developer
Jul 20, 2012 at 6:25 PM

OK, sure, thanks!

Developer
Jul 31, 2012 at 1:09 PM

In the fork, the latest modification makes comments searchable by their CommentText property. When searching comments the metadata showing the creation date is a link to the comment, but since this link is generated by the current route, this link points to the search page (unlike when we are looking at the comments of for example a blog entry). Should we redesign CommentPart to be able to display the link correctly (pointing to the comment on the page of the corresponding commented contentitem)?

Coordinator
Jul 31, 2012 at 4:23 PM

I'd say yes.

Developer
Jul 31, 2012 at 7:40 PM

We need to have some way to prevent a Get() for each comment though. AFAIK currently how such "joins" are dealt with (i.e. if there's a record "stored" in another record) is that if there's a corresponding content item (like with CommonPart.Container) that item is fetched lazily through ContentManager. Now using this technique for Comments would most possibly present an unnecessary performance hit (fetching the corresponding content item for each comment, every time).

Coordinator
Aug 2, 2012 at 4:30 PM

Not true if I understand correctly, you can ask the ContentManager to preload them. Look at how it's done for tags for example.

Developer
Aug 7, 2012 at 1:31 PM

If with QueryHints it's possible to also expand records with such properties then it's good, although it would still have the same problem when searching for comments: the search controller can't expand records without knowing about them if I know correctly.

Aug 7, 2012 at 1:32 PM

Is it possible to tell the ContentManager to preload 'everything' of a certain item?

Developer
Aug 7, 2012 at 2:45 PM

Not that I know of. With the addition of this ContentQuery feature it would be possible to query content items with a set of ids, which in turn could make possible the usage of the WithQueryHintsFor() query methods. Now there are two problems with that:

  1. This method still need the content type names in order to work (what is possible when fetching comments in the Comments module, as we know that the content type is "Comment", but not possible in the Search module).
  2. It needs a query object that ha type arguments for a part and a record.
Developer
Aug 14, 2012 at 11:03 PM

Please take a look at the questions currently open for tomorrow's meeting:

  1. Currently an e-mail address is not required for a comment, but if one is present, it's validated. Should this stay, or should it be required too?
  2. I asked this formerly, but again CommentContainerPart confuses me. Is it needed? Isn't it partially redundant with CommentsPart?

Please consider the AnonymOwnerPart idea again; we're not in a hurry with Comments, it most possibly coming for 1.6, so this new feature could well be included too. How corresponding users are stored for comments should be refactored anyway. It's hurting for me that this feature is well implemented in Comments but could have a good use elsewhere too :-).

These are currently blocking us, pretty much everything else is done, so it would be good to make decisions here.

Developer
Aug 15, 2012 at 10:11 PM

We've found something strange regarding Comments. On the admin page (/Admin/Comments) the table that lists the comments now renders shapes. But: it also displays a shape above the table that is used for eg. blog posts (Content_SummaryAdmin) to edit/publish/unpublish, etc. the give content item. For comments it's completely unnecessary, but we could only remove it partially with placement.info configuration (by disabling Parts_Common_Metadata_SummaryAdmin and Parts_Contents_Publish_SummaryAdmin for Comment ContentType and SummaryAdmin DisplayType).
Please take a look at the fork and give us a hint on this if you can.

Aug 21, 2012 at 5:16 PM
Edited Aug 21, 2012 at 5:19 PM

Since both myself and Andy have been writing themes for Orchard a lot recently, we'd like to pitch in our opinions on how commenting could be improved. These suggestions are from a non-developer perspective, since we haven't looked into implementation at all.

  1. There are serious security issues present when posting comments. Users can post any HTML/CSS/Script they wish.

  2. The default mark-up for comments could be a lot more HTML5 semantic, since Orchard is built around HTML5 I think this would be of a high level of importance. Something like this would be what we're thinking for comments:

    <section id="comments">
    <h1>12 Comments...</h1>
    <article id="comment-1" class="comment">
    <header>
    <time datetime="UTC comment time" pubtime>Comment date/time would be here</time>
    <h3><a href="#" rel="external nofollow">Joe Bloggs</a> says...</h3>
    </header>
    </article>
    <!-- And so on... -->
    </section>

    The comment form mark-up would look something like this:

    <section id="respond">
    <h1>Comment...</h1>
    <form action="#">
    <fieldset>
    <label for="comment">Comment</label><textarea id="comment" name="comment">
    </textarea>
    <!-- And so on... -->
    </fieldset>
    </form>
    </section>

    In the comment form, we don't see the use for an <ol>.

  3. Comment threading is something that we'd really like to see. UX wise something the same as WordPress. Also comment pagination would be nice.

  4. Gravatar support.

  5. Maybe we could format comments using Markdown.

Coordinator
Aug 23, 2012 at 1:13 AM

I agree with this list, especially markdown, although this should probably be the same flavor setting we have everywhere else. Mixing threading with pagination promises to be loads of fun however.

I would like to add the ability to download the comments asynchronously, like Disqus does: on a product page for example, you don't want the comments to make the page slower to load, even if it has hundreds of comments.