Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

PetrDlouhy
Copy link
Contributor

No description provided.

@claudep
Copy link
Member

claudep commented Oct 20, 2021

Would you be able to write a test for that?

@PetrDlouhy
Copy link
Contributor Author

@claudep OK, I will get to it.

@PetrDlouhy
Copy link
Contributor Author

@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
Copy link
Member

claudep commented Jul 8, 2022

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

author = models.ForeignKey(Author, on_delete=models.CASCADE)
headline = models.CharField(max_length=100)

def save(self, *args, **kwargs):
import pudb; pudb.set_trace()
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

oops :-)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry :-D Fixed.

@PetrDlouhy
Copy link
Contributor Author

I fixed the useless code and added some basic documentation.

content_type=ContentType.objects.get_for_model(
self.target_object,
for_concrete_model=COMMENTS_FOR_CONCRETE_MODEL,
),
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure if the new setting is the preferred way 🤔 What do you think about adding the for_concrete_model=True keyword argument to the get_comment_object() and get_comment_create_data()?

@PetrDlouhy
Copy link
Contributor Author

@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
Copy link
Contributor Author

@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 PetrDlouhy marked this pull request as draft August 2, 2022 13:46
@PetrDlouhy
Copy link
Contributor Author

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
Copy link

It's looks good now.

@PetrDlouhy
Copy link
Contributor Author

PetrDlouhy commented Oct 6, 2022

@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
Copy link

@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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants