Improve the comments framework customizability

[Thejaswi Puthraya]

Currently the comments framework allows to customize comments by requiring the three attributes get_model(), get_form() and get_form_target() in the custom comments app. But by hard-coding the get_model(), get_form() and get_form_target() in django.contrib.comments.__int__.py we are restricting the extensibility.

The patch attached allows easy customization. This is *NOT* a new feature, just hard-coded it by mistake while sending patches to Jacob.

[Thejaswi Puthraya]

git-patch against the latest trunk checkout

[Malcolm Tredinnick]

Yes, it is a feature. The comments work fine without this patch, so it's not a bug fix. It's adding new functionality.

[Thejaswi Puthraya]

Replying to mtredinnick:

Yes, it is a feature. The comments work fine without this patch, so it's not a bug fix. It's adding new functionality.

But we have no way to customize comments without the patch :(

[Jacob]

You're not understanding how those methods are intended to be called: an app that wants to provide custom comments provides its own app; you'd call, for example, get_comments_app().get_form() -- this would return COMMENTS_APP.get_form().

Either way, though, COMMENTS_APP is a hook designed for the future; the goal of comments in 1.0 is to *work*. Extensibility comes next.

[Carl Meyer]

In that case, the template tags in django.contrib.comments need fixing to use get_comments_app(). Currently the template tags are hardcoded to call comments.get_form() etc, which means there is no usable customization hook in contrib.comments.

[Carl Meyer]

A patch to use get_comment_app in templatetags, so customization can work (but I actually think Thejaswi's patch is a better approach)

[Carl Meyer]

Thejaswi's original patch, in svn diff style

[Thejaswi Puthraya]

Carljm,
I actually wanted to sneak this patch before 1.0 but Malcolm and Jacob have a valid point that comments currently work and the extensibility can be taken care of post-1.0. This patch might also require documenting the process of writing custom comments. So a patch just addresses the issue is not sufficient, docs are also vital.

Anyways, let this ticket rest till 1.0 is released :)

[Carl Meyer]

Replying to thejaswi_puthraya:

Anyways, let this ticket rest till 1.0 is released :)

Sure - I'm not advocating for this to happen pre-1.0. Just needed to do custom comments for my own project now anyway, so thought I might as well update things here while I was at it. I'm happy to help work on docs when the time comes.

[Carl Meyer]

admin.py needs to not register the Comment model if it's been replaced with a custom model

[amir]

Also removed the checks in get_comment_app() so it's possible to override only part of the functions and use the default for the rest.

[Carl Meyer]

I'm ready to write draft docs and put together the patch for this, whenever it gets blessed with Accepted (though there may be a design decision needed first?)

[Jannis Leidel]

That's a valid issue and needs to be adressed.

[Carl Meyer]

consolidated patch

[Carl Meyer]

consolidated patch, with docs

[Carl Meyer]

Ok, this patch is ready for a looking-over by someone who knows what they're doing. This is my first time trying to actually write any Django docs, so feedback and suggestions are welcome.

A couple other notes on decisions I made in the patch:

• I took Thejaswi's approach of modifying contrib.comments' get_model() etc functions to be smart about delegating to a custom COMMENTS_APP. To me, this seems cleanest, as no other code (ie the templatetags) has to duplicate the get_comment_app() song and dance, it all happens in one place.

• I also incorporated amir's suggestion to make all the customization functions optional, so you can customize what you like and leave the rest.

• No tests yet. I'm not clear where to go with this, as contrib.comments has no tests at all currently. Should I be writing tests for the whole app as part of this ticket? (Seems like maybe that should be its own ticket). Should I just introduce some simple tests for the customizability, and leave everything else untested?

Thoughts and feedback welcome.

[Carl Meyer]

Oops - of course the comments app has tests, they're just with the rest of the Django tests, not with the app. Off to add some tests for customization...

[Carl Meyer]

