Django

Code

Ticket #9958 (closed: fixed)

Opened 1 year ago

Last modified 1 year ago

Split contrib.comments CommentForm class to allow easier customization

Reported by: arne Assigned to: nobody
Milestone: 1.1 beta Component: django.contrib.comments
Version: SVN Keywords: comments, customization
Cc: carljm Triage Stage: Design decision needed
Has patch: 1 Needs documentation: 0
Needs tests: 0 Patch needs improvement: 0

Description

The CommentForm class in contrib/comments/forms.py is very monolithic and contains all spam-checking code and the application-specific code. Splitting the Form into a BaseCommentForm class which contains only the spam-checking code and a CommentForm class which only contains the application-specific code would allow much easier customization of the comments-app.

Using the hooks from #8630 and defining your own model and form (which for example don't include fields for email and url) would either need a copy of the whole form-class code including the modification (about 100 lines of code) or one must subclass the current CommentForm class and manually delete the fields (email and url in this example) before using the form.

The appended patch splits the current CommentForm class into two classes which would allow easier customization of the comments-app. When reviewing this ticket please also see #8630 for more details.

Attachments

comments.diff (5.8 kB) - added by arne on 01/04/09 23:45:45.
9958.diff (4.6 kB) - added by thejaswi_puthraya on 01/10/09 22:29:32.
git-patch against the latest checkout
9958_2.diff (8.0 kB) - added by thejaswi_puthraya on 01/11/09 07:00:03.
a patch that hopefully solves arne's and my problem
9958_2.2.diff (8.0 kB) - added by thejaswi_puthraya on 01/11/09 07:01:06.
a patch that hopefully solves arne's and my problem

Change History

01/04/09 23:45:45 changed by arne

  • attachment comments.diff added.

01/05/09 10:33:24 changed by carljm

  • cc set to carljm.
  • needs_better_patch changed.
  • needs_tests changed.
  • needs_docs changed.

01/10/09 22:28:59 changed by thejaswi_puthraya

  • stage changed from Unreviewed to Design decision needed.

I am a +1 on this feature because people might not want to have spam protection which might be redundant if they use an external spam detection facility.

I don't like the way the patch has been written because it gives a feeling to the user that a custom form should be inherited from BaseCommentForm?. So i have made another patch.

But I would like to hear what a developer has to say about this feature.

01/10/09 22:29:32 changed by thejaswi_puthraya

  • attachment 9958.diff added.

git-patch against the latest checkout

(follow-up: ↓ 4 ) 01/11/09 05:13:36 changed by arne

Using 9958.diff solves a different problem than my original patch comments.diff. With the approach in 9958.diff neither inheriting from BaseCommentForm nor inheriting from CommentForm allows for easy customization of the fields the form uses.

Please consider that not every projects needs a comment form with the fields "name", "email", "url" and "comment". Sometimes "name" and "comment" may be enough and 9958.diff gives no form-class from which I can inherit to cleanly build my own comment form with less fields than the original CommentForm. I feel like overriding __init__() and using del to delete form-fields is not a clean solution.

Using an approach like in comments.diff one can either subclass BaseCommentForm to customize the fields the form has and use all the existing Spam-protection logic or could use a completly other form (inheriting form forms.Form) to circumvent the Spam-protection as thejaswi_puthraya pointed out and the ability to customize the fields.

(in reply to: ↑ 3 ) 01/11/09 06:41:40 changed by thejaswi_puthraya

Replying to arne:

Using 9958.diff solves a different problem than my original patch comments.diff. With the approach in 9958.diff neither inheriting from BaseCommentForm nor inheriting from CommentForm allows for easy customization of the fields the form uses. Please consider that not every projects needs a comment form with the fields "name", "email", "url" and "comment". Sometimes "name" and "comment" may be enough and 9958.diff gives no form-class from which I can inherit to cleanly build my own comment form with less fields than the original CommentForm. I feel like overriding __init__() and using del to delete form-fields is not a clean solution.

Thank you for providing an insight into your problem.

Using an approach like in comments.diff one can either subclass BaseCommentForm to customize the fields the form has and use all the existing Spam-protection logic or could use a completly other form (inheriting form forms.Form) to circumvent the Spam-protection as thejaswi_puthraya pointed out and the ability to customize the fields.

I have seen a lot of requests in IRC from people who felt that the current honeypot mechanism provided by the comments was insufficient and/or was redundant with their home-grown/third-party solution. Hence I segregated the two forms to remove that redundancy.

Here are my observations regarding your patch: * I would prefer your patch provided it does not have the honeypot in the BaseCommentForm? because it forces the spam protection on users who don't require one. * It still does not remove the pain of say having a form that removes the email field and I am sure no other patch can remove that pain. It is best to draw a line somewhere and let the user take a small pain (one-time effort) to write a custom form.

My personal opinion is that spam protection should be optional. Let me come up with another patch that tackles mixes both your problem and mine.

Let's leave it to the devels and see what is the best for such a scenario.

01/11/09 06:59:37 changed by thejaswi_puthraya

Ok here is what is basically done. First there is a BaseCommentForm? that holds the most vital fields of the form ie content_type, object_pk, timestamp and security hash. Then a second form called MetaCommentForm? (that is a subclass of BaseCommentForm?)(please change the name of MetaCommentForm? if it sounds confusing) that holds the name, email, url etc fields. Then a third form called CommentForm? (a subclass of MetaCommentForm?) that holds the honeypot.

01/11/09 07:00:03 changed by thejaswi_puthraya

  • attachment 9958_2.diff added.

a patch that hopefully solves arne's and my problem

01/11/09 07:01:06 changed by thejaswi_puthraya

  • attachment 9958_2.2.diff added.

a patch that hopefully solves arne's and my problem

03/19/09 22:22:24 changed by thejaswi_puthraya

  • milestone set to 1.1 beta.

Is a fairly simple and backwards-compatible fix that could make into 1.1beta

03/19/09 22:22:37 changed by thejaswi_puthraya

  • has_patch set to 1.

03/21/09 08:45:31 changed by jacob

  • status changed from new to closed.
  • resolution set to fixed.

(In [10110]) Fixed #9958: split the CommentForm into a set of smaller forms. This for better encapsulation, but also so that it's easier for subclasses to get at the pieces they might need. Thanks to Thejaswi Puthraya.


Add/Change #9958 (Split contrib.comments CommentForm class to allow easier customization)




Change Properties
Action