Opened 7 years ago

Closed 6 years ago

Last modified 6 years ago

#8630 closed (fixed)

Improve the comments framework customizability

Reported by: thejaswi_puthraya Owned by: carljm
Component: contrib.comments Version: master
Severity: Keywords: comments, customization
Cc: dane.springmeyer@…, steingrd@…, david, halfdan@… Triage Stage: Accepted
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: UI/UX:

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 7 years ago.
git-patch against the latest trunk checkout
8630_r8782_alternative.diff (1.7 KB) - added by carljm 7 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 carljm 7 years ago.
Thejaswi's original patch, in svn diff style
8630_r8782_admin.diff (1.9 KB) - added by carljm 7 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 7 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 carljm 7 years ago.
consolidated patch
8630_r9084.2.diff (12.2 KB) - added by carljm 7 years ago.
consolidated patch, with docs
8630_r9084_with_tests.diff (15.8 KB) - added by carljm 7 years ago.
consolidated patch with docs and tests
8630_new.diff (17.3 KB) - added by thejaswi_puthraya 6 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 carljm 6 years ago.
updated patch with minor typo fixes
8630_r9781.diff (17.2 KB) - added by carljm 6 years ago.
this time with all the files included :-)

Download all attachments as: .zip

Change History (36)

Changed 7 years ago by thejaswi_puthraya

git-patch against the latest trunk checkout

comment:1 follow-up: Changed 7 years ago by mtredinnick

  • milestone 1.0 deleted
  • Needs documentation unset
  • Needs tests unset
  • Patch needs improvement unset

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

comment:2 in reply to: ↑ 1 Changed 7 years ago by 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 :(

comment:3 Changed 7 years ago by 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.

comment:4 Changed 7 years ago by carljm

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

Changed 7 years ago by carljm

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

Changed 7 years ago by carljm

Thejaswi's original patch, in svn diff style

comment:5 follow-up: Changed 7 years ago by 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 :)

comment:6 in reply to: ↑ 5 Changed 7 years ago by carljm

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.

Changed 7 years ago by carljm

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

comment:7 Changed 7 years ago by jezdez

  • Cc jezdez added

Changed 7 years ago by 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.

comment:8 Changed 7 years ago by carljm

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 Changed 7 years ago by jezdez

  • Cc jezdez removed
  • Needs documentation set
  • Needs tests set
  • Triage Stage changed from Unreviewed to Accepted

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

comment:10 Changed 7 years ago by carljm

  • Owner changed from nobody to carljm

Changed 7 years ago by carljm

consolidated patch

Changed 7 years ago by carljm

consolidated patch, with docs

comment:11 Changed 7 years ago by carljm

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 Changed 7 years ago by carljm

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

Changed 7 years ago by carljm

consolidated patch with docs and tests

comment:13 Changed 7 years ago by anonymous

  • Cc dane.springmeyer@… added

comment:14 Changed 7 years ago by carljm

#9562 was a duplicate.

comment:15 Changed 7 years ago by steingrd <steingrd@…>

  • Cc steingrd@… added

Changed 6 years ago by thejaswi_puthraya

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

comment:16 Changed 6 years ago by thejaswi_puthraya

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

Changed 6 years ago by carljm

updated patch with minor typo fixes

comment:17 Changed 6 years ago by carljm

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 Changed 6 years ago by carljm

  • Cc carl@… removed

comment:19 Changed 6 years ago by david

  • Cc david added

Changed 6 years ago by carljm

this time with all the files included :-)

comment:20 Changed 6 years ago by 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.

comment:21 Changed 6 years ago by jacob

  • Resolution set to fixed
  • Status changed from new to closed

(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 Changed 6 years ago by halfdan

  • Cc halfdan@… added

comment:23 follow-up: Changed 6 years ago by joshvanwyck

  • Resolution fixed deleted
  • Status changed from closed to reopened

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 Changed 6 years ago by carljm

  • Resolution set to fixed
  • Status changed from reopened to closed

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

comment:25 in reply to: ↑ 23 Changed 6 years ago by 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

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