Opened 16 years ago

Closed 15 years ago

Last modified 15 years ago

#8630 closed (fixed)

Improve the comments framework customizability

Reported by: Thejaswi Puthraya Owned by: Carl Meyer
Component: contrib.comments Version: dev
Severity: Keywords: comments, customization
Cc: dane.springmeyer@…, steingrd@…, David Larlet, halfdan@… Triage Stage: Accepted
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description

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.

Attachments (11)

8630.diff (1.2 KB ) - added by Thejaswi Puthraya 16 years ago.
git-patch against the latest trunk checkout
8630_r8782_alternative.diff (1.7 KB ) - added by Carl Meyer 16 years ago.
A patch to use get_comment_app in templatetags, so customization can work (but I actually think Thejaswi's patch is a better approach)
8630_r8782_svn.diff (1.2 KB ) - added by Carl Meyer 16 years ago.
Thejaswi's original patch, in svn diff style
8630_r8782_admin.diff (1.9 KB ) - added by Carl Meyer 16 years ago.
admin.py needs to not register the Comment model if it's been replaced with a custom model
__init__.py.patch (1.9 KB ) - added by amir 16 years ago.
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.
8630_r9084.diff (12.2 KB ) - added by Carl Meyer 15 years ago.
consolidated patch
8630_r9084.2.diff (12.2 KB ) - added by Carl Meyer 15 years ago.
consolidated patch, with docs
8630_r9084_with_tests.diff (15.8 KB ) - added by Carl Meyer 15 years ago.
consolidated patch with docs and tests
8630_new.diff (17.3 KB ) - added by Thejaswi Puthraya 15 years ago.
git-patch that has tests, docs and the patch. Slight change from Carljm's patch.
8630_r9776.diff (7.3 KB ) - added by Carl Meyer 15 years ago.
updated patch with minor typo fixes
8630_r9781.diff (17.2 KB ) - added by Carl Meyer 15 years ago.
this time with all the files included :-)

Download all attachments as: .zip

Change History (36)

by Thejaswi Puthraya, 16 years ago

Attachment: 8630.diff added

git-patch against the latest trunk checkout

comment:1 by Malcolm Tredinnick, 16 years ago

milestone: 1.0

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

in reply to:  1 comment:2 by Thejaswi Puthraya, 16 years ago

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 :(

comment:3 by Jacob, 16 years ago

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.

comment:4 by Carl Meyer, 16 years ago

Cc: carl@… added

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.

by Carl Meyer, 16 years ago

Attachment: 8630_r8782_alternative.diff added

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

by Carl Meyer, 16 years ago

Attachment: 8630_r8782_svn.diff added

Thejaswi's original patch, in svn diff style

comment:5 by Thejaswi Puthraya, 16 years ago

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 :)

in reply to:  5 comment:6 by Carl Meyer, 16 years ago

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.

by Carl Meyer, 16 years ago

Attachment: 8630_r8782_admin.diff added

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

comment:7 by Jannis Leidel, 16 years ago

Cc: Jannis Leidel added

by amir, 16 years ago

Attachment: __init__.py.patch added

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.

comment:8 by Carl Meyer, 16 years ago

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?)

comment:9 by Jannis Leidel, 16 years ago

Cc: Jannis Leidel removed
Needs documentation: set
Needs tests: set
Triage Stage: UnreviewedAccepted

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

comment:10 by Carl Meyer, 16 years ago

Owner: changed from nobody to Carl Meyer

by Carl Meyer, 15 years ago

Attachment: 8630_r9084.diff added

consolidated patch

by Carl Meyer, 15 years ago

Attachment: 8630_r9084.2.diff added

consolidated patch, with docs

comment:11 by Carl Meyer, 15 years ago

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.

comment:12 by Carl Meyer, 15 years ago

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...

by Carl Meyer, 15 years ago

Attachment: 8630_r9084_with_tests.diff added

consolidated patch with docs and tests

comment:13 by anonymous, 15 years ago

Cc: dane.springmeyer@… added

comment:14 by Carl Meyer, 15 years ago

#9562 was a duplicate.

comment:15 by steingrd <steingrd@…>, 15 years ago

Cc: steingrd@… added

by Thejaswi Puthraya, 15 years ago

Attachment: 8630_new.diff added

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

comment:16 by Thejaswi Puthraya, 15 years ago

Needs documentation: unset
Needs tests: unset

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

by Carl Meyer, 15 years ago

Attachment: 8630_r9776.diff added

updated patch with minor typo fixes

comment:17 by Carl Meyer, 15 years ago

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.

comment:18 by Carl Meyer, 15 years ago

Cc: carl@… removed

comment:19 by David Larlet, 15 years ago

Cc: David Larlet added

by Carl Meyer, 15 years ago

Attachment: 8630_r9781.diff added

this time with all the files included :-)

comment:20 by Jacob, 15 years ago

(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.

comment:21 by Jacob, 15 years ago

Resolution: fixed
Status: newclosed

(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.

comment:22 by halfdan, 15 years ago

Cc: halfdan@… added

comment:23 by joshvanwyck, 15 years ago

Resolution: fixed
Status: closedreopened

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

comment:24 by Carl Meyer, 15 years ago

Resolution: fixed
Status: reopenedclosed

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

in reply to:  23 comment:25 by Thejaswi Puthraya, 15 years ago

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

Note: See TracTickets for help on using tickets.
Back to Top