ellmetha / django-machina

A Django forum engine for building powerful community driven websites.
https://django-machina.readthedocs.org
BSD 3-Clause "New" or "Revised" License
590 stars 126 forks source link

get_absolute_url for topic and posts #219

Closed BoPeng closed 1 year ago

BoPeng commented 3 years ago

When I send notification to a poster and I would like to include a link to the post itself. However, there is no get_absolute_url defined for the Topic or Post models. Is there any reason they are not defined?

https://github.com/ellmetha/django-machina/blob/421b7da7d19087e143da5ab165e22ca43251bb28/machina/apps/forum_conversation/urls.py#L36-L41

looks like a good url for Topic, but there is no similar view_post url.

BoPeng commented 3 years ago
    def get_absolute_url(self):
        return reverse(
            "forum_conversation:topic",
            kwargs={
                "slug": self.slug,
                'pk': self.pk,
                'forum_slug': self.forum.slug,
                'forum_pk': self.forum.pk
            })

should work for Topic.

franga2000 commented 3 years ago

Not the maintainer, but I remember those methods were removed around 2017, not sure if a reason was mentioned. I added them back in my fork, so feel free to cherry-pick this commit if you need them: https://github.com/ellmetha/django-machina/commit/905f23a1f2a4eb64b282191778522fd83f9ca7d7.

Edit: 2016, actually: changelog.

ellmetha commented 3 years ago

There is no post_view URL (although it could be possible to add one). If you need to link to a post specifically you can make use of the post GET parameter in a topic URL (example). It might definitely be nice to simplify this process in a future release. 😉

Regarding get_absolute_url, it's really related to a personal preference: models shouldn't define presentation-related things like URLs in my opinion. But yeah I agree that this goes against the general practices in Django. I can properly be convinced to reintroduce those if someone wants to take the time to submit a PR. 🙂 Though I would also like to point out the existence of the ABSOLUTE_URL_OVERRIDES setting that allows any project to define its own set of absolute URLs on a per-model basis.

BoPeng commented 3 years ago

https://github.com/ellmetha/django-machina/commit/905f23a1f2a4eb64b282191778522fd83f9ca7d7 might not apply cleanly but all needed changes appear to be there.

I really do not understand why get_absolute_url() was removed because it provides a natural Django way to refer to the forum, topic, or post without the long reverse(slug, pk, slug, pk) calls. Yes it means "presentation-related things in model", it means server side rendering and might not work well with RESTful or SPA, but as a Django app it is natural to provide them and leave it to users on how to use them (and ABSOLUTE_URL_OVERRIDES is one way to override it).

I have added these to my extended Topic class and might submit a PR when I get some time. Adding get_absolute_url is trivial but it takes more time to use it consistently throughout the project.

ellmetha commented 3 years ago

I really do not understand why get_absolute_url() was removed because it provides a natural Django way to refer to the forum, topic, or post without the long reverse(slug, pk, slug, pk) calls. Yes it means "presentation-related things in model", it means server side rendering and might not work well with RESTful or SPA, but as a Django app it is natural to provide them and leave it to users on how to use them (and ABSOLUTE_URL_OVERRIDES is one way to override it).

Well, I think I explicitly stated in my previous message why it was removed: to improve separation of concern. 🙂 There is no need to debate whether or not having this method defined in models is a good or bad practice in general: I already stated that I was open to see it reintroduced.