danirus / django-comments-xtd

A pluggable Django comments application with thread support, follow-up notifications, mail confirmation, like/dislike flags, moderation, a ReactJS plugin and Bootstrap 5.3.
https://django-comments-xtd.readthedocs.io
BSD 2-Clause "Simplified" License
594 stars 157 forks source link

refactor commentbox_props() into classmethod to allow better overriding #372

Closed PetrDlouhy closed 2 years ago

PetrDlouhy commented 2 years ago

I am using the commentbox_props() function in my app to create my own comment view. I wanted to restrict the queryset in this function to show correct number of comments (not counting comments private to user).

With this slight change of the commentbox_props() function into classmethods I am able to do this without need of copying of the entire code of the function.

PetrDlouhy commented 2 years ago

BTW If you want to see the diff without huge amount of noise caused by indentation change, use the "Hide whitespace" function of GitHub: Snímek obrazovky_2022-07-07_15-36-51

danirus commented 2 years ago

Thank you @PetrDlouhy.

Maybe you want to consider sending the same changes (and modifying the existing tests for the frontend.py module) to django-comments-ink. It's still under development, but at some point it will supersede django-comments-xtd.

PetrDlouhy commented 2 years ago

@danirus Thanks for merging and improving my PR.

I will look at the -ink repository. Are somewhere summarized the main changes and improvements? For www.blenderkit.com we ended up using our custom JavaScript frontend written in Svelte/Vite (which translates to pure JavaScript without any additional bindings needed). We probably will not be rewriting that to use django-comments-ink's JavaScript frontend, but I can send you the code if you are interested (or make a separate pluggable app, if I have some spare tiime).

Should I re-post my PRs from here also to django-comments-ink, or you will transfer them there? Or should I start posting them only there?

I polished #342, so please merge that, I will look at the other rests later.

danirus commented 2 years ago

Thanks for the offer of the Svelte plugin, but I have very little spare time. It would nice if you made it open source and people could install it with npm or yarn. :-)

In django-comments-ink the major differences with -xtd are that:

  1. It does not depend on any JavaScript framework. The plugin is vanilla JavaScript.
  2. There is a new CommentReaction model (and ObjectReaction) that is customizable.
  3. There is no tree of comments but a rather plain QuerySet that is rendered as a tree. The render_xtdcomment_tree is no longer available, and it's been replaced by render_inkcomment_list tag.
  4. Comments can be paginated.
  5. A lot of tests.

I'm adding more features at the speed of a snail.

It would be great if you could bring there the changes you suggested about using UUIDs for comment PKs. I remember that it had little explanation, it looked like a hack. If you don't want to spend time writing literature, do you have an open public repository using django-comments and django-comments-xtd with UUIDs to illustrate the case?

I will look into #342 at the end of the week. Thanks for your contributions!

PetrDlouhy commented 2 years ago

@denirus Using UUIDs as PKs itself is almost no problem with current implementation. The only change needed are few forgotten id instead of pks, which I fixed in #342.

What is more challenging is setting comment URLs to non-PK field, which is needed if you want to add UUID to current model which is using IDs and don't want to rewrite half of the application and end up in migration hell. I did use proxy models to achieve that. I did few hacks on the side of my application, and few non-hack modification on the side of django-comments-xtd.

I don't have public implementation, but I can transform the code into documentation or sample application.

I might also try to explore more another solution - to have settings with field mappings. Something like:

COMMENTS_ID_OVERRIDES = {
   'Article': 'my_uuid_field',
   'Item': 'another_id_field',
}

But I must say, that I did dive into that before, and I was not able to implement it. One of the reason is, that it did require many modifications also at the side of django-comments-contrib.

PetrDlouhy commented 2 years ago

@danirus I have completed that taught in https://github.com/django/django-contrib-comments/pull/188 and #374. I think, that this solution is much cleaner and easier for implementors. But it relies on the changes being accepted in django-contrib-comments. So I will first have to finish the discussion and work on PR there and then I can polish the PR here.