search

django / django-contrib-comments

BSD 3-Clause "New" or "Revised" License
614 stars 196 forks source link

allow to make comments for non-abstract models by COMMENTS_FOR_CONCRETE_MODEL setting #177

Open PetrDlouhy opened 2 years ago

claudep commented 2 years ago

Would you be able to write a test for that?

PetrDlouhy commented 2 years ago

@claudep OK, I will get to it.

PetrDlouhy commented 2 years ago

@claudep I just bumped into this and realized that I forgot to finish it. Now I have written the tests. How can I let them run here on GitHub?

claudep commented 2 years ago

I would love to get a quick second review by someone else (maybe @felixxm ?). I think some documentation may still be needed.

PetrDlouhy commented 2 years ago

I fixed the useless code and added some basic documentation.

PetrDlouhy commented 2 years ago

@felixxm I have rewritten the code to use the for_concrete_model parameter. I had to use try-catch to handle overrides that doesn't count with the extra parameter such as: https://github.com/danirus/django-comments-xtd/blob/2ef74e3127f669293504fe88f46480456cca1809/django_comments_xtd/forms.py#L59

I will probably use the COMMENTS_FOR_CONCRETE_MODEL for django-comments-xtd implementation anyway, because there it has to be on several places across the application and I can't think of better way.

Another way to support this would be to have some mapping in settings like COMMENTS_ALLOWED_PROXY_MODELS which would allow to have different settings for every model. But I don't think that would be necessary - I think that having for_concrete_model=False everywhere would be good default except it would change behavior for current implementations.

PetrDlouhy commented 2 years ago

@felixxm I taught about it more, and I came up with new approach to my problem: #188 Having the model key mapping in settings solves my problem (using different key than PK) much better, than using proxy models (which is very hacky on the side of my application).

That said, this PR is contains little change to the core code and can solve different use-cases where using proxy models is needed.

PetrDlouhy commented 2 years ago

I have implemented functionality that I need in #188, however this might be helpful in different use cases so I converted it to draft. Anybody feel free to take this over.

blockchainGuru1018 commented 1 year ago

It's looks good now.

PetrDlouhy commented 1 year ago

@blockchainGuru1018 Could you please rather review #188. I converted this to draft in case someone wants to take this work over and adapt it for his/hers purposes. Overridable model keys are solving my use-case much better.

blockchainGuru1018 commented 1 year ago

@blockchainGuru1018 Could you please rather review #188. I converted this to draft in case someone wants to tak this work over and adapt it for his/hers purposes. Overridable model keys are solving my use-case much better.

Reviewed your PR @PetrDlouhy