consolidated patch with docs and tests

[Carl Meyer]

#9562 was a duplicate.

[Thejaswi Puthraya]

git-patch that has tests, docs and the patch. Slight change from Carljm's patch.

[Thejaswi Puthraya]

The latest patch is based on carljm's patch with a few changes. Docs are courtesy of carljm. Thanks carljm.

[Carl Meyer]

updated patch with minor typo fixes

[Carl Meyer]

Thanks Thejaswi. It'd be nice if you could summarize your changes to the patch to make it easier to look over.

I attached an updated version that fixes a typo your latest patch introduced in contrib/comments/init.py in get_delete_url(), and uses forward slashes instead of dot notation to refer to file locations in docs/ref/contrib/comments/custom.txt.

[Carl Meyer]

this time with all the files included :-)

[Jacob]

(In [9889]) Refactored CommentForm.get_comment_object into a handful of separete methods to make it easier for subclasses to provide custom models and data. Refs #8630.

[Jacob]

(In [9890]) Fixed #8630: finished the custom comment app API that was left out of 1.0. This means it's now possible to override any of the models, forms, or views used by the comment app; see the new custom comment app docs for details and an example. Thanks to Thejaswi Puthraya for the original patch, and to carljm for docs and tests.

[joshvanwyck]

Greetings,

I'm trying to take advantage of the newest version of the django's customizable comments framework. I am following all the documentation when I try and implement a simple comment system that has an additional field. I tried initially with a BooleanField, but have since resorted to trying to simply follow the example from the documentation provided.

When I run it, I get the following error message:
Caught an exception while rendering: Cannot resolve keyword 'is_public' into field. Choices are: content_type, id, object_pk, site, title

I suspect that this is a bug or either mistaken documentation, as I'm assuming if I follow the example online character for character it should work.

A couple of things I've found by digging through the code. If we extend the BaseCommentAbstractModel, how can we receive the Comment model's fields? Why does line 84 of templatetags/comments.py contain is_public = true. This field will only exist if we have extended the Comment model but this isn't an option as we are supposed to extend the BaseCommentAbstractModel.

Also the unit testing doesn't seem to test the rendering which is where the problems seem to be occurring.

Maybe I'm completely off here, this is my first time posting any comments so if I have broken any posting rules please let me know.

Thanks for your work on this portion.

Josh

[Carl Meyer]

This is valid, but should get its own ticket. Created #10548.

[Thejaswi Puthraya]

Hi Josh,

Thanks for your comments. They have been invaluable and they have helped us locate a bug elsewhere.

Replying to joshvanwyck:

Greetings,

I'm trying to take advantage of the newest version of the django's customizable comments framework. I am following all the documentation when I try and implement a simple comment system that has an additional field. I tried initially with a BooleanField, but have since resorted to trying to simply follow the example from the documentation provided.

When I run it, I get the following error message:
Caught an exception while rendering: Cannot resolve keyword 'is_public' into field. Choices are: content_type, id, object_pk, site, title

I suspect that this is a bug or either mistaken documentation, as I'm assuming if I follow the example online character for character it should work.

Ideally you should be subclassing from the Comment Model. But if your requirements are different, then BaseCommentAbstractModel should do. This problem was fixed in [9891] but there is a small typo in that.

Will reopen that ticket and get it fixed. Also I will open a ticket for patching the documentation that makes it very clear.

A couple of things I've found by digging through the code. If we extend the BaseCommentAbstractModel, how can we receive the Comment model's fields? Why does line 84 of templatetags/comments.py contain is_public = true. This field will only exist if we have extended the Comment model but this isn't an option as we are supposed to extend the BaseCommentAbstractModel.

Also the unit testing doesn't seem to test the rendering which is where the problems seem to be occurring.

Maybe I'm completely off here, this is my first time posting any comments so if I have broken any posting rules please let me know.

Thanks for your work on this portion.

Josh